[Fix] Android/ColorProps: ColorProps with value null should be defaultColor instead of transparent

[hank121314]

Summary

This pr:

• Fixes: TintColor style isn't correctly applied when changed on Android on an Image #30183
• Fixes: ActivityIndicator works only on iOS #30056
• Fixes: Text disappers on Android #29950
• Fixes: Android <Text> color becomes invisible when switching back to initial state #29717
• Fixes: ActivityIndicator not visible without color prop on Pixel 3a #29495
• Fixes: [Android] Since 0.63.1 images won't render on Android after subsequently applying & removing tintColor #29412
• Fixes: RN 0.63.1: ActivityIndicator disapearing #29378

Because most of @ReactProps(name = ViewProps.COLOR) accept @ Nullable Integer.
For example:

react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java

Lines 472 to 479 in abb6433

	@ReactProp(name = ViewProps.COLOR, customType = "Color")
	public void setColor(@Nullable Integer color) {
	mIsColorSet = (color != null);
	if (mIsColorSet) {
	mColor = color;
	}
	markUpdated();
	}

After update to react-native 0.63.2 to make PlatformColor work, there is a new ColorPropSetter.

react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagersPropertyCache.java

Lines 194 to 215 in abb6433

	private static class ColorPropSetter extends PropSetter {
	private final int mDefaultValue;
	public ColorPropSetter(ReactProp prop, Method setter) {
	this(prop, setter, 0);
	}
	public ColorPropSetter(ReactProp prop, Method setter, int defaultValue) {
	super(prop, "mixed", setter);
	mDefaultValue = defaultValue;
	}
	@Override
	protected Object getValueOrDefault(Object value, Context context) {
	if (value == null) {
	return mDefaultValue;
	}
	return ColorPropConverter.getColor(value, context);
	}
	}

But ColorPropSetter won't return an nullable value with getValueOrDefault, it will always return it's defaultValue which is 0.
And 0 is equal to TRANSPARENT, will cause disappear.

Changelog

[Android] [Fixed] - ColorProps with value null should be defaultColor instead of transparent

Test Plan

Please initiated a new project and replaced the app with the following code:

import * as React from 'react';
import {Text, View, TouchableOpacity, PlatformColor} from 'react-native';

export default function App() {
  const [active, setActive] = React.useState(false);

  return (
    <View>
      <Text style={active ? {color: 'green'} : null}>Example</Text>
      <Text
        style={
          active ? {color: PlatformColor('@android:color/holo_purple')} : null
        }>
        Example2
      </Text>
      <TouchableOpacity onPress={() => setActive(!active)}>
        <Text>Toggle Active</Text>
      </TouchableOpacity>
    </View>
  );
}

Thanks you so much for your code review!

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	9,239,782	-11
android	hermes	armeabi-v7a	8,749,720	-5
android	hermes	x86	9,702,093	-6
android	hermes	x86_64	9,667,310	-16
android	jsc	arm64-v8a	10,886,756	-47
android	jsc	armeabi-v7a	9,787,976	-54
android	jsc	x86	10,944,476	-53
android	jsc	x86_64	11,551,223	-47

Base commit: 4246c75

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 4246c75

[safaiyeh]

Nice work! @hank121314 I built RNTester to test this out:

Working as expected, tagging PR submitted on the issue.

[safaiyeh]

cc @JoshuaGross

[hank121314]

Nice work! @hank121314 I built RNTester to test this out:

Working as expected, tagging PR submitted on the issue.

Thank you so much for help and testing! 😄

[Danite]

I think when the color is undefined it should also fallback to the defaultColor.

Take this case:
<Text style={[someTextStyleWithoutColor, isActive && activeTextStyle]}> Hello there! </Text>
it will make the text transparent once is not active.

[hank121314]

I think when the color is undefined it should also fallback to the defaultColor.

Take this case:
<Text style={[someTextStyleWithoutColor, isActive && activeTextStyle]}> Hello there! </Text>
it will make the text transparent once is not active.

Yes, this pr is also included undefined in javascript.

[analysis-bot]

Code analysis results:

• google-java-format found some issues. See https://github.com/google/google-java-format

[analysis-bot]

google-java-format suggested changes:

@@ -337 +337 @@
-          return ColorPropConverter.getColor(value, context);
+        return ColorPropConverter.getColor(value, context);

[hank121314]

apply google-java-format suggested changes.

[fabOnReact]

thanks a lot @hank121314. Your pr also fixes #29378 (not linked in your pr summary), I was searching for the solution as explained in #29412 (comment) and then found your PR from another related issue.

I checkout out your branch and as in the below test case, verified that it fixes also #29378

CLICK TO OPEN TESTS RESULTS

BEFORE	AFTER

Thanks a lot. 🙏 ☮️

[hank121314]

Hi @fabriziobertoglio1987 .
Thanks for your investigation and review!
Hope this pr can be merged soon. 😄

[StanislavMayorov]

@fabriziobertoglio1987 @hank121314 Hi. Could you merge it please?

[taylorkline]

This did not fix ActivityIndicator from being transparent for me. #30057 worked for me.

[taylorkline]

Actually, in general, this fix is not working for me on 0.64.2.

[hank121314]

Hi @taylorkline, did you try to patch this pr with Building from source?
If it still does not work, could you give me some reproducible code?
Thanks!

[janicduplessis]

LGTM, sorry this is taking forever to merge, will ping some people at fb.

[taylorkline]

@hank121314 No, I did not - my apologies, likely user error on my part then.

[facebook-github-bot]

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@lunaleaps merged this pull request in 842bcb9.