From f8a31d48cfb774e4a47cf89f89900d14d8a3a819 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 12 Sep 2026 05:49:58 +0200 Subject: [PATCH] fix(ui): a dialog keeps the focus it was given, and hands it back where it took it from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 extracted `ui/dialogFocus` so no shell would hand-roll the focus contract, then introduced four ways to lose the focus anyway — all four measured in jsdom against the real components. **Ownership was narrower than the trap.** `ui/Modal` traps Tab over the whole dialog on purpose, so a toast's Undo or ✕ stays reachable; ownership was tracked over the panel alone. Tabbing to that ✕ therefore read as "the user left the dialog", and since removing a toast fires no `focusin` to re-arm, closing afterwards skipped the restore and left the focus on . Before this hook existed the restore was unconditional, so it was a regression. **A dialog stacked above us is still ours.** A nested `ui/Modal` portals into its own host, so it is outside the outer dialog's root however wide that scope is drawn — widening alone could not fix it. Both closing in one commit left the focus on : the outer skipped its restore, and the child's own target (a control inside the outer panel) was already detached, because passive cleanups run parent-first. Ownership now means "inside my dialog, or inside a modal layer above it", which is what the rule always intended. **The wizard disagreed with `ui/Modal` on its first line.** `trapTab(e, panel)`, with neither the dialog-root scope (so its own toast host was outside the cycle — the connection notice never auto-dismisses, and its ✕ was unreachable from the keyboard) nor the `container` argument the JSDoc exists for: `panel.contains( panel)` is true, so focus sitting ON the tabIndex=-1 panel matched no branch and a Shift+Tab walked into the app behind the scrim. Both call sites now pass the same three arguments. **The toast handover was a render-phase read-modify-write.** A `useMemo` read a module global that the next line overwrote; React re-invokes a component body and every `useMemo` factory in it on a StrictMode double render, and the LAST invocation commits — so the second pass re-read what the first had just written, the takeover host classified its OWN new toast as inherited, and the dev build behaved exactly like the code round 5 replaced. Snapshotted through a ref keyed on `isTop`, so a repeated invocation of one render is a no-op. And the fourth `aria-modal` shell, `Welcome`'s screenshot lightbox, is finally swept into the same contract: it declared itself a modal dialog while leaving the focus on the thumbnail behind an 80%-black scrim and letting Tab walk the landing page underneath. The headline fix of the previous commit — StrictMode's re-mount taking the focus off an `autoFocus` field — had no test at all; deleting it left all 1009 green. It has one now. 9 tests, mutation-checked against 8 mutations, each red on the test that names it. --- .../OnboardingWizard.dialog.dom.test.tsx | 60 ++++++++++++++- frontend/src/components/OnboardingWizard.tsx | 14 +++- frontend/src/components/Welcome.tsx | 21 ++++-- frontend/src/components/ui/Modal.dom.test.tsx | 73 +++++++++++++++++++ .../ui/Modal.strictmode.dom.test.tsx | 54 ++++++++++++++ frontend/src/components/ui/Modal.tsx | 7 +- .../ui/Toaster.strictmode.dom.test.tsx | 66 +++++++++++++++++ frontend/src/components/ui/Toaster.tsx | 24 +++++- frontend/src/components/ui/dialogFocus.ts | 36 +++++++-- .../welcomeMode.render.dom.test.tsx | 47 +++++++++++- 10 files changed, 378 insertions(+), 24 deletions(-) create mode 100644 frontend/src/components/ui/Modal.strictmode.dom.test.tsx create mode 100644 frontend/src/components/ui/Toaster.strictmode.dom.test.tsx diff --git a/frontend/src/components/OnboardingWizard.dialog.dom.test.tsx b/frontend/src/components/OnboardingWizard.dialog.dom.test.tsx index d7bc0bca..5e3194bd 100644 --- a/frontend/src/components/OnboardingWizard.dialog.dom.test.tsx +++ b/frontend/src/components/OnboardingWizard.dialog.dom.test.tsx @@ -8,7 +8,7 @@ // `focusablesIn` filters on `el.offsetParent !== null` and jsdom reports null for everything, so the // trap is untestable as written without the same stub `ui/Modal.dom.test.tsx` uses. import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { render, cleanup, screen, fireEvent } from "@testing-library/react"; +import { render, cleanup, screen, fireEvent, act } from "@testing-library/react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; vi.mock("react-i18next", async (importOriginal) => { @@ -23,6 +23,7 @@ vi.mock("../lib/onboarding", () => ({ beginGrant: vi.fn(), endOnboarding: vi.fn( import OnboardingWizard from "./OnboardingWizard"; import { api, type Me } from "../lib/api"; +import { clearAll, notify } from "../lib/notifications"; const me = { id: 1, email: "a@b", can_read: false, can_write: false } as unknown as Me; @@ -84,4 +85,61 @@ describe("the onboarding wizard as a dialog", () => { expect(document.activeElement).toBe(outside); outside.remove(); }); + + it("holds a Shift+Tab that starts on the panel itself", () => { + // `Node.contains(self)` is true, so focus sitting ON the tabIndex=-1 panel — where a click on + // its dead space puts it — matched no branch of `trapTab` and the browser's native Shift+Tab + // ran, into the app behind the scrim. That is what the third argument exists for, and this + // caller was written without it on the very commit that extracted the helper (round 6). + mount(); + const panel = screen.getByRole("dialog"); + panel.focus(); + expect(document.activeElement).toBe(panel); + const stops = [...panel.querySelectorAll("button, a[href]")]; + fireEvent.keyDown(window, { key: "Tab", shiftKey: true }); + expect(document.activeElement).toBe(stops[stops.length - 1]); + }); + + it("still hands the focus back after it visited the wizard's own toast", () => { + // Ownership has to be measured over the same scope the Tab trap uses. The wizard's toast host + // sits beside the panel and INSIDE the dialog, so tabbing to a toast's ✕ is legal — but read + // against the panel alone it said "the user left", and since removing the toast fires no + // focusin to re-arm, closing afterwards stranded the focus on (round 6). Unlike + // `ui/Modal`, whose root carries the aria-modal attribute, nothing else can rescue this one. + const outside = document.createElement("button"); + document.body.appendChild(outside); + outside.focus(); + const { unmount } = mount(); + act(() => void notify({ level: "error", message: "offline" })); + const dismissToast = screen.getByTitle("notifications.dismiss"); + dismissToast.focus(); + expect(document.activeElement).toBe(dismissToast); + act(() => clearAll()); // the toast goes away under the focus — no focusin follows + unmount(); + expect(document.activeElement).toBe(outside); + outside.remove(); + }); + + it("keeps its own toast's dismiss button inside the Tab cycle", () => { + // The wizard renders its own `` beside the panel, and the connection notice it + // can raise never auto-dismisses — so a trap scoped to the panel alone left that ✕ permanently + // unreachable from the keyboard. `ui/Modal` spans the whole dialog for exactly this reason. + mount(); + const panel = screen.getByRole("dialog"); + act(() => void notify({ level: "error", message: "offline" })); + try { + const dismissToast = screen.getByTitle("notifications.dismiss"); + expect(panel.contains(dismissToast)).toBe(false); + const stops = [...panel.querySelectorAll("button, a[href]")]; + stops[stops.length - 1]!.focus(); + fireEvent.keyDown(window, { key: "Tab" }); + // No longer the edge: Tab carries on (natively) towards the toast instead of wrapping. + expect(document.activeElement).toBe(stops[stops.length - 1]); + dismissToast.focus(); + fireEvent.keyDown(window, { key: "Tab" }); + expect(document.activeElement).toBe(stops[0]); + } finally { + act(() => clearAll()); + } + }); }); diff --git a/frontend/src/components/OnboardingWizard.tsx b/frontend/src/components/OnboardingWizard.tsx index 57b1d377..863b11d8 100644 --- a/frontend/src/components/OnboardingWizard.tsx +++ b/frontend/src/components/OnboardingWizard.tsx @@ -118,14 +118,22 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: () // into the app behind the scrim — where the focus ring is invisible (deep-review round 5). The trap // and the roles come from the same place `ui/Modal` gets them, not from a second hand-written copy. const panelRef = useRef(null); + const rootRef = useRef(null); const titleId = useId(); - useDialogChrome(panelRef); + useDialogChrome(panelRef, rootRef); useEffect(() => { const onKey = (e: KeyboardEvent) => { // Only while nothing is above us: an error dialog opened from inside the wizard owns the Tab. if (e.key !== "Tab" || !layer.isTopmost()) return; const panel = panelRef.current; - if (panel) trapTab(e, panel); + // The SAME three arguments `ui/Modal` passes, for the same two reasons — and this call had + // neither of them (round 6, on the commit that extracted the helper so the two shells could + // not disagree). Scope is the whole dialog, so the toast host beside the panel stays in the + // Tab cycle (a sticky connection notice's ✕ was unreachable from the keyboard); `container` + // is the tabIndex=-1 panel, because `panel.contains(panel)` is true, so focus sitting ON the + // panel — where a click on its dead space puts it — matched no branch at all and a Shift+Tab + // walked out into the app behind the scrim. + if (panel) trapTab(e, rootRef.current ?? panel, panel); }; window.addEventListener("keydown", onKey); return () => window.removeEventListener("keydown", onKey); @@ -145,7 +153,7 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: () return ( -
+
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. + const panelRef = useRef(null); + useDialogChrome(panelRef); useEffect(() => { const onKey = (e: KeyboardEvent) => { if (e.key === "Escape") onClose(); + const panel = panelRef.current; + if (e.key === "Tab" && panel) trapTab(e, panel, panel); }; window.addEventListener("keydown", onKey); - const prev = document.body.style.overflow; - document.body.style.overflow = "hidden"; // no background scroll while open - return () => { - window.removeEventListener("keydown", onKey); - document.body.style.overflow = prev; - }; + return () => window.removeEventListener("keydown", onKey); }, [onClose]); return (
{ @@ -199,6 +200,78 @@ describe("Modal focus trap (offsetParent stubbed 'visible')", () => { expect(document.activeElement).toBe(trigger); document.body.removeChild(trigger); }); + + it("still restores the trigger after the focus visited the dialog's own toast", () => { + // The Tab trap deliberately spans the dialog ROOT so a toast's Undo/✕ is reachable; ownership + // was tracked over the PANEL, so touching that ✕ read as "the user left the dialog" and the + // restore was skipped — and removing the toast fires no focusin to re-arm it. Measured landing + // on (deep-review round 6). + const trigger = document.createElement("button"); + document.body.appendChild(trigger); + trigger.focus(); + const { unmount } = render( + {}}> + + , + ); + act(() => void notify({ level: "error", message: "Nope" })); + const dismissToast = screen.getByTitle("notifications.dismiss"); + dismissToast.focus(); + expect(document.activeElement).toBe(dismissToast); // the scenario is actually being driven + act(() => clearAll()); // the toast goes away under the focus — no focusin follows + expect(document.activeElement).toBe(document.body); + unmount(); + expect(document.activeElement).toBe(trigger); + document.body.removeChild(trigger); + }); + + it("restores the trigger when a dialog and the dialog stacked on it close together", () => { + // A nested Modal portals into its OWN host, so it is outside the outer dialog's root however + // wide that scope is drawn — reading that as "the user walked away" left the focus on . + // The child cannot cover for it: passive cleanups run parent-first, and by then its own restore + // target (a control INSIDE the outer panel, captured when it opened) is already detached. + const trigger = document.createElement("button"); + document.body.appendChild(trigger); + trigger.focus(); + function Nested() { + const [open, setOpen] = useState(false); + return ( + {}}> + + {open && ( + {}}> + + + )} + + ); + } + const { unmount } = render(); + act(() => void screen.getByText("open inner").click()); + screen.getByText("inner-body").focus(); + unmount(); + expect(document.activeElement).toBe(trigger); + 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. + const trigger = document.createElement("button"); + const elsewhere = document.createElement("button"); + document.body.append(trigger, elsewhere); + trigger.focus(); + const { unmount } = render( + {}}> + + , + ); + elsewhere.focus(); + unmount(); + expect(document.activeElement).toBe(elsewhere); + document.body.removeChild(trigger); + document.body.removeChild(elsewhere); + }); }); describe("Modal focus trap (raw jsdom — every offsetParent null)", () => { diff --git a/frontend/src/components/ui/Modal.strictmode.dom.test.tsx b/frontend/src/components/ui/Modal.strictmode.dom.test.tsx new file mode 100644 index 00000000..29d818da --- /dev/null +++ b/frontend/src/components/ui/Modal.strictmode.dom.test.tsx @@ -0,0 +1,54 @@ +// @vitest-environment jsdom +// +// The dialog's focus behaviour under React 18 StrictMode — its own file because StrictMode's +// mount→cleanup→mount is what the code under test reacts to, and the module-level layer stack in +// ui/Modal (and the overlay-host registry in ui/Toaster) leaks between tests in one file. +// +// Round 5 fixed this in Chrome and shipped it with NO test: the second mount found the focus +// outside the dialog (its own cleanup had just handed it back to the trigger) and moved it to the +// first focusable — taking it off an `autoFocus` field a beat after the browser had put it there. +// The command palette opened focused on its ✕ in dev and on its search field in prod, from the same +// code. Round 6 measured that deleting the fix left all 1009 tests green, which is why this exists. +// +// Same offsetParent stub as Modal.dom.test.tsx: jsdom never lays out, so every element reports +// offsetParent null and `focusablesIn` would see nothing. +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { StrictMode } from "react"; +import { render, screen, cleanup } from "@testing-library/react"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useTranslation: () => ({ t: (key: string) => key }) }; +}); +vi.mock("../../lib/history", () => ({ useBackToClose: () => {} })); + +import Modal from "./Modal"; + +describe("Modal focus under StrictMode", () => { + let origDesc: PropertyDescriptor | undefined; + beforeEach(() => { + origDesc = Object.getOwnPropertyDescriptor(HTMLElement.prototype, "offsetParent"); + Object.defineProperty(HTMLElement.prototype, "offsetParent", { + configurable: true, + get(this: HTMLElement) { + return this.isConnected ? document.body : null; + }, + }); + }); + afterEach(() => { + cleanup(); + if (origDesc) Object.defineProperty(HTMLElement.prototype, "offsetParent", origDesc); + }); + + it("keeps the focus its autoFocus child asked for across the double mount", () => { + render( + + {}}> + + + , + ); + // Without the remembered focus the second mount lands on the first focusable — the header ✕. + expect(document.activeElement).toBe(screen.getByLabelText("field")); + }); +}); diff --git a/frontend/src/components/ui/Modal.tsx b/frontend/src/components/ui/Modal.tsx index cdb062ab..068baf70 100644 --- a/frontend/src/components/ui/Modal.tsx +++ b/frontend/src/components/ui/Modal.tsx @@ -42,15 +42,18 @@ export default function Modal({ // still gets the focus (round 5). Both rules live in the hook; `ui/AnchoredPopover` shares it. const host = useFullscreenPortalHost(); const panelRef = useRef(null); + const rootRef = useRef(null); const titleId = useId(); - useDialogChrome(panelRef); + // Focus lands in the panel; OWNERSHIP is the whole dialog — the same element the Tab trap below + // is scoped to, toast host included. Tracking ownership over the panel alone meant tabbing to an + // in-dialog toast counted as leaving, and the close-time restore was then skipped (round 6). + useDialogChrome(panelRef, rootRef); const overlayOrder = useOverlayOrder(); // Backdrop-click close must only fire when the press STARTED on the backdrop too. Otherwise a // drag that begins inside the dialog (e.g. selecting text in an input) and is released out on // the backdrop wrongly dismisses — the click event bubbles to the common ancestor (the backdrop). const pressedOnBackdrop = useRef(false); - const rootRef = useRef(null); // Keep a stable handler across renders so the stack id is assigned once per mount. const onCloseRef = useRef(onClose); diff --git a/frontend/src/components/ui/Toaster.strictmode.dom.test.tsx b/frontend/src/components/ui/Toaster.strictmode.dom.test.tsx new file mode 100644 index 00000000..877d3b93 --- /dev/null +++ b/frontend/src/components/ui/Toaster.strictmode.dom.test.tsx @@ -0,0 +1,66 @@ +// @vitest-environment jsdom +// +// The toast handover under React 18 StrictMode — its own file, because `lastTopIds` is module-level +// state that leaks between tests. +// +// Round 5 made a taking-over host inherit "what the PREVIOUS top host last had on screen" by reading +// a module global in a `useMemo` and overwriting it in the same render pass. React re-invokes a +// component body — and every `useMemo` factory in it — on a StrictMode double render, and the LAST +// invocation is what commits: so the second pass re-read what the first had just written, the host +// classified its OWN new toast as inherited, and the dev build behaved exactly like the code round 5 +// replaced. Measured in Chrome and in jsdom: role "status" without StrictMode, null with it. +// +// The named case is UploadDialog's — notify and close the overlay in ONE handler, so the toast is +// raised in the very commit that hands the app's host the top spot. +import { describe, it, expect, afterEach, vi } from "vitest"; +import { StrictMode, useState } from "react"; +import { render, screen, cleanup, fireEvent } from "@testing-library/react"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useTranslation: () => ({ t: (key: string) => key }) }; +}); + +import Toaster, { useOverlayOrder } from "./Toaster"; +import { clearAll, notify } from "../../lib/notifications"; + +afterEach(() => { + cleanup(); + clearAll(); +}); + +/** An overlay with its own toast host, plus the app's — the shape every dialog produces. */ +function Harness() { + const [open, setOpen] = useState(true); + return ( + <> + + {open && } + + + ); +} +function Overlay() { + const order = useOverlayOrder(); + return ; +} + +describe("the toast handover", () => { + it("announces a toast raised in the takeover commit, even under StrictMode", () => { + render( + + + , + ); + fireEvent.click(screen.getByText("finish")); + // Not inherited → it carries its own role, because the live region is not armed yet. + expect(screen.getByText("Uploaded").closest("[role]")?.getAttribute("role")).toBe("status"); + }); +}); diff --git a/frontend/src/components/ui/Toaster.tsx b/frontend/src/components/ui/Toaster.tsx index 497d0608..d2c83aeb 100644 --- a/frontend/src/components/ui/Toaster.tsx +++ b/frontend/src/components/ui/Toaster.tsx @@ -1,4 +1,4 @@ -import { useEffect, useId, useMemo, useState, useSyncExternalStore } from "react"; +import { useEffect, useId, useRef, useState, useSyncExternalStore } from "react"; import { useTranslation } from "react-i18next"; import { AlertCircle, AlertTriangle, CheckCircle2, Info, X, XCircle } from "lucide-react"; import { dismiss, getActiveToasts, subscribe, type NotifLevel } from "../../lib/notifications"; @@ -43,6 +43,7 @@ const topHost = () => { // What the host that is currently top last rendered, for the next host to inherit. See `inherited`. let lastTopIds: number[] = []; +const EMPTY_IDS: ReadonlySet = new Set(); export default function Toaster({ overlay = false, @@ -81,9 +82,24 @@ export default function Toaster({ // — the handover must be readable by the taking-over host in the SAME commit, and an effect // cleanup runs one phase too late; only the host that IS top writes, so the value is the losing // host's last screen either way. - const inherited = useMemo(() => new Set(isTop ? lastTopIds : []), [isTop]); - // AFTER the memo above has read the outgoing host's screen — this host's own first render as top - // would otherwise overwrite the handover with the toasts it is about to classify. + // + // Snapshotted through a ref keyed on `isTop`, not a `useMemo`: React re-invokes a component body + // — and every `useMemo` factory in it — on a StrictMode double render and on a restarted one, and + // the LAST invocation is what commits. With a memo the second pass re-read `lastTopIds` after the + // first pass had already overwritten it on the line below, so the takeover host classified its + // OWN new toast as inherited and the dev build behaved exactly like the code this replaced + // (measured in jsdom: role "status" without StrictMode, null with it — round 6). The ref only + // recomputes when `isTop` actually flips, so a repeated invocation of one render is a no-op. + const handover = useRef<{ top: boolean; ids: ReadonlySet }>({ + top: false, + ids: EMPTY_IDS, + }); + if (handover.current.top !== isTop) + handover.current = { top: isTop, ids: new Set(isTop ? lastTopIds : []) }; + const inherited = handover.current.ids; + // AFTER the snapshot above has read the outgoing host's screen — this host's own first render as + // top would otherwise overwrite the handover with the toasts it is about to classify. Writing the + // same value twice is harmless; that is the point of the ref. if (isTop) lastTopIds = toasts.map((n) => n.id); const [armed, setArmed] = useState(false); useEffect(() => { diff --git a/frontend/src/components/ui/dialogFocus.ts b/frontend/src/components/ui/dialogFocus.ts index f52e5b3a..00d444ab 100644 --- a/frontend/src/components/ui/dialogFocus.ts +++ b/frontend/src/components/ui/dialogFocus.ts @@ -47,8 +47,13 @@ export function trapTab( } /** The rest of the contract: lock the page behind, open focused, hand the focus back on close. - * `scopeRef` is the panel the focus should land in. */ -export function useDialogChrome(scopeRef: RefObject): void { + * `scopeRef` is the panel the focus should land in; `ownerRef`, when the dialog is bigger than its + * panel, is that whole dialog — it must be the SAME element `trapTab` is scoped to, or the two + * disagree about what counts as "inside" (see `heldRef`). */ +export function useDialogChrome( + scopeRef: RefObject, + ownerRef?: RefObject, +): void { // Capture the trigger to restore focus to, during THIS render — i.e. before commit runs the // dialog's autoFocus (or our own initial focus) and moves activeElement into the dialog. Reading it // later (in the effect) would capture a now-inside element that unmounts on close, stranding focus @@ -65,6 +70,13 @@ export function useDialogChrome(scopeRef: RefObject): void { // time a passive cleanup runs on a real unmount the panel is already out of the document and the // focus has fallen to , so "does the panel contain activeElement" answers false for both a // close and a user who clicked away — and those two want opposite things. + // + // Measured against the DIALOG, not the panel. The two scopes must be the same one the Tab trap + // uses, and `ui/Modal`'s trap deliberately spans the dialog root so an in-dialog toast's Undo/✕ + // stays reachable — so tabbing to that ✕ read as "the user left the dialog", and since removing + // the toast fires no `focusin` to re-arm, closing the dialog afterwards stranded the focus on + // instead of handing it back to the trigger. Measured in jsdom, and a regression: before + // this hook existed the restore was unconditional (deep-review round 6). const heldRef = useRef(true); useEffect(() => { const prev = document.body.style.overflow; @@ -82,9 +94,21 @@ export function useDialogChrome(scopeRef: RefObject): void { target.focus(); } const onFocusIn = () => { - const inside = scopeRef.current?.contains(document.activeElement) ?? false; - heldRef.current = inside; - if (inside) focusedRef.current = document.activeElement as HTMLElement; + const owner = ownerRef?.current ?? scopeRef.current; + const active = document.activeElement; + // "Still ours" also means a dialog stacked ABOVE us. A nested `ui/Modal` portals into its own + // host on , so it is outside our root BY CONSTRUCTION — widening the scope cannot see + // it — and while it held the focus we read that as the user walking away. Then both closing + // together left the focus on : ours skipped the restore, and the child's own + // `restoreTo` was already detached (passive cleanups run parent-first). Measured in jsdom, + // and it is the ordinary case — an error dialog raised over a dialog (round 6). + heldRef.current = + (owner?.contains(active) ?? false) || + (active instanceof Element && active.closest('[aria-modal="true"]') != null); + // Where to put the focus back on a re-mount, though: the PANEL's own last focus. A toast is + // gone by then, so restoring onto it would strand the focus exactly as before. + if (scopeRef.current?.contains(document.activeElement)) + focusedRef.current = document.activeElement as HTMLElement; }; onFocusIn(); document.addEventListener("focusin", onFocusIn); @@ -96,5 +120,5 @@ export function useDialogChrome(scopeRef: RefObject): void { // rudest thing a closing dialog can do. if (heldRef.current && restoreTo && restoreTo.isConnected) restoreTo.focus(); }; - }, [restoreTo, scopeRef]); + }, [restoreTo, scopeRef, ownerRef]); } diff --git a/frontend/src/components/welcomeMode.render.dom.test.tsx b/frontend/src/components/welcomeMode.render.dom.test.tsx index 2705bf23..1f2cced3 100644 --- a/frontend/src/components/welcomeMode.render.dom.test.tsx +++ b/frontend/src/components/welcomeMode.render.dom.test.tsx @@ -9,7 +9,7 @@ // i18n is mocked to a key-passthrough so queries read stable keys, and the API module is stubbed — // the card probes /auth/config on mount and must not reach the network. import { describe, it, expect, afterEach, beforeEach, vi } from "vitest"; -import { render, screen, fireEvent, cleanup, act } from "@testing-library/react"; +import { render, screen, fireEvent, cleanup, act, waitFor } from "@testing-library/react"; vi.mock("react-i18next", async (importOriginal) => { const actual = await importOriginal(); @@ -134,3 +134,48 @@ describe("the verification outcome handed down from App", () => { expect(screen.queryByText("welcome.auth.verifyInvalid")).toBeNull(); }); }); + +describe("the screenshot lightbox", () => { + // The fourth `aria-modal` surface in the app, and the one the round-5 focus extraction did not + // sweep in: it declared itself a modal dialog while leaving focus on the thumbnail behind an + // 80%-black scrim and letting Tab walk the landing page underneath (round 6). + let origDesc: PropertyDescriptor | undefined; + beforeEach(() => { + origDesc = Object.getOwnPropertyDescriptor(HTMLElement.prototype, "offsetParent"); + Object.defineProperty(HTMLElement.prototype, "offsetParent", { + configurable: true, + get(this: HTMLElement) { + return this.isConnected ? document.body : null; + }, + }); + }); + afterEach(() => { + if (origDesc) Object.defineProperty(HTMLElement.prototype, "offsetParent", origDesc); + }); + + const openIt = async () => { + render(); + const shot = (await screen.findAllByLabelText("welcome.preview.feed"))[0]!; + shot.focus(); + fireEvent.click(shot); + return { shot, panel: await screen.findByRole("dialog") }; + }; + + it("opens focused inside itself and holds the Tab instead of letting it walk the page", async () => { + const { panel } = await openIt(); + expect(panel.getAttribute("aria-modal")).toBe("true"); + expect(panel.contains(document.activeElement)).toBe(true); + // Assert the trap ACTED (the keydown was cancelled), not where focus ended up: jsdom never + // performs the browser's default Tab traversal, so "focus is still inside" is true with no trap + // at all — a green assertion about nothing (the mutation check caught exactly that). + screen.getByLabelText("common.close").focus(); + const notCancelled = fireEvent.keyDown(window, { key: "Tab", cancelable: true }); + expect(notCancelled).toBe(false); + }); + + it("hands the focus back to the thumbnail that opened it", async () => { + const { shot } = await openIt(); + fireEvent.keyDown(window, { key: "Escape" }); + await waitFor(() => expect(document.activeElement).toBe(shot)); + }); +});