Skip to content

fix(chrome): show monitor icon for system theme preference - #1482

Merged
rumblefrog merged 1 commit into
mainfrom
fix/theme-toggle-system-icon
May 27, 2026
Merged

rumblefrog merged 1 commit into
mainfrom
fix/theme-toggle-system-icon

Conversation

@rumblefrog

Copy link
Copy Markdown
Member

Summary

The topbar theme toggle's icon only swapped between sun (light) and moon (dark) — system mode showed whichever icon matched the OS preference, so operators following OS dark mode saw the moon and had no way to tell their preference was actually system, not pinned dark. The cycle is light → dark → system → light; two-thirds of the visual states were ambiguous and the next click was a coin flip.

  • Add a monitor Lucide placeholder; gate icon visibility on a new <html data-theme-pref="light|dark|system"> attribute (mirror of localStorage['sbpp-theme']).
  • All five anti-FOUC bootloader copies (core/header.tpl, page_kickit.tpl, page_blockit.tpl, page_uploadfile.tpl, updater.tpl) write the attribute synchronously before first paint so the right icon lands on frame 1 — no FOUC.
  • applyTheme() rewrites the toggle's aria-label per mode ("Color theme: system (auto). Click to switch to light.") — screen-reader users get the same indicator sighted users get from the icon swap.

The class-vs-attribute split is deliberate: html.dark stays the resolved theme (what's painted, what CSS tokens cascade against); data-theme-pref is the choice (what the toggle's icon reflects).

Deferred follow-up from #1185 (which shipped the two-state sun/moon swap and explicitly left the system indicator as a follow-up).

Test plan

Quality gates ran green locally:

  • `./sbpp.sh phpstan` — clean, level 5
  • `./sbpp.sh ts-check` — clean
  • `./sbpp.sh test --filter IframeChromeAntiFoucBootloader` — 4 tests / 8 assertions pass; new required-fragment locks the `setAttribute('data-theme-pref', m)` line into all five bootloader copies and the byte-equivalence-after-normalization assertion keeps them in lockstep
  • `./sbpp.sh e2e specs/flows/ui/theme-toggle.spec.ts --project=chromium` — 4 tests pass:
    • cycle test extended with `data-theme-pref` + `aria-label` assertions at each phase (light/dark/system)
    • new "icon reflects preference, not resolved theme" test uses `getComputedStyle(...).display` to assert exactly one of sun/moon/monitor is visible per preference (collect-then-assert, not first-match-wins — catches CSS regressions where multiple icons paint at once)

Adversarial-review fixes folded in

A reviewer pass caught three issues, all addressed before opening this PR:

  1. MINOR — `visibleIcon` was first-match-wins, which would silently pass a CSS regression that left multiple icons visible at once. Reshaped to `visibleIcons()` that returns the full set of visible slots; each arm asserts `toEqual([expected])`.
  2. MINOR — the new dynamic `aria-label` had no test coverage. Added a `THEME_LABELS` constant (mirror of theme.js's), an `expectAriaLabel` helper, and per-phase assertions in the cycle test.
  3. NIT — `requiredFragments()` docblock said "four" but the array now has five entries; fixed.

Pre-fix the topbar theme toggle's icon only swapped between sun
(light) and moon (dark), so operators in `system` mode saw whichever
of the two matched their OS preference and had no way to tell the
toggle was set to "follow OS" instead of pinned. The cycle is
light → dark → system → light, but two-thirds of the visual states
were ambiguous and the next click was a coin flip.

Add a `monitor` Lucide placeholder, gate icon visibility on a new
`<html data-theme-pref="light|dark|system">` attribute (mirror of
`localStorage['sbpp-theme']`), and write the attribute synchronously
from all five anti-FOUC bootloader copies so the right icon paints
on first frame. The class-vs-attribute split is deliberate:
`html.dark` stays the *resolved* theme (what's painted, what CSS
tokens cascade against); `data-theme-pref` is the *choice* (what
the toggle's icon reflects).

Bonus accessibility: `applyTheme()` now rewrites the toggle's
`aria-label` per mode so screen-reader users hear "Color theme:
system (auto). Click to switch to light." rather than the static
"Toggle color theme" they'd been stuck with.

This is the deferred follow-up from #1185 (which shipped the
two-state sun/moon swap and explicitly left "system" indicator as
a follow-up).
@rumblefrog
rumblefrog added this pull request to the merge queue May 27, 2026
Merged via the queue into main with commit 6126cc8 May 27, 2026
5 checks passed
@rumblefrog
rumblefrog deleted the fix/theme-toggle-system-icon branch May 27, 2026 23:14
@github-actions github-actions Bot locked and limited conversation to collaborators May 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant