Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: over reporting trait changes #39439

Closed
wants to merge 6 commits into from
Closed

fix: over reporting trait changes #39439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
65eae90
fix: don't report trait changes when in background
alanjhughes Sep 13, 2023
0aae245
Make requesting the trait collection synchronous
alanjhughes Sep 14, 2023
e01e081
use RCTUnsafeExecuteOnMainQueueSync
alanjhughes Sep 14, 2023
68a4887
get initial value in initializer
alanjhughes Sep 15, 2023
de742dc
add invalidate
alanjhughes Sep 20, 2023
6ef6687
remove observers
alanjhughes Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/react-native/React/Base/RCTRootView.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,9 @@ - (void)contentViewInvalidated
- (void)traitCollectionDidChange:(UITraitCollection *)previousTraitCollection
{
[super traitCollectionDidChange:previousTraitCollection];
if (RCTSharedApplication().applicationState == UIApplicationStateBackground) {
return;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@alanjhughes alanjhughes Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[[NSNotificationCenter defaultCenter]
postNotificationName:RCTUserInterfaceStyleDidChangeNotification
Expand Down
1 change: 1 addition & 0 deletions packages/react-native/React/CoreModules/RCTAppearance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ RCT_EXTERN NSString *RCTCurrentOverrideAppearancePreference();
RCT_EXTERN NSString *RCTColorSchemePreference(UITraitCollection *traitCollection);

@interface RCTAppearance : RCTEventEmitter <RCTBridgeModule>
- (instancetype)init;
@end
32 changes: 15 additions & 17 deletions packages/react-native/React/CoreModules/RCTAppearance.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,7 @@ void RCTOverrideAppearancePreference(NSString *const colorSchemeOverride)
return RCTAppearanceColorSchemeLight;
}

traitCollection = traitCollection ?: [UITraitCollection currentTraitCollection];
return appearances[@(traitCollection.userInterfaceStyle)] ?: RCTAppearanceColorSchemeLight;

// Default to light on older OS version - same behavior as Android.
return RCTAppearanceColorSchemeLight;
}

@interface RCTAppearance () <NativeAppearanceSpec>
Expand All @@ -71,6 +67,19 @@ @implementation RCTAppearance {
NSString *_currentColorScheme;
}

- (instancetype)init
{
if ((self = [super init])) {
UITraitCollection *traitCollection = RCTSharedApplication().delegate.window.traitCollection;
_currentColorScheme = RCTColorSchemePreference(traitCollection);
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(appearanceChanged:)
name:RCTUserInterfaceStyleDidChangeNotification
object:nil];
}
Comment on lines +75 to +79
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return self;
}

RCT_EXPORT_MODULE(Appearance)

+ (BOOL)requiresMainQueueSetup
Expand Down Expand Up @@ -100,9 +109,6 @@ - (dispatch_queue_t)methodQueue

RCT_EXPORT_SYNCHRONOUS_TYPED_METHOD(NSString *, getColorScheme)
{
if (_currentColorScheme == nil) {
_currentColorScheme = RCTColorSchemePreference(nil);
}
return _currentColorScheme;
}

Expand All @@ -127,16 +133,8 @@ - (void)appearanceChanged:(NSNotification *)notification
return @[ @"appearanceChanged" ];
}

- (void)startObserving
{
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(appearanceChanged:)
name:RCTUserInterfaceStyleDidChangeNotification
object:nil];
}

- (void)stopObserving
{
- (void)invalidate {
[super invalidate];
[[NSNotificationCenter defaultCenter] removeObserver:self];
}

Expand Down