react-native-svg icon indicating copy to clipboard operation
react-native-svg copied to clipboard

fix: Fix combining of ClipPath child elements on Android

Open wodin opened this issue 1 year ago • 5 comments

Summary

Fixes combining of ClipPath child elements to match the spec and browsers, Inkscape, etc. The problem affects both Android and iOS, but this PR currently only fixes it on Android I started looking at the iOS code, but got a bit lost

Relevant issues

  • #983
  • #1520

The code was conflating clipRules with how child elements need to be combined. These are unrelated. Children always need to be combined using UNION

From https://www.w3.org/TR/SVG11/masking.html#EstablishingANewClippingPath

When the ‘clipPath’ element contains multiple child elements, the silhouettes of the child elements are logically OR'd together to create a single silhouette which is then used to restrict the region onto which paint can be applied.

iOS

After spending some more time trying to understand the iOS code it looks to me like it does not keep track of the individual clipRules on the ClipPath's child elements when adding them together. I believe this is part of the problem. As mentioned above, all of the child elements' silhouettes need to be OR'd (or UNION'd) together. The clipRule affects what the silhouette of each child element looks like. So one needs to keep track of the individual clipRules, or else somehow calculate the silhouette while adding the child elements together.

Given that the SVG spec says the following, maybe this can be implemented in terms of masks.

A clipping path can be thought of as a mask wherein those pixels outside the clipping path are black with an alpha value of zero and those pixels inside the clipping path are white with an alpha value of one (with the possible exception of anti-aliasing along the edge of the silhouette).

And according to ChatGPT, that is what SVGKit does:

  1. Path Translation: SVGKit translates the SVG paths specified in clipPath elements into CGPath objects. This includes all the path commands and sub-paths.
  2. Handling clip-rule: The clip-rule property in SVG, which can be nonzero or evenodd, affects how the interior of a path is determined. In Core Graphics, when creating a CGPath, you can specify the fill rule (kCGPathFillRuleEvenOdd or kCGPathFillRuleNonZero). This allows SVGKit to respect the clip-rule specified in the SVG file when creating the mask.
  3. Creating Masks: Once the paths are translated, SVGKit uses these paths to create CAShapeLayer objects or similar constructs, which can act as masks. By setting the path of a CAShapeLayer and using it as a mask, it can effectively clip the content.
  4. Composite Masks for Multiple Elements: When a clipPath contains multiple elements, SVGKit combines them into a single mask. This is done by rendering each element into an offscreen bitmap and then using this composite image as a mask. The composite operation inherently respects the individual path's fill rules as they were rendered.
  5. Applying Masks: The mask is then applied to the relevant layers, effectively clipping them as per the clipPath definition.

Related commits

766926f75874dedd7b13dadf65315f7030c4234a a1097b85942c559ebcef6b93fa2ce601434d9c50

Note

This is a breaking change and the ClipPath example image will need to be updated in USAGE.

Test Plan

yarn test gave the following warning on the main branch (i.e. before my change. My fix is completely unrelated to this file):

/tmp/react-native-svg/src/css/LocalSvg.tsx
  26:34  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

After adding // eslint-disable-next-line @typescript-eslint/no-explicit-any on the previous line, everything is happy with my changes applied:

% yarn test
yarn run v1.22.19
$ npm run lint && npm run tsc

> [email protected] lint
> eslint --ext .ts,.tsx src


> [email protected] tsc
> tsc --noEmit

✨  Done in 5.09s.

yarn jest failed before my change. I ran yarn jest -u on the main branch to update the snapshots. After that, yarn jest passes on the branch containing my change.

% git checkout wodin/fix-combining-of-clip-path-children
M	__tests__/__snapshots__/css.test.tsx.snap
M	src/css/LocalSvg.tsx
Switched to branch 'wodin/fix-combining-of-clip-path-children'
% yarn jest
yarn run v1.22.19
$ jest
 PASS  __tests__/css.test.tsx
  ✓ inlines styles (6 ms)
  ✓ supports CSS in style element (8 ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   2 passed, 2 total
Time:        0.737 s, estimated 1 s
Ran all test suites.
✨  Done in 2.72s.

What's required for testing (prerequisites)?

N/A

What are the steps to reproduce (after prerequisites)?

Code like this reproduces the problem and demonstrates that the fix works:

    <Svg viewBox="0 0 250 234">
      <ClipPath id="clip">
        <Polygon points="31,0 31,234 170,234 250,0" />
        <Rect x="0" y="0" width="41" height="234" rx="8" ry="8" />
        <Rect x="0" y="112" width="250" height="10" />
      </ClipPath>
      <Rect x="0" y="0" width="250" height="234" fill="red" clipPath="url(#clip)" />
    </Svg>

Before the fix the result looks like this: Before

After the fix it looks like this: After

Compatibility

OS Implemented
iOS
Android

Checklist

  • [X] I have tested this on a device and a simulator
  • [ ] I added documentation in README.md
  • [ ] I updated the typed files (typescript)
  • [ ] I added a test for the API in the __tests__ folder

wodin avatar Dec 23 '23 11:12 wodin

@WoLewicki I would love your thoughts on this if you have any?

Unfortunately I have no experience with the iOS side of this, so struggled to get anywhere with it, and have run out of energy for the moment.

While looking into this I came across the SVG 1.1 Second Edition Test Suite

I wonder whether it would be useful to take the test cases from the test suite and display the RNSVG version and a WebView version next to each other for visual comparison?

wodin avatar Jan 05 '24 13:01 wodin

I am no expert in this code either unfortunately. As for displaying the web versions of the components next to the original ones, I am up for it 🚀 We also have a web version inside Example app so it can be also seen there. Do you want me to merge this one for start or to wait until the solution on iOS is also found to keep the behavior consistent?

WoLewicki avatar Jan 05 '24 14:01 WoLewicki

I'm not sure whether it's better to be consistent between Android and iOS or at least fix clipPath on Android

wodin avatar Jan 05 '24 14:01 wodin

We also have a web version inside Example app so it can be also seen there.

Do you mean react-native-web? OK. Although I think it might be easier to compare them if they're on the same screen

I thought of maybe running svgr during the build process to convert the .svg files from the test suite to components. I suppose we'd probably have to delete all the ones that contain animation? And other stuff like <set> and <script> etc.

EDIT: It's not quite as straightforward as that, but it sort of works with a bit of tweaking:

screenshot of Android Emulator showing RNSVG vs. WebView

RNSVG is on top and the WebView is below it. This one is testing currentColor.

wodin avatar Jan 05 '24 14:01 wodin

Here's one that's relevant to this PR:

masking-path-01-b.svg masking-path-01-b.svg

Pass Criteria

The test at the top shows an orange rectangle (with black stroke) being clipped by another rectangle. So only the middle portion of the orange rectangle should be visible. Also the black stroke should only be visible along the top and bottom edge of the rectangle.

The example at the bottom has a group containing a text string and two rectangles. The group has a clipping path defined using two overlapping rectangles. Of concern is the overlapping area shared by the two rectangles. There should not be holes in this overlapping area, the clip region is the union of the two rectangles. For clarity, guide rectangles in grey show the position of the clipping rectangles.

The rendered picture should match the reference image exactly, except for possible variations in the labelling text (per CSS2 rules).

Test Description

Test to see if the basic clipping works using the clipPath element and the clip-path property.

This test uses the following elements : <clipPath> and the following properties : clip-path.

wodin avatar Jan 05 '24 16:01 wodin