From 8a7457bbae0efc715cfdf8b2700dee49815a8a5c Mon Sep 17 00:00:00 2001 From: Francesco Bigiarini Date: Tue, 6 Oct 2026 17:25:18 +0200 Subject: [PATCH 1/3] Add a dark theme, chosen in the settings or following the system The window painted light only, and said so in its color-scheme declaration (#560). Now it paints both: a Theme control on the General tab offers Light, Dark and System, and System, the default, follows the operating system. Main gives the choice to Electron's nativeTheme, read from the store before the window is made and set again as the setting is changed; Chromium answers the page's prefers-color-scheme from it and paints the window's chrome and the native form controls to match. The window reads only that answer: a wrapper around the design system's provider seeds the dark ramp from the prototype's dark background when the scheme is dark, and passes no colour in the light scheme, so the light theme is pixel for pixel what it was. The terminal, which takes its colours as values, reads them again when the scheme changes. The older component library's dialogs and popovers, painted white in its own stylesheet, are painted with tokens. A dark window is made in the dark colour, and index.html paints the body the same colour under the dark scheme until the provider has mounted, so nothing white shows before the page has its tokens; color-scheme.test.cjs holds the declaration and that colour to the theme module. The screenshot fixtures pin the pictures to the light theme whatever the maintainer's machine is set to, with SHOTS_THEME=dark for a dark one, and the harness lets the page follow the app's theme rather than Playwright's light emulation; a journey can ask for the same with colorScheme: null, and the theme journey does. Co-Authored-By: Claude Fable 5.1 --- docs/guide/settings.md | 1 + scripts/screenshots/capture.cjs | 5 ++ scripts/screenshots/fixtures.cjs | 6 +- src/main.js | 28 +++++++- src/renderer/components/app-theme.jsx | 47 +++++++++++++ src/renderer/components/settings-dialog.jsx | 38 ++++++++++- src/renderer/hooks/use-site-terminal.jsx | 13 ++++ src/renderer/index.html | 30 +++++---- src/renderer/index.jsx | 12 ++-- src/renderer/settings-view.cjs | 16 ++++- src/renderer/shell.css | 30 +++++++++ src/renderer/terminal-theme.cjs | 17 +++-- src/settings.cjs | 13 ++++ src/theme.cjs | 53 +++++++++++++++ tests/e2e/helpers/app.cjs | 25 ++++--- tests/e2e/journeys/settings.spec.js | 69 +++++++++++++++++++ tests/unit/color-scheme.test.cjs | 46 ++++++++----- tests/unit/ipc-wiring.test.cjs | 75 +++++++++++++++++++-- tests/unit/settings-view.test.cjs | 10 ++- tests/unit/settings.test.cjs | 19 ++++-- tests/unit/theme.test.cjs | 27 ++++++++ 21 files changed, 513 insertions(+), 67 deletions(-) create mode 100644 src/renderer/components/app-theme.jsx create mode 100644 src/theme.cjs create mode 100644 tests/unit/theme.test.cjs diff --git a/docs/guide/settings.md b/docs/guide/settings.md index 12af7ce9..e4031670 100644 --- a/docs/guide/settings.md +++ b/docs/guide/settings.md @@ -4,6 +4,7 @@ The app's settings are in one dialog, opened from the cog at the bottom right of ## General +- **Theme** — light, dark, or whatever your operating system is set to, which is the default. A change applies as you make it, to the whole window; nothing running is touched. - **Language** — which language the app is shown in: your system's language, which is the default, or one of the languages the app has a translation for. A change applies after a relaunch, which the dialog offers; relaunching stops running servers and builds, as quitting does. Translations come from [translate.wordpress.org](https://translate.wordpress.org/projects/meta/contributor-toolkit/) and ship with the app once they are mostly complete, so the list grows from release to release. - **Start the server when I open a site** and **Start the build watch when I open a site** — off unless you turn them on. On, opening a site that is set up starts its [development server](./running-the-site), its build watch, or both, as it would if you pressed Start; on WordPress Core the server's start brings the watch with it. Nothing starts for a site whose setup, update or deletion is under way, and nothing already running is started again. - **When I quit, running servers and build watches** — quitting always stops them, as it always has. **Stop them, and start them again next time** remembers which sites had a server or a watch running and starts them again when the app next opens, whichever site it opens on. diff --git a/scripts/screenshots/capture.cjs b/scripts/screenshots/capture.cjs index c939b251..9be48238 100644 --- a/scripts/screenshots/capture.cjs +++ b/scripts/screenshots/capture.cjs @@ -91,6 +91,11 @@ async function launchApp(env) { // From plain Node, require('electron') resolves to the binary's path — // the same trick scripts/run-tests-electron.cjs uses. executablePath: require('electron'), + // Playwright holds a page to the light scheme unless told not to. The + // pictures are of the theme the fixture's profile sets (#560), light + // unless SHOTS_THEME says dark, which the app reads from its own + // setting; so the page is left to follow the app. + colorScheme: null, args: [...ELECTRON_SWITCHES, repoRoot], // Dates rendered by the app must not rewrite screenshots according to the // maintainer's locale or timezone. diff --git a/scripts/screenshots/fixtures.cjs b/scripts/screenshots/fixtures.cjs index b236e707..6383967d 100644 --- a/scripts/screenshots/fixtures.cjs +++ b/scripts/screenshots/fixtures.cjs @@ -338,10 +338,14 @@ function buildFixture(variant) { return { userDataDir, sites: { wizardSite, readySite, staleSite, incompleteSite } }; } +// The pictures are of the light theme whatever the maintainer's machine is +// set to (#560), unless a dark one is asked for: `SHOTS_THEME=dark`. +const SHOTS_THEME = process.env.SHOTS_THEME || 'light'; + function writeSettings(userDataDir, settings) { fs.writeFileSync( path.join(userDataDir, 'settings.json'), - JSON.stringify(settings, null, '\t') + JSON.stringify({ ...settings, preferences: { theme: SHOTS_THEME, ...(settings.preferences || {}) } }, null, '\t') ); } diff --git a/src/main.js b/src/main.js index d6c2da65..f37259ec 100644 --- a/src/main.js +++ b/src/main.js @@ -1,4 +1,4 @@ -const { app, BrowserWindow, Menu, ipcMain, dialog, shell, screen } = require('electron'); +const { app, BrowserWindow, Menu, ipcMain, dialog, shell, screen, nativeTheme } = require('electron'); const path = require('path'); const os = require('os'); const crypto = require('crypto'); @@ -93,6 +93,7 @@ const { addFilter } = require('@wordpress/hooks'); const { mergeInProgressError, mergeCheckFailedError } = require('./renderer/merge-in-progress.cjs'); const { parseHandle } = require('./wporg-handle.cjs'); const { SETTINGS, readSettings, acceptSetting } = require('./settings.cjs'); +const { windowBackground } = require('./theme.cjs'); const { parseEventName, buildProvenanceHeader, handoffFilename } = require('./patch-provenance.cjs'); const { describeRefused } = require('./safe-log'); const { detectEditors, matchDetectedEditor, openSiteInEditor, REFUSAL_REASONS } = require('./editor-launch'); @@ -561,6 +562,9 @@ function createWindow() { mainWindow = new BrowserWindow({ ...mainWindowSize(screen.getPrimaryDisplay().workAreaSize), icon: process.platform === 'linux' ? path.join(__dirname, '..', 'build', 'icon.png') : undefined, + // The colour of the theme the window is made in (#560), so that a dark + // window is not white for the moment before its page has painted. + backgroundColor: windowBackground(nativeTheme.shouldUseDarkColors), webPreferences: { preload: path.join(__dirname, 'preload.js'), contextIsolation: true, @@ -710,6 +714,21 @@ function localeReply() { return localeReplyPromise; } +// The theme (#560), given to Electron. `nativeTheme` is the one place the +// choice is made: Chromium answers the page's `prefers-color-scheme` from it, +// and paints the window's chrome and the native form controls to match, so +// the page only has to follow what it is told, as it would the operating +// system's. Read from the store before the window is made, so the window is +// made in it; a store that cannot be read leaves the system's theme, with a +// line in the log, as it leaves the system's language. +async function applyStoredTheme() { + try { + nativeTheme.themeSource = readSettings((await getStore()).get('preferences')).theme; + } catch (e) { + logError('theme', `the settings could not be read, so the theme is the system's: ${String(e && e.message ? e.message : e)}`); + } +} + // The languages the settings offer: what the build ships, read once. let languagesPromise = null; function languages() { @@ -2652,6 +2671,8 @@ app.whenReady().then(async () => { // renderer output into the log file, which only applies to windows created // afterwards. initLogging(); + // Before the window: it is made in the theme. + await applyStoredTheme(); // Before the menu and the window: both build their labels from `__()`. applyLocale(await localeReply(), { setLocaleData, addFilter }); Menu.setApplicationMenu(Menu.buildFromTemplate(buildMenuTemplate({ @@ -3570,7 +3591,10 @@ ipcMain.handle('settings:set', async (_e, key, value) => { if (!accepted.ok) return { ok: false, error: accepted.error }; await setPreference(key, accepted.value); const s = await getStore(); - return { ok: true, settings: readSettings(s.get('preferences')) }; + const settings = readSettings(s.get('preferences')); + // The theme applies at once (#560): Electron tells the window. + if (key === 'theme') nativeTheme.themeSource = settings.theme; + return { ok: true, settings }; }); // The fallback that needs no configuration at all — see site-registry.js for why diff --git a/src/renderer/components/app-theme.jsx b/src/renderer/components/app-theme.jsx new file mode 100644 index 00000000..33d233cc --- /dev/null +++ b/src/renderer/components/app-theme.jsx @@ -0,0 +1,47 @@ +import { createContext, useContext, useEffect, useState } from 'react'; +import { ThemeProvider } from '@wordpress/theme'; +import { themeColorSeeds } from '../../theme.cjs'; + +// What Chromium says of the window's colour scheme. It says it from the theme +// main gave Electron (#560), or from the operating system under 'system', so +// this is the one thing the window reads: not the setting, which would be a +// second answer to the same question. +const DARK_SCHEME = '(prefers-color-scheme: dark)'; + +// Whether the window is in the dark scheme, for whatever paints with values +// rather than with a stylesheet (the terminal) and so has to be told when it +// changes. +const DarkSchemeContext = createContext(false); + +function usePrefersDark() { + const [dark, setDark] = useState(() => window.matchMedia(DARK_SCHEME).matches); + useEffect(() => { + const media = window.matchMedia(DARK_SCHEME); + const onChange = (event) => setDark(event.matches); + media.addEventListener('change', onChange); + // A change between the first read and the listener is not missed. + setDark(media.matches); + return () => media.removeEventListener('change', onChange); + }, []); + return dark; +} + +// The design system's provider, at the root of the window. `isRoot` puts what +// it overrides on the document rather than on its own wrapper, which is what +// reaches a modal or a popover: those are portalled to `body`, outside this +// tree. In the light scheme it is given no colour, so the tokens stylesheet's +// values stand as they ship; in the dark scheme it is given the dark seed and +// builds every colour token from it, for its own components, for the older +// ones through the variables they read, and for the app's own styles. +export function AppTheme({ children }) { + const dark = usePrefersDark(); + return ( + + {children} + + ); +} + +export function useDarkScheme() { + return useContext(DarkSchemeContext); +} diff --git a/src/renderer/components/settings-dialog.jsx b/src/renderer/components/settings-dialog.jsx index 4f21117a..6767d3ed 100644 --- a/src/renderer/components/settings-dialog.jsx +++ b/src/renderer/components/settings-dialog.jsx @@ -6,7 +6,7 @@ import { useEffect, useId, useMemo, useState } from 'react'; import { __experimentalToggleGroupControl as ToggleGroupControl, __experimentalToggleGroupControlOption as ToggleGroupControlOption } from '@wordpress/components'; import { __ } from '@wordpress/i18n'; import { Button, Dialog, InputControl, Notice, SelectControl, Stack, SwitchControl, Tabs, Text } from '@wordpress/ui'; -import { githubAccountLine, newSiteLocationNote, languageItems, languageValue, languageChanged, phpVersionChoice, quitItems, SYSTEM_LANGUAGE } from '../settings-view.cjs'; +import { githubAccountLine, newSiteLocationNote, languageItems, languageValue, languageChanged, phpVersionChoice, quitItems, themeItems, SYSTEM_LANGUAGE } from '../settings-view.cjs'; import { FolderField } from './folder-field.jsx'; // A notice here is read by its role, and is not also spoken: the dialog it @@ -81,6 +81,41 @@ function LanguageControl({ settings, loaded, onChange }) { ); } +// The window's theme (#560): light, dark, or the operating system's. Main +// gives the choice to Electron, and the window follows what Chromium then +// says of the colour scheme, so the change is on screen as the control is +// pressed. +function ThemeControl({ settings, onChange }) { + const [error, setError] = useState(''); + const keep = async (value) => { + const result = await onChange('theme', value); + setError(result?.ok ? '' : (result?.error || __('Could not keep that.'))); + }; + return ( + <> + { if (value) keep(value); }} + > + {themeItems().map((item) => ( + + ))} + + {error ? ( + + {error} + + ) : null} + + ); +} + // What a site opened starts, and what the quit does with what is running. // The quit stops servers and watches either way: the one choice is whether // the next launch starts them again. @@ -156,6 +191,7 @@ function GeneralTab({ settings, loaded, onChange }) { }>{__('Appearance')} + diff --git a/src/renderer/hooks/use-site-terminal.jsx b/src/renderer/hooks/use-site-terminal.jsx index c9a86f4a..3e6c2be3 100644 --- a/src/renderer/hooks/use-site-terminal.jsx +++ b/src/renderer/hooks/use-site-terminal.jsx @@ -2,6 +2,7 @@ import { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react import { Terminal } from '@xterm/xterm'; import { terminalFont, terminalTheme, tokenName, TERMINAL_READABILITY } from '../terminal-theme.cjs'; import { terminalGrid } from '../tray.cjs'; +import { useDarkScheme } from '../components/app-theme.jsx'; // What the terminal is painted with, read off the design system's tokens // where the terminal stands (#557). The terminal takes its colours and its @@ -58,6 +59,7 @@ const TERMINAL_INSTALL_ALIASES = ['npm install', 'npm i', 'install']; // None of them depends on the three runners, which may change as often as // they like. export function useSiteTerminal({ allowedScripts, runInstall, runScript, killCurrent, shown }) { + const dark = useDarkScheme(); // Read through a ref by the terminal's command handlers rather than closed // over: the xterm instance is created by an effect that depends on // `printHelp`, so a new array identity here would otherwise dispose and @@ -382,6 +384,17 @@ export function useSiteTerminal({ allowedScripts, runInstall, runScript, killCur fitTerminal(); }); + // Painted again when the window's scheme changes (#560). The terminal was + // given its colours as values when it opened, and a change to the tokens + // does not reach a value; so they are read again, after the provider has + // put the new tokens on the document, which it does in a layout effect, + // before this one runs. A terminal not yet opened is given them when it is. + useEffect(() => { + const term = terminalRef.current; + if (!term || !term.element || !container) return; + term.options.theme = readTerminalLook(container).theme; + }, [dark, container]); + // Fitted again whenever its element changes size: the tray dragged, the // window resized, and the element coming back on screen, which is a change // from no size to one. diff --git a/src/renderer/index.html b/src/renderer/index.html index badd3200..c88495c0 100644 --- a/src/renderer/index.html +++ b/src/renderer/index.html @@ -16,19 +16,25 @@ + + - + diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index efc33e42..3cc0da93 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -9,7 +9,6 @@ import { Page } from '@wordpress/admin-ui'; import { __, _x, setLocaleData } from '@wordpress/i18n'; import { addFilter } from '@wordpress/hooks'; import { drawerLeft, globe } from '@wordpress/icons'; -import { ThemeProvider } from '@wordpress/theme'; import { Badge, Button as UiButton, Card as UiCard, EmptyState, IconButton, Notice, Spinner as UiSpinner, Stack, Text, VisuallyHidden } from '@wordpress/ui'; // The design system's tokens: every `--wpds-*` custom property, at its default, // on `:root`. @@ -80,6 +79,7 @@ import { ApplyCard, ApplyPreviewDialog, PrCheckoutNotice } from './components/ap import { applyHeldReason, previewShown } from './apply-card.cjs'; import { TicketCard } from './components/ticket-card.jsx'; import { TicketListCard } from './components/ticket-list.jsx'; +import { AppTheme } from './components/app-theme.jsx'; import { useDetectedEditors } from './hooks/use-detected-editors.jsx'; import { useContributorProvenance } from './hooks/use-contributor-provenance.jsx'; import { useSettings } from './hooks/use-settings.jsx'; @@ -2406,13 +2406,9 @@ async function loadLocale() { document.title = __('WordPress Contributor Toolkit'); } -// The design system's provider, at its defaults: the tokens stylesheet already -// holds every value, so this changes nothing on screen. It is the one place -// to set colour and corner radius from, for whatever comes to set them. `isRoot` -// puts whatever it overrides on the document rather than on its own wrapper, -// which is what reaches a modal or a popover: those are portalled to `body`, -// outside this tree. +// Under the design system's provider, in the theme the window is in (#560): +// see app-theme.jsx. loadLocale().then(() => { const root = createRoot(document.getElementById('root')); - root.render(); + root.render(); }); diff --git a/src/renderer/settings-view.cjs b/src/renderer/settings-view.cjs index fc6aa021..5ab38796 100644 --- a/src/renderer/settings-view.cjs +++ b/src/renderer/settings-view.cjs @@ -123,6 +123,20 @@ function quitItems() { ]; } +/** + * The entries of the theme control (#560): light, dark, or the operating + * system's. The prototype's fourth, a custom pair of colours, is not offered. + * + * @return {Array<{value: string, label: string}>} + */ +function themeItems() { + return [ + { value: 'light', label: __('Light') }, + { value: 'dark', label: __('Dark') }, + { value: 'system', label: __('System') } + ]; +} + /** * What the next launch starts for a site, from the list the last quit left: * its server, its watch, both, or nothing. @@ -138,4 +152,4 @@ function resumeFor(resume, sitePath) { return server || watch ? { server, watch } : null; } -module.exports = { githubAccountLine, newSiteLocationNote, languageItems, languageValue, languageChanged, phpVersionChoice, quitItems, resumeFor, SYSTEM_LANGUAGE }; +module.exports = { githubAccountLine, newSiteLocationNote, languageItems, languageValue, languageChanged, phpVersionChoice, quitItems, themeItems, resumeFor, SYSTEM_LANGUAGE }; diff --git a/src/renderer/shell.css b/src/renderer/shell.css index 5e458ef7..c5acb48b 100644 --- a/src/renderer/shell.css +++ b/src/renderer/shell.css @@ -686,6 +686,36 @@ body { font-size: inherit; } +/* The older library's dialogs and popovers are painted white with dark text + in its own stylesheet, with no token behind the colour, so in the dark + theme (#560) they would be the one light thing on the page. Painted here + with the tokens the design system's own dialogs use, so they follow the + theme as everything else does. Their shadows stay as the library draws + them: a shadow is black under either theme, as the design system's own + are. The ring the popover draws in the same property is restated as the + hairline, since it was a light grey of its own. */ +.components-modal__frame, +.components-popover__content { + background: var(--wpds-color-background-surface-neutral-strong); + color: var(--wpds-color-foreground-content-neutral); +} + +.components-modal__frame h1, +.components-modal__frame h2, +.components-modal__frame h3 { + color: var(--wpds-color-foreground-content-neutral); +} + +.components-popover__content { + box-shadow: + 0 0 0 var(--wpds-border-width-xs) var(--wpds-color-stroke-surface-neutral), + 0 4px 12px color-mix(in srgb, var(--wpds-color-foreground-content-neutral) 10%, transparent); +} + +.components-dropdown__content .components-menu-group + .components-menu-group { + border-top-color: var(--wpds-color-stroke-surface-neutral); +} + /* What a first-timer cannot know about pull requests, behind its question. */ .destination-how { color: var(--wpds-color-foreground-content-neutral-weak); diff --git a/src/renderer/terminal-theme.cjs b/src/renderer/terminal-theme.cjs index 04b09993..6f4aee43 100644 --- a/src/renderer/terminal-theme.cjs +++ b/src/renderer/terminal-theme.cjs @@ -1,19 +1,22 @@ // What the site's terminal is painted with (#557). The terminal draws itself // and is told its colours and its font as values, not as CSS, so they are -// read off the design system's tokens when it is made: which token each of -// its colours is, is decided here. They are read once: a terminal made under -// one theme keeps it. +// read off the design system's tokens when it is made, and again when the +// window's theme changes (#560): which token each of its colours is, is +// decided here. // -// It is a light surface with dark text, like the log panes beside it. The +// It is the surface the log panes beside it have, with the text's own colour +// on it: light with dark text in the light theme, dark with light text in +// the dark one, since every token below follows the theme. The // colours a command can ask for by number are the design system's nearest: // red is what an error is said in, green a success, yellow a warning. A // command's "black" and its bright "white" are the text's own colour, and -// its "white" and bright "black" the quieter text's, since on this surface -// white could not be read. +// its "white" and bright "black" the quieter text's, since on the light +// surface white could not be read. // // The eight bright colours are the eight plain ones. The design system's // stronger colours are for text on a tinted notice, and on this surface are -// all but black: an error asked for in bright red would lose its red. +// all but black in the light theme: an error asked for in bright red would +// lose its red. 'use strict'; const NEUTRAL = 'var(--wpds-color-foreground-content-neutral)'; diff --git a/src/settings.cjs b/src/settings.cjs index e4ea16db..d42d21f7 100644 --- a/src/settings.cjs +++ b/src/settings.cjs @@ -18,6 +18,7 @@ */ const { __ } = require('@wordpress/i18n'); +const { THEMES } = require('./theme.cjs'); // What the quit setting can be. const QUIT_BEHAVIOURS = ['stop', 'restart']; @@ -79,6 +80,17 @@ const SETTINGS = { return { ok: true, value }; } }, + // The window's theme (#560): light, dark, or the operating system's, + // which is the fallback. Main applies it to Electron's native theme, and + // the window follows what Chromium then says of the colour scheme. + theme: { + fallback: 'system', + accept(value) { + if (value === null || value === undefined || value === '') return { ok: true, value: null }; + if (!THEMES.includes(value)) return { ok: false, error: __('Choose light, dark, or your system’s theme.') }; + return { ok: true, value }; + } + }, // The folder new sites are made in, each in a subfolder of its own. Unset, // the create-site dialog asks for one every time, as it did before. The // path is kept as the system's dialog gave it: a folder's name can end in @@ -120,6 +132,7 @@ function readSettings(preferences = {}) { autoStartServer: flag('autoStartServer'), autoStartWatch: flag('autoStartWatch'), quitBehavior: QUIT_BEHAVIOURS.includes(stored.quitBehavior) ? stored.quitBehavior : SETTINGS.quitBehavior.fallback, + theme: THEMES.includes(stored.theme) ? stored.theme : SETTINGS.theme.fallback, newSiteLocation: text('newSiteLocation') }; } diff --git a/src/theme.cjs b/src/theme.cjs new file mode 100644 index 00000000..e1ffaa19 --- /dev/null +++ b/src/theme.cjs @@ -0,0 +1,53 @@ +'use strict'; + +/** + * The window's theme (#560): light, dark, or whatever the operating system + * is set to, and what each side of the app does with the answer. + * + * Main gives the choice to Electron's `nativeTheme`, which is the one place + * the choice has to be made: Chromium then answers `prefers-color-scheme` in + * the window from it, and paints the window's own chrome and the native form + * controls to match. The window only reads that answer, and seeds the design + * system's colours from it. So there is one source of truth, and a change + * reaches the window as the operating system's would. + * + * Pure, and shared by main and the window: the dark seed is both the colour + * the design system builds its dark ramp from and the colour a window is + * made with, so that nothing white shows before the page has painted. + */ + +// What the theme setting can be. 'system' follows the operating system. +const THEMES = ['light', 'dark', 'system']; + +// The design system's own seeds, as the prototype has them: the colour the +// dark theme's surfaces are built from, and the colour the light theme's +// are. In the light theme the window passes no seed at all, since the tokens +// stylesheet already holds the light values and generating them again would +// move them by a hair; the light seed is the window's background only. +const DARK_BACKGROUND = '#1e1e1e'; +const LIGHT_BACKGROUND = '#fcfcfc'; + +/** + * What the design system's provider is given for the scheme the window is + * in: a seed to build the dark ramp from, or nothing, which is the light + * theme as the tokens stylesheet ships it. + * + * @param {boolean} dark Whether the window is in the dark scheme. + * @return {Object} The provider's `color` prop. + */ +function themeColorSeeds(dark) { + return dark ? { background: DARK_BACKGROUND } : {}; +} + +/** + * The colour a window is made with, so that the frame is not white for the + * moment before the page paints a dark one. + * + * @param {boolean} dark Whether the window is in the dark scheme. + * @return {string} `#rrggbb`. + */ +function windowBackground(dark) { + return dark ? DARK_BACKGROUND : LIGHT_BACKGROUND; +} + +module.exports = { THEMES, themeColorSeeds, windowBackground, DARK_BACKGROUND, LIGHT_BACKGROUND }; diff --git a/tests/e2e/helpers/app.cjs b/tests/e2e/helpers/app.cjs index 2f3579f6..813cf9bd 100644 --- a/tests/e2e/helpers/app.cjs +++ b/tests/e2e/helpers/app.cjs @@ -195,19 +195,27 @@ class Session { /** * Seeds settings.json and launches the app. * - * @param {Object} settings Initial electron-store contents. Defaults to a - * first-launch app with no sites. + * @param {Object} settings Initial electron-store contents. Defaults to a + * first-launch app with no sites. * @param {Object} [options] - * @param {string|false} [options.lang] The locale to launch in, in place of en-US, - * or `false` for no `--lang` at all: the - * app then picks its language as it does - * for a contributor, from the settings - * and the OS. It holds across restart(). + * @param {string|false} [options.lang] The locale to launch in, in place of en-US, + * or `false` for no `--lang` at all: the + * app then picks its language as it does + * for a contributor, from the settings + * and the OS. It holds across restart(). + * @param {?string} [options.colorScheme] Left out, Playwright holds the page + * to the light scheme whatever the machine + * and the theme setting say, so a journey + * is the same on every machine. `null` + * lets the page follow the app's own theme + * (#560), for a journey about it. It holds + * across restart(). * @return {Promise<{app: Object, page: Object}>} The Electron app and its first window. */ - async start( settings = EMPTY_SETTINGS, { lang } = {} ) { + async start( settings = EMPTY_SETTINGS, { lang, colorScheme } = {} ) { if ( this.app ) throw new Error( 'This session already has an app running; call restart() instead.' ); this.lang = lang; + this.colorScheme = colorScheme; this.writeSettings( settings ); return this.#launch(); } @@ -252,6 +260,7 @@ class Session { // does not recognise without a word — so a `slowMo` added here would leave the // tests passing at full speed and look like it had worked. ...( VIDEO_DIR ? { recordVideo: { dir: VIDEO_DIR } } : {} ), + ...( this.colorScheme === undefined ? {} : { colorScheme: this.colorScheme } ), env: { ...process.env, TZ: 'UTC', diff --git a/tests/e2e/journeys/settings.spec.js b/tests/e2e/journeys/settings.spec.js index c7775e4f..75c0de8e 100644 --- a/tests/e2e/journeys/settings.spec.js +++ b/tests/e2e/journeys/settings.spec.js @@ -249,3 +249,72 @@ test( 'the Sites tab keeps the PHP version and the debug flags the next server s await expect( kept.getByRole( 'switch', { name: 'Report notices and deprecations (WP_DEBUG)', exact: true } ) ).not.toBeChecked(); await expect( kept.getByRole( 'switch', { name: 'Use unminified scripts (SCRIPT_DEBUG)', exact: true } ) ).toBeChecked(); } ); + +test( 'the theme set in the settings is the one the window is painted in, and a change is on screen as it is made (#560)', async ( { session } ) => { + const site = await makeSite( session ); + // The page follows the app's own theme here, and not the light scheme + // Playwright holds every other journey to. + const { app, page } = await session.start( { ...site.settings, preferences: { theme: 'dark' } }, { colorScheme: null } ); + const themeSource = ( electronApp ) => electronApp.evaluate( ( { nativeTheme } ) => nativeTheme.themeSource ); + const prefersDark = ( window ) => window.evaluate( () => window.matchMedia( '(prefers-color-scheme: dark)' ).matches ); + const bodyColour = () => page.evaluate( () => window.getComputedStyle( document.body ).backgroundColor ); + const tokenColour = ( token ) => ui.tokenColour( page, token ); + + // INVARIANT — stored dark, the window starts dark: Electron is told, the + // page is in the dark scheme, and once the app has mounted the body is + // painted with the token as the dark theme has it, which the light + // theme's value below is not. Before the mount the body is the dark seed + // and the tokens still the stylesheet's, which is the moment the window + // is made dark for; so the body is read once it is the token's colour. + expect( await themeSource( app ) ).toBe( 'dark' ); + await expect.poll( () => prefersDark( page ) ).toBe( true ); + await expect( ui.renderedApp( page ) ).toBeVisible(); + await expect.poll( async () => ( await bodyColour() ) === ( await tokenColour( 'var(--wpds-color-background-surface-neutral)' ) ) ).toBe( true ); + const darkBody = await bodyColour(); + + // INVARIANT — the terminal, which is told its colours as values, is + // painted with the dark tokens too. + await ui.openTray( page, 'Terminal' ); + // By its class and not under the tray's role: the settings dialog, once + // open, makes the rest of the page inert, and a role under it is not + // found. One site, so one terminal. + const viewport = page.locator( '.xterm-viewport' ); + const terminalSurface = () => viewport.evaluate( ( el ) => window.getComputedStyle( el ).backgroundColor ); + await expect.poll( terminalSurface ).toBe( await tokenColour( 'var(--wpds-color-background-surface-neutral-weak)' ) ); + const darkTerminal = await terminalSurface(); + + // INVARIANT — the control shows Dark. Light chosen is kept, Electron is + // told, and the page, the body and the terminal are light at once, with + // no relaunch. + await ui.settingsButton( page ).click(); + const dialog = ui.settingsDialog( page ); + const themes = dialog.getByRole( 'radiogroup', { name: 'Theme', exact: true } ); + await expect( themes.getByRole( 'radio', { name: 'Dark', exact: true } ) ).toBeChecked(); + await themes.getByRole( 'radio', { name: 'Light', exact: true } ).click(); + await expect( themes.getByRole( 'radio', { name: 'Light', exact: true } ) ).toBeChecked(); + await expect.poll( () => session.readSettings().preferences?.theme ).toBe( 'light' ); + await expect.poll( () => themeSource( app ) ).toBe( 'light' ); + await expect.poll( () => prefersDark( page ) ).toBe( false ); + const lightBody = await bodyColour(); + expect( lightBody ).toBe( await tokenColour( 'var(--wpds-color-background-surface-neutral)' ) ); + expect( lightBody ).not.toBe( darkBody ); + await expect.poll( terminalSurface ).toBe( await tokenColour( 'var(--wpds-color-background-surface-neutral-weak)' ) ); + expect( await terminalSurface() ).not.toBe( darkTerminal ); + + // INVARIANT — System is kept as the system's, and Electron is left to + // follow it. + await themes.getByRole( 'radio', { name: 'System', exact: true } ).click(); + await expect.poll( () => session.readSettings().preferences?.theme ).toBe( 'system' ); + await expect.poll( () => themeSource( app ) ).toBe( 'system' ); + + // INVARIANT — started again with dark kept, the window is made dark, so + // it is not white before its page paints, and the control says so. + await themes.getByRole( 'radio', { name: 'Dark', exact: true } ).click(); + await expect.poll( () => session.readSettings().preferences?.theme ).toBe( 'dark' ); + const again = await session.restart(); + expect( await themeSource( again.app ) ).toBe( 'dark' ); + expect( ( await again.app.evaluate( ( { BrowserWindow } ) => BrowserWindow.getAllWindows()[ 0 ].getBackgroundColor() ) ).toLowerCase() ).toBe( '#1e1e1e' ); + await expect.poll( () => prefersDark( again.page ) ).toBe( true ); + await ui.settingsButton( again.page ).click(); + await expect( ui.settingsDialog( again.page ).getByRole( 'radio', { name: 'Dark', exact: true } ) ).toBeChecked(); +} ); diff --git a/tests/unit/color-scheme.test.cjs b/tests/unit/color-scheme.test.cjs index adb3029a..2b3b1592 100644 --- a/tests/unit/color-scheme.test.cjs +++ b/tests/unit/color-scheme.test.cjs @@ -1,39 +1,55 @@ 'use strict'; // The window declares which colour schemes it supports, and the browser paints -// the parts it owns — native form controls — to match. Declaring `light dark` -// on a machine in dark mode gave charcoal inputs on hand-painted white cards, -// with placeholder text at dark-on-dark contrast. That is not cosmetic here: -// this app puts guidance in placeholders ("Ticket number or URL, e.g. 62281", -// "WordPress.org username, e.g. janedoe"), so the unreadable text is the text -// a first-timer most needs. +// the parts it owns — native form controls — to match. Until #560 the window +// painted light only, and declared `light` only: declaring `light dark` on a +// machine in dark mode gave charcoal inputs on hand-painted white cards, with +// placeholder text at dark-on-dark contrast. Now that the page paints both +// schemes, the declaration has to say both: with `light` alone, the controls +// would be painted light inside a dark window, and the placeholders this app +// puts its guidance in ("Ticket number or URL, e.g. 62281") would be the +// unreadable text again, the other way round. // // A string assertion looks trivial, and the regression it guards is not: the // declaration is one word in a file nobody opens, the app looks perfect to // anyone whose OS is in light mode, and CI machines are in light mode too. The // only thing that would catch it is a reviewer on a dark laptop, which is how -// it was found. +// the first form of it was found. const test = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); +const { DARK_BACKGROUND } = require('../../src/theme.cjs'); + const INDEX_HTML = path.join(__dirname, '..', '..', 'src', 'renderer', 'index.html'); -test('the window declares only the colour scheme it actually implements', () => { +test('the window declares both colour schemes, since it paints both (#560)', () => { const html = fs.readFileSync(INDEX_HTML, 'utf8'); const declaration = / { + const html = fs.readFileSync(INDEX_HTML, 'utf8'); + const prePaint = /@media\s*\(prefers-color-scheme:\s*dark\)\s*\{\s*html:not\(\[data-wpds-root-provider\]\)\s+body\s*\{\s*background:\s*(#[0-9a-f]{6})\s*;?\s*\}\s*\}/i.exec(html); + + assert.ok(prePaint, 'index.html no longer paints the body for the dark scheme before the provider mounts, on `html:not([data-wpds-root-provider]) body`'); + assert.equal(prePaint[1].toLowerCase(), DARK_BACKGROUND.toLowerCase(), 'the colour painted before the app mounts is not the dark seed the app builds its theme from'); }); diff --git a/tests/unit/ipc-wiring.test.cjs b/tests/unit/ipc-wiring.test.cjs index 6ed65621..17555862 100644 --- a/tests/unit/ipc-wiring.test.cjs +++ b/tests/unit/ipc-wiring.test.cjs @@ -173,6 +173,13 @@ function createElectronStub({ ready = false } = {}) { isReady: () => true }, BrowserWindow: BrowserWindowStub, + // The theme (#560): what main sets, and what Electron would then say of + // it. Under 'system' this machine is taken to be light. + nativeTheme: { + themeSource: 'system', + get shouldUseDarkColors() { return this.themeSource === 'dark'; }, + on() {} + }, Menu: { buildFromTemplate: (template) => ({ template }), setApplicationMenu: (menu) => { calls.applicationMenu.push(menu); } @@ -348,7 +355,9 @@ function loadMain({ stubs = {}, ready = false } = {}) { // logging is stubbed everywhere: electron-log resolves its file path through // `app.getPath`, which the electron stub only pretends to have, and a test has // no business writing to the contributor's log file either way. -function silentLogging() { +// `overrides` replaces any of the functions, for a test that reads what was +// logged. +function silentLogging(overrides = {}) { return { './logging': { initLogging: () => {}, @@ -356,7 +365,8 @@ function silentLogging() { logChildOutput: () => {}, flushChildOutput: () => {}, logEvent: () => {}, - logError: () => {} + logError: () => {}, + ...overrides } }; } @@ -1872,7 +1882,7 @@ test('settings:set asks the disk whether the folder is there', async (t) => { const settings = fakeSettingsStore(); const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs } }); - assert.deepEqual(await main.invoke('settings:set', 'newSiteLocation', folder), { ok: true, settings: { locale: null, phpVersion: '8.3', wpDebug: true, scriptDebug: true, autoStartServer: false, autoStartWatch: false, quitBehavior: 'stop', newSiteLocation: folder } }); + assert.deepEqual(await main.invoke('settings:set', 'newSiteLocation', folder), { ok: true, settings: { locale: null, phpVersion: '8.3', wpDebug: true, scriptDebug: true, autoStartServer: false, autoStartWatch: false, quitBehavior: 'stop', theme: 'system', newSiteLocation: folder } }); const gone = await main.invoke('settings:set', 'newSiteLocation', path.join(folder, 'gone')); assert.equal(gone.ok, false); assert.equal(settings.values.preferences.newSiteLocation, folder); @@ -1887,12 +1897,65 @@ test('settings:set refuses a key that is not a setting without writing, whatever const settings = fakeSettingsStore({ preferences: { wporgHandle: 'janedoe' } }); const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs } }); - assert.equal((await main.invoke('settings:set', 'theme', folder)).ok, false); + assert.equal((await main.invoke('settings:set', 'editor', folder)).ok, false); assert.equal((await main.invoke('settings:set', '__proto__', folder)).ok, false); assert.equal((await main.invoke('settings:set', 'newSiteLocation', 'sites')).ok, false, 'a path that is not a full one'); assert.deepEqual(settings.values.preferences, { wporgHandle: 'janedoe' }); }); +// The theme (#560) is Electron's to apply: main gives it `nativeTheme`, and +// Chromium answers the page's `prefers-color-scheme` from that. +test('settings:set gives the theme to Electron as it is kept, and the fallback when it is forgotten (#560)', async () => { + const settings = fakeSettingsStore(); + const main = loadMain({ stubs: { ...silentLogging(), ...settings.stubs } }); + + assert.equal((await main.invoke('settings:set', 'theme', 'dark')).settings.theme, 'dark'); + assert.equal(main.electron.nativeTheme.themeSource, 'dark'); + assert.equal(settings.values.preferences.theme, 'dark'); + + // A refusal leaves it. + assert.equal((await main.invoke('settings:set', 'theme', 'custom')).ok, false); + assert.equal(main.electron.nativeTheme.themeSource, 'dark'); + + assert.equal((await main.invoke('settings:set', 'theme', null)).settings.theme, 'system'); + assert.equal(main.electron.nativeTheme.themeSource, 'system'); + + // Another setting does not touch it. + await main.invoke('settings:set', 'theme', 'light'); + await main.invoke('settings:set', 'wpDebug', false); + assert.equal(main.electron.nativeTheme.themeSource, 'light'); +}); + +test('the ready path gives Electron the stored theme before the window is made, and makes the window in it (#560)', async () => { + const main = loadMain({ ready: true, stubs: { ...silentLogging(), ...fakeSettingsStore({ preferences: { theme: 'dark' } }).stubs, './i18n.cjs': { resolveCatalog: async () => null } } }); + await menuBuilt(main); + + assert.equal(main.electron.nativeTheme.themeSource, 'dark'); + assert.equal(main.windows.length, 1); + assert.equal(main.windows[0].options.backgroundColor, '#1e1e1e', 'a dark window is made dark, not white until its page paints'); +}); + +test('the ready path makes a light window light, and a store that cannot be read leaves the system\'s theme (#560)', async () => { + const light = loadMain({ ready: true, stubs: { ...silentLogging(), ...fakeSettingsStore({ preferences: { theme: 'light' } }).stubs, './i18n.cjs': { resolveCatalog: async () => null } } }); + await menuBuilt(light); + assert.equal(light.electron.nativeTheme.themeSource, 'light'); + assert.equal(light.windows[0].options.backgroundColor, '#fcfcfc'); + + const logged = []; + const broken = loadMain({ + ready: true, + stubs: { + ...silentLogging({ logError: (scope, message) => logged.push([scope, message]) }), + './settings-store': { getStore: async () => { throw new Error('settings.json is not JSON'); }, peekStore: () => null }, + './i18n.cjs': { resolveCatalog: async () => null } + } + }); + await menuBuilt(broken); + assert.equal(broken.electron.nativeTheme.themeSource, 'system'); + assert.equal(broken.windows.length, 1, 'the window still opens'); + assert.ok(logged.some(([scope, message]) => scope === 'theme' && message.includes('settings.json is not JSON')), `logged: ${JSON.stringify(logged)}`); +}); + // The menu's Settings… reaches the main window and brings it forward, and // not whichever window Electron lists first: a patch window is one too. async function menuBuilt(main) { @@ -6544,10 +6607,10 @@ test('settings:set keeps a language the build has, and refuses one it has not (# stubs: { ...silentLogging(), ...settings.stubs, './i18n.cjs': { resolveCatalog: async () => null, languageChoices: () => [{ tag: 'de', label: 'Deutsch' }, { tag: 'en', label: 'English' }] } } }); - assert.deepEqual(await main.invoke('settings:set', 'locale', 'de'), { ok: true, settings: { locale: 'de', phpVersion: '8.3', wpDebug: true, scriptDebug: true, autoStartServer: false, autoStartWatch: false, quitBehavior: 'stop', newSiteLocation: null } }); + assert.deepEqual(await main.invoke('settings:set', 'locale', 'de'), { ok: true, settings: { locale: 'de', phpVersion: '8.3', wpDebug: true, scriptDebug: true, autoStartServer: false, autoStartWatch: false, quitBehavior: 'stop', theme: 'system', newSiteLocation: null } }); assert.equal((await main.invoke('settings:set', 'locale', 'fr')).ok, false); assert.equal(settings.values.preferences.locale, 'de'); - assert.deepEqual(await main.invoke('settings:set', 'locale', null), { ok: true, settings: { locale: null, phpVersion: '8.3', wpDebug: true, scriptDebug: true, autoStartServer: false, autoStartWatch: false, quitBehavior: 'stop', newSiteLocation: null } }); + assert.deepEqual(await main.invoke('settings:set', 'locale', null), { ok: true, settings: { locale: null, phpVersion: '8.3', wpDebug: true, scriptDebug: true, autoStartServer: false, autoStartWatch: false, quitBehavior: 'stop', theme: 'system', newSiteLocation: null } }); }); test('app:relaunch relaunches through a quit, so the child sweep runs, without the launch\'s link or --lang (#559)', async (t) => { diff --git a/tests/unit/settings-view.test.cjs b/tests/unit/settings-view.test.cjs index 0908a210..e2c649cf 100644 --- a/tests/unit/settings-view.test.cjs +++ b/tests/unit/settings-view.test.cjs @@ -3,7 +3,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { githubAccountLine, newSiteLocationNote, languageItems, languageValue, languageChanged, phpVersionChoice, quitItems, resumeFor, SYSTEM_LANGUAGE } = require('../../src/renderer/settings-view.cjs'); +const { githubAccountLine, newSiteLocationNote, languageItems, languageValue, languageChanged, phpVersionChoice, quitItems, themeItems, resumeFor, SYSTEM_LANGUAGE } = require('../../src/renderer/settings-view.cjs'); test('the GitHub line says the account is still being read, and offers no sign-out, until it is', () => { assert.deepEqual(githubAccountLine(null), { text: 'Reading…', canSignOut: false }); @@ -74,6 +74,14 @@ test('the quit control offers stop and restart, and not the prototype\'s leaving assert.ok(quitItems().every((item) => item.label)); }); +test('the theme control offers light, dark and system, in that order, and not the prototype\'s custom colours (#560)', () => { + assert.deepEqual(themeItems(), [ + { value: 'light', label: 'Light' }, + { value: 'dark', label: 'Dark' }, + { value: 'system', label: 'System' } + ]); +}); + test('what the next launch starts for a site comes from the list the quit left', () => { const resume = { servers: ['/a', '/both'], watches: ['/w', '/both'] }; assert.deepEqual(resumeFor(resume, '/a'), { server: true, watch: false }); diff --git a/tests/unit/settings.test.cjs b/tests/unit/settings.test.cjs index e0fbeae4..5c499e98 100644 --- a/tests/unit/settings.test.cjs +++ b/tests/unit/settings.test.cjs @@ -12,19 +12,19 @@ const languages = (tags) => ({ isLanguage: (tag) => tags.includes(tag) }); const php = (versions) => ({ isPhpVersion: (version) => versions.includes(version) }); test('readSettings falls back for a store with nothing in it, and for values of the wrong kind', () => { - const fallbacks = { locale: null, phpVersion: '8.3', wpDebug: true, scriptDebug: true, autoStartServer: false, autoStartWatch: false, quitBehavior: 'stop', newSiteLocation: null }; + const fallbacks = { locale: null, phpVersion: '8.3', wpDebug: true, scriptDebug: true, autoStartServer: false, autoStartWatch: false, quitBehavior: 'stop', theme: 'system', newSiteLocation: null }; assert.deepEqual(readSettings(), fallbacks); assert.deepEqual(readSettings(undefined), fallbacks); assert.deepEqual(readSettings({}), fallbacks); - assert.deepEqual(readSettings({ newSiteLocation: 42, locale: ['de'], phpVersion: 8.4, wpDebug: 'false', scriptDebug: 0, autoStartServer: 'yes', autoStartWatch: 1, quitBehavior: 'leave' }), fallbacks); + assert.deepEqual(readSettings({ newSiteLocation: 42, locale: ['de'], phpVersion: 8.4, wpDebug: 'false', scriptDebug: 0, autoStartServer: 'yes', autoStartWatch: 1, quitBehavior: 'leave', theme: 'custom' }), fallbacks); assert.deepEqual(readSettings({ newSiteLocation: '', locale: '', phpVersion: '', wpDebug: null, scriptDebug: null }), fallbacks); assert.deepEqual(readSettings('not an object'), fallbacks); }); test('readSettings gives back a stored folder without asking the disk about it', () => { assert.deepEqual( - readSettings({ newSiteLocation: '/Users/jane/sites', locale: 'de', phpVersion: '8.4', wpDebug: false, scriptDebug: false, autoStartServer: true, autoStartWatch: true, quitBehavior: 'restart' }), - { locale: 'de', phpVersion: '8.4', wpDebug: false, scriptDebug: false, autoStartServer: true, autoStartWatch: true, quitBehavior: 'restart', newSiteLocation: '/Users/jane/sites' } + readSettings({ newSiteLocation: '/Users/jane/sites', locale: 'de', phpVersion: '8.4', wpDebug: false, scriptDebug: false, autoStartServer: true, autoStartWatch: true, quitBehavior: 'restart', theme: 'dark' }), + { locale: 'de', phpVersion: '8.4', wpDebug: false, scriptDebug: false, autoStartServer: true, autoStartWatch: true, quitBehavior: 'restart', theme: 'dark', newSiteLocation: '/Users/jane/sites' } ); }); @@ -54,7 +54,7 @@ test('a folder is refused when it is not a string, not a full path, or not on th test('a key that is not a setting is refused, and nothing is asked of the disk', () => { let asked = 0; - const result = acceptSetting('theme', 'dark', { isAbsolute: () => { asked++; return true; }, isDirectory: () => { asked++; return true; } }); + const result = acceptSetting('editor', 'vim', { isAbsolute: () => { asked++; return true; }, isDirectory: () => { asked++; return true; } }); assert.equal(result.ok, false); assert.equal(asked, 0); }); @@ -91,6 +91,15 @@ test('a debug flag is on or off, nothing means the fallback, and a string is ref } }); +test('the theme is light, dark or system, nothing means the system\'s, and the prototype\'s custom is refused (#560)', () => { + for (const theme of ['light', 'dark', 'system']) assert.deepEqual(acceptSetting('theme', theme, {}), { ok: true, value: theme }); + assert.deepEqual(acceptSetting('theme', null, {}), { ok: true, value: null }); + assert.deepEqual(acceptSetting('theme', '', {}), { ok: true, value: null }); + assert.deepEqual(acceptSetting('theme', 'custom', {}), { ok: false, error: 'Choose light, dark, or your system’s theme.' }); + assert.equal(acceptSetting('theme', 'Dark', {}).ok, false); + assert.equal(acceptSetting('theme', true, {}).ok, false); +}); + test('what happens on quit is stop or restart, nothing means the fallback, and anything else is refused', () => { assert.deepEqual(acceptSetting('quitBehavior', 'restart', {}), { ok: true, value: 'restart' }); assert.deepEqual(acceptSetting('quitBehavior', 'stop', {}), { ok: true, value: 'stop' }); diff --git a/tests/unit/theme.test.cjs b/tests/unit/theme.test.cjs new file mode 100644 index 00000000..74e03694 --- /dev/null +++ b/tests/unit/theme.test.cjs @@ -0,0 +1,27 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { THEMES, themeColorSeeds, windowBackground, DARK_BACKGROUND, LIGHT_BACKGROUND } = require('../../src/theme.cjs'); + +test('the themes are light, dark and system: the prototype\'s custom colours are not one (#560)', () => { + assert.deepEqual(THEMES, ['light', 'dark', 'system']); +}); + +// In the light scheme the provider is given no colour, so the tokens +// stylesheet's values stand as they ship: generating them again from a seed +// would move every colour by a hair, and the light theme is what every +// picture in the guide shows. In the dark scheme it is given the dark seed +// and builds every token from it. +test('the provider is seeded in the dark scheme only, and from the dark background', () => { + assert.deepEqual(themeColorSeeds(false), {}); + assert.deepEqual(themeColorSeeds(true), { background: DARK_BACKGROUND }); +}); + +test('a window is made in the colour of its scheme, and the dark one is the seed the theme is built from', () => { + assert.equal(windowBackground(true), DARK_BACKGROUND); + assert.equal(windowBackground(false), LIGHT_BACKGROUND); + assert.match(DARK_BACKGROUND, /^#[0-9a-f]{6}$/); + assert.match(LIGHT_BACKGROUND, /^#[0-9a-f]{6}$/); +}); From 64cad1acff6b8e0d79d8b9de4f96b6c3f61d1a6f Mon Sep 17 00:00:00 2001 From: Francesco Bigiarini Date: Tue, 6 Oct 2026 17:42:44 +0200 Subject: [PATCH 2/3] Keep the window's colour with the theme, and the light popover as it was From the first review pass. The colour the window was made with is given again whenever Electron says the theme changed: a setting changed, the system's theme under 'system', or a deep link that opened the window before the stored theme was read. The popover's ring and shadow are restated in the dark scheme only, so the light theme is what it was, and a rule that matched nothing is gone. The theme journey reads the light body once the app has repainted. The theme's words carry a translator's context, SHOTS_THEME is checked against the themes, and the tests name the colours by their constants. Co-Authored-By: Claude Fable 5.1 --- scripts/screenshots/fixtures.cjs | 8 ++++++- src/main.js | 15 ++++++++---- src/renderer/settings-view.cjs | 8 +++---- src/renderer/shell.css | 20 ++++++++-------- tests/e2e/journeys/settings.spec.js | 11 +++++---- tests/unit/ipc-wiring.test.cjs | 37 ++++++++++++++++++++++++++--- tests/unit/theme.test.cjs | 14 ++++++----- 7 files changed, 81 insertions(+), 32 deletions(-) diff --git a/scripts/screenshots/fixtures.cjs b/scripts/screenshots/fixtures.cjs index 6383967d..0da0fc04 100644 --- a/scripts/screenshots/fixtures.cjs +++ b/scripts/screenshots/fixtures.cjs @@ -21,6 +21,7 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); const { pathToFileURL } = require('url'); +const { THEMES } = require('../../src/theme.cjs'); // The fixture layer the journeys build their sites with: the app's own Git // binary, so a repository made here is one the app reads as it reads a clone. const { gitOk, initRepo, commitFiles, removeRepo } = require('../../tests/unit/helpers/git.cjs'); @@ -339,8 +340,13 @@ function buildFixture(variant) { } // The pictures are of the light theme whatever the maintainer's machine is -// set to (#560), unless a dark one is asked for: `SHOTS_THEME=dark`. +// set to (#560), unless a dark one is asked for: `SHOTS_THEME=dark`. Anything +// else is refused here: the app would fall back to the system's theme, which +// is the one thing the pin exists to keep out of the pictures. const SHOTS_THEME = process.env.SHOTS_THEME || 'light'; +if (!THEMES.includes(SHOTS_THEME)) { + throw new Error(`SHOTS_THEME must be one of ${THEMES.join(', ')}, got "${SHOTS_THEME}"`); +} function writeSettings(userDataDir, settings) { fs.writeFileSync( diff --git a/src/main.js b/src/main.js index f37259ec..ff94ca2a 100644 --- a/src/main.js +++ b/src/main.js @@ -688,9 +688,9 @@ ipcMain.handle('deep-link:ready', () => { // Resolved once: main applies it at startup for its own strings (the menu, the // native dialogs, the sentences it sends), and the window gets the same reply, // so the two cannot end up in different languages. That is also why a change -// in the settings shows after a relaunch and not before. This is the first -// read of the store, before there is a window: a store that cannot be read -// is logged and counts as no choice, since the window has to open to say so. +// in the settings shows after a relaunch and not before. Read before there +// is a window: a store that cannot be read is logged and counts as no +// choice, since the window has to open to say so. const LANGUAGES_DIR = path.join(__dirname, 'languages'); let localeReplyPromise = null; function localeReply() { @@ -2671,7 +2671,14 @@ app.whenReady().then(async () => { // renderer output into the log file, which only applies to windows created // afterwards. initLogging(); - // Before the window: it is made in the theme. + // Before the window: it is made in the theme. And kept in it: the colour + // the window was made with shows wherever the page has not painted yet (a + // live resize, a reload), so it follows the theme as the page does, when + // the setting changes, when the system's theme does under 'system', and + // when a deep link opened the window before the stored theme was read. + nativeTheme.on('updated', () => { + if (mainWindow && !mainWindow.isDestroyed()) mainWindow.setBackgroundColor(windowBackground(nativeTheme.shouldUseDarkColors)); + }); await applyStoredTheme(); // Before the menu and the window: both build their labels from `__()`. applyLocale(await localeReply(), { setLocaleData, addFilter }); diff --git a/src/renderer/settings-view.cjs b/src/renderer/settings-view.cjs index 5ab38796..af6e4394 100644 --- a/src/renderer/settings-view.cjs +++ b/src/renderer/settings-view.cjs @@ -8,7 +8,7 @@ * returns. */ -const { __, sprintf } = require('@wordpress/i18n'); +const { __, _x, sprintf } = require('@wordpress/i18n'); /** * The line under "GitHub": whose account the app holds, or why none. @@ -131,9 +131,9 @@ function quitItems() { */ function themeItems() { return [ - { value: 'light', label: __('Light') }, - { value: 'dark', label: __('Dark') }, - { value: 'system', label: __('System') } + { value: 'light', label: _x('Light', 'the window’s theme') }, + { value: 'dark', label: _x('Dark', 'the window’s theme') }, + { value: 'system', label: _x('System', 'the window’s theme: the operating system’s') } ]; } diff --git a/src/renderer/shell.css b/src/renderer/shell.css index c5acb48b..dd573cc8 100644 --- a/src/renderer/shell.css +++ b/src/renderer/shell.css @@ -692,8 +692,10 @@ body { with the tokens the design system's own dialogs use, so they follow the theme as everything else does. Their shadows stay as the library draws them: a shadow is black under either theme, as the design system's own - are. The ring the popover draws in the same property is restated as the - hairline, since it was a light grey of its own. */ + are. The ring the popover draws in the same property as its shadow is a + light grey of its own, so in the dark theme, and only there, the two are + restated as the hairline and one soft layer; in the light theme the + popover is as the library draws it. */ .components-modal__frame, .components-popover__content { background: var(--wpds-color-background-surface-neutral-strong); @@ -706,14 +708,12 @@ body { color: var(--wpds-color-foreground-content-neutral); } -.components-popover__content { - box-shadow: - 0 0 0 var(--wpds-border-width-xs) var(--wpds-color-stroke-surface-neutral), - 0 4px 12px color-mix(in srgb, var(--wpds-color-foreground-content-neutral) 10%, transparent); -} - -.components-dropdown__content .components-menu-group + .components-menu-group { - border-top-color: var(--wpds-color-stroke-surface-neutral); +@media (prefers-color-scheme: dark) { + .components-popover__content { + box-shadow: + 0 0 0 var(--wpds-border-width-xs) var(--wpds-color-stroke-surface-neutral), + 0 4px 12px color-mix(in srgb, var(--wpds-color-foreground-content-neutral) 10%, transparent); + } } /* What a first-timer cannot know about pull requests, behind its question. */ diff --git a/tests/e2e/journeys/settings.spec.js b/tests/e2e/journeys/settings.spec.js index 75c0de8e..79da457d 100644 --- a/tests/e2e/journeys/settings.spec.js +++ b/tests/e2e/journeys/settings.spec.js @@ -20,6 +20,7 @@ const { test, expect } = require( '../helpers/app.cjs' ); const ui = require( '../helpers/ui.cjs' ); const { makeSite } = require( '../helpers/git-site.cjs' ); const { pseudoLocalize } = require( '../../../src/renderer/pseudo-locale.cjs' ); +const { DARK_BACKGROUND } = require( '../../../src/theme.cjs' ); const openFromMenu = ( app ) => app.evaluate( ( { Menu } ) => Menu.getApplicationMenu().getMenuItemById( 'settings' ).click() ); @@ -295,9 +296,10 @@ test( 'the theme set in the settings is the one the window is painted in, and a await expect.poll( () => session.readSettings().preferences?.theme ).toBe( 'light' ); await expect.poll( () => themeSource( app ) ).toBe( 'light' ); await expect.poll( () => prefersDark( page ) ).toBe( false ); - const lightBody = await bodyColour(); - expect( lightBody ).toBe( await tokenColour( 'var(--wpds-color-background-surface-neutral)' ) ); - expect( lightBody ).not.toBe( darkBody ); + // The scheme flips before the app has repainted for it, so the body is + // read once it has: until then the token is the dark one too. + await expect.poll( bodyColour ).not.toBe( darkBody ); + expect( await bodyColour() ).toBe( await tokenColour( 'var(--wpds-color-background-surface-neutral)' ) ); await expect.poll( terminalSurface ).toBe( await tokenColour( 'var(--wpds-color-background-surface-neutral-weak)' ) ); expect( await terminalSurface() ).not.toBe( darkTerminal ); @@ -309,11 +311,12 @@ test( 'the theme set in the settings is the one the window is painted in, and a // INVARIANT — started again with dark kept, the window is made dark, so // it is not white before its page paints, and the control says so. + // CHARACTERISATION — the colour it is made in is the dark seed. await themes.getByRole( 'radio', { name: 'Dark', exact: true } ).click(); await expect.poll( () => session.readSettings().preferences?.theme ).toBe( 'dark' ); const again = await session.restart(); expect( await themeSource( again.app ) ).toBe( 'dark' ); - expect( ( await again.app.evaluate( ( { BrowserWindow } ) => BrowserWindow.getAllWindows()[ 0 ].getBackgroundColor() ) ).toLowerCase() ).toBe( '#1e1e1e' ); + expect( ( await again.app.evaluate( ( { BrowserWindow } ) => BrowserWindow.getAllWindows()[ 0 ].getBackgroundColor() ) ).toLowerCase() ).toBe( DARK_BACKGROUND ); await expect.poll( () => prefersDark( again.page ) ).toBe( true ); await ui.settingsButton( again.page ).click(); await expect( ui.settingsDialog( again.page ).getByRole( 'radio', { name: 'Dark', exact: true } ) ).toBeChecked(); diff --git a/tests/unit/ipc-wiring.test.cjs b/tests/unit/ipc-wiring.test.cjs index 17555862..c96c0233 100644 --- a/tests/unit/ipc-wiring.test.cjs +++ b/tests/unit/ipc-wiring.test.cjs @@ -50,6 +50,7 @@ const { // The applied-layer module turns the handler's measured status into the // attribution the renderer shows. const { attributeConflicts } = require('../../src/renderer/applied-layer.cjs'); +const { DARK_BACKGROUND, LIGHT_BACKGROUND } = require('../../src/theme.cjs'); const { nodeExecPath } = require('../../src/node-shims.cjs'); const SRC_DIR = path.join(__dirname, '..', '..', 'src'); const MAIN_PATH = path.join(SRC_DIR, 'main.js'); @@ -90,6 +91,7 @@ function createElectronStub({ ready = false } = {}) { const handlers = new Map(); const oneWay = new Map(); const appEvents = new Map(); + const nativeThemeListeners = []; const windows = []; const calls = { openExternal: [], @@ -128,6 +130,7 @@ function createElectronStub({ ready = false } = {}) { show() {} focus() {} restore() {} + setBackgroundColor(color) { this.options = { ...this.options, backgroundColor: color }; this.backgrounds = [...(this.backgrounds || []), color]; } isMinimized() { return false; } isDestroyed() { return false; } close() {} @@ -178,7 +181,9 @@ function createElectronStub({ ready = false } = {}) { nativeTheme: { themeSource: 'system', get shouldUseDarkColors() { return this.themeSource === 'dark'; }, - on() {} + on(event, listener) { nativeThemeListeners.push({ event, listener }); }, + // What Electron would do: tell main the theme changed. + update() { for (const { event, listener } of nativeThemeListeners) if (event === 'updated') listener(); } }, Menu: { buildFromTemplate: (template) => ({ template }), @@ -1932,14 +1937,40 @@ test('the ready path gives Electron the stored theme before the window is made, assert.equal(main.electron.nativeTheme.themeSource, 'dark'); assert.equal(main.windows.length, 1); - assert.equal(main.windows[0].options.backgroundColor, '#1e1e1e', 'a dark window is made dark, not white until its page paints'); + assert.equal(main.windows[0].options.backgroundColor, DARK_BACKGROUND, 'a dark window is made dark, not white until its page paints'); +}); + +// The colour the window was made with shows wherever its page has not +// painted yet, so it follows the theme: Electron says the theme changed, +// and the window is given the colour of the theme it is now in. +test('the window is given the colour of the theme as it changes, a deep link\'s window included (#560)', async () => { + const main = loadMain({ ready: true, stubs: { ...silentLogging(), ...fakeSettingsStore({ preferences: { theme: 'dark' } }).stubs, './i18n.cjs': { resolveCatalog: async () => null } } }); + await menuBuilt(main); + const [window] = main.windows; + + await main.invoke('settings:set', 'theme', 'light'); + main.electron.nativeTheme.update(); + assert.deepEqual(window.backgrounds, [LIGHT_BACKGROUND]); + + // The system's theme changing under 'system': Electron says so, and the + // window follows what it now says of the colours. + await main.invoke('settings:set', 'theme', 'system'); + main.electron.nativeTheme.update(); + Object.defineProperty(main.electron.nativeTheme, 'shouldUseDarkColors', { value: true, configurable: true }); + main.electron.nativeTheme.update(); + assert.deepEqual(window.backgrounds, [LIGHT_BACKGROUND, LIGHT_BACKGROUND, DARK_BACKGROUND]); + + // A window that is gone is left alone. + window.isDestroyed = () => true; + main.electron.nativeTheme.update(); + assert.equal(window.backgrounds.length, 3); }); test('the ready path makes a light window light, and a store that cannot be read leaves the system\'s theme (#560)', async () => { const light = loadMain({ ready: true, stubs: { ...silentLogging(), ...fakeSettingsStore({ preferences: { theme: 'light' } }).stubs, './i18n.cjs': { resolveCatalog: async () => null } } }); await menuBuilt(light); assert.equal(light.electron.nativeTheme.themeSource, 'light'); - assert.equal(light.windows[0].options.backgroundColor, '#fcfcfc'); + assert.equal(light.windows[0].options.backgroundColor, LIGHT_BACKGROUND); const logged = []; const broken = loadMain({ diff --git a/tests/unit/theme.test.cjs b/tests/unit/theme.test.cjs index 74e03694..89c22f42 100644 --- a/tests/unit/theme.test.cjs +++ b/tests/unit/theme.test.cjs @@ -3,7 +3,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { THEMES, themeColorSeeds, windowBackground, DARK_BACKGROUND, LIGHT_BACKGROUND } = require('../../src/theme.cjs'); +const { THEMES, themeColorSeeds, windowBackground, DARK_BACKGROUND } = require('../../src/theme.cjs'); test('the themes are light, dark and system: the prototype\'s custom colours are not one (#560)', () => { assert.deepEqual(THEMES, ['light', 'dark', 'system']); @@ -19,9 +19,11 @@ test('the provider is seeded in the dark scheme only, and from the dark backgrou assert.deepEqual(themeColorSeeds(true), { background: DARK_BACKGROUND }); }); -test('a window is made in the colour of its scheme, and the dark one is the seed the theme is built from', () => { - assert.equal(windowBackground(true), DARK_BACKGROUND); - assert.equal(windowBackground(false), LIGHT_BACKGROUND); - assert.match(DARK_BACKGROUND, /^#[0-9a-f]{6}$/); - assert.match(LIGHT_BACKGROUND, /^#[0-9a-f]{6}$/); +// The window's colour and the provider's seed have to be the one colour, or +// the page changes colour as it mounts; and both have to be what Electron +// takes for a window's colour, `#rrggbb`. +test('a window is made in the colour of its scheme, which in the dark scheme is the seed the theme is built from', () => { + assert.equal(windowBackground(true), themeColorSeeds(true).background); + assert.notEqual(windowBackground(false), windowBackground(true)); + for (const colour of [windowBackground(true), windowBackground(false)]) assert.match(colour, /^#[0-9a-f]{6}$/); }); From 742aa6809a66f3201c898c250405b5f7a1e2a7ff Mon Sep 17 00:00:00 2001 From: Francesco Bigiarini Date: Tue, 6 Oct 2026 17:54:05 +0200 Subject: [PATCH 3/3] Paint the window from the setting itself, and refuse the system theme in the shots From the second review pass. The window's colour is set as the theme setting is changed, and when the stored theme is applied, rather than left to Electron's `updated`, which is not promised for a change to 'system'; the listener stays for the system's theme changing under 'system'. The theme journey reads the window's own colour after each choice. SHOTS_THEME takes light or dark and not the system's, which is what the pin keeps out of the pictures. The dark popover's soft layer is said to be the glow it is. Co-Authored-By: Claude Fable 5.1 --- scripts/screenshots/fixtures.cjs | 10 ++++++---- src/main.js | 30 +++++++++++++++++++---------- src/renderer/shell.css | 14 ++++++++------ tests/e2e/journeys/settings.spec.js | 15 +++++++++++---- tests/unit/ipc-wiring.test.cjs | 19 ++++++++++++------ 5 files changed, 58 insertions(+), 30 deletions(-) diff --git a/scripts/screenshots/fixtures.cjs b/scripts/screenshots/fixtures.cjs index 0da0fc04..1906a08b 100644 --- a/scripts/screenshots/fixtures.cjs +++ b/scripts/screenshots/fixtures.cjs @@ -341,11 +341,13 @@ function buildFixture(variant) { // The pictures are of the light theme whatever the maintainer's machine is // set to (#560), unless a dark one is asked for: `SHOTS_THEME=dark`. Anything -// else is refused here: the app would fall back to the system's theme, which -// is the one thing the pin exists to keep out of the pictures. +// else is refused here, the system's theme by name included: a value the +// app does not know falls back to the system's, which is the one thing the +// pin exists to keep out of the pictures. +const SHOTS_THEMES = THEMES.filter((theme) => theme !== 'system'); const SHOTS_THEME = process.env.SHOTS_THEME || 'light'; -if (!THEMES.includes(SHOTS_THEME)) { - throw new Error(`SHOTS_THEME must be one of ${THEMES.join(', ')}, got "${SHOTS_THEME}"`); +if (!SHOTS_THEMES.includes(SHOTS_THEME)) { + throw new Error(`SHOTS_THEME must be one of ${SHOTS_THEMES.join(', ')}, got "${SHOTS_THEME}"`); } function writeSettings(userDataDir, settings) { diff --git a/src/main.js b/src/main.js index ff94ca2a..ded9d5a9 100644 --- a/src/main.js +++ b/src/main.js @@ -727,6 +727,16 @@ async function applyStoredTheme() { } catch (e) { logError('theme', `the settings could not be read, so the theme is the system's: ${String(e && e.message ? e.message : e)}`); } + // A deep link can have opened the window while the store was read. + paintWindowForTheme(); +} + +// The colour the window was made with shows wherever the page has not +// painted yet (a live resize, a reload), so it is given again whenever the +// theme is: by the setting, here and in `settings:set`, and by Electron's +// `updated`, which is how the system's theme reaches it under 'system'. +function paintWindowForTheme() { + if (mainWindow && !mainWindow.isDestroyed()) mainWindow.setBackgroundColor(windowBackground(nativeTheme.shouldUseDarkColors)); } // The languages the settings offer: what the build ships, read once. @@ -2671,14 +2681,9 @@ app.whenReady().then(async () => { // renderer output into the log file, which only applies to windows created // afterwards. initLogging(); - // Before the window: it is made in the theme. And kept in it: the colour - // the window was made with shows wherever the page has not painted yet (a - // live resize, a reload), so it follows the theme as the page does, when - // the setting changes, when the system's theme does under 'system', and - // when a deep link opened the window before the stored theme was read. - nativeTheme.on('updated', () => { - if (mainWindow && !mainWindow.isDestroyed()) mainWindow.setBackgroundColor(windowBackground(nativeTheme.shouldUseDarkColors)); - }); + // Before the window: it is made in the theme, and kept in it when the + // system's theme changes under 'system'. + nativeTheme.on('updated', paintWindowForTheme); await applyStoredTheme(); // Before the menu and the window: both build their labels from `__()`. applyLocale(await localeReply(), { setLocaleData, addFilter }); @@ -3599,8 +3604,13 @@ ipcMain.handle('settings:set', async (_e, key, value) => { await setPreference(key, accepted.value); const s = await getStore(); const settings = readSettings(s.get('preferences')); - // The theme applies at once (#560): Electron tells the window. - if (key === 'theme') nativeTheme.themeSource = settings.theme; + // The theme applies at once (#560): Electron tells the page, and the + // window's own colour is set here rather than left to Electron's + // `updated`, which is not promised for a change to 'system'. + if (key === 'theme') { + nativeTheme.themeSource = settings.theme; + paintWindowForTheme(); + } return { ok: true, settings }; }); diff --git a/src/renderer/shell.css b/src/renderer/shell.css index dd573cc8..ff0db516 100644 --- a/src/renderer/shell.css +++ b/src/renderer/shell.css @@ -690,12 +690,14 @@ body { in its own stylesheet, with no token behind the colour, so in the dark theme (#560) they would be the one light thing on the page. Painted here with the tokens the design system's own dialogs use, so they follow the - theme as everything else does. Their shadows stay as the library draws - them: a shadow is black under either theme, as the design system's own - are. The ring the popover draws in the same property as its shadow is a - light grey of its own, so in the dark theme, and only there, the two are - restated as the hairline and one soft layer; in the light theme the - popover is as the library draws it. */ + theme as everything else does. The modal's shadow stays as the library + draws it: black, as the design system's own are under either theme. The + ring the popover draws in the same property as its shadow is a light grey + of its own, so in the dark theme, and only there, the two are restated: + the hairline, and one soft layer mixed from the text's colour, which on a + dark surface is a faint light glow rather than a black shadow, since a + black shadow on a near-black page is no edge at all. In the light theme + the popover is as the library draws it. */ .components-modal__frame, .components-popover__content { background: var(--wpds-color-background-surface-neutral-strong); diff --git a/tests/e2e/journeys/settings.spec.js b/tests/e2e/journeys/settings.spec.js index 79da457d..22095b74 100644 --- a/tests/e2e/journeys/settings.spec.js +++ b/tests/e2e/journeys/settings.spec.js @@ -20,7 +20,7 @@ const { test, expect } = require( '../helpers/app.cjs' ); const ui = require( '../helpers/ui.cjs' ); const { makeSite } = require( '../helpers/git-site.cjs' ); const { pseudoLocalize } = require( '../../../src/renderer/pseudo-locale.cjs' ); -const { DARK_BACKGROUND } = require( '../../../src/theme.cjs' ); +const { DARK_BACKGROUND, LIGHT_BACKGROUND } = require( '../../../src/theme.cjs' ); const openFromMenu = ( app ) => app.evaluate( ( { Menu } ) => Menu.getApplicationMenu().getMenuItemById( 'settings' ).click() ); @@ -257,6 +257,10 @@ test( 'the theme set in the settings is the one the window is painted in, and a // Playwright holds every other journey to. const { app, page } = await session.start( { ...site.settings, preferences: { theme: 'dark' } }, { colorScheme: null } ); const themeSource = ( electronApp ) => electronApp.evaluate( ( { nativeTheme } ) => nativeTheme.themeSource ); + // The colour the window itself was made with, which shows where the page + // has not painted yet; and the colour of the theme Electron says it is in. + const windowColour = ( electronApp ) => electronApp.evaluate( ( { BrowserWindow } ) => BrowserWindow.getAllWindows()[ 0 ].getBackgroundColor().toLowerCase() ); + const systemColour = ( electronApp ) => electronApp.evaluate( ( { nativeTheme } ) => nativeTheme.shouldUseDarkColors ).then( ( dark ) => ( dark ? DARK_BACKGROUND : LIGHT_BACKGROUND ) ); const prefersDark = ( window ) => window.evaluate( () => window.matchMedia( '(prefers-color-scheme: dark)' ).matches ); const bodyColour = () => page.evaluate( () => window.getComputedStyle( document.body ).backgroundColor ); const tokenColour = ( token ) => ui.tokenColour( page, token ); @@ -302,12 +306,15 @@ test( 'the theme set in the settings is the one the window is painted in, and a expect( await bodyColour() ).toBe( await tokenColour( 'var(--wpds-color-background-surface-neutral)' ) ); await expect.poll( terminalSurface ).toBe( await tokenColour( 'var(--wpds-color-background-surface-neutral-weak)' ) ); expect( await terminalSurface() ).not.toBe( darkTerminal ); + // And the window itself, where the page has not painted. + await expect.poll( () => windowColour( app ) ).toBe( LIGHT_BACKGROUND ); - // INVARIANT — System is kept as the system's, and Electron is left to - // follow it. + // INVARIANT — System is kept as the system's, Electron is left to follow + // it, and the window is the colour of whichever theme that is. await themes.getByRole( 'radio', { name: 'System', exact: true } ).click(); await expect.poll( () => session.readSettings().preferences?.theme ).toBe( 'system' ); await expect.poll( () => themeSource( app ) ).toBe( 'system' ); + await expect.poll( () => windowColour( app ) ).toBe( await systemColour( app ) ); // INVARIANT — started again with dark kept, the window is made dark, so // it is not white before its page paints, and the control says so. @@ -316,7 +323,7 @@ test( 'the theme set in the settings is the one the window is painted in, and a await expect.poll( () => session.readSettings().preferences?.theme ).toBe( 'dark' ); const again = await session.restart(); expect( await themeSource( again.app ) ).toBe( 'dark' ); - expect( ( await again.app.evaluate( ( { BrowserWindow } ) => BrowserWindow.getAllWindows()[ 0 ].getBackgroundColor() ) ).toLowerCase() ).toBe( DARK_BACKGROUND ); + expect( await windowColour( again.app ) ).toBe( DARK_BACKGROUND ); await expect.poll( () => prefersDark( again.page ) ).toBe( true ); await ui.settingsButton( again.page ).click(); await expect( ui.settingsDialog( again.page ).getByRole( 'radio', { name: 'Dark', exact: true } ) ).toBeChecked(); diff --git a/tests/unit/ipc-wiring.test.cjs b/tests/unit/ipc-wiring.test.cjs index c96c0233..9a5f03c8 100644 --- a/tests/unit/ipc-wiring.test.cjs +++ b/tests/unit/ipc-wiring.test.cjs @@ -1941,21 +1941,27 @@ test('the ready path gives Electron the stored theme before the window is made, }); // The colour the window was made with shows wherever its page has not -// painted yet, so it follows the theme: Electron says the theme changed, -// and the window is given the colour of the theme it is now in. -test('the window is given the colour of the theme as it changes, a deep link\'s window included (#560)', async () => { +// painted yet, so it follows the theme: the setting gives it as it is +// changed, without waiting for Electron's `updated`, which is not promised +// for a change to 'system'; and `updated` gives it when the system's theme +// changes under 'system'. +test('the window is given the colour of the theme as the setting changes, and as the system\'s theme does (#560)', async () => { const main = loadMain({ ready: true, stubs: { ...silentLogging(), ...fakeSettingsStore({ preferences: { theme: 'dark' } }).stubs, './i18n.cjs': { resolveCatalog: async () => null } } }); await menuBuilt(main); const [window] = main.windows; + // Made in the stored theme, and not painted again for it: the ready path + // reads the store before it makes the window. + assert.equal(window.options.backgroundColor, DARK_BACKGROUND); + assert.equal(window.backgrounds, undefined); + // The setting, with no event from Electron. await main.invoke('settings:set', 'theme', 'light'); - main.electron.nativeTheme.update(); assert.deepEqual(window.backgrounds, [LIGHT_BACKGROUND]); + await main.invoke('settings:set', 'theme', 'system'); + assert.deepEqual(window.backgrounds, [LIGHT_BACKGROUND, LIGHT_BACKGROUND], 'under system this machine is light'); // The system's theme changing under 'system': Electron says so, and the // window follows what it now says of the colours. - await main.invoke('settings:set', 'theme', 'system'); - main.electron.nativeTheme.update(); Object.defineProperty(main.electron.nativeTheme, 'shouldUseDarkColors', { value: true, configurable: true }); main.electron.nativeTheme.update(); assert.deepEqual(window.backgrounds, [LIGHT_BACKGROUND, LIGHT_BACKGROUND, DARK_BACKGROUND]); @@ -1963,6 +1969,7 @@ test('the window is given the colour of the theme as it changes, a deep link\'s // A window that is gone is left alone. window.isDestroyed = () => true; main.electron.nativeTheme.update(); + await main.invoke('settings:set', 'theme', 'light'); assert.equal(window.backgrounds.length, 3); });