fix(ui): a dialog keeps the focus it was given, and hands it back where it took it from
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 <body>. 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 <body>: 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.
This commit is contained in:
@@ -8,7 +8,7 @@
|
|||||||
// `focusablesIn` filters on `el.offsetParent !== null` and jsdom reports null for everything, so the
|
// `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.
|
// 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 { 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";
|
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||||
|
|
||||||
vi.mock("react-i18next", async (importOriginal) => {
|
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 OnboardingWizard from "./OnboardingWizard";
|
||||||
import { api, type Me } from "../lib/api";
|
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;
|
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);
|
expect(document.activeElement).toBe(outside);
|
||||||
outside.remove();
|
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<HTMLElement>("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 <body> (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 `<Toaster overlay>` 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<HTMLElement>("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());
|
||||||
|
}
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -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
|
// 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.
|
// and the roles come from the same place `ui/Modal` gets them, not from a second hand-written copy.
|
||||||
const panelRef = useRef<HTMLDivElement>(null);
|
const panelRef = useRef<HTMLDivElement>(null);
|
||||||
|
const rootRef = useRef<HTMLDivElement>(null);
|
||||||
const titleId = useId();
|
const titleId = useId();
|
||||||
useDialogChrome(panelRef);
|
useDialogChrome(panelRef, rootRef);
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
const onKey = (e: KeyboardEvent) => {
|
const onKey = (e: KeyboardEvent) => {
|
||||||
// Only while nothing is above us: an error dialog opened from inside the wizard owns the Tab.
|
// Only while nothing is above us: an error dialog opened from inside the wizard owns the Tab.
|
||||||
if (e.key !== "Tab" || !layer.isTopmost()) return;
|
if (e.key !== "Tab" || !layer.isTopmost()) return;
|
||||||
const panel = panelRef.current;
|
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);
|
window.addEventListener("keydown", onKey);
|
||||||
return () => window.removeEventListener("keydown", onKey);
|
return () => window.removeEventListener("keydown", onKey);
|
||||||
@@ -145,7 +153,7 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: ()
|
|||||||
|
|
||||||
return (
|
return (
|
||||||
<Overlay>
|
<Overlay>
|
||||||
<div className="fixed inset-0 z-overlay grid place-items-center p-4">
|
<div ref={rootRef} className="fixed inset-0 z-overlay grid place-items-center p-4">
|
||||||
<div
|
<div
|
||||||
className="absolute inset-0 bg-black/50 animate-[overlayIn_0.2s_ease]"
|
className="absolute inset-0 bg-black/50 animate-[overlayIn_0.2s_ease]"
|
||||||
onClick={close}
|
onClick={close}
|
||||||
|
|||||||
@@ -29,6 +29,7 @@ import { HttpError } from "../lib/api";
|
|||||||
import { setLanguage, type LangCode } from "../i18n";
|
import { setLanguage, type LangCode } from "../i18n";
|
||||||
import LanguageSwitcher from "./LanguageSwitcher";
|
import LanguageSwitcher from "./LanguageSwitcher";
|
||||||
import { Button } from "./ui/Button";
|
import { Button } from "./ui/Button";
|
||||||
|
import { trapTab, useDialogChrome } from "./ui/dialogFocus";
|
||||||
import { inputCls } from "./ui/form";
|
import { inputCls } from "./ui/form";
|
||||||
import { entryHash, entrySearch } from "../lib/entryUrl";
|
import { entryHash, entrySearch } from "../lib/entryUrl";
|
||||||
import type { VerifyEntry } from "../lib/useVerifyEntry";
|
import type { VerifyEntry } from "../lib/useVerifyEntry";
|
||||||
@@ -266,21 +267,27 @@ function Preview({
|
|||||||
// (aspect ratio preserved via object-contain). Closes on backdrop click, the ✕, or Escape.
|
// (aspect ratio preserved via object-contain). Closes on backdrop click, the ✕, or Escape.
|
||||||
function Lightbox({ src, label, onClose }: { src: string; label: string; onClose: () => void }) {
|
function Lightbox({ src, label, onClose }: { src: string; label: string; onClose: () => void }) {
|
||||||
const { t } = useTranslation();
|
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<HTMLDivElement>(null);
|
||||||
|
useDialogChrome(panelRef);
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
const onKey = (e: KeyboardEvent) => {
|
const onKey = (e: KeyboardEvent) => {
|
||||||
if (e.key === "Escape") onClose();
|
if (e.key === "Escape") onClose();
|
||||||
|
const panel = panelRef.current;
|
||||||
|
if (e.key === "Tab" && panel) trapTab(e, panel, panel);
|
||||||
};
|
};
|
||||||
window.addEventListener("keydown", onKey);
|
window.addEventListener("keydown", onKey);
|
||||||
const prev = document.body.style.overflow;
|
return () => window.removeEventListener("keydown", onKey);
|
||||||
document.body.style.overflow = "hidden"; // no background scroll while open
|
|
||||||
return () => {
|
|
||||||
window.removeEventListener("keydown", onKey);
|
|
||||||
document.body.style.overflow = prev;
|
|
||||||
};
|
|
||||||
}, [onClose]);
|
}, [onClose]);
|
||||||
return (
|
return (
|
||||||
<div
|
<div
|
||||||
className="fixed inset-0 z-popover grid place-items-center bg-black/80 overlay-backdrop p-4 sm:p-8"
|
ref={panelRef}
|
||||||
|
tabIndex={-1}
|
||||||
|
className="fixed inset-0 z-popover grid place-items-center bg-black/80 overlay-backdrop p-4 sm:p-8 outline-none"
|
||||||
onClick={onClose}
|
onClick={onClose}
|
||||||
role="dialog"
|
role="dialog"
|
||||||
aria-modal="true"
|
aria-modal="true"
|
||||||
|
|||||||
@@ -10,6 +10,7 @@
|
|||||||
// The overlay/history machinery (useBackToClose) is exercised in history.dom.test.ts, so it's mocked
|
// The overlay/history machinery (useBackToClose) is exercised in history.dom.test.ts, so it's mocked
|
||||||
// out here to isolate the focus-trap behaviour from its module-global history side-effects.
|
// out here to isolate the focus-trap behaviour from its module-global history side-effects.
|
||||||
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
|
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
|
||||||
|
import { useState } from "react";
|
||||||
import { render, screen, act, cleanup } from "@testing-library/react";
|
import { render, screen, act, cleanup } from "@testing-library/react";
|
||||||
|
|
||||||
vi.mock("react-i18next", async (importOriginal) => {
|
vi.mock("react-i18next", async (importOriginal) => {
|
||||||
@@ -199,6 +200,78 @@ describe("Modal focus trap (offsetParent stubbed 'visible')", () => {
|
|||||||
expect(document.activeElement).toBe(trigger);
|
expect(document.activeElement).toBe(trigger);
|
||||||
document.body.removeChild(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 <body> (deep-review round 6).
|
||||||
|
const trigger = document.createElement("button");
|
||||||
|
document.body.appendChild(trigger);
|
||||||
|
trigger.focus();
|
||||||
|
const { unmount } = render(
|
||||||
|
<Modal title="t" onClose={() => {}}>
|
||||||
|
<Body />
|
||||||
|
</Modal>,
|
||||||
|
);
|
||||||
|
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 <body>.
|
||||||
|
// 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 (
|
||||||
|
<Modal title="outer" onClose={() => {}}>
|
||||||
|
<button onClick={() => setOpen(true)}>open inner</button>
|
||||||
|
{open && (
|
||||||
|
<Modal title="inner" onClose={() => {}}>
|
||||||
|
<button>inner-body</button>
|
||||||
|
</Modal>
|
||||||
|
)}
|
||||||
|
</Modal>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
const { unmount } = render(<Nested />);
|
||||||
|
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(
|
||||||
|
<Modal title="t" onClose={() => {}}>
|
||||||
|
<Body />
|
||||||
|
</Modal>,
|
||||||
|
);
|
||||||
|
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)", () => {
|
describe("Modal focus trap (raw jsdom — every offsetParent null)", () => {
|
||||||
|
|||||||
@@ -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<typeof import("react-i18next")>();
|
||||||
|
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(
|
||||||
|
<StrictMode>
|
||||||
|
<Modal title="t" onClose={() => {}}>
|
||||||
|
<input aria-label="field" autoFocus />
|
||||||
|
</Modal>
|
||||||
|
</StrictMode>,
|
||||||
|
);
|
||||||
|
// Without the remembered focus the second mount lands on the first focusable — the header ✕.
|
||||||
|
expect(document.activeElement).toBe(screen.getByLabelText("field"));
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -42,15 +42,18 @@ export default function Modal({
|
|||||||
// still gets the focus (round 5). Both rules live in the hook; `ui/AnchoredPopover` shares it.
|
// still gets the focus (round 5). Both rules live in the hook; `ui/AnchoredPopover` shares it.
|
||||||
const host = useFullscreenPortalHost();
|
const host = useFullscreenPortalHost();
|
||||||
const panelRef = useRef<HTMLDivElement>(null);
|
const panelRef = useRef<HTMLDivElement>(null);
|
||||||
|
const rootRef = useRef<HTMLDivElement>(null);
|
||||||
const titleId = useId();
|
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();
|
const overlayOrder = useOverlayOrder();
|
||||||
|
|
||||||
// Backdrop-click close must only fire when the press STARTED on the backdrop too. Otherwise a
|
// 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
|
// 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).
|
// the backdrop wrongly dismisses — the click event bubbles to the common ancestor (the backdrop).
|
||||||
const pressedOnBackdrop = useRef(false);
|
const pressedOnBackdrop = useRef(false);
|
||||||
const rootRef = useRef<HTMLDivElement>(null);
|
|
||||||
|
|
||||||
// Keep a stable handler across renders so the stack id is assigned once per mount.
|
// Keep a stable handler across renders so the stack id is assigned once per mount.
|
||||||
const onCloseRef = useRef(onClose);
|
const onCloseRef = useRef(onClose);
|
||||||
|
|||||||
@@ -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<typeof import("react-i18next")>();
|
||||||
|
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 (
|
||||||
|
<>
|
||||||
|
<button
|
||||||
|
onClick={() => {
|
||||||
|
notify({ level: "success", message: "Uploaded" });
|
||||||
|
setOpen(false); // …in the SAME handler: one batch, one commit
|
||||||
|
}}
|
||||||
|
>
|
||||||
|
finish
|
||||||
|
</button>
|
||||||
|
{open && <Overlay />}
|
||||||
|
<Toaster />
|
||||||
|
</>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
function Overlay() {
|
||||||
|
const order = useOverlayOrder();
|
||||||
|
return <Toaster overlay order={order} />;
|
||||||
|
}
|
||||||
|
|
||||||
|
describe("the toast handover", () => {
|
||||||
|
it("announces a toast raised in the takeover commit, even under StrictMode", () => {
|
||||||
|
render(
|
||||||
|
<StrictMode>
|
||||||
|
<Harness />
|
||||||
|
</StrictMode>,
|
||||||
|
);
|
||||||
|
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");
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -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 { useTranslation } from "react-i18next";
|
||||||
import { AlertCircle, AlertTriangle, CheckCircle2, Info, X, XCircle } from "lucide-react";
|
import { AlertCircle, AlertTriangle, CheckCircle2, Info, X, XCircle } from "lucide-react";
|
||||||
import { dismiss, getActiveToasts, subscribe, type NotifLevel } from "../../lib/notifications";
|
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`.
|
// What the host that is currently top last rendered, for the next host to inherit. See `inherited`.
|
||||||
let lastTopIds: number[] = [];
|
let lastTopIds: number[] = [];
|
||||||
|
const EMPTY_IDS: ReadonlySet<number> = new Set<number>();
|
||||||
|
|
||||||
export default function Toaster({
|
export default function Toaster({
|
||||||
overlay = false,
|
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
|
// — 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
|
// 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.
|
// 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
|
// Snapshotted through a ref keyed on `isTop`, not a `useMemo`: React re-invokes a component body
|
||||||
// would otherwise overwrite the handover with the toasts it is about to classify.
|
// — 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<number> }>({
|
||||||
|
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);
|
if (isTop) lastTopIds = toasts.map((n) => n.id);
|
||||||
const [armed, setArmed] = useState(false);
|
const [armed, setArmed] = useState(false);
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
|
|||||||
@@ -47,8 +47,13 @@ export function trapTab(
|
|||||||
}
|
}
|
||||||
|
|
||||||
/** The rest of the contract: lock the page behind, open focused, hand the focus back on close.
|
/** 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. */
|
* `scopeRef` is the panel the focus should land in; `ownerRef`, when the dialog is bigger than its
|
||||||
export function useDialogChrome(scopeRef: RefObject<HTMLElement | null>): void {
|
* 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<HTMLElement | null>,
|
||||||
|
ownerRef?: RefObject<HTMLElement | null>,
|
||||||
|
): void {
|
||||||
// Capture the trigger to restore focus to, during THIS render — i.e. before commit runs the
|
// 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
|
// 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
|
// 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<HTMLElement | null>): void {
|
|||||||
// time a passive cleanup runs on a real unmount the panel is already out of the document and the
|
// time a passive cleanup runs on a real unmount the panel is already out of the document and the
|
||||||
// focus has fallen to <body>, so "does the panel contain activeElement" answers false for both a
|
// focus has fallen to <body>, so "does the panel contain activeElement" answers false for both a
|
||||||
// close and a user who clicked away — and those two want opposite things.
|
// 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
|
||||||
|
// <body> 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);
|
const heldRef = useRef(true);
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
const prev = document.body.style.overflow;
|
const prev = document.body.style.overflow;
|
||||||
@@ -82,9 +94,21 @@ export function useDialogChrome(scopeRef: RefObject<HTMLElement | null>): void {
|
|||||||
target.focus();
|
target.focus();
|
||||||
}
|
}
|
||||||
const onFocusIn = () => {
|
const onFocusIn = () => {
|
||||||
const inside = scopeRef.current?.contains(document.activeElement) ?? false;
|
const owner = ownerRef?.current ?? scopeRef.current;
|
||||||
heldRef.current = inside;
|
const active = document.activeElement;
|
||||||
if (inside) focusedRef.current = document.activeElement as HTMLElement;
|
// "Still ours" also means a dialog stacked ABOVE us. A nested `ui/Modal` portals into its own
|
||||||
|
// host on <body>, 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 <body>: 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();
|
onFocusIn();
|
||||||
document.addEventListener("focusin", onFocusIn);
|
document.addEventListener("focusin", onFocusIn);
|
||||||
@@ -96,5 +120,5 @@ export function useDialogChrome(scopeRef: RefObject<HTMLElement | null>): void {
|
|||||||
// rudest thing a closing dialog can do.
|
// rudest thing a closing dialog can do.
|
||||||
if (heldRef.current && restoreTo && restoreTo.isConnected) restoreTo.focus();
|
if (heldRef.current && restoreTo && restoreTo.isConnected) restoreTo.focus();
|
||||||
};
|
};
|
||||||
}, [restoreTo, scopeRef]);
|
}, [restoreTo, scopeRef, ownerRef]);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -9,7 +9,7 @@
|
|||||||
// i18n is mocked to a key-passthrough so queries read stable keys, and the API module is stubbed —
|
// 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.
|
// the card probes /auth/config on mount and must not reach the network.
|
||||||
import { describe, it, expect, afterEach, beforeEach, vi } from "vitest";
|
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) => {
|
vi.mock("react-i18next", async (importOriginal) => {
|
||||||
const actual = await importOriginal<typeof import("react-i18next")>();
|
const actual = await importOriginal<typeof import("react-i18next")>();
|
||||||
@@ -134,3 +134,48 @@ describe("the verification outcome handed down from App", () => {
|
|||||||
expect(screen.queryByText("welcome.auth.verifyInvalid")).toBeNull();
|
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(<Welcome verify={noVerify} />);
|
||||||
|
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));
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user