Skip to content

Fix Application always in light mode on initial load. #44335

Closed
filip131311 wants to merge 3 commits into
facebook:mainfrom
filip131311:main
Closed

Fix Application always in light mode on initial load. #44335
filip131311 wants to merge 3 commits into
facebook:mainfrom
filip131311:main

Conversation

@filip131311
Copy link
Copy Markdown
Contributor

@filip131311 filip131311 commented Apr 30, 2024

Hi, I'm Filip from software mansion. This PR solves a problem I stumbled upon.

Summary:

On iOS, applications are always in light mode on initial load. Even if the device is turned to dark mode.

Cause of the problem:

The initial appearance is taken from RCTKeyWindow(), but at the time of initialization of RCTAppearance it does not exist yet.

Solution:

This PR moves repeats initialization of the appearance the first time getColorScheme() is called if it was not initialized properly before.

Changelog:

[IOS] [FIXED] - Fix dark mode on initial load.

Test Plan:

  • Create new React native app with npx react-native@latest init AwesomeProjec
  • Run the application on iphone using simulator
  • turn on dark mode using cmd+shift+A
  • close application and run it again

without changes:

The application will turn on in light mode despite the simulator being set to dark mode.
When you reload the application it works as expected (is in dark mode)

with changes:

Works as expected

note:

any change to device ui settings will trigger a listener that will set appearance to correct state, so testing of this problem should happen in as isolated conditions as possible.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

Hi @filip131311!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, 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.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

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

@facebook-github-bot 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 Apr 30, 2024
@facebook-github-bot
Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot 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 Apr 30, 2024
Copy link
Copy Markdown
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Hi @filip131311, thank you for the contribution!

there are some extra changes we might want to do.

Copy link
Copy Markdown
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Sorry for the double ping @filip131311.

There are some tests that are failing in CI.

-[RNTesterIntegrationTests testAccessibilityManagerTest]
-[RNTesterIntegrationTests testAppEventsTest]
-[RNTesterIntegrationTests testImageSnapshotTest]
-[RNTesterIntegrationTests testIntegrationTestHarnessTest]
-[RNTesterIntegrationTests testPromiseTest]
-[RNTesterIntegrationTests testSimpleSnapshotTest]
-[RNTesterIntegrationTests testTheTester_waitOneFrame]
-[RNTesterIntegrationTests testWebSocketTest]

You can test them by running the script

./scripts/objc-test.sh

from the React Native root folder.

I think that the problem is that these tests tries to access _currentColorScheme without ever invoking the getColorScheme method and it fails.

I think we can:

  1. add a new constant in RCTColorSchemePreference of unknown if the appearance is not ready yet.
  2. keep initializing the _currentColorScheme in the initializer, but initialize it to "unknown"
  3. change the check in getColorScheme to
    if ([_currentColorScheme isEqualToString: RCTAppearanceColorSchemeUnknown]) {
        UITraitCollection *traitCollection = RCTKeyWindow().traitCollection;
        _currentColorScheme = RCTColorSchemePreference(traitCollection);
      }

What do you think?

@filip131311
Copy link
Copy Markdown
Contributor Author

@cipolleschi I think you're right, I'll try going with your approach, Thank you!

@filip131311
Copy link
Copy Markdown
Contributor Author

@cipolleschi Hello, I had a chance to look at the problem and unfortunately can not reproduce the CI failure.

  1. Running test with ./scripts/objc-test.sh I get TEST SUCCEEDED:
Screenshot 2024-04-30 at 20 30 13

Full log file is here:
Run-RNTester-2024.04.30_20-20-55-+0200.xcresult.zip

  1. Running tests with xcode I get:
Screenshot 2024-04-30 at 19 23 31

so there is one fail but expected one, since I did not start web socket server by hand.

Furthermore, looking at my changes I don't agree that it is possible for anything to try and access _currentColorScheme before it is initialized, because the only other time in the entire codebase it is called is in appearanceChanged function in the same file, but if it was nil there it would also just be initialized.

Do you have any thoughts?

@cipolleschi
Copy link
Copy Markdown
Contributor

hum.. tests are failing and before they weren't.. So there is something. I've tried to rerun the jobs, let's see if it is a fluke of the CI, although 4 jobs out of 4 testing it seems pretty systematic to me.

@cipolleschi
Copy link
Copy Markdown
Contributor

I restarted the jobs but they are still failing.
Let's try to rebase on top of main, but I think that there is something wrong here.

@cipolleschi
Copy link
Copy Markdown
Contributor

/rebase - this comment automatically rebase on top of main

@filip131311
Copy link
Copy Markdown
Contributor Author

filip131311 commented May 6, 2024

@cipolleschi I am sorry it took me so long, but I added changes that fix the issue. It does not add a 3rd value as you suggested, because it could interfere with the rest of the code base, instead I added a static bool that indicates if the _currentColorSheme is Real or a default value.

What do you think?

@algarcia-vector
Copy link
Copy Markdown

I am getting this issue too, when is going to be fixed? :D

@facebook-github-bot
Copy link
Copy Markdown
Contributor

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

@filip131311
Copy link
Copy Markdown
Contributor Author

filip131311 commented Jun 6, 2024

@cipolleschi Hey, should I resolve the conflict? I wouldn't ask but it seems you already pulled my commits to internal version

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jun 7, 2024
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@cipolleschi merged this pull request in b957513.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 7, 2024

This pull request was successfully merged by filip131311 in b957513.

When will my fix make it into a release? | How to file a pick request?

kosmydel pushed a commit to kosmydel/react-native that referenced this pull request Jun 11, 2024
Summary:
Hi, I'm Filip from software mansion.  This PR solves a problem I stumbled upon.

On iOS, applications are always in light mode on initial load. Even if the device is turned to dark mode.

### Cause of the problem:

The initial appearance is taken from `RCTKeyWindow()`, but at the time of initialization of `RCTAppearance` it does not exist yet.

### Solution:

This PR moves repeats initialization of the appearance the first time `getColorScheme()` is called if it was not initialized properly before.

## Changelog:

[IOS] [FIXED] - Fix dark mode on initial load.

Pull Request resolved: facebook#44335

Test Plan:
- Create new React native app with `npx react-native@latest init AwesomeProjec`
- Run the application on iphone using simulator
- turn on dark mode using `cmd+shift+A`
- close application and run it again

### without changes:
  The application will turn on in light mode despite the simulator being set to dark mode.
  When you reload the application it works as expected (is in dark mode)

### with changes:
  Works as expected

#### note:
any change to device ui settings will trigger a listener that will set appearance to correct state, so testing of this problem should happen in as isolated conditions as possible.

Reviewed By: cortinico

Differential Revision: D58189058

Pulled By: cipolleschi

fbshipit-source-id: 9a864f3d045e966bc88601f661d221c4796c5c95
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants