feat: Add clip-path support for Android

[paradowstack]

Summary:

This PR adds support for clip-path CSS attribute for JS. It follows CSS spec described here. It does not provide support for SVG source (<clip-source> values), what can be added in the additional PR if needed. Supported syntax is [<basic-shape>] || <geometry-box>] with almost full support of every basic shape (<basic-shape> = circle | ellipse | rect | polygon | inset | xywh) and references boxes (<geometry-box> = margin-box | border-box | padding-box | content-box | fill-box | stoke-box | view-box).

Work has been split into three PRs for more convenient reviewing process. This part adds Android support for clipping, path and bounds calculations.

Changelog:

[ANDROID] [ADDED] - Add clip-path support for Android

Test Plan:

Merge JS and iOS PRs and run RNTester app.

• Test Clip Path screen in RNTester app (ClipPathExample.js)

[meta-codesync[bot]]

@jorge-cab has imported this pull request. If you are a Meta employee, you can view this in D88081136.

[paradowstack]

Thanks @jorge-cab for the comments and review!

We can try adding a parameter to CompositeBackgroundDrawable's constructor for clipPath. Then let's override draw and clip the canvas there, after which we can call super.draw() don't save/restore, this way the clipping will persist through the entire view's drawing logic since the canvas is shared between the background and the view itself. ... I'm pretty sure backgrounds are drawn before any of the view's drawing operations so the canvas should remain clipped for any subsequent operations.

Generally I definitely like your proposed solution much more, but I have tried this approach initially but couldn't get it to work - children were not clipped, only background was. I noticed that BackgroundStyleApplicator.clipToPaddingBox() function is used in several View classes and followed went that way.

I made a second attempt today and ended up with the same result. After the investigation and debugging I noticed that the reason why children may not be clipped is hardware acceleration. In this case drawing of background is isolated and canvas state not shared with the View's drawing (see drawBackground function of View.java). At least this is my understanding so far, I will try to validate that and investigate further but would like to hear you thoughts on that.

[paradowstack]

I took a look again at solution leveraging CompositeBackgroundDrawable by overriding draw method. In order to get it to work (clip children correctly) I got to turn off the hardware acceleration. Without that, canvas state is not shared between background and the rest, that is the View and children. What's more, I failed to do this only for one specific View, I got to turn it off for the whole app. I believe it also can be achieved by turning it off for the Window, but both ways are no-gos.

However, I was able to get working solution by adding a clip path to getOutline (already overridden in CompositeBackgroundDrawable) and setting setClipToOutline on View to true. What do you think about this approach? Can you see any potential problems with this solution? @jorge-cab

[jorge-cab]

However, I was able to get working solution by adding a clip path to getOutline (already overridden in CompositeBackgroundDrawable) and setting setClipToOutline on View to true. What do you think about this approach? Can you see any potential problems with this solution? @jorge-cab

@paradowstack I think this could potentially conflict with other things that do depend on the outline. Stuff like elevation, I think your current approach while it feels unideal is ultimately fine IMO. The main thing that keeps me from stamping this is that we are setting margins for every View just to get the margin information for clipPath.

I'm not really sure what the solution would be here. A JNI call would run the risk of getting the layout information of a stale shadow tree. This goes out of my comfort zone but you might have to do something on FabricMountingManager so that on commit of the shadow tree we can conditional record the margins somewhere in way that doesn't cost anything to views without clipPath

Sorry I'm having such a long turnaround time for this.

[paradowstack]

@jorge-cab I have provided a solution for storing computed layout information. Below you can find an explanation of some decisions and open questions:

• First of all, not only margins have to be recorded but paddings as well. Paddings are used for content-box geometry. In the previous solution I also added a setPadding function to ReactViewManager.kt and propagated information to Android's View - now this is gone and I do not interact with Android's layout system.
• From the Java side, stored layout information is obtained through FabricUIManagerBinding and UIManagerHelper classes.
• I decided to store margin and padding information in a new class ComputedBoxModelRegistry. It lives in the FabricUIManagerBinding class so it can be easily accessed during JNI calls. It is passed down to FabricMountingManager. It has a straightforward easy thread-safe API (store/get/remove). I am open to change this class name, location or API - just let me know.
• FabricMountingManager stores layout information in computedBoxModelRegistry_ only if ShadowView has the clipPath property set, or removes it if clipPath has been unset. Right now detection of clipPath is achieved by casting props to BaseViewProps and checking if the clipPath member is set. Detection happens once for ShadowViewMutation::Insert and twice for ShadowViewMutation::Update case. However, property detection can be solved in several ways:
  • As now - casting and checking std::optional property
  • Looking up hash map rawProps - doesn't require casting, but is more expensive
  • Checking for a new trait - add a new ShadowNodeTrait that is set once in ViewShadowNode::initialize based on the clipPath property and read later. That would definitely be the most efficient way, but also the most intrusive and potentially overkill. Please let me know what do you think about it, which solution to detection you like the most?
• Margins and paddings are computed in the layoutMetricsFromYogaNode(yoga::Node &yogaNode) function and stored for every ShadowView. Computation cannot take place in any Fabric class (e.g., FabricMountingManager) since the yoga node's layout is required (use of YGNodeLayoutGetMargin/Padding()) and it's not exposed outside.
• I decided to compute and store margin and padding for every LayoutMetrics. I think it is cheaper and cleaner than conditionally checking if clipPath is set. What is more, this information can be used for iOS or in other cases, and LayoutMetrics already contains similar properties (contentInsets, overflowInsets, borderWidth). However, there are of course other possibilities. FabricMountingManager needs to get those fields from ShadowView instances in the executeMount() function, so I thought that the best solution would be to extend the LayoutMetrics class. I am curious what your thoughts are on it?

[jorge-cab]

Checking for a new trait - add a new ShadowNodeTrait that is set once in ViewShadowNode::initialize based on the clipPath property and read later. That would definitely be the most efficient way, but also the most intrusive and potentially overkill. Please let me know what do you think about it, which solution to detection you like the most?

@paradowstack I like the idea of the new trait better since we can avoid having to cast to BaseViewProps on every check/mount, added a comment.

Why do you think it would be overkill/intrusive?

[jorge-cab]

I decided to compute and store margin and padding for every LayoutMetrics. I think it is cheaper and cleaner than conditionally checking if clipPath is set. What is more, this information can be used for iOS or in other cases, and LayoutMetrics already contains similar properties (contentInsets, overflowInsets, borderWidth). However, there are of course other possibilities. FabricMountingManager needs to get those fields from ShadowView instances in the executeMount() function, so I thought that the best solution would be to extend the LayoutMetrics class. I am curious what your thoughts are on it?

@paradowstack My concern would be that LayoutMetrics is a very often used class. I can imagine some concern about increasing the memory requirements for every LayoutMetrics class just for a single prop.

It does seem logical to have it there but I think we'll have a better chance to get this landed if we avoid extending often used classes.

We can revisit this if we end up adding more css props that need this information readily available and that get widely adopted, but for now I'd advice against this

I'll get a second opinion on this and update here