fix(ui): a closing dialog leaves the focus to the dialog standing above it

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 <body>, 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.
This commit is contained in:
2026-09-12 16:22:06 +02:00
parent 8036251589
commit c7a377396b
4 changed files with 118 additions and 9 deletions
+35 -8
View File
@@ -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<HTMLDivElement>(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 `<Welcome/>` 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 (
<div
ref={panelRef}
@@ -254,6 +254,43 @@ describe("Modal focus trap (offsetParent stubbed 'visible')", () => {
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 && (
<Modal title="lower" onClose={() => {}}>
<button onClick={() => setLower(false)}>close lower</button>
</Modal>
)}
<Modal title="upper" onClose={() => {}}>
<button>upper-body</button>
</Modal>
</>
);
}
render(<Siblings />);
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.
+21 -1
View File
@@ -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]);
}
@@ -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 `<Welcome/>` 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());
});
});