Skip to content

Commit b36cfed

Browse files
committed
fix(admin/spa): drop redundant abort guard and add Modal a11y
Two follow-ups from the review of #649: 1. useApi.ts: removed the `if (ctrl.signal.aborted) return;` check inside the catch handler. The cleanup function sets `cancelled = true` *before* calling `ctrl.abort()`, so any abort path is already covered by the preceding `cancelled` check. The second guard was harmless belt-and-suspenders dead code; the comment now records why one check is enough. 2. Modal.tsx: added the WAI-ARIA dialog contract (`role="dialog"`, `aria-modal="true"`, `aria-labelledby` pointing at the title) and a focus trap: - On open: store the previously-focused element, then focus the first focusable inside the dialog so keyboard users do not stay stranded on the page underneath. Empty dialogs fall back to focusing the dialog container itself with tabindex=-1. - Tab / Shift+Tab cycle within the dialog instead of escaping to the page underneath. - On close: restore focus to whoever opened the dialog (skipped when that element has been unmounted, e.g. the dialog deleted the row whose button opened it). - Escape and backdrop click stay wired through the existing `busy` gate so a half-applied operation cannot be dismissed. Implementation is vanilla DOM querying — focus-trap-react / react-focus-lock would have added 5–7 KB gzip for what is a ~40 line vanilla solution. Bundle delta from this PR is +0.4 KB gzip (60.23 -> 60.59 KB). Verified with `npm run build` and `npm run lint` (tsc --strict --noUnusedLocals --noUncheckedSideEffectImports), and `go build ./...`. Stacked on top of #649 (feat/admin-dashboard-spa); merge that one first, then this rebases cleanly onto main.
1 parent fb8085c commit b36cfed

2 files changed

Lines changed: 101 additions & 6 deletions

File tree

web/admin/src/components/Modal.tsx

Lines changed: 98 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useEffect } from "react";
1+
import { useEffect, useId, useRef } from "react";
22
import type { ReactNode } from "react";
33

44
interface ModalProps {
@@ -15,14 +15,69 @@ interface ModalProps {
1515
// Lightweight modal: no portal, no animation library. shadcn/ui's
1616
// Dialog primitive would pull in @radix-ui/react-dialog (~10KB gzip)
1717
// and we only need a single confirmation/edit surface per page.
18+
//
19+
// Accessibility (per the WAI-ARIA Authoring Practices for dialogs):
20+
// - role="dialog" + aria-modal="true" so AT announce the dialog
21+
// and treat the rest of the page as inert.
22+
// - aria-labelledby on the title <div> so the dialog is named.
23+
// - Focus is moved into the dialog on open and restored to the
24+
// previously-focused element on close.
25+
// - Tab and Shift+Tab are wrapped to keep focus inside the dialog
26+
// until it closes, so keyboard users cannot accidentally tab to
27+
// the page underneath.
1828
export function Modal({ title, open, onClose, children, busy }: ModalProps) {
29+
const dialogRef = useRef<HTMLDivElement>(null);
30+
const previouslyFocusedRef = useRef<HTMLElement | null>(null);
31+
const titleId = useId();
32+
1933
useEffect(() => {
2034
if (!open) return;
35+
36+
previouslyFocusedRef.current = (document.activeElement as HTMLElement | null) ?? null;
37+
// Focus the first focusable element so keyboard users land
38+
// inside the dialog instead of the page underneath. Run on the
39+
// next tick so the dialog DOM exists and any autofocus has had
40+
// a chance to settle.
41+
queueMicrotask(() => focusFirstFocusable(dialogRef.current));
42+
2143
const onKey = (e: KeyboardEvent) => {
22-
if (e.key === "Escape" && !busy) onClose();
44+
if (e.key === "Escape" && !busy) {
45+
onClose();
46+
return;
47+
}
48+
if (e.key !== "Tab") return;
49+
const root = dialogRef.current;
50+
if (!root) return;
51+
const focusables = focusableElements(root);
52+
if (focusables.length === 0) {
53+
// Empty dialog: keep Tab from leaving via the page.
54+
e.preventDefault();
55+
return;
56+
}
57+
const first = focusables[0];
58+
const last = focusables[focusables.length - 1];
59+
const active = document.activeElement;
60+
if (e.shiftKey && active === first) {
61+
e.preventDefault();
62+
last.focus();
63+
} else if (!e.shiftKey && active === last) {
64+
e.preventDefault();
65+
first.focus();
66+
}
2367
};
68+
2469
window.addEventListener("keydown", onKey);
25-
return () => window.removeEventListener("keydown", onKey);
70+
return () => {
71+
window.removeEventListener("keydown", onKey);
72+
// Restore focus to whoever opened the dialog. Guard against
73+
// the trigger being unmounted (e.g. the dialog deleted the
74+
// row whose button opened it) by checking isConnected.
75+
const restore = previouslyFocusedRef.current;
76+
previouslyFocusedRef.current = null;
77+
if (restore && restore.isConnected) {
78+
restore.focus();
79+
}
80+
};
2681
}, [open, busy, onClose]);
2782

2883
if (!open) return null;
@@ -33,9 +88,15 @@ export function Modal({ title, open, onClose, children, busy }: ModalProps) {
3388
if (e.target === e.currentTarget && !busy) onClose();
3489
}}
3590
>
36-
<div className="w-full max-w-md rounded-lg border border-border bg-surface shadow-xl">
91+
<div
92+
ref={dialogRef}
93+
role="dialog"
94+
aria-modal="true"
95+
aria-labelledby={titleId}
96+
className="w-full max-w-md rounded-lg border border-border bg-surface shadow-xl"
97+
>
3798
<div className="border-b border-border px-4 py-3 flex items-center">
38-
<div className="font-semibold text-sm">{title}</div>
99+
<div id={titleId} className="font-semibold text-sm">{title}</div>
39100
<button
40101
type="button"
41102
onClick={onClose}
@@ -51,3 +112,35 @@ export function Modal({ title, open, onClose, children, busy }: ModalProps) {
51112
</div>
52113
);
53114
}
115+
116+
// focusableSelector targets the elements an end user can tab to.
117+
// Excludes [tabindex="-1"] (programmatic-only focus targets) and
118+
// disabled / hidden inputs. Kept narrow on purpose: the modal only
119+
// hosts buttons / form fields / links, not embedded media or
120+
// content-editable surfaces.
121+
const focusableSelector = [
122+
"a[href]",
123+
"button:not([disabled])",
124+
"input:not([disabled]):not([type='hidden'])",
125+
"select:not([disabled])",
126+
"textarea:not([disabled])",
127+
'[tabindex]:not([tabindex="-1"])',
128+
].join(",");
129+
130+
function focusableElements(root: HTMLElement): HTMLElement[] {
131+
return Array.from(root.querySelectorAll<HTMLElement>(focusableSelector));
132+
}
133+
134+
function focusFirstFocusable(root: HTMLElement | null): void {
135+
if (!root) return;
136+
const focusables = focusableElements(root);
137+
if (focusables.length > 0) {
138+
focusables[0].focus();
139+
return;
140+
}
141+
// Fallback: focus the dialog container itself so screen readers
142+
// still announce it. Add tabindex=-1 dynamically so the browser
143+
// accepts the focus call without making the dialog tab-stop bait.
144+
root.setAttribute("tabindex", "-1");
145+
root.focus();
146+
}

web/admin/src/lib/useApi.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,10 @@ export function useApiQuery<T>(
4444
setLoading(false);
4545
})
4646
.catch((err: unknown) => {
47+
// The cleanup function (below) sets `cancelled = true` *before*
48+
// calling `ctrl.abort()`, so any abort path is already covered
49+
// by the `cancelled` check above. No second guard needed.
4750
if (cancelled) return;
48-
if (ctrl.signal.aborted) return;
4951
if (err instanceof ApiError) {
5052
if (err.status === 401) markUnauthorizedRef.current();
5153
setError(err);

0 commit comments

Comments
 (0)