fix(design-system): let a mode live in the value, not in a selector - #4603
Conversation
Astryx inverts a surface by setting `color-scheme: dark` on it — MediaTheme, which wraps every toast body and every overlay scrim — and its on-media token overrides move only text/icon primary and accent, because everything else is expected to follow `light-dark()` on its own. Maka's palette encodes its modes in a `.dark` selector on <html>, which nothing under an inverted surface can follow. Once #4509 pointed Astryx's ink at the product tiers, that gap became visible: `<Text type="supporting">` in an error toast painted the light-mode grey on #AA071E at 1.04:1. The hairlines and `--color-overlay-hover` were wrong on the same surfaces for the same reason — a near-black 4% hover wash on a dark plate. Every colour that differs by mode now carries both sides in its own value, as `light-dark(light, dark)`, declared once. The `.dark` block is down to two alphas; `--ring-soft` keeps one recipe with the pair inside it; the ten palette catalogues lose their second block each, and the default palette's dark block turns out to have been re-declaring nine derivations byte-for-byte. Nothing needed new plumbing: theme.ts, cached-theme-bootstrap.ts and the Storybook preview already set `color-scheme` beside the class, before the first paint and on every change. Values are unchanged — 134 tokens across 11 palettes and both modes compare identical before and after, the only delta being dark `--ring-soft` gaining light's two transparent placeholder layers. The toast's ink is a second decision, not a consequence of the first. Converted, its description lands at 2.99:1 — which is where Astryx's own secondary sits on that plate too, and still under AA. DESIGN.md §3's Tinted Surface Rule already says text on a tint is one tier; a solid inverted surface is that rule at full tint, so makaTheme.ts's onDark/onLight point text and icon secondary at the same on-color as primary. 1.04:1 → 7.61:1, and the rule gains no exception. ink-ladder-contract gains both halves: no token makaTheme aliases — nor anything those values transitively read — may be declared under `.dark`, and the generated media blocks must carry the collapsed tier. The set is derived from makaTheme.ts rather than listed, so the next alias is covered when it is written. Two mode switches went through the class alone. The e2e link-colour spec now sets both halves and waits for the mode to reach the content — Astryx's <Theme> re-declares color-scheme on its own wrapper, so the app's subtree turns over one React commit after the root; the switch is now whole rather than half a frame mixed. The palette-matrix story needs neither: a palette block carries both modes and resolves against the root, so its `useIsDark` observer is gone. Generated-by: Claude Code
115aa63 to
eee8278
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
I reviewed exact head eee82787b6ecb58ff41e7051a9b1b65086a93ac0. One P1, two P3s. Not approving — the P1 is introduced here and lands on an ordinary Windows path.
The diagnosis and the mechanism are right. Encoding the mode in a .dark selector on <html> genuinely cannot follow a subtree that inverts itself with color-scheme, and moving the mode into the value is the upstream fix rather than a patch list. That part verifies: across 134 tokens × 11 palettes × 2 modes (2,948 readings) nothing moves except the documented --ring-soft placeholder layers, and re-running those tokens inside a nested context whose color-scheme is the opposite of the root's resolves every one of them to the correct side — 0 mismatches. Both new contract assertions were reverse-mutated and each fails as intended. The Storybook smoke listed under "Not run" was run: 254 stories, 259 renders, all pass.
P1 — the Windows title-bar overlay samples --background and now gets pure black
The overlay colour is sampled rather than hardcoded: apps/desktop/src/renderer/theme.ts:212-220 reads the --background custom property and theme.ts:257-271 resolves it by assigning it to a detached canvas's fillStyle; the result goes to setTitleBarOverlay() at apps/desktop/src/main/main-window.ts:639-643.
maka-tokens.css:82 changes --background from oklch(1.000 0 0) to light-dark(oklch(1.000 0 0), oklch(0.205 0.004 286)), and every palette gets the same treatment. Reading a custom property returns its declared token stream, not a resolved colour, so what reaches the canvas is the literal light-dark(...) — and canvas 2D has no color-scheme to resolve it against, so the assignment is ignored and fillStyle stays at its default black.
Both guards fail open. CSS.supports('color', raw) is true, because light-dark() is a valid colour function — so the light/dark fallback pair never fires. Then alpha !== 255 passes, because opaque black has alpha 255. Black is returned as if it were a real sample.
Reproduced on Electron 43.4.1: the root reads back light-dark(oklch(1.000 0 0), oklch(0.205 0.004 286)), CSS.supports returns true, and the canvas yields [0,0,0,255]. The same token resolves correctly the moment it is used as an element's background-color.
Every Windows light-mode launch, mode switch and palette switch therefore sets the native overlay to black while the control symbols stay #1c1d21 — close, minimise and maximise land near 1.2:1 and are effectively invisible. That is the seam this sampling code exists to prevent.
Minimal fix: resolve through a document-attached probe — set background-color: var(--background) on it and read the computed backgroundColor — then convert. apps/desktop/e2e/link-color-contract.spec.ts:47-51 already reads values in their consumed state this way. Don't hand a custom property's declared value to fillStyle.
Worth noting for its own sake: this is the blind spot in the 2,948-reading equivalence pass. Comparing CSS use values proves colours and shadows don't drift; it cannot see a JavaScript consumer that reads the raw custom-property string. Any token this PR converts and JavaScript reads directly deserves the same check.
P3 — theme.ts:195-196 still says palettes switch on .dark
The comment describes the old mechanism; the switch is now color-scheme. The production setter writes both together, so nothing is broken today — but it will mislead the next caller that flips the class alone.
P3 — the ink-tier collapse is applied everywhere and evidenced on one plate
To be clear, the collapse itself is sound: --muted-foreground on #AA071E is 2.99:1 even once it follows the right mode, that is under AA, and DESIGN.md's Two-Tier Reading Rule rules out inventing a third grey to sit between them. No argument with the outcome.
The record is what I'd change. The generated media blocks point text and icon secondary at the on-colour for any element carrying data-astryx-media — every MediaTheme surface, not only the error toast — while the contrast evidence covers that one plate. DESIGN.md justifies it by analogy, "a solid inverted surface is the same rule with the tint turned all the way up," but a 0.24 tint over the page background and a surface that has flipped its whole colour scheme are mechanically different situations, and the analogy is doing load-bearing work.
The stronger justification is structural and goes unstated: --color-on-dark is a single flat #ffffff serving every inverted surface, so a per-plate secondary is not expressible with the tokens that exist. That is a constraint, not an analogy, and it is what actually forces one tier. Since DESIGN.md is the authority the next change will cite, a rule resting on "full tint" invites someone to apply the same reasoning to an actual tinted surface, where it does not hold.
label and test are both green on this head. The block is the P1 alone, not a failing check.
简体中文
我审的是 eee82787b6ecb58ff41e7051a9b1b65086a93ac0：一条 P1、两条 P3。不批——那条 P1 是本次引入的，而且落在 Windows 的普通用户路径上。
诊断和机制方向都对。把明暗模式写在 <html> 的 .dark 选择器上，确实追不上一个用 color-scheme 自我反转的子树；把模式搬进值里是上游机制，不是打补丁。这部分我核过：134 个 token × 11 个调色板 × 2 个模式共 2,948 次读数，除了作者已说明的 --ring-soft 占位层之外没有任何漂移；把这些 token 放进一个 color-scheme 与根节点相反的嵌套上下文里重测，全部解析到正确的一边，0 处不匹配。两条新增契约各自做了反向变异，都如预期失败。描述里列为「未跑」的 Storybook 冒烟其实跑了：254 个 story、259 次渲染，全过。
P1：Windows 标题栏配色是采样出来的，现在采到纯黑。 theme.ts:212-220 读 --background 这个自定义属性，theme.ts:257-271 把它丢给一个离屏 canvas 的 fillStyle 去解析，结果送到 main-window.ts:639-643 的 setTitleBarOverlay()。而 maka-tokens.css:82 把 --background 从具体颜色改成了 light-dark(...)。读自定义属性拿到的是那串函数文本本身，不是算好的颜色，canvas 又没有 color-scheme 可以拿来解析它，赋值被忽略，fillStyle 停在默认的黑。
两道保险同时失效：CSS.supports('color', raw) 返回 true（light-dark() 是合法颜色函数），所以不走回退；alpha !== 255 也过了，因为不透明黑的 alpha 正好是 255。于是纯黑被当作真实采样结果返回。已在 Electron 43.4.1 上复现。
后果：Windows 上每次浅色启动、切模式、切调色板，都会把原生标题栏 overlay 设成黑色，而控件符号色仍是 #1c1d21，对比度约 1.2:1，关闭/最小化/最大化几乎看不见——正是这段采样代码本来要避免的那种断层。
最小修法：改成用挂在文档里的探针来解析——给它设 background-color: var(--background)，读 computed 的 backgroundColor，再转换。link-color-contract.spec.ts:47-51 已经在用这种「取消费态的值」的做法。不要把自定义属性的声明值直接交给 fillStyle。
另外值得单独说一句：这正是那 2,948 次等价性验证的盲区。 比对 CSS 使用值能证明颜色和阴影没漂，但看不见一个直接读取自定义属性原始字符串的 JavaScript 消费者。凡是这次被转成 light-dark()、同时又被 JS 直接读取的 token，都该按这条再查一遍。
P3：theme.ts:195-196 的注释还在说调色板靠 .dark 切换。 现在真正的开关是 color-scheme。生产路径两者同时拨，所以今天不坏，但会误导下一个只拨 class 的调用者。
P3：次级墨色塌成一级，作用范围是全部，证据只有一块板。 先说清楚：塌本身是对的——--muted-foreground 画在 #AA071E 上即便跟对了模式也只有 2.99:1，不到 AA，而 DESIGN.md 的两级阅读规则又排除了在中间新造第三种灰。结论我不反对。
我想改的是记录方式。生成出来的 media 块把文字和图标的次级色指向 on-color，对任何带 data-astryx-media 的元素生效——是所有 MediaTheme 表面，不只是那个错误提示条——而对比度证据只覆盖了那一块板。DESIGN.md 用的理由是类比：「实心反转表面就是这条规则把底色调到最满」。但 0.24 的淡色叠在页面底色上，和一个整体翻转了配色方案的表面，机制上并不是一回事，这个类比却在承重。
更强的理由是结构性的，而且没被写出来：--color-on-dark 是一个扁平的 #ffffff，供所有反转表面共用，所以「每块板一个次级色」在现有 token 下根本表达不出来。这是约束，不是类比，也才是真正逼出「只能一级」的原因。DESIGN.md 是后续改动会引用的权威，一条建立在「满格淡色」上的规则，会引诱别人把同样的推理套到真正的淡色表面上，而在那里它不成立。
这个 head 上 label 和 test 都是绿的。拦下来的理由只有那条 P1，不是检查没过。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Moving the mode into the value made a token declaration stop being a colour:
`getComputedStyle(root).getPropertyValue('--background')` now reads back
`light-dark(oklch(1.000 0 0), oklch(0.205 0.004 286))`, a recipe that only
resolves where it is used. The Windows titlebar sampler fed that string to a
canvas, and both of its guards passed it: `CSS.supports('color', …)` accepts a
light-dark() pair, and the rejected `fillStyle` assignment left the canvas at
its default opaque black, whose alpha is 255. Measured in Chromium 151, light
and dark both sampled `#000000` — so the native control strip went black in
every palette, putting the light-mode symbol colour `#1c1d21` on it at 1.25:1
and, in dark mode, seaming a black strip against a `#171719` app background.
That seam is the whole reason this code samples instead of hard-coding a pair.
The colour comes off a painted element now — `getComputedStyle().backgroundColor`
is a used value, so the engine has already picked the branch, the same read
`readModalBackdropColor` already makes. That element is the root: `--background`
moves from `body` to `html`, which paints identically (whichever of the two
declares it propagates to the canvas) and leaves the strip's colour on the
surface the strip actually sits over, rather than on a child the sampler would
have to go find. The helper takes an element rather than a string, so an
unresolved declaration has nowhere to enter, and `CSS.supports` goes with it.
Its replacement is a transparent starting fillStyle: an assignment the canvas
cannot parse now reads back at alpha 0 and takes the fallback, for any input,
instead of sampling the black it defaulted to. Verified across palettes and
both modes — default #ffffff/#171719, onedark #eceff2/#15191d.
ink-ladder-contract gains the rule rather than the instance — no product source
reads a custom property as a colour at all. There were none besides this one.
Generated-by: Claude Code
ccb97d3 to
f41b010
Compare
The record on the ink collapse rested on an analogy — a solid inverted surface as the 0.24 tint turned all the way up. The analogy is not what forces the result and does not survive being reused: a tint over `--background` and a surface that has flipped its whole `color-scheme` are mechanically different, so a reader who follows it back the other way gets a conclusion the evidence does not support. What forces it is structural: `--color-on-dark` and `--color-on-light` are one flat value each, shared by every inverted surface, so a rung muted against a particular plate cannot be written — the choice is one tier or an unmuted grey that ignores the surface under it. The 2.99:1 measurement agrees with that rather than deciding it. DESIGN.md §3 and makaTheme.ts now say so, and §3 says outright that the two halves of the rule are not evidence for each other. `applyThemePalette`'s comment still told the next caller that a palette's light and dark variants switch on the `.dark` class. They switch on `color-scheme` now, and a palette carries both modes in each value. Generated-by: Claude Code
|
All three addressed on P1 — confirmed, fixed at the root rather than through a probeReproduced independently before touching anything (Chromium 151): the declaration reads back verbatim, Two departures from the suggested fix: No probe element. That is also what kept The guards are replaced, not repaired. Verified across palettes and both modes: default On the blind spot — that is the more useful half of the finding, and it is now a contract rather than an audit. P3 — the palette commentFixed. It now says a palette carries both modes in each value and follows P3 — the record on the ink collapseAgreed, and the structural constraint you name is the right one. DESIGN.md §3 and CIGreen on 简体中文三条都已处理，落在 P1 — 确认，修在根节点而不是探针元素。 动手前独立复现过（Chromium 151）：声明原样读回， 两处偏离建议： 不用探针元素。 这也是 两道守卫是被替换，不是被修补。 跨调色板和两种模式都验过：default 关于盲区——这是这条 finding 里更有价值的一半，现在它是契约而不是一次审计。 P3 — 调色板注释。 已改：现在写的是每个调色板把两种模式都装在值里、跟随 P3 — 墨色塌级的论证记录。 同意，你指出的结构性约束才是对的理由。DESIGN.md §3 和 CI。 |
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head aa55e0a303adeec9d45fedfcc4b37c46e4e0fd19. No P0 or P1. All three findings from my earlier review are resolved, and two of them are resolved better than what I asked for.
The P1 is fixed at the right layer. --background moved from body to html, so the sampler reads a painted root and getComputedStyle hands the canvas an already-resolved colour. Two things make this more than a point fix: fillStyle is now pre-set to transparent, so anything unparseable reads back at alpha 0 and takes the fallback instead of resting at canvas's default opaque black; and ink-ladder-contract gained a check that forbids getPropertyValue('--x') anywhere in TS/TSX. That last one closes the class, not the instance — the failure mode was that a token's declaration stopped being a colour, and now nothing can read one that way again.
The stale comment on applyThemePalette is corrected, and it now states the trap outright: flipping the class alone no longer switches anything.
The Tinted Surface Rule now gives the structural reason instead of the analogy. --color-on-dark and --color-on-light are one flat value each, shared by every inverted surface, so a rung muted against a specific plate cannot be written at all — the alternative to collapsing is not a second tier but an unmuted grey that ignores the surface beneath it. The measurement agrees rather than decides. It also adds the guard the old wording invited someone to ignore: the two halves of the rule are not evidence for each other, and a conclusion about a 0.24 tint does not carry to a surface that has flipped its whole color-scheme. The comment in makaTheme.ts carries the same correction.
For the record on what this PR set out to do: the toast description goes from 1.04:1 — present but unreadable — to 7.61:1, and the mechanism that got it there follows any inverted surface rather than the ones someone remembered to patch.
test is green on this head.
I am not merging.
简体中文
在 aa55e0a303adeec9d45fedfcc4b37c46e4e0fd19 上批准。没有 P0/P1。上一条评审里的三条发现全部解决，其中两条比我要求的做得更好。
P1 修在了对的层面。 --background 从 body 移到了 html，采样读到的是一个真正被绘制的根元素，getComputedStyle 交给 canvas 的已经是解析好的颜色。有两处让它不只是修一个点：fillStyle 现在先被设成透明，任何解析不了的值都会以 alpha 0 读回并走回退，而不是停在 canvas 默认的不透明黑；以及 ink-ladder-contract 新增了一条检查，禁止全仓 TS/TSX 再出现 getPropertyValue('--x')。后者关掉的是一整类而不是一个实例——这次的失效根源就是「token 的声明值不再是颜色」，现在没有代码能再那样读它。
过时注释已经改对，而且把陷阱直接写明了：只拨 class 现在什么都不会切换。
淡色表面规则现在给的是结构性理由，不再是类比。--color-on-dark 和 --color-on-light 各自只有一个扁平值、供所有反转表面共用，所以「相对这块板做出的次级灰」根本写不出来——不塌级的替代方案不是第二个层级，而是一种无视身下表面的、未经调和的灰。测量是印证而不是裁决。它还补上了旧措辞会诱人忽略的那道防线：这条规则的两半互相之间不构成证据，关于 0.24 淡色的结论不能搬到一个整体翻转了 color-scheme 的表面上。makaTheme.ts 里的注释也做了同样的更正。
最后记一句这个 PR 本来要解决的事：提示条描述文字从 1.04:1（在那里，但读不出来）到 7.61:1，而且达成它的机制能跟随任何反转表面，不是只覆盖某人碰巧想起来要打补丁的那几个。
这个 head 上 test 是绿的。我不合并。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
2 participants
Summary
An error toast currently paints its description at 1.04:1 on the red plate — the text is there, and it cannot be read.
Astryx inverts a surface by setting
color-scheme: darkon it (MediaTheme, which wraps every toast body and every overlay scrim). Its on-media overrides move only text/icon primary and accent, because everything else is expected to followlight-dark()on its own. Maka's palette encodes its modes in a.darkselector on<html>, and nothing under an inverted surface can follow a selector matched on the root. The gap was invisible while Astryx's ink was its own literals; #4509 pointed those tokens at the product tiers, and the toast description became the page's light-mode grey painted on#AA071E. The hairlines and--color-overlay-hoverwere wrong on the same surfaces for the same reason — a near-black 4% hover wash on a dark plate.The mode now lives in the value. Every colour that differs by mode carries both sides as
light-dark(light, dark), declared once. This is the upstream mechanism rather than a list of patches: it covers--color-border,--color-overlay-hover, the background stack and any token a future change aliases, without anyone remembering to add it somewhere. The.darkblock is down to two alphas;--ring-softkeeps one recipe with the pair inside it; the ten palette catalogues lose their second block each; the default palette's dark block turns out to have been re-declaring nine derivations byte-for-byte. No new plumbing was needed —theme.ts,cached-theme-bootstrap.tsand the Storybook preview already setcolor-schemebeside the class, before the first paint and on every change.The toast's ink is a second decision, not a consequence. Converted, the description lands at 2.99:1 — which is where Astryx's own secondary sits on that plate too, and still under AA. DESIGN.md §3's Tinted Surface Rule already says text on a tint is one tier; a solid inverted surface is that rule at full tint, so
makaTheme.ts'sonDark/onLightpoint text and icon secondary at the same on-color as primary. That is a deliberate deviation from upstream, recorded as one.ink-ladder-contractgains both halves: no tokenmakaTheme.tsaliases — nor anything those values transitively read — may be declared under.dark, and the generated media blocks must carry the collapsed tier. The set is derived frommakaTheme.tsrather than listed, so the next alias is covered when it is written.Refs #4509
Verification
Values are unchanged. Every custom property in
maka-tokens.cssread back as a computed colour and box-shadow, across 11 palettes × 2 modes — 2948 readings — before and after:The only delta is dark
--ring-softpicking up light's two transparent zero-size placeholder layers, so both modes now carry the same shadow-list shape. Nothing else moves in any palette or mode.The inverted surface, measured on
#AA071Ewithdata-astryx-media="dark":Checks run:
ink-ladder-contract(4 pass; the new.darkassertion was negative-checked by parking--foregroundback under the selector, which fails it),apps/desktope2elink-color-contract(pass),npm run format,npm run lint,npm run astryx:theme -- --check,tsc -p tsconfig.main.json,tsc -p tsconfig.storybook.json,@maka/uibuild, both Storybook builds.Not run: the repo-wide suite and the Storybook visual smoke. The token-equivalence pass above is the stronger oracle for "no pixel moves" than a smoke over one palette would be.
BEFORE is
main's theme files in this branch's Storybook build (the story set is identical, so onlymaka-tokens.cssandastryx-theme/were reverted); AFTER is the branch. Same story, same viewport, same colour scheme.Review focus
onDark/onLightcollapse the ink tier on everyMediaThemesurface, not just the error toast — today that is Astryx'sToastandOverlayScrim. If a future inverted surface wants two tiers, this is the line to argue with.<Theme>re-declarescolor-schemeon its own wrapper from the class, so the app's subtree turns over on that React commit rather than onsetDarkClass's line. Previously the product palette flipped a frame before Astryx's tokens did; now the whole switch lands together, one repaint later. The e2e that toggled the class alone now sets both halves and waits for the mode to reach the content.palette-matrix.stories.tsxneeded neither half: a palette block carries both modes and resolves against the root, so itsuseIsDarkMutationObserver is gone.AI use
Select exactly one:
Tool(s) and scope: Claude Code — diagnosis, the
light-dark()conversion, theonDark/onLightcollapse, the contract tests, and the verification scripts, under review at each step.Checklist
Does this PR entail a change in behavior?