From 41b652f8fd6408da585484c43c5ebc03ac06e77a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 12 Sep 2026 03:41:37 +0200 Subject: [PATCH] fix(ui): a dialog opens focused where it said it would, and the wizard is a dialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../OnboardingWizard.dialog.dom.test.tsx | 87 +++++++++++++++++++ frontend/src/components/OnboardingWizard.tsx | 49 +++++++++-- .../src/components/ui/AnchoredPopover.tsx | 24 ++--- .../ui/Modal.fullscreen.dom.test.tsx | 35 ++++++++ frontend/src/components/ui/Modal.tsx | 79 +++-------------- .../src/components/ui/Toaster.dom.test.tsx | 36 ++++++++ frontend/src/components/ui/Toaster.tsx | 28 ++++-- frontend/src/components/ui/dialogFocus.ts | 72 +++++++++++++++ frontend/src/lib/fullscreen.ts | 55 ++++++++++-- 9 files changed, 366 insertions(+), 99 deletions(-) create mode 100644 frontend/src/components/OnboardingWizard.dialog.dom.test.tsx create mode 100644 frontend/src/components/ui/dialogFocus.ts diff --git a/frontend/src/components/OnboardingWizard.dialog.dom.test.tsx b/frontend/src/components/OnboardingWizard.dialog.dom.test.tsx new file mode 100644 index 00000000..d7bc0bca --- /dev/null +++ b/frontend/src/components/OnboardingWizard.dialog.dom.test.tsx @@ -0,0 +1,87 @@ +// @vitest-environment jsdom +// +// The wizard is a modal overlay in every way that matters to a user, but it renders its own shell +// instead of going through `ui/Modal` — and for a keyboard or screen-reader user it had nothing of +// the dialog contract: no role, no label, no initial focus, no trap, no restore. Tab past its last +// control walked into the app behind the scrim, where the focus ring is invisible (round 5). +// +// `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 { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), + Trans: ({ i18nKey }: { i18nKey: string }) => i18nKey, + }; +}); +vi.mock("../lib/onboarding", () => ({ beginGrant: vi.fn(), endOnboarding: vi.fn() })); + +import OnboardingWizard from "./OnboardingWizard"; +import { api, type Me } from "../lib/api"; + +const me = { id: 1, email: "a@b", can_read: false, can_write: false } as unknown as Me; + +let origDesc: PropertyDescriptor | undefined; +beforeEach(() => { + vi.spyOn(api, "myStatus").mockResolvedValue({ channels_total: 0 } as never); + 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); + else delete (HTMLElement.prototype as unknown as Record).offsetParent; + vi.restoreAllMocks(); +}); + +const mount = () => + render( + + {}} /> + , + ); + +describe("the onboarding wizard as a dialog", () => { + it("announces itself as a labelled modal dialog", () => { + mount(); + const panel = screen.getByRole("dialog"); + expect(panel.getAttribute("aria-modal")).toBe("true"); + // The label is the step's own heading — the wizard has no header row to take a title from. + const heading = document.getElementById(panel.getAttribute("aria-labelledby")!); + expect(heading?.textContent).toBe("onboarding.read.title"); + }); + + it("opens focused inside itself and keeps Tab from leaving", () => { + const outside = document.createElement("button"); + outside.textContent = "behind the scrim"; + document.body.appendChild(outside); + outside.focus(); + + const { unmount } = mount(); + const panel = screen.getByRole("dialog"); + expect(panel.contains(document.activeElement)).toBe(true); + + // From the last control, Tab wraps back to the first instead of reaching the page behind. + const stops = [...panel.querySelectorAll("button, a[href]")]; + stops[stops.length - 1]!.focus(); + fireEvent.keyDown(window, { key: "Tab" }); + expect(document.activeElement).toBe(stops[0]); + + // …and closing hands the focus back to whatever opened it. + unmount(); + expect(document.activeElement).toBe(outside); + outside.remove(); + }); +}); diff --git a/frontend/src/components/OnboardingWizard.tsx b/frontend/src/components/OnboardingWizard.tsx index e8a7de83..57b1d377 100644 --- a/frontend/src/components/OnboardingWizard.tsx +++ b/frontend/src/components/OnboardingWizard.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef } from "react"; +import { useEffect, useId, useRef } from "react"; import { Trans, useTranslation } from "react-i18next"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { qk } from "../lib/queryKeys"; @@ -9,6 +9,7 @@ import { Button } from "./ui/Button"; import Overlay from "./ui/Overlay"; import Toaster, { useOverlayOrder } from "./ui/Toaster"; import { useLayer } from "../lib/layerStack"; +import { trapTab, useDialogChrome } from "./ui/dialogFocus"; // A first-login wizard that walks the user through granting YouTube access one scope at a // time, each with a plain explanation and an up-front heads-up about Google's "unverified @@ -109,7 +110,26 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: () // A layer in the shared stack, so Escape closes the wizard only when it is on top: a raw document // listener ran BEFORE the error dialog's (a sync that failed inside the wizard opens one), and one // Esc closed the error AND ended onboarding, skipping the write-scope step (deep-review round 4). - useLayer(true, close); + const layer = useLayer(true, close); + + // The rest of the dialog contract. This is a modal overlay in every way that matters to a user, but + // it renders its own shell (it is not a titled dialog with a header row), so Escape ordering was all + // it had: focus started on , nothing announced a dialog, and Tab past the last control walked + // 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 titleId = useId(); + useDialogChrome(panelRef); + 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); + }; + window.addEventListener("keydown", onKey); + return () => window.removeEventListener("keydown", onKey); + }, [layer]); // Are we currently importing (or about to)? Show a progress screen instead of the // "you're connected" step until the first import settles. @@ -130,7 +150,14 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: () className="absolute inset-0 bg-black/50 animate-[overlayIn_0.2s_ease]" onClick={close} /> -
+
-

{t("onboarding.read.title")}

+

+ {t("onboarding.read.title")} +

-

{t("onboarding.importing.title")}

+

+ {t("onboarding.importing.title")} +

{t("onboarding.importing.body")}

@@ -184,7 +215,9 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: ()
-

{t("onboarding.write.title")}

+

+ {t("onboarding.write.title")} +

{importSubs.isError ? ( <>{t("onboarding.write.importFailed")} @@ -241,7 +274,9 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: ()

-

{t("onboarding.done.title")}

+

+ {t("onboarding.done.title")} +

{t("onboarding.done.body")}

diff --git a/frontend/src/components/ui/AnchoredPopover.tsx b/frontend/src/components/ui/AnchoredPopover.tsx index 2e0daabb..deb95573 100644 --- a/frontend/src/components/ui/AnchoredPopover.tsx +++ b/frontend/src/components/ui/AnchoredPopover.tsx @@ -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 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(() => - 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 ; 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; diff --git a/frontend/src/components/ui/Modal.fullscreen.dom.test.tsx b/frontend/src/components/ui/Modal.fullscreen.dom.test.tsx index 42e1fb87..816fd154 100644 --- a/frontend/src/components/ui/Modal.fullscreen.dom.test.tsx +++ b/frontend/src/components/ui/Modal.fullscreen.dom.test.tsx @@ -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( + {}}> + + , + ); + 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( + {}}> + + , + ); + 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(); + }); +}); diff --git a/frontend/src/components/ui/Modal.tsx b/frontend/src/components/ui/Modal.tsx index 4cdc113a..cdb062ab 100644 --- a/frontend/src/components/ui/Modal.tsx +++ b/frontend/src/components/ui/Modal.tsx @@ -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(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 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 , 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 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(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 . 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(
{ const actual = await importOriginal(); @@ -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 ( + <> + + {open && ( + <> + + + + )} + + ); + } + render(); + 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. diff --git a/frontend/src/components/ui/Toaster.tsx b/frontend/src/components/ui/Toaster.tsx index 56d52e63..497d0608 100644 --- a/frontend/src/components/ui/Toaster.tsx +++ b/frontend/src/components/ui/Toaster.tsx @@ -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 (
diff --git a/frontend/src/components/ui/dialogFocus.ts b/frontend/src/components/ui/dialogFocus.ts new file mode 100644 index 00000000..25626aa0 --- /dev/null +++ b/frontend/src/components/ui/dialogFocus.ts @@ -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(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): 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 . 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]); +} diff --git a/frontend/src/lib/fullscreen.ts b/frontend/src/lib/fullscreen.ts index 7b907665..054ddb6b 100644 --- a/frontend/src/lib/fullscreen.ts +++ b/frontend/src/lib/fullscreen.ts @@ -1,4 +1,10 @@ -import { useSyncExternalStore } from "react"; +import { + useEffect, + useInsertionEffect, + useLayoutEffect, + useState, + useSyncExternalStore, +} from "react"; // The browser's Fullscreen API, WebKit included. // @@ -56,9 +62,48 @@ const fsSnapshot = () => fullscreenElement() as HTMLElement | null; const fsServerSnapshot = () => null; /** The fullscreen element as reactive state, for code that must RENDER differently while something - * holds the lock — chiefly the popover platform, which has to portal INTO that element (a node - * portaled to is painted underneath the fullscreen element, i.e. invisible). Components that - * merely react to the transition should keep using `onFullscreenChange` directly. */ -export function useFullscreenElement(): HTMLElement | null { + * holds the lock — today only `useFullscreenPortalHost` below, which has to portal INTO that element + * (a node portaled to is painted underneath the fullscreen element, i.e. invisible). + * Components that merely react to the transition should keep using `onFullscreenChange` directly. */ +function useFullscreenElement(): HTMLElement | null { return useSyncExternalStore(onFullscreenChange, fsSnapshot, fsServerSnapshot); } + +/** ONE container div for an overlay's whole life, re-parented between `` and the fullscreen + * element instead of handed to `createPortal` as a changing container: React tears a portal down + * and builds it again when its container changes, and the overlay's contents go with it (a + * half-filled form reset to its defaults the moment the viewer left true fullscreen). + * + * Two rules this owns, which the two hand-written copies it replaced disagreed about (deep-review + * round 5): + * + * 1. **It is attached BEFORE the portal's children commit.** `useInsertionEffect` runs in the + * mutation phase, and React handles `autoFocus` in `commitMount` during the LAYOUT phase — one + * whole phase later. Attaching in a `useLayoutEffect` is too late: layout effects commit + * children-before-parent, so every `autoFocus` descendant called `.focus()` on a node that was + * still disconnected, where `focus()` is a no-op. Every autoFocus in every dialog was dead. + * 2. **`appendChild` MOVES the node, it does not teleport it.** Per the DOM spec an already-parented + * node is removed and re-inserted, and removing a focused element's ancestor resets focus to + * `` — so the focused element is saved and re-focused across the move. + */ +export function useFullscreenPortalHost(): HTMLElement | null { + const fsRoot = useFullscreenElement(); + const [host] = useState(() => + typeof document === "undefined" ? null : document.createElement("div"), + ); + useInsertionEffect(() => { + if (host && !host.parentNode) document.body.appendChild(host); + }, [host]); + useLayoutEffect(() => { + if (!host) return; + const parent = fsRoot ?? document.body; + if (host.parentNode === parent) return; + const focused = host.contains(document.activeElement) + ? (document.activeElement as HTMLElement) + : null; + parent.appendChild(host); + focused?.focus(); + }, [fsRoot, host]); + useEffect(() => () => host?.remove(), [host]); + return host; +}