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

Add Appearance.setColorScheme support #35989

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions Libraries/Utilities/Appearance.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ export namespace Appearance {
*/
export function getColorScheme(): ColorSchemeName;

/**
* Set the color scheme preference. This is useful for overriding the default
* color scheme preference for the app. Note that this will not change the
* appearance of the system UI, only the appearance of the app.
* Only available on iOS 13+ and Android 10+.
*/
export function setColorScheme(
scheme: ColorSchemeName | null | undefined,
): void;

/**
* Add an event handler that is fired when appearance preferences change.
*/
Expand Down
13 changes: 13 additions & 0 deletions Libraries/Utilities/Appearance.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,19 @@ module.exports = {
return nativeColorScheme;
},

setColorScheme(colorScheme: ?ColorSchemeName): void {
const nativeColorScheme = colorScheme == null ? 'unspecified' : colorScheme;

invariant(
colorScheme === 'dark' || colorScheme === 'light' || colorScheme == null,
"Unrecognized color scheme. Did you mean 'dark', 'light' or null?",
);

if (NativeAppearance != null) {
NativeAppearance.setColorScheme(nativeColorScheme);
}
},

/**
* Add an event handler that is fired when appearance preferences change.
*/
Expand Down
1 change: 1 addition & 0 deletions Libraries/Utilities/NativeAppearance.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export interface Spec extends TurboModule {
// types.
/* 'light' | 'dark' */
+getColorScheme: () => ?string;
+setColorScheme: (colorScheme: string) => void;

// RCTEventEmitter
+addListener: (eventName: string) => void;
Expand Down
1 change: 1 addition & 0 deletions React/CoreModules/RCTAppearance.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#import <UIKit/UIKit.h>

#import <React/RCTConvert.h>
#import <React/RCTBridgeModule.h>
#import <React/RCTEventEmitter.h>

Expand Down
26 changes: 26 additions & 0 deletions React/CoreModules/RCTAppearance.mm
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#import <FBReactNativeSpec/FBReactNativeSpec.h>
#import <React/RCTConstants.h>
#import <React/RCTEventEmitter.h>
#import <React/RCTUtils.h>

#import "CoreModulesPlugins.h"

Expand Down Expand Up @@ -68,6 +69,20 @@ void RCTOverrideAppearancePreference(NSString *const colorSchemeOverride)
return RCTAppearanceColorSchemeLight;
}

@implementation RCTConvert (UIUserInterfaceStyle)

RCT_ENUM_CONVERTER(
UIUserInterfaceStyle,
(@{
@"light" : @(UIUserInterfaceStyleLight),
@"dark" : @(UIUserInterfaceStyleDark),
@"unspecified" : @(UIUserInterfaceStyleUnspecified)
}),
UIUserInterfaceStyleUnspecified,
integerValue);

@end

@interface RCTAppearance () <NativeAppearanceSpec>
@end

Expand All @@ -92,6 +107,17 @@ - (dispatch_queue_t)methodQueue
return std::make_shared<NativeAppearanceSpecJSI>(params);
}

RCT_EXPORT_METHOD(setColorScheme : (NSString *)style) {
UIUserInterfaceStyle userInterfaceStyle = [RCTConvert UIUserInterfaceStyle:style];
NSArray<__kindof UIWindow*>* windows = RCTSharedApplication().windows;
if (@available(iOS 13.0, *)) {
for (UIWindow *window in windows)
{
window.overrideUserInterfaceStyle = userInterfaceStyle;
}
}
}

RCT_EXPORT_SYNCHRONOUS_TYPED_METHOD(NSString *, getColorScheme)
{
if (_currentColorScheme == nil) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import android.content.Context;
import android.content.res.Configuration;
import androidx.annotation.Nullable;
import androidx.appcompat.app.AppCompatDelegate;
import com.facebook.fbreact.specs.NativeAppearanceSpec;
import com.facebook.react.bridge.Arguments;
import com.facebook.react.bridge.ReactApplicationContext;
Expand Down Expand Up @@ -66,6 +67,17 @@ private String colorSchemeForCurrentConfiguration(Context context) {
return "light";
}

@Override
public void setColorScheme(String style) {
if (style == "dark") {
Copy link
Contributor

Choose a reason for hiding this comment

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

@birkir I missed that birkir@e633876 changed this from .equals() to ==.

== against two Java strings does a reference comparison, instead of a value comparison. So it is not safe for using to compare string value.

Would you be willing to make a followup change to move this back to .equals()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good spot and @birkir can you follow up with a test case to cover this please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey there @birkir @jacdebug. Because I think this change may not work, and we don't have any RNTester page or automated tests to verify a fix, I'm going to revert this change.

Do feel free to resubmit this change, with a fix, and test collateral. I do think this API makes sense as part of Appearance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey there @birkir @jacdebug. Because I think this change may not work, and we don't have any RNTester page or automated tests to verify a fix, I'm going to revert this change

Hopefully we'll have it soon (cc @kelset :))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, added RNTester and fixed the bug in a new PR #36122

In retrospect, I think the strict equals bug came about because of coding style change (yoda) and the fact Kotlin uses == the same as Java equals.

AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_YES);
} else if (style == "light") {
AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_NO);
} else if (style == "unspecified") {
AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM);
}
}

@Override
public String getColorScheme() {
// Attempt to use the Activity context first in order to get the most up to date
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ rn_android_library(
deps = [
react_native_dep("third-party/java/jsr-305:jsr-305"),
react_native_dep("third-party/android/androidx:annotation"),
react_native_dep("third-party/android/androidx:appcompat"),
react_native_target("java/com/facebook/react/bridge:bridge"),
react_native_target("java/com/facebook/react/common:common"),
react_native_target("java/com/facebook/react/module/annotations:annotations"),
Expand Down
1 change: 1 addition & 0 deletions ReactAndroid/src/main/java/com/facebook/react/shell/BUCK
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ rn_android_library(
react_native_dep("libraries/fresco/fresco-react-native:imagepipeline"),
react_native_dep("libraries/soloader/java/com/facebook/soloader:soloader"),
react_native_dep("third-party/android/androidx:annotation"),
react_native_dep("third-party/android/androidx:appcompat"),
react_native_dep("third-party/android/androidx:core"),
react_native_dep("third-party/android/androidx:fragment"),
react_native_dep("third-party/android/androidx:legacy-support-core-utils"),
Expand Down