diff --git a/frontend/src/components/Home.dom.test.tsx b/frontend/src/components/Home.dom.test.tsx index c19f8d3e..bc6bb059 100644 --- a/frontend/src/components/Home.dom.test.tsx +++ b/frontend/src/components/Home.dom.test.tsx @@ -372,8 +372,8 @@ describe("Home", () => { it("an unrelated mutation does not put the reroll button into a spinner", async () => { // The other edge of the test above. Because the pick key sits under the `["discover"]` prefix, // every mark-watched, bookmark, pin and shelf move on this page refetches it in the BACKGROUND — - // so binding the button's `loading` to raw `isFetching` made it spin and, since `ui/Button` turns - // `loading` into `disabled`, swallow clicks after actions the user never associated with a + // so binding the button's `loading` to raw `isFetching` made it spin and, since a `loading` + // `ui/Button` swallows its clicks, ignore presses after actions the user never associated with a // reroll (round 2). A press is outstanding only while THIS key has no answer of its own. const rerollBtn = () => screen.getByText("home.hero.reroll").closest("button") as HTMLButtonElement; @@ -391,7 +391,6 @@ describe("Home", () => { // The positive control, and it is the half most easily broken while fixing the other: this is the // FIRST press, so there is no previous pick for the placeholder to hold, and a predicate resting // on `isPlaceholderData` alone would drop the spinner from exactly the press that needs it. - expect(rerollBtn().disabled).toBe(true); expect(rerollBtn().getAttribute("aria-busy")).toBe("true"); await act(async () => { @@ -409,7 +408,6 @@ describe("Home", () => { }); await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(2)); - expect(rerollBtn().disabled).toBe(false); expect(rerollBtn().getAttribute("aria-busy")).toBeNull(); }); @@ -434,7 +432,7 @@ describe("Home", () => { // `notifyManager`'s own `setTimeout(0)`, registered AFTER any timer this test schedules, so // "await one macrotask then assert" races it. Round 4 measured that race as an intermittent red. failPick(new Error("boom")); - await waitFor(() => expect(rerollBtn().disabled).toBe(false)); // the failure releases the button + await waitFor(() => expect(rerollBtn().getAttribute("aria-busy")).toBeNull()); // the failure releases the button // Now an unrelated action refetches that errored key in the background. discoverPick.mockReturnValue(new Promise(() => {})); @@ -442,7 +440,6 @@ describe("Home", () => { void qc.invalidateQueries({ queryKey: qk.discover() }); }); await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(2)); - expect(rerollBtn().disabled).toBe(false); expect(rerollBtn().getAttribute("aria-busy")).toBeNull(); }); @@ -479,14 +476,14 @@ describe("Home", () => { screen.getByText("home.hero.reroll").closest("button") as HTMLButtonElement; fireEvent.click(screen.getByText("home.hero.reroll")); await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(2)); - expect(rerollBtn().disabled).toBe(true); // the press is outstanding + expect(rerollBtn().getAttribute("aria-busy")).toBe("true"); // the press is outstanding failPick(new Error("boom")); // Wait for a signal that the REJECTION has landed, not for a fixed tick: react-query notifies // the observer through its own `setTimeout(0)`, and the empty state is already on screen while // the press is in flight — so asserting it straight away would pass with or without the fix. // The button releasing is that signal (`isFetched` flips once the key settles, error included). - await waitFor(() => expect(rerollBtn().disabled).toBe(false)); + await waitFor(() => expect(rerollBtn().getAttribute("aria-busy")).toBeNull()); expect(screen.getByText("home.hero.emptyBudget")).toBeTruthy(); expect(screen.queryByText("Hero v1")).toBeNull(); }); diff --git a/frontend/src/components/PlayerModal.dom.test.tsx b/frontend/src/components/PlayerModal.dom.test.tsx index e24c3c67..dee9b97c 100644 --- a/frontend/src/components/PlayerModal.dom.test.tsx +++ b/frontend/src/components/PlayerModal.dom.test.tsx @@ -290,3 +290,63 @@ describe("PlayerModal — toasts raised while it is open", () => { } }); }); + +// --- the dialog contract it now shares (ui/dialogFocus) ----------------------------------------- +// Until E11 this was the one `aria-modal` shell with no accessible name, no Tab trap and no focus +// restore: Tab walked out of an open player into the page behind the scrim, and closing it with +// Escape left a keyboard user on , their place in a long feed lost (deep-review round 7). +describe("PlayerModal — the dialog contract", () => { + const open = (onClose = () => {}) => { + vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {})); + const A = vid("A", 0); + return renderModal( + {}} />, + ); + }; + + it("is named by the video's title", () => { + open(); + expect(screen.getByRole("dialog", { name: "Video A" })).toBeTruthy(); + }); + + it("opens focused on its card, not on a control, so its shortcuts keep working", () => { + open(); + const active = document.activeElement as HTMLElement; + expect(active.tagName).not.toBe("BUTTON"); + expect(screen.getByTestId("player-modal").contains(active)).toBe(true); + }); + + it("gives the focus back to what opened it when it closes", () => { + const opener = document.createElement("button"); + document.body.appendChild(opener); + opener.focus(); + try { + const view = open(); + expect(document.activeElement).not.toBe(opener); + view.unmount(); + expect(document.activeElement).toBe(opener); + } finally { + opener.remove(); + } + }); + + it("keeps Tab inside the player", () => { + open(); + const dialog = screen.getByTestId("player-modal"); + const tabbables = [...dialog.querySelectorAll("button:not([disabled]), a[href]")]; + const last = tabbables[tabbables.length - 1]!; + last.focus(); + // jsdom has no layout, so the trap's visibility filter would see nothing tabbable; give every + // element an offsetParent for the duration of this check. + const spy = vi + .spyOn(HTMLElement.prototype, "offsetParent", "get") + .mockReturnValue(document.body); + try { + fireEvent.keyDown(window, { key: "Tab" }); + expect(dialog.contains(document.activeElement)).toBe(true); + expect(document.activeElement).not.toBe(last); + } finally { + spy.mockRestore(); + } + }); +}); diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 15ccd35e..2159391a 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useId, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { createPortal } from "react-dom"; import { useNavigationActions } from "./NavigationProvider"; @@ -33,6 +33,7 @@ import Avatar from "./ui/Avatar"; import AddToPlaylist from "./AddToPlaylist"; import { PopoverSurface, useAnchoredPopover } from "./ui/AnchoredPopover"; import { pushLayer, removeLayer, anyLayerAbove, escapeHandled } from "../lib/layerStack"; +import { trapTab, useDialogChrome } from "./ui/dialogFocus"; import DownloadButton from "./DownloadButton"; import { api, type Video, type VideoStatus } from "../lib/api"; import type { YTPlayer } from "../lib/youtube"; @@ -166,6 +167,12 @@ export default function PlayerModal({ // modal adjusts volume (not just the small player area). const dialogRef = useRef(null); const focusModal = () => cardRef.current?.focus(); + // The dialog contract every other `aria-modal` shell shares (ui/dialogFocus): the focus goes back + // to whatever opened the player, and the page lock is the one counted lock. It opens focused on the + // CARD, not its first button, so Space/F/arrows reach the shortcuts. Until E11 this was the one + // hand-rolled shell — no accessible name, no Tab trap, no restore (deep-review round 7). + useDialogChrome(cardRef, dialogRef, { focusScope: true }); + const titleId = useId(); const togglePlay = () => { const p = playerRef.current; if (!p || typeof p.getPlayerState !== "function") return; @@ -422,6 +429,13 @@ export default function PlayerModal({ nudgeSeek(secs); }; const onKey = (e: KeyboardEvent) => { + // The shared dialog Tab trap — only while nothing is layered above the player: a Modal opened + // over it (the download dialog) traps for itself. + if (e.key === "Tab") { + if (layerIdRef.current != null && anyLayerAbove(layerIdRef.current)) return; + if (dialogRef.current) trapTab(e, dialogRef.current, cardRef.current); + return; + } dispatch(e, "ytPlayer", { close: (ev) => { // Defer to any layer opened above the player FIRST (a Modal like the download dialog, or an @@ -468,11 +482,9 @@ export default function PlayerModal({ }; window.addEventListener("keydown", onKey); focusModal(); - const prevOverflow = document.body.style.overflow; - document.body.style.overflow = "hidden"; + // (The scroll lock is the shared, counted one in useDialogChrome.) return () => { window.removeEventListener("keydown", onKey); - document.body.style.overflow = prevOverflow; // (The debounced volume write is flushed by useYtVolume's own unmount cleanup.) }; }, [onClose]); @@ -511,6 +523,7 @@ export default function PlayerModal({ onClick={onClose} role="dialog" aria-modal="true" + aria-labelledby={titleId} > {/* The card, flanked by full-height glassy strips that step through the feed's order (or a playlist). Each strip sits just outside the card, spans its full height, and fades out at @@ -735,7 +748,7 @@ export default function PlayerModal({
{/* Title row — title (with hover description) on the left, Close on the right. */}
-

+

{/* Hover target is the text itself (inline), not the whole row. */} and lost their +// place (E10 review round 1, measured again in Chrome in E11 S2). Everything around the rail that is +// not the rename path is stubbed. +import { describe, it, expect, vi, afterEach, beforeAll } from "vitest"; +import { render, screen, fireEvent, cleanup, act } from "@testing-library/react"; +import { FloatingTree } from "@floating-ui/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactNode } from "react"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useTranslation: () => ({ t: (k: string) => k, i18n: { language: "en" } }) }; +}); +vi.mock("./PlaylistsProvider", () => ({ + usePlaylists: () => ({ search: "", selectedId: 1 }), + usePlaylistsActions: () => ({ setSelectedId: vi.fn() }), +})); +vi.mock("../lib/useMe", () => ({ useMe: () => ({ can_write: true }) })); +vi.mock("./comfortPin", () => ({ + useComfortPin: () => ({ item: () => [] }), + pinPlaylist: (id: number) => id, +})); +const rename = vi.fn(async () => {}); +vi.mock("./usePlaylistActions", () => ({ + usePlaylistActions: () => ({ rename, remove: vi.fn() }), +})); +// The SidePanel/PanelGroups chrome is not what is under test: render the rail's bodies directly. +vi.mock("./FilterRail", () => ({ + default: ({ items }: { items: { id: string; body: ReactNode }[] }) => ( + <> + {items.map((i) => ( +
{i.body}
+ ))} + + ), +})); + +import PlaylistsRail from "./PlaylistsRail"; +import { api, type Playlist } from "../lib/api"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; + globalThis.IntersectionObserver ??= Stub as unknown as typeof IntersectionObserver; + Element.prototype.scrollIntoView ??= () => {}; +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + rename.mockClear(); +}); + +const alpha = { + id: 1, + name: "alpha", + kind: "custom", + source: "local", + item_count: 3, + total_duration_seconds: 0, + cover_thumbnail: null, + dirty: false, +} as unknown as Playlist; + +async function openRename() { + vi.spyOn(api, "playlists").mockResolvedValue([alpha]); + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + + {}} + collapsed={false} + onToggleCollapse={() => {}} + /> + + , + ); + const row = (await screen.findByText("alpha")).closest("button")!; + act(() => row.focus()); + fireEvent.keyDown(row, { key: "F10", shiftKey: true }); + fireEvent.click(await screen.findByText("playlists.rename")); + // The menu runs its action a task after it closes (see ui/ActionMenu). + const field = await screen.findByRole("textbox", { name: "playlists.rename" }); + await vi.waitFor(() => expect(document.activeElement).toBe(field)); + return field as HTMLInputElement; +} + +const rowButton = () => screen.getByText("alpha").closest("button"); + +describe("PlaylistsRail · rename from the keyboard", () => { + it("Escape cancels and puts the focus back on the row", async () => { + const field = await openRename(); + fireEvent.change(field, { target: { value: "alpha2" } }); + fireEvent.keyDown(field, { key: "Escape" }); + await vi.waitFor(() => expect(document.activeElement).toBe(rowButton())); + expect(rename).not.toHaveBeenCalled(); + }); + + it("Enter commits and puts the focus back on the row", async () => { + const field = await openRename(); + fireEvent.change(field, { target: { value: "beta" } }); + fireEvent.keyDown(field, { key: "Enter" }); + expect(rename).toHaveBeenCalledWith(1, "beta"); + await vi.waitFor(() => expect(document.activeElement).toBe(rowButton())); + }); +}); diff --git a/frontend/src/components/PlaylistsRail.tsx b/frontend/src/components/PlaylistsRail.tsx index e07391f0..ec8053cf 100644 --- a/frontend/src/components/PlaylistsRail.tsx +++ b/frontend/src/components/PlaylistsRail.tsx @@ -167,35 +167,20 @@ export default function PlaylistsRail({ }, }); - // KNOWN GAP: Enter and Escape unmount the focused and re-render the row as a , ); const btn = screen.getByRole("button", { name: "Save" }) as HTMLButtonElement; - expect(btn.disabled).toBe(true); expect(btn.getAttribute("aria-busy")).toBe("true"); + expect(btn.getAttribute("aria-disabled")).toBe("true"); // the leading icon is swapped for a spinner expect(btn.querySelector("[data-testid=ic]")).toBeNull(); fireEvent.click(btn); expect(onClick).not.toHaveBeenCalled(); }); - it("loading disables even when an explicit disabled={false} is passed", () => { + // E11 S2, measured in Chrome: a focused button that becomes natively disabled drops the focus to + // and never gets it back, so every "Something else" press threw a keyboard user out of Home. + // jsdom has no focus fixup, so what is pinned here is the cause — native `disabled` stays off. + it("loading keeps the button natively enabled, so it keeps the focus", () => { + render(); + const btn = screen.getByRole("button", { name: "Reroll" }) as HTMLButtonElement; + expect(btn.disabled).toBe(false); + }); + + it("loading blocks clicks even when an explicit disabled={false} is passed", () => { // Regression: `disabled ?? loading` let an explicit boolean disabled shadow loading, so a // call-site with `disabled={!valid} loading={pending}` (valid → disabled=false) stayed - // clickable mid-mutation and double-submitted. The contract is: loading always disables. + // clickable mid-mutation and double-submitted. The contract is: loading always blocks. const onClick = vi.fn(); render( , ); - const btn = screen.getByRole("button", { name: "Create" }) as HTMLButtonElement; - expect(btn.disabled).toBe(true); - fireEvent.click(btn); + fireEvent.click(screen.getByRole("button", { name: "Create" })); expect(onClick).not.toHaveBeenCalled(); }); + it("a busy submit button cancels the form's submission", () => { + // Enter in a field submits through a click on the default button; the guard cancels that click. + const onSubmit = vi.fn((e: { preventDefault: () => void }) => e.preventDefault()); + render( +
+ +
, + ); + fireEvent.click(screen.getByRole("button", { name: "Send" })); + expect(onSubmit).not.toHaveBeenCalled(); + }); + + it("a non-busy button still calls onClick", () => { + const onClick = vi.fn(); + render(); + fireEvent.click(screen.getByRole("button", { name: "Go" })); + expect(onClick).toHaveBeenCalledTimes(1); + }); + it("a link gets no button padding, and an explicit size still wins", () => { // The size default is variant-dependent (link → "none"). Locking it because the failure is // silent: a link that quietly inherits px-3 py-2 reads as a button, and nothing errors. diff --git a/frontend/src/components/ui/Button.tsx b/frontend/src/components/ui/Button.tsx index 4db42540..98630d25 100644 --- a/frontend/src/components/ui/Button.tsx +++ b/frontend/src/components/ui/Button.tsx @@ -44,28 +44,32 @@ const VARIANTS: Record = { // and the ~100 hand-rolled ones had drifted into two different looks. That split IS the // inconsistency this epic exists to remove; the primitive follows the majority, not the reverse. outline: - "font-medium border border-border text-muted enabled:hover:text-accent enabled:hover:border-accent", - "outline-accent": "font-medium border border-accent/40 text-accent enabled:hover:bg-accent/10", + "font-medium border border-border text-muted enabled:not-aria-disabled:hover:text-accent enabled:not-aria-disabled:hover:border-accent", + "outline-accent": + "font-medium border border-accent/40 text-accent enabled:not-aria-disabled:hover:bg-accent/10", // Neutral until you reach for it. NOT `danger-soft`, which is already red at rest — the difference // is whether the button should advertise the destruction before the pointer arrives. "outline-danger": - "font-medium border border-border text-muted enabled:hover:text-danger enabled:hover:border-danger/40", - "outline-warn": "font-medium border border-warn/40 text-warn enabled:hover:bg-warn/10", - ghost: "font-medium text-muted enabled:hover:text-fg enabled:hover:bg-card", - glass: "font-medium glass-card glass-hover enabled:hover:text-accent", + "font-medium border border-border text-muted enabled:not-aria-disabled:hover:text-danger enabled:not-aria-disabled:hover:border-danger/40", + "outline-warn": + "font-medium border border-warn/40 text-warn enabled:not-aria-disabled:hover:bg-warn/10", + ghost: + "font-medium text-muted enabled:not-aria-disabled:hover:text-fg enabled:not-aria-disabled:hover:bg-card", + glass: "font-medium glass-card glass-hover enabled:not-aria-disabled:hover:text-accent", "danger-soft": - "font-medium border border-danger/60 text-danger enabled:hover:bg-danger/10 enabled:hover:border-danger", + "font-medium border border-danger/60 text-danger enabled:not-aria-disabled:hover:bg-danger/10 enabled:not-aria-disabled:hover:border-danger", // No font-weight here on purpose: BASE already sets font-medium, and a second font-* utility would // be a coin-flip (both are single-class utilities of equal specificity, and without tailwind-merge // the winner is decided by the generated stylesheet order, not by the class list order). - link: "font-semibold text-accent enabled:hover:underline", + link: "font-semibold text-accent enabled:not-aria-disabled:hover:underline", }; -// Why every hover above is `enabled:hover:` — Tailwind's `:not(:disabled)`. A disabled button that -// still lights up on hover advertises a click that does nothing, and the ONE migrated call site that -// had guarded its own hover (TagManager's Add) silently lost the guard when it moved onto this -// primitive (E10 review round 1). The contract belongs here, not in ~160 call sites: BASE dims a -// disabled button, and the variant must not paint over that dimming. +// Why every hover above is `enabled:not-aria-disabled:hover:` — neither natively disabled nor busy. A +// disabled button that still lights up on hover advertises a click that does nothing, and the ONE +// migrated call site that had guarded its own hover (TagManager's Add) silently lost the guard when it +// moved onto this primitive (E10 review round 1). `not-aria-disabled` is the `loading` half: a busy +// button stays natively enabled (see `loading` below) and must not light up either. The contract +// belongs here, not in ~160 call sites: BASE dims both, and the variant must not paint over it. // The geometry ladder, and the whole of it — a call site picks a rung, it does not invent one. The // rungs were derived from what the app actually used (counted during E10 S1b): `px-3 py-2 text-sm` @@ -86,7 +90,7 @@ const SIZES: Record = { // link looks like everywhere in this app) and the action buttons want medium. Two font-* utilities on // one element is a coin flip without tailwind-merge, so exactly one has to emit it — see the test. const BASE = - "inline-flex items-center justify-center transition disabled:opacity-50 disabled:cursor-not-allowed"; + "inline-flex items-center justify-center transition disabled:opacity-50 disabled:cursor-not-allowed aria-disabled:opacity-50 aria-disabled:cursor-not-allowed"; interface ButtonOwnProps extends ButtonHTMLAttributes { variant?: ButtonVariant; @@ -98,7 +102,8 @@ interface ButtonOwnProps extends ButtonHTMLAttributes { iconOnly?: boolean; /** Accessible name + tooltip — required for `iconOnly`, optional otherwise. */ label?: string; - /** Disable + swap the icon for a spinner + mark aria-busy. */ + /** Busy: swap the icon for a spinner, mark aria-busy + aria-disabled, and swallow activation — but + * NOT native `disabled`, which would throw the focus out of the page (see the render below). */ loading?: boolean; fullWidth?: boolean; } @@ -131,6 +136,7 @@ const ButtonImpl = forwardRef(function Button children, disabled, type, + onClick, ...rest }, ref, @@ -149,11 +155,23 @@ const ButtonImpl = forwardRef(function Button +

+ ); +} + +describe("useDialogChrome · the page scroll lock", () => { + // Each dialog used to save and restore `overflow` itself, so the one mounted SECOND but unmounted + // LAST put back the "hidden" it had saved — a page left locked with no dialog open. + it("unlocks only when the last dialog closes, in whatever order they close", () => { + document.body.style.overflow = ""; + const player = render(); + const modal = render(); + expect(document.body.style.overflow).toBe("hidden"); + + player.unmount(); // the one underneath goes first + expect(document.body.style.overflow).toBe("hidden"); // the other is still open + + modal.unmount(); + expect(document.body.style.overflow).toBe(""); + }); +}); + +describe("useDialogChrome · initial focus", () => { + it("lands on the first control by default", () => { + // jsdom has no layout, so the visibility filter (offsetParent) would see nothing focusable. + const spy = vi + .spyOn(HTMLElement.prototype, "offsetParent", "get") + .mockReturnValue(document.body); + try { + const { getByText } = render(); + expect(document.activeElement).toBe(getByText("first")); + } finally { + spy.mockRestore(); + } + }); + + it("lands on the scope itself with focusScope", () => { + // Same layout stub as above: without it jsdom sees no focusable child, so the scope would get + // the focus with or without the option and this would pin nothing. + const spy = vi + .spyOn(HTMLElement.prototype, "offsetParent", "get") + .mockReturnValue(document.body); + try { + const { getByTestId } = render(); + expect(document.activeElement).toBe(getByTestId("scope")); + } finally { + spy.mockRestore(); + } + }); +}); diff --git a/frontend/src/components/ui/dialogFocus.ts b/frontend/src/components/ui/dialogFocus.ts index 321ef0be..56d32440 100644 --- a/frontend/src/components/ui/dialogFocus.ts +++ b/frontend/src/components/ui/dialogFocus.ts @@ -46,13 +46,36 @@ export function trapTab( } } +// ONE body scroll lock for every open dialog, counted. Each dialog used to save and restore +// `overflow` on its own, so two unmounting out of order (the player's cleanup running before a Modal +// opened above it) put back a stale "hidden". Invisible today — the app scrolls inside
, not +// the document (measured, E11 S2) — but a count cannot be restored out of order, and PlayerModal's +// second copy of the lock is gone. +let scrollLocks = 0; +let unlockedOverflow = ""; +function lockBodyScroll(): () => void { + if (scrollLocks++ === 0) { + unlockedOverflow = document.body.style.overflow; + document.body.style.overflow = "hidden"; + } + let released = false; + return () => { + if (released) return; + released = true; + if (--scrollLocks === 0) document.body.style.overflow = unlockedOverflow; + }; +} + /** 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; `ownerRef`, when the dialog is bigger than its * panel, is that whole dialog — it must be the SAME element `trapTab` is scoped to, or the two - * disagree about what counts as "inside" (see `heldRef`). */ + * disagree about what counts as "inside" (see `heldRef`). `focusScope` opens the dialog focused on + * the scope element itself rather than its first control: the player keeps the focus on its card, so + * Space and the arrows reach its shortcuts instead of activating a button. */ export function useDialogChrome( scopeRef: RefObject, ownerRef?: RefObject, + { focusScope = false }: { focusScope?: boolean } = {}, ): 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 @@ -79,18 +102,17 @@ export function useDialogChrome( // this hook existed the restore was unconditional (deep-review round 6). const heldRef = useRef(true); useEffect(() => { - const prev = document.body.style.overflow; - document.body.style.overflow = "hidden"; + const unlockScroll = lockBodyScroll(); // Initial focus: unless an autoFocus child already placed it inside (e.g. a rename input), move - // focus to where this dialog was focused before — or, the first time, to the first control, so - // the dialog opens focused rather than stranded on the background. + // focus to where this dialog was focused before — or, the first time, to the first control (or + // the scope itself, with `focusScope`), so the dialog opens focused rather than stranded on the + // background. const scope = scopeRef.current; if (scope && !scope.contains(document.activeElement)) { const back = focusedRef.current; const target = (back?.isConnected && scope.contains(back) ? back : null) ?? - focusablesIn(scope)[0] ?? - scope; + (focusScope ? scope : (focusablesIn(scope)[0] ?? scope)); target.focus(); } const onFocusIn = () => { @@ -114,7 +136,7 @@ export function useDialogChrome( document.addEventListener("focusin", onFocusIn); return () => { document.removeEventListener("focusin", onFocusIn); - document.body.style.overflow = prev; + unlockScroll(); // 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. @@ -140,5 +162,5 @@ export function useDialogChrome( if (heldRef.current && !standingAbove && restoreTo && restoreTo.isConnected) restoreTo.focus(); }; - }, [restoreTo, scopeRef, ownerRef]); + }, [restoreTo, scopeRef, ownerRef, focusScope]); } diff --git a/frontend/src/lib/useHoverFocusWithin.dom.test.tsx b/frontend/src/lib/useHoverFocusWithin.dom.test.tsx new file mode 100644 index 00000000..86dddb3c --- /dev/null +++ b/frontend/src/lib/useHoverFocusWithin.dom.test.tsx @@ -0,0 +1,53 @@ +// @vitest-environment jsdom +// +// The focus half of the side panels' peek-open model. What is pinned: a dialog opened from inside +// the panel (rendered at the app root, so outside it in both the DOM and the React tree) does not +// collapse the panel — collapsing unmounted the row the dialog hands the focus back to, stranding a +// keyboard user on after Cancel (measured in Chrome, E11 S2). +import { afterEach, describe, expect, it } from "vitest"; +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { useHoverFocusWithin } from "./useHoverFocusWithin"; + +afterEach(cleanup); + +function Panel() { + const peek = useHoverFocusWithin(); + return ( + <> +
+ + {peek.active && expanded} +
+
+ +
+ + + ); +} + +const moveFocus = (from: HTMLElement, to: HTMLElement) => + act(() => { + fireEvent.blur(from, { relatedTarget: to }); + to.focus(); + }); + +describe("useHoverFocusWithin · focus", () => { + it("stays open while the focus is in a dialog opened over the page", () => { + render(); + const row = screen.getByText("row"); + act(() => row.focus()); + expect(screen.queryByText("expanded")).not.toBeNull(); + + moveFocus(row, screen.getByText("cancel")); + expect(screen.queryByText("expanded")).not.toBeNull(); + }); + + it("still closes when the focus leaves for the page itself", () => { + render(); + const row = screen.getByText("row"); + act(() => row.focus()); + moveFocus(row, screen.getByText("elsewhere")); + expect(screen.queryByText("expanded")).toBeNull(); + }); +}); diff --git a/frontend/src/lib/useHoverFocusWithin.ts b/frontend/src/lib/useHoverFocusWithin.ts index 3f27423d..6ecd49f0 100644 --- a/frontend/src/lib/useHoverFocusWithin.ts +++ b/frontend/src/lib/useHoverFocusWithin.ts @@ -58,7 +58,16 @@ export function useHoverFocusWithin() { const focusHandlers = { onFocus: () => setFocused(true), onBlur: (e: FocusEvent) => { - if (!e.currentTarget.contains(e.relatedTarget as Node)) setFocused(false); + const to = e.relatedTarget as Element | null; + if (e.currentTarget.contains(to)) return; + // A dialog opened FROM inside the panel (the rail row's "Delete?" confirm) renders at the app + // root, so neither the DOM nor the React tree sees it as inside — and collapsing the panel under + // it unmounted the very row the dialog hands the focus back to, leaving a keyboard user on + // after Cancel (measured in Chrome, E11 S2). Hold while the focus is in a dialog: when it + // closes, the focus either comes back in here (onFocus) or lands nowhere, which the + // document-level net below clears. + if (to?.closest('[aria-modal="true"]')) return; + setFocused(false); }, };