Bug 98144

Summary:

Layout test for "Fixed position visibility check does not consider descendants"

Product:

WebKit

Reporter:

Sami Kyöstilä <skyostil>

Component:

Layout and Rendering

Assignee:

Xianzhu Wang <wangxianzhu>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

bdakin, dglazkov, eric, jamesr, jer.noble, simon.fraser, wangxianzhu, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

https://www.gov.uk/organise-fete-street-party/telling-your-local-council

Bug Depends on:

98421

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Try to fix layout test break	none
Patch	none

Sami Kyöstilä

Reported 2012-10-02 04:33:08 PDT

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.

Attachments
Patch (7.30 KB, patch) 2012-10-02 04:37 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (9.53 KB, patch) 2012-10-04 05:55 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (7.78 KB, patch) 2012-10-25 11:42 PDT, Xianzhu Wang	no flags	Details Formatted Diff Diff
Patch (7.42 KB, patch) 2012-10-25 17:35 PDT, Xianzhu Wang	no flags	Details Formatted Diff Diff
Patch (7.53 KB, patch) 2012-10-26 10:51 PDT, Xianzhu Wang	no flags	Details Formatted Diff Diff
Try to fix layout test break (8.91 KB, patch) 2012-10-26 14:01 PDT, Xianzhu Wang	no flags	Details Formatted Diff Diff
Patch (8.05 KB, patch) 2012-11-16 14:37 PST, Xianzhu Wang	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Sami Kyöstilä

Comment 1 2012-10-02 04:37:58 PDT

Created attachment 166666 [details] Patch

Xianzhu Wang

Comment 2 2012-10-02 08:53:29 PDT

*** Bug 96584 has been marked as a duplicate of this bug. ***

Xianzhu Wang

Comment 3 2012-10-02 08:54:49 PDT

Thanks Sami. The patch looks good to me.

Simon Fraser (smfr)

Comment 4 2012-10-02 11:49:47 PDT

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?

Sami Kyöstilä

Comment 5 2012-10-04 05:51:14 PDT

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.

Sami Kyöstilä

Comment 6 2012-10-04 05:55:12 PDT

Created attachment 167090 [details] Patch Only walk subtree if necessary. Restore fixed position composition flag between tests. PTAL?

Simon Fraser (smfr)

Comment 7 2012-10-04 08:37:50 PDT

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.

WebKit Review Bot

Comment 8 2012-10-04 08:57:49 PDT

Comment on attachment 167090 [details] Patch Clearing flags on attachment: 167090 Committed r130396: <http://trac.webkit.org/changeset/130396>

WebKit Review Bot

Comment 9 2012-10-04 08:57:53 PDT

All reviewed patches have been landed. Closing bug.

Jer Noble

Comment 10 2012-10-04 10:12:38 PDT

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

WebKit Review Bot

Comment 11 2012-10-04 10:19:14 PDT

Re-opened since this is blocked by bug 98421

Simon Fraser (smfr)

Comment 12 2012-10-04 10:45:07 PDT

"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)

Xianzhu Wang

Comment 13 2012-10-19 13:45:36 PDT

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?

Xianzhu Wang

Comment 14 2012-10-25 11:42:51 PDT

Created attachment 170698 [details] Patch

Build Bot

Comment 15 2012-10-25 17:02:33 PDT

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

Xianzhu Wang

Comment 16 2012-10-25 17:35:13 PDT

Created attachment 170772 [details] Patch

WebKit Review Bot

Comment 17 2012-10-25 22:21:27 PDT

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

Xianzhu Wang

Comment 18 2012-10-26 10:51:40 PDT

Created attachment 170955 [details] Patch

WebKit Review Bot

Comment 19 2012-10-26 12:04:49 PDT

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

Xianzhu Wang

Comment 20 2012-10-26 14:01:35 PDT

Created attachment 171000 [details] Try to fix layout test break

Xianzhu Wang

Comment 21 2012-10-29 09:32:30 PDT

@skyostil, @Simon, @jamesr, what do you think about the simplified version?

Simon Fraser (smfr)

Comment 22 2012-10-29 11:37:17 PDT

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.

Xianzhu Wang

Comment 23 2012-10-31 11:33:43 PDT

(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.

Sami Kyöstilä

Comment 24 2012-11-02 10:08:14 PDT

(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.

Beth Dakin

Comment 25 2012-11-07 14:29:03 PST

This should be resolved now by http://trac.webkit.org/changeset/133807 *** This bug has been marked as a duplicate of bug 101303 ***

Xianzhu Wang

Comment 26 2012-11-16 14:35:10 PST

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.

Xianzhu Wang

Comment 27 2012-11-16 14:37:08 PST

Created attachment 174762 [details] Patch

Beth Dakin

Comment 28 2012-11-16 16:11:57 PST

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?

Xianzhu Wang

Comment 29 2012-11-16 16:27:23 PST

(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 ***

Note You need to log in before you can comment on or make changes to this bug.