diff --git a/docs/architecture/react.md b/docs/architecture/react.md index 5d063e7bec0..c64fcfd7fb2 100644 --- a/docs/architecture/react.md +++ b/docs/architecture/react.md @@ -141,20 +141,19 @@ doc](https://reactjs.org/docs/context.html)): > [...]) > is not subject to the shouldComponentUpdate method -We also confirmed this behavior experimentally, in a 2020 version of -`MessageList` which used `ThemeContext` to get the theme colors. -(Since 1ba871910, `MessageList` uses a transparent background and so -doesn't need the theme; since 4fa2418b8 it doesn't mention -`ThemeContext`.) That component re-`render`ed when the theme changed, -*even though its `shouldComponentUpdate` always returned `false`*. -This didn't cause a live problem because the UI doesn't -allow changing the theme while a `MessageList` is in the navigation -stack. If it were possible, it would be a concern: setting a short -interval to automatically toggle the theme, we see that the message -list's color scheme changes as we'd want it to, but we also see the -bad effects that `shouldComponentUpdate` returning `false` is meant to -prevent: losing the scroll position, mainly (but also, we expect, -discarding the image cache, etc.). +Concretely, this means that our `MessageList` component updates +(re-`render`s) when the theme changes, since it's a `ThemeContext` +consumer, *even though its `shouldComponentUpdate` always returns +`false`*. This generally isn't a problem because the UI for +changing our own theme setting can't appear while a `MessageList` is +in the navigation stack; so the theme can change only once we have +#4009, via the OS-level theme changing (either automatically on +schedule, or because the user changed it in system settings.) When +this does happen we see that the message list's color scheme changes +as we'd want it to, but we also see the bad effects that +`shouldComponentUpdate` returning `false` is meant to prevent: +losing the scroll position, mainly (but also, we expect, discarding +the image cache, etc.). ### The exception: `MessageList` diff --git a/src/boot/OfflineNoticeProvider.js b/src/boot/OfflineNoticeProvider.js index a08cae6d5f5..9512e6fcd45 100644 --- a/src/boot/OfflineNoticeProvider.js +++ b/src/boot/OfflineNoticeProvider.js @@ -1,7 +1,15 @@ // @flow strict-local import React, { useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react'; import type { Node } from 'react'; -import { AccessibilityInfo, View, Animated, LayoutAnimation, Platform, Easing } from 'react-native'; +import { + AccessibilityInfo, + View, + Animated, + LayoutAnimation, + Platform, + Easing, + useColorScheme, +} from 'react-native'; import NetInfo from '@react-native-community/netinfo'; import { SafeAreaView } from 'react-native-safe-area-context'; import type { ViewProps } from 'react-native/Libraries/Components/View/ViewPropTypes'; @@ -10,6 +18,7 @@ import type { DimensionValue } from 'react-native/Libraries/StyleSheet/StyleShee import * as logging from '../utils/logging'; import { useDispatch, useGlobalSelector } from '../react-redux'; import { getGlobalSession, getGlobalSettings } from '../directSelectors'; +import { getThemeToUse } from '../settings/settingsSelectors'; import { useHasStayedTrueForMs, usePrevious } from '../reactUtils'; import type { JSONableDict } from '../utils/jsonable'; import { createStyleSheet } from '../styles'; @@ -131,7 +140,7 @@ const backgroundColorForTheme = (theme: ThemeName): string => // TODO(redesign): Choose these more intentionally; these are just the // semitransparent HALF_COLOR flattened with themeData.backgroundColor. // See https://github.com/zulip/zulip-mobile/pull/5491#issuecomment-1282859332 - theme === 'default' ? '#bfbfbf' : '#50565e'; + theme === 'light' ? '#bfbfbf' : '#50565e'; /** * Shows a notice if the app is working in offline mode. @@ -148,6 +157,8 @@ const backgroundColorForTheme = (theme: ThemeName): string => */ export function OfflineNoticeProvider(props: ProviderProps): Node { const theme = useGlobalSelector(state => getGlobalSettings(state).theme); + const osScheme = useColorScheme(); + const themeToUse = getThemeToUse(theme, osScheme); const _ = useContext(TranslationContext); const isOnline = useGlobalSelector(state => getGlobalSession(state).isOnline); const shouldShowUncertaintyNotice = useShouldShowUncertaintyNotice(); @@ -249,7 +260,7 @@ export function OfflineNoticeProvider(props: ProviderProps): Node { // If changing, also change the status bar color in // OfflineNoticePlaceholder. - backgroundColor: backgroundColorForTheme(theme), + backgroundColor: backgroundColorForTheme(themeToUse), justifyContent: 'center', alignItems: 'center', @@ -262,7 +273,7 @@ export function OfflineNoticeProvider(props: ProviderProps): Node { }, noticeText: { fontSize: 14 }, }), - [isNoticeVisible, theme], + [isNoticeVisible, themeToUse], ); /** @@ -382,6 +393,8 @@ type PlaceholderProps = $ReadOnly<{| */ export function OfflineNoticePlaceholder(props: PlaceholderProps): Node { const theme = useGlobalSelector(state => getGlobalSettings(state).theme); + const osScheme = useColorScheme(); + const themeToUse = getThemeToUse(theme, osScheme); const { style: callerStyle } = props; const { isNoticeVisible, noticeContentAreaHeight } = useContext(OfflineNoticeContext); @@ -451,7 +464,7 @@ export function OfflineNoticePlaceholder(props: PlaceholderProps): Node { isNoticeVisible && ( ) } diff --git a/src/boot/ThemeProvider.js b/src/boot/ThemeProvider.js index 3a186547549..4776c82232f 100644 --- a/src/boot/ThemeProvider.js +++ b/src/boot/ThemeProvider.js @@ -2,11 +2,13 @@ import React from 'react'; import type { Node } from 'react'; +import { useColorScheme } from 'react-native'; import { useGlobalSelector } from '../react-redux'; import { getGlobalSettings } from '../directSelectors'; import { themeData, ThemeContext } from '../styles/theme'; import ZulipStatusBar from '../common/ZulipStatusBar'; +import { getThemeToUse } from '../settings/settingsSelectors'; type Props = $ReadOnly<{| children: Node, @@ -15,8 +17,11 @@ type Props = $ReadOnly<{| export default function ThemeProvider(props: Props): Node { const { children } = props; const theme = useGlobalSelector(state => getGlobalSettings(state).theme); + const osScheme = useColorScheme(); + const themeToUse = getThemeToUse(theme, osScheme); + return ( - + {children} diff --git a/src/boot/ZulipSafeAreaProvider.js b/src/boot/ZulipSafeAreaProvider.js index 7b5acbd8325..80d2e113b09 100644 --- a/src/boot/ZulipSafeAreaProvider.js +++ b/src/boot/ZulipSafeAreaProvider.js @@ -1,11 +1,13 @@ // @flow strict-local import * as React from 'react'; import { SafeAreaProvider } from 'react-native-safe-area-context'; +import { useColorScheme } from 'react-native'; import { BRAND_COLOR } from '../styles'; import { useGlobalSelector } from '../react-redux'; import { getGlobalSettings, getIsHydrated } from '../directSelectors'; import { themeData } from '../styles/theme'; +import { getThemeToUse } from '../settings/settingsSelectors'; type Props = {| +children: React.Node, @@ -27,6 +29,8 @@ export default function ZulipSafeAreaProvider(props: Props): React.Node { // // We can make this quirk virtually invisible by giving it the background // color used across the app. + const osScheme = useColorScheme(); + const backgroundColor = useGlobalSelector(state => { if (!getIsHydrated(state)) { // The only screen we'll be showing at this point is the loading @@ -34,7 +38,9 @@ export default function ZulipSafeAreaProvider(props: Props): React.Node { return BRAND_COLOR; } - return themeData[getGlobalSettings(state).theme].backgroundColor; + const theme = getGlobalSettings(state).theme; + const themeToUse = getThemeToUse(theme, osScheme); + return themeData[themeToUse].backgroundColor; }); return {props.children}; diff --git a/src/common/Popup.js b/src/common/Popup.js index 1b9c838a1c4..8986bf03af7 100644 --- a/src/common/Popup.js +++ b/src/common/Popup.js @@ -1,11 +1,12 @@ /* @flow strict-local */ import React, { useContext } from 'react'; import type { Node } from 'react'; -import { View } from 'react-native'; +import { View, useColorScheme } from 'react-native'; import { ThemeContext, createStyleSheet } from '../styles'; import { useGlobalSelector } from '../react-redux'; import { getGlobalSettings } from '../directSelectors'; +import { getThemeToUse } from '../settings/settingsSelectors'; const styles = createStyleSheet({ popup: { @@ -45,8 +46,12 @@ type Props = $ReadOnly<{| */ export default function Popup(props: Props): Node { const themeContext = useContext(ThemeContext); + const theme = useGlobalSelector(state => getGlobalSettings(state).theme); + const osScheme = useColorScheme(); + const themeToUse = getThemeToUse(theme, osScheme); + // TODO(color/theme): find a cleaner way to express this - const isDarkTheme = useGlobalSelector(state => getGlobalSettings(state).theme !== 'default'); + const isDarkTheme = themeToUse !== 'light'; return ( {props.children} diff --git a/src/common/ZulipStatusBar.js b/src/common/ZulipStatusBar.js index bdbc92e6455..e5b3ac5a53b 100644 --- a/src/common/ZulipStatusBar.js +++ b/src/common/ZulipStatusBar.js @@ -2,7 +2,7 @@ import React from 'react'; import type { Node } from 'react'; -import { Platform, StatusBar } from 'react-native'; +import { Platform, StatusBar, useColorScheme } from 'react-native'; // $FlowFixMe[untyped-import] import Color from 'color'; @@ -10,11 +10,12 @@ import type { ThemeName } from '../types'; import { useGlobalSelector } from '../react-redux'; import { foregroundColorFromBackground } from '../utils/color'; import { getGlobalSession, getGlobalSettings } from '../selectors'; +import { getThemeToUse } from '../settings/settingsSelectors'; type BarStyle = React$ElementConfig['barStyle']; export const getStatusBarColor = (backgroundColor: string | void, theme: ThemeName): string => - backgroundColor ?? (theme === 'night' ? 'hsl(212, 28%, 18%)' : 'white'); + backgroundColor ?? (theme === 'dark' ? 'hsl(212, 28%, 18%)' : 'white'); export const getStatusBarStyle = (statusBarColor: string): BarStyle => foregroundColorFromBackground(statusBarColor) === 'white' /* force newline */ @@ -46,9 +47,13 @@ type Props = $ReadOnly<{| export default function ZulipStatusBar(props: Props): Node { const { hidden = false } = props; const theme = useGlobalSelector(state => getGlobalSettings(state).theme); + const osScheme = useColorScheme(); + const themeToUse = getThemeToUse(theme, osScheme); + const orientation = useGlobalSelector(state => getGlobalSession(state).orientation); const backgroundColor = props.backgroundColor; - const statusBarColor = getStatusBarColor(backgroundColor, theme); + const statusBarColor = getStatusBarColor(backgroundColor, themeToUse); + return ( orientation === 'PORTRAIT' && ( { test('returns specific color when given, regardless of theme', () => { - expect(getStatusBarColor('#fff', themeDefault)).toEqual('#fff'); - expect(getStatusBarColor('#fff', themeNight)).toEqual('#fff'); + expect(getStatusBarColor('#fff', themeLight)).toEqual('#fff'); + expect(getStatusBarColor('#fff', themeDark)).toEqual('#fff'); }); test('returns color according to theme for default case', () => { - expect(getStatusBarColor(undefined, themeDefault)).toEqual('white'); - expect(getStatusBarColor(undefined, themeNight)).toEqual('hsl(212, 28%, 18%)'); + expect(getStatusBarColor(undefined, themeLight)).toEqual('white'); + expect(getStatusBarColor(undefined, themeDark)).toEqual('hsl(212, 28%, 18%)'); }); }); diff --git a/src/nav/ZulipNavigationContainer.js b/src/nav/ZulipNavigationContainer.js index b3bb214723f..7c52b5e4ba5 100644 --- a/src/nav/ZulipNavigationContainer.js +++ b/src/nav/ZulipNavigationContainer.js @@ -1,6 +1,7 @@ /* @flow strict-local */ import React, { useContext, useEffect } from 'react'; import type { Node } from 'react'; +import { useColorScheme } from 'react-native'; import { NavigationContainer, DefaultTheme, DarkTheme } from '@react-navigation/native'; import { useGlobalSelector } from '../react-redux'; @@ -8,6 +9,7 @@ import { ThemeContext } from '../styles'; import * as NavigationService from './NavigationService'; import { getGlobalSettings } from '../selectors'; import AppNavigator from './AppNavigator'; +import { getThemeToUse } from '../settings/settingsSelectors'; type Props = $ReadOnly<{||}>; @@ -23,6 +25,8 @@ type Props = $ReadOnly<{||}>; */ export default function ZulipAppContainer(props: Props): Node { const themeName = useGlobalSelector(state => getGlobalSettings(state).theme); + const osScheme = useColorScheme(); + const themeToUse = getThemeToUse(themeName, osScheme); useEffect( () => @@ -36,11 +40,11 @@ export default function ZulipAppContainer(props: Props): Node { const themeContext = useContext(ThemeContext); - const BaseTheme = themeName === 'night' ? DarkTheme : DefaultTheme; + const BaseTheme = themeToUse === 'dark' ? DarkTheme : DefaultTheme; const theme = { ...BaseTheme, - dark: themeName === 'night', + dark: themeToUse === 'dark', colors: { ...BaseTheme.colors, primary: themeContext.color, diff --git a/src/reduxTypes.js b/src/reduxTypes.js index 9fcef823313..0166d20a4f4 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -348,10 +348,24 @@ export type RealmState = {| +serverEmojiData: ServerEmojiData | null, |}; -// TODO: Stop using the 'default' name. Any 'default' semantics should -// only apply the device level, not within the app. See -// https://github.com/zulip/zulip-mobile/issues/4009#issuecomment-619280681. -export type ThemeName = 'default' | 'night'; +/** + * The visual theme of the app. + * + * To get a ThemeName from the ThemeSetting, first check the current + * OS theme by calling the React Native hook useColorScheme and pass + * that to the helper function getThemeToUse. + */ +export type ThemeName = 'light' | 'dark'; + +/** + * The theme setting. + * + * This represents the value the user chooses in SettingsScreen. + * + * To determine the actual theme to show the user, use a ThemeName; + * see there for details. + */ +export type ThemeSetting = 'default' | 'night'; /** What browser the user has set to use for opening links in messages. * @@ -392,7 +406,7 @@ export type GlobalSettingsState = $ReadOnly<{ // The user's chosen language, as an IETF BCP 47 language tag. language: string, - theme: ThemeName, + theme: ThemeSetting, browser: BrowserPreference, // TODO cut this? what was it? diff --git a/src/settings/settingsSelectors.js b/src/settings/settingsSelectors.js new file mode 100644 index 00000000000..3b555f41566 --- /dev/null +++ b/src/settings/settingsSelectors.js @@ -0,0 +1,6 @@ +/* @flow strict-local */ +import type { ColorSchemeName } from 'react-native/Libraries/Utilities/NativeAppearance'; +import type { ThemeSetting, ThemeName } from '../reduxTypes'; + +export const getThemeToUse = (theme: ThemeSetting, osScheme: ?ColorSchemeName): ThemeName => + theme === 'default' ? 'light' : 'dark'; diff --git a/src/start/IosCompliantAppleAuthButton/Custom.js b/src/start/IosCompliantAppleAuthButton/Custom.js index 6043745fa26..a8412ed0ca6 100644 --- a/src/start/IosCompliantAppleAuthButton/Custom.js +++ b/src/start/IosCompliantAppleAuthButton/Custom.js @@ -23,10 +23,10 @@ const styles = createStyleSheet({ borderRadius: 22, overflow: 'hidden', }, - nightFrame: { + darkFrame: { backgroundColor: 'black', }, - dayFrame: { + lightFrame: { backgroundColor: 'white', borderWidth: 1.5, borderColor: 'black', @@ -34,10 +34,10 @@ const styles = createStyleSheet({ text: { fontSize: 16, }, - nightText: { + darkText: { color: 'white', }, - dayText: { + lightText: { color: 'black', }, }); @@ -57,13 +57,13 @@ type Props = $ReadOnly<{| */ export default function Custom(props: Props): Node { const { style, onPress, theme } = props; - const logoSource = theme === 'default' ? appleLogoBlackImg : appleLogoWhiteImg; + const logoSource = theme === 'light' ? appleLogoBlackImg : appleLogoWhiteImg; const frameStyle = [ styles.frame, - theme === 'default' ? styles.dayFrame : styles.nightFrame, + theme === 'light' ? styles.lightFrame : styles.darkFrame, style, ]; - const textStyle = [styles.text, theme === 'default' ? styles.dayText : styles.nightText]; + const textStyle = [styles.text, theme === 'light' ? styles.lightText : styles.darkText]; return ( diff --git a/src/start/IosCompliantAppleAuthButton/index.js b/src/start/IosCompliantAppleAuthButton/index.js index d7e483a5769..89a91cf7316 100644 --- a/src/start/IosCompliantAppleAuthButton/index.js +++ b/src/start/IosCompliantAppleAuthButton/index.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import React, { useState, useEffect } from 'react'; import type { Node } from 'react'; -import { View } from 'react-native'; +import { View, useColorScheme } from 'react-native'; import type { ViewStyle } from 'react-native/Libraries/StyleSheet/StyleSheet'; import * as AppleAuthentication from 'expo-apple-authentication'; import { useGlobalSelector } from '../../react-redux'; @@ -9,6 +9,7 @@ import { useGlobalSelector } from '../../react-redux'; import type { SubsetProperties } from '../../generics'; import Custom from './Custom'; import { getGlobalSettings } from '../../selectors'; +import { getThemeToUse } from '../../settings/settingsSelectors'; type Props = $ReadOnly<{| // See `ZulipButton`'s `style` prop, where a comment discusses this @@ -35,6 +36,9 @@ type Props = $ReadOnly<{| export default function IosCompliantAppleAuthButton(props: Props): Node { const { style, onPress } = props; const theme = useGlobalSelector(state => getGlobalSettings(state).theme); + const osScheme = useColorScheme(); + const themeToUse = getThemeToUse(theme, osScheme); + const [isNativeButtonAvailable, setIsNativeButtonAvailable] = useState(undefined); useEffect(() => { @@ -56,7 +60,7 @@ export default function IosCompliantAppleAuthButton(props: Props): Node { ); } else { - return ; + return ; } } diff --git a/src/storage/__tests__/migrations-test.js b/src/storage/__tests__/migrations-test.js index 1db613464d0..49a0dce1b2c 100644 --- a/src/storage/__tests__/migrations-test.js +++ b/src/storage/__tests__/migrations-test.js @@ -128,7 +128,9 @@ describe('migrations', () => { // whether any properties outside `storeKeys` are present or not. [ 'check dropCache at 55', - { ...endBase, migrations: { version: 54 }, mute: [], nonsense: [1, 2, 3] }, + // Just before the `dropCache`, plus a `cacheKeys` property, plus junk. + { ...base52, migrations: { version: 54 }, mute: [], nonsense: [1, 2, 3] }, + // Should wind up with the same result as without the extra properties. endBase, ], diff --git a/src/styles/theme.js b/src/styles/theme.js index 68c4e3c1e1a..879c963f7ab 100644 --- a/src/styles/theme.js +++ b/src/styles/theme.js @@ -5,14 +5,16 @@ import type { Context } from 'react'; import type { ThemeName } from '../reduxTypes'; export type ThemeData = {| + themeName: ThemeName, color: string, backgroundColor: string, cardColor: string, dividerColor: string, |}; -export const themeData: {| [name: ThemeName | 'light']: ThemeData |} = { - night: { +export const themeData: {| [name: ThemeName]: ThemeData |} = { + dark: { + themeName: 'dark', color: 'hsl(210, 11%, 85%)', backgroundColor: 'hsl(212, 28%, 18%)', cardColor: 'hsl(212, 31%, 21%)', @@ -21,6 +23,7 @@ export const themeData: {| [name: ThemeName | 'light']: ThemeData |} = { dividerColor: 'hsla(0, 0%, 100%, 0.12)', }, light: { + themeName: 'light', color: 'hsl(0, 0%, 20%)', backgroundColor: 'white', cardColor: 'hsl(0, 0%, 97%)', @@ -29,6 +32,5 @@ export const themeData: {| [name: ThemeName | 'light']: ThemeData |} = { dividerColor: 'hsla(0, 0%, 0%, 0.12)', }, }; -themeData.default = themeData.light; -export const ThemeContext: Context = React.createContext(themeData.default); +export const ThemeContext: Context = React.createContext(themeData.light); diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 1726f2d9cc7..70a9b8eedd5 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -16,6 +16,8 @@ import type { EditMessage, } from '../types'; import { assumeSecretlyGlobalState } from '../reduxTypes'; +import type { ThemeData } from '../styles'; +import { ThemeContext } from '../styles'; import { connect } from '../react-redux'; import { getCurrentTypingUsers, @@ -123,6 +125,9 @@ const webviewAssetsUrl = new URL('webview/', assetsUrl); const baseUrl = new URL('index.html', webviewAssetsUrl); class MessageListInner extends React.Component { + static contextType = ThemeContext; + context: ThemeData; + webviewRef = React.createRef>(); sendInboundEventsIsReady: boolean; unsentInboundEvents: WebViewInboundEvent[] = []; @@ -176,10 +181,10 @@ class MessageListInner extends React.Component { const contentHtml = messageListElementsForShownMessages .map(element => messageListElementHtml({ backgroundData, element, _ })) .join(''); - const { auth, theme } = backgroundData; + const { auth } = backgroundData; const html: string = getHtml( contentHtml, - theme, + this.context.themeName, { scrollMessageId: initialScrollMessageId, auth, diff --git a/src/webview/backgroundData.js b/src/webview/backgroundData.js index 6e4d47cdf92..02075c03f04 100644 --- a/src/webview/backgroundData.js +++ b/src/webview/backgroundData.js @@ -12,7 +12,6 @@ import type { PerAccountState, Subscription, Stream, - ThemeName, UserId, User, UserOrBot, @@ -63,7 +62,6 @@ export type BackgroundData = $ReadOnly<{| streams: Map, subscriptions: Map, unread: UnreadState, - theme: ThemeName, twentyFourHourTime: boolean, userSettingStreamNotification: boolean, displayEmojiReactionUsers: boolean, @@ -96,7 +94,6 @@ export const getBackgroundData = ( streams: getStreamsById(state), subscriptions: getSubscriptionsById(state), unread: getUnread(state), - theme: globalSettings.theme, twentyFourHourTime: getRealm(state).twentyFourHourTime, userSettingStreamNotification: getSettings(state).streamNotification, displayEmojiReactionUsers: getSettings(state).displayEmojiReactionUsers, diff --git a/src/webview/css/css.js b/src/webview/css/css.js index 6e770e1aa6e..35f199a6251 100644 --- a/src/webview/css/css.js +++ b/src/webview/css/css.js @@ -27,8 +27,8 @@ export default (theme: ThemeName, serverEmojiData: ServerEmojiData | null): stri