fix(ui): raise design tokens to clear WCAG AA contrast#35
Merged
Conversation
Addresses the preexisting contrast violations surfaced by the axe-core suite so the a11y spec now passes with no exceptions: - --primary-color: #007AFF → #0062CC. Clears AA 4.5:1 for white text on .btn-primary (was 4.01:1 → now 5.79:1) in both color schemes. Also used as text color on --primary-light tinted backgrounds; darker blue reads better there too. - --primary-light / --primary-lighter: RGB updated to match the new base. - --text-secondary opacity 0.6 → 0.76 (light mode). Fixes sidebar nav inactive items, .section-description, empty-state copy — all dropped below AA on white surfaces. - --text-secondary #8E8E93 → #AEAEB2 (dark mode). Fixes sidebar nav on the translucent near-black sidebar bg. - .nav-item.active in dark mode: override to --text-primary (white). --primary-color #0062CC is too saturated to stay readable on the --primary-light tint over the dark sidebar. Mirrored the changes in popup.css so both extension surfaces share tokens. Empties KNOWN_CONTRAST_EXCEPTIONS in e2e/a11y.spec.ts — the suite now runs with no allowlist, so any future regression fails immediately.
Owner
Author
Code Review — Approved ✅SummaryThis PR raises design token values to satisfy WCAG AA contrast requirements and removes all entries from Review FindingsConsistency with PR description: The diff matches the described token changes exactly — Correctness: I independently verified the contrast ratios:
Standards & conventions:
Security: No concerns — CSS-only changes with no dynamic values or external references. Test coverage: The existing CI StatusAll checks passed:
VerdictClean, minimal, well-scoped change. The diff is small (22 additions / 34 deletions across 3 files), the rationale is solid, and the token approach means a single source-of-truth fix rather than scattered overrides. Merging. Generated by Claude Code |
helebest
added a commit
that referenced
this pull request
Apr 23, 2026
* docs: sync v1.6.1 references and formalize release checklist
About panel, README roadmap, docs/index.html JSON-LD softwareVersion,
STORE_LISTING Current Version block, SEO_GUIDE header, and the hardcoded
version assertion in e2e/options.spec.ts were all stuck at v1.6.0 (or
older) after the 1.6.1 cut — evidence that the release process had no
single source of truth for "where does the version string appear."
CONTRIBUTING.md rewritten to match the real project (plain JS + three-
component Vite build, no TS source, no React) and gains a three-tier
Release Checklist so future bumps can't silently miss JSON-LD schema,
store listing copy, or the e2e assertion that asserts the About text.
* test(e2e): tighten visual regression tolerance and refresh stale baselines
playwright.config.ts maxDiffPixelRatio was 0.05 — a ~46,000-pixel budget
on a 1280x720 frame. That band was wide enough to silently absorb the
entire v1.5.1 -> v1.6.1 text change in options-about.png, so the baseline
quietly stayed three versions out of date while CI kept passing. Drop
the global threshold to 0.01 and remove the per-test 0.02 overrides in
popup-visual.spec.ts so there is a single rule.
Note: content drift below ~1% of pixels (single-character diffs like the
version number itself) is still invisible to toHaveScreenshot even at
0.01. That class of drift is caught by text assertions such as
options.spec.ts's toContainText('X-Proxy v1.6.1'), not by pixel diff.
Regenerate all nine baselines via --update-snapshots=all so screenshots
reflect the shipped UI, and add a discoverable `npm run test:e2e:update`
script so contributors stop having to remember the raw playwright flag.
* fix(test): eliminate CSS-transition race in axe color-contrast scans
The dark-mode axe scans in a11y.spec.ts were failing spuriously because
page.emulateMedia({colorScheme:'dark'}) flips :root custom properties
synchronously, but elements carrying `transition: all var(--transition-
base)` animate their resolved color between the old and new var() values
over 250ms. Axe, injected immediately after the media flip, samples the
mid-transition rgba value — empirically rgba(120,120,125,0.87) at 100ms
and rgba(170,170,174,0.99) at 200ms — and reports it as insufficient
contrast against the (already-final) dark background.
Fix: disable CSS transitions and animations via addStyleTag BEFORE
emulateMedia fires, so the color change is instantaneous instead of
interpolated. playwright.config.ts's toHaveScreenshot.animations:
'disabled' only applies to screenshots; axe runs page JS and needs
CSS-level suppression.
Ordering note documented in-file: disableTransitions must precede
emulateMedia, otherwise transitions are already armed at the moment of
the flip and !important on transition-property can't unwind them.
Three pre-existing "failures" cleared by this fix:
- li[data-section=about] > span inactive nav label (reported 2.35)
- #importProfilesBtn .btn-secondary (reported 3.67)
- #exportProfilesBtn .btn-secondary (reported 3.67)
All three are in fact above AA in the shipped CSS; they just weren't
observable through the racing transition.
* test(e2e): add modal and dark-mode visual baselines (Phase 2)
Previously only the popup had visual regression coverage for dark mode
and nothing guarded the Add/Edit Profile modal in either theme. A revert
of #34 / #35's contrast fixes, or any drift in iOS design tokens
(--border-radius, --text-secondary, etc.), would ship without any test
failing. Six baselines close the gap:
modal-add-light / modal-add-dark — Add Profile form
modal-edit-light / modal-edit-dark — Edit form with a seeded profile
options-profiles-dark — Profiles page dark (was uncovered)
options-about-dark — About page dark
Reuses the disableTransitions() helper approach from a11y.spec.ts —
addStyleTag must precede emulateMedia, otherwise CSS transitions are
already armed at the moment of the media flip and screenshots capture
mid-interpolation rgba values. Same race as the axe scans.
Seeded profile uses the v2 storage shape (lib/storage-migration.js) so
no migration step runs mid-test.
* docs(contributing): add visual baseline refresh + caveats to Release Checklist
Phase 1 and Phase 2 uncovered three things that were not institutionalized:
1. After any CSS / HTML / design-token change, baselines under
e2e/__screenshots__/ must be regenerated via npm run test:e2e:update
and the PNG diffs reviewed in the PR. Tier 2 now calls this out as
its top bullet — explicit review, not just file-count confirmation.
2. The About-section baseline in visual.spec.ts is effectively a second
version string and must be regenerated whenever the version bumps.
Added it to the Tier 1 table so it can't be missed.
3. Gaps that the automation does NOT catch are now documented so
reviewers don't over-trust a green check:
- single-character text drift below the 1% threshold (content
assertions handle this, not pixel diff)
- CSS transition mid-flight values fooling axe after emulateMedia
- worker-parallelism flakes vs real test failures
Sanity-check section also gains the build + full-e2e + serial-e2e
commands that Phase 1 / Phase 2 executions leaned on.
* test(e2e): add storage migration integration guard (Phase 4a)
tests/mode-migration.test.js already covers every branch of migrateData()
as a pure function. This spec catches a different regression class:
someone disconnecting migrateData() from the actual read or write path.
Unit tests would not detect that.
Three integration invariants pinned down:
1. Read path is non-destructive — reading v1-shaped storage through the
popup must not silently rewrite it to v2. If someone adds an auto-
persist "convenience" to readData(), this fails.
2. Write path upgrades to v2 — first user action (click System) after
the popup loads v1 data must persist back in v2 shape with the
correct `mode` field. If someone forgets version: SCHEMA_VERSION in
writeData() or bypasses migrateData(), this fails.
3. Direct mode is preserved — same as #2 but targets the Direct button,
since v2's raison d'être was to distinguish Direct from System. A
regression that collapsed them would slip past test #2.
Scope note: UI-level "seed v1 → popup shows migrated mode" assertions
would be complementary but require forcing the background service
worker to re-initialize against seeded data. chrome.runtime.reload()
inside a persistent Playwright context leaves the extension in a
transient ERR_BLOCKED_BY_CLIENT state that does not reliably clear.
Deferred — unit coverage already exhaustive, and write-path tests here
detect real regressions.
TDD red-check performed during development: flipping the version
assertion from 2 to 1 in test #2 produces "Expected: 1, Received: 2",
confirming the test actually exercises the migration write path rather
than reading back its own seeded data.
* feat(a11y): keyboard navigation for profile modal (Phase 4b)
Pre-v1.6.1 options.js shipped with no keyboard handling on #profileModal:
no Escape-to-close, no focus return on close, no auto-focus on open.
The only exit was Tab-hunting for Cancel or the X button, which fails
WCAG 2.1.2 ("No Keyboard Trap") for keyboard-only users.
This lands three behaviors driven red-green by e2e/keyboard-nav.spec.ts:
1. Escape closes the modal when .show is present. Document-level
keydown listener, guarded by the class so it's a no-op otherwise.
2. Focus is saved at showProfileModal() (document.activeElement before
open) and restored at hideProfileModal() so Add/Edit Profile
buttons regain focus on close. Defensive: skips restore if the
saved node was removed from the DOM mid-session.
3. On open, #profileName receives focus synchronously so keyboard
users can type immediately instead of being stranded behind the
modal.
TDD flow: four tests in keyboard-nav.spec.ts failed against the prior
codebase (no handlers), then all four passed after the three changes
above. Unit + full serial E2E (96 tests) stay green — no regressions
in existing modal-open/close paths.
Scope note: focus trap (Tab cycling within the modal, blocking Tab
from escaping to background content) is deferred. The current Cancel
/Save button pair is the expected Tab-end and browsers already return
to page top after Tab past the last focusable element, which is a
survivable degradation for keyboard users given Escape always works.
* feat(a11y): trap Tab focus inside profile modal (Phase 4b+)
Phase 4b (ba16edc) made the profile modal keyboard-operable: Escape
closes it, focus saved on open and restored on close, first field
auto-focused. But Tab from the Save button still bled focus into the
background page (footer Save All, sidebar nav), so a keyboard user
would see the modal visually open while their keyboard was driving
the content behind it. WAI-ARIA's dialog pattern requires focus be
contained.
This change adds a trap:
- Tab on the last focusable inside the modal wraps to the first.
- Shift+Tab on the first focusable wraps to the last.
- "Focusable" enumerates buttons / inputs / selects / textareas /
tabindex≠-1, excluding disabled and display:none nodes — the last
point matters because #pacDetails, #proxyDetails, #routingRulesPanel
and auth inputs toggle visibility based on proxy type.
Extracted getModalFocusables(modal) as an instance method so future
modals (if any are added) can reuse the same visibility-aware
enumeration. No shared lib/focus-trap.js yet — one caller doesn't
justify that indirection.
TDD: two new tests in keyboard-nav.spec.ts failed against Phase 4b
(focus escaped to page elements outside #profileModal), pass after
this change. Full regression: 91 unit + 98 E2E green, no changes to
prior Escape / focus-return / auto-focus behavior.
* docs(changelog): populate [Unreleased] with the E2E-coverage overhaul
[Unreleased] had placeholder "Future improvements planned" text rather
than actual unreleased work. Replaced with concrete Added / Changed /
Fixed entries covering:
- Keyboard navigation for the profile modal (Escape, focus return,
auto-focus, Tab trap).
- Three new E2E spec files (modal-visual, migration, keyboard-nav)
and the test:e2e:update convenience script.
- Visual regression threshold tightened from 5% to 1%.
- CONTRIBUTING.md rewritten with a three-tier Release Checklist.
- CSS-transition race in axe color-contrast scans eliminated.
- Stale v1.6.0 references across options.html, README, docs/*,
e2e/options.spec.ts synced to v1.6.1.
"Future improvements planned" preserved under a labeled subsection so
the aspirational list is kept but no longer conflated with shipped work.
* test(e2e): fix CI Linux baseline drift + address code review feedback
Three follow-up fixes from PR #39 review:
1. Linux CI visual baseline for options-about-dark.png was failing at
9636 px (~2.08%) against a 1% global threshold. Cause is font-hinting
divergence between macOS (baseline authoring) and Xvfb+Linux (CI)
amplified by the About page being the text-heaviest screen in the
UI — version line, description paragraph, nine-item feature list,
link row.
Narrowly scoped per-test override `maxDiffPixelRatio: 0.03` on that
single screenshot, with an inline justification comment. 0.03 still
leaves real token / layout regressions detectable (off-by-color-
button is tens of thousands of pixels, an order of magnitude above
this floor). No other baseline approached 1% on CI, so no other
override needed.
2. disableTransitions() helper was duplicated verbatim in a11y.spec.ts
and modal-visual.spec.ts. Extracted to e2e/helpers.ts with a single
authoritative JSDoc documenting the "run before emulateMedia"
ordering constraint. Both specs now import it.
3. migration.spec.ts header said "Why only two tests here" but the
file contains three. Rewrote the scope note to enumerate all three
invariants the tests pin down (non-destructive read, System-click
upgrade, Direct-click upgrade) and dropped the misleading count.
Local regression (serial, --workers=1): 98 E2E + 91 unit, green.
---------
Co-authored-by: hele.hl <hele.hl@bytedance.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #34, which merged the dim-input fix and the
e2e/a11y.spec.tsaxe-core suite but not the token-raising commit. Ships that commit on its own and emptiesKNOWN_CONTRAST_EXCEPTIONSso the a11y suite runs with zero allowlist — every future contrast regression fails in CI the moment it's introduced.Token changes
Mirrored in
options.cssandpopup.cssso both surfaces share the same tokens.--primary-color#007AFF#0062CC.btn-primarywas 4.01:1; now 5.79:1--primary-light/--primary-lighterrgba(0, 122, 255, …)rgba(0, 98, 204, …)--text-secondary(light)rgba(60, 60, 67, 0.6)rgba(60, 60, 67, 0.76).section-description/ empty-state copy were 3.28–3.69:1--text-secondary(dark)#8E8E93#AEAEB2.nav-item.active(dark only)color: var(--primary-color)color: var(--text-primary)#0062CCtext on--primary-lighttint over dark sidebar was 2.79:1; iOS convention here is white text + tinted bgWhy one PR for all of them
These violations share a root cause (two design tokens) so splitting them would mean either (a) multiple PRs each touching the same tokens, or (b) landing half the fix and keeping most of the
KNOWN_CONTRAST_EXCEPTIONSlist — which defeats the point of the axe suite. The total diff is 22 lines added / 34 removed across three files (the deletions are the now-unneeded allowlist entries).Test plan
npm test— 83 unit tests passnpm run test:e2e— all 73 e2e tests pass (a11y suite now runs with 0 allowlist entries)npm run type-check— cleannpm run build— bundles updated🤖 Generated with Claude Code