react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Fix Application always in light mode on initial load.

Open filip131311 opened this issue 1 month ago • 8 comments

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.

filip131311 avatar Apr 30 '24 13:04 filip131311

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 [email protected]. Thanks!

facebook-github-bot avatar Apr 30 '24 13:04 facebook-github-bot

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 avatar Apr 30 '24 14:04 facebook-github-bot

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

filip131311 avatar Apr 30 '24 15:04 filip131311

@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?

filip131311 avatar Apr 30 '24 18:04 filip131311

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 avatar May 01 '24 12:05 cipolleschi

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 avatar May 01 '24 13:05 cipolleschi

/rebase - this comment automatically rebase on top of main

cipolleschi avatar May 01 '24 13:05 cipolleschi

@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?

filip131311 avatar May 06 '24 14:05 filip131311