Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(408)

Issue 2588853002: Fix border radius on composited children. (Closed)

Created:
4 years ago by Stephen Chennney
Modified:
3 years, 11 months ago
Reviewers:
chrishtr, trchen
CC:
ajuma+watch_chromium.org, blink-layers+watch_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, eae+blinkwatch, f(malita), jbroman, jchaffraix+rendering, Justin Novosad, leviw+renderwatch, mac-reviews_chromium.org, pdr+graphicswatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, szager+layoutwatch_chromium.org, zoltan1
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix border radius on composited children. When a child of an overflow: hidden object is composited (and the ancestor is not) we were failing to apply the border-radius clip. This patch fixes the problem by adding a new masking layer for this situation, and modifying the layers used for painting to set the clip appropriately. New tests are added to verify the behavior for border styles, and existing layout tests cover the bug (and fail before this patch). Unit testing is used for verifying update behavior. There are remaining issues tracked in TODOs with corresponding bugs. Reworked patch https://codereview.chromium.org/2194273002 BUG=157218 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/d3ba7664a7ebc6e74861602eb9696c7ca0a4ed2e Cr-Commit-Position: refs/heads/master@{#441592}

Patch Set 1 #

Patch Set 2 : Seems to work. Get baselines. #

Patch Set 3 : New baselines #

Total comments: 4

Patch Set 4 : Original patch for diff #

Patch Set 5 : New patch again for diff #

Patch Set 6 : SPV2 expectations #

Unified diffs Side-by-side diffs Delta from patch set Stats (+523 lines, -84 lines) Patch
M third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/border-radius-styles-with-composited-child.html View 1 chunk +41 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/border-radius-styles-with-composited-child-expected.png View 1 2 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/border-radius-styles-with-composited-child-expected.txt View 1 chunk +17 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/empty-composited-child-of-border-radius-ancestor.html View 1 chunk +24 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/empty-composited-child-of-border-radius-ancestor-expected.html View 1 chunk +14 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/siblings-composited-with-border-radius-ancestor.html View 4 1 chunk +37 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/siblings-composited-with-border-radius-ancestor-expected.png View 1 2 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/siblings-composited-with-border-radius-ancestor-expected.txt View 1 2 4 1 chunk +11 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/siblings-with-border-radius-ancestor.html View 4 1 chunk +36 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/siblings-with-border-radius-ancestor-expected.png View 1 2 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/compositing/overflow/siblings-with-border-radius-ancestor-expected.txt View 1 2 4 1 chunk +11 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/borders/border-radius-with-composited-child.html View 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/fast/borders/border-radius-with-composited-child-expected.png View Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/fast/borders/border-radius-with-composited-child-expected.txt View 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/fast/clip/overflow-border-radius-composited-expected.png View Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/borders/border-radius-with-composited-child-expected.png View Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/borders/border-radius-with-composited-child-expected.txt View 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/clip/overflow-border-radius-composited-expected.png View Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/borders/border-radius-with-composited-child-expected.png View Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/borders/border-radius-with-composited-child-expected.txt View 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/clip/overflow-border-radius-composited-expected.png View Binary file 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBox.cpp View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h View 4 chunks +28 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp View 1 4 17 chunks +87 lines, -24 lines 0 comments Download
M third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp View 1 2 3 4 5 1 chunk +81 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/paint/FilterPainter.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/paint/LayerClipRecorder.h View 1 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/paint/LayerClipRecorder.cpp View 1 4 chunks +16 lines, -17 lines 0 comments Download
M third_party/WebKit/Source/core/paint/PaintLayerPainter.h View 1 4 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp View 1 4 10 chunks +71 lines, -14 lines 0 comments Download
M third_party/WebKit/Source/core/paint/PaintLayerPaintingInfo.h View 1 chunk +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/CompositingReasons.h View 1 1 chunk +6 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/CompositingReasons.cpp View 1 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/GraphicsLayerClient.h View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 32 (10 generated)
Stephen Chennney
Still to do: Add unit testing for invalidation, get new baselines, verify the Chrome login ...
3 years, 11 months ago (2017-01-04 16:35:12 UTC) #4
Stephen Chennney
On 2017/01/04 16:35:12, Stephen Chennney wrote: > Still to do: Add unit testing for invalidation, ...
3 years, 11 months ago (2017-01-04 16:40:08 UTC) #5
Stephen Chennney
On 2017/01/04 16:40:08, Stephen Chennney wrote: > On 2017/01/04 16:35:12, Stephen Chennney wrote: > > ...
3 years, 11 months ago (2017-01-04 18:51:23 UTC) #6
Stephen Chennney
https://codereview.chromium.org/2588853002/diff/40001/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 (right): https://codereview.chromium.org/2588853002/diff/40001/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2#newcode2200 third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:2200: crbug.com/157218 compositing/overflow/border-radius-styles-with-composited-child.html [ Failure ] I'll follow up with ...
3 years, 11 months ago (2017-01-04 19:22:46 UTC) #7
chrishtr
What is the diff from the previous patch? Could you upload a patchset directly to ...
3 years, 11 months ago (2017-01-04 20:56:12 UTC) #8
Stephen Chennney
On 2017/01/04 20:56:12, chrishtr wrote: > What is the diff from the previous patch? Could ...
3 years, 11 months ago (2017-01-04 20:59:25 UTC) #9
Stephen Chennney
On 2017/01/04 20:59:25, Stephen Chennney wrote: > On 2017/01/04 20:56:12, chrishtr wrote: > > What ...
3 years, 11 months ago (2017-01-04 21:34:27 UTC) #10
chrishtr
Looks good. Please rename the CL issue title to match first line of description.
3 years, 11 months ago (2017-01-04 23:04:23 UTC) #11
chrishtr
lgtm
3 years, 11 months ago (2017-01-04 23:04:26 UTC) #12
trchen
On 2017/01/04 23:04:26, chrishtr wrote: > lgtm FYI: This implementation won't handle clip escaping correctly. ...
3 years, 11 months ago (2017-01-04 23:49:14 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2588853002/80001
3 years, 11 months ago (2017-01-05 01:15:02 UTC) #15
chrishtr
On 2017/01/04 at 23:49:14, trchen wrote: > On 2017/01/04 23:04:26, chrishtr wrote: > > lgtm ...
3 years, 11 months ago (2017-01-05 01:51:36 UTC) #16
trchen
On 2017/01/05 01:51:36, chrishtr wrote: > On 2017/01/04 at 23:49:14, trchen wrote: > > On ...
3 years, 11 months ago (2017-01-05 02:16:43 UTC) #17
trchen
By the way I also worry about the ancestor radius mask being painted in the ...
3 years, 11 months ago (2017-01-05 02:27:10 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_tests_slimming_paint_v2/builds/2179)
3 years, 11 months ago (2017-01-05 03:06:08 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2588853002/100001
3 years, 11 months ago (2017-01-05 04:14:02 UTC) #23
commit-bot: I haz the power
Committed patchset #6 (id:100001)
3 years, 11 months ago (2017-01-05 05:59:42 UTC) #26
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/d3ba7664a7ebc6e74861602eb9696c7ca0a4ed2e Cr-Commit-Position: refs/heads/master@{#441592}
3 years, 11 months ago (2017-01-05 06:02:00 UTC) #28
Stephen Chennney
On 2017/01/05 02:16:43, trchen wrote: > On 2017/01/05 01:51:36, chrishtr wrote: > > On 2017/01/04 ...
3 years, 11 months ago (2017-01-05 17:11:01 UTC) #29
chrishtr
On 2017/01/05 at 17:11:01, schenney wrote: > On 2017/01/05 02:16:43, trchen wrote: > > On ...
3 years, 11 months ago (2017-01-05 17:54:14 UTC) #30
Stephen Chennney
On 2017/01/05 17:54:14, chrishtr wrote: > On 2017/01/05 at 17:11:01, schenney wrote: > > On ...
3 years, 11 months ago (2017-01-05 18:03:57 UTC) #31
Stephen Chennney
3 years, 11 months ago (2017-01-05 18:17:25 UTC) #32
Message was sent while issue was closed.
On 2017/01/05 18:03:57, Stephen Chennney wrote:
> On 2017/01/05 17:54:14, chrishtr wrote:
> > On 2017/01/05 at 17:11:01, schenney wrote:
> > > On 2017/01/05 02:16:43, trchen wrote:
> > > > On 2017/01/05 01:51:36, chrishtr wrote:
> > > > > On 2017/01/04 at 23:49:14, trchen wrote:
> > > > > > On 2017/01/04 23:04:26, chrishtr wrote:
> > > > > > > lgtm
> > > > > > 
> > > > > > FYI: This implementation won't handle clip escaping correctly. For
> > example,
> > > > > http://output.jsbin.com/poxiwon/
> > > > > > That said, it is super difficult, if not impossible, to implement in
> > SPv1. I
> > > > > feel this CL will fix more things than it will break, so still lgtm.
> > > > > 
> > > > > This is a good point. I don't think it's safe to start clipping those,
> > it'll
> > > > > regress real content. I think we should add a compositing reason for
> such
> > > > > children. CompositingReasonOutOfFlowClipping does this for children of
a
> > > > > composited scroller already. We can also promote descendants of a
> > > > border-radius
> > > > > non-stacking PaintLayer...
> > > > 
> > > > I don't see how it solves the problem though. In the example, both the
> > opacity
> > > > element and the animating element are composited already. (For opacity
> with
> > > > composited descendant, and animation, respectively.)
> > > > 
> > > > The problem is that layer->setMaskLayer() is a group mask (i.e. applies
to
> > the
> > > > subtree as a whole) instead of a local mask (i.e. applies to the current
> > layer
> > > > only). A solution is to pull the layer contents from m_graphicsLayer to
a
> > > > separate layer, and apply the mask to that layer. (m_backgroundLayer and
> > > > m_foregroundLayer sounds like the right place.)
> > > 
> > > What's really odd about this test case and the landed patch is that the
> > animated element is masked at the corners of the containing div, but not at
> the
> > straight sides. I don't understand that at all. I guess I still have more to
> > learn.
> > 
> > Why would it be masked at all? Those children should be siblings in the
> graphics
> > layer tree.
> 
> I'll look into it. I just don't see how a mask layer that masks out the
corners
> fails to also mask out the straight bits. The only explanation I can come up
> with is some nine-piece effect.

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=678669. Will copy
this discussion there.

Powered by Google App Engine
This is Rietveld 408576698