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;
<rdar://problem/12642222>
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?
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.
(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.
Created attachment 172682 [details] Patch
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?
Comment on attachment 172682 [details] Patch Attachment 172682 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14757235
Created attachment 172850 [details] Patch
My most recent patch resolved https://bugs.webkit.org/show_bug.cgi?id=98144 as well.
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.
(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
*** Bug 98144 has been marked as a duplicate of this bug. ***
This has caused bug 102144.
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.
This has also caused bug 104276.
Beth, will you have a chance to look into the regressions soon?
(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.
(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.