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.

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

[TextInput][iOS] Default color should adapt to Dark Mode #38014

Closed
wants to merge 1 commit into from

Conversation

Saadnajmi
Copy link
Contributor

Summary:

Currently, TextInputs on iOS explicitly default to having black as the text color. This seems wrong: ever since dark mode was introduced, defaults should adapt to the OS (Be it Light/Dark/Increase Contrast / Dark Elevated, etc). That is the benefit of using UIKit system controls, they're tightly coupled to the OS. If the user wanted the TextInput to always be a specific color (because they hardcoded the background color), then they can specify that in the style prop. If their app doesn't support dark mode, then their app plist should reflect that.

I looked into the commit history, and it seems the change was introduced here. It seems that when Dark Mode was introduced, the "default" text color changing to depend on system theme was seen as an RN regression. This is simply not true, iOS Dark Mode was opt in, and intentionally changed the defaults and asked apps to adapt their style to match. The "breaking change" was not in RN, but in the OS itself. Had the app been purely native, there would still be a change. I don't think it's RN's job to enforce a default when the OS itself has changed it, I think RN should follow the OS.

Changelog:

[IOS] [FIXED] - TextInput default color should adapt to Dark Mode

Test Plan:

Tested the TextInput example page on RNTester in dark mode before and after my change.

Before:
Screenshot 2023-06-21 at 11 09 26 PM
After:
Screenshot 2023-06-21 at 11 21 08 PM

@facebook-github-bot 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: Microsoft Partner: Microsoft Partner labels Jun 22, 2023
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,966,498 +0
android hermes armeabi-v7a 8,227,758 +0
android hermes x86 9,480,122 +3
android hermes x86_64 9,322,876 +0
android jsc arm64-v8a 9,527,585 +0
android jsc armeabi-v7a 8,666,173 -2
android jsc x86 9,612,089 +1
android jsc x86_64 9,858,942 -2

Base commit: 0e41ad0
Branch: main

@NickGerleman
Copy link
Contributor

I think this change is likely to break existing apps which are not color scheme aware.

I think this is more a bug that RNTester changes background colors in response to color scheme but not text color.

@NickGerleman
Copy link
Contributor

If their app doesn't support dark mode, then their app plist should reflect that.

Imagine a scenario where an app has multiple surfaces. Some are color scheme aware and subscribe to that information from RN, but sone are not scheme aware.

Would RN need to tell iOS that it is color scheme aware in order to be able to observe these changes?

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Jun 22, 2023

If their app doesn't support dark mode, then their app plist should reflect that.

Imagine a scenario where an app has multiple surfaces. Some are color scheme aware and subscribe to that information from RN, but sone are not scheme aware.

Would RN need to tell iOS that it is color scheme aware in order to be able to observe these changes?

When you say, "color scheme aware", do you mean that the RN surface has been designed for both light and dark mode? So in your case an app might have surface 1 and 2 be color scheme aware, but not surface 3? If so, then it sounds like surface 3 has a bug.

@Saadnajmi
Copy link
Contributor Author

I think this change is likely to break existing apps which are not color scheme aware.

I think this is more a bug that RNTester changes background colors in response to color scheme but not text color.

A few more notes:

  • Dark Mode was initially opt-in, I'm not sure about the current state anymore.
  • All of the system text (and therefore RN Text) changes color, along with all the RN components that leverage UIKit. And also anything that uses either PlatformColor or DynamicColorIOS.
  • It is a breaking change in that, Apple made it a breaking (but opt in) change for everyone when Dark Mode was introduced. No, several years later, every OS that RN supports has a concept of a dark mode, and I believe it's considered a minimum for an experience to adapt to light/dark. If an experience (in a multi-experience app that supports dark mode) isn't color scheme aware, it feels like it is making the active decision to do so, and should set its' own colors, rather than rely on the system defaults.

@NickGerleman
Copy link
Contributor

When you say, "color scheme aware", do you mean that the RN surface has been designed for both light and dark mode?

So, say there is some code written before dark mode which sets a background to white, and adds a TextInput. At the time it was written, the TextInput would show black on white.

Now later, we have color schemes, and hooks like useColorScheme to power theming. We can now add specific behavior in dark mode, to specify a new background color, new text color, etc. This requires being able to observe the color scheme.

The place where I think this might fall down a bit is if you are embedding some leaf-node native component, where it would be themed according to color scheme.

On the native iOS side, you mentioned an app-wide plist. Is there something more scoped for this sort of thing? I still expect this change is probably too breaking though.

@Saadnajmi
Copy link
Contributor Author

When you say, "color scheme aware", do you mean that the RN surface has been designed for both light and dark mode?

So, say there is some code written before dark mode which sets a background to white, and adds a TextInput. At the time it was written, the TextInput would show black on white.

Now later, we have color schemes, and hooks like useColorScheme to power theming. We can now add specific behavior in dark mode, to specify a new background color, new text color, etc. This requires being able to observe the color scheme.

The place where I think this might fall down a bit is if you are embedding some leaf-node native component, where it would be themed according to color scheme.

On the native iOS side, you mentioned an app-wide plist. Is there something more scoped for this sort of thing? I still expect this change is probably too breaking though.

Every UIView in iOS has a UITraitCollection that tells you its current color scheme. This allows you to force specific views to be light/dark as you wish, I believe. Prior to RN 0.72, there wasn't a way to set that. Now, there's the setColorScheme() hook. Looking at its implementation, it is setting it app wide instead of just the root view, but 🤷🏽‍♂️.

The leaf node native component would follow the OS unless you pass in colors to it, yes. So if you had a light-only experience in dark mode, you didn't call setColorScheme('light') in JS, and passed in a leaf node native component, it would render in dark mode and look off. This is exactly why I think it's important all experiences are color scheme aware, as that's basically a requirement now for any framework using native components.

Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 20, 2023
@Saadnajmi
Copy link
Contributor Author

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

Eh... will revisit in new years. I want to do a comparison of native platforms & their dark modes and see what the standard is outside of RN

@github-actions github-actions bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 21, 2023
Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jun 18, 2024
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Jun 25, 2024
@Saadnajmi Saadnajmi deleted the textinput-color branch July 9, 2024 06:37
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. p: Microsoft Partner: Microsoft Partner Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants