Bug 138551 – Border-radius does not clip a composited descendant

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Byungseon(Sun) Shin]

Created attachment 241264 [details]
Clip compositing for animating decendent test

Clip compositing does not work for animating decendent on attached test case.

[Simon Fraser (smfr)]

Known issue.

[Byungseon(Sun) Shin]

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

[Simon Fraser (smfr)]

I'm not working on a patch, but please note that http://trac.webkit.org/changeset/175794 changed some code in this area.

[Byungseon(Sun) Shin]

Created attachment 241288 [details]
Patch

[Simon Fraser (smfr)]

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()

[Byungseon(Sun) Shin]

Created attachment 241702 [details]
Patch

[Byungseon(Sun) Shin]

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.

[Chris Dumez]

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"

[Byungseon(Sun) Shin]

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.

[Byungseon(Sun) Shin]

Created attachment 241725 [details]
Patch

[Byungseon(Sun) Shin]

Created attachment 243292 [details]
Patch

[Byungseon(Sun) Shin]

Created attachment 243293 [details]
Patch

[Simon Fraser (smfr)]

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.

[Byungseon(Sun) Shin]

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?

[Byungseon(Sun) Shin]

Created attachment 243630 [details]
Patch

[Simon Fraser (smfr)]

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.

[Byungseon(Sun) Shin]

Created attachment 243860 [details]
Patch

[Byungseon(Sun) Shin]

Created attachment 243861 [details]
Test for changing between uniform to non-uniform corner radius & removing border-radius.

[Byungseon(Sun) Shin]

(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.

[Byungseon(Sun) Shin]

Created attachment 243862 [details]
Patch

[Simon Fraser (smfr)]

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?

[Byungseon(Sun) Shin]

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.

[WebKit Commit Bot]

Comment on attachment 243862 [details]
Patch

Clearing flags on attachment: 243862

Committed r178029: <http://trac.webkit.org/changeset/178029>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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

[Simon Fraser (smfr)]

This isn't working correctly at all. See bug 140536, bug 140535

[Byungseon(Sun) Shin]

I will check those issues and update soon.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

There are also issues in the mac code path when an element has both CSS clip: and border-radius.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 244814 [details]
Testcase that asserts on non-Mac

[Byungseon(Sun) Shin]

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.

[Byungseon(Sun) Shin]

(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.

[Byungseon(Sun) Shin]

Reopening to attach new patch.

[Byungseon(Sun) Shin]

Created attachment 244830 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 244830 [details]
Patch

Can this be tested?

[Byungseon(Sun) Shin]

(In reply to comment #36)
> Comment on attachment 244830 [details]
> Patch
> 
> Can this be tested?

Yes, I have verified it on GTK port.

[Simon Fraser (smfr)]

(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".

[Byungseon(Sun) Shin]

Oh, I misunderstood. I will try it.

[Byungseon(Sun) Shin]

Created attachment 245307 [details]
Patch

[Byungseon(Sun) Shin]

I have added ref-test to verify this bug.

[WebKit Commit Bot]

Comment on attachment 245307 [details]
Patch

Clearing flags on attachment: 245307

Committed r179147: <http://trac.webkit.org/changeset/179147>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.