diff --git a/specs/milestone-23.md b/specs/milestone-23.md index 1e482d5..ba594f4 100644 --- a/specs/milestone-23.md +++ b/specs/milestone-23.md @@ -465,21 +465,21 @@ That last clause is the check that keeps the scope honest, and it holds: `react-aria-components` is imported only by the four `ui/` wrappers. A RAC `Button` in a feature file would still be a breach. -### The focus-ring floor does not reach a listbox option +### The focus-ring floor was briefly amended on a false premise -`Select.tsx` gives the trigger the shared focus ring, but the popover's options -show keyboard focus as a background inversion — `primary` behind `primaryText` -— with the outline removed, and the listbox itself has no ring. That is the -selection-highlight convention for a listbox and a strong-contrast indicator, -but it is not ruling 3's 2px focus-token outline, and the code is in breach as -the ruling is written. Two things make the ring the wrong mechanism here rather -than a missing one: an option inside a scrolling popover clips its own outline, -and RAC drives option focus through its `isFocused` render prop, so a -`:focus-visible` rule — which is how the shared `focusRing` is written — may -never match the element at all. The floor is amended to cover controls that -take DOM focus; a listbox option instead requires a focus indicator with a -contrast change against both schemes, which the inversion provides. Every other -interactive element, the trigger included, keeps the ring. +S2 left the Select's options with their outline removed, showing keyboard focus +as a background inversion alone. This section previously amended ruling 3 to +excuse that, arguing RAC drives option focus through an `isFocused` render prop +so a `:focus-visible` rule could never match. That argument is wrong. +`useSelectableItem` gives options a roving `tabIndex` and focuses the option's +own DOM node (`react-aria/dist/private/selection/useSelectableItem.mjs:64`), +which `ListBoxItem` spreads onto its element — so `:focus-visible` matches and +the ring was simply missing. The amendment is withdrawn and ruling 3 stands +unchanged. `Select.tsx` now composes the shared inset ring onto every option: +inset because an option flush against a scrolling popover clips an outset +outline, and recoloured to `primaryText` on the highlighted row because the +focus token is the same blue that row paints behind it. The inversion stays as +the listbox convention; it is no longer the only indicator. ### Two notes for S3 diff --git a/web/scripts/assert-css-layers.mjs b/web/scripts/assert-css-layers.mjs index c88574e..5ab165c 100644 --- a/web/scripts/assert-css-layers.mjs +++ b/web/scripts/assert-css-layers.mjs @@ -4,6 +4,16 @@ // selector says, so a single unlayered rule silently beats the StyleX atomic // rules it was written to sit under. That failure renders wrong and passes // every other gate: no test asserts computed style, and the bundler is happy. +// It also checks the layer ORDER, which is the invariant that actually matters: +// a later layer beats an earlier one, so `reset` has to be declared first. +// +// What it does not catch, so nobody reads more into a pass than is there: an +// unlayered rule that sets only custom properties is allowed, because StyleX +// emits its token `:root` block exactly that way and this cannot tell that +// block from an override of it; a declaration value containing `@layer` or a +// brace inside a string blinds the stripper; and with several stylesheets it +// judges each alone, not their load order in the document. +// // This check runs from web/ as part of `npm run build`. import { readdirSync, readFileSync } from "node:fs"; @@ -58,9 +68,36 @@ function stylesSomething(body) { .some((declaration) => declaration.length > 0 && !declaration.startsWith("--")); } +/** + * Layer names in the order their position is fixed, which is where each name is + * first mentioned — a later block under an already-named layer does not move it. + */ +function layerOrder(css) { + const seen = []; + for (const [, names] of css.replace(/\/\*[\s\S]*?\*\//g, "").matchAll(/@layer\s+([^{;]+)[{;]/g)) { + for (const name of names.split(",")) { + const trimmed = name.trim(); + if (trimmed.length > 0 && !seen.includes(trimmed)) seen.push(trimmed); + } + } + return seen; +} + let failed = false; for (const sheet of sheets) { - const leftover = outsideLayers(readFileSync(join(distDir, sheet), "utf8")); + const css = readFileSync(join(distDir, sheet), "utf8"); + + // Order is the whole point: a later layer wins, so the reset has to be first. + const order = layerOrder(css); + if (order.length > 0 && order[0] !== "reset") { + console.error( + `assert-css-layers: ${sheet} declares layers in the order ${order.join(", ")} — ` + + `'reset' must come first or it outranks the StyleX rules written against it.`, + ); + failed = true; + } + + const leftover = outsideLayers(css); for (const [, selector, body] of leftover.matchAll(/([^{}]+)\{([^{}]*)\}/g)) { if (!stylesSomething(body)) continue; console.error( diff --git a/web/src/styles.css b/web/src/styles.css index e5bc682..7e3af78 100644 --- a/web/src/styles.css +++ b/web/src/styles.css @@ -9,7 +9,7 @@ * their font. Deleting this block does not restore browser defaults — it * silently changes the meaning of every size and spacing value in the app. * Rules are limited to what this app renders; it is not a general reset. - + * * The reset lives in its own cascade layer, declared here before StyleX emits * its own. Layer order is priority order, and unlayered author CSS outranks * every layer: leaving these rules unlayered silently beat every StyleX rule in @@ -65,9 +65,21 @@ kbd, samp, pre { + font-family: ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace; font-size: 1em; } + /* Without this the spinner stretches a number input taller than its row. */ + ::-webkit-inner-spin-button, + ::-webkit-outer-spin-button { + height: auto; + } + + /* Firefox draws a red glow on an invalid field; the form shows its own error. */ + :-moz-ui-invalid { + box-shadow: none; + } + table { border-collapse: collapse; text-indent: 0; @@ -166,7 +178,7 @@ height: auto; } - [hidden] { + [hidden]:not([hidden="until-found"]) { display: none !important; } } diff --git a/web/src/ui/Select.tsx b/web/src/ui/Select.tsx index 7051ab0..9ab400c 100644 --- a/web/src/ui/Select.tsx +++ b/web/src/ui/Select.tsx @@ -87,11 +87,18 @@ const styles = stylex.create({ fontSize: "0.875rem", lineHeight: "1.25rem", cursor: "pointer", - outlineStyle: "none", }, + /** + * The inverted row is the listbox convention, and the ring is the milestone-9 + * floor; an option gets both. RAC focuses the option's own DOM node, so the + * shared `:focus-visible` ring does apply here — it is drawn inset because an + * option flush against a scrolling popover clips an outset one, and recoloured + * because the focus token is the same blue this row just painted behind it. + */ itemFocused: { backgroundColor: colors.primary, color: colors.primaryText, + outlineColor: { default: null, ":focus-visible": colors.primaryText }, }, itemSelected: { fontWeight: 600, @@ -125,6 +132,7 @@ export default function Select({ options, value, onChange, label, "aria-label": className={({ isFocused, isSelected }) => stylex.props( styles.item, + shared.insetFocusRing, isSelected && styles.itemSelected, isFocused && styles.itemFocused, ).className ?? ""