Bug 107618 – Fix painting phases for composited scrolling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

The scrolling contents graphics layer that is created for composited scrolling has its painting phase set to GraphicsLayerPaintForeground. A consequence of this is that focus ring rects are incorrectly painted into it and leave behind lines that scroll with the content.

The aim of this patch is to split the GraphicsLayerPaintForeground phase into two: GraphicsLayerPaintForeground and GraphicsLayerPaintOutline. Whichever layer was set to paint the GraphicsLayerPaintForeground phase, will also be set to paint the GraphicsLayerPaintOutline phase. In the case of composited scrolling, only the GraphicsLayerPaintForeground phase would be shifted to the scrolling contents layer. Whichever layer had been painting the GraphicsLayerPaintOutline phase would continue to do so.

Created attachment 184118 [details]
WIP. Not ready for review.

[Simon Fraser (smfr)]

What about borders?

[Alexey Proskuryakov]

Removing "not ready for review" from bug title. It's not meaningful to have that in a bug title, bugs are not reviewed.

Created attachment 184533 [details]
Patch

(In reply to comment #2)
  > What about borders?

Borders appear to be drawn in the background phase. The scrolling contents layer
didn't seem to pick those up since it painted other phases, the
RenderLayerBacking's graphicsLayer() ended up painting these. With this change,
that layer would get the outlines and focus rects, too.

[WebKit Review Bot]

Comment on attachment 184533 [details]
Patch

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

New failing tests:
platform/chromium/virtual/gpu/compositedscrolling/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
platform/chromium/virtual/softwarecompositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

[Build Bot]

Comment on attachment 184533 [details]
Patch

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

New failing tests:
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

Created attachment 184756 [details]
Updated expectations.

Created attachment 185664 [details]
Patch

With composited scrolling, the scrolling contents layer paints the foreground and the main graphics layer paints the background. This causes a few problems:

 1) If we create a foreground layer, we end up with two layers painting the foreground phase.

 2) Focus rings / outlines paint into the foreground layer, so they end up moving around with the scrolling contents.

 3) Neg z-order descendants paint in the the main graphics layer and will therefore not scroll.

My solution for 1) is not to create a special, foreground layer when we're using composited scrolling. I don't think we need this extra layer in this case. Simon, please let me know if this is wrong, but here is my reasoning. We really shouldn't be using composited scrolling when we have out-of-flow positioned descendants. Things will render differently if we do (and we never automatically opt-in if this is the case). So it seems to make sense to paint all descendants into the same layer since they'll be scrolling together anyhow.

To deal with 2) and 3), I've modified RenderLayer::paintLayerContents to customize the "compositing background" and "compositing foreground" phases if the layer is using composited scrolling. This approach seemed simpler than adding new entries to the paint phase enums.

[WebKit Review Bot]

Comment on attachment 185664 [details]
Patch

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

New failing tests:
platform/chromium/virtual/gpu/compositedscrolling/overflow/composited-scrolling-creates-a-stacking-container.html
compositing/overflow/composited-scrolling-creates-a-stacking-container.html
platform/chromium/virtual/softwarecompositing/overflow/composited-scrolling-creates-a-stacking-container.html

[Build Bot]

Comment on attachment 185664 [details]
Patch

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

New failing tests:
compositing/overflow/composited-scrolling-creates-a-stacking-container.html

[Build Bot]

Comment on attachment 185664 [details]
Patch

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

New failing tests:
compositing/overflow/paint-neg-z-order-descendants-into-scrolling-contents-layer.html
compositing/overflow/composited-scrolling-creates-a-stacking-container.html
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

Created attachment 185841 [details]
Updating test expectations.

Created attachment 188625 [details]
Patch

The last patch incorrectly did away with the foreground layer when using
composited scrolling. This patch makes minor adjustments to ensure that the
foreground layer behaves properly with composited scrolling.

[Simon Fraser (smfr)]

Comment on attachment 188625 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:3770
> -        // Now walk the sorted list of children with negative z-indices.
> -        paintList(negZOrderList(), context, localPaintingInfo, localPaintFlags);
> +        if (!usesCompositedScrolling()) {
> +            // Now walk the sorted list of children with negative z-indices.
> +            paintList(negZOrderList(), context, localPaintingInfo, localPaintFlags);
> +        }
> +
> +        // When using composited scrolling, outlines paint into the background.
> +        if (usesCompositedScrolling() && shouldPaintOutline && !outlineRect.isEmpty()) {
> +            // Paint our own outline
> +            PaintInfo paintInfo(context, pixelSnappedIntRect(outlineRect.rect()), PaintPhaseSelfOutline, paintBehavior, paintingRootForRenderer, localPaintingInfo.region);
> +            clipToRect(localPaintingInfo.rootLayer, context, localPaintingInfo.paintDirtyRect, outlineRect, DoNotIncludeSelfForBorderRadius);
> +            renderer()->paint(paintInfo, paintOffset);
> +            restoreClip(context, localPaintingInfo.paintDirtyRect, outlineRect);
> +        }
>      }
>      
>      if (localPaintFlags & PaintLayerPaintingCompositingForegroundPhase) {
> +        if (usesCompositedScrolling()) {
> +            // Now walk the sorted list of children with negative z-indices.
> +            paintList(negZOrderList(), context, localPaintingInfo, localPaintFlags);
> +        }

Seems wrong to do it like this. RenderLayer shouldn't really care about what compositing layers exist; the behavior should be expressible via paint phases.

> LayoutTests/ChangeLog:32
> +        * compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html: Added.
> +        * compositing/overflow/paint-neg-z-order-descendants-into-scrolling-contents-layer.html: Added.
> +        * platform/chromium/TestExpectations:
> +        * platform/mac-wk2/TestExpectations:

I'd like to see a test with layerTeeAsText output here.

> LayoutTests/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html:3
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
> +   "http://www.w3.org/TR/html4/loose.dtd">
> +<html lang="en">

Please use an HTML5-style DOCTYPE.

Created attachment 191745 [details]
Patch

(In reply to comment #14)
> (From update of attachment 188625 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188625&action=review
>
> > Source/WebCore/rendering/RenderLayer.cpp:3770
> > -        // Now walk the sorted list of children with negative z-indices.
> > -        paintList(negZOrderList(), context, localPaintingInfo, localPaintFlags);
> > +        if (!usesCompositedScrolling()) {
> > +            // Now walk the sorted list of children with negative z-indices.
> > +            paintList(negZOrderList(), context, localPaintingInfo, localPaintFlags);
> > +        }
> > +
> > +        // When using composited scrolling, outlines paint into the background.
> > +        if (usesCompositedScrolling() && shouldPaintOutline && !outlineRect.isEmpty()) {
> > +            // Paint our own outline
> > +            PaintInfo paintInfo(context, pixelSnappedIntRect(outlineRect.rect()), PaintPhaseSelfOutline, paintBehavior, paintingRootForRenderer, localPaintingInfo.region);
> > +            clipToRect(localPaintingInfo.rootLayer, context, localPaintingInfo.paintDirtyRect, outlineRect, DoNotIncludeSelfForBorderRadius);
> > +            renderer()->paint(paintInfo, paintOffset);
> > +            restoreClip(context, localPaintingInfo.paintDirtyRect, outlineRect);
> > +        }
> >      }
> >
> >      if (localPaintFlags & PaintLayerPaintingCompositingForegroundPhase) {
> > +        if (usesCompositedScrolling()) {
> > +            // Now walk the sorted list of children with negative z-indices.
> > +            paintList(negZOrderList(), context, localPaintingInfo, localPaintFlags);
> > +        }
>
> Seems wrong to do it like this. RenderLayer shouldn't really care about what compositing layers exist; the behavior should be expressible via paint phases.
Done.
>
> > LayoutTests/ChangeLog:32
> > +        * compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html: Added.
> > +        * compositing/overflow/paint-neg-z-order-descendants-into-scrolling-contents-layer.html: Added.
> > +        * platform/chromium/TestExpectations:
> > +        * platform/mac-wk2/TestExpectations:
>
> I'd like to see a test with layerTeeAsText output here.
Thanks, this turned out to be very helpful. In order to build a layerTreeAsText-based test, I was forced to update the paintingPhase of the graphics layers rather than doing last minute paint phase hacks in RenderLayerBacking::paintIntoLayer. This simplified logic everywhere and allows for a new kind of text-based testing.
>
> > LayoutTests/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html:3
> > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
> > +   "http://www.w3.org/TR/html4/loose.dtd">
> > +<html lang="en">
>
> Please use an HTML5-style DOCTYPE.
Done.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/compositing/overflow/composited-scrolling-paint-phases-expected.txt', u'LayoutTests/compositing/overflow/composited-scrolling-paint-phases.html', u'LayoutTests/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html', u'LayoutTests/compositing/overflow/paint-neg-z-order-descendants-into-scrolling-contents-layer.html', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/Frame.h', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/GraphicsLayerClient.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl']" exit_code: 1
Source/WebCore/testing/Internals.h:224:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/testing/Internals.h:225:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 20 files


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

[Build Bot]

Comment on attachment 191745 [details]
Patch

Attachment 191745 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17068086

New failing tests:
compositing/overflow/composited-scrolling-paint-phases.html

(In reply to comment #16)
> Attachment 191745 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/compositing/overflow/composited-scrolling-paint-phases-expected.txt', u'LayoutTests/compositing/overflow/composited-scrolling-paint-phases.html', u'LayoutTests/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html', u'LayoutTests/compositing/overflow/paint-neg-z-order-descendants-into-scrolling-contents-layer.html', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/Frame.h', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/GraphicsLayerClient.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl']" exit_code: 1
> Source/WebCore/testing/Internals.h:224:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Source/WebCore/testing/Internals.h:225:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Total errors found: 2 in 20 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

The new enumerator follows the precedent set in Internals.h. I believe the reason for the non-standard casing of enum members is to match the constants defined in Internals.idl.

[Simon Fraser (smfr)]

Comment on attachment 191745 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:3662
> +    bool shouldPaintOutline = isSelfPaintingLayer && !isPaintingOverlayScrollbars
> +        && ((isPaintingScrollingContent && isPaintingCompositedBackground)
> +        || (!isPaintingScrollingContent && isPaintingCompositedForeground));

I think this deserves a comment. It also means that focus rings will end up layering differently with composited scrolling, right?

[Build Bot]

Comment on attachment 191745 [details]
Patch

Attachment 191745 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17084037

New failing tests:
compositing/overflow/composited-scrolling-paint-phases.html

Created attachment 191913 [details]
Patch

(In reply to comment #19)
> (From update of attachment 191745 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191745&action=review
>
> > Source/WebCore/rendering/RenderLayer.cpp:3662
> > +    bool shouldPaintOutline = isSelfPaintingLayer && !isPaintingOverlayScrollbars
> > +        && ((isPaintingScrollingContent && isPaintingCompositedBackground)
> > +        || (!isPaintingScrollingContent && isPaintingCompositedForeground));
>
> I think this deserves a comment. It also means that focus rings will end up layering differently with composited scrolling, right?

Added a comment.

I think that the focus rings will get painted into the same layer that they would have if we weren't using comp-scrolling, but you're right -- this will cause a paint inversion with the focus rings wrt to the foreground. But since the foreground is getting painted into a scrolling layer that is composited above the RLB's primary graphics layer, if a platform decides that it wants to draw the focus rings inside the clip, *and* have them appear above the scrolled content, I'm not sure that can be easily accommodated without adding an additional graphics layer. There's a good chance I'm misunderstanding the RLB's layer tree, though, so please correct me if I've got that wrong.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/compositing/overflow/composited-scrolling-paint-phases-expected.txt', u'LayoutTests/compositing/overflow/composited-scrolling-paint-phases.html', u'LayoutTests/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html', u'LayoutTests/compositing/overflow/paint-neg-z-order-descendants-into-scrolling-contents-layer.html', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/mac/compositing/overflow/composited-scrolling-paint-phases-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/Frame.h', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/GraphicsLayerClient.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl']" exit_code: 1
Source/WebCore/testing/Internals.h:225:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/testing/Internals.h:226:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 21 files


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

[WebKit Review Bot]

Comment on attachment 191913 [details]
Patch

Clearing flags on attachment: 191913

Committed r145067: <http://trac.webkit.org/changeset/145067>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rafael Weinstein]

This caused failures reported here:

https://bugs.webkit.org/show_bug.cgi?id=111768