milestone 23: restore the focus ring on select options, check layer order, close preflight gaps
This commit is contained in:
+14
-14
@@ -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
|
||||
|
||||
|
||||
@@ -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(
|
||||
|
||||
+14
-2
@@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 ?? ""
|
||||
|
||||
Reference in New Issue
Block a user