-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Fix combining of ClipPath child elements on Android #2198
base: main
Are you sure you want to change the base?
fix: Fix combining of ClipPath child elements on Android #2198
Conversation
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. Related commits: 766926f a1097b8
@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? |
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 |
I'm not sure whether it's better to be consistent between Android and iOS or at least fix clipPath on Android |
Do you mean I thought of maybe running EDIT: It's not quite as straightforward as that, but it sort of works with a bit of tweaking: RNSVG is on top and the WebView is below it. This one is testing |
Here's one that's relevant to this PR:
|
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
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
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
clipRule
s on theClipPath
'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 individualclipRule
s, 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.
And according to ChatGPT, that is what SVGKit does:
Related commits
766926f
a1097b8
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 themain
branch (i.e. before my change. My fix is completely unrelated to this file):After adding
// eslint-disable-next-line @typescript-eslint/no-explicit-any
on the previous line, everything is happy with my changes applied:yarn jest
failed before my change. I ranyarn jest -u
on themain
branch to update the snapshots.After that,
yarn jest
passes on the branch containing my change.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:
Before the fix the result looks like this:
After the fix it looks like this:
Compatibility
Checklist
README.md
__tests__
folder