Bug 91117 - Add support for compositing the contents of overflow:scroll areas
Summary: Add support for compositing the contents of overflow:scroll areas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on: 95076 95494
Blocks: 78862 94353 94743
  Show dependency treegraph
 
Reported: 2012-07-12 10:40 PDT by Simon Fraser (smfr)
Modified: 2018-02-22 12:35 PST (History)
20 users (show)

See Also:


Attachments
Patch (38.42 KB, patch)
2012-07-24 15:37 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Tests (6.92 KB, application/octet-stream)
2012-07-24 15:40 PDT, Simon Fraser (smfr)
no flags Details
Archive of layout-test-results from gce-cr-linux-07 (1.05 MB, application/zip)
2012-07-24 16:30 PDT, WebKit Review Bot
no flags Details
Patch (43.12 KB, patch)
2012-08-16 12:09 PDT, Sami Kyöstilä
no flags Details | Formatted Diff | Diff
Patch (45.36 KB, patch)
2012-08-17 08:09 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (129.23 KB, patch)
2012-08-17 10:20 PDT, Sami Kyöstilä
no flags Details | Formatted Diff | Diff
Patch (141.35 KB, patch)
2012-08-23 10:31 PDT, Sami Kyöstilä
no flags Details | Formatted Diff | Diff
Patch (142.40 KB, patch)
2012-08-24 10:32 PDT, Sami Kyöstilä
no flags Details | Formatted Diff | Diff
Patch for landing (163.64 KB, patch)
2012-08-24 15:44 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2012-07-12 10:40:35 PDT
In order to support accelerated overflow:scroll, we need to put the scrollable contents into their own compositing layer.
Comment 1 Peter Beverloo 2012-07-24 09:35:39 PDT
Hi Simon, could you please give an update about the patch?
Comment 2 Simon Fraser (smfr) 2012-07-24 11:50:32 PDT
Going to work on this today.
Comment 3 Simon Fraser (smfr) 2012-07-24 15:37:21 PDT
Created attachment 154157 [details]
Patch
Comment 4 Simon Fraser (smfr) 2012-07-24 15:40:04 PDT
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.
Comment 5 Simon Fraser (smfr) 2012-07-24 15:40:21 PDT
Created attachment 154158 [details]
Tests
Comment 6 WebKit Review Bot 2012-07-24 16:30:24 PDT
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
Comment 7 WebKit Review Bot 2012-07-24 16:30:30 PDT
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
Comment 8 Sami Kyöstilä 2012-07-25 07:52:14 PDT
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.
Comment 9 Sami Kyöstilä 2012-07-26 11:21:32 PDT
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()".
Comment 10 Antonio Gomes 2012-07-26 11:23:14 PDT
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?
Comment 11 James Robinson 2012-07-27 12:54:49 PDT
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.
Comment 12 Sami Kyöstilä 2012-08-16 12:09:33 PDT
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.
Comment 13 vollick 2012-08-17 08:09:19 PDT
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.
Comment 14 Sami Kyöstilä 2012-08-17 10:20:52 PDT
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.
Comment 15 Adam Barth 2012-08-20 14:05:41 PDT
@jamesr: Would you be willing to review this patch?
Comment 16 Julien Chaffraix 2012-08-22 14:49:56 PDT
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)
Comment 17 Sami Kyöstilä 2012-08-23 10:04:21 PDT
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.
Comment 18 Sami Kyöstilä 2012-08-23 10:31:06 PDT
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.
Comment 19 Peter Beverloo 2012-08-24 06:22:35 PDT
James or Simon, could you please take a look?
Comment 20 Simon Fraser (smfr) 2012-08-24 10:02:54 PDT
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.
Comment 21 Sami Kyöstilä 2012-08-24 10:32:53 PDT
Created attachment 160447 [details]
Patch

Thanks Simon. I removed the extra whitespace.
Comment 22 WebKit Review Bot 2012-08-24 11:32:38 PDT
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
Comment 23 Adam Barth 2012-08-24 15:44:58 PDT
Created attachment 160514 [details]
Patch for landing
Comment 24 Adam Barth 2012-08-24 15:46:11 PDT
I moved the diff to TestExpectation someone arbitrary in the file (rather than the end) in an attempt to avoid merge conflicts.
Comment 25 Adam Barth 2012-08-24 15:46:25 PDT
s/someone/somewhere/
Comment 26 WebKit Review Bot 2012-08-24 17:35:16 PDT
Comment on attachment 160514 [details]
Patch for landing

Clearing flags on attachment: 160514

Committed r126663: <http://trac.webkit.org/changeset/126663>
Comment 27 WebKit Review Bot 2012-08-24 17:35:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Mark Lam 2012-08-24 18:54:12 PDT
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.
Comment 29 James Robinson 2012-08-24 18:56:26 PDT
Looks like they just need expectations in platform/mac to reflect that mac doesn't have ENABLE(OVERFLOW_SCROLLING) turned on.