RenderView layer is marked as fixed position container in the scrolling tree if page scale != 1

[Sami Kyostila]

Render layers with CSS transforms should become containers for any fixed positioned descendants. However, because this check is done with RenderLayer::hasTransform(), we also end up marking the RenderLayer for the RenderView as fixed position container if a non-identity page scale factor is used. This is because page scale is applied as a transform for that layer.

This breaks fixed position layers, because they become fixed relative to the RenderView layer instead of outer scroll clip layer.

The fix is to avoid marking any RenderView layers as fixed position containers.

[Sami Kyostila]

Created attachment 147825 [details]
Patch

[James Robinson]

(In reply to comment #0)
> Render layers with CSS transforms should become containers for any fixed positioned descendants.

This surprises me a bit - why is there a relationship between CSS transforms and fixed positioned descendants?  Is this in a spec somewhere?

> However, because this check is done with RenderLayer::hasTransform(), we also end up marking the RenderLayer for the RenderView as fixed position container if a non-identity page scale factor is used. This is because page scale is applied as a transform for that layer.
> 
> This breaks fixed position layers, because they become fixed relative to the RenderView layer instead of outer scroll clip layer.
> 
> The fix is to avoid marking any RenderView layers as fixed position containers.

[Shawn Singh]

http://dev.w3.org/csswg/css3-transforms/ -- just search for the word "fixed".

I'm sure Simon will have something more authoritative to say, though.  Perhaps that part of the spec is still in a bit of flux?

[Simon Fraser (smfr)]

" The object acts as a containing block for fixed positioned descendants"

Yep, it's like that because Dave Hyatt said it would be really hard to do any other way.

[Simon Fraser (smfr)]

Comment on attachment 147825 [details]
Patch

r- for lack of a test.

[James Robinson]

Wow, that's weird (although I guess doing otherwise would be a pain in the butt).  OK.

[Sami Kyostila]

Created attachment 148091 [details]
Patch

- Added testing framework for ScrollingCoordinator.
- Added layout test to verify fix.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Gyuyoung Kim]

Comment on attachment 148091 [details]
Patch

Attachment 148091 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12979166

[Sami Kyostila]

Created attachment 148101 [details]
Patch

- Fixed efl build failure.

[Simon Fraser (smfr)]

Comment on attachment 148101 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148101&action=review

> Source/WebCore/ChangeLog:3
> +        RenderView layer is marked as fixed position container if page scale != 1

Normally when we talked about "fixed position containers", we're talking about the render tree, but this bug only exists in the scrolling tree. The bug title should say this.

> Source/WebCore/ChangeLog:22
> +        This patch also adds a testing framework for ScrollingCoordinator. Layout
> +        tests can call testRunner.scrollingTreeAsText() to retrieve a text
> +        representation of the scrolling tree of the current page. This feature is
> +        currently only implemented for the Chromium scrolling coordinator back-end.

Do you think that the scrolling tree output is comparable across implementations? If not, the utility of this way of testing is reduced.

> LayoutTests/platform/chromium/compositing/fixed-position-container-with-page-scale.html:22
> +        testRunner.dumpAsText(true);

Why dumpAsText(true)? The pixel result doesn't seem useful.

[Sami Kyostila]

Comment on attachment 148101 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148101&action=review

Thanks Simon.

>> Source/WebCore/ChangeLog:3
>> +        RenderView layer is marked as fixed position container if page scale != 1
> 
> Normally when we talked about "fixed position containers", we're talking about the render tree, but this bug only exists in the scrolling tree. The bug title should say this.

Good point. I'll change this.

>> Source/WebCore/ChangeLog:22
>> +        currently only implemented for the Chromium scrolling coordinator back-end.
> 
> Do you think that the scrolling tree output is comparable across implementations? If not, the utility of this way of testing is reduced.

The current format certainly isn't, but I think I can make a few tweaks to make it more feasible for all implementations to produce the same output. I think we can also adjust the format later if there is need.

>> LayoutTests/platform/chromium/compositing/fixed-position-container-with-page-scale.html:22
>> +        testRunner.dumpAsText(true);
> 
> Why dumpAsText(true)? The pixel result doesn't seem useful.

Ah, I think I got confused by the dumpAsText arguments again. Will fix.

[Sami Kyostila]

Created attachment 148129 [details]
Patch

- Made scrollingTreeAsText() output less Chromium-specific.
- Only save text reference in the test.
- Updated bug title.

[Sami Kyöstilä]

Created attachment 148530 [details]
Patch

Rebased. Another look please?

[Simon Fraser (smfr)]

Comment on attachment 148530 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148530&action=review

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:295
> +String ScrollingCoordinator::scrollingTreeAsText() const
> +{
> +    ASSERT(isMainThread());
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (!m_page || !m_page->mainFrame())
> +        return String();
> +
> +    RenderView* renderView = m_page->mainFrame()->contentRenderer();
> +    if (!renderView || !renderView->compositor() || !renderView->compositor()->rootGraphicsLayer())
> +        return String();
> +
> +    TextStream textStream;
> +    dumpLayer(textStream, renderView->compositor()->rootGraphicsLayer(), 0);
> +    return textStream.release();
> +#else
> +    return String();
> +#endif
> +}

I'd expect this to walk the ScrollingTree nodes, not to just go to GraphicsLayers. Not all platforms will necessarily store scrolling tree info under GraphicsLayers.

I think you should split outs the scrollingTreeAsText changes into a separate bug.

[Sami Kyöstilä]

(In reply to comment #15)
> I'd expect this to walk the ScrollingTree nodes, not to just go to GraphicsLayers. Not all platforms will necessarily store scrolling tree info under GraphicsLayers.

The problem is that the Chromium ScrollingCoordinator implementation doesn't really have a concept of a scrolling tree, so this is the best it can do. I'm open to suggestions on how to make the coordinator testable while allowing for multiple back-end implementations.
 
> I think you should split outs the scrollingTreeAsText changes into a separate bug.

Ok, I'll do that.

[Sami Kyöstilä]

Created attachment 148580 [details]
Patch

- Removed scrolling tree test framework and test.

[Simon Fraser (smfr)]

Comment on attachment 148580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148580&action=review

> Source/WebCore/ChangeLog:19
> +        No new test because the scrolling tree isn't currently testable.

But can't you make a pixel test or ref test that would show the bug?

[Sami Kyöstilä]

(In reply to comment #18)
> But can't you make a pixel test or ref test that would show the bug?

The problem is that the bug is triggered only if the scrolling coordinator tries to position the fixed layer independently of the main thread. In Chromium's case this happens if the compositor processes scroll input events and renders *before* it gets a chance to synchronize the layer tree with the main thread.

Since the layout tests are driven from the main thread, I'm not sure how to capture a snapshot of this intermediate state. For manual testing I usually tie up the main thread with some blocking JS to allow the compositor thread to work independently.

Cases like this are why I wanted to have a more formal way to test ScrollingCoordinator.

[Sami Kyöstilä]

Testing the scrolling tree is now split off into bug 89576.

[Adam Barth]

Comment on attachment 148580 [details]
Patch

There seems to be some controversy in Bug 89576 about how to test the scrolling tree.  Would it make senes to land this fix and continue discussing the testing issues in Bug 89576?

[Sami Kyöstilä]

Any opinions about comment #21?

[Simon Fraser (smfr)]

Comment on attachment 148580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148580&action=review

> Source/WebCore/ChangeLog:8
> +        Render layers with CSS transforms should become containers for anyfixed

any/fixed

> Source/WebCore/rendering/RenderLayerBacking.cpp:455
> +            bool isContainer = m_owningLayer->hasTransform() && !renderer()->isRenderView();

We also use isRootLayer() for the renderview's layer.

[Sami Kyöstilä]

Created attachment 151727 [details]
Patch

Fixed typo and used isRootLayer() instead of isRenderView()

[WebKit Review Bot]

Comment on attachment 151727 [details]
Patch

Clearing flags on attachment: 151727

Committed r122343: <http://trac.webkit.org/changeset/122343>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.