RESOLVED FIXED 101303
Fixed position elements that are out of view still end up forcing non-threaded scrolling 
https://bugs.webkit.org/show_bug.cgi?id=101303
Summary Fixed position elements that are out of view still end up forcing non-threade...
Beth Dakin
Reported 2012-11-05 22:22:57 PST
http://trac.webkit.org/changeset/133536 made it so that pages with position:fixed elements can scroll on the scrolling thread. However, pages with out-of-viewport fixed position elements are still failing to scroll on the scrolling thread because we don't create layers for the out-of-view elements. This issues is tracked by these tests: platform/mac/tiled-drawing/fixed/fixed-position-out-of-view.html platform/mac/tiled-drawing/fixed/fixed-position-out-of-view-negative-zindex.html If you comment out these lines in RenderLayerCompositor::requiresCompositingForPosition(), then we do create the layers and the bug goes away: // Fixed position elements that are invisible in the current view don't get their own layer. FrameView* frameView = m_renderView->frameView(); if (frameView && !layer->absoluteBoundingBox().intersects(IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()))) return false;
Attachments
Patch (3.96 KB, patch)
2012-11-06 13:49 PST, Beth Dakin 		simon.fraser: review-
DetailsFormatted DiffDiff
Patch (7.57 KB, patch)
2012-11-06 17:29 PST, Beth Dakin 		eflews.bot: commit-queue-
DetailsFormatted DiffDiff
Patch (10.03 KB, patch)
2012-11-07 11:38 PST, Beth Dakin 		simon.fraser: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2012-11-05 22:23:46 PST
<rdar://problem/12642222>
Beth Dakin
Comment 2 2012-11-06 13:49:05 PST
Created attachment 172642 [details] Patch Here's one simple way to fix this bug. Simon, I am wondering if you think this is okay or if you have another fix in mind?
Simon Fraser (smfr)
Comment 3 2012-11-06 14:04:22 PST
Comment on attachment 172642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172642&action=review > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:279 > + bool layerIsInsideViewport = layer->absoluteBoundingBox().intersects(IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize())); Checking absoluteBoundingBox() isn't sufficient: you can have a 0x0 fixedpos with an abspos child. Is hasNonLayerFixedObjects() called after every layout, so it can update at the right time? I also think this should share some code with RenderLayerCompositor::requiresCompositingForPosition(), since if that's correct, we should never have a visible fixedpos element that is visible.
Beth Dakin
Comment 4 2012-11-06 14:58:38 PST
(In reply to comment #3) > (From update of attachment 172642 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172642&action=review > > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:279 > > + bool layerIsInsideViewport = layer->absoluteBoundingBox().intersects(IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize())); > Ah, yes, good call. > Checking absoluteBoundingBox() isn't sufficient: you can have a 0x0 fixedpos with an abspos child. > > Is hasNonLayerFixedObjects() called after every layout, so it can update at the right time? > Yes. > I also think this should share some code with RenderLayerCompositor::requiresCompositingForPosition(), since if that's correct, we should never have a visible fixedpos element that is visible. Yes, I will look into that.
Beth Dakin
Comment 5 2012-11-06 17:29:53 PST
Created attachment 172682 [details] Patch
Simon Fraser (smfr)
Comment 6 2012-11-06 17:39:33 PST
Comment on attachment 172682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172682&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:1937 > +bool RenderLayerCompositor::layerIsOutsideView(const RenderLayer* layer) const > +{ > + IntRect boundingBox = layer->absoluteBoundingBox(); > + return !boundingBox.isEmpty() && !layerIsVisibleInView(layer); > +} > + > +bool RenderLayerCompositor::layerIsVisibleInView(const RenderLayer* layer) const > +{ > + // FIXME: This test should be much more sophisticated and consider descendants. > + // https://bugs.webkit.org/show_bug.cgi?id=98144 > + FrameView* frameView = m_renderView->frameView(); > + return frameView && layer->absoluteBoundingBox().intersects(IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize())); > +} I'm confused about why we need both of these?
EFL EWS Bot
Comment 7 2012-11-06 19:30:14 PST
Comment on attachment 172682 [details] Patch Attachment 172682 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14757235
Beth Dakin
Comment 8 2012-11-07 11:38:52 PST
Created attachment 172850 [details] Patch
Beth Dakin
Comment 9 2012-11-07 13:42:45 PST
My most recent patch resolved https://bugs.webkit.org/show_bug.cgi?id=98144 as well.
Simon Fraser (smfr)
Comment 10 2012-11-07 13:56:27 PST
Comment on attachment 172850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172850&action=review > Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:95 > + virtual bool hasNonLayerFixedObjects(FrameView*) const { return false; } I think this function name is wrong. I think it should be hasVisibleSlowRepaintFixedObjects() or something. > LayoutTests/platform/mac/tiled-drawing/fixed/absolute-inside-out-of-view-fixed.html:40 > + if (window.internals) { > + document.getElementById('results').innerText = internals.scrollingStateTreeAsText(document); > + testRunner.notifyDone(); I think it would also be good for this test to dump the layer tree, since you're changing compositing behavior. Or add another test under compositing/ that does this, and isn't tied to tiled drawing.
Beth Dakin
Comment 11 2012-11-07 14:28:19 PST
(In reply to comment #10) > (From update of attachment 172850 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172850&action=review > > > Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:95 > > + virtual bool hasNonLayerFixedObjects(FrameView*) const { return false; } > > I think this function name is wrong. I think it should be hasVisibleSlowRepaintFixedObjects() or something. > > > LayoutTests/platform/mac/tiled-drawing/fixed/absolute-inside-out-of-view-fixed.html:40 > > + if (window.internals) { > > + document.getElementById('results').innerText = internals.scrollingStateTreeAsText(document); > > + testRunner.notifyDone(); > > I think it would also be good for this test to dump the layer tree, since you're changing compositing behavior. Or add another test under compositing/ that does this, and isn't tied to tiled drawing. Thanks, Simon! I addressed both of these things. http://trac.webkit.org/changeset/133807
Beth Dakin
Comment 12 2012-11-07 14:29:03 PST
*** Bug 98144 has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 13 2012-11-16 13:44:53 PST
This has caused bug 102144.
Xianzhu Wang
Comment 14 2012-11-16 16:27:23 PST
*** Bug 98144 has been marked as a duplicate of this bug. ***
John Knottenbelt
Comment 15 2012-11-21 10:07:31 PST
I noticed that this patch has also introduced an assertion failure while running the ACID3 CSS tests: http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r135405%20%283886%29/results.html ( Also reported in https://bugs.webkit.org/show_bug.cgi?id=102599 ) I tried the test without this patch reverted and it worked fine. Any ideas on how to fix much appreciated.
Alexey Proskuryakov
Comment 16 2012-12-06 10:55:51 PST
This has also caused bug 104276.
Alexey Proskuryakov
Comment 17 2012-12-06 10:59:07 PST
Beth, will you have a chance to look into the regressions soon?
Beth Dakin
Comment 18 2012-12-06 11:08:45 PST
(In reply to comment #17) > Beth, will you have a chance to look into the regressions soon? I have been working on a reduction for one of them this yes. So, yes.
Beth Dakin
Comment 19 2012-12-06 11:10:11 PST
(In reply to comment #18) > (In reply to comment #17) > > Beth, will you have a chance to look into the regressions soon? > > I have been working on a reduction for one of them this yes. So, yes. *This week.
Note You need to log in before you can comment on or make changes to this bug.