Skip to content

Commit 1e0a924

Browse files
authored
fix(admin/spa): drop redundant abort guard and add Modal a11y (#650)
## Summary Two follow-ups from the [#649](#649) review that were intentionally deferred to keep that PR focused on the functional bugs. **Stacked on `feat/admin-dashboard-spa`** (#649). Once #649 merges, this rebases cleanly onto `main`. ### 1. `useApi.ts` — drop the redundant abort guard The `catch` handler had two checks back-to-back: ```ts if (cancelled) return; if (ctrl.signal.aborted) return; // ← unreachable ``` The cleanup function sets `cancelled = true` *before* calling `ctrl.abort()`, so any abort path is already covered by the first check. Dropped the second guard and recorded *why* one check is enough in a comment. ### 2. `Modal.tsx` — full WAI-ARIA dialog contract + focus trap Added: - `role="dialog"`, `aria-modal="true"`, `aria-labelledby` on the dialog root (uses `useId()` for the title id). - Focus management: - On open: store the previously-focused element, focus the first focusable inside the dialog. - Tab / Shift+Tab cycle within the dialog instead of escaping to the page underneath. - On close: restore focus to whoever opened the dialog (skipped if that element was unmounted by the dialog action — e.g. the dialog deleted the row whose button opened it). - Empty-dialog fallback focuses the dialog container itself with `tabindex=-1`. Vanilla DOM implementation — `focus-trap-react` / `react-focus-lock` would have been 5–7 KB gzip for what is a ~40 line solution. **Bundle delta**: +0.4 KB gzip (60.23 → 60.59 KB JS, 14.55 → 14.57 KB CSS). ## Test plan - [x] `cd web/admin && npm install && npm run build` - [x] `cd web/admin && npm run lint` — `tsc --strict` - [x] `go build ./...` - [ ] Manual smoke (after #649 merges and the dashboard is visitable): - Open Create Table / Delete Table dialog → focus lands inside the dialog. - Tab past the last button → wraps back to the close button. - Shift+Tab from the close button → wraps to the last form field. - Escape closes (when not `busy`) and focus returns to the trigger button. - Screen reader announces the dialog name from the title text.
2 parents bf67fa5 + 086480c commit 1e0a924

2 files changed

Lines changed: 114 additions & 5 deletions

File tree

web/admin/src/components/Modal.tsx

Lines changed: 111 additions & 4 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,11 +15,80 @@ 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+
33+
// Focus capture / restore is tied to `open` ONLY. Folding `busy`
34+
// into the same effect as the previous version did caused the
35+
// cleanup to fire on every busy toggle (e.g. the user clicks Save
36+
// and the parent flips busy=true): focus would briefly leave the
37+
// dialog, previouslyFocusedRef would get clobbered with the
38+
// trigger button, then the next run would snap focus back. Real
39+
// bug for screen-reader users — the dialog became "exited" mid-
40+
// operation. Splitting the effects keeps each invariant tied to
41+
// its own state slice (Claude review on #650).
42+
useEffect(() => {
43+
if (!open) return;
44+
previouslyFocusedRef.current = (document.activeElement as HTMLElement | null) ?? null;
45+
// queueMicrotask defers focus until after sibling useEffect
46+
// hooks (notably any autofocus on form fields inside the
47+
// dialog children) have settled, so we focus the truly-first
48+
// tab stop rather than racing with autofocus.
49+
queueMicrotask(() => focusFirstFocusable(dialogRef.current));
50+
return () => {
51+
// Restore focus to whoever opened the dialog. Guard against
52+
// the trigger being unmounted (e.g. the dialog deleted the
53+
// row whose button opened it) by checking isConnected.
54+
const restore = previouslyFocusedRef.current;
55+
previouslyFocusedRef.current = null;
56+
if (restore && restore.isConnected) {
57+
restore.focus();
58+
}
59+
};
60+
}, [open]);
61+
62+
// Keyboard handler legitimately needs `busy` (Esc gates on it) and
63+
// `onClose`, so it re-binds when either changes. Re-binding is
64+
// cheap — one window listener swap per change — and unlike the
65+
// focus effect it has no observable side effect on the user.
1966
useEffect(() => {
2067
if (!open) return;
2168
const onKey = (e: KeyboardEvent) => {
22-
if (e.key === "Escape" && !busy) onClose();
69+
if (e.key === "Escape" && !busy) {
70+
onClose();
71+
return;
72+
}
73+
if (e.key !== "Tab") return;
74+
const root = dialogRef.current;
75+
if (!root) return;
76+
const focusables = focusableElements(root);
77+
if (focusables.length === 0) {
78+
// Empty dialog: keep Tab from leaving via the page.
79+
e.preventDefault();
80+
return;
81+
}
82+
const first = focusables[0];
83+
const last = focusables[focusables.length - 1];
84+
const active = document.activeElement;
85+
if (e.shiftKey && active === first) {
86+
e.preventDefault();
87+
last.focus();
88+
} else if (!e.shiftKey && active === last) {
89+
e.preventDefault();
90+
first.focus();
91+
}
2392
};
2493
window.addEventListener("keydown", onKey);
2594
return () => window.removeEventListener("keydown", onKey);
@@ -33,9 +102,15 @@ export function Modal({ title, open, onClose, children, busy }: ModalProps) {
33102
if (e.target === e.currentTarget && !busy) onClose();
34103
}}
35104
>
36-
<div className="w-full max-w-md rounded-lg border border-border bg-surface shadow-xl">
105+
<div
106+
ref={dialogRef}
107+
role="dialog"
108+
aria-modal="true"
109+
aria-labelledby={titleId}
110+
className="w-full max-w-md rounded-lg border border-border bg-surface shadow-xl"
111+
>
37112
<div className="border-b border-border px-4 py-3 flex items-center">
38-
<div className="font-semibold text-sm">{title}</div>
113+
<div id={titleId} className="font-semibold text-sm">{title}</div>
39114
<button
40115
type="button"
41116
onClick={onClose}
@@ -51,3 +126,35 @@ export function Modal({ title, open, onClose, children, busy }: ModalProps) {
51126
</div>
52127
);
53128
}
129+
130+
// focusableSelector targets the elements an end user can tab to.
131+
// Excludes [tabindex="-1"] (programmatic-only focus targets) and
132+
// disabled / hidden inputs. Kept narrow on purpose: the modal only
133+
// hosts buttons / form fields / links, not embedded media or
134+
// content-editable surfaces.
135+
const focusableSelector = [
136+
"a[href]",
137+
"button:not([disabled])",
138+
"input:not([disabled]):not([type='hidden'])",
139+
"select:not([disabled])",
140+
"textarea:not([disabled])",
141+
'[tabindex]:not([tabindex="-1"])',
142+
].join(",");
143+
144+
function focusableElements(root: HTMLElement): HTMLElement[] {
145+
return Array.from(root.querySelectorAll<HTMLElement>(focusableSelector));
146+
}
147+
148+
function focusFirstFocusable(root: HTMLElement | null): void {
149+
if (!root) return;
150+
const focusables = focusableElements(root);
151+
if (focusables.length > 0) {
152+
focusables[0].focus();
153+
return;
154+
}
155+
// Fallback: focus the dialog container itself so screen readers
156+
// still announce it. Add tabindex=-1 dynamically so the browser
157+
// accepts the focus call without making the dialog tab-stop bait.
158+
root.setAttribute("tabindex", "-1");
159+
root.focus();
160+
}

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)