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.
What about borders?
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.
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 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.
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 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 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.
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.
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 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.
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 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.
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 on attachment 191913 [details] Patch Clearing flags on attachment: 191913 Committed r145067: <http://trac.webkit.org/changeset/145067>
All reviewed patches have been landed. Closing bug.
This caused failures reported here: https://bugs.webkit.org/show_bug.cgi?id=111768