fix: return the correct default trait collection #38214
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
Base commit: 41477c8 |
cipolleschi
requested changes
Sep 9, 2023
There was a problem hiding this comment.
Hi @alanjhughes, thanks for taking the time for doing this.
I think this can fix multiple issue we had on React Native related to the same root cause, so I'm very keen to land it.
I suggested a change to simplify the code: once updated I'll import it.... Hoping that our internal tests will actually pass. If that won't be the case, we would have to figure out a way to solve this... but hopefully there would not be the need!
cipolleschi
reviewed
Sep 9, 2023
|
Thanks for the review @cipolleschi Sorry for the delay. I've made the changes |
alanjhughes
force-pushed
the
fix/return-correct-appearance
branch
from
e788f63
to
3da09de
Compare
cipolleschi
approved these changes
Sep 12, 2023
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request was successfully merged by @alanjhughes in 94fea18. When will my fix make it into a release? | Upcoming Releases |
adamaveray
added a commit
to adamaveray/react-native
that referenced
this pull request
Sep 13, 2023
* main: (1012 commits) Add simple constructor for JSError (facebook#39415) Breaking: per-node pointScaleFactor (facebook#39403) Implement "tickleJS" for Hermes (facebook#39289) Add thread idle indicator (facebook#39206) Unblock `yarn android` on main (facebook#39413) Remove Codegen buck-oss scripts as they're unused (facebook#39422) Immediately dispatch events to the shared C++ infrastructure to support interruptability (facebook#39380) Fix race condition in Binding::reportMount (facebook#39420) Clone free state progression (facebook#39357) fix: return the correct default trait collection (facebook#38214) Read the React Native version and set the new arch flag properly (facebook#39388) Deprecate default_flags in Podfile (facebook#39389) Create Helper to read the package.json from Cocoapods (facebook#39390) Create helper to enforce the New Arch enabled for double released (facebook#39387) Remove layoutContext Drilling (facebook#39401) Remove JNI Binding usage of layoutContext (facebook#39402) Extract isBaselineLayout() (facebook#39400) Refactor and separate layout affected nodes react marker (facebook#39249) Bump AGP to 8.1.1 (facebook#39392) Fix broken Gradle Sync when opening the project with Android Studio (facebook#39412) ...
|
This pull request has been reverted by 7888338. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 26, 2023
Summary: Closes #35972 Closes #36713 This PR addresses a couple of issues with `useColorScheme` and the `Appearance` API. - #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: #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
ShevO27
pushed a commit
to ShevO27/react-native
that referenced
this pull request
Sep 26, 2023
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Currently, when the
_currentColorSchemeis nil the default comes from[UITraitCollection currentTraitCollection]which provides the trait collection of the context it was called in. Generally, this will work but in some cases the context can be different and this will return the wrong value also if theAppearanceproperty is set in the plist, then initially that value is returned, regardless of if you have overridden it. Seen as theuserInterfaceStyleof the window is overridden when the value is changed, then the default should be whatever the current style of the window is and not dependent on the calling context.Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-07-06.at.09.24.05.mp4
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-07-06.at.09.25.08.mp4
Changelog:
[IOS] [FIXED] - Fix the default trait collection to always return the value of the window
Test Plan:
Tested on rn-tester and verified the current behaviour is unaffected and also in our own repo to make sure it addresses the problem. Video provided above