From 722abdb780c715c0a89df268ed48f6c741ffd569 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 07:37:36 +0000 Subject: [PATCH 1/4] fix(a11y): make unavailable placeholder buttons reachable by keyboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The disabled-placeholder pattern put `disabled` on the button and attached the reason it was unavailable via `aria-describedby` to an `sr-only` span, plus a `title`. But `disabled` removes the tab stop, so a keyboard user — and a screen-reader user moving by Tab rather than by virtual cursor — could never land on the control. The explanation was written and unreachable; the control simply vanished for them. Convert the controls that are unavailable for a *stated* reason (feature not built yet, or this record lacks the data) to `aria-disabled="true"` plus a shared inert handler, keeping the title and the described-by reason. 24 sites across 12 components, including the four that carried `disabled` and `aria-disabled` together — belt and braces that never helped, because the native attribute still won on focus. Transiently inert controls keep native `disabled`, which is correct there: a request in flight, a pager at its last page, a form action awaiting validity. Those are listed in the wiring doc so the next pass does not "fix" them. Styling had to move with the attribute: `disabled:` variants stop applying once the native attribute is gone, and the control becomes hoverable. The `controlDisabled` recipe grew its `aria-disabled:` half, and the therapy recipes switched `hover:enabled:` to `hover:not-aria-disabled:enabled:` so a converted control does not light up under the cursor. Contract updated with the code, rather than left saying the opposite: - docs/wiring-conventions.md replaces the "native disabled is the default here" argument with the stated-reason vs transient split, and settles the unreconciled pairing tracked as ledger #291. - require-button-wiring gains `redundantDisabledPair`, failing on the two attributes together on any diff --git a/src/components/ui-primitives.tsx b/src/components/ui-primitives.tsx index 87d4dbdc7..7a38228cb 100644 --- a/src/components/ui-primitives.tsx +++ b/src/components/ui-primitives.tsx @@ -1,5 +1,5 @@ import { Ban, Landmark, Loader2, ShieldCheck, TriangleAlert, X, type LucideIcon } from "lucide-react"; -import type { ButtonHTMLAttributes, ReactNode } from "react"; +import type { ButtonHTMLAttributes, MouseEvent, ReactNode } from "react"; import { extractionQualityLabel, formatClinicalDate, @@ -37,6 +37,28 @@ export function cn(...classes: Array) { return twMergeClinical(classes.filter(Boolean).join(" ")); } +/** + * The click handler for an `aria-disabled` placeholder — a control whose feature + * is not built yet, or whose action needs data this record does not have. + * + * Those controls carry `aria-disabled="true"` rather than the native `disabled` + * attribute, because `disabled` takes a button out of the tab order: a keyboard + * user (and a screen-reader user moving by Tab rather than by virtual cursor) + * can never land on it, so the `title` and the `aria-describedby` reason we went + * to the trouble of writing are never reached. `aria-disabled` keeps the tab + * stop and the announcement — "dimmed"/"unavailable" plus the description — and + * moves the job of doing nothing to this handler. + * + * It stops propagation as well as preventing the default, because that is what + * the native attribute did: a disabled button fires no click at all, so nothing + * bubbled to a clickable ancestor. Without `stopPropagation` a placeholder + * inside a clickable row would start activating the row. + */ +export function ignoreUnavailableActivation(event: MouseEvent) { + event.preventDefault(); + event.stopPropagation(); +} + export const transitionSurface = "transition-colors transition-shadow motion-reduce:transition-none"; export const transitionTransform = "transition-transform motion-reduce:transform-none"; @@ -59,8 +81,14 @@ export const panel = // flatten the fill to --surface-subtle, put the label on --disabled, drop the // shadow, and remove the press affordance. `!` is required because the variant // classes that follow this base would otherwise win on source order. +// The `aria-disabled:` half is not belt-and-braces: an unavailable placeholder +// carries `aria-disabled="true"` and no native attribute (see +// `ignoreUnavailableActivation`), so without these the control would lose the +// whole encoding and render as available. The `!` also does a second job here — +// it outranks the un-suffixed `hover:` colours these recipes ship, which a +// native `disabled` control never reaches but an `aria-disabled` one does. export const controlDisabled = - "disabled:cursor-not-allowed disabled:border-[color:var(--border)] disabled:bg-[color:var(--surface-subtle)]! disabled:text-[color:var(--disabled)]! disabled:shadow-none! disabled:active:translate-y-0 aria-disabled:cursor-not-allowed"; + "disabled:cursor-not-allowed disabled:border-[color:var(--border)] disabled:bg-[color:var(--surface-subtle)]! disabled:text-[color:var(--disabled)]! disabled:shadow-none! disabled:active:translate-y-0 aria-disabled:cursor-not-allowed aria-disabled:border-[color:var(--border)] aria-disabled:bg-[color:var(--surface-subtle)]! aria-disabled:text-[color:var(--disabled)]! aria-disabled:shadow-none! aria-disabled:active:translate-y-0"; export const controlBase = `inline-flex min-h-tap items-center justify-center gap-2 rounded-lg text-sm font-semibold transition active:translate-y-px focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-[color:var(--focus)] forced-colors:border ${controlDisabled}`; export const primaryControl = `${controlBase} bg-[color:var(--command)] px-5 text-[color:var(--command-contrast)] shadow-[var(--shadow-tight)] hover:bg-[color:var(--command-hover)] hover:shadow-[var(--shadow-hover)]`; export const floatingControl = `inline-flex min-h-tap items-center justify-center gap-2 rounded-lg border border-[color:var(--border-lux)] bg-[color:var(--surface-raised)] px-3 text-sm font-semibold text-[color:var(--text)] shadow-[var(--shadow-inset)] transition hover:border-[color:var(--border-strong)] hover:bg-[color:var(--surface-subtle)] focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-[color:var(--focus)] forced-colors:border ${controlDisabled}`; diff --git a/tests/auth-panel-product-truth.dom.test.tsx b/tests/auth-panel-product-truth.dom.test.tsx index 7ee4c5a32..4be394db4 100644 --- a/tests/auth-panel-product-truth.dom.test.tsx +++ b/tests/auth-panel-product-truth.dom.test.tsx @@ -42,7 +42,10 @@ describe("AuthPanel product truth", () => { render(); const apple = screen.getByRole("button", { name: "Apple sign-in unavailable" }); - expect(apple).toBeDisabled(); + // `aria-disabled`, not the native attribute — the reason below is only worth + // writing if a keyboard user can land on the control and hear it. + expect(apple).toHaveAttribute("aria-disabled", "true"); + expect(apple).not.toBeDisabled(); expect(apple).toHaveAttribute("title", "Apple sign-in is unavailable — coming soon"); expect(apple).toHaveAccessibleDescription( "Apple sign-in is unavailable. Continue with email, Google, or Microsoft.", diff --git a/tests/favourites-auth-gate.dom.test.tsx b/tests/favourites-auth-gate.dom.test.tsx index b423f4f77..2e6763231 100644 --- a/tests/favourites-auth-gate.dom.test.tsx +++ b/tests/favourites-auth-gate.dom.test.tsx @@ -164,7 +164,10 @@ describe("favourites auth gate DOM", () => { for (const provider of ["Apple", "Google", "Microsoft"]) { const button = screen.getByRole("button", { name: `${provider} sign-in unavailable` }); - expect(button).toBeDisabled(); + // `aria-disabled`, not the native attribute: these carry a description the + // reader has to be able to reach, which a lost tab stop would prevent. + expect(button).toHaveAttribute("aria-disabled", "true"); + expect(button).not.toBeDisabled(); expect(button).toHaveAttribute("title", `${provider} sign-in is unavailable — coming soon`); expect(button).toHaveAccessibleDescription(`${provider} sign-in is unavailable. Continue with email.`); } diff --git a/tests/favourites-hub-unavailable-controls.dom.test.tsx b/tests/favourites-hub-unavailable-controls.dom.test.tsx index a9f03ffa1..ba39f44af 100644 --- a/tests/favourites-hub-unavailable-controls.dom.test.tsx +++ b/tests/favourites-hub-unavailable-controls.dom.test.tsx @@ -1,4 +1,5 @@ import { render, screen, within } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { FavouritesHub } from "@/components/clinical-dashboard/favourites-hub"; @@ -28,24 +29,51 @@ describe("FavouritesHub unavailable controls", () => { favouritesHook.registryStatus = "ready"; }); - it("keeps unavailable actions natively disabled and exposes their reasons", () => { + it("keeps unavailable actions reachable and exposes their reasons", () => { render( undefined} demoMode={false} />); const recent = screen.getByRole("button", { name: "Recent" }); const add = screen.getByRole("button", { name: /Add favourite/ }); const newSet = screen.getByRole("button", { name: "New set" }); - expect(recent).toBeDisabled(); - expect(recent).not.toHaveAttribute("aria-disabled"); + // `aria-disabled`, never the native attribute: `disabled` removes the tab + // stop, so the description each of these carries would be written and then + // never reached by a keyboard user. See docs/wiring-conventions.md. + for (const control of [recent, add, newSet]) { + expect(control).toHaveAttribute("aria-disabled", "true"); + expect(control).not.toBeDisabled(); + } expect(recent).toHaveAccessibleDescription("Additional sort options are coming soon."); - expect(add).toBeDisabled(); - expect(add).not.toHaveAttribute("aria-disabled"); expect(add).toHaveAccessibleDescription("Adding favourites from this screen is coming soon."); - expect(newSet).toBeDisabled(); - expect(newSet).not.toHaveAttribute("aria-disabled"); expect(newSet).toHaveAccessibleDescription("Creating favourite sets is coming soon."); }); + it("lets the keyboard reach an unavailable action, and does nothing when it is activated", async () => { + const user = userEvent.setup(); + render( undefined} demoMode={false} />); + + const add = screen.getByRole("button", { name: /Add favourite/ }); + + // The whole point of the conversion: focus lands on it. `.focus()` would + // pass on a natively disabled button in jsdom, so tab to it for real — that + // is the assertion nothing pinned before, and it is what regressed when the + // native attribute was there. + add.focus(); + expect(add).toHaveFocus(); + await user.tab(); + expect(add).not.toHaveFocus(); + await user.tab({ shift: true }); + expect(add).toHaveFocus(); + + // Focusable must not mean operable. Activating by keyboard and by pointer + // both no-op, and the accessible description is what the user gets instead. + await user.keyboard("{Enter}"); + await user.keyboard(" "); + await user.click(add); + expect(add).toHaveAccessibleDescription("Adding favourites from this screen is coming soon."); + expect(screen.getByTestId("favourites-hub")).toBeInTheDocument(); + }); + it("does not assert library zeroes while the saved registry is still loading", () => { favouritesHook.status = "loading"; render( undefined} demoMode={false} />); diff --git a/tests/mobile-interaction-regressions.test.ts b/tests/mobile-interaction-regressions.test.ts index 4bb0b0627..829ce73c2 100644 --- a/tests/mobile-interaction-regressions.test.ts +++ b/tests/mobile-interaction-regressions.test.ts @@ -87,14 +87,17 @@ describe("mobile interaction regressions", () => { const tools = source("src/components/applications-launcher-page.tsx"); const header = source("src/components/clinical-dashboard/master-search-header.tsx"); + // `aria-disabled` + an inert handler, and NOT the native `disabled` attribute + // alongside it: the native one wins on focus, so pairing them left the reason + // unreachable by keyboard exactly as if the aria attribute were absent. expect(visualEvidence).toContain('title="Add to favourites — coming soon"'); expect(visualEvidence).toMatch( - /type="button"\s+disabled\s+aria-disabled="true"\s+aria-describedby="visual-evidence-add-unavailable"/, + /type="button"\s+aria-disabled="true"\s+onClick=\{ignoreUnavailableActivation\}\s+aria-describedby="visual-evidence-add-unavailable"/, ); expect(visualEvidence).not.toContain("setAdded(true)"); expect(evidencePanels).toContain('title="Add to favourites — coming soon"'); expect(evidencePanels).toMatch( - /type="button"\s+disabled\s+aria-disabled="true"\s+aria-describedby="clinical-notes-add-unavailable"/, + /type="button"\s+aria-disabled="true"\s+onClick=\{ignoreUnavailableActivation\}\s+aria-describedby="clinical-notes-add-unavailable"/, ); expect(evidencePanels).not.toContain("setAdded(true)"); diff --git a/tests/require-button-wiring.test.ts b/tests/require-button-wiring.test.ts new file mode 100644 index 000000000..7d3c05bb4 --- /dev/null +++ b/tests/require-button-wiring.test.ts @@ -0,0 +1,65 @@ +import { Linter } from "eslint"; +import { describe, expect, it } from "vitest"; + +import rule from "../eslint-rules/require-button-wiring.mjs"; + +/** + * The wiring gate is only worth having if it still fires. `npm run lint` going + * green proves the repo is clean, not that the rule can fail — a rule that + * silently matches nothing looks identical from the outside. These cases pin + * both directions, and in particular the `redundantDisabledPair` message added + * when the disabled-placeholder pattern moved to `aria-disabled` (ledger `#291`). + */ +const linter = new Linter(); + +function lint(code: string) { + return linter.verify(code, { + languageOptions: { + parserOptions: { ecmaFeatures: { jsx: true }, ecmaVersion: "latest", sourceType: "module" }, + }, + plugins: { local: { rules: { "require-button-wiring": rule } } }, + rules: { "local/require-button-wiring": "error" }, + }); +} + +function messageIds(code: string) { + return lint(code).map((message) => message.messageId); +} + +describe("require-button-wiring", () => { + it("flags a type=button with no handler and no disabled state", () => { + expect(messageIds('')).toEqual(["unwired"]); + }); + + it("accepts either disabled encoding on its own", () => { + expect(messageIds('')).toEqual([]); + expect(messageIds('')).toEqual([]); + expect(messageIds('')).toEqual([]); + }); + + it("flags the two encodings together, because native disabled wins on focus", () => { + expect(messageIds('')).toEqual([ + "redundantDisabledPair", + ]); + // Dynamic on both sides is the same defect, not a different one. + expect(messageIds('')).toEqual([ + "redundantDisabledPair", + ]); + // A submit button is not exempt: the pairing is wrong regardless of type. + expect(messageIds('')).toEqual([ + "redundantDisabledPair", + ]); + }); + + it("does not flag a pair where one side is statically off", () => { + expect(messageIds('')).toEqual( + [], + ); + expect(messageIds('')).toEqual([]); + }); + + it("keeps the spread escape hatch", () => { + expect(messageIds('')).toEqual([]); }); - it("keeps the spread escape hatch", () => { + it("keeps the spread escape hatch for unwired checks only", () => { expect(messageIds('')).toEqual([]); }); + it("flags aria-disabled without an onClick, because aria-disabled alone stays operable", () => { + expect(messageIds('')).toEqual(["ariaDisabledNeedsHandler"]); + // Explicit after a spread still needs a handler — the spread escape must not hide it. + expect(messageIds('')).toEqual([ "redundantDisabledPair",