fix: start and end points linear gradient algorithm #47003

intergalacticspacehighway · 2024-10-13T11:24:19Z

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 CAGradientLayer which 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 with CGGradient.
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 😅

jorge-cab

Do you have any plans to implement other backgroundImage features in the near future? If so do give us a heads up!

jorge-cab · 2024-10-14T17:03:13Z

packages/react-native/Libraries/StyleSheet/processBackgroundImage.js

@@ -123,25 +140,30 @@ function parseCSSLinearGradient(
  while ((match = linearGradientRegex.exec(cssString))) {
    const gradientContent = match[1];
    const parts = gradientContent.split(',');
-    let points = TO_BOTTOM_START_END_POINTS;
+    let orientation: LinearGradientOrientation = DEFAULT_ORIENTATION;
    const trimmedDirection = parts[0].trim().toLowerCase();
    const colorStopRegex =
      /\s*((?:(?:rgba?|hsla?)\s*\([^)]+\))|#[0-9a-fA-F]+|[a-zA-Z]+)(?:\s+(-?[0-9.]+%?)(?:\s+(-?[0-9.]+%?))?)?\s*/gi;


Can we add a comment explaining what this regex matches to and same with the linearGradientRegex?

eg from processFilter: // matches on functions with args and nested functions like "drop-shadow(10 10 10 rgba(0, 0, 0, 1))"

Or maybe a link to the syntax we are matching from the spec?

jorge-cab · 2024-10-14T17:44:36Z

packages/react-native/Libraries/StyleSheet/processBackgroundImage.js

@@ -123,25 +140,30 @@ function parseCSSLinearGradient(
  while ((match = linearGradientRegex.exec(cssString))) {


It would be nice if we only call parseCSSLinearGradient if we already know what we are parsing is a linearGradient. I'm thinking it can be a head start for if we add other backgroundImage functions like radial gradients or even url.

We did something similar for drop-shadow on processFilter.js

I'll leave this up to you since its not a blocker for this.

@jorge-cab yes, will follow up on this when adding more support to backgroundImage. First i want to implement color hints syntax support with linear gradients, which will be done in this PR. Thanks for reviewing!

lunaleaps · 2024-10-15T01:51:00Z

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?

intergalacticspacehighway · 2024-10-15T02:38:02Z

@lunaleaps Sure. Linear gradient was done in these PRs (#45434) and #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

lunaleaps · 2024-10-15T18:07:28Z

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!

facebook-github-bot · 2024-10-16T21:13:49Z

@jorge-cab has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jorge-cab

Math looks good! Nice job figuring this out.

jorge-cab · 2024-10-16T21:55:58Z

packages/react-native/ReactCommon/react/renderer/components/view/conversions.h

+          orientationIt->second.hasType<std::unordered_map<std::string, RawValue>>()) {
+        auto orientationMap = static_cast<std::unordered_map<std::string, RawValue>>(orientationIt->second);
+
+        auto typeIt = orientationMap.find("type");


nit: this naming is shadowing we should rename

jorge-cab · 2024-10-16T22:09:55Z

packages/react-native/React/Fabric/Utils/RCTLinearGradient.mm

+
+    auto colorSpace = getDefaultColorSpace() == ColorSpace::sRGB ? CGColorSpaceCreateDeviceRGB() : CGColorSpaceCreateWithName(kCGColorSpaceDisplayP3);
+
+    CGGradientRef gradient = CGGradientCreateWithColors(colorSpace, (__bridge CFArrayRef)colors, locations);


nit: This naming is shadowing, we should rename

jorge-cab · 2024-10-17T16:08:24Z

packages/react-native/React/Fabric/Utils/RCTLinearGradient.mm

+  UIImage *gradientImage = [renderer imageWithActions:^(UIGraphicsImageRendererContext * _Nonnull rendererContext) {
+    CGContextRef context = rendererContext.CGContext;
+    NSMutableArray *colors = [NSMutableArray array];
+    CGFloat *locations = (CGFloat *)malloc(sizeof(CGFloat) * colorStops.size());


We really want to avoid malloc can we do something like:

Suggested change

CGFloat *locations = (CGFloat *)malloc(sizeof(CGFloat) * colorStops.size());

CGFloat locations[colorStops.size()];

jorge-cab · 2024-10-17T16:53:15Z

packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm

+            backgroundImageLayer.borderWidth = layer.borderWidth;
+            backgroundImageLayer.borderColor = layer.borderColor;


We don't need to set these two here, border layer will be composited on top of backgroundImage

Suggested change

backgroundImageLayer.borderWidth = layer.borderWidth;

backgroundImageLayer.borderColor = layer.borderColor;

jorge-cab · 2024-10-17T16:54:56Z

packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm

+        backgroundImageLayer.frame = layer.bounds;
+        backgroundImageLayer.masksToBounds = YES;
+        // border styling to work with gradient layers
+        if (useCoreAnimationBorderRendering) {


This section should just be used to round the corners of the backgroundImage we don't really care about most of the things useCoreAnimationBorderRendering checks for. We can focus on the border being uniform

Suggested change

if (useCoreAnimationBorderRendering) {

if (borderMetrics.borderRadii.isUniform()) {

jorge-cab · 2024-10-17T16:59:58Z

packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm

+            CAShapeLayer *maskLayer = [CAShapeLayer layer];
+            CGPathRef path = RCTPathCreateWithRoundedRect(
+                self.bounds,
+                RCTGetCornerInsets(RCTCornerRadiiFromBorderRadii(borderMetrics.borderRadii), UIEdgeInsetsZero),
+                nil);
+            maskLayer.path = path;
+            CGPathRelease(path);
+            backgroundImageLayer.mask = maskLayer;


Can we use createMaskLayer from line ~1094 instead

jorge-cab · 2024-10-17T17:02:44Z

packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm

+            maskLayer.path = path;
+            CGPathRelease(path);
+            backgroundImageLayer.mask = maskLayer;
+          }


We need to reset corners to 0 in case of a state change backgroundImageLayer.cornerRadius = 0;

jorge-cab · 2024-10-17T17:09:47Z

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/style/Gradient.kt

@@ -21,63 +20,46 @@ public class Gradient(gradient: ReadableMap?, context: Context) {
  }



Can we make this class internal instead of public. We don' want people to start relying on this yet

jorge-cab · 2024-10-17T17:10:33Z

...react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/style/LinearGradient.kt

+import kotlin.math.atan
+import kotlin.math.tan
+
+public class LinearGradient(


Suggested change

public class LinearGradient(

internal class LinearGradient(

jorge-cab · 2024-10-17T17:12:12Z

...react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/style/LinearGradient.kt

+  private val orientation: Orientation
+
+  init {
+    orientation = when (val type = orientationMap.getString("type")) {
+      "angle" -> {
+        val angle = orientationMap.getDouble("value")
+        Orientation.Angle(angle)
+      }
+
+      "direction" -> {
+        val direction = orientationMap.getString("value")
+          ?: throw IllegalArgumentException("Direction value cannot be null")
+        Orientation.Direction(direction)
+      }
+
+      else -> throw IllegalArgumentException("Invalid orientation type: $type")
+    }
+  }


nit: We should just assign this to the variable no need to use init

intergalacticspacehighway · 2024-10-17T20:48:31Z

Pushed all fixes @jorge-cab. Thanks for the quick review! <3

facebook-github-bot · 2024-10-18T17:15:58Z

@jorge-cab has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jorge-cab · 2024-10-18T22:40:19Z

@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.

intergalacticspacehighway · 2024-10-18T22:47:22Z

@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 · 2024-10-23T18:25:17Z

@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

intergalacticspacehighway · 2024-10-28T22:07:07Z

@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.

NickGerleman · 2024-10-29T15:24:40Z

packages/react-native/Libraries/StyleSheet/processBackgroundImage.js

+      ) {
+        const parsedDirection = getDirectionString(bgImage.direction);
+        if (parsedDirection != null) {
+          orientation = {


nit: if the single keyword variants are directlty convertible to angles, that might make sense to simplify during parsing

https://www.w3.org/TR/css-images-3/#linear-gradient-syntax

I thought about this too but it seems keywords like to top left (which means the angle should align with the view's corner) need information about the view which we can't do during parsing.

We could however preemptively parse to top, to left etc since those map to constant angle values. However naively I feel like we should avoid parsing in such a fragmented way

i also had same thoughts as @jorge-cab about parsing in fragmented way but i am down to re-using angles and passing only corner styles, will make the change, lmk if sounds good to you @jorge-cab.
i was also thinking in the longer term that dimension-dependent styles like these could be part of layout nodes right before setting native view dimensions, lot of native styling logic could be moved there and code can be reused among platforms. e.g transform-origin, translate with %, gradients, also support for calc function etc. Is there an ongoing effort in that direction? i can also take a look, i know @NickGerleman is working on native CSS parser, but i haven't dug very deep into it yet.

Updated here, loving it. It made it more concise and reusable.

NickGerleman · 2024-10-29T15:25:57Z

packages/react-native/Libraries/StyleSheet/processBackgroundImage.js

+        if (parsedDirection != null) {
+          orientation = {
+            type: 'direction',
+            value: parsedDirection,


Apart from those that can be simplified to angles, we have four corners this could point to, so it might be simpler to pass one of four well known values to native for these.

NickGerleman · 2024-10-29T15:27:12Z

packages/react-native/Libraries/StyleSheet/processBackgroundImage.js

@@ -17,22 +17,37 @@ const processColor = require('./processColor').default;
 const DIRECTION_REGEX =


nit: I think this should also be case insensitive, since CSS keywords generally are

NickGerleman · 2024-10-29T15:28:41Z

packages/react-native/Libraries/StyleSheet/processBackgroundImage.js

+
+type LinearGradientOrientation =
+  | {type: 'angle', value: number}
+  | {type: 'direction', value: string};


nit: consider keyword or keywords instead of direction, to map to spec, since angle is considered a direction in verbiage

NickGerleman · 2024-10-29T15:31:13Z

packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm

-          [colors addObject:(id)color.CGColor];
-          [locations addObject:location];
+    // iterate in reverse to match CSS specification
+    for (auto it = _props->backgroundImage.rbegin(); it != _props->backgroundImage.rend(); ++it) {


nit: I think we require min XCode which supports std::ranges::reverse_view as well,

NickGerleman · 2024-10-29T16:15:17Z

...react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/style/LinearGradient.kt

+) {
+  private sealed class Orientation {
+    public data class Angle(val value: Double) : Orientation()
+    public data class Direction(val value: String) : Orientation()


Any way we could parse this into one of four possible unique values as enum, instead of keeping string around?

NickGerleman · 2024-10-29T16:17:47Z

packages/react-native/ReactCommon/react/renderer/components/view/conversions.h


-    backgroundImage.push_back(gradientValue);
+      backgroundImage.push_back(BackgroundImage{BackgroundImageType::LinearGradient, linearGradient});


Suggested change

backgroundImage.push_back(BackgroundImage{BackgroundImageType::LinearGradient, linearGradient});

backgroundImage.push_back(BackgroundImage{BackgroundImageType::LinearGradient, std::move(linearGradient)});

NickGerleman · 2024-10-29T16:18:20Z

packages/react-native/ReactCommon/react/renderer/graphics/BackgroundImage.h

+  bool operator==(const BackgroundImage& other) const {
+    return type == other.type && value == other.value;
  }


Here and below, does not seem like we need to provide explicit equality operator

Suggested change

bool operator==(const BackgroundImage& other) const {

return type == other.type && value == other.value;

}

bool operator==(const BackgroundImage& other) const = default;

NickGerleman · 2024-10-29T16:18:56Z

packages/react-native/ReactCommon/react/renderer/graphics/LinearGradient.h

+struct ColorStop {
+  bool operator==(const ColorStop& other) const = default;
+  SharedColor color;
+  Float position;


nit: provide explicit initial value for primitives

NickGerleman · 2024-10-29T16:20:34Z

packages/react-native/ReactCommon/react/renderer/graphics/LinearGradient.h

+
+struct GradientOrientation {
+  GradientOrientationType type;
+  std::variant<Float, std::string> value;


Can we parse the Direction/Keyword version into enum as early as possible instead of keeping string around?

NickGerleman · 2024-11-01T12:00:39Z

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.

…tion to direction and add more testcases

intergalacticspacehighway · 2024-11-02T02:03:04Z

@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 RCTLinearGradientLayer that can handle it's own drawing.
@joevilches lmk if there're any relevant changes affecting clipping/borders.

start and end points algorithm fix

cfa8dd5

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 13, 2024

fix: snapshot test

42f4ae3

jorge-cab reviewed Oct 14, 2024

View reviewed changes

pr feedback - add comments in regex

45d800d

jorge-cab reviewed Oct 17, 2024

View reviewed changes

pr feedback

b5d8e1a

NickGerleman reviewed Oct 29, 2024

View reviewed changes

NickGerleman mentioned this pull request Nov 1, 2024

experimental_backgroundImage works incorrect #47335

Open

3 tasks

intergalacticspacehighway added 4 commits November 2, 2024 09:42

simplify angle parsing for side direction keywords and rename orienta…

d308b7d

…tion to direction and add more testcases

Merge branch 'main' into fix/linear-gradient-start-end-points

b1c8efd

use ranges reverse_view and refactor type from BackgroundImage.h

1822438

use enums for keyword

35f6d6b

		@@ -123,25 +140,30 @@ function parseCSSLinearGradient(
		while ((match = linearGradientRegex.exec(cssString))) {


		auto colorSpace = getDefaultColorSpace() == ColorSpace::sRGB ? CGColorSpaceCreateDeviceRGB() : CGColorSpaceCreateWithName(kCGColorSpaceDisplayP3);

		CGGradientRef gradient = CGGradientCreateWithColors(colorSpace, (__bridge CFArrayRef)colors, locations);

	CGFloat locations = (CGFloat )malloc(sizeof(CGFloat) * colorStops.size());
	CGFloat locations[colorStops.size()];

		backgroundImageLayer.borderWidth = layer.borderWidth;
		backgroundImageLayer.borderColor = layer.borderColor;

	if (useCoreAnimationBorderRendering) {
	if (borderMetrics.borderRadii.isUniform()) {

		@@ -21,63 +20,46 @@ public class Gradient(gradient: ReadableMap?, context: Context) {
		}

		@@ -17,22 +17,37 @@ const processColor = require('./processColor').default;
		const DIRECTION_REGEX =


		backgroundImage.push_back(gradientValue);
		backgroundImage.push_back(BackgroundImage{BackgroundImageType::LinearGradient, linearGradient});

fix: start and end points linear gradient algorithm #47003

Are you sure you want to change the base?

fix: start and end points linear gradient algorithm #47003

Conversation

intergalacticspacehighway commented Oct 13, 2024 • edited Loading

Summary:

Changelog:

Test Plan:

Aside

jorge-cab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intergalacticspacehighway Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

lunaleaps commented Oct 15, 2024

intergalacticspacehighway commented Oct 15, 2024

lunaleaps commented Oct 15, 2024

facebook-github-bot commented Oct 16, 2024

jorge-cab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intergalacticspacehighway commented Oct 17, 2024 • edited Loading

facebook-github-bot commented Oct 18, 2024

jorge-cab commented Oct 18, 2024

intergalacticspacehighway commented Oct 18, 2024

jorge-cab commented Oct 23, 2024

intergalacticspacehighway commented Oct 28, 2024

Choose a reason for hiding this comment

jorge-cab Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

intergalacticspacehighway Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

intergalacticspacehighway Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickGerleman Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickGerleman commented Nov 1, 2024

intergalacticspacehighway commented Nov 2, 2024 • edited Loading

intergalacticspacehighway commented Oct 13, 2024 •

edited

Loading

intergalacticspacehighway Oct 16, 2024 •

edited

Loading

intergalacticspacehighway commented Oct 17, 2024 •

edited

Loading

jorge-cab Oct 31, 2024 •

edited

Loading

intergalacticspacehighway Nov 1, 2024 •

edited

Loading

intergalacticspacehighway Nov 1, 2024 •

edited

Loading

NickGerleman Oct 29, 2024 •

edited

Loading

intergalacticspacehighway commented Nov 2, 2024 •

edited

Loading