Fix macOS ActivityIndicator to color properly in different themes

[tom-un]

Please select one of the following

• I am removing an existing difference between facebook/react-native and Microsoft/react-native 👍
• I am cherry-picking a change from Facebook's react-native into Microsoft/react-native 👍
• I am making a fix / change for the macOS implementation of react-native
• I am making a change required for Microsoft usage of react-native

Description of changes

PROBLEM
ISS#3218502: LPC in Dark Mode: progress spinner doesn't spin/animate
The RCTActivityIndicatorView.m was flattening an NSColor and using its components to create layer contentFilter at the time that the color prop was being set. In Mojave, NSColors should only ever be converted to colorspaces in one of four methods: updateLayer, drawRect:, layout, updateConstraints. Since it was not performing the operation in any of these methods, the contentFilter was not responding to theme changes.

FIX
Override the updateLayer method and there flatten the _color prop to its components only if it exists, and then set the contentFilter.

PRODUCT IMPACT
Impacts any macOS SDX that uses the ActivityIndicator (LPC) and makes it render properly in all themes.

VERIFICATION

Added semantic and dynamic colors in the ActivityIndicatorExample in RNTester to validate.

@markavitale ; @HeyImChris

/azp run

[tom-un]

It is in fact eslint that decided to make the ternary expression be a one liner in one case and a multi line in the other case – I assume because of length. From: Mark A Vitale <[email protected]> Sent: Wednesday, March 13, 2019 11:45 AM To: Microsoft/react-native <[email protected]> Cc: Tom Underhill <[email protected]>; Assign <[email protected]> Subject: Re: [Microsoft/react-native] Fix macOS ActivityIndicator to color properly in different themes (#13) @markavitale requested changes on this pull request.

________________________________ In RNTester/js/ActivityIndicatorExample.js<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Freact-native%2Fpull%2F13%23discussion_r265256646&data=02%7C01%7CTom.Underhill%40microsoft.com%7Cf9a6e0b188644692705208d6a7e3edfe%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636880994733366232&sdata=drEXT%2Bgit%2FFLhwtdWIf6WU39D2P0xQJuCzXGiTTYcTs%3D&reserved=0>:

@@ -99,8 +100,18 @@ exports.examples = [

render() { return ( <View style={styles.horizontal}> - <ActivityIndicator color="#0000ff" /> - <ActivityIndicator color="#aa00aa" /> + <ActivityIndicator + color={ + Platform.OS === 'macos' + ? {dynamic: {light: 'black', dark: 'white'}} + : '#0000ff' + } + /> + <ActivityIndicator + color={ + Platform.OS === 'macos' ? {semantic: 'textColor'} : '#aa00aa' I'm guessing it's some JS linter that is causing this to be one line while the ternary operator is broken on the ?. If it's not a linter, I'd recommend matching style.

________________________________ In React/Views/RCTActivityIndicatorView.m<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Freact-native%2Fpull%2F13%23discussion_r265278748&data=02%7C01%7CTom.Underhill%40microsoft.com%7Cf9a6e0b188644692705208d6a7e3edfe%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636880994733376244&sdata=eHjYF1DfgqE6%2BLy6VFG%2FtAIYzuzisaq5j%2FiyrEFyyaQ%3D&reserved=0>:

@@ -66,18 +66,23 @@ - (void)setActivityIndicatorViewStyle:(UIActivityIndicatorViewStyle)activityIndi

} } Don't we still need a setting of the color property to result in kicking off a layer update? I think this is right, but we should double check that setNeedsDisplay: results in updateLayer being called on both iOS and macOS. ⬇️ Suggested change - +- (void)setColor: (UIColor*)color +{ + if (_color != color) { + _color = color; + [self setNeedsDisplay:YES]; + } +} — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Freact-native%2Fpull%2F13%23pullrequestreview-214117726&data=02%7C01%7CTom.Underhill%40microsoft.com%7Cf9a6e0b188644692705208d6a7e3edfe%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636880994733376244&sdata=zM%2Bqt3%2F5WyoG%2FLPgKSAYajG82NrwPuvWQch6n6MccXQ%3D&reserved=0>, or mute the thread<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAcqVBhNrCR-vDGCj_suKi3FPALdhvEd8ks5vWUcOgaJpZM4btyYg&data=02%7C01%7CTom.Underhill%40microsoft.com%7Cf9a6e0b188644692705208d6a7e3edfe%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636880994733386249&sdata=kXbiZSk%2BIkYiC5a3HcN%2FEn3nAxAd57PwocTBwxdAP3M%3D&reserved=0>.

[markavitale]

Looks good so long as we've verified updateLayer is getting called correctly on both iOS and macOS.

[tom-un]

Looks good so long as we've verified updateLayer is getting called correctly on both iOS and macOS.

It is on mac. The code change does not effect ios