| 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, glenn, gyuyoung.kim, Hironori.Fujii, kondapallykalyan, rniwa, simon.fraser, zalan | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Bug Depends on: | 115614 | ||||||||||||||||||||
| Bug Blocks: | 115616, 114638, 115615 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Noam Rosenthal
2013-05-05 15:16:46 PDT
Created attachment 200597 [details]
Patch
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! :) (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? Created attachment 200658 [details]
Patch
> 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. Created attachment 201069 [details]
Patch
Any chance to get this reviewed? 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. Created attachment 203912 [details]
Patch
Comment on attachment 203912 [details] Patch Attachment 203912 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/712609 Comment on attachment 203912 [details] Patch Attachment 203912 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/716439 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 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 Comment on attachment 203912 [details] Patch Attachment 203912 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/712615 Created attachment 203917 [details]
Patch
Comment on attachment 203917 [details] Patch Attachment 203917 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/747906 Created attachment 203918 [details]
Patch
Ping review? 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. 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. Created attachment 214131 [details]
Patch
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. Created attachment 214240 [details]
Patch
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. 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? *** Bug 122880 has been marked as a duplicate of this bug. *** 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. Un-assigning to default. I'm not working on this stuff anymore. Unassigning. I'm not going to submit more patches for this, if someone else want to you're welcome to it. 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! Created a new ticket for Test Case 1. Bug 243629 – composited canvas element should update the layer configuration after creating a WebGL context (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! (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.. Yup. Resolved. |