From 169622913569f5646e9ae4afd4d40e1577bfef37 Mon Sep 17 00:00:00 2001 From: OpenStaxClaude Date: Thu, 10 Sep 2026 18:56:33 +0000 Subject: [PATCH 1/9] CORE-2720: add a global CSS token file generated from the JS theme MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Establishes one place a theme value is written and referenced from CSS, with a test that fails if the two disagree. The sweep of the already-migrated stylesheets onto it follows separately. src/theme/theme.css holds a single :root block. Colour tokens are the kebab-case palette key (palette.neutralLighter -> --ox-color-neutral-lighter) plus --ox-color-link, --ox-color-link-hover, --ox-z-index-* and --ox-padding-navbar-*. The --ox- prefix avoids collisions with a consuming app's own variables. The file is generated, not hand-written. themeCss.ts owns the projection and npm run generate:theme-css writes it; build.bash runs it as its first step, before either tsc pass and before the rsync, and publish.bash inherits that via build:clean, so a published package cannot ship a stale file. It is committed as well as generated because jest and ladle read src/ directly and CI runs lint/test rather than build. Deliberately not hooked into pretest — regenerating before the suite would make the freshness check pass vacuously. Adds the four button variant colours to the palette, which theme/buttons.ts had been holding as bare string literals with nothing recording that they are hover/active variants of orange and darkGray. Purely additive. Enforcement, in tokens.spec.ts, on top of the CORE-2736 engine: 1. The committed theme.css is exactly what the generator produces. One equality, so a missing token, an orphan token and a stale value all fail the same way. 2. No component stylesheet writes a colour literal that duplicates a theme value. 3. No component stylesheet introduces a colour that is neither a theme value nor on the KNOWN_OFF_PALETTE allowlist, each entry with a reason. 4. No component stylesheet reads an --ox-* token that does not exist, which would otherwise fall through to its fallback silently. Check 2 cannot pass yet — 16 stylesheets migrated before the tokens existed still carry hand-copied literals. PENDING_SWEEP names them, and is asserted to be exactly the failing set so it cannot rot in either direction: dropping a name without sweeping the file fails, and sweeping a file without dropping its name fails too. The list reaches empty in the sweep PR and goes away with the assertion. No CSS @import: build.bash rsyncs CSS 1:1 with no bundler, so an @import would depend on each consumer's resolver. Component .tsx files import theme.css alongside their own stylesheet instead. Consumers need do nothing. Split out of #143. Stacked on #149. Co-Authored-By: Claude Opus 5 --- README.md | 90 +++++++++ package.json | 1 + scripts/build.bash | 5 + scripts/generate-theme-css.bash | 26 +++ src/theme/buttons.ts | 8 +- src/theme/palette.ts | 6 +- src/theme/theme.css | 74 +++++++ src/theme/themeCss.ts | 69 +++++++ src/theme/tokens.spec.ts | 336 ++++++++++++++++++++++++++++++++ 9 files changed, 610 insertions(+), 5 deletions(-) create mode 100755 scripts/generate-theme-css.bash create mode 100644 src/theme/theme.css create mode 100644 src/theme/themeCss.ts create mode 100644 src/theme/tokens.spec.ts diff --git a/README.md b/README.md index 689338ff3..1a5c372c1 100644 --- a/README.md +++ b/README.md @@ -51,6 +51,96 @@ npm run lint # Check code rules npm run dist # Build distribution files ``` +## Styling + +Components are styled with plain CSS in a sibling `.css` file, imported for side effects. + +### Theme tokens + +Theme values are defined once, in JavaScript, and projected into CSS custom properties by +`src/theme/theme.css`. That file is **generated** from `src/theme/palette.ts` and +`src/theme.ts` — never edit it by hand. `npm run build` regenerates it as its first step, so +a published package can never ship a stale one. To refresh it without a full build: + +``` +npm run generate:theme-css +``` + +It is committed as well as generated, because jest and ladle read `src/` directly and never +run the build — `src/theme/tokens.spec.ts` fails when the committed copy is stale, which is +what makes CI (which runs lint and test, not build) catch it. + +**Never write a theme value as a literal in a component stylesheet** — reference the token +instead: + +```css +/* no */ +.thing { border-color: #d5d5d5; } + +/* yes */ +.thing { border-color: var(--ox-color-pale); } +``` + +Tokens are `--ox-`-prefixed, so they will not collide with a consuming app's own variables. +Colour tokens are the kebab-case form of the `src/theme/palette.ts` key +(`palette.neutralLighter` → `--ox-color-neutral-lighter`); there are also `--ox-color-link`, +`--ox-color-link-hover`, `--ox-z-index-*` and `--ox-padding-navbar-*`. + +Any component whose CSS uses a token must import the token file alongside its own stylesheet: + +```ts +import './MyComponent.css'; +import '../theme/theme.css'; +``` + +Consumers need do nothing — bundlers deduplicate the import, and `sideEffects` in +`package.json` keeps it from being tree-shaken. + +### Component override hooks and when to bind in JS + +A component may expose its own `--component-*` custom property so consumers can restyle it. +Declare the **default in the stylesheet** with the token as the fallback, and do not bind it +from JavaScript: + +```css +.tabs [role="tab"] { border-color: var(--tabs-active-border-color, var(--ox-color-dark-green)); } +``` + +Bind a custom property from JavaScript only when its value genuinely varies at runtime — a +variant lookup, a numeric prop, a disabled state. A static colour pushed through an inline +style is duplication with extra steps, and it wins over the cascade in ways callers do not +expect. + +Widen the component's `style` prop to `CSSPropertiesWithVariables` (from `src/types`) so +callers can set these without a cast. + +### What CI enforces + +`src/theme/tokens.spec.ts` fails the build if: + +- the committed `theme.css` is not what the generator produces from the JS theme (the build + regenerates it; this is what stops a stale copy reaching jest, ladle or a reviewer) +- a component stylesheet writes a colour literal that duplicates a theme value +- a component stylesheet introduces a colour that is not a theme value and not in the + `KNOWN_OFF_PALETTE` allowlist +- a component stylesheet reads an `--ox-*` token that does not exist + +The colour check parses declarations, so it covers every syntax a colour can be written in — +hex, `rgb()`/`hsl()`/`oklch()`/`color()`, and bare named colours wherever they appear, +including inside shorthands and gradient stops. Functions that merely *contain* colours +(`var()`, `color-mix()`, the gradients) are descended into rather than treated as literals, +so building a value out of tokens stays clean. A translucent colour is accepted when its +opaque channels are a theme value — `rgba(0, 0, 0, 0.2)` is black at 20% and there is no +token form for it — which still refuses a new hue smuggled in through `rgba()`. + +The check has its own tests, so the guarantee is a tested one rather than an asserted one. + +Adding a genuinely new colour is therefore a deliberate act: put it in `palette.ts` if it is +part of the design, or in the allowlist with a reason if it is a one-off we are keeping. + +Breakpoints are the known gap — `@media (min-width: var(--x))` is not valid CSS, so +breakpoint values are still repeated in media queries and are not covered by the tests. + ## Testing Across Projects To test changes before tagging: diff --git a/package.json b/package.json index 3384fd4dd..5f0013696 100644 --- a/package.json +++ b/package.json @@ -40,6 +40,7 @@ ], "scripts": { "build": "./scripts/build.bash", + "generate:theme-css": "./scripts/generate-theme-css.bash", "clean": "rm -rf ./dist", "build:clean": "npm-run-all clean build", "dist": "npm-run-all clean build", diff --git a/scripts/build.bash b/scripts/build.bash index c86b3997a..ec96d6725 100755 --- a/scripts/build.bash +++ b/scripts/build.bash @@ -6,6 +6,11 @@ project_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )/.." >/dev/null 2>&1 && pwd cd "$project_dir" +# theme.css is generated from the JS theme, so regenerate before anything copies it. +# Committed too, because jest and ladle read src/ directly and never run this script; +# src/theme/tokens.spec.ts fails when the committed file is stale. +./scripts/generate-theme-css.bash + tsc_args=(--noEmit false --declaration) npx tsc --project tsconfig.without-specs.esm.json "${tsc_args[@]}" diff --git a/scripts/generate-theme-css.bash b/scripts/generate-theme-css.bash new file mode 100755 index 000000000..818103f1c --- /dev/null +++ b/scripts/generate-theme-css.bash @@ -0,0 +1,26 @@ +#!/usr/bin/env bash +# spell-checker: ignore pipefail outdir +# +# Regenerates src/theme/theme.css from the JavaScript theme. The committed file is checked +# against this output by src/theme/tokens.spec.ts, so CI fails if palette.ts or theme.ts +# changes without this being re-run. +set -euo pipefail; if [ -n "${DEBUG-}" ]; then set -x; fi + +project_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )/.." >/dev/null 2>&1 && pwd )" +cd "$project_dir" + +# The theme is TypeScript and the repo has no TS runner outside jest, so compile the +# renderer and its (small, pure) import graph to a throwaway directory and run that. +outdir="$(mktemp -d)" +trap 'rm -rf "$outdir"' EXIT + +npx tsc src/theme/themeCss.ts \ + --outDir "$outdir" --rootDir . \ + --module commonjs --target es2019 --moduleResolution node --esModuleInterop --skipLibCheck + +node -e " + const fs = require('fs'); + const { renderThemeCss } = require('$outdir/src/theme/themeCss'); + fs.writeFileSync('src/theme/theme.css', renderThemeCss()); + console.log('wrote src/theme/theme.css'); +" diff --git a/src/theme/buttons.ts b/src/theme/buttons.ts index 994c82e28..882c7762f 100644 --- a/src/theme/buttons.ts +++ b/src/theme/buttons.ts @@ -19,8 +19,8 @@ const asButtonStyleSetTypes = (st: { [K in keyof T]: ButtonStyleSet }) => st; const buttonStyleSets = asButtonStyleSetTypes({ primary: { background: palette.orange, - backgroundActive: "#b03808", - backgroundHover: "#be3c08", + backgroundActive: palette.darkerOrange, + backgroundHover: palette.darkOrange, color: palette.white, outline: palette.white, shadow: palette.black, @@ -36,8 +36,8 @@ const buttonStyleSets = asButtonStyleSetTypes({ }, secondary: { background: palette.darkGray, - backgroundActive: "#4c4c4c", - backgroundHover: "#646464", + backgroundActive: palette.darkerGray, + backgroundHover: palette.mediumGray, color: palette.white, outline: palette.white, shadow: palette.black, diff --git a/src/theme/palette.ts b/src/theme/palette.ts index e24d4ca17..f4881551e 100644 --- a/src/theme/palette.ts +++ b/src/theme/palette.ts @@ -18,7 +18,9 @@ export const palette = { neutralLightBlue: "#0dc0dc", tangerine: "#ffbd3e", gray: "#5e5e5e", + mediumGray: "#646464", // darkGray, hover darkGray: "#757575", + darkerGray: "#4c4c4c", // darkGray, active pale: "#d5d5d5", light: "#e4e4e4", white: "#ffffff", @@ -34,5 +36,7 @@ export const palette = { neutralFeedback: "#555", // another dark gray neutralDarker: "#424242", // very dark gray black: "#000000", - orange: "#D4450C" + orange: "#D4450C", + darkOrange: "#be3c08", // orange, hover + darkerOrange: "#b03808" // orange, active } as const; diff --git a/src/theme/theme.css b/src/theme/theme.css new file mode 100644 index 000000000..6e9c5ce6c --- /dev/null +++ b/src/theme/theme.css @@ -0,0 +1,74 @@ +/* + * Global theme tokens. + * + * GENERATED FILE — do not edit by hand. Run `npm run generate:theme-css`. + * + * Projected from the JavaScript theme, which stays the single source of truth: + * src/theme/palette.ts (colours) and src/theme.ts (link colours, z-index, padding). + * src/theme/tokens.spec.ts fails if this file is out of date, so add the value to the + * JS theme and regenerate rather than editing here. + * + * Component CSS should reference these rather than repeating a literal. Where a + * component exposes its own --component-* override hook, use the token as the + * fallback: var(--tabs-border-color, var(--ox-color-pale)). + * + * Imported for side effects by every component whose CSS depends on it, the same + * way component stylesheets are imported. Consumers need do nothing. + */ +:root { + /* palette — src/theme/palette.ts */ + --ox-color-red: #ca2026; + --ox-color-danger: #c2002f; + --ox-color-dark-red: #c22032; + --ox-color-light-red: #e298a0; + --ox-color-pale-red: #FBE7EA; + --ox-color-green: #77af42; + --ox-color-light-green: #8bc753; + --ox-color-pale-green: #e0edd3; + --ox-color-dark-green: #63a524; + --ox-color-darker-green: #4e7226; + --ox-color-pale-yellow: #ffffbb; + --ox-color-teal: #0dc0de; + --ox-color-dark-teal: #007297; + --ox-color-blue: #007da4; + --ox-color-medium-blue: #026AA1; + --ox-color-light-blue: #34bdd8; + --ox-color-neutral-light-blue: #0dc0dc; + --ox-color-tangerine: #ffbd3e; + --ox-color-gray: #5e5e5e; + --ox-color-medium-gray: #646464; + --ox-color-dark-gray: #757575; + --ox-color-darker-gray: #4c4c4c; + --ox-color-pale: #d5d5d5; + --ox-color-light: #e4e4e4; + --ox-color-white: #ffffff; + --ox-color-neutral-lightest: #f9f9f9; + --ox-color-neutral-cool: #f6f7f8; + --ox-color-neutral-bright: #f5f5f5; + --ox-color-neutral-lighter: #f1f1f1; + --ox-color-neutral-light: #e5e5e5; + --ox-color-neutral-medium: #a0a0a0; + --ox-color-neutral: #818181; + --ox-color-neutral-thin: #6f6f6f; + --ox-color-neutral-dark: #5f6163; + --ox-color-neutral-feedback: #555; + --ox-color-neutral-darker: #424242; + --ox-color-black: #000000; + --ox-color-orange: #D4450C; + --ox-color-dark-orange: #be3c08; + --ox-color-darker-orange: #b03808; + + /* link colours — src/theme.ts */ + --ox-color-link: #026AA1; + --ox-color-link-hover: #005481; + + /* z-index scale — src/theme.ts */ + --ox-z-index-navbar: 10; + --ox-z-index-sidebar: 20; + --ox-z-index-modals: 30; + --ox-z-index-toasts: 40; + + /* padding — src/theme.ts */ + --ox-padding-navbar-mobile: 1.6rem; + --ox-padding-navbar-desktop: 3.2rem; +} diff --git a/src/theme/themeCss.ts b/src/theme/themeCss.ts new file mode 100644 index 000000000..0c3dc458b --- /dev/null +++ b/src/theme/themeCss.ts @@ -0,0 +1,69 @@ +import { palette } from "./palette"; +import { colors, padding, zIndex } from "../theme"; + +const kebab = (key: string) => key.replace(/([a-z0-9])([A-Z])/g, "$1-$2").toLowerCase(); + +type Token = readonly [name: string, value: string]; + +/** + * The tokens theme.css is made of, grouped the way the file is laid out. This is the + * single description of the projection from the JavaScript theme into CSS custom + * properties: `renderThemeCss` writes it out, and `tokens.spec.ts` checks the committed + * file still matches. Nothing reads the CSS to decide what the tokens are. + */ +export const themeTokenGroups: ReadonlyArray<{ label: string; tokens: Token[] }> = [ + { + label: "palette — src/theme/palette.ts", + tokens: Object.entries(palette).map(([key, value]) => [`--ox-color-${kebab(key)}`, value] as const), + }, + { + label: "link colours — src/theme.ts", + tokens: [ + ["--ox-color-link", colors.link.color], + ["--ox-color-link-hover", colors.link.hover], + ], + }, + { + label: "z-index scale — src/theme.ts", + tokens: Object.entries(zIndex).map(([key, value]) => [`--ox-z-index-${kebab(key)}`, String(value)] as const), + }, + { + label: "padding — src/theme.ts", + tokens: [ + ["--ox-padding-navbar-mobile", `${padding.navbar.mobile}rem`], + ["--ox-padding-navbar-desktop", `${padding.navbar.desktop}rem`], + ], + }, +]; + +/** Every token as a flat name -> value map. */ +export const themeTokens = (): Map => + new Map(themeTokenGroups.flatMap((group) => group.tokens)); + +const header = `/* + * Global theme tokens. + * + * GENERATED FILE — do not edit by hand. Run \`npm run generate:theme-css\`. + * + * Projected from the JavaScript theme, which stays the single source of truth: + * src/theme/palette.ts (colours) and src/theme.ts (link colours, z-index, padding). + * src/theme/tokens.spec.ts fails if this file is out of date, so add the value to the + * JS theme and regenerate rather than editing here. + * + * Component CSS should reference these rather than repeating a literal. Where a + * component exposes its own --component-* override hook, use the token as the + * fallback: var(--tabs-border-color, var(--ox-color-pale)). + * + * Imported for side effects by every component whose CSS depends on it, the same + * way component stylesheets are imported. Consumers need do nothing. + */`; + +export const renderThemeCss = () => { + const body = themeTokenGroups + .map(({ label, tokens }) => + [` /* ${label} */`, ...tokens.map(([name, value]) => ` ${name}: ${value};`)].join("\n") + ) + .join("\n\n"); + + return `${header}\n:root {\n${body}\n}\n`; +}; diff --git a/src/theme/tokens.spec.ts b/src/theme/tokens.spec.ts new file mode 100644 index 000000000..6fe88b9ac --- /dev/null +++ b/src/theme/tokens.spec.ts @@ -0,0 +1,336 @@ +import fs from 'fs'; +import path from 'path'; +import { + colorKey, describeColor, FoundColor, opaqueKey, stripNoise, stylesheetColors, +} from './cssColors'; +import { renderThemeCss, themeTokens } from './themeCss'; + +const srcDir = path.join(__dirname, '..'); +const themeCssPath = path.join(__dirname, 'theme.css'); + +/** + * Colours that appear in component CSS but are deliberately not theme values. Anything + * here is a value we inherited from the styled-components originals and chose not to snap + * to the nearest palette entry, because doing so would be a visual change rather than a + * refactor. Adding to this list should be a deliberate act — prefer adding the colour to + * palette.ts if it is really part of the design. + * + * Keyed by the form the checker computes: `colorKey` for a colour it can resolve, or the + * whitespace-collapsed literal for one it cannot. See `allowlistKey` below. + * + * Translucent colours do not need an entry when their opaque channels are a theme value — + * `rgba(0, 0, 0, 0.2)` is black at 20% and passes on its own. That rule is what lets + * shadows and overlays stay readable without allowlisting every alpha we happen to use, + * while still refusing a new hue smuggled in through rgba(). + */ +const KNOWN_OFF_PALETTE = new Map([ + ['#cccccc', 'Tooltip border and the uncontrolled-form h3 rule. Predates the palette; nearest entry is pale (#d5d5d5).'], + ['#dddddd', 'Modal header bottom rule. Predates the palette; nearest entry is pale (#d5d5d5).'], +]); + +/** + * How a colour is looked up in KNOWN_OFF_PALETTE. + * + * A resolvable colour is keyed by its channels, so the entry covers every spelling of it + * at once. One we cannot resolve has no channels to key by, so it falls back to the + * literal as written — meaning `hsl()` and friends have to be allowlisted per spelling, + * which is the right amount of friction for a value the checker cannot reason about. + */ +const allowlistKey = ({ literal, rgba }: FoundColor) => + rgba === null ? literal.replace(/\s+/g, ' ').trim().toLowerCase() : colorKey(rgba); + +const walk = (dir: string, out: string[] = []): string[] => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + walk(full, out); + } else if (entry.name.endsWith('.css')) { + out.push(full); + } + } + return out; +}; + +const here = path.basename(__filename); + +/** + * Every colour the theme defines, as token name -> the value written in the JS theme. + * + * Read off the projection in themeCss.ts rather than re-derived from palette.ts and + * theme.ts, so the set the checks below run over is by construction the set theme.css is + * generated from. Restating the list here is what let `--ox-color-link` go missing: the + * generator had it, this file did not, and nothing tied the two together. + */ +const themeColors: ReadonlyArray = [...themeTokens()].filter( + ([name]) => name.startsWith('--ox-color-') +); + +/** + * Theme colours describeColor cannot reduce to channels. Asserted empty below rather than + * cast away: such an entry would drop out of themeValues, and the colour would then read + * as off-palette everywhere it is used — a confusing failure a long way from its cause. + * + * Taken as a function of the entries so that the guard itself can be tested against a + * malformed value, rather than only ever being run over a theme that happens to be sound. + */ +const unresolvableColors = (entries: ReadonlyArray) => entries + .filter(([, value]) => describeColor(value) === null) + .map(([token, value]) => `${token}: ${value}`); + +const unresolvableThemeColors = unresolvableColors(themeColors); + +/** + * Every theme colour, by opaque channels, so a literal can be traced back to its token. + * + * A value can carry more than one token — `--ox-color-link` and `--ox-color-medium-blue` + * are both #026AA1 — so every token holding a value is kept. Reporting all of them lets + * the author pick the one that says what they mean, rather than being sent to whichever + * entry happened to be written last. + */ +const themeValues = themeColors.reduce((byValue, [token, value]) => { + const rgba = describeColor(value); + if (rgba !== null) { + const key = opaqueKey(rgba); + byValue.set(key, [...(byValue.get(key) ?? []), token]); + } + return byValue; +}, new Map()); + +/** Everything wrong with the colours in one stylesheet. Empty means the file is clean. */ +const colorProblems = (css: string): string[] => { + const problems: string[] = []; + + for (const found of stylesheetColors(css)) { + const { literal, rgba } = found; + const key = allowlistKey(found); + + if (KNOWN_OFF_PALETTE.has(key)) { continue; } + + if (rgba === null) { + problems.push( + `"${literal}" is a colour this check cannot resolve — build it from a theme token, or add "${key}" to KNOWN_OFF_PALETTE in ${here} with a reason` + ); + continue; + } + + const hex = opaqueKey(rgba); + const tokens = themeValues.get(hex); + + if (rgba.a < 1) { + // An alpha variant of a theme colour is fine — there is no token form for it. + if (!tokens) { + problems.push( + `"${literal}" is translucent and its channels (${hex}) are not a theme value — add ${hex} to palette.ts, or "${key}" to KNOWN_OFF_PALETTE in ${here} with a reason` + ); + } + } else if (tokens) { + problems.push( + `${literal} duplicates the theme — use ${tokens.map((name) => `var(${name})`).join(' or ')}` + ); + } else { + problems.push( + `${literal} is not a theme value — add it to palette.ts, or to KNOWN_OFF_PALETTE in ${here} with a reason` + ); + } + } + + return problems; +}; + +/** --ox-* tokens a stylesheet reads but theme.css does not define. */ +const unknownTokenReferences = (css: string, defined: Map) => [ + ...new Set( + [...stripNoise(css).matchAll(/var\(\s*(--ox-[\w-]+)/g)] + .map((match) => match[1]) + .filter((name) => !defined.has(name)) + ), +]; + +describe('theme.css', () => { + it('is what the generator produces from the JS theme', () => { + // theme.css is generated, not written: `npm run generate:theme-css`. If this fails, + // the JS theme moved and the committed CSS did not — regenerate rather than editing + // theme.css by hand. + expect(fs.readFileSync(themeCssPath, 'utf8')).toEqual(renderThemeCss()); + }); +}); + +/** + * The checker below is only worth anything if it fails on the things it claims to fail on. + * These cases are the contract: every colour syntax reaches the palette check, and the + * ways of writing a colour that are legitimately fine stay quiet. + * + * The parsing underneath is covered in cssColors.spec.ts. What is tested here is the layer + * this file adds: which colours the ui-components palette recognises, and what an author + * is told about the ones it does not. + */ +describe('the colour check itself', () => { + const rule = (declaration: string) => colorProblems(`.x { ${declaration} }`); + + it.each([ + ['hex', 'color: #d5d5d5;', '--ox-color-pale'], + ['short hex', 'color: #FFF;', '--ox-color-white'], + ['named colour', 'color: white;', '--ox-color-white'], + ['named colour in a shorthand', 'border: 1px solid whitesmoke;', '--ox-color-neutral-bright'], + ['functional rgb', 'color: rgb(213, 213, 213);', '--ox-color-pale'], + ['space-separated rgb', 'color: rgb(213 213 213 / 100%);', '--ox-color-pale'], + ['percentage rgb', 'color: rgb(100%, 100%, 100%);', '--ox-color-white'], + ['hex in a var() fallback', 'color: var(--thing, #d5d5d5);', '--ox-color-pale'], + ['colour in a gradient stop', 'background: linear-gradient(to right, #d5d5d5, transparent);', '--ox-color-pale'], + ['named colour in a custom property', '--tabs-border-color: whitesmoke;', '--ox-color-neutral-bright'], + ['named colour in box-shadow', 'box-shadow: 0 0 0.2rem white;', '--ox-color-white'], + ['named colour in a vendor-prefixed property', '-webkit-text-fill-color: white;', '--ox-color-white'], + ['hex outside a colour property', 'animation-name: #d5d5d5;', '--ox-color-pale'], + ])('flags a %s that duplicates a token', (_case, declaration, token) => { + expect(rule(declaration)).toEqual([expect.stringContaining(`use var(${token})`)]); + }); + + it.each([ + ['hex', 'color: #123456;'], + ['named colour', 'color: tan;'], + ['named colour in a longhand', 'color: red;'], + ['named colour in a shorthand', 'border: 1px solid red;'], + ['named colour in a gradient', 'background: linear-gradient(to right, tan, transparent);'], + ['rgb', 'color: rgb(1, 2, 3);'], + ['hsl', 'color: hsl(200 50% 50%);'], + ['oklch', 'color: oklch(70% 0.1 200);'], + ['color()', 'color: color(display-p3 1 0 0);'], + ['translucent off-palette colour', 'background: rgba(1, 2, 3, 0.5);'], + // rgb() may legally hold var() channels, but then we cannot tell what colour it is; + // flagging beats skipping, which would let an off-palette value through unchecked. + ['rgb() with var() channels', 'background: rgba(var(--channels), 0.2);'], + ])('flags an untokenised %s', (_case, declaration) => { + expect(rule(declaration)).toHaveLength(1); + }); + + it.each([ + ['a token reference', 'color: var(--ox-color-pale);'], + ['a nested token fallback', 'color: var(--tabs-border-color, var(--ox-color-pale));'], + ['color-mix over tokens', 'background: color-mix(in srgb, var(--ox-color-black) 20%, transparent);'], + ['transparent', 'background: transparent;'], + ['currentcolor', 'border-color: currentcolor;'], + ['a system colour', 'outline: 0.2rem auto Highlight;'], + ['an allowlisted colour', 'border-color: #ccc;'], + ['alpha over a theme colour', 'box-shadow: 0 0 0.2rem rgba(0, 0, 0, 0.2);'], + ['a keyword that merely contains a colour name', 'animation-name: moveblue;'], + ['an animation named after a colour', 'animation-name: red;'], + ['a font named after a colour', 'font-family: white;'], + ['a grid area named after a colour', 'grid-area: gold;'], + ['a non-colour value', 'filter: grayscale(1);'], + ])('stays quiet for %s', (_case, declaration) => { + expect(rule(declaration)).toEqual([]); + }); + + it('ignores colour-shaped text outside declaration values', () => { + expect(colorProblems('.red { }')).toEqual([]); + expect(colorProblems('.x { content: "tan"; }')).toEqual([]); + expect(colorProblems('.x { /* #d5d5d5 */ color: var(--ox-color-pale); }')).toEqual([]); + }); + + it('checks declarations nested in at-rules', () => { + const css = '@media screen and (min-width: 75em) { .x { color: #d5d5d5; } }'; + expect(colorProblems(css)).toEqual([expect.stringContaining('use var(--ox-color-pale)')]); + }); + + it('can reduce every theme colour to channels', () => { + // Guards the themeValues map: see unresolvableThemeColors above for why a silent drop + // would be worse than a failure here. + expect(unresolvableThemeColors).toEqual([]); + }); + + it('would fail if a theme colour were malformed', () => { + // The guard above only means something if it can fail. `#ggg` is the case that used to + // slip through it: expanded to six characters it looked like a colour, so it entered + // themeValues under a key nothing could ever match. + expect(unresolvableColors([['--ox-color-bad', '#ggg']])).toEqual(['--ox-color-bad: #ggg']); + }); + + it('checks every colour token the theme projects, semantic ones included', () => { + // themeColors is derived from the projection, so this cannot drift the way the + // hand-written list did — --ox-color-link was absent from it, leaving the link colour + // outside both the resolvability guard and the duplicate check. + expect(themeColors.map(([name]) => name)).toEqual( + [...themeTokens().keys()].filter((name) => name.startsWith('--ox-color-')) + ); + expect(themeColors.map(([name]) => name)).toEqual( + expect.arrayContaining(['--ox-color-link', '--ox-color-link-hover']) + ); + }); + + it('names every token that carries a colour when more than one does', () => { + // #026AA1 is both palette.mediumBlue and colors.link.color. Naming only one would send + // half the authors who hit this to a token that does not say what they mean. + expect(rule('color: #026AA1;')).toEqual([ + '#026AA1 duplicates the theme — use var(--ox-color-medium-blue) or var(--ox-color-link)', + ]); + }); + + it('flags a reference to a token that does not exist', () => { + const defined = themeTokens(); + expect(unknownTokenReferences('.x { color: var(--ox-color-pale); }', defined)).toEqual([]); + expect(unknownTokenReferences('.x { color: var(--ox-color-palee); }', defined)) + .toEqual(['--ox-color-palee']); + }); +}); + +/** + * Stylesheets migrated before the tokens existed, still carrying hand-copied literals. + * Each is removed by the PR that sweeps it; the list is expected to reach empty, at which + * point it and the assertion below go away with it. + * + * The point of listing them rather than skipping the check is that the list is asserted to + * be *exactly* the failing set, so it cannot rot in either direction: dropping a name + * without sweeping the file fails, and sweeping a file without dropping its name fails too. + */ +const PENDING_SWEEP = new Set([ + 'components/Button.css', + 'components/ButtonBar.css', + 'components/Checkbox/Checkbox.css', + 'components/CloseModalButton.css', + 'components/DropdownMenu.css', + 'components/Modal.css', + 'components/NavBar.css', + 'components/NavBar.stories.css', + 'components/NavBarMenuButtons.css', + 'components/Overlay.css', + 'components/Radio.css', + 'components/Tabs.css', + 'components/Text.css', + 'components/Toast.css', + 'components/Tooltip.css', + 'components/forms/uncontrolled/inputTypes.css', +]); + +describe('component CSS', () => { + const cssFiles = walk(srcDir).filter((file) => file !== themeCssPath); + const tokens = themeTokens(); + const name = (file: string) => path.relative(srcDir, file); + + it('has files to check', () => { + // Guards against the walk silently finding nothing and the suite passing vacuously. + expect(cssFiles.length).toBeGreaterThan(0); + }); + + it('lists exactly the stylesheets still awaiting the sweep', () => { + const failing = cssFiles + .filter((file) => colorProblems(fs.readFileSync(file, 'utf8')).length > 0) + .map(name); + expect(failing.sort()).toEqual([...PENDING_SWEEP].sort()); + }); + + it.each( + cssFiles.filter((file) => !PENDING_SWEEP.has(name(file))).map((file) => [name(file), file]) + )( + '%s uses tokens rather than repeating theme values', + (_name, file) => { + expect(colorProblems(fs.readFileSync(file, 'utf8'))).toEqual([]); + } + ); + + it.each(cssFiles.map((file) => [path.relative(srcDir, file), file]))( + '%s only references tokens that exist', + (_name, file) => { + expect(unknownTokenReferences(fs.readFileSync(file, 'utf8'), tokens)).toEqual([]); + } + ); +}); From f6592ab03947c295b1a442777e3b0d23a843a79d Mon Sep 17 00:00:00 2001 From: Roy Johnson Date: Thu, 10 Sep 2026 18:56:41 +0000 Subject: [PATCH 2/9] rename button variant colors css variables, too (Roy's cbf780d from #143, theme side. The Button.css and DropdownMenu.css half of the same commit moves with the sweep, since those files are not touched until then. theme.css is regenerated rather than hand-edited.) --- src/theme/buttons.ts | 8 ++++---- src/theme/palette.ts | 10 ++++++---- src/theme/theme.css | 8 ++++---- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/theme/buttons.ts b/src/theme/buttons.ts index 882c7762f..aa821eca3 100644 --- a/src/theme/buttons.ts +++ b/src/theme/buttons.ts @@ -19,8 +19,8 @@ const asButtonStyleSetTypes = (st: { [K in keyof T]: ButtonStyleSet }) => st; const buttonStyleSets = asButtonStyleSetTypes({ primary: { background: palette.orange, - backgroundActive: palette.darkerOrange, - backgroundHover: palette.darkOrange, + backgroundActive: palette.orangeActive, + backgroundHover: palette.orangeHover, color: palette.white, outline: palette.white, shadow: palette.black, @@ -36,8 +36,8 @@ const buttonStyleSets = asButtonStyleSetTypes({ }, secondary: { background: palette.darkGray, - backgroundActive: palette.darkerGray, - backgroundHover: palette.mediumGray, + backgroundActive: palette.darkGrayActive, + backgroundHover: palette.darkGrayHover, color: palette.white, outline: palette.white, shadow: palette.black, diff --git a/src/theme/palette.ts b/src/theme/palette.ts index f4881551e..d4d213a91 100644 --- a/src/theme/palette.ts +++ b/src/theme/palette.ts @@ -18,9 +18,10 @@ export const palette = { neutralLightBlue: "#0dc0dc", tangerine: "#ffbd3e", gray: "#5e5e5e", - mediumGray: "#646464", // darkGray, hover darkGray: "#757575", - darkerGray: "#4c4c4c", // darkGray, active + // button variants + darkGrayHover: "#646464", + darkGrayActive: "#4c4c4c", pale: "#d5d5d5", light: "#e4e4e4", white: "#ffffff", @@ -37,6 +38,7 @@ export const palette = { neutralDarker: "#424242", // very dark gray black: "#000000", orange: "#D4450C", - darkOrange: "#be3c08", // orange, hover - darkerOrange: "#b03808" // orange, active + // button variants + orangeHover: "#be3c08", + orangeActive: "#b03808" } as const; diff --git a/src/theme/theme.css b/src/theme/theme.css index 6e9c5ce6c..4df8ef3b9 100644 --- a/src/theme/theme.css +++ b/src/theme/theme.css @@ -36,9 +36,9 @@ --ox-color-neutral-light-blue: #0dc0dc; --ox-color-tangerine: #ffbd3e; --ox-color-gray: #5e5e5e; - --ox-color-medium-gray: #646464; --ox-color-dark-gray: #757575; - --ox-color-darker-gray: #4c4c4c; + --ox-color-dark-gray-hover: #646464; + --ox-color-dark-gray-active: #4c4c4c; --ox-color-pale: #d5d5d5; --ox-color-light: #e4e4e4; --ox-color-white: #ffffff; @@ -55,8 +55,8 @@ --ox-color-neutral-darker: #424242; --ox-color-black: #000000; --ox-color-orange: #D4450C; - --ox-color-dark-orange: #be3c08; - --ox-color-darker-orange: #b03808; + --ox-color-orange-hover: #be3c08; + --ox-color-orange-active: #b03808; /* link colours — src/theme.ts */ --ox-color-link: #026AA1; From df49bac4e6c7b324fc9642ba629b0f16e6b27735 Mon Sep 17 00:00:00 2001 From: OpenStaxClaude Date: Thu, 10 Sep 2026 18:57:00 +0000 Subject: [PATCH 3/9] CORE-2720: document the PENDING_SWEEP exemption in the README The "what CI enforces" list read as though the duplicate-literal check covered every stylesheet, which it will not until the sweep lands. Says which files are exempt, why the list cannot drift, and that new stylesheets are not to be added to it. Co-Authored-By: Claude Opus 5 --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index 1a5c372c1..68c127849 100644 --- a/README.md +++ b/README.md @@ -135,6 +135,12 @@ token form for it — which still refuses a new hue smuggled in through `rgba()` The check has its own tests, so the guarantee is a tested one rather than an asserted one. +The stylesheets migrated before the tokens existed are listed in `PENDING_SWEEP` in +`tokens.spec.ts` and are exempt from the duplicate-literal check until they are swept. The +list is asserted to be exactly the set of files that still fail, so it cannot drift: you +cannot exempt a clean file, and you cannot sweep a file without removing it from the list. +Do not add to it — new stylesheets are expected to use the tokens from the start. + Adding a genuinely new colour is therefore a deliberate act: put it in `palette.ts` if it is part of the design, or in the allowlist with a reason if it is a one-off we are keeping. From 8ed36c2f3798bb0010dc0c1cb242faffbf0cb5ec Mon Sep 17 00:00:00 2001 From: OpenStaxClaude Date: Thu, 10 Sep 2026 19:40:16 +0000 Subject: [PATCH 4/9] CORE-2720: run the off-palette check over pending-sweep files too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PENDING_SWEEP exempted a file from every colorProblems finding, not only the duplicate literals it was listed for. The exactness assertion only asked whether each file had at least one problem, so a new off-palette colour added to an already-failing file left the failing set unchanged and the suite passed — enforcement rule 3 had a hole exactly where the migrated stylesheets are. colorProblems now returns { duplicates, offPalette }. The off-palette check runs over every stylesheet; PENDING_SWEEP defers duplicates only, and the exactness assertion is over the duplicate findings. Verified both directions: appending `color: #ab12cd` to Button.css passed on the previous spec and fails on this one. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 10 +++-- src/theme/tokens.spec.ts | 88 +++++++++++++++++++++++++++++++--------- 2 files changed, 75 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 68c127849..610937e78 100644 --- a/README.md +++ b/README.md @@ -136,10 +136,12 @@ token form for it — which still refuses a new hue smuggled in through `rgba()` The check has its own tests, so the guarantee is a tested one rather than an asserted one. The stylesheets migrated before the tokens existed are listed in `PENDING_SWEEP` in -`tokens.spec.ts` and are exempt from the duplicate-literal check until they are swept. The -list is asserted to be exactly the set of files that still fail, so it cannot drift: you -cannot exempt a clean file, and you cannot sweep a file without removing it from the list. -Do not add to it — new stylesheets are expected to use the tokens from the start. +`tokens.spec.ts` and are exempt from the duplicate-literal check until they are swept — +from that check only. A colour the theme does not have is still refused in those files, so +the list defers work already owed rather than opening a gap. The list is asserted to be +exactly the set of files that still carry duplicates, so it cannot drift: you cannot exempt +a clean file, and you cannot sweep a file without removing it from the list. Do not add to +it — new stylesheets are expected to use the tokens from the start. Adding a genuinely new colour is therefore a deliberate act: put it in `palette.ts` if it is part of the design, or in the allowlist with a reason if it is a one-off we are keeping. diff --git a/src/theme/tokens.spec.ts b/src/theme/tokens.spec.ts index 6fe88b9ac..2873eebf4 100644 --- a/src/theme/tokens.spec.ts +++ b/src/theme/tokens.spec.ts @@ -96,9 +96,25 @@ const themeValues = themeColors.reduce((byValue, [token, value]) => { return byValue; }, new Map()); -/** Everything wrong with the colours in one stylesheet. Empty means the file is clean. */ -const colorProblems = (css: string): string[] => { - const problems: string[] = []; +/** + * Everything wrong with the colours in one stylesheet, split by what it would take to fix. + * + * `duplicates` are literals the theme already holds a token for: a mechanical swap that + * changes nothing on screen, and the only kind of finding PENDING_SWEEP defers. + * + * `offPalette` are colours the theme does not have at all, including the ones the checker + * cannot resolve. Introducing one is a design decision rather than a missed swap, so it is + * refused in every stylesheet — a file awaiting the sweep is no more entitled to a new + * colour than a clean one. Keeping the two apart is what stops a pending file from + * smuggling one in under cover of the literals it is already known to carry. + * + * Both empty means the file is clean. + */ +interface ColorProblems { duplicates: string[]; offPalette: string[] } + +const colorProblems = (css: string): ColorProblems => { + const duplicates: string[] = []; + const offPalette: string[] = []; for (const found of stylesheetColors(css)) { const { literal, rgba } = found; @@ -107,7 +123,7 @@ const colorProblems = (css: string): string[] => { if (KNOWN_OFF_PALETTE.has(key)) { continue; } if (rgba === null) { - problems.push( + offPalette.push( `"${literal}" is a colour this check cannot resolve — build it from a theme token, or add "${key}" to KNOWN_OFF_PALETTE in ${here} with a reason` ); continue; @@ -119,22 +135,28 @@ const colorProblems = (css: string): string[] => { if (rgba.a < 1) { // An alpha variant of a theme colour is fine — there is no token form for it. if (!tokens) { - problems.push( + offPalette.push( `"${literal}" is translucent and its channels (${hex}) are not a theme value — add ${hex} to palette.ts, or "${key}" to KNOWN_OFF_PALETTE in ${here} with a reason` ); } } else if (tokens) { - problems.push( + duplicates.push( `${literal} duplicates the theme — use ${tokens.map((name) => `var(${name})`).join(' or ')}` ); } else { - problems.push( + offPalette.push( `${literal} is not a theme value — add it to palette.ts, or to KNOWN_OFF_PALETTE in ${here} with a reason` ); } } - return problems; + return { duplicates, offPalette }; +}; + +/** Both kinds of finding together, for the cases where the distinction does not matter. */ +const allColorProblems = (css: string): string[] => { + const { duplicates, offPalette } = colorProblems(css); + return [...duplicates, ...offPalette]; }; /** --ox-* tokens a stylesheet reads but theme.css does not define. */ @@ -165,7 +187,7 @@ describe('theme.css', () => { * is told about the ones it does not. */ describe('the colour check itself', () => { - const rule = (declaration: string) => colorProblems(`.x { ${declaration} }`); + const rule = (declaration: string) => allColorProblems(`.x { ${declaration} }`); it.each([ ['hex', 'color: #d5d5d5;', '--ox-color-pale'], @@ -222,14 +244,28 @@ describe('the colour check itself', () => { }); it('ignores colour-shaped text outside declaration values', () => { - expect(colorProblems('.red { }')).toEqual([]); - expect(colorProblems('.x { content: "tan"; }')).toEqual([]); - expect(colorProblems('.x { /* #d5d5d5 */ color: var(--ox-color-pale); }')).toEqual([]); + expect(allColorProblems('.red { }')).toEqual([]); + expect(allColorProblems('.x { content: "tan"; }')).toEqual([]); + expect(allColorProblems('.x { /* #d5d5d5 */ color: var(--ox-color-pale); }')).toEqual([]); }); it('checks declarations nested in at-rules', () => { const css = '@media screen and (min-width: 75em) { .x { color: #d5d5d5; } }'; - expect(colorProblems(css)).toEqual([expect.stringContaining('use var(--ox-color-pale)')]); + expect(allColorProblems(css)).toEqual([expect.stringContaining('use var(--ox-color-pale)')]); + }); + + it('sorts a finding by whether the theme already has the colour', () => { + // The split is what PENDING_SWEEP keys off, so it is worth stating directly: a file + // may be excused the literals it copied from the theme, never a colour the theme + // does not have. Both kinds in one stylesheet, to show neither absorbs the other. + const css = '.x { color: #d5d5d5; border-color: #123456; background: hsl(200 50% 50%); }'; + expect(colorProblems(css)).toEqual({ + duplicates: [expect.stringContaining('use var(--ox-color-pale)')], + offPalette: [ + expect.stringContaining('#123456 is not a theme value'), + expect.stringContaining('cannot resolve'), + ], + }); }); it('can reduce every theme colour to channels', () => { @@ -278,9 +314,14 @@ describe('the colour check itself', () => { * Each is removed by the PR that sweeps it; the list is expected to reach empty, at which * point it and the assertion below go away with it. * + * The exemption is narrow: only the duplicate-literal check. The off-palette check below + * runs over these files too, so being on this list defers a swap that is already owed and + * grants nothing else — a new colour in one of them fails exactly as it would anywhere. + * * The point of listing them rather than skipping the check is that the list is asserted to - * be *exactly* the failing set, so it cannot rot in either direction: dropping a name - * without sweeping the file fails, and sweeping a file without dropping its name fails too. + * be *exactly* the set with duplicates left, so it cannot rot in either direction: dropping + * a name without sweeping the file fails, and sweeping a file without dropping its name + * fails too. */ const PENDING_SWEEP = new Set([ 'components/Button.css', @@ -311,23 +352,32 @@ describe('component CSS', () => { expect(cssFiles.length).toBeGreaterThan(0); }); - it('lists exactly the stylesheets still awaiting the sweep', () => { + it('lists exactly the stylesheets whose literals still duplicate the theme', () => { const failing = cssFiles - .filter((file) => colorProblems(fs.readFileSync(file, 'utf8')).length > 0) + .filter((file) => colorProblems(fs.readFileSync(file, 'utf8')).duplicates.length > 0) .map(name); expect(failing.sort()).toEqual([...PENDING_SWEEP].sort()); }); + it.each(cssFiles.map((file) => [name(file), file]))( + '%s introduces no colour the theme does not have', + (_name, file) => { + // Every stylesheet, PENDING_SWEEP included: the exemption is for literals that + // duplicate a token, not a licence to add a colour while the file waits its turn. + expect(colorProblems(fs.readFileSync(file, 'utf8')).offPalette).toEqual([]); + } + ); + it.each( cssFiles.filter((file) => !PENDING_SWEEP.has(name(file))).map((file) => [name(file), file]) )( '%s uses tokens rather than repeating theme values', (_name, file) => { - expect(colorProblems(fs.readFileSync(file, 'utf8'))).toEqual([]); + expect(colorProblems(fs.readFileSync(file, 'utf8')).duplicates).toEqual([]); } ); - it.each(cssFiles.map((file) => [path.relative(srcDir, file), file]))( + it.each(cssFiles.map((file) => [name(file), file]))( '%s only references tokens that exist', (_name, file) => { expect(unknownTokenReferences(fs.readFileSync(file, 'utf8'), tokens)).toEqual([]); From 288fb5712be5d56791f905132607fc28382d8f61 Mon Sep 17 00:00:00 2001 From: OpenStaxClaude Date: Thu, 10 Sep 2026 21:40:20 +0000 Subject: [PATCH 5/9] CORE-2720: match var() case-insensitively in the unknown-token check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CSS function names are ASCII case-insensitive, so `VAR(--ox-color-palee)` is a real reference to a token nothing defines. The regex only knew the lowercase spelling, so that typo fell through to its fallback unreported — precisely what check 4 exists to catch. The lookup stays case-sensitive: custom property names are, so `var(--OX-color-pale)` is genuinely undefined and is now reported rather than skipped. Both spellings are covered by tests, which fail against the old regex. The colour checker itself was already sound here — cssColors lowercases function names before dispatching — but `VAR()` cases are added alongside the lowercase ones so that stays pinned from this file too. Co-Authored-By: Claude Opus 5 (1M context) --- src/theme/tokens.spec.ts | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/src/theme/tokens.spec.ts b/src/theme/tokens.spec.ts index 2873eebf4..8d24c1000 100644 --- a/src/theme/tokens.spec.ts +++ b/src/theme/tokens.spec.ts @@ -159,10 +159,19 @@ const allColorProblems = (css: string): string[] => { return [...duplicates, ...offPalette]; }; -/** --ox-* tokens a stylesheet reads but theme.css does not define. */ +/** + * --ox-* tokens a stylesheet reads but theme.css does not define. + * + * The match is case-insensitive because CSS function names are: `VAR(--ox-color-pale)` is + * the same reference as `var(--ox-color-pale)`, and a check that only knew the lowercase + * spelling would let a typo through in the other one. The lookup stays case-sensitive, + * because custom property *names* are — `var(--OX-color-pale)` really is a reference to + * something nothing defines, and silently falling through to its fallback is the failure + * this check exists to catch. + */ const unknownTokenReferences = (css: string, defined: Map) => [ ...new Set( - [...stripNoise(css).matchAll(/var\(\s*(--ox-[\w-]+)/g)] + [...stripNoise(css).matchAll(/var\(\s*(--ox-[\w-]+)/gi)] .map((match) => match[1]) .filter((name) => !defined.has(name)) ), @@ -198,6 +207,7 @@ describe('the colour check itself', () => { ['space-separated rgb', 'color: rgb(213 213 213 / 100%);', '--ox-color-pale'], ['percentage rgb', 'color: rgb(100%, 100%, 100%);', '--ox-color-white'], ['hex in a var() fallback', 'color: var(--thing, #d5d5d5);', '--ox-color-pale'], + ['hex in an uppercase var() fallback', 'color: VAR(--thing, #d5d5d5);', '--ox-color-pale'], ['colour in a gradient stop', 'background: linear-gradient(to right, #d5d5d5, transparent);', '--ox-color-pale'], ['named colour in a custom property', '--tabs-border-color: whitesmoke;', '--ox-color-neutral-bright'], ['named colour in box-shadow', 'box-shadow: 0 0 0.2rem white;', '--ox-color-white'], @@ -228,6 +238,7 @@ describe('the colour check itself', () => { it.each([ ['a token reference', 'color: var(--ox-color-pale);'], ['a nested token fallback', 'color: var(--tabs-border-color, var(--ox-color-pale));'], + ['an uppercase token reference', 'color: VAR(--ox-color-pale);'], ['color-mix over tokens', 'background: color-mix(in srgb, var(--ox-color-black) 20%, transparent);'], ['transparent', 'background: transparent;'], ['currentcolor', 'border-color: currentcolor;'], @@ -307,6 +318,25 @@ describe('the colour check itself', () => { expect(unknownTokenReferences('.x { color: var(--ox-color-palee); }', defined)) .toEqual(['--ox-color-palee']); }); + + it('reads a reference however the var() is spelled', () => { + // CSS function names are ASCII case-insensitive. Knowing only the lowercase spelling + // would mean an uppercase typo fell through to its fallback unreported — the one + // thing this check is for. + const defined = themeTokens(); + expect(unknownTokenReferences('.x { color: VAR(--ox-color-pale); }', defined)).toEqual([]); + expect(unknownTokenReferences('.x { color: VAR(--ox-color-palee); }', defined)) + .toEqual(['--ox-color-palee']); + expect(unknownTokenReferences('.x { color: Var( --ox-color-palee ); }', defined)) + .toEqual(['--ox-color-palee']); + }); + + it('flags a token name whose case does not match', () => { + // Property names, unlike function names, are case-sensitive: --OX-color-pale is not + // the token, so the declaration resolves to its fallback or nothing at all. + expect(unknownTokenReferences('.x { color: var(--OX-color-pale); }', themeTokens())) + .toEqual(['--OX-color-pale']); + }); }); /** From 3e48cc81e208d8dd03fa8f82e0677087ef970baf Mon Sep 17 00:00:00 2001 From: OpenStaxClaude Date: Thu, 10 Sep 2026 21:58:13 +0000 Subject: [PATCH 6/9] CORE-2720: say what the color check actually covers, and drop the u MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review points. The README claimed the check "covers every syntax a color can be written in" and reads bare names "wherever they appear". Neither is true: a bare name is read only where the property could take a color, which is deliberate and is what keeps `animation-name: red` and `font-family: white` quiet, and escaped spellings of property and function names are missed altogether (CORE-2885). Replaced with what is actually covered, what is excluded on purpose, and what is a known gap. Each claim checked against the checker rather than read off the source. Spelling swept to color throughout the files this PR owns, per review. The engine in #149 still reads colour; that PR is being merged as-is, so the sweep for cssColors.ts and its spec belongs in #143 rather than reopening it. theme.css is regenerated, not hand-edited — the wording lives in themeCss.ts. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 49 ++++++++++++------ src/theme/theme.css | 4 +- src/theme/themeCss.ts | 4 +- src/theme/tokens.spec.ts | 106 +++++++++++++++++++-------------------- 4 files changed, 91 insertions(+), 72 deletions(-) diff --git a/README.md b/README.md index 610937e78..07d51bf79 100644 --- a/README.md +++ b/README.md @@ -82,7 +82,7 @@ instead: ``` Tokens are `--ox-`-prefixed, so they will not collide with a consuming app's own variables. -Colour tokens are the kebab-case form of the `src/theme/palette.ts` key +Color tokens are the kebab-case form of the `src/theme/palette.ts` key (`palette.neutralLighter` → `--ox-color-neutral-lighter`); there are also `--ox-color-link`, `--ox-color-link-hover`, `--ox-z-index-*` and `--ox-padding-navbar-*`. @@ -107,7 +107,7 @@ from JavaScript: ``` Bind a custom property from JavaScript only when its value genuinely varies at runtime — a -variant lookup, a numeric prop, a disabled state. A static colour pushed through an inline +variant lookup, a numeric prop, a disabled state. A static color pushed through an inline style is duplication with extra steps, and it wins over the cascade in ways callers do not expect. @@ -120,30 +120,49 @@ callers can set these without a cast. - the committed `theme.css` is not what the generator produces from the JS theme (the build regenerates it; this is what stops a stale copy reaching jest, ladle or a reviewer) -- a component stylesheet writes a colour literal that duplicates a theme value -- a component stylesheet introduces a colour that is not a theme value and not in the +- a component stylesheet writes a color literal that duplicates a theme value +- a component stylesheet introduces a color that is not a theme value and not in the `KNOWN_OFF_PALETTE` allowlist - a component stylesheet reads an `--ox-*` token that does not exist -The colour check parses declarations, so it covers every syntax a colour can be written in — -hex, `rgb()`/`hsl()`/`oklch()`/`color()`, and bare named colours wherever they appear, -including inside shorthands and gradient stops. Functions that merely *contain* colours -(`var()`, `color-mix()`, the gradients) are descended into rather than treated as literals, -so building a value out of tokens stays clean. A translucent colour is accepted when its -opaque channels are a theme value — `rgba(0, 0, 0, 0.2)` is black at 20% and there is no -token form for it — which still refuses a new hue smuggled in through `rgba()`. - -The check has its own tests, so the guarantee is a tested one rather than an asserted one. +The color check parses declarations rather than grepping for hex, so it reads a value the +way the property does. What that covers: + +- Hex, and the color functions — `rgb()`, `hsl()`, `hwb()`, `lab()`, `lch()`, `oklab()`, + `oklch()`, `color()` and their `a` variants — in any declaration, whatever the property. +- A bare name such as `whitesmoke` only where the property could take a color: a property + whose name contains `color`, one of the color shorthands (`background`, `border`, + `outline`, `box-shadow`, `text-decoration`, `fill` and the rest), or a custom property, + which has no grammar to go on. `var()` keeps the gate of the property it is written in, + since its fallback is whatever that property makes of it. +- Functions that merely *contain* colors (`var()`, `color-mix()`, `light-dark()`, the + gradients) are descended into rather than read as literals, so a value built out of + tokens stays clean. The gradients and `color-mix()` open the gate on their own, because + `linear-gradient(red, blue)` is a gradient between two colors whatever it is assigned to. +- A translucent color where its opaque channels are a theme value — `rgba(0, 0, 0, 0.2)` is + black at 20% and there is no token form for it. A new hue smuggled in through `rgba()` is + still refused. + +What it does not cover, deliberately: a bare name outside a color context. `animation-name: +red`, `font-family: white` and `grid-area: gold` are identifiers that happen to spell +colors, and are not reported. + +What it does not cover, for now: only the literal ASCII spelling of a *name* is recognized. +Escapes are decoded in color names themselves, but not in property or function names, so +`c\6f lor: red` — which is `color: red` to a browser — is missed. Nothing we ship is +written that way; CORE-2885 tracks it along with the rest of the lexing work. + +The check has its own tests, so what is claimed above is pinned rather than asserted. The stylesheets migrated before the tokens existed are listed in `PENDING_SWEEP` in `tokens.spec.ts` and are exempt from the duplicate-literal check until they are swept — -from that check only. A colour the theme does not have is still refused in those files, so +from that check only. A color the theme does not have is still refused in those files, so the list defers work already owed rather than opening a gap. The list is asserted to be exactly the set of files that still carry duplicates, so it cannot drift: you cannot exempt a clean file, and you cannot sweep a file without removing it from the list. Do not add to it — new stylesheets are expected to use the tokens from the start. -Adding a genuinely new colour is therefore a deliberate act: put it in `palette.ts` if it is +Adding a genuinely new color is therefore a deliberate act: put it in `palette.ts` if it is part of the design, or in the allowlist with a reason if it is a one-off we are keeping. Breakpoints are the known gap — `@media (min-width: var(--x))` is not valid CSS, so diff --git a/src/theme/theme.css b/src/theme/theme.css index 4df8ef3b9..68f6774df 100644 --- a/src/theme/theme.css +++ b/src/theme/theme.css @@ -4,7 +4,7 @@ * GENERATED FILE — do not edit by hand. Run `npm run generate:theme-css`. * * Projected from the JavaScript theme, which stays the single source of truth: - * src/theme/palette.ts (colours) and src/theme.ts (link colours, z-index, padding). + * src/theme/palette.ts (colors) and src/theme.ts (link colors, z-index, padding). * src/theme/tokens.spec.ts fails if this file is out of date, so add the value to the * JS theme and regenerate rather than editing here. * @@ -58,7 +58,7 @@ --ox-color-orange-hover: #be3c08; --ox-color-orange-active: #b03808; - /* link colours — src/theme.ts */ + /* link colors — src/theme.ts */ --ox-color-link: #026AA1; --ox-color-link-hover: #005481; diff --git a/src/theme/themeCss.ts b/src/theme/themeCss.ts index 0c3dc458b..ceb605e7f 100644 --- a/src/theme/themeCss.ts +++ b/src/theme/themeCss.ts @@ -17,7 +17,7 @@ export const themeTokenGroups: ReadonlyArray<{ label: string; tokens: Token[] }> tokens: Object.entries(palette).map(([key, value]) => [`--ox-color-${kebab(key)}`, value] as const), }, { - label: "link colours — src/theme.ts", + label: "link colors — src/theme.ts", tokens: [ ["--ox-color-link", colors.link.color], ["--ox-color-link-hover", colors.link.hover], @@ -46,7 +46,7 @@ const header = `/* * GENERATED FILE — do not edit by hand. Run \`npm run generate:theme-css\`. * * Projected from the JavaScript theme, which stays the single source of truth: - * src/theme/palette.ts (colours) and src/theme.ts (link colours, z-index, padding). + * src/theme/palette.ts (colors) and src/theme.ts (link colors, z-index, padding). * src/theme/tokens.spec.ts fails if this file is out of date, so add the value to the * JS theme and regenerate rather than editing here. * diff --git a/src/theme/tokens.spec.ts b/src/theme/tokens.spec.ts index 8d24c1000..742a8019a 100644 --- a/src/theme/tokens.spec.ts +++ b/src/theme/tokens.spec.ts @@ -9,16 +9,16 @@ const srcDir = path.join(__dirname, '..'); const themeCssPath = path.join(__dirname, 'theme.css'); /** - * Colours that appear in component CSS but are deliberately not theme values. Anything + * Colors that appear in component CSS but are deliberately not theme values. Anything * here is a value we inherited from the styled-components originals and chose not to snap * to the nearest palette entry, because doing so would be a visual change rather than a - * refactor. Adding to this list should be a deliberate act — prefer adding the colour to + * refactor. Adding to this list should be a deliberate act — prefer adding the color to * palette.ts if it is really part of the design. * - * Keyed by the form the checker computes: `colorKey` for a colour it can resolve, or the + * Keyed by the form the checker computes: `colorKey` for a color it can resolve, or the * whitespace-collapsed literal for one it cannot. See `allowlistKey` below. * - * Translucent colours do not need an entry when their opaque channels are a theme value — + * Translucent colors do not need an entry when their opaque channels are a theme value — * `rgba(0, 0, 0, 0.2)` is black at 20% and passes on its own. That rule is what lets * shadows and overlays stay readable without allowlisting every alpha we happen to use, * while still refusing a new hue smuggled in through rgba(). @@ -29,9 +29,9 @@ const KNOWN_OFF_PALETTE = new Map([ ]); /** - * How a colour is looked up in KNOWN_OFF_PALETTE. + * How a color is looked up in KNOWN_OFF_PALETTE. * - * A resolvable colour is keyed by its channels, so the entry covers every spelling of it + * A resolvable color is keyed by its channels, so the entry covers every spelling of it * at once. One we cannot resolve has no channels to key by, so it falls back to the * literal as written — meaning `hsl()` and friends have to be allowlisted per spelling, * which is the right amount of friction for a value the checker cannot reason about. @@ -54,7 +54,7 @@ const walk = (dir: string, out: string[] = []): string[] => { const here = path.basename(__filename); /** - * Every colour the theme defines, as token name -> the value written in the JS theme. + * Every color the theme defines, as token name -> the value written in the JS theme. * * Read off the projection in themeCss.ts rather than re-derived from palette.ts and * theme.ts, so the set the checks below run over is by construction the set theme.css is @@ -66,8 +66,8 @@ const themeColors: ReadonlyArray = [...themeTokens()] ); /** - * Theme colours describeColor cannot reduce to channels. Asserted empty below rather than - * cast away: such an entry would drop out of themeValues, and the colour would then read + * Theme colors describeColor cannot reduce to channels. Asserted empty below rather than + * cast away: such an entry would drop out of themeValues, and the color would then read * as off-palette everywhere it is used — a confusing failure a long way from its cause. * * Taken as a function of the entries so that the guard itself can be tested against a @@ -80,7 +80,7 @@ const unresolvableColors = (entries: ReadonlyArray) = const unresolvableThemeColors = unresolvableColors(themeColors); /** - * Every theme colour, by opaque channels, so a literal can be traced back to its token. + * Every theme color, by opaque channels, so a literal can be traced back to its token. * * A value can carry more than one token — `--ox-color-link` and `--ox-color-medium-blue` * are both #026AA1 — so every token holding a value is kept. Reporting all of them lets @@ -97,15 +97,15 @@ const themeValues = themeColors.reduce((byValue, [token, value]) => { }, new Map()); /** - * Everything wrong with the colours in one stylesheet, split by what it would take to fix. + * Everything wrong with the colors in one stylesheet, split by what it would take to fix. * * `duplicates` are literals the theme already holds a token for: a mechanical swap that * changes nothing on screen, and the only kind of finding PENDING_SWEEP defers. * - * `offPalette` are colours the theme does not have at all, including the ones the checker + * `offPalette` are colors the theme does not have at all, including the ones the checker * cannot resolve. Introducing one is a design decision rather than a missed swap, so it is * refused in every stylesheet — a file awaiting the sweep is no more entitled to a new - * colour than a clean one. Keeping the two apart is what stops a pending file from + * color than a clean one. Keeping the two apart is what stops a pending file from * smuggling one in under cover of the literals it is already known to carry. * * Both empty means the file is clean. @@ -124,7 +124,7 @@ const colorProblems = (css: string): ColorProblems => { if (rgba === null) { offPalette.push( - `"${literal}" is a colour this check cannot resolve — build it from a theme token, or add "${key}" to KNOWN_OFF_PALETTE in ${here} with a reason` + `"${literal}" is a color this check cannot resolve — build it from a theme token, or add "${key}" to KNOWN_OFF_PALETTE in ${here} with a reason` ); continue; } @@ -133,7 +133,7 @@ const colorProblems = (css: string): ColorProblems => { const tokens = themeValues.get(hex); if (rgba.a < 1) { - // An alpha variant of a theme colour is fine — there is no token form for it. + // An alpha variant of a theme color is fine — there is no token form for it. if (!tokens) { offPalette.push( `"${literal}" is translucent and its channels (${hex}) are not a theme value — add ${hex} to palette.ts, or "${key}" to KNOWN_OFF_PALETTE in ${here} with a reason` @@ -188,50 +188,50 @@ describe('theme.css', () => { /** * The checker below is only worth anything if it fails on the things it claims to fail on. - * These cases are the contract: every colour syntax reaches the palette check, and the - * ways of writing a colour that are legitimately fine stay quiet. + * These cases are the contract: every color syntax reaches the palette check, and the + * ways of writing a color that are legitimately fine stay quiet. * * The parsing underneath is covered in cssColors.spec.ts. What is tested here is the layer - * this file adds: which colours the ui-components palette recognises, and what an author + * this file adds: which colors the ui-components palette recognizes, and what an author * is told about the ones it does not. */ -describe('the colour check itself', () => { +describe('the color check itself', () => { const rule = (declaration: string) => allColorProblems(`.x { ${declaration} }`); it.each([ ['hex', 'color: #d5d5d5;', '--ox-color-pale'], ['short hex', 'color: #FFF;', '--ox-color-white'], - ['named colour', 'color: white;', '--ox-color-white'], - ['named colour in a shorthand', 'border: 1px solid whitesmoke;', '--ox-color-neutral-bright'], + ['named color', 'color: white;', '--ox-color-white'], + ['named color in a shorthand', 'border: 1px solid whitesmoke;', '--ox-color-neutral-bright'], ['functional rgb', 'color: rgb(213, 213, 213);', '--ox-color-pale'], ['space-separated rgb', 'color: rgb(213 213 213 / 100%);', '--ox-color-pale'], ['percentage rgb', 'color: rgb(100%, 100%, 100%);', '--ox-color-white'], ['hex in a var() fallback', 'color: var(--thing, #d5d5d5);', '--ox-color-pale'], ['hex in an uppercase var() fallback', 'color: VAR(--thing, #d5d5d5);', '--ox-color-pale'], - ['colour in a gradient stop', 'background: linear-gradient(to right, #d5d5d5, transparent);', '--ox-color-pale'], - ['named colour in a custom property', '--tabs-border-color: whitesmoke;', '--ox-color-neutral-bright'], - ['named colour in box-shadow', 'box-shadow: 0 0 0.2rem white;', '--ox-color-white'], - ['named colour in a vendor-prefixed property', '-webkit-text-fill-color: white;', '--ox-color-white'], - ['hex outside a colour property', 'animation-name: #d5d5d5;', '--ox-color-pale'], + ['color in a gradient stop', 'background: linear-gradient(to right, #d5d5d5, transparent);', '--ox-color-pale'], + ['named color in a custom property', '--tabs-border-color: whitesmoke;', '--ox-color-neutral-bright'], + ['named color in box-shadow', 'box-shadow: 0 0 0.2rem white;', '--ox-color-white'], + ['named color in a vendor-prefixed property', '-webkit-text-fill-color: white;', '--ox-color-white'], + ['hex outside a color property', 'animation-name: #d5d5d5;', '--ox-color-pale'], ])('flags a %s that duplicates a token', (_case, declaration, token) => { expect(rule(declaration)).toEqual([expect.stringContaining(`use var(${token})`)]); }); it.each([ ['hex', 'color: #123456;'], - ['named colour', 'color: tan;'], - ['named colour in a longhand', 'color: red;'], - ['named colour in a shorthand', 'border: 1px solid red;'], - ['named colour in a gradient', 'background: linear-gradient(to right, tan, transparent);'], + ['named color', 'color: tan;'], + ['named color in a longhand', 'color: red;'], + ['named color in a shorthand', 'border: 1px solid red;'], + ['named color in a gradient', 'background: linear-gradient(to right, tan, transparent);'], ['rgb', 'color: rgb(1, 2, 3);'], ['hsl', 'color: hsl(200 50% 50%);'], ['oklch', 'color: oklch(70% 0.1 200);'], ['color()', 'color: color(display-p3 1 0 0);'], - ['translucent off-palette colour', 'background: rgba(1, 2, 3, 0.5);'], - // rgb() may legally hold var() channels, but then we cannot tell what colour it is; + ['translucent off-palette color', 'background: rgba(1, 2, 3, 0.5);'], + // rgb() may legally hold var() channels, but then we cannot tell what color it is; // flagging beats skipping, which would let an off-palette value through unchecked. ['rgb() with var() channels', 'background: rgba(var(--channels), 0.2);'], - ])('flags an untokenised %s', (_case, declaration) => { + ])('flags an untokenized %s', (_case, declaration) => { expect(rule(declaration)).toHaveLength(1); }); @@ -242,19 +242,19 @@ describe('the colour check itself', () => { ['color-mix over tokens', 'background: color-mix(in srgb, var(--ox-color-black) 20%, transparent);'], ['transparent', 'background: transparent;'], ['currentcolor', 'border-color: currentcolor;'], - ['a system colour', 'outline: 0.2rem auto Highlight;'], - ['an allowlisted colour', 'border-color: #ccc;'], - ['alpha over a theme colour', 'box-shadow: 0 0 0.2rem rgba(0, 0, 0, 0.2);'], - ['a keyword that merely contains a colour name', 'animation-name: moveblue;'], - ['an animation named after a colour', 'animation-name: red;'], - ['a font named after a colour', 'font-family: white;'], - ['a grid area named after a colour', 'grid-area: gold;'], - ['a non-colour value', 'filter: grayscale(1);'], + ['a system color', 'outline: 0.2rem auto Highlight;'], + ['an allowlisted color', 'border-color: #ccc;'], + ['alpha over a theme color', 'box-shadow: 0 0 0.2rem rgba(0, 0, 0, 0.2);'], + ['a keyword that merely contains a color name', 'animation-name: moveblue;'], + ['an animation named after a color', 'animation-name: red;'], + ['a font named after a color', 'font-family: white;'], + ['a grid area named after a color', 'grid-area: gold;'], + ['a non-color value', 'filter: grayscale(1);'], ])('stays quiet for %s', (_case, declaration) => { expect(rule(declaration)).toEqual([]); }); - it('ignores colour-shaped text outside declaration values', () => { + it('ignores color-shaped text outside declaration values', () => { expect(allColorProblems('.red { }')).toEqual([]); expect(allColorProblems('.x { content: "tan"; }')).toEqual([]); expect(allColorProblems('.x { /* #d5d5d5 */ color: var(--ox-color-pale); }')).toEqual([]); @@ -265,9 +265,9 @@ describe('the colour check itself', () => { expect(allColorProblems(css)).toEqual([expect.stringContaining('use var(--ox-color-pale)')]); }); - it('sorts a finding by whether the theme already has the colour', () => { + it('sorts a finding by whether the theme already has the color', () => { // The split is what PENDING_SWEEP keys off, so it is worth stating directly: a file - // may be excused the literals it copied from the theme, never a colour the theme + // may be excused the literals it copied from the theme, never a color the theme // does not have. Both kinds in one stylesheet, to show neither absorbs the other. const css = '.x { color: #d5d5d5; border-color: #123456; background: hsl(200 50% 50%); }'; expect(colorProblems(css)).toEqual({ @@ -279,22 +279,22 @@ describe('the colour check itself', () => { }); }); - it('can reduce every theme colour to channels', () => { + it('can reduce every theme color to channels', () => { // Guards the themeValues map: see unresolvableThemeColors above for why a silent drop // would be worse than a failure here. expect(unresolvableThemeColors).toEqual([]); }); - it('would fail if a theme colour were malformed', () => { + it('would fail if a theme color were malformed', () => { // The guard above only means something if it can fail. `#ggg` is the case that used to - // slip through it: expanded to six characters it looked like a colour, so it entered + // slip through it: expanded to six characters it looked like a color, so it entered // themeValues under a key nothing could ever match. expect(unresolvableColors([['--ox-color-bad', '#ggg']])).toEqual(['--ox-color-bad: #ggg']); }); - it('checks every colour token the theme projects, semantic ones included', () => { + it('checks every color token the theme projects, semantic ones included', () => { // themeColors is derived from the projection, so this cannot drift the way the - // hand-written list did — --ox-color-link was absent from it, leaving the link colour + // hand-written list did — --ox-color-link was absent from it, leaving the link color // outside both the resolvability guard and the duplicate check. expect(themeColors.map(([name]) => name)).toEqual( [...themeTokens().keys()].filter((name) => name.startsWith('--ox-color-')) @@ -304,7 +304,7 @@ describe('the colour check itself', () => { ); }); - it('names every token that carries a colour when more than one does', () => { + it('names every token that carries a color when more than one does', () => { // #026AA1 is both palette.mediumBlue and colors.link.color. Naming only one would send // half the authors who hit this to a token that does not say what they mean. expect(rule('color: #026AA1;')).toEqual([ @@ -346,7 +346,7 @@ describe('the colour check itself', () => { * * The exemption is narrow: only the duplicate-literal check. The off-palette check below * runs over these files too, so being on this list defers a swap that is already owed and - * grants nothing else — a new colour in one of them fails exactly as it would anywhere. + * grants nothing else — a new color in one of them fails exactly as it would anywhere. * * The point of listing them rather than skipping the check is that the list is asserted to * be *exactly* the set with duplicates left, so it cannot rot in either direction: dropping @@ -390,10 +390,10 @@ describe('component CSS', () => { }); it.each(cssFiles.map((file) => [name(file), file]))( - '%s introduces no colour the theme does not have', + '%s introduces no color the theme does not have', (_name, file) => { // Every stylesheet, PENDING_SWEEP included: the exemption is for literals that - // duplicate a token, not a licence to add a colour while the file waits its turn. + // duplicate a token, not a licence to add a color while the file waits its turn. expect(colorProblems(fs.readFileSync(file, 'utf8')).offPalette).toEqual([]); } ); From f7d1055efbcc8185978a4a78c1cc11eea202e870 Mon Sep 17 00:00:00 2001 From: OpenStaxClaude Date: Thu, 10 Sep 2026 22:17:48 +0000 Subject: [PATCH 7/9] CORE-2720: make the token file reachable, and enforce the import rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot is right on both halves. Nothing on this branch imports theme.css — no stylesheet reads a token until the sweep in #143 — and the `./*` export pattern rewrote every subpath to `*.js`, so `@openstax/ui-components/theme/ theme.css` resolved to `theme.css.js` and threw. As published, the tokens were unreachable by either route. The export map now carries a `./*.css` pattern ahead of `./*`, so CSS subpaths resolve to the real files in both trees. Verified against a built dist with node's own resolver: the CSS subpath resolves to dist/cjs under require and dist/esm under import, and JS subpaths are unaffected. The import itself is now a rule the suite enforces rather than a line in the README: a stylesheet that reads a token whose importing module does not import theme.css fails. Proved by making Loader.css read a token — the test fails until Loader.tsx imports the token file. That is what stops the sweep in #143 shipping a stylesheet whose var() silently takes its fallback, which looks right on screen because the fallback is the literal the token replaced. README documents the direct entry point for an app that wants the tokens without rendering one of our components. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 21 +++++++-- package.json | 5 +++ src/theme/tokens.spec.ts | 94 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 113 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 07d51bf79..ea8e774c6 100644 --- a/README.md +++ b/README.md @@ -86,15 +86,29 @@ Color tokens are the kebab-case form of the `src/theme/palette.ts` key (`palette.neutralLighter` → `--ox-color-neutral-lighter`); there are also `--ox-color-link`, `--ox-color-link-hover`, `--ox-z-index-*` and `--ox-padding-navbar-*`. -Any component whose CSS uses a token must import the token file alongside its own stylesheet: +Any component whose CSS uses a token must import the token file alongside its own +stylesheet: ```ts import './MyComponent.css'; import '../theme/theme.css'; ``` -Consumers need do nothing — bundlers deduplicate the import, and `sideEffects` in -`package.json` keeps it from being tree-shaken. +There is no bundler in the build — `build.bash` copies CSS 1:1 — so nothing resolves an +`@import` on our behalf and a stylesheet does not drag `theme.css` in by itself. Forget the +import and every `var(--ox-*)` in that file quietly takes its fallback, which usually looks +right on screen because the fallback is the literal the token replaced. `tokens.spec.ts` +fails when a stylesheet reads a token and the module importing it does not import the +tokens, so this cannot be forgotten rather than merely being documented. + +Consumers of the package need do nothing: the import rides along with the component, +bundlers deduplicate it, and `sideEffects` in `package.json` keeps it from being +tree-shaken. An app that wants the tokens without rendering one of our components — to +build its own styles on the palette, say — can load the file directly: + +```ts +import '@openstax/ui-components/theme/theme.css'; +``` ### Component override hooks and when to bind in JS @@ -124,6 +138,7 @@ callers can set these without a cast. - a component stylesheet introduces a color that is not a theme value and not in the `KNOWN_OFF_PALETTE` allowlist - a component stylesheet reads an `--ox-*` token that does not exist +- a stylesheet reads a token and the module that imports it does not import `theme.css` The color check parses declarations rather than grepping for hex, so it reads a value the way the property does. What that covers: diff --git a/package.json b/package.json index 5f0013696..b17f20e7d 100644 --- a/package.json +++ b/package.json @@ -19,6 +19,11 @@ "import": "./dist/esm/index.js", "require": "./dist/cjs/index.js" }, + "./*.css": { + "browser": "./dist/esm/*.css", + "import": "./dist/esm/*.css", + "require": "./dist/cjs/*.css" + }, "./*": { "browser": "./dist/esm/*.js", "import": "./dist/esm/*.js", diff --git a/src/theme/tokens.spec.ts b/src/theme/tokens.spec.ts index 742a8019a..a3d999c84 100644 --- a/src/theme/tokens.spec.ts +++ b/src/theme/tokens.spec.ts @@ -39,18 +39,26 @@ const KNOWN_OFF_PALETTE = new Map([ const allowlistKey = ({ literal, rgba }: FoundColor) => rgba === null ? literal.replace(/\s+/g, ' ').trim().toLowerCase() : colorKey(rgba); -const walk = (dir: string, out: string[] = []): string[] => { +const walk = ( + dir: string, matches: (fileName: string) => boolean, out: string[] = [] +): string[] => { for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { const full = path.join(dir, entry.name); if (entry.isDirectory()) { - walk(full, out); - } else if (entry.name.endsWith('.css')) { + walk(full, matches, out); + } else if (matches(entry.name)) { out.push(full); } } return out; }; +const isStylesheet = (fileName: string) => fileName.endsWith('.css'); + +/** Source modules, specs excluded: a spec's CSS import is mapped away by jest anyway. */ +const isModule = (fileName: string) => + /\.tsx?$/.test(fileName) && !/\.spec\.tsx?$/.test(fileName); + const here = path.basename(__filename); /** @@ -373,7 +381,7 @@ const PENDING_SWEEP = new Set([ ]); describe('component CSS', () => { - const cssFiles = walk(srcDir).filter((file) => file !== themeCssPath); + const cssFiles = walk(srcDir, isStylesheet).filter((file) => file !== themeCssPath); const tokens = themeTokens(); const name = (file: string) => path.relative(srcDir, file); @@ -414,3 +422,81 @@ describe('component CSS', () => { } ); }); + +/** + * Stylesheets a module imports, as absolute paths. + * + * Only relative imports, because that is how a component reaches its own CSS and the token + * file. A package-relative spelling would not resolve inside src/ anyway. + */ +const importedStylesheets = (moduleFile: string, source: string): string[] => + [...source.matchAll(/import\s+['"](\.[^'"]*\.css)['"]/g)] + .map((match) => path.resolve(path.dirname(moduleFile), match[1])); + +/** + * Stylesheets this module pulls in that read a token, when the module does not also pull in + * the file that defines them. Empty means the module is sound. + * + * There is no bundler here: build.bash rsyncs CSS 1:1, so nothing resolves an `@import` for + * us and a stylesheet does not drag theme.css in by itself. The component that imports the + * stylesheet has to import the token file too, or every `var(--ox-*)` in it silently takes + * its fallback — which is exactly the failure that is invisible in review, because a + * fallback is usually the literal the token replaced and so looks right on screen. + */ +const missingThemeImport = ( + moduleFile: string, source: string, readsTokens: ReadonlySet +): string[] => { + const imported = importedStylesheets(moduleFile, source); + const needy = imported.filter((file) => readsTokens.has(file)); + + return imported.includes(themeCssPath) ? [] : needy; +}; + +/** Whether a stylesheet reads a theme token at all. */ +const readsThemeToken = (css: string) => /var\(\s*--ox-/i.test(stripNoise(css)); + +describe('the token import rule', () => { + const moduleFile = path.join(srcDir, 'components/Thing.tsx'); + const ownStyles = path.join(srcDir, 'components/Thing.css'); + const readsTokens = new Set([ownStyles]); + + it('flags a component that imports a token-reading stylesheet and not the tokens', () => { + expect(missingThemeImport(moduleFile, "import './Thing.css';", readsTokens)) + .toEqual([ownStyles]); + }); + + it('stays quiet when the component imports the token file too', () => { + const source = "import './Thing.css';\nimport '../theme/theme.css';"; + expect(missingThemeImport(moduleFile, source, readsTokens)).toEqual([]); + }); + + it('stays quiet when the stylesheet reads no token', () => { + expect(missingThemeImport(moduleFile, "import './Thing.css';", new Set())).toEqual([]); + }); + + it('resolves an import from a subdirectory', () => { + const nested = path.join(srcDir, 'components/Thing/Thing.tsx'); + const nestedStyles = path.join(srcDir, 'components/Thing/Thing.css'); + expect(missingThemeImport(nested, "import './Thing.css';", new Set([nestedStyles]))) + .toEqual([nestedStyles]); + }); + + it('reads a token reference through comments and however var() is spelled', () => { + expect(readsThemeToken('.x { color: VAR(--ox-color-pale); }')).toBe(true); + expect(readsThemeToken('.x { /* var(--ox-color-pale) */ color: red; }')).toBe(false); + expect(readsThemeToken('.x { color: var(--tabs-border-color); }')).toBe(false); + }); + + it.each(walk(srcDir, isModule).map((file) => [path.relative(srcDir, file), file]))( + '%s imports theme.css if its stylesheet needs it', + (_name, file) => { + // Vacuous on this branch by construction — no stylesheet reads a token until the + // sweep in #143 converts one, and the rule exists so that sweep cannot forget the + // import. The helper above is tested on its own, so the rule itself is pinned now. + const readsTokens = new Set( + walk(srcDir, isStylesheet).filter((css) => readsThemeToken(fs.readFileSync(css, 'utf8'))) + ); + expect(missingThemeImport(file, fs.readFileSync(file, 'utf8'), readsTokens)).toEqual([]); + } + ); +}); From a83c3b48bc9530be5df15de98e5c068003a0b413 Mon Sep 17 00:00:00 2001 From: OpenStaxClaude Date: Thu, 10 Sep 2026 22:44:25 +0000 Subject: [PATCH 8/9] CORE-2720: parse imports instead of matching them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The import scan was a regex over the source, so `// import '../theme/theme.css';` counted as an import. That fails the wrong way round for this check: it reports the token file as present when it is absent, so deleting a real import and leaving a commented-out one beside it would pass. Collects the module's ImportDeclaration nodes through the TypeScript parser instead, with the script kind chosen from the extension since TS and TSX disagree about `x`. Comments and string contents are no longer imports. Four cases added: a commented-out import in both syntaxes, the specifier inside a string, a real import with a trailing comment, and a .tsx module. The first two fail against the regex. Not deferred to CORE-2885 — that ticket is the CSS lexer, this is TypeScript, and the parser is already a devDependency, so the exact answer costs less than the approximation did. Co-Authored-By: Claude Opus 5 (1M context) --- src/theme/tokens.spec.ts | 60 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 55 insertions(+), 5 deletions(-) diff --git a/src/theme/tokens.spec.ts b/src/theme/tokens.spec.ts index a3d999c84..62c426b87 100644 --- a/src/theme/tokens.spec.ts +++ b/src/theme/tokens.spec.ts @@ -1,5 +1,6 @@ import fs from 'fs'; import path from 'path'; +import ts from 'typescript'; import { colorKey, describeColor, FoundColor, opaqueKey, stripNoise, stylesheetColors, } from './cssColors'; @@ -426,12 +427,33 @@ describe('component CSS', () => { /** * Stylesheets a module imports, as absolute paths. * - * Only relative imports, because that is how a component reaches its own CSS and the token - * file. A package-relative spelling would not resolve inside src/ anyway. + * Parsed rather than matched. A regex over the source also sees `// import './x.css';` and + * the same text inside a string, and reading either as an import is the wrong way round for + * a check like this: it says the token file is present when it is not, so deleting a real + * import next to a commented-out one would pass. + * + * Only relative specifiers, because that is how a component reaches its own CSS and the + * token file; a package-relative spelling would not resolve inside src/ anyway. Only static + * imports, which is what a stylesheet side effect is written as here — `require` and dynamic + * `import()` of CSS appear nowhere in src, and would need their own handling if they did. */ -const importedStylesheets = (moduleFile: string, source: string): string[] => - [...source.matchAll(/import\s+['"](\.[^'"]*\.css)['"]/g)] - .map((match) => path.resolve(path.dirname(moduleFile), match[1])); +const importedStylesheets = (moduleFile: string, source: string): string[] => { + const parsed = ts.createSourceFile( + moduleFile, + source, + ts.ScriptTarget.Latest, + /* setParentNodes */ false, + moduleFile.endsWith('.tsx') ? ts.ScriptKind.TSX : ts.ScriptKind.TS + ); + + return parsed.statements + .filter(ts.isImportDeclaration) + .map(({ moduleSpecifier }) => moduleSpecifier) + .filter(ts.isStringLiteral) + .map(({ text }) => text) + .filter((specifier) => specifier.startsWith('.') && specifier.endsWith('.css')) + .map((specifier) => path.resolve(path.dirname(moduleFile), specifier)); +}; /** * Stylesheets this module pulls in that read a token, when the module does not also pull in @@ -474,6 +496,34 @@ describe('the token import rule', () => { expect(missingThemeImport(moduleFile, "import './Thing.css';", new Set())).toEqual([]); }); + it('does not count a commented-out import of the token file', () => { + // The failure this rule exists to catch is a missing import, so anything that reads as + // present when it is absent defeats it — deleting the real import and leaving the + // comment behind is the exact shape of that. + const source = "import './Thing.css';\n// import '../theme/theme.css';"; + expect(missingThemeImport(moduleFile, source, readsTokens)).toEqual([ownStyles]); + + const block = "import './Thing.css';\n/* import '../theme/theme.css'; */"; + expect(missingThemeImport(moduleFile, block, readsTokens)).toEqual([ownStyles]); + }); + + it('does not count the specifier appearing in a string', () => { + const source = "import './Thing.css';\nexport const doc = \"import '../theme/theme.css';\";"; + expect(missingThemeImport(moduleFile, source, readsTokens)).toEqual([ownStyles]); + }); + + it('counts a real import written with a comment beside it', () => { + const source = "import './Thing.css';\nimport '../theme/theme.css'; // tokens\n"; + expect(missingThemeImport(moduleFile, source, readsTokens)).toEqual([]); + }); + + it('parses a .tsx module', () => { + // TS and TSX disagree about `x`, so the kind is chosen from the extension rather + // than left at the default. A component file is the case that actually matters here. + const tsx = "import './Thing.css';\nexport const T = () =>
;"; + expect(missingThemeImport(moduleFile, tsx, readsTokens)).toEqual([ownStyles]); + }); + it('resolves an import from a subdirectory', () => { const nested = path.join(srcDir, 'components/Thing/Thing.tsx'); const nestedStyles = path.join(srcDir, 'components/Thing/Thing.css'); From 1e9e3646920574622669b8e4535f69cbf5a2136e Mon Sep 17 00:00:00 2001 From: OpenStaxClaude Date: Fri, 11 Sep 2026 15:10:26 +0000 Subject: [PATCH 9/9] CORE-2720: read the whole token name, non-ASCII included MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `[\w-]+` stopped at the first character outside ASCII and handed back a prefix, so `var(--ox-color-redé)` was looked up as `--ox-color-red`, found, and passed. CSS name code points include everything at U+0080 and above, so that is a valid name and a different one — the check reported an undefined reference as fine, which is the single answer it must never give. The class now runs to the end of the identifier. Regression cases for a combining-accent name, an astral one (surrogate pair), a non-ASCII character in the middle, and the prefix it truncated to, which still passes. The first three return [] against the old class. Escaped spellings are untouched and now documented where the regex is: they read literally, so `var(--ox-color-r\65 d)` is reported as undefined. That is the decode-the-escapes work in CORE-2885 rather than a boundary this regex can fix, and it errs toward a failure rather than a pass. Noted on the ticket. Co-Authored-By: Claude Opus 5 (1M context) --- src/theme/tokens.spec.ts | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/src/theme/tokens.spec.ts b/src/theme/tokens.spec.ts index 62c426b87..06ec4380a 100644 --- a/src/theme/tokens.spec.ts +++ b/src/theme/tokens.spec.ts @@ -177,10 +177,22 @@ const allColorProblems = (css: string): string[] => { * because custom property *names* are — `var(--OX-color-pale)` really is a reference to * something nothing defines, and silently falling through to its fallback is the failure * this check exists to catch. + * + * The name runs to the end of the CSS identifier, non-ASCII included, because anything at + * U+0080 or above is a name code point. `[\w-]+` stopped at the first of them and handed + * back a prefix, so `var(--ox-color-redé)` was looked up as `--ox-color-red`, found, and + * passed — the undefined reference reported as fine, which is the one answer this check + * must never give. + * + * Escaped spellings are still read literally: `var(--ox-color-r\65 d)` is a reference to + * `--ox-color-red` that this reads as `--ox-color-r` and reports as undefined. Wrong, but + * wrong in the direction of a failure rather than a pass, and it is the same + * decode-the-escapes work as the rest of CORE-2885 rather than a boundary this regex can + * fix. */ const unknownTokenReferences = (css: string, defined: Map) => [ ...new Set( - [...stripNoise(css).matchAll(/var\(\s*(--ox-[\w-]+)/gi)] + [...stripNoise(css).matchAll(/var\(\s*(--ox-(?:[\w-]|[^\x00-\x7f])+)/gi)] .map((match) => match[1]) .filter((name) => !defined.has(name)) ), @@ -340,6 +352,23 @@ describe('the color check itself', () => { .toEqual(['--ox-color-palee']); }); + it('reads the whole name when it carries a character outside ASCII', () => { + // CSS name code points include everything at U+0080 and above, so `--ox-color-redé` is + // a valid name and a different one from `--ox-color-red`. Truncating to the prefix + // found a token that exists and passed the reference that does not — a missing token + // reported as present, which is the failure mode this check cannot have. + const defined = themeTokens(); + expect(defined.has('--ox-color-red')).toBe(true); + expect(unknownTokenReferences('.x { color: var(--ox-color-redé); }', defined)) + .toEqual(['--ox-color-redé']); + expect(unknownTokenReferences('.x { color: var(--ox-color-red🎨); }', defined)) + .toEqual(['--ox-color-red🎨']); + expect(unknownTokenReferences('.x { color: var(--ox-cölor-pale); }', defined)) + .toEqual(['--ox-cölor-pale']); + // and the name it is a prefix of still passes + expect(unknownTokenReferences('.x { color: var(--ox-color-red); }', defined)).toEqual([]); + }); + it('flags a token name whose case does not match', () => { // Property names, unlike function names, are case-sensitive: --OX-color-pale is not // the token, so the declaration resolves to its fallback or nothing at all.