fix: over reporting trait changes #39439

alanjhughes · 2023-09-14T08:07:31Z

Summary:

This PR addresses a couple of issues with useColorScheme and the Appearance API.

fix: return the correct default trait collection #38214 introduced a regression. Using to RCTExecuteOnMainQueue was a mistake as we need this to happen synchronously to return the result. Doing it async causes the traitCollection to remain uninitialized.
The useColorScheme hook is updating when the app is in the background on iOS and the OS is taking the snapshots for the app switcher. This causes a flash when returning to the app as the correct color is set again. Here, we can check for the app state in traitCollectionDidChange and not send these events when in the background.
Removed a line that was left over after some OS version checks were removed when support for iOS 12 was dropped.

Changelog:

[IOS] [FIXED] - Don't send the RCTUserInterfaceStyleDidChangeNotification when the app is in the background.

Test Plan:

Tested on rn-tester, logged the changes whenever useColorScheme updates. It no longer happens when the app is in the background. The returned interface style on the initial render is always correct now.

javache · 2023-09-14T13:20:42Z

packages/react-native/React/CoreModules/RCTAppearance.mm

+    return RCTSharedApplication().delegate.window.traitCollection;
+  } else {
+    __block UITraitCollection* traitCollection = nil;
+    dispatch_sync(dispatch_get_main_queue(), ^{


We should avoid using dispatch_sync as much as possible, as it can lead to deadlocks.

This module is marked as requiresMainQueueSetup, which means the constructor is called from the main thread. Is there any way we can access this attribute there?

Also, as applies to most classes in UIKit, this class is not safe to use outside the main thread. We should grab just the userInterfaceStyle prop, and not leak anything else.

Something like this?

@implementation RCTAppearance { NSString *_currentColorScheme; } - (instancetype)init { if ((self = [super init])) { UITraitCollection *traitCollection = RCTSharedApplication().delegate.window.traitCollection; _currentColorScheme = RCTColorSchemePreference(traitCollection); } return self; }

javache · 2023-09-18T16:35:21Z

packages/react-native/React/CoreModules/RCTAppearance.mm

+  if ((self = [super init])) {
+    UITraitCollection *traitCollection = RCTSharedApplication().delegate.window.traitCollection;
+    _currentColorScheme = RCTColorSchemePreference(traitCollection);
+  }
+  return self;


Perhaps we should set-up the observer here by default (now in startObserving). Since otherwise _currentColorScheme will go out of date if there's no listeners on the JS side.

javache · 2023-09-20T09:53:37Z

7888338 was just backed out, could you rebase this on top of that?

alanjhughes · 2023-09-20T09:54:58Z

@javache - Yes, that's done, just waiting on CI to finish

javache · 2023-09-20T09:56:34Z

packages/react-native/React/CoreModules/RCTAppearance.mm

+    [[NSNotificationCenter defaultCenter] addObserver:self
+                                             selector:@selector(appearanceChanged:)
+                                                 name:RCTUserInterfaceStyleDidChangeNotification
+                                               object:nil];
+  }


You should move the logic from stopObserving to invalidate since we always want to keep the listener.

facebook-github-bot · 2023-09-20T10:34:11Z

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

github-actions · 2023-09-20T11:42:24Z

	Warnings
⚠️	One hour and a half have passed and the E2E jobs haven't finished yet.

Generated by 🚫 dangerJS against de742dc

javache · 2023-09-21T12:13:10Z

packages/react-native/React/Base/RCTRootView.m

@@ -368,6 +368,9 @@ - (void)contentViewInvalidated
 - (void)traitCollectionDidChange:(UITraitCollection *)previousTraitCollection
 {
  [super traitCollectionDidChange:previousTraitCollection];
+  if (RCTSharedApplication().applicationState == UIApplicationStateBackground) {
+    return;
+  }



To confirm: where do we update the trait collection when the application foregrounds? If you change to dark mode while the app is backgrounded, do we pick it up on foreground?

We'll get it on the foreground but we're in the foreground state before the user sees it so the change isn't visible to them

The main issue I was trying to solve is when _currentColorScheme was set to nil initially, the default color scheme was light so on first render it would be light regardless of the users settings. So if the users device was in dark mode on initial render it would be reported as light until you toggled it and then it synced up

Right, the changes to _currentColorScheme makes sense, I'm just a bit confused about how the RCTRootView changes are related here. Is it required as part of this PR?

We'll get it on the foreground but we're in the foreground state

Where does this happen? Does traitCollectionDidChange when the application is foregrounded?

Yes, currently traitCollectionDidChangewill fire twice when the app moves to the background because the OS is taking the snapshots for the app switcher. It'll take one using the current scheme. Switch to the opposite and then switch back to the original. The changes in RCTRootView is just to ignore those changes.

github-actions · 2023-09-26T03:05:35Z

This pull request was successfully merged by @alanjhughes in 6118aff.

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

Summary: Closes facebook#35972 Closes facebook#36713 This PR addresses a couple of issues with `useColorScheme` and the `Appearance` API. - facebook#38214 introduced a regression. Using to `RCTExecuteOnMainQueue` was a mistake as we need this to happen synchronously to return the result. Doing it async causes the `traitCollection` to remain uninitialized. - The `useColorScheme` hook is updating when the app is in the background on iOS and the OS is taking the snapshots for the app switcher. This causes a flash when returning to the app as the correct color is set again. Here, we can check for the app state in `traitCollectionDidChange` and not send these events when in the background. - Removed a line that was left over after some OS version checks were removed when support for iOS 12 was dropped. ## Changelog: [IOS] [FIXED] - Don't send the `RCTUserInterfaceStyleDidChangeNotification` when the app is in the background. Pull Request resolved: facebook#39439 Test Plan: Tested on `rn-tester`, logged the changes whenever `useColorScheme` updates. It no longer happens when the app is in the background. The returned interface style on the initial render is always correct now. Reviewed By: NickGerleman Differential Revision: D49454281 Pulled By: javache fbshipit-source-id: 87e24158a49c50608c79e73fb484442f5aad36a6

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. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 14, 2023

alanjhughes force-pushed the fix/overreporting-trait-changes branch from cffdb07 to 5715b14 Compare September 14, 2023 08:43

javache requested changes Sep 14, 2023

View reviewed changes

This was referenced Sep 14, 2023

fix theme flashing bluesky-social/social-app#1417

Merged

fix theme switching on web without refresh bluesky-social/social-app#1451

Merged

alanjhughes requested a review from javache September 15, 2023 10:29

javache requested changes Sep 18, 2023

View reviewed changes

alanjhughes requested review from javache September 19, 2023 08:33

alanjhughes mentioned this pull request Sep 20, 2023

fix: return the correct default trait collection #38214

Closed

alanjhughes added 4 commits September 20, 2023 09:41

fix: don't report trait changes when in background

65eae90

Make requesting the trait collection synchronous

0aae245

use RCTUnsafeExecuteOnMainQueueSync

e01e081

get initial value in initializer

68a4887

alanjhughes force-pushed the fix/overreporting-trait-changes branch from 18ce003 to 68a4887 Compare September 20, 2023 08:46

javache requested a review from philIip September 20, 2023 09:53

javache reviewed Sep 20, 2023

View reviewed changes

alanjhughes added 2 commits September 20, 2023 11:10

add invalidate

de742dc

remove observers

6ef6687

javache reviewed Sep 21, 2023

View reviewed changes

github-actions bot added the Merged This PR has been merged. label Sep 26, 2023

facebook-github-bot closed this in 6118aff Sep 26, 2023

estrattonbailey mentioned this pull request Dec 27, 2023

Remove patched color scheme code bluesky-social/social-app#2340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: over reporting trait changes #39439

fix: over reporting trait changes #39439

alanjhughes commented Sep 14, 2023 •

edited

Loading

javache Sep 14, 2023

alanjhughes Sep 14, 2023 •

edited

Loading

javache Sep 18, 2023

javache commented Sep 20, 2023 •

edited

Loading

alanjhughes commented Sep 20, 2023

javache Sep 20, 2023

alanjhughes Sep 20, 2023

facebook-github-bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

javache Sep 21, 2023

alanjhughes Sep 21, 2023

alanjhughes Sep 21, 2023

javache Sep 25, 2023

alanjhughes Sep 25, 2023 •

edited

Loading

github-actions bot commented Sep 26, 2023

fix: over reporting trait changes #39439

fix: over reporting trait changes #39439

Conversation

alanjhughes commented Sep 14, 2023 • edited Loading

Summary:

Changelog:

Test Plan:

Choose a reason for hiding this comment

alanjhughes Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javache commented Sep 20, 2023 • edited Loading

alanjhughes commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

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

alanjhughes Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 26, 2023

alanjhughes commented Sep 14, 2023 •

edited

Loading

alanjhughes Sep 14, 2023 •

edited

Loading

javache commented Sep 20, 2023 •

edited

Loading

alanjhughes Sep 25, 2023 •

edited

Loading