Skip to content

Rework the theme selector ahead of the design refresh - #3259

Open
enf0rc3 wants to merge 7 commits into
mainfrom
wl/theme-selector-rework
Open

Rework the theme selector ahead of the design refresh#3259
enf0rc3 wants to merge 7 commits into
mainfrom
wl/theme-selector-rework

Conversation

@enf0rc3

@enf0rc3 enf0rc3 commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Rewrites the theme switcher so it is a solid base for the design refresh. A 1:1 port of the existing behaviour, with the bugs fixed and the moving parts in idiomatic Astro places.

Bugs fixed

  1. The two switchers desynced. Each instance cached isDarkMode at construction, so toggling the mobile one left the header one stale. Resize past 1130px and it showed the wrong position; clicking it then set data-theme to the value it already had.
  2. The knob animated on every page load. Slider position came from a JS-applied .dark-mode class, so in dark mode it painted on the light side and slid across after hydration. Now driven by html[data-theme], correct on the first frame.
  3. Keyboard users tabbed to a non-widget. tabindex="-1" on the input plus tabindex="0" on the label meant the focusable element was a bare <label> — no role, name or state exposed to a screen reader. The real input[role=switch] is focusable again, the deprecated keyCode handler is gone, and the focus ring is drawn via input:focus-visible + label.
  4. localStorage was unguarded. A throw in the head script (Safari private mode, cookie-blocked iframes) killed the IIFE and left the site permanently light regardless of OS preference.
  5. Enter did not toggle the switch. A native checkbox ignores Enter, so it is bound explicitly and routed through click(), keeping the change listener the only writer of state. Space and Enter are both tested.

What changed

File
src/lib/theme.ts new — attribute names, storage key and types in one place
src/scripts/theme-switcher.ts new — the controller, bundled by Astro
src/themes/octopus/components/ThemeScript.astro new — pre-paint inline script, values via define:vars from theme.ts
src/components/ThemeSwitcher.astro new — one switcher component, takes an id
tests/theme.spec.ts new — 9 tests
public/docs/js/modules/theme-switcher.js deleted
  • The controller moved from public/ into src/scripts/, so Astro bundles, minifies and dedupes it rather than shipping an unhashed public file loaded via is:inline.
  • The pre-paint script moved out of the HEADER_SCRIPTS string in config.ts into ThemeScript.astro, rendered first in <head> — ahead of the stylesheet links, so the parser is not waiting on a CSS download to run it. It stays is:inline deliberately; it must not be bundled or deferred.
  • Switcher markup deduplicated into ThemeSwitcher.astro, used by both Header.astro and MobileMenu.astro.
  • astro:after-swap handler so the switcher keeps working if <ClientRouter /> is ever added.
  • OS preference changes are followed until the user picks a side; other tabs sync via the storage event.
  • Removed the dead <body data-theme="dark">, and the DOMContentLoaded wrapper the old script did not need.
  • Accessible name is now "Dark mode" — "Toggle theme between light and dark mode, switch, on" read badly.

We tried adding color-scheme so native scrollbars would follow the theme, but our existing text-colour transitions made it look wrong, so it is not included.

Testing

tests/theme.spec.ts — 9 tests, all passing: OS preference followed in both schemes, explicit choice beating the OS preference and surviving navigation, both switchers staying in sync, Space and Enter each toggling both ways, fallback when localStorage throws, data-theme present with scripting disabled, and a regression guard that the knob position is attribute-driven rather than script-driven.

pnpm build passes (2697 pages).

I have also manually tested the above bugs and can confirm they are resolved, i found one trivial thing that was fixed being pressing enter on the toggle when tabbing through ui

🤖 Generated with Claude Code

@enf0rc3
enf0rc3 force-pushed the wl/theme-selector-rework branch from 84e1d85 to 8f97202 Compare July 28, 2026 04:41
@team-marketing-branch-protections

team-marketing-branch-protections commented Jul 28, 2026

Copy link
Copy Markdown

Pull request environment is available at https://stoctodocspr3259.z22.web.core.windows.net.

You can view the ephemeral environment status in Octopus Deploy.

This environment will be automatically deprovisioned when the pull request is closed, or after 7 days of inactivity.

@enf0rc3 enf0rc3 mentioned this pull request Jul 28, 2026
Merged
@enf0rc3
enf0rc3 marked this pull request as ready for review July 28, 2026 05:29
@enf0rc3
enf0rc3 force-pushed the wl/theme-selector-rework branch from 3d27ca5 to cd273a3 Compare July 28, 2026 20:35
enf0rc3
enf0rc3 commented Jul 28, 2026
View reviewed changes
Comment thread playwright.config.ts Outdated
borland
borland reviewed Jul 28, 2026
View reviewed changes
Comment thread playwright.config.ts Outdated
@borland

borland commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

I don't profess to understand the nitty-gritty detail of css and root selectors vs other things, but at a high level it all seems sound and reasonable.

I noticed though on the staging site, when switching between light and dark mode, the color changes on the header items now "pulse" in a way that they didn't before.

To experience this, go to this page: https://octopus.com/docs/infrastructure/accounts/openid-connect on the main site, and toggle between light and dark a few times quickly.

Then do the same on the staging site: https://stoctodocspr3259.z22.web.core.windows.net/docs/infrastructure/accounts/openid-connect and note how all the header text such as "OpenID Connect" animates differently.

While personally I think it's kind of weird looking, It isn't unacceptable -- we could choose to accept the new color changes. But if we can fix them instead, we should

@enf0rc3

enf0rc3 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for finding that bug @borland So it turns out it was the addition of using "color-scheme" because of pre-existing "transtions" that were defined on main.

Super weird to have transitions there, but it may be there for a reason, so i reverted the scroll bar back to has it was to not use color-scheme

@enf0rc3
enf0rc3 requested a review from borland July 28, 2026 23:26
enf0rc3 and others added 7 commits July 29, 2026 11:44
@enf0rc3 @claude
Rework the theme selector ahead of the design refresh
3bd6cae 
Move theme handling onto a single contract in src/lib/theme.ts, shared by the
pre-paint inline script, the client controller and the stylesheets.

<html> now always carries data-theme="light" or "dark" (never absent, never
"system"), so incoming design token stylesheets can key off the attribute with
no :root fallback. data-theme-preference records the raw choice, including
"system", ready for a three-position control.

Fixes along the way:

- The two switchers each cached their own state, so toggling one left the other
  stale. Both now read the same attribute.
- The knob position came from a JS-applied class, so it painted on the light
  side and slid across on every load. It is now driven by html[data-theme].
- tabindex="-1" on the input plus tabindex="0" on the label made the focusable
  element a bare <label> with no role, name or state. The input is focusable
  again and the deprecated keyCode handler is gone.
- A localStorage throw in the head script left the site permanently light.
- color-scheme is set per theme, so native controls and scrollbars follow.
- OS preference changes are followed until the user picks a side; other tabs
  stay in sync via the storage event.

The controller moves from public/ into src/ so Astro bundles, minifies and
dedupes it, and an astro:after-swap handler keeps it working if <ClientRouter />
is ever added. Adds tests/theme.spec.ts (7 tests) and PLAYWRIGHT_PORT so the
suite can run beside a dev server.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@enf0rc3 @claude
Restore Enter as a second activation key on the theme switch
22a19ca 
Moving to a native checkbox meant only Space toggled it - a checkbox ignores
Enter. The switch role treats Enter as an optional second activation key and
the control supported it before, so bind it explicitly on the input.

Tests now press each key twice to check it toggles both ways.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@enf0rc3 @claude
Apply review feedback on the theme switcher
7182d33 
- Route the Enter handler through box.click() so the change listener stays the
  only writer of theme state, instead of flipping box.checked separately.
- Scope the dark slider rule to .theme-switcher so a future .switch-slider
  elsewhere does not inherit it.
- Use auto-retrying toHaveAttribute assertions in the theme spec rather than
  one-shot getAttribute reads.
- Document that theme:change does not fire at boot, and that the
  src/pages/report/*.astro pages render their own <html> and sit outside the
  data-theme contract.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@enf0rc3 @claude
Reconcile the theme contract with the design token stylesheets
41525b7 
#3261 added the token CSS, which defines its custom properties only under
[data-theme='light'] and [data-theme='dark'] with nothing at :root, and added a
server-rendered data-theme="light" on <html> so the attribute is present even
without scripting. Both are kept: ThemeScript.astro resolves the real value
before first paint on top of that default.

- Point the HtmlHead token comment at ThemeScript.astro rather than
  HEADER_SCRIPTS, which no longer holds the theme script.
- Spell out in theme.ts why the attribute has to be unconditional.
- Add a javaScriptEnabled: false test so the server-rendered default cannot be
  dropped again without a failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@enf0rc3 @claude
Drop two explanatory comments from the theme CSS
402bafa 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@enf0rc3 @claude
Remove color-scheme, which broke the existing theme crossfade
281cdfe 
color-scheme was added by this branch as polish so native scrollbars would
follow the theme. It is not a normal paint property: it changes how colours are
resolved document-wide, and flipping it in the same style recalculation as the
themed colours makes Chromium restart every descendant `color` transition on
each frame.

The effect was two visible transitions instead of one. Headings crawled about
10% of the way over 300ms while body's transition was in flight, then ran their
real 300ms fade once body settled - roughly 640ms end to end.

The crossfade is deliberate: 23f3a01 added it with a
`prefers-reduced-motion: reduce` guard on every rule. Removing these two
declarations restores it. Measured on a real build, heading colour now completes
in one ramp at 291ms, matching the pre-branch timing.

Scrollbars go back to the browser default in dark mode, as before this branch.
`scrollbar-color` would theme them without disturbing colour resolution, and
measured clean, but that is a separate change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@enf0rc3 @claude
Revert playwright.config.ts to main's version
0892d9f 
The PLAYWRIGHT_PORT override was only there so I could run the suite while a dev
server held 3000. It is not needed by this change, and `webServer.port` alone
cannot move the server - `astro preview` takes its port from server.port in
astro.config.mjs, so it needed a --port flag as well, which is more explaining
than the convenience is worth.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@enf0rc3
enf0rc3 force-pushed the wl/theme-selector-rework branch from 5046d6f to 0892d9f Compare July 28, 2026 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@borland borland Awaiting requested review from borland

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@enf0rc3 @team-marketing-branch-protections @borland