Bug 97357 – Respect -webkit-overflow-scrolling:touch if it is safe to do so

• Status: NEW
• Component: WebCore Misc.

If a render layer is composited, and is therefore a stacking context, there is no risk in putting it on the fast scrolling path. So in this case, RenderLayer::usesCompositedScrolling() should return true.

Created attachment 165179 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 165179 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        If the scrolling render layer is composited, then RenderLayer::usesCompositedScrolling
> +        will return true.

Might be nice to describe what this does. :)  Even if it's obvious to the compositor folks.

[Simon Fraser (smfr)]

Comment on attachment 165179 [details]
Patch

I don't follow this at all. It's quite possible to have a <div> be in a compositing layer, but to not have set up the inner scrolling layers that make it scroll compositedly.

[Build Bot]

Comment on attachment 165179 [details]
Patch

Attachment 165179 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13950752

New failing tests:
compositing/overflow/overflow-scrollbar-layers.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/overflow/content-gains-scrollbars.html
compositing/layer-creation/overflow-scroll-overlap.html

[James Robinson]

If we do have a layer that is set up to scroll we should throw it down this path without needing the webkit-overflow-scroll property.  What other internal setup is required, Simon?

[Simon Fraser (smfr)]

It doesn't follow that being composited means that something is a stacking context.

[James Robinson]

(In reply to comment #6)
> It doesn't follow that being composited means that something is a stacking context.

Huh?  What do we make composited that's not a stacking context?

[Simon Fraser (smfr)]

Layers that are composited because of overlap, for example.

[James Robinson]

If the layer is a stacking context (for whatever reason) we should be able to go down this path, though.

[WebKit Review Bot]

Comment on attachment 165179 [details]
Patch

Attachment 165179 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13950786

New failing tests:
compositing/overflow/scrollbars-with-clipped-owner.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/overflow/content-gains-scrollbars.html
compositing/repaint/newly-composited-repaint-rect.html
compositing/overflow/overflow-scrollbar-layers.html
compositing/overflow/clip-content-under-overflow-controls.html
WebFilterOperationsTest.saveAndRestore
compositing/layer-creation/overflow-scroll-overlap.html

In light of the previous comments, I tried returning true in RenderLayer::usesCompositedScrolling if isComposited() and isStackingContext() were both true, but this caused some problems in the layout tests. The problem is that when composited scrolling is used, the scrolling contents are rendered behind transparent scroll controls rather than being clipped. Hopefully this behavior can be changed, but at present, automatically opting into composited scrolling will cause correctness issues.

Respecting -webkit-overflow-scrolling:touch is also currently dangerous, because it creates a stacking context and the promotion to a composited layer can affect anti-aliasing, so we will break the z-ordering and anti-aliasing of some existing pages.

Given these facts, I propose the following:
 - We respect -webkit-overflow-scrolling:touch *if*
   a) We're already a stacking context, and
   b) We're already composited.

This would let us explicitly opt into composited scrolling in a way that prevents us from breaking the z-ordering and anti-aliasing of existing pages.

In future, if the problem of rendering the scrollable content behind the overflow scroll controls is addressed, we can look into automatically opting into this path without -webkit-overflow-scrolling:touch. We might also relax the isStackingContext restriction -- it should be sufficient that promotion to a stacking context would not affect z-ordering.

I will upload a patch that does this shortly. In that patch, if ENABLE(ACCELERATED_OVERFLOW_SCROLLING) is defined, then -webkit-overflow-scrolling:touch will *force* us to use composited scrolling, otherwise that property will only turn on composited scrolling if we're already a composited stacking context.

Created attachment 165271 [details]
Patch

Contains the changes mentioned in the previous comment.

[WebKit Review Bot]

Attachment 165271 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderLayer.cpp:1597:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 165272 [details]
Patch

I apologize for the spam. This patch addresses the style issue.

[Antonio Gomes]

So you are doing this for feature detection purposes?

(In reply to comment #15)
> So you are doing this for feature detection purposes?

It's more that I'm trying to enabled a feature rather than detect it (but perhaps I'm misunderstanding your question). Composited scrolling is great -- it saves us some very costly painting -- but designers currently have no way to take advantage of it on browsers built without ACCELERATED_OVERFLOW_SCROLLING enabled. My hope was to provide *some* recourse for designers who want to use composited scrolling on all WebKit-based browsers, but do so in a way that doesn't break existing pages.

With this change, a designer can use -webkit-overflow-scrolling:touch in combination with -webkit-transform:translateZ(0px) to opt into composited scrolling.

I should stress that this isn't the end of the work on this, either. My hope is that we can use composited scrolling on more and more pages, eventually without using -webkit-overflow-scrolling at all.

[Simon Fraser (smfr)]

(In reply to comment #16)
> With this change, a designer can use -webkit-overflow-scrolling:touch in combination with -webkit-transform:translateZ(0px) to opt into composited scrolling.

I really don't want more translateZ(0) cargo-cultism than already exists, especially when combined with the already mysterious -webkit-overflow-scrolling.

(In reply to comment #17)
> (In reply to comment #16)
> > With this change, a designer can use -webkit-overflow-scrolling:touch in combination with -webkit-transform:translateZ(0px) to opt into composited scrolling.
> 
> I really don't want more translateZ(0) cargo-cultism than already exists, especially when combined with the already mysterious -webkit-overflow-scrolling.

I'm still working on a proper response, but in the meantime, I wanted to lists the tests that fail when composited scrolling is used because the scrolled contents are drawn behind/over the overflow controls. These are
compositing/overflow/clip-content-under-overflow-controls.html
compositing/overflow/scrollbars-with-clipped-owner.html

[James Robinson]

usesCompositedScrolling() controls too many behaviors.  There are a few concepts here.  For the compositor tree setup it shouldn't make a different what this value is set to, although other parts of the rendering code do care about this property.

What I think we should do here is set up the RenderLayerBacking's GraphicsLayer hierarchy so that scrolling is only a matter of updating one layer's position and make sure clipping, etc, is correct.  Then it's a simple matter to hook up RenderLayer::scrollTo() to simply update this position and rely on the compositor to repaint content as needed.  Look at how RenderLayerCompositor's GraphicsLayer hierarchy is different to accomodate this.  The m_containmentLayer/m_scrollingLayer/m_scrollingContentsLayer fields on RLB appear to be partway there, but I think we should just make it part of the default setup and not rely on things like offsetFromRenderer.

[Andreas Kling]

Comment on attachment 165272 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.