Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -269,11 +269,14 @@ action must perform one; a page that ships must be reachable.

- **Buttons.** Every interactive `<button>` must do something: an `onClick`, a `type="submit"`
inside a `<form onSubmit>`, or navigation (wrap it in a `<Link>` / call `router.push`). A control
whose feature is not yet built uses the explicit disabled-placeholder pattern — `disabled` or
`aria-disabled="true"` + `title="… — coming soon"` + an `sr-only` note wired via
`aria-describedby` (see `favourites-hub.tsx`). **Never** ship a styled, `aria-label`led button
with no handler and no disabled state — that was the "Language and region" defect fixed
2026-07-21.
that is unavailable for a **stated reason** — feature not built, or this record lacks the data —
uses `aria-disabled="true"` + `onClick={ignoreUnavailableActivation}` + `title="… — coming soon"`
- an `sr-only` note wired via `aria-describedby` (see `favourites-hub.tsx`). Native `disabled`
would remove the tab stop and the reason would never be reached. Keep native `disabled` for
**transient** inertness (request in flight, pager at its last page, form action awaiting
validity). Never both attributes on one button — lint fails on the pair. **Never** ship a styled,
`aria-label`led button with no handler and no disabled state — that was the "Language and region"
defect fixed 2026-07-21.
- **Navigation.** Internal navigation uses `<Link>`, `router.push`, or server `redirect()` — never
a raw `<a href="/…">` to an internal route. Build hrefs from the existing sources
(`src/lib/app-modes.ts`, `src/lib/tools-catalog.ts`, `src/lib/universal-search.ts`), not
Expand Down
5 changes: 3 additions & 2 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,9 @@ applies; unknown non-document paths fail closed to heavy scope.
These fail builds, so they are worth knowing before you write code:

- **Button wiring.** Every `<button>` does something — handler, submit inside a form, or
navigation. Not-yet-built features use the explicit disabled-placeholder pattern
(`disabled`/`aria-disabled` + `title="… — coming soon"` + `sr-only` note). Enforced by
navigation. A control unavailable for a stated reason uses `aria-disabled="true"` + an inert
handler + `title="… — coming soon"` + `sr-only` note; native `disabled` is for transient
inertness only, and the two attributes together fail lint. Enforced by
`eslint-rules/require-button-wiring.mjs`. Never blanket-disable the rule.
- **No orphan routes.** A new production page route needs an inbound link from real nav,
then `npm run sitemap:update`, a `docs/codebase-index.md` entry, and a reachability
Expand Down
15 changes: 8 additions & 7 deletions docs/branch-review-ledger.md

Large diffs are not rendered by default.

103 changes: 69 additions & 34 deletions docs/wiring-conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,28 @@ Every interactive `<button>` must resolve to a behaviour:
- **Busy / async** — route through the shared busy-state contract in `src/components/ui-primitives.tsx`
(spinner + `disabled` + live-region announcement), not an ad-hoc disabled flag.

For a feature that is **not yet built**, use the explicit disabled-placeholder pattern — never a fake
or empty handler. The reference markup is `favourites-hub.tsx`:
### Unavailable controls: which disabled encoding, and why it is a real decision

Two kinds of control look identical on screen and are not the same thing:

- **Unavailable for a stated reason** — the feature is not built yet, or the action needs data this
record does not have ("no official source URL is recorded for this form"). The reason is written
down, in a `title` and usually an `sr-only` span wired by `aria-describedby`.
- **Transiently inert** — a submit button while a request is in flight, a pager at its first or last
page, a form action that is off until the form is valid. There is nothing to explain; the state
resolves itself as the user works.

**Stated reason → `aria-disabled="true"` plus an inert handler.** Never the native attribute:
`disabled` removes the tab stop, so a keyboard user — and a screen-reader user who moves by Tab
rather than by virtual cursor — can never land on the control, and the reason we went to the trouble
of writing is never announced. The explanation existed and was unreachable; the control simply
vanished. Reference markup is `favourites-hub.tsx`:

```tsx
<button
type="button"
disabled
aria-disabled="true"
onClick={ignoreUnavailableActivation}
aria-describedby="thing-unavailable"
className="… cursor-not-allowed opacity-60 …"
title="Thing — coming soon"
Expand All @@ -40,28 +55,48 @@ or empty handler. The reference markup is `favourites-hub.tsx`:
</span>
```

**Native `disabled`, not `aria-disabled`, is the default here** — and the reason is worth stating,
because `disabled` looks like it should suppress the `aria-describedby` reason and does not. A
disabled button stays in the accessibility tree with its accessible description intact, so a screen
reader reaching it by virtual cursor or swipe still announces why it is unavailable. That is asserted,
not assumed: `tests/favourites-hub-unavailable-controls.dom.test.tsx` pins `toBeDisabled()`,
`not.toHaveAttribute("aria-disabled")` **and** `toHaveAccessibleDescription(...)` together on all three
hub placeholders. What `disabled` does remove is the tab stop, which is why the `title` matters for
pointer users and why WCAG permits it (a disabled control is exempt from focus-order requirements).

Reach for `aria-disabled="true"` plus a no-op handler only when the control genuinely must stay
tabbable — a roving-tabindex group where skipping a dead end would strand arrow navigation, as in
`ResultFilterSheet`. `AGENTS.md` accepts either form, and `require-button-wiring` treats both as
wired.

**Both attributes together is a third shape, and the repo currently pins it two different ways.**
`tests/mobile-interaction-regressions.test.ts` asserts the density placeholders in
`differential-presentation-workflow-page.tsx` are native-only (`not.toContain("aria-disabled")`),
and in the same file asserts `disabled aria-disabled="true"` together for the Add placeholders in
`visual-evidence.tsx` and `evidence-panels.tsx`. Those two positions have not been reconciled — the
pairing is redundant (native `disabled` already conveys the state, and the two attributes disagree
about focusability), but it is pinned, so do not "tidy" either shape without settling which one wins.
Tracked as `#291`.
`ignoreUnavailableActivation` (`ui-primitives.tsx`) is the shared handler. It calls
`preventDefault()` **and** `stopPropagation()`, because that is what the native attribute did: a
disabled button fires no click at all, so nothing bubbled to a clickable ancestor.

**Transient → keep native `disabled`.** It is correct there: the control is genuinely inert and
momentary, the browser's own semantics are right, and making it focusable would be a regression, not
a fix. Sites deliberately left native include the compare action in `differential-stream-workspace.tsx`
(needs two diagnoses selected — and the same sentence is already rendered as visible text above it),
the pin editor's save button in `search-pins-menu.tsx` (form validity), the services compare and
clear actions in `services-navigator-page.tsx`, and the composer send in `master-search-header.tsx`.

A third case keeps `aria-disabled` for a different reason: a **roving-tabindex group** where skipping
a dead end would strand arrow navigation, as in `ResultFilterSheet` (`result-filter-control.tsx`) and
the facet chips in `document-search-results.tsx`. Same encoding, same guarded click.

**The two attributes together is not belt and braces — it is the bug wearing a disguise.** The native
attribute wins on focus, so `disabled aria-disabled="true"` behaves exactly like `disabled` alone
while looking like it was thought about. `require-button-wiring` now fails on that pair
(`redundantDisabledPair`), on any `<button>` regardless of `type`; a pair where either side is
statically off (`disabled={false}` beside `aria-disabled="true"`) still passes, since that is a real
way to spell a conditional placeholder. This settles ledger `#291`, which tracked the repo pinning
the pairing two contradictory ways.

**Styling does not follow for free.** With the native attribute gone, `disabled:` variant classes
stop applying and the control becomes hoverable. Convert the variants alongside the attribute:
`disabled:` → `aria-disabled:`, and suppress hover with `hover:not-aria-disabled:` (the therapy
recipes in `therapy-compass/controls.ts` do this; `controlDisabled` in `ui-primitives.tsx` carries
both halves so anything built on `controlBase` / `floatingControl` / `toolbarButton` is already
covered). A converted control that quietly lights up on hover reads as available again.

What holds this in place: `tests/require-button-wiring.test.ts` pins that the lint rule actually
fires in both directions, and `tests/favourites-hub-unavailable-controls.dom.test.tsx` tabs onto a
converted placeholder, asserts it takes focus, asserts the accessible description is what the reader
gets, and asserts activating it by keyboard and by pointer does nothing.

**Not yet converted**, and deliberately so — the four "not available in this comparison view"
placeholders and the Compact/Detailed density pair in
`differentials/differential-presentation-workflow-page.tsx`. That page is scheduled for a rewrite, and
`tests/mobile-interaction-regressions.test.ts` still pins the density pair as native-only. Convert
them with that rewrite, not before. `document-viewer/document-image-filmstrip.tsx` and the
`DocumentViewer.tsx` summarize action are also still native (the latter mixes an auth reason with a
loading state, so it needs splitting before it can be classified).

**Read-only indicators are not controls.** The shared `ToggleSwitch` (`ui-primitives.tsx`) renders an
operable `role="switch"` only when an `onToggle` is passed; without it, it is a presentational
Expand Down Expand Up @@ -133,12 +168,12 @@ both wiring gates skip them.

## The gates

| Gate | Catches | Runs in |
| ------------------------------------------ | --------------------------------------------------------- | ----------------------------------- |
| `eslint-rules/require-button-wiring.mjs` | `<button type="button">` with no handler / disabled state | `npm run lint` → `verify:cheap`, CI |
| `tests/route-reachability.test.ts` | static production page routes with no inbound nav link | `npm run test` → `verify:cheap`, CI |
| `tests/site-map.test.ts` / `sitemap:check` | routes / nav hrefs missing from `docs/site-map.md` | `npm run test`, `verify:cheap`, CI |
| `npm run check:knip` | dead exports / orphan modules (e.g. unused href builders) | `verify:cheap`, CI |
| Gate | Catches | Runs in |
| ------------------------------------------ | ------------------------------------------------------------------------------------------------ | ----------------------------------- |
| `eslint-rules/require-button-wiring.mjs` | `<button type="button">` with no handler / disabled state; `disabled` + `aria-disabled` together | `npm run lint` → `verify:cheap`, CI |
| `tests/route-reachability.test.ts` | static production page routes with no inbound nav link | `npm run test` → `verify:cheap`, CI |
| `tests/site-map.test.ts` / `sitemap:check` | routes / nav hrefs missing from `docs/site-map.md` | `npm run test`, `verify:cheap`, CI |
| `npm run check:knip` | dead exports / orphan modules (e.g. unused href builders) | `verify:cheap`, CI |

Intentional exceptions are documented, not silenced:

Expand All @@ -161,6 +196,6 @@ builders remain open. `#007` (`/tools` vs `/?mode=tools`) is resolved: `/tools`
without updating API contract tests and docs together.
- **Coming-soon placeholders (`#010`)** — audited forms refine/reset + Forms tab, favourites hub
sort/add/new-set, favourites command-library move/remove, and presentation Compact/Detailed density.
All use `disabled` or the `aria-disabled` + `title` + `sr-only` / `aria-describedby` pattern (or
presentational `ToggleSwitch` without `onToggle`). No fake-interactive controls found; leave
unwired until the underlying features land. Reference markup remains `favourites-hub.tsx`.
No fake-interactive controls found; leave unwired until the underlying features land. Reference
markup remains `favourites-hub.tsx` — which now carries `aria-disabled` + an inert handler rather
than the native attribute, per the section above.
75 changes: 73 additions & 2 deletions eslint-rules/require-button-wiring.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,20 @@
* (which may default to submit inside a <form>) are left alone.
* - Any spread ({...props}) skips the element, since a handler may arrive
* dynamically (same escape hatch as require-lucide-icon-aria).
* - An unbuilt feature is expressed the codebase's way — `disabled` or
* - An explicitly inert control is expressed the codebase's way — `disabled` or
* `aria-disabled` (typically with a "coming soon" note) — and passes.
*
* Which of the two to reach for is a real decision, not a stylistic one, and
* `docs/wiring-conventions.md` is where it is made: a control that is
* unavailable for a *stated reason* (feature not built, this record has no such
* data) carries `aria-disabled="true"` plus an inert handler, because the native
* attribute removes the tab stop and the reason then cannot be reached by
* keyboard. A control that is merely *transiently* inert — a request in flight,
* a pager at its last page, a form action awaiting validity — keeps native
* `disabled`. This rule accepts both, since both are genuinely wired.
*
* What it does NOT accept is the two together (see `hasRedundantDisabledPair`).
*
* There is no auto-fix: wiring a button requires knowing what it should do, so
* the fix is a human decision (add the handler, or make it an explicit
* disabled placeholder). Design-scratch mockups are exempt via eslint.config.mjs.
Expand Down Expand Up @@ -75,6 +86,48 @@ function isWiringAttr(attr) {
return !isStaticallyOff(attr.value);
}

/**
* True when a `<button>` carries BOTH a live native `disabled` and a live
* `aria-disabled` — the shape that reads as belt-and-braces and is not.
*
* The native attribute wins on focus: the element leaves the tab order whatever
* `aria-disabled` says, so the pairing buys nothing and actively hides the bug.
* Every site that carried it also carried an `aria-describedby` reason that no
* keyboard user could reach, which is what made it worth gating rather than
* merely documenting (ledger `#291`).
*
* Statically-off values are excluded on both sides, so `disabled={false}` beside
* `aria-disabled="true"` — a legitimate way to spell a conditional placeholder —
* still passes, as does any pair where the aria side is `"false"`.
*/
function hasLiveAttr(attributes, name) {
return attributes.some(
(attr) =>
attr.type === "JSXAttribute" &&
attr.name.type === "JSXIdentifier" &&
attr.name.name === name &&
!isStaticallyOff(attr.value),
);
}

function hasRedundantDisabledPair(attributes) {
return hasLiveAttr(attributes, "disabled") && hasLiveAttr(attributes, "aria-disabled");
}

/**
* True when a button advertises `aria-disabled` without an inert (or any) click
* handler. Native `disabled` is itself inert; `aria-disabled` is not — without
* an onClick that prevents activation the control stays fully operable, which
* is the opposite of the unavailable-placeholder contract.
*/
function hasAriaDisabledWithoutHandler(attributes) {
return (
hasLiveAttr(attributes, "aria-disabled") &&
!hasLiveAttr(attributes, "disabled") &&
!hasLiveAttr(attributes, "onClick")
);
}

/** @type {import("eslint").Rule.RuleModule} */
const rule = {
meta: {
Expand All @@ -87,13 +140,31 @@ const rule = {
messages: {
unwired:
'This <button type="button"> has no onClick and no disabled/aria-disabled state — it does nothing when clicked. Wire it with onClick, or make it an explicit disabled "coming soon" placeholder.',
redundantDisabledPair:
'This <button> carries both `disabled` and `aria-disabled` — the native attribute wins on focus, so the aria one changes nothing and the control still leaves the tab order. Keep `disabled` alone for a transiently inert control, or `aria-disabled="true"` plus an inert onClick when the control is unavailable for a stated reason a keyboard user needs to reach.',
ariaDisabledNeedsHandler:
"This <button> carries `aria-disabled` without an onClick — unlike native `disabled`, aria-disabled does not block activation. Add an inert onClick (for example `ignoreUnavailableActivation`) so the control stays reachable but does nothing.",
},
},
create(context) {
return {
JSXOpeningElement(node) {
if (node.name.type !== "JSXIdentifier" || node.name.name !== "button") return;
// A spread may inject a handler dynamically — don't flag.
// The redundant pairing is wrong on any button, not just type="button",
// and must run before the spread escape: explicit `disabled` +
// `aria-disabled` after `{...props}` still create the forbidden pair
// (native wins on focus regardless of anything the spread injects).
if (hasRedundantDisabledPair(node.attributes)) {
Comment thread
cursor[bot] marked this conversation as resolved.
context.report({ node, messageId: "redundantDisabledPair" });
return;
}
// Same for aria-disabled without a handler: an explicit attribute after
// a spread still leaves the control operable unless onClick is present.
if (hasAriaDisabledWithoutHandler(node.attributes)) {
context.report({ node, messageId: "ariaDisabledNeedsHandler" });
return;
}
// A spread may inject a handler dynamically — don't flag unwired.
if (node.attributes.some((attr) => attr.type === "JSXSpreadAttribute")) return;
// Only inspect explicit type="button"; submit/reset/dynamic are out of scope.
if (!node.attributes.some((attr) => isTypeButton(attr))) return;
Expand Down
6 changes: 5 additions & 1 deletion scripts/check-design-system-contract.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,11 @@ assert(
assert(globalsForTherapy.includes("body:has([data-therapy-root])"), "Therapy print isolation must stay in globals.css");
assert(globalsForTherapy.includes("[data-therapy-no-print]"), "Therapy no-print hooks must stay in globals.css");
const controlsSource = textAt("src/components/therapy-compass/controls.ts");
assert(controlsSource.includes("hover:enabled:"), "Therapy buttons need a hover state");
// Unavailable placeholders now use aria-disabled (keyboard-reachable reason), so
// hover must stay quiet under both encodings: `hover:not-aria-disabled:enabled:`.
// The older `hover:enabled:` form still passes — it is what native-disabled-only
// recipes used before the dual-encoding contract.
assert(/hover:(?:not-aria-disabled:)?enabled:/.test(controlsSource), "Therapy buttons need a hover state");
assert(controlsSource.includes("disabled:"), "Therapy buttons need a disabled state");
assert(controlsSource.includes("export const therapyBtn"), "Therapy shared button recipe is missing");

Expand Down
Loading
Loading