From c7a377396b2fe8b0e862534652185e63c51c1483 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 12 Sep 2026 16:22:06 +0200 Subject: [PATCH] fix(ui): a closing dialog leaves the focus to the dialog standing above it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6 widened dialog focus ownership to count focus inside ANY `aria-modal` subtree as ours, so a dialog and the dialog stacked on it closing together would still hand the focus back. That clause has no way to tell a layer closing WITH us from one that stays open, so a dialog closing on its own — an in-flight mutation's `onSuccess`, not a click, since a scrim covers it — under a modal raised above it pulled the focus onto its own pre-open trigger, behind that scrim, where Enter still activated it. Three verifiers confirmed the mechanism and two corrected the severity down: it self-heals on the next Tab, which is why this is a containment violation rather than a broken end state. `lib/layerStack` cannot refine it. Measured against the real hook: at the instant the lower dialog's focus cleanup runs, the both-closing case and the sibling-stays-open case give identical readings — `anyLayerAbove` true, the upper layer topmost — because passive cleanups run parent-first and `useDialogChrome` is declared before the effect that calls `removeLayer`. Gating on the stack turns round 6's own test red. What separates them is the DOM: React detaches a deleted subtree before passive cleanups, so a layer closing with us is already disconnected while one that stays open is not. Both tests are green, and each was mutation-checked against the other. Verified in Chrome on localdev: opening the cheat sheet from a sidebar button and closing it restores that button; with the command palette stacked over the cheat sheet, closing the top one leaves the focus neither on the background trigger nor on , and closing both returns it to the trigger. Welcome's Lightbox got the trap and the restore in round 6 but not the layer membership the other shells have, so its Escape and Tab fired from a raw window listener whatever was on top — the shape the wizard's comment records being bitten by, unreachable today only by a mount ordering nothing enforces. It joins the stack. Its comment also read as though the four-shell sweep were finished: `PlayerModal` is still an `aria-modal` surface with no name, no trap and no restore, which is on ROADMAP.md now rather than implied to be done. --- frontend/src/components/Welcome.tsx | 43 +++++++++++++++---- frontend/src/components/ui/Modal.dom.test.tsx | 37 ++++++++++++++++ frontend/src/components/ui/dialogFocus.ts | 22 +++++++++- .../welcomeMode.render.dom.test.tsx | 25 +++++++++++ 4 files changed, 118 insertions(+), 9 deletions(-) diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index 68362b41..70d57391 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -30,6 +30,7 @@ import { setLanguage, type LangCode } from "../i18n"; import LanguageSwitcher from "./LanguageSwitcher"; import { Button } from "./ui/Button"; import { trapTab, useDialogChrome } from "./ui/dialogFocus"; +import { isTopmost, pushLayer, removeLayer } from "../lib/layerStack"; import { inputCls } from "./ui/form"; import { entryHash, entrySearch } from "../lib/entryUrl"; import type { VerifyEntry } from "../lib/useVerifyEntry"; @@ -267,22 +268,48 @@ function Preview({ // (aspect ratio preserved via object-contain). Closes on backdrop click, the ✕, or Escape. function Lightbox({ src, label, onClose }: { src: string; label: string; onClose: () => void }) { const { t } = useTranslation(); - // The fourth `aria-modal` shell in the app, and the one the round-5 extraction did not sweep in: - // it declared itself a modal dialog while leaving focus on the thumbnail behind an 80%-black - // scrim, announcing nothing, and letting Tab walk the landing page underneath — the ✕ reachable - // only after tabbing past every remaining gallery shot (deep-review round 6). The contract comes - // from the same place `ui/Modal` and the wizard get theirs; the body-scroll lock moved with it. + // An `aria-modal` shell that round 5's focus extraction did not sweep in: it declared itself a + // modal dialog while leaving focus on the thumbnail behind an 80%-black scrim, announcing nothing, + // and letting Tab walk the landing page underneath — the ✕ reachable only after tabbing past every + // remaining gallery shot (deep-review round 6). The contract comes from the same place `ui/Modal` + // and the wizard get theirs; the body-scroll lock moved with it, and round 7 added the layer + // membership below. + // + // It is the THIRD of four to get the contract, not the last: `PlayerModal` is also + // `role="dialog" aria-modal="true"` and still hand-rolls its own scroll lock with no accessible + // name, no Tab trap and no focus restore. Saying "the fourth shell" here read as though the sweep + // were finished, which is the false-completeness that let a demo refusal sit unconverted for a + // whole round. It is on ROADMAP.md instead (round 7). const panelRef = useRef(null); useDialogChrome(panelRef); + // Keep a stable handler across renders so the stack id is assigned once per mount (as in ui/Modal). + const onCloseRef = useRef(onClose); + onCloseRef.current = onClose; useEffect(() => { + // Round 6 gave this shell the trap and the restore but not the LAYER MEMBERSHIP the other three + // have, so its Escape and Tab fired from a raw window listener whatever was on top of it. That + // is the shape the wizard's own comment records being bitten by — "one Esc closed the error AND + // ended onboarding" — and it is unreachable today only because `App` returns `` before + // it mounts `ErrorDialog` and the Toaster, i.e. by an ordering nothing enforces. Joining the + // stack costs two lines and removes the trap-fights-trap failure mode from the next layer the + // landing page grows (deep-review round 7). + const id = pushLayer(); const onKey = (e: KeyboardEvent) => { - if (e.key === "Escape") onClose(); + if (!isTopmost(id)) return; + if (e.key === "Escape") { + e.stopPropagation(); + onCloseRef.current(); + return; + } const panel = panelRef.current; if (e.key === "Tab" && panel) trapTab(e, panel, panel); }; window.addEventListener("keydown", onKey); - return () => window.removeEventListener("keydown", onKey); - }, [onClose]); + return () => { + window.removeEventListener("keydown", onKey); + removeLayer(id); + }; + }, []); return (
{ document.body.removeChild(trigger); }); + it("leaves the focus alone when a dialog above it is still standing", () => { + // The other side of the clause above. Counting focus in ANY `aria-modal` subtree as ours cannot + // tell a layer closing WITH us from one that stays open, and the layer stack cannot either: at + // the moment this cleanup runs the child's `removeLayer` has not fired, so both cases read "a + // layer is above me" (measured). So a dialog that closes on its own — an in-flight mutation's + // `onSuccess`, not a click, since a scrim covers it — under a modal raised above it yanked the + // focus out of that standing dialog onto its own trigger, behind the scrim, where Enter still + // activated it (deep-review round 7). Sibling dialogs, not nested: that is the arrangement + // `ConfirmProvider` and the app-wide `ErrorDialog` actually use. + const trigger = document.createElement("button"); + document.body.appendChild(trigger); + trigger.focus(); + function Siblings() { + const [lower, setLower] = useState(true); + return ( + <> + {lower && ( + {}}> + + + )} + {}}> + + + + ); + } + render(); + const upper = screen.getByText("upper-body"); + upper.focus(); + // The lower dialog closes by itself while the upper one holds the focus. + act(() => void screen.getByText("close lower").click()); + expect(screen.queryByText("close lower")).toBeNull(); // it really did unmount + expect(document.activeElement).toBe(upper); + document.body.removeChild(trigger); + }); + it("leaves the focus alone when the user moved it out of the dialog themselves", () => { // The other half of the same rule, and the branch nothing drove: taking focus back from where // the user deliberately put it is the rudest thing a closing dialog can do. diff --git a/frontend/src/components/ui/dialogFocus.ts b/frontend/src/components/ui/dialogFocus.ts index 00d444ab..321ef0be 100644 --- a/frontend/src/components/ui/dialogFocus.ts +++ b/frontend/src/components/ui/dialogFocus.ts @@ -118,7 +118,27 @@ export function useDialogChrome( // Restore focus to the trigger, if it's still in the document and focusable — and only if the // dialog still held the focus. Once the user has moved it elsewhere, taking it back is the // rudest thing a closing dialog can do. - if (heldRef.current && restoreTo && restoreTo.isConnected) restoreTo.focus(); + // + // Not, though, when a dialog stacked above us is STILL STANDING. `heldRef` counts focus in any + // `aria-modal` subtree as ours (see above), which cannot tell a layer closing WITH us from one + // that stays open — and the layer stack cannot tell them apart either: at this instant the + // child's `removeLayer` has not run, because passive cleanups are parent-first and this hook + // is declared before that effect, so BOTH cases read "a layer is above me" (measured). What + // does separate them is the DOM: React detaches a deleted subtree before passive cleanups, so + // a layer closing with us is already disconnected while one that stays open is not. Without + // this, a dialog closing on its own (an in-flight mutation's `onSuccess`) under a modal raised + // above it pulled the focus out of that standing dialog onto our own trigger, behind its + // scrim — where Enter still activated it (deep-review round 7). + const owner = ownerRef?.current ?? scopeRef.current; + const active = document.activeElement; + const above = active instanceof Element ? active.closest('[aria-modal="true"]') : null; + const standingAbove = + above !== null && + above.isConnected && + !above.contains(owner as Node | null) && + !(owner?.contains(above) ?? false); + if (heldRef.current && !standingAbove && restoreTo && restoreTo.isConnected) + restoreTo.focus(); }; }, [restoreTo, scopeRef, ownerRef]); } diff --git a/frontend/src/components/welcomeMode.render.dom.test.tsx b/frontend/src/components/welcomeMode.render.dom.test.tsx index 1f2cced3..13172a1c 100644 --- a/frontend/src/components/welcomeMode.render.dom.test.tsx +++ b/frontend/src/components/welcomeMode.render.dom.test.tsx @@ -28,6 +28,7 @@ vi.mock("../lib/api", async (importOriginal) => { }); import Welcome from "./Welcome"; +import { pushLayer, removeLayer } from "../lib/layerStack"; import type { VerifyEntry } from "../lib/useVerifyEntry"; // App confirms the emailed token and hands the outcome down; these tests are about the card's @@ -178,4 +179,28 @@ describe("the screenshot lightbox", () => { fireEvent.keyDown(window, { key: "Escape" }); await waitFor(() => expect(document.activeElement).toBe(shot)); }); + + it("defers its Escape and its Tab to a layer opened above it", async () => { + // Round 6 gave this shell the trap and the restore but not the LAYER MEMBERSHIP the other + // three have, so both fired from a raw window listener no matter what was on top. That is the + // shape the wizard's own comment records being bitten by ("one Esc closed the error AND ended + // onboarding"), and it was unreachable here only because `App` renders `` before it + // mounts `ErrorDialog` and the Toaster — an ordering nothing enforces (round 7). + const { panel } = await openIt(); + const above = pushLayer(); // whatever the landing page grows next + try { + fireEvent.keyDown(window, { key: "Escape" }); + // Still open: the layer above owns that Escape. + expect(screen.queryByRole("dialog")).toBe(panel); + // And it must not fight the layer above for the Tab either — same reasoning as the test + // above, so assert the keydown was NOT cancelled rather than where the focus went. + screen.getByLabelText("common.close").focus(); + expect(fireEvent.keyDown(window, { key: "Tab", cancelable: true })).toBe(true); + } finally { + removeLayer(above); + } + // With the layer above gone it is topmost again and closes normally. + fireEvent.keyDown(window, { key: "Escape" }); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + }); });