The check against creating composition layers for invisible fixed positioned elements is too aggressive in that it does not consider descendants of the fixed positioned element that may be visible even though the element itself is out of view. Minimal test case follows. Here the visibility check fails because the fixed position element has a zero layout size, but an absolute positioned descendant is still visible. <!DOCTYPE html> <html> <head> <style> .fixed { position: fixed; } .absolute { position: absolute; top: 50px; left: 50px; } .box { border: solid thin blue; width: 40px; height: 40px; } body { height: 1000px; } </style> </head> <body> <div class="fixed"> <div class="absolute box"></div> </div> </body> </html> See https://www.gov.uk/organise-fete-street-party/telling-your-local-council for another example.
Created attachment 166666 [details] Patch
*** Bug 96584 has been marked as a duplicate of this bug. ***
Thanks Sami. The patch looks good to me.
Comment on attachment 166666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166666&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:1859 > + IntRect layerBounds = calculateCompositedBounds(layer, rootRenderLayer()); calculateCompositedBounds is doing a subtree walk, which could be expensive. > LayoutTests/compositing/layer-creation/fixed-position-absolute-descendant.html:30 > + window.internals.settings.setEnableCompositingForFixedPosition(true); > + window.internals.settings.setFixedPositionCreatesStackingContext(true); Are these both correctly reset by InternalSettings between tests?
Thanks Simon. (In reply to comment #4) > (From update of attachment 166666 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166666&action=review > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1859 > > + IntRect layerBounds = calculateCompositedBounds(layer, rootRenderLayer()); > > calculateCompositedBounds is doing a subtree walk, which could be expensive. Good point. I originally thought about inverting this so that each layer would walk up to see if its fixed by an ancestor, but the rest of the code (e.g. isFixedPositionedByAncestor) assumes that the fixed element is composited. I've now changed this to only walk the subtree if the fixed element itself is invisible. This should reduce the number of walks. > > LayoutTests/compositing/layer-creation/fixed-position-absolute-descendant.html:30 > > + window.internals.settings.setEnableCompositingForFixedPosition(true); > > + window.internals.settings.setFixedPositionCreatesStackingContext(true); > > Are these both correctly reset by InternalSettings between tests? Well spotted, the former wasn't. It's already being used in compositing/layer-creation/fixed-position-out-of-view.html.
Created attachment 167090 [details] Patch Only walk subtree if necessary. Restore fixed position composition flag between tests. PTAL?
Comment on attachment 167090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167090&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:1861 > + if (FrameView* frameView = m_renderView->frameView()) { > + IntRect viewBounds = IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()); > + if (!layer->absoluteBoundingBox().intersects(viewBounds) && !calculateCompositedBounds(layer, rootRenderLayer()).intersects(viewBounds)) > + return false; > + } This could be further optimized by having a version of of calculateCompositedBounds() that is like compositedExtentIntersectsRect() that you can early-return from as soon as you find something inside the viewport. But no need to do that now.
Comment on attachment 167090 [details] Patch Clearing flags on attachment: 167090 Committed r130396: <http://trac.webkit.org/changeset/130396>
All reviewed patches have been landed. Closing bug.
Four tests are crashing on the debug Lion and Mountain Lion bots are crashing after this commit. http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r130396%20(1853)/results.html http/tests/misc/acid3.html crash log fast/repaint/fixed-in-page-scale.html crash log fast/repaint/fixed-right-bottom-in-page-scale.html crash log fast/repaint/fixed-right-in-page-scale.html
Re-opened since this is blocked by bug 98421
"Crashes" were RenderGeometryMap assertions: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x000000010ed2ed0e WebCore::RenderGeometryMap::absoluteRect(WebCore::FloatRect const&) const + 686 (RenderGeometryMap.cpp:85) 1 com.apple.WebCore 0x000000010ed7bb3e WebCore::RenderLayerCompositor::addToOverlapMap(WebCore::RenderLayerCompositor::OverlapMap&, WebCore::RenderLayer*, WebCore::IntRect&, bool&) + 174 (RenderLayerCompositor.cpp:651) 2 com.apple.WebCore 0x000000010ed79a30 WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::RenderLayerCompositor::OverlapMap*, WebCore::CompositingState&, bool&, bool&) + 1616 (RenderLayerCompositor.cpp:836) 3 com.apple.WebCore 0x000000010ed7990d WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::RenderLayerCompositor::OverlapMap*, WebCore::CompositingState&, bool&, bool&) + 1325 (RenderLayerCompositor.cpp:813) 4 com.apple.WebCore 0x000000010ed7990d WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::RenderLayerCompositor::OverlapMap*, WebCore::CompositingState&, bool&, bool&) + 1325 (RenderLayerCompositor.cpp:813) 5 com.apple.WebCore 0x000000010ed7990d WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::RenderLayerCompositor::OverlapMap*, WebCore::CompositingState&, bool&, bool&) + 1325 (RenderLayerCompositor.cpp:813) 6 com.apple.WebCore 0x000000010ed78ea3 WebCore::RenderLayerCompositor::updateCompositingLayers(WebCore::CompositingUpdateType, WebCore::RenderLayer*) + 675 (RenderLayerCompositor.cpp:405) 7 com.apple.WebCore 0x000000010e174885 WebCore::FrameView::updateFixedElementsAfterScrolling() + 117 (FrameView.cpp:1853) 8 com.apple.WebCore 0x000000010efa51f2 WebCore::ScrollView::scrollTo(WebCore::IntSize const&) + 194 (ScrollView.cpp:387) 9 com.apple.WebCore 0x000000010e176eee WebCore::FrameView::scrollTo(WebCore::IntSize const&) + 78 (FrameView.cpp:2729) 10 com.apple.WebCore 0x000000010efa50e7 WebCore::ScrollView::setScrollOffset(WebCore::IntPoint const&) + 1063 (ScrollView.cpp:366) 11 com.apple.WebCore 0x000000010efa511f non-virtual thunk to WebCore::ScrollView::setScrollOffset(WebCore::IntPoint const&) + 47 12 com.apple.WebCore 0x000000010ef7c240 WebCore::ScrollableArea::scrollPositionChanged(WebCore::IntPoint const&) + 96 (ScrollableArea.cpp:149) 13 com.apple.WebCore 0x000000010ef7c521 WebCore::ScrollableArea::setScrollOffsetFromAnimation(WebCore::IntPoint const&) + 81 (ScrollableArea.cpp:193) 14 com.apple.WebCore 0x000000010ef7e42b WebCore::ScrollAnimator::notifyPositionChanged() + 59 (ScrollAnimator.cpp:150) 15 com.apple.WebCore 0x000000010ef829e9 WebCore::ScrollAnimatorMac::notifyPositionChanged() + 41 (ScrollAnimatorMac.mm:738) 16 com.apple.WebCore 0x000000010ef82532 WebCore::ScrollAnimatorMac::immediateScrollTo(WebCore::FloatPoint const&) + 210 (ScrollAnimatorMac.mm:717) 17 com.apple.WebCore 0x000000010ef82453 WebCore::ScrollAnimatorMac::scrollToOffsetWithoutAnimation(WebCore::FloatPoint const&) + 67 (ScrollAnimatorMac.mm:693) 18 com.apple.WebCore 0x000000010ef7c08c WebCore::ScrollableArea::scrollToOffsetWithoutAnimation(WebCore::FloatPoint const&) + 60 (ScrollableArea.cpp:127) 19 com.apple.WebCore 0x000000010efa3cd0 WebCore::ScrollView::updateScrollbars(WebCore::IntSize const&) + 5760 (ScrollView.cpp:624) 20 com.apple.WebCore 0x000000010efa5839 WebCore::ScrollView::setScrollPosition(WebCore::IntPoint const&) + 233 (ScrollView.cpp:421) 21 com.apple.WebCore 0x000000010e17433b WebCore::FrameView::setScrollPosition(WebCore::IntPoint const&) + 171 (FrameView.cpp:1763) 22 com.apple.WebCore 0x000000010dfe40ac WebCore::DOMWindow::scrollTo(int, int) const + 236 (DOMWindow.cpp:1417) 23 com.apple.WebCore 0x000000010e623b87 WebCore::jsDOMWindowPrototypeFunctionScrollTo(JSC::ExecState*) + 679 (JSDOMWindow.cpp:12663) 24 ??? 0x000038298b601265 0 + 61751083143781 25 com.apple.JavaScriptCore 0x000000010cdd8cb4 JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*) + 84 (JITCode.h:134) 26 com.apple.JavaScriptCore 0x000000010cdd5a42 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1746 (Interpreter.cpp:961) 27 com.apple.JavaScriptCore 0x000000010cc80d42 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 306 (CallData.cpp:39) 28 com.apple.WebCore 0x000000010e523452 WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 146 (JSMainThreadExecState.h:56) 29 com.apple.WebCore 0x000000010e65f186 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 1238 (JSEventListener.cpp:126) 30 com.apple.WebCore 0x000000010e08fa67 WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul>&) + 359 (EventTarget.cpp:206) 31 com.apple.WebCore 0x000000010e08f8c5 WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 325 (EventTarget.cpp:174) 32 com.apple.WebCore 0x000000010dfde160 WebCore::DOMWindow::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, WTF::PassRefPtr<WebCore::EventTarget>) + 272 (DOMWindow.cpp:1657) 33 com.apple.WebCore 0x000000010dfe5108 WebCore::DOMWindow::dispatchLoadEvent() + 296 (DOMWindow.cpp:1631) 34 com.apple.WebCore 0x000000010de3739f WebCore::Document::dispatchWindowLoadEvent() + 143 (Document.cpp:3708) 35 com.apple.WebCore 0x000000010de34d40 WebCore::Document::implicitClose() + 480 (Document.cpp:2493) 36 com.apple.WebCore 0x000000010e1467bb WebCore::FrameLoader::checkCallImplicitClose() + 155 (FrameLoader.cpp:812) 37 com.apple.WebCore 0x000000010e146483 WebCore::FrameLoader::checkCompleted() + 323 (FrameLoader.cpp:756) 38 com.apple.WebCore 0x000000010e146625 WebCore::FrameLoader::loadDone() + 21 (FrameLoader.cpp:701) 39 com.apple.WebCore 0x000000010db934b2 WebCore::CachedResourceLoader::loadDone() + 82 (CachedResourceLoader.cpp:680) 40 com.apple.WebCore 0x000000010f137734 WebCore::SubresourceLoader::releaseResources() + 180 (SubresourceLoader.cpp:342) 41 com.apple.WebCore 0x000000010ef079c9 WebCore::ResourceLoader::didFinishLoading(double) + 73 (ResourceLoader.cpp:303) 42 com.apple.WebCore 0x000000010f137345 WebCore::SubresourceLoader::didFinishLoading(double) + 581 (SubresourceLoader.cpp:302) 43 com.apple.WebCore 0x000000010ef081a5 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) + 53 (ResourceLoader.cpp:442) 44 com.apple.WebCore 0x000000010ef04dba -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] + 186 (ResourceHandleMac.mm:861)
Thought of a simpler change: --- a/Source/WebCore/rendering/RenderLayerCompositor.cpp +++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp @@ -1885,7 +1885,8 @@ bool RenderLayerCompositor::requiresCompositingForPosition(RenderObject* rendere // 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()))) + if (frameView && !layer->firstChild() + && !layer->absoluteBoundingBox().intersects(IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()))) return false; return true; It only skips compositing fixed pos elements that don't have sublayers. This is not complete but fixes the issue and can still reduce full page layers in cases like techcrunch.com. What do you think?
Created attachment 170698 [details] Patch
Comment on attachment 170698 [details] Patch Attachment 170698 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14555811 New failing tests: compositing/layer-creation/fixed-position-absolute-descendant.html
Created attachment 170772 [details] Patch
Comment on attachment 170772 [details] Patch Attachment 170772 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14546915 New failing tests: compositing/geometry/fixed-position-transform-composited-page-scale-down.html
Created attachment 170955 [details] Patch
Comment on attachment 170955 [details] Patch Attachment 170955 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14615113 New failing tests: compositing/geometry/fixed-position-transform-composited-page-scale-down.html
Created attachment 171000 [details] Try to fix layout test break
@skyostil, @Simon, @jamesr, what do you think about the simplified version?
Comment on attachment 171000 [details] Try to fix layout test break View in context: https://bugs.webkit.org/attachment.cgi?id=171000&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:1910 > + if (frameView && !layer->firstChild() > + && !layer->absoluteBoundingBox().intersects(IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()))) I think this change will cause us to miss this optimization too often. All it would take is something that has non-static position, or opacity, or overflow inside the fixed thting.
(In reply to comment #22) > (From update of attachment 171000 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171000&action=review > > I think this change will cause us to miss this optimization too often. All it would take is something that has non-static position, or opacity, or overflow inside the fixed thting. @skyostil, would you continue to fix the failures? Or I'd take over this bug. Another choice is to workaround the bug first, then optimize it. If agreed, I'll create another bug for the workaround and leave this bug open.
(In reply to comment #23) > @skyostil, would you continue to fix the failures? Or I'd take over this bug. I'm still wondering if there's a different way to avoid the explosion of layers from inconveniently z-ordered fixed position elements. The workarounds we've tried so far have either not covered all the cases or have had unfortunate side effects (e.g., an out-of-view fixed pos forces us to slow-scroll). I'm not quite sure what the alternative scheme would look like, exactly.
This should be resolved now by http://trac.webkit.org/changeset/133807 *** This bug has been marked as a duplicate of bug 101303 ***
Though the bug itself has been resolved in http://trac.webkit.org/changeset/133807, we still the a test to avoid regressions. Reopen this bug to add the layout test and restore enableCompositingForFixedPosition between tests.
Created attachment 174762 [details] Patch
I did add some tests for this: LayoutTests/compositing/absolute-inside-out-of-view-fixed.html LayoutTests/platform/mac/tiled-drawing/fixed/absolute-inside-out-of-view-fixed.html Is the third test necessary?
(In reply to comment #28) > I did add some tests for this: > > LayoutTests/compositing/absolute-inside-out-of-view-fixed.html > LayoutTests/platform/mac/tiled-drawing/fixed/absolute-inside-out-of-view-fixed.html > > Is the third test necessary? Oh, I didn't see them in the latest patch of the bug and overlooked the added test in the landed patch set. *** This bug has been marked as a duplicate of bug 101303 ***