Created attachment 241264 [details] Clip compositing for animating decendent test Clip compositing does not work for animating decendent on attached test case.
Known issue.
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
I'm not working on a patch, but please note that http://trac.webkit.org/changeset/175794 changed some code in this area.
Created attachment 241288 [details] Patch
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()
Created attachment 241702 [details] Patch
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 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 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.
Created attachment 241725 [details] Patch
Created attachment 243292 [details] Patch
Created attachment 243293 [details] Patch
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.
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?
Created attachment 243630 [details] Patch
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.
Created attachment 243860 [details] Patch
Created attachment 243861 [details] Test for changing between uniform to non-uniform corner radius & removing border-radius.
(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.
Created attachment 243862 [details] Patch
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 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 on attachment 243862 [details] Patch Clearing flags on attachment: 243862 Committed r178029: <http://trac.webkit.org/changeset/178029>
All reviewed patches have been landed. Closing bug.
(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
This isn't working correctly at all. See bug 140536, bug 140535
I will check those issues and update soon.
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.
There are also issues in the mac code path when an element has both CSS clip: and border-radius.
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.
Created attachment 244814 [details] Testcase that asserts on non-Mac
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.
(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.
Reopening to attach new patch.
Created attachment 244830 [details] Patch
Comment on attachment 244830 [details] Patch Can this be tested?
(In reply to comment #36) > Comment on attachment 244830 [details] > Patch > > Can this be tested? Yes, I have verified it on GTK port.
(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".
Oh, I misunderstood. I will try it.
Created attachment 245307 [details] Patch
I have added ref-test to verify this bug.
Comment on attachment 245307 [details] Patch Clearing flags on attachment: 245307 Committed r179147: <http://trac.webkit.org/changeset/179147>