Bug 91117 – Add support for compositing the contents of overflow:scroll areas

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

In order to support accelerated overflow:scroll, we need to put the scrollable contents into their own compositing layer.

[Peter Beverloo]

Hi Simon, could you please give an update about the patch?

[Simon Fraser (smfr)]

Going to work on this today.

[Simon Fraser (smfr)]

Created attachment 154157 [details]
Patch

[Simon Fraser (smfr)]

I'm not able to test this  in desktop Mac builds, so the patch does not include any testscase. I shall attach a zip file of tests we have added on iOS, but they will need some tweaking.

[Simon Fraser (smfr)]

Created attachment 154158 [details]
Tests

[WebKit Review Bot]

Comment on attachment 154157 [details]
Patch

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

New failing tests:
fast/css/resize-corner-tracking-transformed.html
compositing/geometry/clip.html
compositing/overflow/overflow-scroll.html

[WebKit Review Bot]

Created attachment 154170 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Sami Kyöstilä]

Comment on attachment 154157 [details]
Patch

Thanks Simon. Overall I like the way this looks. I'll still try combine it with the Android patch from bug 78862 and test it in practice. If I'm reading the patch right, the scrolling contents layer will be repainted after every scroll. Is that something you were planning to fix here?

By the way, looks like overflow-div-scrolling.js is missing from the test zip.

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

> Source/WebCore/rendering/RenderLayer.cpp:3007
> +                IgnoreOverlayScrollbarSize, paintFlags & PaintLayerPaintingOverflowContents ? IgnoreOverflowClip : RespectOverflowClip);

Nit: suggest using "(paintFlags & PaintLayerPaintingOverflowContents) ? ..." for consistency here and elsewhere.

> Source/WebCore/rendering/RenderLayer.cpp:3140
> +    if (this != rootLayer || !(localPaintFlags & PaintLayerPaintingOverflowContents))

Are you checking against the root layer here because child layers will automatically get the extended damage rect from calculateClipRects? If so, I'm wondering whether would be clearer to avoid passing PaintLayerPaintingOverflowContents to child layers in the first place. That way you wouldn't need the other this != rootLayer() checks either.

> Source/WebCore/rendering/RenderLayerBacking.cpp:261
> +    

Nit: extra whitespace

> Source/WebCore/rendering/RenderLayerBacking.cpp:523
> +        IntRect paddingBox(renderBox->borderLeft(), renderBox->borderTop(),

Looks like you could just offset graphicsLayerParentLocation by (renderBox->borderLeft(), renderBox()->borderTop()) instead of calculating the full paddingBox.

> Source/WebCore/rendering/RenderLayerBacking.cpp:535
> +        IntRect parentClipRect = pixelSnappedIntRect(m_owningLayer->backgroundClipRect(compAncestor, 0, TemporaryClipRects, IgnoreOverlayScrollbarSize, RenderLayer::IgnoreOverflowClip).rect()); // FIXME: Incorrect for CSS regions.

Is it always safe to ignore overflow clipping here, or should it only be done for clipping ancestors that use composited scrolling?

> Source/WebCore/rendering/RenderLayerBacking.cpp:653
> +        // Note that we implement the contents offset via the bounds origin on this layer, rather than a position on the sublayer.

Any particular reason for this choice? It feels like setting the position would be more intuitive.

> Source/WebCore/rendering/RenderLayerBacking.cpp:665
> +        // Scrolling the content layer does not need to trigger a repaint. The offset will be compensated away during painting.

Is this first comment a FIXME? Currently setOffsetFromRenderer() will always fully invalidate the layer, right?

> Source/WebCore/rendering/RenderLayerBacking.cpp:711
> +        superlayer->addChild(m_scrollingLayer.get());

Should we call m_scrollingLayer->removeFromParent first()?

> Source/WebCore/rendering/RenderLayerBacking.cpp:718
> +        // We don't have to consider overflow controls, because we know that the scrollabrs are drawn elsewhere.

Nit: s/absr/bars/

> Source/WebCore/rendering/RenderLayerBacking.cpp:723
> +        m_graphicsLayer->setDrawsContent(hasNonScrollingPaintedContent);

Nice, this is a useful optimization.

> Source/WebCore/rendering/RenderLayerBacking.cpp:880
> +bool RenderLayerBacking::updateScrollingLayers(bool scrollingLayers)

Nit: needsScrollingLayers for consistency.

[Sami Kyöstilä]

Comment on attachment 154157 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:709
> +    if (m_scrollingLayer) {

Should the scrolling container be added before the overflow controls above? Otherwise the content will be drawn on top of the scrollbars.

Also, RenderLayerCompositor::rebuildCompositingLayerTree():1028 reparents overflow controls as children of the scrolling content layer unless the layer has a clipping layer. I think that condition should be "!layerBacking->hasClippingLayer() && !layerBacking->hasScrollingLayer()".

[Antonio Gomes]

Simon, with your patch applied locally and code to promote the layer to be accelerated (i.e. in RenderLayerCompositor), I am getting overflow-scrolling: touch elements to only render the background layer.

Would there be anything I could obviously be missing?

[James Robinson]

Comment on attachment 154157 [details]
Patch

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

> Source/WebCore/ChangeLog:26
> +        (WebCore::RenderLayer::scrollTo): Dont' repaint if we're doing

typo "Dont'"

> Source/WebCore/ChangeLog:34
> +        Assert if we're fetching cachec rects with a different 'respect clip' flag to the one they were generated

typo "cachec"

> Source/WebCore/platform/graphics/GraphicsLayerClient.h:44
> +    GraphicsLayerPaintOverflowContents = (1 << 3),
>      GraphicsLayerPaintAll = (GraphicsLayerPaintBackground | GraphicsLayerPaintForeground | GraphicsLayerPaintMask)

is there a specific reason this new enum isn't included in the "all" phase? should the "all" phase be renamed to something more specific?

> Source/WebCore/rendering/RenderLayer.cpp:4075
> +    // Need to use temporary clip rects, because the value of 'dontClipToOverflow' may be different from the painting path (<rdar://problem/11844909>).

would it be possible to file / cite a bugs.webkit.org bug here? It's hard for non-Apple people like me to know what this is talking about.

[Sami Kyöstilä]

Created attachment 158870 [details]
Patch

First pass at fixing some problems with the original patch. Still need to clean up the tests.

    - Renamed GraphicsLayerPaintAll => GraphicsLayerPaintAllWithOverflowClip
    - Use layer position instead of bounds origin to position scrolling
      content.
    - Fixed scrolling content appearing over overflow controls.
    - Always promote touch scrollable elements to composition layers
      (RLC::requiresCompositingLayer & RLC::requiresOwnBackingStore).
    - Include touch scrolling in RLC::reasonForCompositing()
    - Include touch scrolling in layer memory usage estimate.
    - Fixed some typos and formatting.

Created attachment 159124 [details]
Patch

Rebased Sami's patch and hopefully fixed the bug that was causing the layout
test failures. I think the problem is in RenderLayer::calculateRects. In
RenderLayer::calculateClipRects, there is this check:

(renderer()->hasOverflowClip() && (respectOverflowClip == RespectOverflowClip || this != rootLayer)

which I interpret as "has an overflow clip, and we should respect it." In
RenderLayer::calculateRects there this check appears three times:

renderer()->hasOverflowClip() && (this != rootLayer || respectOverflowClip == IgnoreOverflowClip)

immediately followed by code that clips to the overflow clip rect. So, in
effect, the code is saying "if we have an overflow clip, and we should ignore
it, then clip," but I think this may have been a typo and the same check as we
see in ::calculateClipRects was meant to be used. The result is that the fgRect
is not clipped, so it draws below the overflow scroll controls, breaking the
layout tests.

Still to be done: get the attached tests in working order.

[Sami Kyöstilä]

Created attachment 159153 [details]
Patch

- Included Ian's fix for the reversed clipping conditions in RenderLayer::calculateRects().
- Mark primary graphics layer as needing display if its painting phase changes.
- Added the tests from Simon's zip along with baselines and some cleanups. Note that some of the tests are expected to fail since ENABLE_OVERFLOW_SCROLLING is still off.
- Removed scrollTo-at-page-load.html since it appears to be unrelated touch overflow scrolling.
- Removed overflow-div-scrolling.html as the test driver JS is missing and it isn't clear what the test should be doing.

[Adam Barth]

@jamesr: Would you be willing to review this patch?

[Julien Chaffraix]

Comment on attachment 159153 [details]
Patch

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

The RenderLayer part of the change is fine, some comments.

> Source/WebCore/rendering/RenderLayer.cpp:1558
> +    if (!scrollsOverflow())

Using scrollsOverflow() you are potentially allowing overflow: auto / overlay RenderLayers to also be composited. I couldn't find how you disallow this case so please correct me if I am wrong here. Note that if we do allow this case, it should be tested.

> Source/WebCore/rendering/RenderLayer.h:523
> +    enum RespectOverflowClipOrNot { IgnoreOverflowClip, RespectOverflowClip };

I would rename that to ShouldRespectOverflowClip (not a super fan of the "OrNot"-suffix)

> LayoutTests/compositing/overflow/textarea-scroll-touch.html:20
> +    if (window.layoutTestController)
> +      layoutTestController.dumpAsText();

s/layoutTestController/testRunner/ (not repeated on all test files, see Ryosuke email on webkit-dev)

[Sami Kyöstilä]

Thanks Julien.

(In reply to comment #16)
> Using scrollsOverflow() you are potentially allowing overflow: auto / overlay RenderLayers to also be composited. I couldn't find how you disallow this case so please correct me if I am wrong here. Note that if we do allow this case, it should be tested.

Good point. I think elements with overflow: auto/overlay should indeed be promoted, but only if there actually is something to scroll. I've changed this to also check allowsScrolling() and added tests for all the various combinations.

> > Source/WebCore/rendering/RenderLayer.h:523
> > +    enum RespectOverflowClipOrNot { IgnoreOverflowClip, RespectOverflowClip };
> 
> I would rename that to ShouldRespectOverflowClip (not a super fan of the "OrNot"-suffix)

Done.

> > LayoutTests/compositing/overflow/textarea-scroll-touch.html:20
> > +    if (window.layoutTestController)
> > +      layoutTestController.dumpAsText();
> 
> s/layoutTestController/testRunner/ (not repeated on all test files, see Ryosuke email on webkit-dev)

Done.

[Sami Kyöstilä]

Created attachment 160192 [details]
Patch

- Renamed RespectOverflowClipOrNot => ShouldRespectOverflowClip.
- s/layoutTestController/testRunner/g.
- Added tests for overflow auto/hidden/visible/overlay with and without actual overflow.
- Check for layer scrollability before promoting.

[Peter Beverloo]

James or Simon, could you please take a look?

[Simon Fraser (smfr)]

Comment on attachment 160192 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:537
> +    if (compAncestor &&  compAncestor->usesCompositedScrolling()) {

Two spaces here after the &&

> Source/WebCore/rendering/RenderLayerBacking.cpp:730
> +        // We don't have to consider overflow controls, because we know that the scrollbars are drawn elsewhere.

This comment may not be appropriate on desktop, but let's fix this later.

[Sami Kyöstilä]

Created attachment 160447 [details]
Patch

Thanks Simon. I removed the extra whitespace.

[WebKit Review Bot]

Comment on attachment 160447 [details]
Patch

Rejecting attachment 160447 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
mit-queue/Source/WebKit/chromium/third_party/skia/gyp --revision 5251 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
43>At revision 5251.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13600223

[Adam Barth]

Created attachment 160514 [details]
Patch for landing

[Adam Barth]

I moved the diff to TestExpectation someone arbitrary in the file (rather than the end) in an attempt to avoid merge conflicts.

[Adam Barth]

s/someone/somewhere/

[WebKit Review Bot]

Comment on attachment 160514 [details]
Patch for landing

Clearing flags on attachment: 160514

Committed r126663: <http://trac.webkit.org/changeset/126663>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

This change cause 4 failures on mac and mac-lion:
compositing/overflow/overflow-auto-with-touch.html
compositing/overflow/overflow-overlay-with-touch.html
compositing/overflow/scrolling-content-clip-to-viewport.html
compositing/overflow/textarea-scroll-touch.html

See http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r126664%20(2782)/results.html.

[James Robinson]

Looks like they just need expectations in platform/mac to reflect that mac doesn't have ENABLE(OVERFLOW_SCROLLING) turned on.