peter/siftlode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(ui): a dialog opens focused where it said it would, and the wizard is a dialog

Browse Source 
Deep-review round 5, over round 4's own fixes.

- Round 4 gave `ui/Modal` a persistent portal host so a fullscreen transition would stop tearing
  the dialog down. The host was attached in a `useLayoutEffect` — one commit phase AFTER React
  runs `autoFocus` in `commitMount`, and `focus()` on a disconnected node is a no-op. Every
  autoFocus in every dialog was dead: Mod+K opened the command palette with the focus on the ✕,
  so typing searched nothing and Enter closed it; the same for the download rename field and
  every non-danger confirm. The host is attached in a `useInsertionEffect` now, a whole phase
  earlier, and the whole re-parent — including the focus save/restore across the move, which
  `appendChild` needs because it is remove-then-insert — lives in one `useFullscreenPortalHost`.
  `ui/AnchoredPopover` had a second copy of that re-parent WITHOUT the focus restore, and a
  comment claiming focus survived it; both now come from the hook.

- `ui/Toaster` classified a toast raised in the SAME commit as a host takeover among the ones it
  inherits, because it read the store live at the takeover render. A dialog that notifies and
  closes in one handler (UploadDialog) therefore put its toast up with no role, into a region
  still `aria-live="off"` — announced by neither path. "Inherited" is now what the outgoing host
  last had on screen, and a toast that is NOT inherited is announced even inside the unarmed
  window.

- The onboarding wizard renders its own shell, and Escape ordering was all of the dialog contract
  it had: no role, no label, no initial focus, no trap, no restore. A keyboard user tabbed out of
  the first-login wizard into the app behind the scrim. The focus half of the contract is now one
  shared `ui/dialogFocus` that both `ui/Modal` and the wizard use.

Every fix has a regression test, each mutation-checked (6 mutations, all red on the assertion
they are named after).
This commit is contained in:
peter
2026-09-12 03:41:37 +02:00
parent 545996f807
commit 41b652f8fd
9 changed files with 366 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/components/ui/AnchoredPopover.tsx
+9 -15
View File
@@ -3,7 +3,6 @@ import {
useContext,
useEffect,
useId,
useLayoutEffect,
useRef,
useState,
type ButtonHTMLAttributes,
@@ -37,7 +36,7 @@ import {
type UseRoleProps,
} from "@floating-ui/react";
import { useLayer } from "../../lib/layerStack";
import { useFullscreenElement } from "../../lib/fullscreen";
import { useFullscreenPortalHost } from "../../lib/fullscreen";
// R8 popover platform. ONE anchored-popover primitive over @floating-ui/react, replacing the ~9
// hand-rolled positioning engines (measured flip / width clamp / rAF scroll-follow / scroller-relative
@@ -528,19 +527,14 @@ function PopoverSurfaceInner({
// A node portaled to <body> is painted UNDERNEATH whatever holds the fullscreen lock, so a popover
// opened from a player's control bar in fullscreen would be invisible; it must portal INTO the
// fullscreen element. We own one persistent container div and re-parent it (body ↔ fullscreen
// element) with appendChild, which MOVES the node atomically — no detach, so the open panel's
// focus, caret, scroll offset and animation state survive a fullscreen transition, and an element
// SWAP (A→B without exiting) is handled by the element-identity dependency. (Keying the portal to
// remount instead would rebuild the subtree and lose all of that.)
const fsRoot = useFullscreenElement();
const [container] = useState<HTMLDivElement | null>(() =>
typeof document === "undefined" ? null : document.createElement("div"),
);
useLayoutEffect(() => {
if (container) (fsRoot ?? document.body).appendChild(container);
}, [container, fsRoot]);
useEffect(() => () => container?.remove(), [container]);
// fullscreen element. One persistent container div is re-parented (body ↔ fullscreen element)
// rather than handed to the portal as a changing container, so the open panel's caret, scroll
// offset and animation state survive the transition, and an element SWAP (A→B without exiting) is
// handled by the element-identity dependency. (Keying the portal to remount instead would rebuild
// the subtree and lose all of that.) Focus does NOT survive on its own — `appendChild` on an
// already-parented node is remove-then-insert, which blurs to <body>; the hook saves and restores
// it. This used to be a second copy of Modal's re-parent that lacked exactly that (round 5).
const container = useFullscreenPortalHost();
const { style: floatingPropStyle, ...floatingProps } = getFloatingProps() as {
style?: CSSProperties;
frontend/src/components/ui/Modal.fullscreen.dom.test.tsx
+35
View File
@@ -50,3 +50,38 @@ describe("a dialog open while fullscreen is left", () => {
stage.remove();
});
});
describe("a dialog with an autoFocus child", () => {
it("opens with the focus in that child, not on the close button", () => {
// The portal host is created detached and attached by the hook. If it is attached a phase too
// late (a `useLayoutEffect` instead of `useInsertionEffect`), every descendant's `autoFocus`
// has already called `.focus()` on a disconnected node — a no-op — and Modal's own fallback
// then puts the focus on the first focusable in the panel, the header's ✕. That is what the
// command palette, the download rename dialog and every non-danger confirm shipped with after
// deep-review round 4 (found in round 5).
render(
<Modal title="Search" onClose={() => {}}>
<input aria-label="query" autoFocus />
</Modal>,
);
expect(document.activeElement).toBe(screen.getByLabelText("query"));
});
it("still has it after a fullscreen transition moves the host", async () => {
const stage = document.createElement("div");
document.body.appendChild(stage);
render(
<Modal title="Search" onClose={() => {}}>
<input aria-label="query" autoFocus />
</Modal>,
);
const field = screen.getByLabelText("query");
expect(document.activeElement).toBe(field);
await setFullscreen(stage); // the player goes fullscreen under the open dialog
expect(stage.contains(field)).toBe(true);
expect(document.activeElement).toBe(field);
stage.remove();
});
});
frontend/src/components/ui/Modal.tsx
+12 -67
View File
@@ -1,21 +1,14 @@
import { useEffect, useId, useLayoutEffect, useRef, useState, type ReactNode } from "react";
import { useEffect, useId, useRef, type ReactNode } from "react";
import { createPortal } from "react-dom";
import { useTranslation } from "react-i18next";
import { X } from "lucide-react";
import { useBackToClose } from "../../lib/history";
import { pushLayer, removeLayer, isTopmost } from "../../lib/layerStack";
import { useFullscreenElement } from "../../lib/fullscreen";
import { useFullscreenPortalHost } from "../../lib/fullscreen";
import { trapTab, useDialogChrome } from "./dialogFocus";
import { Button } from "./Button";
import Toaster, { useOverlayOrder } from "./Toaster";
// Tabbable elements inside the dialog, in DOM order. `:not([tabindex="-1"])` drops programmatically
// focusable-only nodes; the visibility filter (offsetParent) drops display:none ones (e.g. a
// collapsed disclosure) so the trap never lands focus on something the user can't see.
const FOCUSABLE =
'a[href], button:not([disabled]), textarea:not([disabled]), input:not([disabled]), select:not([disabled]), [tabindex]:not([tabindex="-1"])';
const focusablesIn = (root: HTMLElement): HTMLElement[] =>
[...root.querySelectorAll<HTMLElement>(FOCUSABLE)].filter((el) => el.offsetParent !== null);
// Small centered modal shell: backdrop + ESC + scroll-lock close. Each open modal is a layer in the
// shared `layerStack` so ESC only closes the topmost one — e.g. an error dialog over the tag editor
// dismisses just the error and returns to the editor underneath, and a player below defers to it
@@ -44,31 +37,13 @@ export default function Modal({
const { t } = useTranslation();
// Browser/mouse Back closes the topmost modal instead of navigating away.
useBackToClose(onClose);
const fsRoot = useFullscreenElement();
// ONE container element for the dialog's whole life, MOVED between <body> and the fullscreen
// element rather than handed to `createPortal` as a changing container: React tears a portal down
// and builds it again when its container changes, and the dialog's contents went with it — a
// half-filled download form reset to its defaults, and focus fell to <body>, the moment the viewer
// left true fullscreen (deep-review round 4).
const [host] = useState(() => document.createElement("div"));
useLayoutEffect(() => {
const parent = fsRoot ?? document.body;
if (host.parentNode === parent) return;
// appendChild MOVES the node: the browser blurs whatever was focused inside it on the way out.
const focused = host.contains(document.activeElement)
? (document.activeElement as HTMLElement)
: null;
parent.appendChild(host);
focused?.focus();
}, [fsRoot, host]);
useEffect(() => () => host.remove(), [host]);
// ONE container element for the dialog's whole life, re-parented between <body> and the fullscreen
// element (deep-review round 4) — and attached before the children commit, so an `autoFocus` child
// still gets the focus (round 5). Both rules live in the hook; `ui/AnchoredPopover` shares it.
const host = useFullscreenPortalHost();
const panelRef = useRef<HTMLDivElement>(null);
const titleId = useId();
// 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 on <body>. useState initializer runs exactly once, on first render.
const [restoreTo] = useState(() => document.activeElement as HTMLElement | null);
useDialogChrome(panelRef);
const overlayOrder = useOverlayOrder();
// Backdrop-click close must only fire when the press STARTED on the backdrop too. Otherwise a
@@ -96,50 +71,20 @@ export default function Modal({
if (!panel) return;
// The whole dialog, not just the panel: its toast host sits beside the panel (see ui/Toaster),
// and a toast's Undo or dismiss must be reachable from the keyboard like everything else.
const scope = rootRef.current ?? panel;
const items = focusablesIn(scope);
const first = items[0];
const last = items[items.length - 1];
if (!first || !last) {
e.preventDefault();
return;
}
const active = document.activeElement;
if (!scope.contains(active) || active === panel) {
// Focus escaped the dialog, OR sits on the panel container itself (tabIndex=-1, reached by
// clicking dialog dead space) — Node.contains(self) is true, so `active === panel` must be
// checked explicitly or Shift+Tab from the panel would leak to the background. Pull it back.
e.preventDefault();
(e.shiftKey ? last : first).focus();
} else if (e.shiftKey && active === first) {
e.preventDefault();
last.focus();
} else if (!e.shiftKey && active === last) {
e.preventDefault();
first.focus();
}
trapTab(e, rootRef.current ?? panel, panel);
}
};
window.addEventListener("keydown", onKey);
const prev = document.body.style.overflow;
document.body.style.overflow = "hidden";
// Initial focus: unless an autoFocus child already placed it inside (e.g. a rename input), move
// focus to the first control so the dialog opens focused, not stranded on the background.
const panel = panelRef.current;
if (panel && !panel.contains(document.activeElement)) {
(focusablesIn(panel)[0] ?? panel).focus();
}
return () => {
window.removeEventListener("keydown", onKey);
removeLayer(id);
document.body.style.overflow = prev;
// Restore focus to the trigger, if it's still in the document and focusable.
if (restoreTo && restoreTo.isConnected) restoreTo.focus();
};
}, [restoreTo]);
}, []);
const drawer = placement === "drawer";
const top = placement === "top";
// Only on a server render, where the hook has no `document` to build a host from.
if (!host) return null;
return createPortal(
<div
ref={rootRef}
frontend/src/components/ui/Toaster.dom.test.tsx
+36
View File
@@ -6,6 +6,7 @@
// the player. An overlay mounts its own host, and exactly one host shows the toasts.
import { describe, it, expect, vi, afterEach } from "vitest";
import { render, screen, fireEvent, cleanup, act } from "@testing-library/react";
import { useState } from "react";
vi.mock("react-i18next", async (importOriginal) => {
const actual = await importOriginal<typeof import("react-i18next")>();
@@ -89,6 +90,41 @@ describe("Toaster — handing the toasts to another host", () => {
expect(region.getAttribute("aria-live")).toBe("polite"); // and polite for what comes next
});
it("announces a toast RAISED in the same commit as the takeover, and still not the inherited ones", () => {
// A dialog that notifies and closes in ONE handler (UploadDialog: "added to the queue" + close)
// puts its toast into the very batch that hands the app host the top spot. Classifying it from
// the store live at the takeover render counted it among the inherited, so it went in with no
// role into a region that was still `aria-live="off"` — announced by neither path (round 5).
function Stage() {
const [open, setOpen] = useState(true);
return (
<>
<Toaster />
{open && (
<>
<Toaster overlay />
<button
onClick={() => {
notify({ level: "success", message: "upload queued" });
setOpen(false);
}}
>
upload
</button>
</>
)}
</>
);
}
render(<Stage />);
act(() => void notify({ level: "error", message: "old news" }));
fireEvent.click(screen.getByText("upload"));
const card = (text: string) => screen.getByText(text).closest(".toast-card")!;
expect(card("upload queued").getAttribute("role")).toBe("status");
expect(card("old news").getAttribute("role")).toBeNull();
});
it("ranks hosts by when their OVERLAY opened, not by when the host mounted", async () => {
// PlayerModal's host moves into the fullscreen stage and back: remounted, it used to jump over a
// dialog opened above the player, and the toasts went back under that dialog's scrim.
frontend/src/components/ui/Toaster.tsx
+23 -5
View File
@@ -41,6 +41,9 @@ const topHost = () => {
return top?.id ?? null;
};
// What the host that is currently top last rendered, for the next host to inherit. See `inherited`.
let lastTopIds: number[] = [];
export default function Toaster({
overlay = false,
order,
@@ -69,10 +72,19 @@ export default function Toaster({
// LATER task. Not in an effect: React flushes those in the same task as the render that inserted the
// toasts, and a screen reader reads the region's state after the task — by which time it was polite
// again, i.e. the arming did nothing.
const inherited = useMemo(
() => new Set(isTop ? getActiveToasts().map((n) => n.id) : []),
[isTop],
);
//
// "Inherited" is what the PREVIOUS top host last had on screen — not whatever is in the store at
// the takeover render. Reading the store live also silenced a toast RAISED in the same commit as
// the takeover: a dialog that notifies and closes in one handler (UploadDialog) put its success
// toast up in the very batch that handed the app host the top spot, so it was classified as
// inherited and announced by neither path (deep-review round 5). Written during render on purpose
// — 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.
if (isTop) lastTopIds = toasts.map((n) => n.id);
const [armed, setArmed] = useState(false);
useEffect(() => {
if (!isTop) {
@@ -110,7 +122,13 @@ export default function Toaster({
return (
<div
key={toast.id}
role={urgent && !inherited.has(toast.id) ? "alert" : undefined}
// A toast this host did not inherit must be announced even in the unarmed window, where
// the region is still `off`: urgent ones assertively, the rest with their own polite
// `role="status"` (single-element, so its implied atomic re-read is just this toast).
// Once armed, the region itself does the announcing and the role comes back off.
role={
inherited.has(toast.id) ? undefined : urgent ? "alert" : armed ? undefined : "status"
}
className="toast-card glass pointer-events-auto relative overflow-hidden rounded-xl px-3 py-3 flex items-start gap-3 animate-[popIn_0.16s_ease]"
>
<Icon className={`w-5 h-5 shrink-0 mt-0.5 ${color}`} />
frontend/src/components/ui/dialogFocus.ts
+72
View File
@@ -0,0 +1,72 @@
import { useEffect, useState, type RefObject } from "react";
// The focus half of the dialog contract, in ONE place. `ui/Modal` owns it for every dialog that goes
// through the shell; the onboarding wizard renders its own shell (a bare `ui/Overlay`, because it is
// not a titled dialog with a header row) and used to have NONE of this — no trap, no initial focus,
// no restore — so a keyboard user tabbed straight out of the first-login wizard into the app behind
// the scrim, where the focus ring is invisible (deep-review round 5). A second hand-written copy is
// how two shells drift apart; this is the shared one.
// Tabbable elements inside the dialog, in DOM order. `:not([tabindex="-1"])` drops programmatically
// focusable-only nodes; the visibility filter (offsetParent) drops display:none ones (e.g. a
// collapsed disclosure) so the trap never lands focus on something the user can't see.
const FOCUSABLE =
'a[href], button:not([disabled]), textarea:not([disabled]), input:not([disabled]), select:not([disabled]), [tabindex]:not([tabindex="-1"])';
const focusablesIn = (root: HTMLElement): HTMLElement[] =>
[...root.querySelectorAll<HTMLElement>(FOCUSABLE)].filter((el) => el.offsetParent !== null);
/** Keep Tab / Shift+Tab cycling inside `scope` instead of reaching the inert page behind the
* backdrop. Call from a keydown handler that has already decided this dialog is the topmost layer —
* a nested dialog must defer to its child. `container` is the scope's own tabIndex=-1 panel, if it
* has one: focus can land there by clicking dialog dead space, and `Node.contains(self)` is true, so
* without naming it a Shift+Tab from the panel would leak to the background. */
export function trapTab(
e: KeyboardEvent,
scope: HTMLElement,
container?: HTMLElement | null,
): void {
const items = focusablesIn(scope);
const first = items[0];
const last = items[items.length - 1];
if (!first || !last) {
e.preventDefault();
return;
}
const active = document.activeElement;
if (!scope.contains(active) || (container != null && active === container)) {
e.preventDefault();
(e.shiftKey ? last : first).focus();
} else if (e.shiftKey && active === first) {
e.preventDefault();
last.focus();
} else if (!e.shiftKey && active === last) {
e.preventDefault();
first.focus();
}
}
/** 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<HTMLElement | null>): 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
// on <body>. useState initializer runs exactly once, on first render.
const [restoreTo] = useState(() => document.activeElement as HTMLElement | null);
useEffect(() => {
const prev = document.body.style.overflow;
document.body.style.overflow = "hidden";
// Initial focus: unless an autoFocus child already placed it inside (e.g. a rename input), move
// focus to the first control so the dialog opens focused, not stranded on the background.
const scope = scopeRef.current;
if (scope && !scope.contains(document.activeElement)) {
(focusablesIn(scope)[0] ?? scope).focus();
}
return () => {
document.body.style.overflow = prev;
// Restore focus to the trigger, if it's still in the document and focusable.
if (restoreTo && restoreTo.isConnected) restoreTo.focus();
};
}, [restoreTo, scopeRef]);
}