Bug 115613

Summary:

GraphicsLayers should be aware of rounded rect clipping

Product:

WebKit

Reporter:

Noam Rosenthal <noam>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ahmad.saleem792, ap, benjamin, bfulgham, buildbot, commit-queue, dino, eflews.bot, esprehn+autocc, fujii.hironori, glenn, gyuyoung.kim, kondapallykalyan, rniwa, simon.fraser, zalan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

115614

Bug Blocks:

115616, 114638, 115615

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	simon.fraser: review-

Noam Rosenthal

Reported 2013-05-05 15:16:46 PDT

When a GraphicsLayer is clipped by an ancestor with a border radius, or when it has its own border radius and composited contents like canvas, the border radius information should be passed to the port-specific compositor.

Attachments
Patch (13.71 KB, patch) 2013-05-05 15:34 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (32.74 KB, patch) 2013-05-06 04:15 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (17.65 KB, patch) 2013-05-08 06:49 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (18.88 KB, patch) 2013-06-06 01:38 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (18.88 KB, patch) 2013-06-06 02:35 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (18.21 KB, patch) 2013-06-06 02:42 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (18.05 KB, patch) 2013-10-14 00:53 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (18.08 KB, patch) 2013-10-15 00:56 PDT, Noam Rosenthal	simon.fraser: review-	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Noam Rosenthal

Comment 1 2013-05-05 15:34:52 PDT

Created attachment 200597 [details] Patch

Benjamin Poulain

Comment 2 2013-05-05 19:47:24 PDT

Comment on attachment 200597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200597&action=review > LayoutTests/TestExpectations:15 > +# pending border-radius support in port-specific compositors > +webkit.org/b/115613 compositing/border-radius This is a bad idea. See the recent threads about testing on webkit-dev. > LayoutTests/compositing/border-radius/composited-border-radius-descendant-large-radius.html:3 > +<html> > + <head> > + <style> Tabs in tests? Heresy! :)

Noam Rosenthal

Comment 3 2013-05-06 00:16:37 PDT

(In reply to comment #2) > (From update of attachment 200597 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200597&action=review > > > LayoutTests/TestExpectations:15 > > +# pending border-radius support in port-specific compositors > > +webkit.org/b/115613 compositing/border-radius > > This is a bad idea. > > See the recent threads about testing on webkit-dev. As I recall that thread was more about tests that work but need rebaseline. In this case the feature is not ready (it's not ready until at least one compositor supports it), but I wanted the tests to be there for reference. What do you propose?

Noam Rosenthal

Comment 4 2013-05-06 04:15:24 PDT

Created attachment 200658 [details] Patch

Benjamin Poulain

Comment 5 2013-05-07 20:43:45 PDT

> As I recall that thread was more about tests that work but need rebaseline. The problem is TestExpectations is growing faster than it is shrinking. Skipped tests are the worse because we have no idea what is going on in that case. > In this case the feature is not ready (it's not ready until at least one compositor supports it), but I wanted the tests to be there for reference. What do you propose? At a minimum, marking them as failing, not skipped.

Noam Rosenthal

Comment 6 2013-05-08 06:49:40 PDT

Created attachment 201069 [details] Patch

Noam Rosenthal

Comment 7 2013-05-27 01:08:51 PDT

Any chance to get this reviewed?

Simon Fraser (smfr)

Comment 8 2013-05-28 14:11:55 PDT

Comment on attachment 201069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201069&action=review > Source/WebCore/rendering/RenderLayerBacking.cpp:730 > + RoundedRect roundedRect = roundedRectMask(renderer()); > + m_graphicsLayer->setRoundedRectMask(roundedRect); This seems wrong. For a rounded-corner layer with box-shadow or an overflowing child, it will clip the shadow/child.

Noam Rosenthal

Comment 9 2013-06-06 01:38:34 PDT

Created attachment 203912 [details] Patch

Early Warning System Bot

Comment 10 2013-06-06 01:45:52 PDT

Comment on attachment 203912 [details] Patch Attachment 203912 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/712609

EFL EWS Bot

Comment 11 2013-06-06 01:47:06 PDT

Comment on attachment 203912 [details] Patch Attachment 203912 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/716439

EFL EWS Bot

Comment 12 2013-06-06 01:48:11 PDT

Comment on attachment 203912 [details] Patch Attachment 203912 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/729869

Early Warning System Bot

Comment 13 2013-06-06 01:48:38 PDT

Comment on attachment 203912 [details] Patch Attachment 203912 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/777106

Build Bot

Comment 14 2013-06-06 02:24:00 PDT

Comment on attachment 203912 [details] Patch Attachment 203912 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/712615

Noam Rosenthal

Comment 15 2013-06-06 02:35:06 PDT

Created attachment 203917 [details] Patch

EFL EWS Bot

Comment 16 2013-06-06 02:40:55 PDT

Comment on attachment 203917 [details] Patch Attachment 203917 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/747906

Noam Rosenthal

Comment 17 2013-06-06 02:42:59 PDT

Created attachment 203918 [details] Patch

Noam Rosenthal

Comment 18 2013-06-25 13:32:07 PDT

Ping review?

Alexandru Chiculita

Comment 19 2013-07-16 06:33:45 PDT

Comment on attachment 203918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203918&action=review > Source/WebCore/ChangeLog:9 > + This is done by adding new roundedRectMask/setRoundedRectMask properties to GraphicsLayer. I think some compositors would need to use the bitmap masking instead. Maybe we should have an ifdef around it. For example CALayer has a cornerRadius property, but cannot target each corner independently. Somehow related to this patch, I was thinking to fix the "clip-path" property for composited layers. The idea was to force the mask even if it is not specified. Then it would generate the mask by clipping the context and filling the mask with black. > Source/WebCore/rendering/RenderLayerBacking.cpp:596 > + return style->getRoundedBorderFor(toRenderBox(renderer)->pixelSnappedBorderBoxRect()); I think you need to use getRoundedInnerBorderFor instead. At least that's what RenderBox::pushContentsClip is doing. > LayoutTests/ChangeLog:11 > + * TestExpectations: Added expectations for new tests. I would expose the rounded rects in the graphics layer dump. That way we could have some real expected results.

Noam Rosenthal

Comment 20 2013-07-16 06:43:06 PDT

Comment on attachment 203918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203918&action=review >> Source/WebCore/ChangeLog:9 >> + This is done by adding new roundedRectMask/setRoundedRectMask properties to GraphicsLayer. > > I think some compositors would need to use the bitmap masking instead. Maybe we should have an ifdef around it. For example CALayer has a cornerRadius property, but cannot target each corner independently. > > Somehow related to this patch, I was thinking to fix the "clip-path" property for composited layers. The idea was to force the mask even if it is not specified. Then it would generate the mask by clipping the context and filling the mask with black. I agree that using masks for fallback clipping is a good idea. In this case smfr has asked that we leave this decision to a lower level (e.g. GraphicsLayerCA), either seem reasonable. >> Source/WebCore/rendering/RenderLayerBacking.cpp:596 >> + return style->getRoundedBorderFor(toRenderBox(renderer)->pixelSnappedBorderBoxRect()); > > I think you need to use getRoundedInnerBorderFor instead. At least that's what RenderBox::pushContentsClip is doing. right, good catch. We'll need a test for that as well with border-radius+border-width. >> LayoutTests/ChangeLog:11 >> + * TestExpectations: Added expectations for new tests. > > I would expose the rounded rects in the graphics layer dump. That way we could have some real expected results. I was going to add ref-tests when TextureMapper or any other compositor supports it.

Noam Rosenthal

Comment 21 2013-10-14 00:53:52 PDT

Created attachment 214131 [details] Patch

Darin Adler

Comment 22 2013-10-14 10:30:42 PDT

Comment on attachment 214131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214131&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:312 > + virtual void setChildrenRoundedRectMask(const RoundedRect& r) { m_childrenRoundedRectMask = r; } I suggest naming the argument "mask" instead of "r". > Source/WebCore/platform/graphics/GraphicsLayer.h:317 > + virtual void setContentsRoundedRectMask(const RoundedRect& r) { m_contentsRoundedRectMask = r; } I suggest naming the argument "mask" instead of "r". > Source/WebCore/rendering/RenderLayerBacking.cpp:588 > +static RoundedRect roundedRectMask(const RenderObject& renderer) The type here should be RenderLayerModelObject, not RenderObject, at least, since that’s the type returned from RenderLayerBacking::renderer. But it seems we know this is a RenderBox? > Source/WebCore/rendering/RenderLayerBacking.cpp:594 > + return style->getRoundedInnerBorderFor(toRenderBox(renderer).pixelSnappedBorderBoxRect()); What guarantees the renderer is a RenderBox? If the caller has that guarantee, then can we put this cast at the caller's level? > Source/WebCore/rendering/RenderLayerBacking.cpp:711 > + RoundedRect roundedRect = roundedRectMask(renderer()); I would call this mask, not roundedRect. Unless we have other masks, in which case maybe roundedRectMask.

Noam Rosenthal

Comment 23 2013-10-15 00:56:03 PDT

Created attachment 214240 [details] Patch

Darin Adler

Comment 24 2013-10-15 13:54:59 PDT

Comment on attachment 214240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214240&action=review > Source/WebCore/rendering/RenderLayerBacking.cpp:711 > + RoundedRect roundedRectMask = getRoundedRectMask(toRenderBox(renderer())); What guarantees the renderer is a box? Maybe it’s obvious to a rendering expert, but I don’t see any other code here casting to RenderBox.

Noam Rosenthal

Comment 25 2013-10-15 13:57:25 PDT

Comment on attachment 214240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214240&action=review >> Source/WebCore/rendering/RenderLayerBacking.cpp:711 >> + RoundedRect roundedRectMask = getRoundedRectMask(toRenderBox(renderer())); > > What guarantees the renderer is a box? Maybe it’s obvious to a rendering expert, but I don’t see any other code here casting to RenderBox. clippingBox = clipBox(toRenderBox(renderer())); a few lines later?

Ryosuke Niwa

Comment 26 2013-10-16 01:00:54 PDT

*** Bug 122880 has been marked as a duplicate of this bug. ***

Simon Fraser (smfr)

Comment 27 2013-12-17 10:49:29 PST

Comment on attachment 214240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214240&action=review > Source/WebCore/rendering/RenderLayerBacking.cpp:588 > +static RoundedRect getRoundedRectMask(const RenderBox& renderBox) We don't normally use 'get' in a function that returns a value. > Source/WebCore/rendering/RenderLayerBacking.cpp:712 > + m_graphicsLayer->setContentsRoundedRectMask(roundedRectMask); I think it would be better to avoid calling setContentsRoundedRectMask() unless there really is a rounded mask to use. > Source/WebCore/rendering/RenderLayerBacking.cpp:721 > + clipLayer->setChildrenRoundedRectMask(roundedRectMask); Ditto.

Noam Rosenthal

Comment 28 2013-12-17 13:42:43 PST

Un-assigning to default. I'm not working on this stuff anymore.

Noam Rosenthal

Comment 29 2013-12-17 13:43:16 PST

Unassigning. I'm not going to submit more patches for this, if someone else want to you're welcome to it.

Ahmad Saleem

Comment 30 2022-08-04 17:08:29 PDT

I took test cases from this patch and it shows different behavior of Safari 15.6 compared to other browsers: Test Case 1 - border-radius-webgl.html - https://jsfiddle.net/hfjb9gey/show In Safari 15.6, above test case only show single green box with rounder corners while in other browsers, it shows two. Test Case 2 - composited-border-radius-descendant-large-radius.html - (Removed -webkit- prefix) https://jsfiddle.net/oLpf3z0n/show ^This is working same across all browsers (Chrome Canary 106, Firefox Nightly 105 and Safari 15.6) Test Case 3 - composited-border-radius-descendant-uneven.html - https://jsfiddle.net/L9fhjy7e/show ^This is working same across all browsers (Chrome Canary 106, Firefox Nightly 105 and Safari 15.6) Test Case 4 - composited-border-radius-descendant.html - https://jsfiddle.net/pbysd0te/show ^This is working same across all browsers (Chrome Canary 106, Firefox Nightly 105 and Safari 15.6) Test Case 5 - composited-border-radius.html - https://jsfiddle.net/aprxcnw7/show ^This is working same across all browsers (Chrome Canary 106, Firefox Nightly 105 and Safari 15.6) _______ Except first case, Safari 15.6 is working same as other browsers in above test cases, is something required or this can be marked as "RESOLVED LATER" and I can create separate bug for first case (but I doubt I have seen similar case of as such in other bug). Thanks!

Fujii Hironori

Comment 31 2022-08-06 14:43:16 PDT

Created a new ticket for Test Case 1. Bug 243629 – composited canvas element should update the layer configuration after creating a WebGL context

Ahmad Saleem

Comment 32 2022-08-31 14:43:02 PDT

(In reply to Fujii Hironori from comment #31) > Created a new ticket for Test Case 1. > Bug 243629 – composited canvas element should update the layer > configuration after creating a WebGL context Can we close this since the bug 243629 is not resolved? Thanks!

Ahmad Saleem

Comment 33 2022-08-31 14:43:34 PDT

(In reply to Ahmad Saleem from comment #32) > (In reply to Fujii Hironori from comment #31) > > Created a new ticket for Test Case 1. > > Bug 243629 – composited canvas element should update the layer > > configuration after creating a WebGL context > > Can we close this since the bug 243629 is not resolved? Thanks! Is resolved.. Stupid silly typo..

Fujii Hironori

Comment 34 2022-08-31 14:46:42 PDT

Yup. Resolved.

Note You need to log in before you can comment on or make changes to this bug.