Support PlatformColor #1561

oblador · 2021-04-01T13:59:29Z

Summary

Since version 0.63 React Native has supported PlatformColor structs that among other features has dark mode support built in. Using those values with RNSVG today throws a warning "[Object object]" is not a valid color or brush and makes the path have a clear color.

This PR adds support for PlatformColor by utilizing the color parsing code shipped with React Native itself. In order to support the dynamic properties I had to retain the reference as a UIColor and convert it to CGColorRef as it's read. This might have a performance penalty.

The Android implementation doesn't yet have support for dynamic values (EDIT: react-native itself doesn't support it, so we're good there I think). Since it relies on the ColorPropConverter class, I think it means minimum supported version of RN will be raised to 0.63.

Fixes #1391

Test Plan

I couldn't find an example project in the repo, so I used my own example project, see video:

Compatibility

OS	Implemented
iOS	✅
Android	✅

Checklist

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

Saadnajmi · 2021-04-14T21:34:50Z

src/index.d.ts

@@ -1,6 +1,6 @@
 import * as React from 'react';
 import * as ReactNative from 'react-native';
-import { GestureResponderEvent } from 'react-native';
+import { GestureResponderEvent, OpaqueColorValue } from 'react-native';


Shouldn't this just be "ColorValue"?

Saadnajmi · 2021-04-14T21:35:34Z

src/index.d.ts

@@ -101,7 +101,7 @@ export type rgbaArray = ReadonlyArray<number>;
 // int32ARGBColor = 0xaarrggbb
 export type int32ARGBColor = number;

-export type Color = int32ARGBColor | rgbaArray | string;
+export type Color = int32ARGBColor | rgbaArray | OpaqueColorValue | string;



Could we just replace the "Color" type altogether with "ColorValue" from react native? It should represent the same set of types of colors

It's not the same, from the type it's string | OpaqueColorValue so we could replace the string with this one, but not the whole type. I prefer to use OpaqueColorValue here as I'm not sure what types will be added to ColorValue in the future.

I was under the impression that ColorValue is just the standard Color type in React Native 0.63+, and that OpaqueColorValue will include all the extra types of colors that can be passed to native. In react-native-macos (which we recently added support for in react-native-svg), I extended ColorValue/OpaqueColorValue to include a new API ColorWithSystemEffectMacOS (microsoft/react-native-macos#751).

But also, I thought the idea of OpaqueColorValue is that we do not need to know the specifics of the type, as long as it gets passed over the bridge to RCTConvert UIColor:] (or whatever the equivalent on Android is), React Native will make sure it's a valid native color. Is there a reason we need to know the specifics of the types of colors now? I was under the impression we could abstract most of that away to React Native and would need less processing on this repo?

You can find the definition here:

export type ColorValue = string | OpaqueColorValue;

So it's not the same, and it can't be replaced as the way this library deals with colors is quite custom. I'm not the author of this library, but i guess it's partly due to color inheritance. If it was not dealing with colors in a custom way then this PR wouldn't be necessary.

Saadnajmi · 2021-04-14T22:17:54Z

src/lib/extract/extractBrush.ts

+  // iOS PlatformColor
+  if ('semantic' in color) {
+    return [0, color];
+  }
+
+  // Android PlatformColor
+  if ('resource_paths' in color) {
+    return [0, color];
+  }


At least on iOS, "semantic" isn't enough, because a color might be a DynamicColor instead. Could we instead do something like a type check? if (typeof color === ColorValue) ....

DynamicColorIOS isn't supported in this PR, and I don't really see that API surviving long term as it's iOS only.

Surviving in react-native, or surviving in this repo? I don't think it's going anywhere as long as PlatformColor exists. We also have a react-native-macos equivalent DynacmicColorMacOS that won't go anywhere as long as DynamicColor exists.

I meant in react-native. DynamicColorIOS predates PlaformColor, and while it's not able to fully replace it just yet, I don't see that API as useful without cross platform support. I guess is possible to add however.

oblador · 2021-10-22T15:59:26Z

@Saadnajmi How can we make this PR progress?

gaodeng · 2022-01-12T03:35:49Z

@oblador @Saadnajmi @brentvatne @msand
Based on oblador's code, I added DynamicColorIOS support. Can someone please help to review and merge it?
oblador#1

Gregoirevda · 2022-02-09T09:56:28Z

Any update to support PlatformColor?

Gregoirevda · 2022-02-09T10:02:41Z

@oblador I've installed your PR branch through npm, but it doesn't compile with Babel:
The right-hand side of an 'in' expression must be of type 'any', an object type or a type parameter
for
if ('semantic' in color) { and if ('resource_paths' in color) {

WoLewicki

I left some suggestions regarding the code, which we can fix in a follow-up PR. Other from that, it looks like it's ready to be merged 🎉

WoLewicki · 2022-02-23T14:34:06Z

apple/Brushes/RNSVGSolidColorBrush.m

@@ -13,48 +13,49 @@

 @implementation RNSVGSolidColorBrush
 {
-    CGColorRef _color;
+    UIColor *_color;


I believe we should use RNSVGColor so it compiles on macOS.

WoLewicki · 2022-02-23T14:37:22Z

apple/Brushes/RNSVGSolidColorBrush.m

-    return CGColorCreateCopyWithAlpha(_color, opacity * CGColorGetAlpha(_color));
+    CGColorRef baseColor = _color.CGColor;
+    CGColorRef color = CGColorCreateCopyWithAlpha(baseColor, opacity * CGColorGetAlpha(baseColor));
+    CGColorRelease(baseColor);


We should not release it since it will release CGColor prop of _color, so trying to use _color.CGColor again will lead to crash.

Are you sure about that? Why would releasing _baseColor affect _color?

Oh, now I see, good catch 👍

WoLewicki · 2022-02-23T14:44:21Z

src/lib/extract/extractBrush.ts

@@ -40,6 +40,16 @@ export default function extractBrush(color?: Color) {
    return int32ARGBColor;
  }

+  // iOS PlatformColor
+  if ('semantic' in color) {


I think here we should also add a check for if color is an object, as suggested here by @gaodeng : https://github.com/oblador/react-native-svg/pull/1/files#diff-955398674a5278517eeeb9b5daca466f8e891b408d79757c3afbfa58a552a324R78

WoLewicki · 2022-02-23T15:25:59Z

One more thing about Android, I tried to set color prop with PlatformColor on Svg and it throws error unless we add changes in android folder from this PR: https://github.com/oblador/react-native-svg/pull/1/files. We can also do it in a follow-up PR so it is OK for now.

Follow-up PR to #1561 fixing the problems mentioned there. Also fixing the wrong releasing introduced in 027b8c1

Support PlatformColor

2312b50

Saadnajmi reviewed Apr 14, 2021

View reviewed changes

rurikoaraki mentioned this pull request May 13, 2021

Add support for PlatformColors - change colors to use ColorValue as type microsoft/fluentui-react-native#694

Merged

10 tasks

Saadnajmi mentioned this pull request Jun 21, 2021

Checkbox microsoft/fluentui-react-native#716

Merged

11 tasks

Merge branch 'develop' into platform-colors

2a8a0d0

WoLewicki approved these changes Feb 23, 2022

View reviewed changes

WoLewicki reviewed Feb 23, 2022

View reviewed changes

WoLewicki merged commit d35878b into software-mansion:develop Feb 23, 2022

WoLewicki mentioned this pull request Feb 23, 2022

fix: PlatformColor crashes on iOS and Android #1703

Merged

1 task

WoLewicki added a commit that referenced this pull request Feb 24, 2022

fix: PlatformColor crashes on iOS and Android (#1703)

b007efe

Follow-up PR to #1561 fixing the problems mentioned there. Also fixing the wrong releasing introduced in 027b8c1

WoLewicki mentioned this pull request Mar 7, 2022

Support ColorValue type from RN 0.63 #1567

Closed

mateusbra mentioned this pull request Aug 10, 2022

update react-native-svg version Expensify/App#10334

Merged

91 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support PlatformColor #1561

Support PlatformColor #1561

oblador commented Apr 1, 2021 •

edited

Loading

Saadnajmi Apr 14, 2021

Saadnajmi Apr 14, 2021

oblador Apr 15, 2021

Saadnajmi Apr 15, 2021

oblador Apr 26, 2021

Saadnajmi Apr 14, 2021

oblador Apr 15, 2021

Saadnajmi Apr 15, 2021

oblador Apr 26, 2021

oblador commented Oct 22, 2021

gaodeng commented Jan 12, 2022

Gregoirevda commented Feb 9, 2022

Gregoirevda commented Feb 9, 2022

WoLewicki left a comment

WoLewicki Feb 23, 2022

WoLewicki Feb 23, 2022

oblador Feb 24, 2022

oblador Feb 24, 2022

WoLewicki Feb 23, 2022

WoLewicki commented Feb 23, 2022

Support PlatformColor #1561

Support PlatformColor #1561

Conversation

oblador commented Apr 1, 2021 • edited Loading

Summary

Test Plan

Compatibility

Checklist

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

oblador commented Oct 22, 2021

gaodeng commented Jan 12, 2022

Gregoirevda commented Feb 9, 2022

Gregoirevda commented Feb 9, 2022

WoLewicki 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

WoLewicki commented Feb 23, 2022

oblador commented Apr 1, 2021 •

edited

Loading