fix(theme): declare --mt-font-size on body so it can see --vscode-font-size - #478
fix(theme): declare --mt-font-size on body so it can see --vscode-font-size#478dormouse-bot wants to merge 3 commits into
Conversation
…t-size
CSS resolves var() inside a custom-property declaration at the element
where that property is declared. --mt-font-size was declared only on
:root, so it could not see the --vscode-font-size that applyTheme()
writes to document.body.style. It resolved to nothing, making
`body { font-size: var(--mt-font-size) }` invalid at computed-value
time, and body fell back to the UA default 16px instead of 13px in every
host where applyTheme() is the sole writer: standalone, website, Pocket.
VS Code was unaffected because that host supplies --vscode-font-size at
the document level itself.
Mirror the token onto the body block next to --mt-font-family, and add a
test that pins every --vscode-*-bound token declared above the body block
to a matching body-level declaration, so the next one added cannot go
missing the same way.
Closes #474
Deploying mouseterm with
|
| Latest commit: |
25cd1a5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://732ad52e.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-issue-474.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
Traced the mechanism independently and it holds: applyTheme() writes --vscode-font-size: 13px to document.body.style (HOST_TYPOGRAPHY_VARS in apply.ts), the :root copy of --mt-font-size resolves against the document element where that variable doesn't exist, so body { font-size: var(--mt-font-size) } in index.css was invalid-at-computed-value-time and body inherited the UA 16px. Mirroring onto body is the right fix and the new guard is load-bearing — running its own logic over theme.css reports ['--mt-font-size'] with the added line removed and [] with it, across 34 document-level declarations.
Worth recording for the visual review, since it bounds what the Chromatic diffs should show: the terminal is not in scope. createXtermHost() reads --vscode-editor-font-size off getComputedStyle(document.body) rather than through --mt-font-size, and the --text-* scale is rem-based (anchored on html, which this doesn't touch), so every element carrying an explicit text-* class holds still. Only unsized text nodes move 16px to 13px. A diff on terminal glyphs or on a text-xs/text-sm element would mean something else changed.
Two suggestions inline, plus one observation that is a judgment call rather than a defect: with --mt-font-size and --mt-font-family now on body, the :root block that declares both is unreachable — nothing renders outside <body>, and in VS Code the body declaration resolves fine because body inherits the host's document-level --vscode-font-size. Deleting that block would make this class of bug impossible for the two --mt-* tokens instead of policing it with a regex over CSS. The @theme duplication genuinely has to stay (Tailwind generates utilities from it); the :root duplication does not.
Not approving — this is a self-authored PR, so GitHub rejects it either way, and the 13px intent that #474 deferred is still a maintainer call. Every automated check on ed9e79a is green; UI Tests (Chromatic) is the human-gated one and is the artifact that decides it.
… the spec Applies both inline suggestions from the review on ed9e79a. The guard now matches any `var()` chain rather than a literal `var(--vscode-`, so indirect bindings like `--color-door-bg: var(--color-header-inactive-bg)` are covered too, and it compares the two declarations' values instead of just checking presence, so repointing one level's binding while the other goes stale fails as well. Still green against the current theme.css, and still load-bearing: removing the `--mt-font-size` line from the body block reports `['--mt-font-size']`. The spec paragraph is rewrapped to the file's ~76-column width (it shipped as one 819-character line) and no longer implies the `:root` copies feed Tailwind utility generation — Tailwind v4 reads `@theme` only.
|
Both inline suggestions are applied in The premise checks out. What changes the calculus is that the guard this PR adds now covers Every automated check was green on |
dormouse-bot
left a comment
There was a problem hiding this comment.
The widened guard holds up under mutation. Running its logic over the current theme.css: green as-is (34 document-level declarations, 27 body-level); ['--mt-font-size'] with the new body line removed; ['--mt-font-size'] again when only the :root binding is repointed to --vscode-editor-font-size — so the value comparison is load-bearing, not just the presence check; and ['--color-new-thing'] for a hypothetical @theme token bound indirectly via var(--color-door-bg), which the old var(--vscode- literal would have missed. The new predicate is a strict superset of the old one, so nothing the previous version caught is now let through. pnpm lint:specs passes (24 specs, 25 files).
Two things on the increment, both about the prose rather than the guard.
The spec and the theme.css comment now describe a narrower rule than the test enforces. Both say a token must be mirrored when its value reads a --vscode-* variable; the test requires mirroring for any var() chain, and requires the two declarations to be equal, not merely both present. That last mutation above is the concrete gap — a token bound to var(--color-door-bg) reads no --vscode-* variable at all, so someone following either doc would add it to @theme only and hit a test failure describing a rule neither doc states. The test's own header comment already got the wording right; suggestions inline to bring the other two in line, since AGENTS.md puts spec accuracy first ("The primary job of a spec is to be an accurate reference for the current state of the code").
#476 rewrites the same docs/specs/theme.md paragraph, and its version drops this invariant. #476 ("audit all 24 specs against the code") replaces the same theme.css declares the theme-dependent... paragraph with a condensed form that mentions neither --mt-font-size nor the both-levels requirement at all — it collapses to "the parallel @theme declarations exist so Tailwind can generate utility classes, but the body-level ones are the runtime source of truth." So whichever lands second conflicts here, and if the resolution takes #476's wording the spec silently loses the rule consumed-keys.test.ts is now enforcing. The two PRs don't collide anywhere else — #476's lib/src/theme.css hunks are all comment rewrites inside @theme, well above the body block this PR touches. Worth noting on whichever merges first so the resolution is deliberate.
Not approving — self-authored, so GitHub rejects it either way, and the 13px intent #474 deferred is still a maintainer call. Every automated check on 8510168 is green (Build & Test, Standalone Smoketest, Standalone Platform Check on macOS + Windows, Webview Boot Smoketest, Visual Regression Tests, Storybook Publish, Cloudflare Pages); UI Tests (Chromatic) is the human-gated one and is the artifact that decides the visual question.
The spec paragraph and the theme.css body-block comment both said a token must be mirrored onto body when its value reads a --vscode-* variable. consumed-keys.test.ts is wider than that: it flags any var() chain, so an indirect binding like --color-door-bg: var(--color-header-inactive-bg) must be mirrored too, and it compares the two declarations' values, so repointing one level without the other fails even though both exist. Both docs now say that. No behavior change.
|
Both inline suggestions are applied in On the #476 collision: it's real, and it's the direction where the invariant can be lost silently. #476 rewrites the same paragraph from a base that predates this PR, so its version has neither the
|
|
Correction to my last paragraph: |
--mt-font-sizewas declared only on:root, where it cannot see the--vscode-font-sizethatapplyTheme()writes todocument.body.style— sobody { font-size: var(--mt-font-size) }was invalid at computed-value time and body fell back to the UA default 16px instead of 13px in standalone, website, and Pocket. This mirrors the token onto thebodyblock next to--mt-font-family, and adds a test that pins everyvar()-bound token declared above the body block to a matching body-level declaration.Verified by the reproduction test: it failed with
expected [ '--mt-font-size' ] to deeply equal []before the one-line CSS change and passes after. The wholelib/src/lib/themes/suite (8 files, 60 tests) is green, andspec-lintpasses.This changes rendered type size in three hosts — every unsized text node inherits 13px where it rendered at 16px before — which is exactly the visual review #474 asked for. The Chromatic diffs on this PR are the artifact for confirming the 13px intent still holds. The terminal is not in scope:
createXtermHost()reads--vscode-editor-font-sizeoffgetComputedStyle(document.body)rather than through--mt-font-size, and the--text-*scale isrem-based offhtml, so anything carrying an explicittext-*class holds still.Root cause, the guard, and a note on #476
CSS resolves
var()inside a custom-property declaration at the element where that property is declared.theme.cssalready knows this — its body block carries a comment spelling out the rule, and re-declares--mt-font-familythere for exactly this reason.--mt-font-sizewas the one binding that block missed. VS Code was unaffected because that host supplies--vscode-font-sizeat the document level itself, so the:rootdeclaration resolves normally there.The new check in
consumed-keys.test.tsparsestheme.css, collects every custom property declared in@themeor:rootwhose value reads anyvar()chain, and asserts each one is re-declared onbodywith the same value. Matching any chain rather than a literalvar(--vscode-covers the indirect bindings (--color-door-bg: var(--color-header-inactive-bg), itself--vscode--bound and broken the same way at document level); comparing values rather than presence catches a binding repointed at one level while the other goes stale, which matters because the spec namesbodythe runtime source of truth.--mt-font-sizewas the only violation across 34 document-level declarations, and the widened predicate is still green on everything else.Conflict warning: #476 rewrites the same
docs/specs/theme.mdparagraph this PR extends (comments-only intheme.css, so the CSS itself won't conflict). Whichever lands second will need the spec paragraph merged by hand — the substantive addition here is the sentence stating that every--vscode-*-reading token must appear at both levels, plus the pointer to the test.Closes #474 — automated triage