Bug 107618 - Fix painting phases for composited scrolling
Summary: Fix painting phases for composited scrolling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on: 111768
Blocks: 107952
  Show dependency treegraph
 
Reported: 2013-01-22 20:34 PST by vollick
Modified: 2013-03-07 13:00 PST (History)
11 users (show)

See Also:


Attachments
WIP. Not ready for review. (6.97 KB, patch)
2013-01-22 20:39 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (11.01 KB, patch)
2013-01-24 11:08 PST, vollick
no flags Details | Formatted Diff | Diff
Updated expectations. (13.35 KB, patch)
2013-01-25 08:21 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (16.54 KB, patch)
2013-01-30 20:14 PST, vollick
no flags Details | Formatted Diff | Diff
Updating test expectations. (18.37 KB, patch)
2013-01-31 12:41 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (18.90 KB, patch)
2013-02-15 13:00 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (38.09 KB, patch)
2013-03-06 06:50 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (42.19 KB, patch)
2013-03-06 21:28 PST, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2013-01-22 20:34:02 PST
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.
Comment 1 vollick 2013-01-22 20:39:10 PST
Created attachment 184118 [details]
WIP. Not ready for review.
Comment 2 Simon Fraser (smfr) 2013-01-22 22:13:53 PST
What about borders?
Comment 3 Alexey Proskuryakov 2013-01-23 09:52:29 PST
Removing "not ready for review" from bug title. It's not meaningful to have that in a bug title, bugs are not reviewed.
Comment 4 vollick 2013-01-24 11:08:01 PST
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.
Comment 5 WebKit Review Bot 2013-01-24 12:20:12 PST
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
Comment 6 Build Bot 2013-01-25 03:19:35 PST
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
Comment 7 vollick 2013-01-25 08:21:45 PST
Created attachment 184756 [details]
Updated expectations.
Comment 8 vollick 2013-01-30 20:14:15 PST
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.
Comment 9 WebKit Review Bot 2013-01-30 22:59:39 PST
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
Comment 10 Build Bot 2013-01-31 01:40:58 PST
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
Comment 11 Build Bot 2013-01-31 04:45:09 PST
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
Comment 12 vollick 2013-01-31 12:41:11 PST
Created attachment 185841 [details]
Updating test expectations.
Comment 13 vollick 2013-02-15 13:00:55 PST
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.
Comment 14 Simon Fraser (smfr) 2013-03-04 11:17:43 PST
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.
Comment 15 vollick 2013-03-06 06:50:44 PST
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.
Comment 16 WebKit Review Bot 2013-03-06 06:58:50 PST
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.
Comment 17 Build Bot 2013-03-06 13:19:25 PST
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
Comment 18 vollick 2013-03-06 13:44:58 PST
(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.
Comment 19 Simon Fraser (smfr) 2013-03-06 14:10:08 PST
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?
Comment 20 Build Bot 2013-03-06 14:21:44 PST
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
Comment 21 vollick 2013-03-06 21:28:04 PST
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.
Comment 22 WebKit Review Bot 2013-03-06 21:32:06 PST
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.
Comment 23 WebKit Review Bot 2013-03-07 06:03:04 PST
Comment on attachment 191913 [details]
Patch

Clearing flags on attachment: 191913

Committed r145067: <http://trac.webkit.org/changeset/145067>
Comment 24 WebKit Review Bot 2013-03-07 06:03:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Rafael Weinstein 2013-03-07 13:00:34 PST
This caused failures reported here:

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