Skip to content

Paint the chosen operator with a colour the theme declares - #39

Merged
ww-mw merged 1 commit into
mainfrom
fix-column-filter-tokens
Oct 3, 2026
Merged

ww-mw merged 1 commit into
mainfrom
fix-column-filter-tokens

Conversation

@ww-mw

@ww-mw ww-mw commented Oct 3, 2026

Copy link
Copy Markdown
Member

The filter popup a column header opens read two CSS custom properties that
are declared nowhere — not in src/webview/components/styles/global.css, not
in src/webview/vscode-theme.css, not locally in the component. That is the
bug class this repo already knows: an undeclared property is not a missing
colour, it is a silent one. Every var() takes the literal written beside
it as a fallback, nothing warns at build or at runtime, and the literal is a
light-theme value by construction. The Variable Editor had a whole family of
these (v1.32.0); test/variableEditorTheme.test.ts is what stopped them
coming back there.

What it cost

.op-button[aria-pressed='true'] — src/webview/components/dex-column-filter.ts:81

The pressed operator button is the popup's entire state display: it is the one
element that says which comparison Apply is about to write. It asked for
--dex-bg-selected and therefore painted #cce4f7 in all four themes, under
whatever ink the theme gives it.

Measured in Chromium over the shipped dist/webview bundle with the real
--vscode-* palette, popup open, pressed button against its own text:

theme before after
Dark Modern 1.22 7.61
Light Modern 8.54 9.14
HC Black 1.31 10.41
HC White 11.09 12.33

Two of the four failed AA, and the worse of them sits below the 2.13:1 that
#26 was filed over. All four pass now.

.writes-value — dex-column-filter.ts:114

--dex-font-family-mono is a font, not a colour, so it cost legibility rather
than contrast: the hard-coded stack beside it always won, which made the one
line in the popup that shows literal syntax to retype the one line that
ignored the font the user types code in.

Why this is not a colour decision

The repo already declares the token for a selected surface:
--dex-color-accent-bg, which vscode-theme.css aliases onto
--dex-selection-bg and test/vscodeThemeTokens.test.ts:106 pins there. Its
light value is #cde4f7 — the invented name's fallback was that same value
with one hex digit changed. So the fix is to spell the existing token
correctly, not to choose a new colour; same for --dex-font-mono, which
carries the user's editor.fontFamily.

Qualification

Phase-3 browser run, before and after, four themes each, zero failed checks in
either direction (every contrast() call returning a value, no console errors,
no failed requests). Rows come from hostRows() — a real .sldd through the
real buildRows().

Controls that must not move, and did not: the unpressed operator buttons, the
popup title, the writes: line, and both chip kinds in the filter bar all
measure identically before and after. Forced colors is 21:1 in both
directions — it was never affected, because the existing
@media (forced-colors: active) block replaces both backgrounds and marks the
pressed button with a Highlight outline. That block is untouched.

One trap worth recording: the first version of the scenario measured
forced-colors and the plain chip after applying the filter, which closes the
popup — both came back {error: "no element matching …"} in all four themes
of both runs, which reads exactly like a clean check in a summary table. The
measurements above are from the corrected run.

The test

test/columnFilterTokens.test.ts is new; no existing test is modified.
It sweeps every var(--dex-*) this component reads and requires each to be
declared in global.css, vscode-theme.css, or the component itself — so the
next invented name fails on the name, in the unit suite, rather than in a
theme nobody ran. Two further assertions name the two sites directly, so the
reason survives a refactor of the sweep.

Two files are knowingly outside the sweep's file list and are named in its
header with what each reads and what it measured
(dex-filter-bar.ts's --dex-bg-badge, dex-tree-table.ts's
--dex-border-color-ultralight). The rule written there is to add a file once
it is clean, not to add an exemption to keep one in. Both are on the agenda:
the badge is benign (8.16–17.92:1, a neutral grey at 18% alpha), and the
ultralight border turns out to sit in CSS that cannot be reached, which is a
question about whether table-style should exist rather than a colour bug.

npm run verify green: 163 files, 3106 tests, leak check OK.

The column-filter popup read two custom properties that are declared
nowhere — not in global.css, not in vscode-theme.css, not locally. An
undeclared property is not a missing colour, it is a silent one: every
var() falls back to the literal written beside it, nothing warns, and the
literal is a light-theme value by construction.

The one that matters is the pressed operator button, which is the popup's
whole state display — the only thing saying which comparison Apply is
about to write. It asked for --dex-bg-selected and so painted #cce4f7 in
every theme, under the near-white ink a dark theme gives it. Measured in
a real browser over the shipped dist/webview bundle, all four themes plus
forced-colors:

                  before   after
  Dark Modern       1.22     7.61
  Light Modern      8.54     9.14
  HC Black          1.31    10.41
  HC White         11.09    12.33

Two of those failed AA and the worse of them is below the 2.13:1 that
issue #26 was filed over; all four pass now. It is a one-word fix,
because the repo already declares the token for a selected surface:
--dex-color-accent-bg, aliased onto --dex-selection-bg by
vscode-theme.css and pinned there by vscodeThemeTokens.test.ts. Its
light value is #cde4f7 — the invented name's fallback was that value with
one digit changed, which is the evidence the name was a slip and not a
design choice, and why this is not a decision about what colour the
button should be.

The second, --dex-font-family-mono, is a font rather than a colour, so it
cost legibility rather than contrast: the one line in the popup that
shows literal syntax to retype was the one line that ignored the font the
user types code in. The declared name is --dex-font-mono, which carries
editor.fontFamily.

Forced colors was never affected and still is not (21:1 in both
directions) — the pressed button's outline there comes from a
@media (forced-colors: active) block that overrides both backgrounds.

columnFilterTokens.test.ts now sweeps every var(--dex-*) this component
reads and demands that each one be declared somewhere, so the next
invented name fails on the name rather than in a theme. Two files are
knowingly outside its file list, named in its header with what each
reads; the rule there is to add a file once it is clean, not to add an
exemption to keep one in.
@ww-mw
ww-mw merged commit 842252a into main Oct 3, 2026
4 checks passed
@ww-mw
ww-mw deleted the fix-column-filter-tokens branch October 3, 2026 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant