Bug 138551 - Border-radius does not clip a composited descendant
Summary: Border-radius does not clip a composited descendant
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 140289 140336 140536
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-09 15:37 PST by Byungseon Shin
Modified: 2015-01-26 16:00 PST (History)
15 users (show)

See Also:


Attachments
Clip compositing for animating decendent test (878 bytes, text/html)
2014-11-09 15:37 PST, Byungseon Shin
no flags Details
Patch (29.74 KB, patch)
2014-11-10 06:33 PST, Byungseon Shin
no flags Details | Formatted Diff | Diff
Patch (19.59 KB, patch)
2014-11-17 05:30 PST, Byungseon Shin
no flags Details | Formatted Diff | Diff
Patch (19.61 KB, patch)
2014-11-17 11:05 PST, Byungseon Shin
no flags Details | Formatted Diff | Diff
Patch (24.17 KB, patch)
2014-12-15 06:29 PST, Byungseon Shin
no flags Details | Formatted Diff | Diff
Patch (24.17 KB, patch)
2014-12-15 06:52 PST, Byungseon Shin
no flags Details | Formatted Diff | Diff
Patch (23.00 KB, patch)
2014-12-22 11:26 PST, Byungseon Shin
no flags Details | Formatted Diff | Diff
Patch (24.77 KB, patch)
2015-01-01 03:17 PST, Byungseon Shin
no flags Details | Formatted Diff | Diff
Test for changing between uniform to non-uniform corner radius & removing border-radius. (1.57 KB, text/html)
2015-01-01 03:20 PST, Byungseon Shin
no flags Details
Patch (24.83 KB, patch)
2015-01-01 03:52 PST, Byungseon Shin
no flags Details | Formatted Diff | Diff
Testcase that asserts on non-Mac (818 bytes, text/html)
2015-01-16 15:59 PST, Simon Fraser (smfr)
no flags Details
Patch (1.79 KB, patch)
2015-01-16 18:40 PST, Byungseon Shin
no flags Details | Formatted Diff | Diff
Patch (5.67 KB, patch)
2015-01-25 09:00 PST, Byungseon Shin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Byungseon Shin 2014-11-09 15:37:23 PST
Created attachment 241264 [details]
Clip compositing for animating decendent test

Clip compositing does not work for animating decendent on attached test case.
Comment 1 Simon Fraser (smfr) 2014-11-09 19:35:22 PST
Known issue.
Comment 2 Byungseon Shin 2014-11-09 20:13:35 PST
Are you woring on the patch also?

We are about to upload a patch based on following Chromium's patches after rebasing lastest master branch.

http://src.chromium.org/viewvc/blink?view=revision&revision=158258
http://src.chromium.org/viewvc/blink?view=revision&revision=160578
http://src.chromium.org/viewvc/blink?view=revision&revision=162463
http://src.chromium.org/viewvc/blink?view=revision&revision=164833
Comment 3 Simon Fraser (smfr) 2014-11-09 20:26:12 PST
I'm not working on a patch, but please note that http://trac.webkit.org/changeset/175794 changed some code in this area.
Comment 4 Byungseon Shin 2014-11-10 06:33:00 PST
Created attachment 241288 [details]
Patch
Comment 5 Simon Fraser (smfr) 2014-11-10 10:44:35 PST
Comment on attachment 241288 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        If an element's layer with accelerated children and border-radius reaches 
> +        the compositing state of |paintsIntoCompositedAncestor|, painting is 
> +        no longer performed due to the early bail in CompositedLayerMapping::doPaintTask, 
> +        which also triggers a debug assert.
> +
> +        The issue was caused by adding support for correctly rendering layers having 
> +        border-radius and overflow-hidden using masks, by the time 
> +        the |paintsIntoCompositedAncestor| compositing state was removed.
> +        However, this compositing state was reintroduced, causing the problem at matter.

This comment seems to come before the relevant comments about support border-radius clip with compositing.

> Source/WebCore/platform/graphics/GraphicsLayer.h:251
> +    GraphicsLayer* contentsClippingMaskLayer() const { return m_contentsClippingMaskLayer; }
> +    virtual void setContentsClippingMaskLayer(GraphicsLayer* layer) { m_contentsClippingMaskLayer = layer; }

You need to look at what I did for Mac in r175794.
On Mac/iOS we can use the existing contentsClippingLayer for border-radius clip, since CA supports clipping via a layer's corner radius. We use a mask shape layer in cases where that's not possible.

Because of that it's not appropriate to add a contentsClippingMaskLayer for all platforms here in the base class; you need to do this in a way that lets platforms optimally clip.

> Source/WebCore/rendering/RenderBox.cpp:1490
> +void RenderBox::paintClippingMask(PaintInfo& paintInfo, const LayoutPoint& paintOffset)
> +{
> +    if (!paintInfo.shouldPaintWithinRoot(*this) || style().visibility() != VISIBLE || paintInfo.phase != PaintPhaseClippingMask || paintInfo.context->paintingDisabled())
> +        return;
> +
> +    LayoutRect paintRect = LayoutRect(paintOffset, size());
> +    paintInfo.context->fillRect(snappedIntRect(paintRect), Color::black, ColorSpaceDeviceRGB);
> +}

I don't understand why any new code in renderers is required.

> Source/WebCore/rendering/RenderLayerBacking.cpp:94
> +static inline bool isAcceleratedContents(const RenderObject& renderer)

This is a bad name: should be hasAcceleratedContents()
Comment 6 Byungseon Shin 2014-11-17 05:30:28 PST
Created attachment 241702 [details]
Patch
Comment 7 Byungseon Shin 2014-11-17 05:42:48 PST
Comment on attachment 241288 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsLayer.h:251
>> +    virtual void setContentsClippingMaskLayer(GraphicsLayer* layer) { m_contentsClippingMaskLayer = layer; }
> 
> You need to look at what I did for Mac in r175794.
> On Mac/iOS we can use the existing contentsClippingLayer for border-radius clip, since CA supports clipping via a layer's corner radius. We use a mask shape layer in cases where that's not possible.
> 
> Because of that it's not appropriate to add a contentsClippingMaskLayer for all platforms here in the base class; you need to do this in a way that lets platforms optimally clip.

At this time, we only consider Fix clipping compositing decendents of an accelerated layer having border radius and clip overflow issue. So we removed contentsClippingMaskLayer.

>> Source/WebCore/rendering/RenderBox.cpp:1490
>> +}
> 
> I don't understand why any new code in renderers is required.

Please check following review history of https://codereview.chromium.org/19954010. 
=========================================================================================
On 2013/07/29 19:17:18, eseidel wrote:
> I'm confused by what this new paint-phase means.  It doesn't look like it
> interacts with SVG at all (its RenderBlock specific?) is that OK?
> 
> Historically the PaintPhases were all defined by CSS paint order, but I guess 
> that's no longer true:
> http://www.w3.org/TR/CSS21/zindex.html#painting-order

This paint phase is valid only for hardware composited elements and it's called
on an auxiliary layer, so this phase is never mixed up with the main layer's
phases. The auxiliary layer is set up as a mask only for the composited elements
which should clip overflow using an irregular clip path (border-radius and,
later, clip-path), and only if there is any composited child to be clipped. I
think this is RenderBlock specific, isn't it? I should probably check that
renderer() is RenderBlock before painting this phase.

>> Source/WebCore/rendering/RenderLayerBacking.cpp:94
>> +static inline bool isAcceleratedContents(const RenderObject& renderer)
> 
> This is a bad name: should be hasAcceleratedContents()

It has been removed.
Comment 8 Chris Dumez 2014-11-17 09:50:53 PST
Comment on attachment 241702 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Fix Border-radius clipping issue on a composited decendent

"descendant"
Comment 9 Byungseon Shin 2014-11-17 10:22:46 PST
Comment on attachment 241702 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Fix Border-radius clipping issue on a composited decendent
> 
> "descendant"

@Chris, thanks for the correction.
Comment 10 Byungseon Shin 2014-11-17 11:05:31 PST
Created attachment 241725 [details]
Patch
Comment 11 Byungseon Shin 2014-12-15 06:29:28 PST
Created attachment 243292 [details]
Patch
Comment 12 Byungseon Shin 2014-12-15 06:52:30 PST
Created attachment 243293 [details]
Patch
Comment 13 Simon Fraser (smfr) 2014-12-15 13:56:18 PST
Comment on attachment 243293 [details]
Patch

This is closer, but I still want a way to pass a Path down to GraphicsLayer for clipping, since on Mac/iOS we are able to clip to a path without a buffer.
Comment 14 Byungseon Shin 2014-12-15 16:00:57 PST
I made it works without adding MaskLayer when GraphicsLayerCA::applyClippingBorder of clippingLayer returns true. And it uses setCornerRadius of GraphicsLayerCA.

Do you want make it works even without clippingLayer as well?
Comment 15 Byungseon Shin 2014-12-22 11:26:21 PST
Created attachment 243630 [details]
Patch
Comment 16 Simon Fraser (smfr) 2014-12-22 16:22:41 PST
Comment on attachment 243630 [details]
Patch

This looks good, but need some tests. These tests should cover:
* adding and removing border-radius on some ancestor of a compositing layer
* changing between uniform to non-uniform corner radius
* nested ancestors with border radius, falling back to the mask code path.

It might be hard to get ref tests that work, but we should at least exercise all code paths.
Comment 17 Byungseon Shin 2015-01-01 03:17:30 PST
Created attachment 243860 [details]
Patch
Comment 18 Byungseon Shin 2015-01-01 03:20:11 PST
Created attachment 243861 [details]
Test for changing between uniform to non-uniform corner radius & removing border-radius.
Comment 19 Byungseon Shin 2015-01-01 03:24:35 PST
(In reply to comment #16)
> Comment on attachment 243630 [details]
> Patch
> 
> This looks good, but need some tests. These tests should cover:
> * adding and removing border-radius on some ancestor of a compositing layer
> * changing between uniform to non-uniform corner radius
> * nested ancestors with border radius, falling back to the mask code path.
> 
> It might be hard to get ref tests that work, but we should at least exercise
> all code paths.

@Simon, Happy Newyear!
I have created test case which covers your comments and updated patch as well.
Please refer "Test for changing between uniform to non-uniform corner radius & removing border-radius." 

I am now working on to convert this test case into refTest.
Comment 20 Byungseon Shin 2015-01-01 03:52:57 PST
Created attachment 243862 [details]
Patch
Comment 21 Simon Fraser (smfr) 2015-01-06 10:40:17 PST
Comment on attachment 243862 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2059
> +    m_layer->setMask(nullptr);

This makes me wonder whether this new code plays nicely with masks (-webkit-mask etc). Have you tried masks and non-uniform rounded corners on the same element?
Comment 22 Byungseon Shin 2015-01-06 16:20:35 PST
Comment on attachment 243862 [details]
Patch

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

>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2059
>> +    m_layer->setMask(nullptr);
> 
> This makes me wonder whether this new code plays nicely with masks (-webkit-mask etc). Have you tried masks and non-uniform rounded corners on the same element?

This code for clearing caMaskLayer of child(descendants)ClippingLayer, so it does not related with mask Layer itself. 
Yes, I have verified this code works well with non-uniform rounded corner and with mask layers.
Comment 23 WebKit Commit Bot 2015-01-07 02:22:21 PST
Comment on attachment 243862 [details]
Patch

Clearing flags on attachment: 243862

Committed r178029: <http://trac.webkit.org/changeset/178029>
Comment 24 WebKit Commit Bot 2015-01-07 02:22:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Csaba Osztrogonác 2015-01-09 03:05:32 PST
(In reply to comment #23)
> Comment on attachment 243862 [details]
> Patch
> 
> Clearing flags on attachment: 243862
> 
> Committed r178029: <http://trac.webkit.org/changeset/178029>

It caused a serious regression on EFL: bug140289
Comment 26 Simon Fraser (smfr) 2015-01-15 20:25:01 PST
This isn't working correctly at all. See bug 140536, bug 140535
Comment 27 Byungseon Shin 2015-01-15 20:54:53 PST
I will check those issues and update soon.
Comment 28 Simon Fraser (smfr) 2015-01-16 15:10:51 PST
What's the invalidation model for that mask layer? It needs to get repainted whenever any of the ancestors that affect the clip change, but I don't see that happening anywhere.
Comment 29 Simon Fraser (smfr) 2015-01-16 15:41:19 PST
There are also issues in the mac code path when an element has both CSS clip: and border-radius.
Comment 30 Simon Fraser (smfr) 2015-01-16 15:58:33 PST
Also, when I make Mac take the clipping mask code path, it asserts:
* thread #1: tid = 0xde2f, 0x000000010370ecca JavaScriptCore`WTFCrash + 42 at Assertions.cpp:321, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
    frame #0: 0x000000010370ecca JavaScriptCore`WTFCrash + 42 at Assertions.cpp:321
  * frame #1: 0x00000001065109d3 WebCore`WebCore::RenderLayerBacking::paintIntoLayer(this=0x0000000117ee1d20, graphicsLayer=0x0000000117748720, context=0x00007fff5fbfb078, paintDirtyRect=0x00007fff5fbfada0, paintBehavior=0, paintingPhase=32) + 131 at RenderLayerBacking.cpp:2178
    frame #2: 0x0000000106511001 WebCore`WebCore::RenderLayerBacking::paintContents(this=0x0000000117ee1d20, graphicsLayer=0x0000000117748720, context=0x00007fff5fbfb078, paintingPhase=32, clip=0x00007fff5fbfaec8) + 769 at RenderLayerBacking.cpp:2264
    frame #3: 0x000000010568f5db WebCore`WebCore::GraphicsLayer::paintGraphicsLayerContents(this=0x0000000117748720, context=0x00007fff5fbfb078, clip=0x00007fff5fbfb170) + 171 at GraphicsLayer.cpp:351
    frame #4: 0x000000010569f5ac WebCore`WebCore::GraphicsLayerCA::platformCALayerPaintContents(this=0x0000000117748720, (null)=0x0000000117ee2eb0, context=0x00007fff5fbfb078, clip=0x00007fff5fbfb170) + 44 at GraphicsLayerCA.cpp:1257
    frame #5: 0x000000010569f5ff WebCore`non-virtual thunk to WebCore::GraphicsLayerCA::platformCALayerPaintContents(this=0x0000000117748978, (null)=0x0000000117ee2eb0, context=0x00007fff5fbfb078, clip=0x00007fff5fbfb170) + 63 at GraphicsLayerCA.cpp:1258

This patch has too many problems. I'm going to roll it out.
Comment 31 Simon Fraser (smfr) 2015-01-16 15:59:03 PST
Created attachment 244814 [details]
Testcase that asserts on non-Mac
Comment 32 Byungseon Shin 2015-01-16 17:17:15 PST
I have fixed same assertion issue of GTK and EFL on https://bugs.webkit.org/show_bug.cgi?id=140336.
It caused when the patch create childMaskLayer when there is border radius.
I will check on Mac using debug build.
Comment 33 Byungseon Shin 2015-01-16 17:17:50 PST
(In reply to comment #32)
> I have fixed same assertion issue of GTK and EFL on
> https://bugs.webkit.org/show_bug.cgi?id=140336.
> It caused when the patch create childMaskLayer when there is no border radius.
> I will check on Mac using debug build.
Comment 34 Byungseon Shin 2015-01-16 18:40:21 PST
Reopening to attach new patch.
Comment 35 Byungseon Shin 2015-01-16 18:40:26 PST
Created attachment 244830 [details]
Patch
Comment 36 Simon Fraser (smfr) 2015-01-16 19:19:57 PST
Comment on attachment 244830 [details]
Patch

Can this be tested?
Comment 37 Byungseon Shin 2015-01-18 23:18:43 PST
(In reply to comment #36)
> Comment on attachment 244830 [details]
> Patch
> 
> Can this be tested?

Yes, I have verified it on GTK port.
Comment 38 Simon Fraser (smfr) 2015-01-19 10:35:44 PST
(In reply to comment #37)
> (In reply to comment #36)
> > Comment on attachment 244830 [details]
> > Patch
> > 
> > Can this be tested?
> 
> Yes, I have verified it on GTK port.

What I meant was "can you make a layout test for this".
Comment 39 Byungseon Shin 2015-01-19 10:51:14 PST
Oh, I misunderstood. I will try it.
Comment 40 Byungseon Shin 2015-01-25 09:00:50 PST
Created attachment 245307 [details]
Patch
Comment 41 Byungseon Shin 2015-01-25 09:03:44 PST
I have added ref-test to verify this bug.
Comment 42 WebKit Commit Bot 2015-01-26 14:59:42 PST
Comment on attachment 245307 [details]
Patch

Clearing flags on attachment: 245307

Committed r179147: <http://trac.webkit.org/changeset/179147>
Comment 43 WebKit Commit Bot 2015-01-26 14:59:51 PST
All reviewed patches have been landed.  Closing bug.