fix: start and end points linear gradient algorithm
Summary:
- Current implementation does not follow CSS spec for rectangle boxes with corner angles. It is using non spec compliant algorithm to calculate start and end points. This PR follows the spec compliant algorithm to implement and makes sure Web, iOS and Android gradients are identical with corner angles.
- Also, currently it is using
CAGradientLayerwhich does not support spec compliant start and end points i.e. start and end point can be outside of rectangle bounds. This leads to inconsistent gradients on iOS for corner angles compared to web and android. So this PR replaces it withCGGradient. - I have also moved some files to make it easier to add more background image types in future.
Changelog:
[GENERAL] [FIXED] - Linear gradient start and end point algorithm.
Test Plan:
- Added multiple gradient example which should be identical in all platforms (Web, iOS and Android) and tested thoroughly on all platforms. I think some visual test cases can help here.
- I have referred to blink's implementation.
Aside
Took a while to understand the spec, but felt great after getting it. Gradients should be 100% identical on all platforms now. Sorry i missed testing cornered angles + rectangles earlier and I found out it is inconsistent on platforms just this weekend 😅
Catching up on context here, was there previous work from you to implement the linear gradient? Or this is a longstanding thing that is being addressed?
Do you have screenshots of the difference between the gradient? What's the con of using a non-standard algorithm today?
@lunaleaps Sure. Linear gradient was done in these PRs (https://github.com/facebook/react-native/pull/45434) and https://github.com/facebook/react-native/pull/45433. It is still behind experimental flag like box shadow and filters. The con of using non standard algo is I used simple unit square for calculation of start and end points which doesn't work well for rectangle boxes when angles are cornered. Something I found last week while testing 🤦♂️ so made this PR. This PR adds standard algo which makes it match to the output of web.
e.g. output of linear-gradient(45deg, red, blue) for box h100, w200 on web and before/after this PR fix. RN will match the web output.
| Web | React Native Before | React Native After |
|---|---|---|
Thanks @intergalacticspacehighway for the details and including the screenshots! Okay, glad to hear its behind an experimental flag -- also didn't realize @jorge-cab from the team is reviewing already and has more context!
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Pushed all fixes @jorge-cab. Thanks for the quick review! <3
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@intergalacticspacehighway I should mention that I'm starting to do work to implement the image features of background-image and potentially radial-gradient after so if you are interested in taking on radial-gradient lmk so that we don't step on each other.
@jorge-cab That's amazing to hear, thanks for informing. i was thinking to pick radial gradients after finishing transition hint syntax with linear gradient. I can implement radial gradiant by next weekend. Is that alright? if not you can pick it
@jorge-cab That's amazing to hear, thanks for informing. i was thinking to pick radial gradients after finishing transition hint syntax with linear gradient. I can implement radial gradiant by next weekend. Is that alright? if not you can pick it
Yeah, feel free to work on it. I don't even know when I'd start with radial gradient so no rush there haha
@jorge-cab Is anything pending on this PR. can we get this merged? I want to do some follow up PRs (px and transition hint support) that might be based on this one. Thanks 🙏
btw i have started doing some analysis for radial gradients. Currently it's a bit challenging with CSS spec as both iOS and Android's APIs lack ellipse shape support, circles are easy. I am still looking if there's a way.
Left a whole bunch of comments, but I’m also about to be on PTO for two weeks, and want to make sure I don’t block anything while I’m gone 😅.
I left a lot of nits/mechanical pieces that aren’t super critical, but the high level points I’m curious about are:
- A case where I wasn’t certain we were safely managing image lifetime
- Not wanting to keep strings around in prop storage layer where we have small finite set of possible values
- Making parsing on Android side graceful instead of throw
- Whether we could do drawing in layer delegate instead of always rasterizing to new bitmap (but the other things already do create a bitmap, even though I’m not sure they should be)
- Some z-index/clipping things in iOS side I want @joevilches to double check since there are concurrent changes happening there
@intergalacticspacehighway to give you some more context on delays after import, imported PRs need approver outside of engineer who imports. Sometimes an imported PR has been reviewed thoroughly by owner/expert before importing, sometimes it has gone through some initial pass and needs more eyes, and other times it hasn’t been reviewed at all. This means that this review internally usually carries a similar weight as the external one.
Larger changes usually spend longer on the review queue, especially those that touch multiple platforms (where say, someone with Android expertise might not feel comfortable signing off on a change with major iOS components). This is tricky, because a long latency from PR to commit encourages grouping more changes together. In general though, small/scoped changes usually get through the pipeline quicker though, where there are opportunities to split things up.
Some of the tooling in Metas monorepo encouraging breaking up changes into smaller reviewable “stacked” commits, which also influences some of the culture of small commits. I’ve heard some folks on the React team end up using “ghstack” to make this pattern easier against GitHub, but haven’t used it myself to vouch for it.
@NickGerleman have a good time on your PTO 😄. Thanks for reviewing the PR and explaining the process in depth. I pushed almost all the changes apart from these two:
- I just tried drawing in layer and it works. I had followed the existing bitmap approach instead of exploring layer drawing. But layer drawing seems to be working fine. I'd prefer to do another PR to replace it since this one has already gotten a bit huge, but lmk! I am thinking of creating
RCTLinearGradientLayerthat can handle it's own drawing. - @joevilches lmk if there're any relevant changes affecting clipping/borders.
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@jorge-cab merged this pull request in facebook/react-native@221d1eceda0e5ab870e96dcdd26e22ab17a3870c.
This pull request was successfully merged by @intergalacticspacehighway in 221d1eceda0e5ab870e96dcdd26e22ab17a3870c
When will my fix make it into a release? | How to file a pick request?