Bug 98144 - Layout test for "Fixed position visibility check does not consider descendants"
Summary: Layout test for "Fixed position visibility check does not consider descendants"
Status: RESOLVED DUPLICATE of bug 101303
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL: https://www.gov.uk/organise-fete-stre...
Keywords:
: 96584 (view as bug list)
Depends on: 98421
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-02 04:33 PDT by Sami Kyöstilä
Modified: 2012-11-16 16:38 PST (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Sami Kyöstilä 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.
Comment 1 Sami Kyöstilä 2012-10-02 04:37:58 PDT
Created attachment 166666 [details]
Patch
Comment 2 Xianzhu Wang 2012-10-02 08:53:29 PDT
*** Bug 96584 has been marked as a duplicate of this bug. ***
Comment 3 Xianzhu Wang 2012-10-02 08:54:49 PDT
Thanks Sami. The patch looks good to me.
Comment 4 Simon Fraser (smfr) 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?
Comment 5 Sami Kyöstilä 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.
Comment 6 Sami Kyöstilä 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?
Comment 7 Simon Fraser (smfr) 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.
Comment 8 WebKit Review Bot 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>
Comment 9 WebKit Review Bot 2012-10-04 08:57:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Jer Noble 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
Comment 11 WebKit Review Bot 2012-10-04 10:19:14 PDT
Re-opened since this is blocked by bug 98421
Comment 12 Simon Fraser (smfr) 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)
Comment 13 Xianzhu Wang 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?
Comment 14 Xianzhu Wang 2012-10-25 11:42:51 PDT
Created attachment 170698 [details]
Patch
Comment 15 Build Bot 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
Comment 16 Xianzhu Wang 2012-10-25 17:35:13 PDT
Created attachment 170772 [details]
Patch
Comment 17 WebKit Review Bot 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
Comment 18 Xianzhu Wang 2012-10-26 10:51:40 PDT
Created attachment 170955 [details]
Patch
Comment 19 WebKit Review Bot 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
Comment 20 Xianzhu Wang 2012-10-26 14:01:35 PDT
Created attachment 171000 [details]
Try to fix layout test break
Comment 21 Xianzhu Wang 2012-10-29 09:32:30 PDT
@skyostil, @Simon, @jamesr, what do you think about the simplified version?
Comment 22 Simon Fraser (smfr) 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.
Comment 23 Xianzhu Wang 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.
Comment 24 Sami Kyöstilä 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.
Comment 25 Beth Dakin 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 ***
Comment 26 Xianzhu Wang 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.
Comment 27 Xianzhu Wang 2012-11-16 14:37:08 PST
Created attachment 174762 [details]
Patch
Comment 28 Beth Dakin 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?
Comment 29 Xianzhu Wang 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 ***