Skip to content

Commit ab0cff4

Browse files
JohnMcLearclaude
andauthored
fix(7606): sync theme-color meta with client-side dark-mode switch (#7690)
* fix(7606): sync theme-color meta with client-side dark-mode switch PR #7636 emits <meta name="theme-color"> server-side from settings.skinVariants. That covers operators who hard-code a dark toolbar in settings.json, but not the runtime path: pad.ts auto-flips the toolbar to super-dark when enableDarkMode is on, the browser reports prefers-color-scheme: dark, and no localStorage white-mode override is set, plus the user can flip it via #options-darkmode. Both paths run skinVariants.updateSkinVariantsClasses(), which until now never touched the meta — so dark-mode users kept the light #ffffff baseline and saw a white address bar above a dark toolbar (stffen on #7606 after 2.7.3). Push the toolbar-color lookup into updateSkinVariantsClasses so the meta tracks every class change: the auto-switch on init, the user toggle, and the skinVariants builder. Mirrors the CSS-source-order table from src/node/utils/SkinColors.ts (last matching *-toolbar token wins). When no meta is present (non-colibris skin, server omits it) the helper is a no-op. Adds Playwright coverage for both paths under colorScheme: 'light' (manual toggle) and 'dark' (auto-switch on dark-OS clients — the case stffen reported). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(theme-color): action Qodo PR review (1) Bug — duplicated toolbar→color table: extract the CSS-source-order mapping and the default-color constant into src/static/js/skin_toolbar_colors, re-imported by both src/node/utils/SkinColors.ts (server, EJS template helper) and src/static/js/skin_variants.ts (client, runtime updates). Lives under static/js so the browser bundle can resolve it; server-side imports of static/js modules already exist (Changeset, AttributeMap, ImportHtml, hooks). One source of truth means a future palette change can no longer silently desync the server-rendered baseline meta from the client updates, which was the exact regression that brought us here. (2) Rule violation — 4-space continuation indentation in the new Playwright spec: re-indent the themeColor helper and the multiline test(...) call to the repo's 2-space rule (.editorconfig). Existing backend coverage (configuredToolbarColor unit tests + the specialpages server-render checks for both pad and timeslider) still passes against the refactored helper, so it's regression-locked end to end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent fe9727b commit ab0cff4

4 files changed

Lines changed: 99 additions & 24 deletions

File tree

src/node/utils/SkinColors.ts

Lines changed: 6 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,16 @@
11
'use strict';
22

3-
// Toolbar background colors that the colibris skin variants resolve to.
4-
// Mirrors --bg-color in src/static/skins/colibris/src/pad-variants.css. Only
5-
// the colibris skin has a known mapping; for any other skin we cannot derive
6-
// the toolbar color server-side and emit no theme-color meta.
7-
//
8-
// Order matters: when skinVariants contains multiple *-toolbar tokens the
9-
// CSS cascade picks the rule defined last in pad-variants.css, so iterate in
10-
// source order and let the last matching token win.
11-
const TOOLBAR_COLORS_IN_CSS_ORDER: Array<[string, string]> = [
12-
['super-light-toolbar', '#ffffff'],
13-
['light-toolbar', '#f2f3f4'],
14-
['super-dark-toolbar', '#485365'],
15-
['dark-toolbar', '#576273'],
16-
];
17-
18-
const COLIBRIS_DEFAULT_TOOLBAR_COLOR = '#ffffff';
3+
import {toolbarColorForTokens} from '../../static/js/skin_toolbar_colors';
194

205
// The toolbar color the user actually sees on first paint, derived from the
21-
// configured skin and skinVariants. Returns null when the skin is unknown so
22-
// callers can omit the meta rather than emit a misleading value.
6+
// configured skin and skinVariants. Only the colibris skin has a known
7+
// mapping (see src/static/js/skin_toolbar_colors). For any other skin we
8+
// cannot derive the toolbar color server-side and return null so callers can
9+
// omit the meta rather than emit a misleading value.
2310
export const configuredToolbarColor = (
2411
skinName: string | undefined | null,
2512
skinVariants: string | undefined | null,
2613
): string | null => {
2714
if (skinName !== 'colibris') return null;
28-
const tokens = new Set((skinVariants || '').split(/\s+/).filter(Boolean));
29-
let color: string | null = null;
30-
for (const [variant, c] of TOOLBAR_COLORS_IN_CSS_ORDER) {
31-
if (tokens.has(variant)) color = c;
32-
}
33-
return color || COLIBRIS_DEFAULT_TOOLBAR_COLOR;
15+
return toolbarColorForTokens((skinVariants || '').split(/\s+/).filter(Boolean));
3416
};
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
'use strict';
2+
3+
// Toolbar background colors that the colibris skin variants resolve to.
4+
// Mirrors --bg-color in src/static/skins/colibris/src/pad-variants.css. Lives
5+
// here (under static/js/) so both the browser bundle (skin_variants.ts) and
6+
// the server-side EJS helper (node/utils/SkinColors.ts) can import it without
7+
// duplication — a drift between client and server tables would silently
8+
// reintroduce the "address bar disagrees with toolbar" bug.
9+
//
10+
// Order matters: when skinVariants contains multiple *-toolbar tokens the
11+
// CSS cascade picks the rule defined last in pad-variants.css, so iterate in
12+
// source order and let the last matching token win.
13+
export const TOOLBAR_COLORS_IN_CSS_ORDER: ReadonlyArray<readonly [string, string]> = [
14+
['super-light-toolbar', '#ffffff'],
15+
['light-toolbar', '#f2f3f4'],
16+
['super-dark-toolbar', '#485365'],
17+
['dark-toolbar', '#576273'],
18+
];
19+
20+
export const COLIBRIS_DEFAULT_TOOLBAR_COLOR = '#ffffff';
21+
22+
// Resolve the toolbar color for a set of skin-variant tokens. Pure data: no
23+
// DOM, no Node APIs — safe to call from both server and client.
24+
export const toolbarColorForTokens = (tokens: Iterable<string>): string => {
25+
const set = new Set(tokens);
26+
let color = COLIBRIS_DEFAULT_TOOLBAR_COLOR;
27+
for (const [variant, c] of TOOLBAR_COLORS_IN_CSS_ORDER) {
28+
if (set.has(variant)) color = c;
29+
}
30+
return color;
31+
};

src/static/js/skin_variants.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,27 @@
11
// @ts-nocheck
22
'use strict';
33

4+
import {toolbarColorForTokens} from './skin_toolbar_colors';
5+
46
const containers = ['editor', 'background', 'toolbar'];
57
const colors = ['super-light', 'light', 'dark', 'super-dark'];
68

9+
// Keep <meta name="theme-color"> in sync with the toolbar the user actually
10+
// sees. The server emits a baseline derived from settings.skinVariants, but
11+
// pad.ts may flip the toolbar to super-dark on first paint (enableDarkMode
12+
// + prefers-color-scheme:dark + no localStorage white-mode override) and
13+
// the user can toggle via #options-darkmode. Without this, dark-mode users
14+
// keep the light meta and see a white address bar above a dark toolbar
15+
// (issue #7606 follow-up). Color resolution lives in skin_toolbar_colors so
16+
// the server-rendered baseline and the client updates share one source of
17+
// truth — Qodo flagged the prior duplicated table as a drift hazard.
18+
const updateThemeColorMeta = (newClasses: string[]) => {
19+
const meta = document.querySelector('meta[name="theme-color"]');
20+
if (!meta) return;
21+
meta.setAttribute('content',
22+
toolbarColorForTokens(newClasses.join(' ').split(/\s+/).filter(Boolean)));
23+
};
24+
725
// add corresponding classes when config change
826
const updateSkinVariantsClasses = (newClasses) => {
927
const domsToUpdate = [
@@ -21,6 +39,8 @@ const updateSkinVariantsClasses = (newClasses) => {
2139
domsToUpdate.forEach((el) => { el.removeClass('full-width-editor'); });
2240

2341
domsToUpdate.forEach((el) => { el.addClass(newClasses.join(' ')); });
42+
43+
updateThemeColorMeta(newClasses);
2444
};
2545

2646

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import {expect, test, Page} from '@playwright/test';
2+
import {goToNewPad} from '../helper/padHelper';
3+
4+
const themeColor = (page: Page) =>
5+
page.locator('meta[name="theme-color"]').getAttribute('content');
6+
7+
test.describe('light color scheme', () => {
8+
test.use({colorScheme: 'light'});
9+
10+
test('theme-color meta tracks the dark-mode toggle', async ({page}) => {
11+
await goToNewPad(page);
12+
// Server emits the light baseline derived from settings.skinVariants.
13+
expect(await themeColor(page)).toBe('#ffffff');
14+
15+
await page.locator('button[data-l10n-id="pad.toolbar.settings.title"]').click();
16+
await expect(page.locator('#theme-toggle-row')).toBeVisible();
17+
18+
// Colibris styles the native checkbox via a sibling label; click the label
19+
// so the toggle fires the real change event the production code listens on.
20+
await page.locator('label[for="options-darkmode"]').click();
21+
// pad.ts forces super-dark-toolbar (#485365) regardless of the configured
22+
// light skinVariants, so the meta must follow the client-applied class.
23+
await expect.poll(() => themeColor(page)).toBe('#485365');
24+
25+
await page.locator('label[for="options-darkmode"]').click();
26+
await expect.poll(() => themeColor(page)).toBe('#ffffff');
27+
});
28+
});
29+
30+
test.describe('dark color scheme', () => {
31+
test.use({colorScheme: 'dark'});
32+
33+
test('theme-color meta follows the auto dark-mode switch on dark-OS clients',
34+
async ({page}) => {
35+
await goToNewPad(page);
36+
// pad.ts auto-switches to super-dark-toolbar when enableDarkMode is on,
37+
// matchMedia(prefers-color-scheme:dark) matches, and no localStorage
38+
// white-mode override is set. The meta must follow the applied class —
39+
// this is the case stffen reported on issue #7606.
40+
await expect.poll(() => themeColor(page)).toBe('#485365');
41+
});
42+
});

0 commit comments

Comments
 (0)