Addressing various issues with the Appearance API (#28823) #29106

mrbrentkelly · 2020-06-10T17:01:11Z

Summary

This PR fixes a few issues with the Appearance API (as noted here #28823).

For the Appearance API to work correctly on Android you need to call AppearanceModule.onConfigurationChanged when the current Activity goes through a configuration change. This was being called in the RNTester app but not in ReactActivity so it meant the Appearance API wouldn't work for Android in newly generated RN projects (or ones upgraded to the latest version of RN).
The Appearance API wasn't working correctly for brownfield scenarios on Android. It's possible to force an app light or dark natively on Android by calling AppCompatDelegate.setDefaultNightMode(). The Appearance API wasn't picking up changes from this function because it was using the Application context instead of the current Activity context.
The Appearance API wasn't working correctly for brownfield scenarios on iOS. Just like on Android its possible to force an app light or dark natively by setting window.overrideUserInterfaceStyle. The Appearance API didn't work with this override because we were overwriting _currentColorScheme back to default as soon as we set it.

Changelog

Fixed

#28823

[Android] [Fixed] - Appearance API now works on Android
[Android] [Fixed] - Appearance API now works correctly when calling AppCompatDelegate.setDefaultNightMode()
[iOS] [Fixed] - Appearance API now works correctly when setting window.overrideUserInterfaceStyle

Test Plan

Ran RNTester on iOS and Android and verified the Appearance examples still worked correctly.

- Updating ReactActivity to push onConfigurationChanged updates to the AppearanceModule. This fixes the issue of the Appearance API not working in React Native projects who haven't implemented their own onConfigurationChanged function. - Updating AppearanceModule on Android to use Activity context (when available) to check the current UIMode. This allows the Appearance API to react when AppCompatDelegate.setDefaultNightMode is called natively (brownfield scenario). - Updating RCTAppearance on iOS to not overwrite the _currentColorScheme value every time getColorScheme is called. This was preventing the Appearance API from reacting to changes when window.overrideUserInterfaceStyle is set natively (brownfield scenario).

facebook-github-bot · 2020-06-10T17:01:23Z

Hi @mrbrentkelly!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2020-06-10T17:11:05Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

analysis-bot · 2020-06-10T17:20:48Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,854,067	-110
android	hermes	armeabi-v7a	8,375,334	-103
android	hermes	x86	9,310,856	-108
android	hermes	x86_64	9,254,001	-110
android	jsc	arm64-v8a	10,585,012	-112
android	jsc	armeabi-v7a	10,089,386	-103
android	jsc	x86	10,603,303	-116
android	jsc	x86_64	11,186,714	-112

Base commit: 06f52f7

analysis-bot · 2020-06-10T17:25:11Z

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

Base commit: 06f52f7

koenpunt · 2020-08-14T20:20:00Z

React/CoreModules/RCTAppearance.mm

+  if (_currentColorScheme == nil) {
+    _currentColorScheme = RCTColorSchemePreference(nil);
+  }


While this change probably makes sense, it doesn't solve the issues with overrideUserInterfaceStyle in a brownfield app for me.

Hey @koenpunt, if you have time to throw up a repo with a brownfield project that's broken (or hit me with some repro steps) I could probably take a look at brownfield a bit deeper sometime this week 👍 .

mrbrentkelly · 2020-08-27T07:00:42Z

~~FYI the test_js and test_js_prev_lts checks are failing due to a Flow version error which doesn’t appear related to the changes in this PR.~~

~~Wrong version of Flow. The config specifies version ^0.131.0 but this is version 0.132.0 Flow check failed.~~

Update this appears to be resolved upstream now.

analysis-bot

Code analysis results:

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

ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java

ReactAndroid/src/main/java/com/facebook/react/modules/appearance/AppearanceModule.java

alexandersandberg · 2021-02-06T15:09:58Z

What's the current status of this PR? On the roadmap? 🙂

mrbrentkelly · 2021-02-06T15:19:27Z

I've been wondering the same thing :)

analysis-bot

Code analysis results:

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

ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java

ReactAndroid/src/main/java/com/facebook/react/modules/appearance/AppearanceModule.java

analysis-bot

Code analysis results:

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

ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java

ReactAndroid/src/main/java/com/facebook/react/modules/appearance/AppearanceModule.java

andresribeiro · 2021-06-13T14:53:36Z

So.... anything new?

svbutko · 2021-09-29T14:16:54Z

Is this issue even on roadmap for next releases?

facebook-github-bot · 2021-09-29T20:39:17Z

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

react-native-bot · 2021-10-06T03:25:04Z

This pull request was successfully merged by @mrbrentkelly in 25a2c60.

^{When will my fix make it into a release? | Upcoming Releases}

Summary: This PR fixes a few issues with the Appearance API (as noted here #28823). 1. For the Appearance API to work correctly on Android you need to call `AppearanceModule.onConfigurationChanged` when the current Activity goes through a configuration change. This was being called in the RNTester app but not in `ReactActivity` so it meant the Appearance API wouldn't work for Android in newly generated RN projects (or ones upgraded to the latest version of RN). 2. The Appearance API wasn't working correctly for brownfield scenarios on Android. It's possible to force an app light or dark natively on Android by calling `AppCompatDelegate.setDefaultNightMode()`. The Appearance API wasn't picking up changes from this function because it was using the Application context instead of the current Activity context. 3. The Appearance API wasn't working correctly for brownfield scenarios on iOS. Just like on Android its possible to force an app light or dark natively by setting `window.overrideUserInterfaceStyle`. The Appearance API didn't work with this override because we were overwriting `_currentColorScheme` back to default as soon as we set it. ## Changelog  ### Fixed #28823 * [Android] [Fixed] - Appearance API now works on Android * [Android] [Fixed] - Appearance API now works correctly when calling `AppCompatDelegate.setDefaultNightMode()` * [iOS] [Fixed] - Appearance API now works correctly when setting `window.overrideUserInterfaceStyle` Pull Request resolved: #29106 Test Plan: Ran RNTester on iOS and Android and verified the Appearance examples still worked [correctly.](url) Reviewed By: hramos Differential Revision: D31284331 Pulled By: sota000 fbshipit-source-id: 45bbe33983e506eb177d596d33ddf15f846708fd

…acebook#29106) Summary: This PR fixes a few issues with the Appearance API (as noted here facebook#28823). 1. For the Appearance API to work correctly on Android you need to call `AppearanceModule.onConfigurationChanged` when the current Activity goes through a configuration change. This was being called in the RNTester app but not in `ReactActivity` so it meant the Appearance API wouldn't work for Android in newly generated RN projects (or ones upgraded to the latest version of RN). 2. The Appearance API wasn't working correctly for brownfield scenarios on Android. It's possible to force an app light or dark natively on Android by calling `AppCompatDelegate.setDefaultNightMode()`. The Appearance API wasn't picking up changes from this function because it was using the Application context instead of the current Activity context. 3. The Appearance API wasn't working correctly for brownfield scenarios on iOS. Just like on Android its possible to force an app light or dark natively by setting `window.overrideUserInterfaceStyle`. The Appearance API didn't work with this override because we were overwriting `_currentColorScheme` back to default as soon as we set it. ## Changelog  ### Fixed facebook#28823 * [Android] [Fixed] - Appearance API now works on Android * [Android] [Fixed] - Appearance API now works correctly when calling `AppCompatDelegate.setDefaultNightMode()` * [iOS] [Fixed] - Appearance API now works correctly when setting `window.overrideUserInterfaceStyle` Pull Request resolved: facebook#29106 Test Plan: Ran RNTester on iOS and Android and verified the Appearance examples still worked [correctly.](url) Reviewed By: hramos Differential Revision: D31284331 Pulled By: sota000 fbshipit-source-id: 45bbe33983e506eb177d596d33ddf15f846708fd

Summary: This PR fixes a few issues with the Appearance API (as noted here facebook/react-native#28823). 1. For the Appearance API to work correctly on Android you need to call `AppearanceModule.onConfigurationChanged` when the current Activity goes through a configuration change. This was being called in the RNTester app but not in `ReactActivity` so it meant the Appearance API wouldn't work for Android in newly generated RN projects (or ones upgraded to the latest version of RN). 2. The Appearance API wasn't working correctly for brownfield scenarios on Android. It's possible to force an app light or dark natively on Android by calling `AppCompatDelegate.setDefaultNightMode()`. The Appearance API wasn't picking up changes from this function because it was using the Application context instead of the current Activity context. 3. The Appearance API wasn't working correctly for brownfield scenarios on iOS. Just like on Android its possible to force an app light or dark natively by setting `window.overrideUserInterfaceStyle`. The Appearance API didn't work with this override because we were overwriting `_currentColorScheme` back to default as soon as we set it. ## Changelog  ### Fixed facebook/react-native#28823 * [Android] [Fixed] - Appearance API now works on Android * [Android] [Fixed] - Appearance API now works correctly when calling `AppCompatDelegate.setDefaultNightMode()` * [iOS] [Fixed] - Appearance API now works correctly when setting `window.overrideUserInterfaceStyle` Pull Request resolved: facebook/react-native#29106 Test Plan: Ran RNTester on iOS and Android and verified the Appearance examples still worked [correctly.](url) Reviewed By: hramos Differential Revision: D31284331 Pulled By: sota000 fbshipit-source-id: 45bbe33983e506eb177d596d33ddf15f846708fd

react-native-bot added Bug Platform: Android Android applications. labels Jun 10, 2020

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 10, 2020

mrbrentkelly mentioned this pull request Aug 14, 2020

Appearance module does not behave correctly: addChangeListener does not react to changes, useColorScheme() does not react to changes. #28823

Closed

koenpunt reviewed Aug 14, 2020

View reviewed changes

mrbrentkelly added 3 commits August 18, 2020 09:15

Merge remote-tracking branch 'upstream/master'

f4cf3d4

Merge remote-tracking branch 'upstream/master'

14ebe6d

Merge remote-tracking branch 'upstream/master'

3672989

mrbrentkelly added 7 commits September 15, 2020 10:18

Merge branch 'master' into master

a2b0936

Merge branch 'master' into master

f01af5d

Merge branch 'master' into master

bf57803

Merge remote-tracking branch 'upstream/master'

3414938

Merge branch 'master' into master

5692995

Merge branch 'master' into master

79ebc17

Merge branch 'master' into master

d5711ec

rikkekantega mentioned this pull request Oct 28, 2020

fix: QA fixes for theming/dark mode AtB-AS/mittatb-app#611

Merged

mrbrentkelly added 2 commits November 3, 2020 09:30

Merge branch 'master' into master

86b91a6

Merge branch 'master' into master

6d12dea

analysis-bot suggested changes Dec 22, 2020

View reviewed changes

Merge branch 'master' into master

7efc38a

analysis-bot suggested changes Apr 21, 2021

View reviewed changes

Addressing google-java-format changes

dfff14f

analysis-bot suggested changes Apr 21, 2021

View reviewed changes

ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java Outdated Show resolved Hide resolved

ReactAndroid/src/main/java/com/facebook/react/modules/appearance/AppearanceModule.java Show resolved Hide resolved

mrbrentkelly added 2 commits April 21, 2021 08:50

Addressing google-java-format changes

2a280ab

Addressing google-java-format changes

bf8f5f0

elicwhite added the Needs: React Native Team Attention label Jun 15, 2021

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Aug 12, 2021

facebook-github-bot closed this in 25a2c60 Oct 6, 2021

react-native-bot added the Merged This PR has been merged. label Oct 6, 2021

This was referenced Oct 29, 2021

Created expo-system-ui module expo/expo#14952

Merged

[template] Update onConfigurationChanged for Android expo/expo#14929

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressing various issues with the Appearance API (#28823) #29106

Addressing various issues with the Appearance API (#28823) #29106

mrbrentkelly commented Jun 10, 2020 •

edited

Loading

facebook-github-bot commented Jun 10, 2020

facebook-github-bot commented Jun 10, 2020

analysis-bot commented Jun 10, 2020 •

edited

Loading

analysis-bot commented Jun 10, 2020 •

edited

Loading

koenpunt Aug 14, 2020

mrbrentkelly Aug 17, 2020

mrbrentkelly commented Aug 27, 2020 •

edited

Loading

analysis-bot left a comment

alexandersandberg commented Feb 6, 2021

mrbrentkelly commented Feb 6, 2021

analysis-bot left a comment

analysis-bot left a comment

andresribeiro commented Jun 13, 2021

svbutko commented Sep 29, 2021

facebook-github-bot commented Sep 29, 2021

react-native-bot commented Oct 6, 2021

Addressing various issues with the Appearance API (#28823) #29106

Addressing various issues with the Appearance API (#28823) #29106

Conversation

mrbrentkelly commented Jun 10, 2020 • edited Loading

Summary

Changelog

Fixed

Test Plan

facebook-github-bot commented Jun 10, 2020

facebook-github-bot commented Jun 10, 2020

analysis-bot commented Jun 10, 2020 • edited Loading

analysis-bot commented Jun 10, 2020 • edited Loading

koenpunt Aug 14, 2020

Choose a reason for hiding this comment

mrbrentkelly Aug 17, 2020

Choose a reason for hiding this comment

mrbrentkelly commented Aug 27, 2020 • edited Loading

analysis-bot left a comment

Choose a reason for hiding this comment

alexandersandberg commented Feb 6, 2021

mrbrentkelly commented Feb 6, 2021

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

andresribeiro commented Jun 13, 2021

svbutko commented Sep 29, 2021

facebook-github-bot commented Sep 29, 2021

react-native-bot commented Oct 6, 2021

mrbrentkelly commented Jun 10, 2020 •

edited

Loading

analysis-bot commented Jun 10, 2020 •

edited

Loading

analysis-bot commented Jun 10, 2020 •

edited

Loading

mrbrentkelly commented Aug 27, 2020 •

edited

Loading