Skip to content

feat(tokens)!: make a control's fill relative to its surface, and disabled one language - #110

Closed
mattmaynes wants to merge 3 commits into
mainfrom
feat/control-context
Closed

feat(tokens)!: make a control's fill relative to its surface, and disabled one language#110
mattmaynes wants to merge 3 commits into
mainfrom
feat/control-context

Conversation

@mattmaynes

Copy link
Copy Markdown
Contributor

Fixes both findings from the Audio review that were parked as canopy#105 and canopy#106. They turned out to be one cause.

The bug

A control computes its colour from the page canvas while sitting on something else.

  • A control reads as a hole. Slider's thumb is bg-surface (stone.900). On a surface-raised card (stone.800) it is darker than the thing it sits on. Fifteen components paint a control with bg-surface.
  • A disabled control disappears. disabled is stone.800 in dark, which is exactly surface-raised: a 1.00:1 fill against its own background, so an outline button became an empty ring with a 1.82:1 glyph. Eight components used that fill.

The first fix was for the composing component to re-point its Seeds by hand, which is what Audio does. It works and does not scale: invisible when forgotten, only visible in dark, and repeated by every future component on a raised surface. AudioRecorder repeated it, and left a comment predicting this exact fix (feedback 0028).

The fix: the surface declares itself

--color-control is the fill of a control on the current surface, and the surface-raised utility paints the background and re-points --color-control, --color-muted, and --color-ring-offset for its whole subtree. bg-control emits var(--color-control), so one class resolves differently by context with no prop, provider, or variant. Audio's and AudioRecorder's hand-written corrections are deleted.

It ships from preset-surface.css, folded into the built preset exactly as preset-motion.css is, because a @utility is not a token and @source can never emit a rule that no class string contains.

Disabled becomes one language

Every control dims. A fill is an absolute colour, so it can collide with what is behind it; a dim is a transform of what the control already renders, so it degrades on any background, including ones that do not exist yet.

This visibly changes every disabled field in the library - a disabled Input is now a faded input rather than a grey slab. That is the trade, taken deliberately. Audio's loading button opts out with disabled:opacity-100 so its spinner stays legible, which now reads as an explicit override rather than one of two competing conventions.

Two things the change surfaced

  • tailwind-merge did not know surface-raised sets a background, because it is a custom utility rather than a generated bg-*. A caller's bg-muted would have sat alongside the default instead of replacing it, leaving stylesheet order to decide. Registered in cn()'s background group - the same trap as the typography roles.
  • Adding a semantic role means every brand must map it, which the sunset example brand's own test caught.

Verification

A new test resolves both contexts in both themes and asserts a control is never the colour of its surface, and in dark is lighter than it. I reintroduced the original values and watched it fail.

Checked in the browser at the computed-style level rather than by eye: --color-control resolves to #554e42 inside a #423d34 card, and a disabled control dims (opacity: 0.5) instead of filling to 1.00:1.

Learning 18 is revised, not deleted. It recorded the field/toggle split as deliberate, and that split is the direct cause of the second bug, so it now says what is true and why the earlier reasoning did not survive a raised surface.

Breaking

Disabled controls dim instead of swapping to the bg-disabled pair, and bg-surface-raised on a component that hosts controls should become the surface-raised utility. disabled / disabled-foreground stay in the token set for consumers who want a fill.

Full turbo build + test + lint + format:check green: 1107 tests.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KDbg9eZEn2H12TtwNNLVYt

…abled one language

Two bugs from the Audio review (feedback 0026), both dark-only, both the
same mistake: a control computed its colour from the page canvas while
sitting on something else.

A Slider thumb is bg-surface (stone.900). On a surface-raised card
(stone.800) that is DARKER than the thing it sits on, so the player's
primary affordance read as a hole punched in the card. Fifteen components
paint a control with bg-surface.

And `disabled` resolves to stone.800 in dark, which is EXACTLY
surface-raised: a disabled control was a 1.00:1 fill against its own
background, so an outline button became an empty ring with a 1.82:1
glyph. Eight components used that fill.

The first fix was for the composing component to re-point its Seeds by
hand, which is what Audio did. It works and does not scale: invisible
when forgotten, only visible in dark, repeated by every future component
on a raised surface. AudioRecorder repeated it and left a comment
predicting this fix (feedback 0028).

So the SURFACE declares itself and the controls inside correct
themselves. `--color-control` is the fill of a control on the current
surface; the `surface-raised` utility paints the background and
re-points --color-control, --color-muted and --color-ring-offset for its
subtree. `bg-control` emits var(--color-control), so one class resolves
differently by context with no prop, provider, or variant. It ships from
preset-surface.css, folded into the built preset exactly as
preset-motion.css is, because a @Utility is not a token and @source can
never emit a rule no class string contains.

Disabled becomes one language: every control dims. A fill is an absolute
colour so it can collide with what is behind it; a dim is a transform of
what the control already renders, so it degrades on any background,
including ones that do not exist yet. This visibly changes every disabled
field in the library - a disabled Input is now a faded input rather than
a grey slab - and that is the trade, taken deliberately. Audio's loading
button opts out with disabled:opacity-100 so its spinner stays legible.

Two things the change surfaced:

- `surface-raised` is a custom utility, so tailwind-merge did not know it
  sets a background: a caller's bg-* would have sat alongside the default
  instead of replacing it, leaving stylesheet order to decide. Registered
  in cn()'s background group, same trap as the typography roles.
- Adding a semantic role means every brand must map it, which the sunset
  example brand's own test caught.

Guarded by a test that resolves both contexts in both themes and asserts
a control is never the colour of its surface, and in dark is lighter than
it. Verified by reintroducing the original values and watching it fail.
Checked in the browser at the computed-style level, not by eye:
--color-control resolves to #554e42 inside a #423d34 card, and a disabled
control now dims rather than filling to 1.00:1.

Learning 18 is revised rather than deleted: it recorded the field/toggle
split as deliberate, and that split is the direct cause of the second
bug, so it now says what is true and why the earlier reasoning did not
survive a raised surface.

BREAKING CHANGE: disabled controls dim instead of swapping to the
bg-disabled pair, and `bg-surface-raised` on a component that hosts
controls should become the `surface-raised` utility. The `disabled` and
`disabled-foreground` tokens remain for consumers who want a fill.

Full turbo build + test + lint + format:check green: 1107 tests.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KDbg9eZEn2H12TtwNNLVYt
The control-context and disabled changes are breaking in the design-system
sense: nothing fails to compile, but every disabled control changes
appearance and `bg-surface-raised` on a control host should become the
`surface-raised` utility. Both migrations are written out, including the
fact that neither is mandatory - the old token still resolves.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KDbg9eZEn2H12TtwNNLVYt
Comment thread packages/canopy/src/twigs/InputOTP.tsx Outdated
'relative flex h-10 w-10 items-center justify-center border-y border-r border-border bg-control text-base md:text-sm text-text',
'first:rounded-l-md first:border-l last:rounded-r-md',
'group-has-[:disabled]:bg-disabled group-has-[:disabled]:text-disabled-foreground',
'group- group-',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 Spectra Engineer - blocker
This is a corrupted find-and-replace, not a class string. group-has-[:disabled]:bg-disabled group-has-[:disabled]:text-disabled-foreground was rewritten to the literal 'group- group-' - two invalid class names that Tailwind emits nothing for. A disabled InputOTP now has no disabled treatment at all: the slots keep full-strength bg-control + text-text and read as enabled. It survived because nothing asserts InputOTP's disabled classes (see the tester note on the InputGroup test).

Suggested change
'group- group-',
'group-has-[:disabled]:opacity-50',

The docblocks at lines 38 and 82 still describe the old bg-disabled / text-disabled-foreground pair and need the same edit.

"$type": "color",
"$description": "Fill of a control that sits ON the current surface (a thumb, a checkbox, a field). Re-pointed by the surface-raised utility, so the same class steps correctly on a raised surface."
},
"control-raised": {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Spectra Architect - major
control-raised is stone.700, and so are muted-raised and border in dark. One rung is now doing three jobs, so inside a surface-raised subtree a control's fill, its border, and the interaction fill it sits on are all #554e42 - 1.00:1 against each other. Measured in the running Storybook, not inferred:

pair (dark, inside surface-raised) ratio
Slider thumb (bg-control) vs track (bg-muted) 1.00
Switch thumb (bg-control) vs off track (bg-border) 1.00
Input fill (bg-control) vs its own border-border 1.00
Toggle outline off (bg-control) vs on (bg-muted) 1.00
Input fill vs the card it sits on 1.31

The spec set out to stop a control colliding with what is behind it and this reintroduces the collision one rung higher up. preset-surface.css re-points --color-control and --color-muted to the same value, so "the control" and "the highlight the control sits on" become indistinguishable by construction.

Give the raised control its own rung rather than borrowing muted-raised's - stone.600 separates it from both muted-raised (stone.700) and surface-raised (stone.800):

Suggested change
"control-raised": {
"control-raised": {
"$value": "{color.stone.600}",

Whatever value is chosen, the guard has to assert control-raised != muted-raised and control-raised != border too, or the next person re-lands this.

aria-labelledby={thumbAriaLabelledBy}
aria-valuetext={thumbAriaValueText}
className="block h-5 w-5 rounded-full border border-border bg-surface shadow-sm focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-ring-offset aria-invalid:ring-2 aria-invalid:ring-danger"
className="block h-5 w-5 rounded-full border border-border bg-control shadow-sm focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-ring-offset aria-invalid:ring-2 aria-invalid:ring-danger"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Spectra Designer - major
I looked at this in Storybook (Branches/Audio → Playground, dark) and the seek thumb is gone. Computed styles off the live page:

track  .bg-muted   -> rgb(85, 78, 66)
thumb  .bg-control -> rgb(85, 78, 66)
thumb  border      -> rgb(85, 78, 66)

Thumb fill, thumb border and track are one colour. All that survives is shadow-sm at 6% alpha. On main the thumb was stone.900 on a stone.900 track but kept a stone.700 border, so it still read as a ring - this change removes the last cue. Audio is the component whose review produced this spec, and its primary affordance is now less visible than before.

Fix is upstream (see the control-raised comment on semantic.dark.json); once a control fill is a real step off muted-raised the thumb reads again. Worth adding an Audio/Slider-on-a-card story in dark so this is visible in the gallery rather than only in a player.

{...props}
>
<SwitchPrimitive.Thumb className="pointer-events-none block h-5 w-5 translate-x-0.5 rounded-full bg-surface shadow-sm transition-transform data-[state=checked]:translate-x-5" />
<SwitchPrimitive.Thumb className="pointer-events-none block h-5 w-5 translate-x-0.5 rounded-full bg-control shadow-sm transition-transform data-[state=checked]:translate-x-5" />

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Spectra Designer - major
Same collision, and it is starker here because a Switch has nothing else to read by. Inside a surface-raised card in dark the track is bg-border #554e42 and this thumb is bg-control -> #554e42. Side by side in Storybook, the off switch on the page canvas shows its thumb clearly; the identical switch on a Card renders as a plain uniform pill with no thumb at all. A user cannot tell an off switch from a decorative capsule, and off vs on is the entire content of the control.

bg-control for a thumb is the right idea; the value it resolves to on a raised surface is not. Fixing control-raised (see the token comment) resolves this, the Slider, and the borderless Input together.

*/
export const inputGroupVariants = cva(
'flex w-full items-center overflow-hidden rounded-md border border-border bg-surface text-base md:text-sm text-text focus-within:outline-none focus-within:ring-2 focus-within:ring-ring focus-within:ring-offset-2 focus-within:ring-offset-ring-offset has-[:disabled]:cursor-not-allowed has-[:disabled]:bg-disabled has-[:disabled]:text-disabled-foreground aria-invalid:border-danger aria-invalid:ring-danger',
'flex w-full items-center overflow-hidden rounded-md border border-border bg-control text-base md:text-sm text-text focus-within:outline-none focus-within:ring-2 focus-within:ring-ring focus-within:ring-offset-2 focus-within:ring-offset-ring-offset has-[:disabled]:cursor-not-allowed has-[:disabled]:opacity-50 aria-invalid:border-danger aria-invalid:ring-danger',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 Spectra Engineer - major
opacity multiplies down the tree, and this change puts two of them in series. The group root dims with has-[:disabled]:opacity-50; the InputGroupInput inside it is an Input, which now carries its own disabled:opacity-50 from inputVariants; InputGroupAddon (line 133) adds a third opacity-50. A disabled InputGroup therefore renders its value and its affix at 0.25, not 0.5 - measured contrast of the field text against the field:

light dark
disabled Input (0.5) 2.97:1 4.33:1
disabled InputGroup (0.25) 1.65:1 2.08:1

Visible in Storybook: the disabled InputGroup is markedly fainter than the disabled Input directly above it, which breaks the stated goal that "a disabled group reads identically" to a disabled field. InputGroupButton compounds the same way.

Dim once, at the frame, and neutralise the children:

Suggested change
'flex w-full items-center overflow-hidden rounded-md border border-border bg-control text-base md:text-sm text-text focus-within:outline-none focus-within:ring-2 focus-within:ring-ring focus-within:ring-offset-2 focus-within:ring-offset-ring-offset has-[:disabled]:cursor-not-allowed has-[:disabled]:opacity-50 aria-invalid:border-danger aria-invalid:ring-danger',
'flex w-full items-center overflow-hidden rounded-md border border-border bg-control text-base md:text-sm text-text focus-within:outline-none focus-within:ring-2 focus-within:ring-ring focus-within:ring-offset-2 focus-within:ring-offset-ring-offset has-[:disabled]:cursor-not-allowed has-[:disabled]:opacity-50 [&_:disabled]:opacity-100 aria-invalid:border-danger aria-invalid:ring-danger',

and drop the opacity-50 on the addon at line 133 (the frame already dims it). This is the general hazard of switching a fill treatment to a transform: fills replace each other, transforms stack. It deserves a line in the revised learning 18.

extend: {
classGroups: {
'font-size': [{ text: TYPOGRAPHY_ROLES }],
'bg-color': SURFACE_CONTEXTS,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Spectra Architect - major
Registering surface-raised in the bg-color group makes tailwind-merge treat it as only a background, but it is a background plus a context. A caller who wants a different panel colour now silently deletes the --color-control / --color-muted / --color-ring-offset re-pointing along with the paint, and every control inside the panel goes back to computing its fill from the page canvas - on a surface that is no longer the page canvas. That is precisely the failure mode the spec says it eliminates ("invisible when forgotten"), reintroduced one layer down.

Drawer.test.tsx:158 pins that behaviour as intended (bg-muted wins, surface-raised is removed), so this is a design decision rather than an oversight - but as written a caller has no way to say "keep the context, change the paint".

Two ways out, either is fine, but the current shape should not ship unremarked:

  • Split the concern: a raised-context utility that only sets the vars, and keep bg-surface-raised as the paint. Components compose both; a caller's bg-* then overrides only the paint.
  • Or keep one utility and register it in a dedicated class group so a caller's bg-* sits alongside rather than replacing it, and document that overriding the paint requires bg-transparent on top.

At minimum, the JSDoc above should state that a caller's bg-* drops the control context too - right now it advertises only the background merge.


/* Everything below is the context. A control inside this subtree is one step up from THIS
surface rather than from the page. */
--color-control: var(--color-control-raised);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧪 Spectra Tester - major
The whole mechanism is unguarded. I deleted both re-points from this file - --color-control here and --color-ring-offset at line 41 - rebuilt Roots, and ran the full suite:

@rogueoak/roots:test   54 passed (54)
@rogueoak/canopy:test  1041 passed (1041)
Tasks: 7 successful, 7 total

Green with the feature gutted. tokens.test.ts asserts the token values are distinct, which is the pre-condition; nothing asserts that the utility actually re-points anything, that it ships in dist/tailwind-preset.css, or that a bg-control descendant resolves to control-raised inside it. The spec's acceptance criterion "the surface-raised utility paints the surface and re-points --color-control, and ships from the built preset" has no test behind it.

Cheapest guard that would have failed: read dist/tailwind-preset.css and assert the @utility surface-raised block contains all three re-points plus background-color: var(--color-surface-raised). A stronger one, since build.mjs already runs before tests, is a jsdom test that mounts <div class="surface-raised"><span class="bg-control"/></div> against the built CSS and asserts the computed fill differs from the base context - that is the assertion the browser check in the PR description made by hand.

Comment thread packages/roots/tokens.test.ts Outdated
// Light is the degenerate case on purpose: both are white, and the control reads by its
// border instead. Only assert the separation where the fill is what carries it.
if (hex(scope, 'surface') === primitives['base-white']) return;
expect(hex(scope, 'control')).not.toBe(hex(scope, 'bg'));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧪 Spectra Tester - major
The test is named "a control on the base surface is not the same colour as it" but compares control against bg, which is the page canvas, not a surface. The comparison the name promises fails: in dark control is stone.900 and surface is stone.900 - 1.00:1.

That is not academic. The spec deliberately keeps bg-surface on SideNav, TopNav, Menubar and the TopNav mobile menu because they are panels, and any control placed on one of those panels now has a fill exactly equal to its background, which is the defect the whole spec is about. The raised context has a fix; the surface context has none, and this assertion is worded as though it covered it.

Either assert what the name says and add the missing rung, or rename the test to what it actually checks so the gap is visible:

Suggested change
expect(hex(scope, 'control')).not.toBe(hex(scope, 'bg'));
expect(hex(scope, 'control')).not.toBe(hex(scope, 'surface'));

(that fails today, which is the point). The related gap: nothing compares control-raised to muted-raised or to border, which is how the 1.00:1 Slider/Switch collisions got through - see the semantic.dark.json comment.

</InputGroup>,
);
expect(screen.getByText('$').className).toContain('text-disabled-foreground');
expect(screen.getByText('$').className).toContain('opacity-50');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧪 Spectra Tester - minor
This assertion was updated to match the new output rather than to check the behaviour, and in doing so it pins a bug: opacity-50 on the addon sits inside a frame that is already at opacity-50, so the rendered affix is at 0.25 (see the InputGroup.tsx:46 comment). The test now says "the addon dims" and passes while the addon dims twice as far as any other disabled control in the library.

Assert the outcome the component promises instead - that a disabled group reads the same as a disabled Input - e.g. that the addon does not add its own opacity on top of the frame's:

Suggested change
expect(screen.getByText('$').className).toContain('opacity-50');
expect(screen.getByText('$').className).not.toContain('opacity-50');

Same class of gap one file over: InputOTP has no disabled-class assertion at all, which is why the corrupted 'group- group-' string shipped green.

// the page when a focused field is below 16px, so the base font must be >=16px on phones; the
// md breakpoint restores the denser 14px on larger (non-zooming) viewports (feedback 0017).
'flex w-full rounded-md border border-border bg-surface px-3 text-base md:text-sm text-text placeholder:text-text-muted focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-ring-offset disabled:cursor-not-allowed disabled:bg-disabled disabled:text-disabled-foreground aria-invalid:border-danger aria-invalid:ring-danger',
'flex w-full rounded-md border border-border bg-control px-3 text-base md:text-sm text-text placeholder:text-text-muted focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-ring-offset disabled:cursor-not-allowed disabled:opacity-50 aria-invalid:border-danger aria-invalid:ring-danger',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Spectra Designer - minor
Looked at this in both themes. Dark is fine. Light is not: the spec's "a faded input rather than a grey slab" turns out to be a faded input that reads as an empty one.

At opacity-50 on the light canvas the field itself barely moves - #ffffff over stone.50 composites to #fbfaf8, 1.04:1 against the page where the enabled field is 1.09:1 - so the only thing that changes is the text, from #1e1b16 to #95928d. That is within a hair of text-muted (#8a8578), the placeholder colour. Side by side in Storybook, a disabled field holding "Maple syrup" and an empty field showing the placeholder "Maple syrup" are the same picture. Text contrast drops 13.49:1 -> 2.97:1, and a disabled placeholder lands at 2.16:1.

WCAG exempts disabled controls, so this is a legibility and semantics call rather than a conformance failure, but "has a value you cannot edit" and "has no value" must not look identical. Keep the single dim language and add one non-colour cue that survives it - the field keeps disabled:bg-muted under the dim, or a disabled:border-dashed/disabled:border-border-strong. Either is still relative to the surface, so the spec's guarantee holds.


/* A focus ring is drawn ON this surface, so its offset has to match it or the ring punches a
page-coloured halo through the card. */
--color-ring-offset: var(--color-surface-raised);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Spectra Architect - minor
Custom properties set in a rule apply to the element itself as well as its subtree, so the raised surface re-points its own ring offset. A focus ring is drawn outside the element's border box, over whatever is behind it - so if a surface-raised element ever draws a ring, its offset paints a card-coloured halo on the page, which is the exact failure this line is written to prevent, inverted.

Nothing draws it today (DialogContent, SheetContent, DrawerContent, Card and the Toast root all carry surface-raised and none currently ring themselves), so this is latent rather than live - but they are all focusable, and a caller adding focus-visible:ring-2 to a Card is an obvious next step.

Scope the context to descendants and leave the element on the page's offset:

Suggested change
--color-ring-offset: var(--color-surface-raised);
--color-ring-offset: var(--color-surface-raised);
}
@utility surface-raised {
& > * {
--color-ring-offset: var(--color-surface-raised);
}

(or however the codebase prefers to express it - the point is that --color-ring-offset should describe what is behind a child, not what is behind the surface itself).

* SelectTrigger - the field button that opens the dropdown. Class tokens mirror the Input
* field (spec 0006) for visual parity: `border-border` + `bg-surface` + `text-text`, the
* field (spec 0006) for visual parity: `border-border` + `bg-control` + `text-text`, the
* shared focus-visible ring, the `disabled:*` token pair (not opacity), and the

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 Spectra Engineer - minor
The docblocks were not updated with the code: this still advertises "the disabled:* token pair (not opacity)" on a component that now uses opacity and nothing else. The same stale sentence is in:

  • seeds/Input.tsx:20 - "the disabled token pair (not opacity)"
  • seeds/Textarea.tsx:13 - same
  • branches/Combobox.tsx:46 and branches/DatePicker.tsx:43 - "the disabled:* token pair (not opacity)"
  • branches/Menubar.tsx:82 - "Disabled triggers drop to the disabled-foreground token"
  • twigs/InputOTP.tsx:38 and :82 - "bg-disabled / text-disabled-foreground ... rather than the toggle-control opacity wash"

These files carry unusually load-bearing comments (they are the reason the old split was discoverable), so leaving them asserting the opposite of the code is worse here than in a normal codebase - the InputOTP ones in particular now describe a treatment that was deleted by a broken replacement.

@@ -1,43 +1,170 @@
{

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 Spectra Engineer - nit
Both sunset files were re-serialised from one-line-per-token to expanded objects, turning a 2-line change into ~200 lines each and hiding the two tokens that were actually added. The same pass escaped every in tokens/color/semantic.json / semantic.dark.json descriptions to , so the existing $description prose is now unreadable in the source.

Neither is part of this spec. Restore the original formatting and the literal arrows, and add control / control-raised in the existing style - the diff then shows the four lines this change makes.


it('dark: a raised control lifts off its surface rather than sinking into it', () => {
const dark = scopeOf('\\.dark');
const luminance = (value: string) =>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧪 Spectra Tester - nit
luminance sums R+G+B, which is not luminance - it weights blue the same as green. It happens to order these two greys correctly, but the name promises a perceptual measure the function does not compute, and the next person reaching for it on a coloured pair will get a wrong answer silently.

This file already has a real relative-luminance implementation behind its contrast helper; reuse that, or rename this one channelSum so its limits are on the label.

* surface rather than in every component that sits on one.
*/
const TRANSPORT_BUTTON_CLASS = 'rounded-full focus-visible:ring-offset-surface-raised';
const TRANSPORT_BUTTON_CLASS = 'rounded-full';

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 Spectra Engineer - minor
The sweep is half done. Audio and AudioRecorder drop focus-visible:ring-offset-surface-raised because the context now supplies it, but seven other sites still hardcode it:

  • branches/Dialog.tsx:96
  • branches/Sheet.tsx:133
  • branches/SideNav.tsx:254
  • branches/ResponsiveDialog.tsx:214
  • branches/NavigationMenu.tsx:174
  • branches/Toast.tsx:170 and :200

Every one of them is inside a surface-raised subtree, so the override is now redundant and resolves to the same colour. Leaving them in is exactly the "two competing conventions" the spec argues against for disabled, and the next reader cannot tell whether they are load-bearing. Delete them in this PR, or keep them all and delete none - either is defensible, the split is not.

Two things from the persona review, and the second one blocks this PR.

The sweep corrupted InputOTP. `\s*has-\[:disabled\]:bg-disabled` also
matched inside `group-has-[:disabled]:bg-disabled`, leaving the literal
string 'group- group-' where the classes had been - so a disabled
InputOTP had NO disabled treatment at all, and nothing asserted it.
Fixed, with the treatment it should have had.

The token guards then measured the wrong pairs and so proved nothing.
"A control on the base surface is not the same colour as it" compared
`control` to `bg`, the page, rather than to `surface`. Corrected, plus a
new guard that a raised control is distinct from its border and its hover
fill. BOTH now FAIL, which is the honest state of this branch:

  dark, inside a raised card
    surface-raised  #423d34
    control-raised  #554e42
    muted-raised    #554e42   <- same
    border          #554e42   <- same
  dark, on a panel
    surface         #322e28
    control         #322e28   <- same

So a Slider thumb on a card is fill #554e42, border #554e42, on a track
#554e42: invisible, and WORSE than main, where it still read as a ring.
Same for a Switch's off thumb. The mechanism in this PR is right and the
ladder underneath it is not: the dark neutral ramp is being asked for six
distinct roles between the page and the text and only has five steps.

Leaving the guards red rather than deleting them or loosening them to
pass. They now say what has to be true, and the values have to move to
meet them - which is a decision about the ramp, not a tweak.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KDbg9eZEn2H12TtwNNLVYt
@mattmaynes
mattmaynes marked this pull request as draft August 10, 2026 14:40
@mattmaynes

Copy link
Copy Markdown
Contributor Author

Blocked - moved to draft. The mechanism is right, the ladder underneath it is not.

The review found a blocker and I fixed it, but chasing it exposed something bigger that I am not going to paper over: this branch is currently a regression, and the fix is a decision about the neutral ramp rather than a tweak.

What is wrong

Measured from the built tokens, in dark, inside a raised card:

role value
surface-raised (the card) #423d34
control-raised (a control's fill) #554e42
muted-raised (its hover fill) #554e42
border (its own edge) #554e42

A Slider thumb on a card is therefore fill #554e42, border #554e42, sitting on a track #554e42. It is invisible - and worse than main, where it at least still read as a ring. The designer confirmed the same for a Switch's off thumb, which is the entire control.

And on a plain panel, control and surface are both #322e28, so every control on the bg-surface panels this spec deliberately keeps (SideNav, TopNav, Menubar) is 1.00:1 against its own background. That one predates this branch; my guard just never measured the right pair to find it.

This is the spec's own bug, one rung up. I fixed a collision at surface-raised and created three more at control-raised.

Why it is not a tweak

The dark neutral ramp has five usable steps between the page and the text, and the raised context needs six distinct roles: page, panel, card, control fill, hover fill, border. There is nowhere for control-raised to go that is not already occupied.

What I have done

  • Fixed the blocker: the sweep left the literal string 'group- group-' in InputOTP, so a disabled OTP field had no disabled treatment. \s*has-\[:disabled\]:bg-disabled also matched inside group-has-[:disabled]:....
  • Corrected the token guards and left them red. The old one compared control to bg (the page) instead of surface, which is why it passed while the real pair collided. They now state what has to be true, and the values have to move to meet them.

What I have not done

Not merged, not released, not touched consumers. Cutting 2.0.0 on this would ship 1.00:1 controls into four apps.

The other reported findings (opacity compounding to 0.25 in InputGroup, cn() letting a caller's bg-* delete the context along with the paint, the unguarded utility, the seven components still hardcoding ring-offset-surface-raised, the eight stale docblocks) are all real and all fixable once the ladder question is settled, since some of them change with it.

@mattmaynes

Copy link
Copy Markdown
Contributor Author

Closing unmerged. This was the wrong size of answer.

The finding was narrow: a Slider thumb and a disabled control read wrong on a raised card. I responded with a token-layer context mechanism plus a library-wide change to how every disabled control looks, and it introduced collisions the original bug did not have - a Slider thumb that is invisible rather than merely dark, and the same for a Switch's off thumb. Fixing that from here means moving the neutral ramp, which is not a reasonable thing to do to fix two components.

Not merging, not reworking. The two underlying issues are unfixed again and stay that way until someone does something proportionate to them.

@mattmaynes mattmaynes closed this Aug 10, 2026
@mattmaynes
mattmaynes deleted the feat/control-context branch August 10, 2026 14:44
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