From 545996f80797f7612c9c5df8f17ad151c66e695b Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 11 Sep 2026 23:43:23 +0200 Subject: [PATCH] fix(ui): a toast host ranks by when its overlay opened, and an overlay keeps its Escape, its dialog and its navigation straight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deep-review round 4, the overlays. - Toast hosts were ranked by mount time, and PlayerModal swaps its host on every fullscreen change: leaving true fullscreen with the download dialog open put the player's host back on top, so toasts were drawn under that dialog's scrim and a click on one closed the dialog. Each overlay takes a rank once, when it opens (`useOverlayOrder`), and hands it to every host it renders. - A dialog open while fullscreen is left keeps a stable portal container (moved, not re-created): React tore the portal down when `fsRoot` changed, resetting a half-filled form and dropping focus to . Focus is put back after the move. - The onboarding wizard is a layer, not a raw document listener: one Escape used to dismiss the error dialog over it AND end onboarding, skipping the write-scope step. - `navigateTo` (a toast's "View", clicked inside the player's own host) goes over the overlays it unmounts, so the player's Back-to-close cleanup no longer pops the page it just opened — the same fix Shift+W already had. The access-requests toast action too. - The live region arms itself a task after it takes over inherited toasts: React flushes effects in the same task as that render, so the "silent inheritance" arming was a no-op. - The overlay-host guard matches class tokens in any order (PlexPlayer's root was never checked), globs `ui/` too (ui/Modal was not checked either), and requires the rank. --- frontend/src/App.tsx | 3 +- .../OnboardingWizard.escape.dom.test.tsx | 63 +++++++++++++++ frontend/src/components/OnboardingWizard.tsx | 18 ++--- frontend/src/components/PlayerModal.tsx | 15 ++-- frontend/src/components/PlexPlayer.tsx | 6 +- .../ui/Modal.fullscreen.dom.test.tsx | 52 +++++++++++++ frontend/src/components/ui/Modal.tsx | 26 ++++++- .../src/components/ui/Toaster.dom.test.tsx | 44 ++++++++++- .../src/components/ui/Toaster.hosts.test.ts | 76 +++++++++++++++---- frontend/src/components/ui/Toaster.tsx | 59 ++++++++++---- frontend/src/lib/nav.dom.test.ts | 40 ++++++++++ frontend/src/lib/nav.ts | 11 ++- 12 files changed, 358 insertions(+), 55 deletions(-) create mode 100644 frontend/src/components/OnboardingWizard.escape.dom.test.tsx create mode 100644 frontend/src/components/ui/Modal.fullscreen.dom.test.tsx create mode 100644 frontend/src/lib/nav.dom.test.ts diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 8d9de728..707e07c6 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -279,7 +279,8 @@ export default function App() { label: t("common.accessRequestsReview"), onClick: () => { focusAccessRequestsTab(); - setPage("users"); + // Over whatever overlay the toast was clicked in — as navigateTo does (see lib/nav). + navigateOverOverlays(() => setPage("users")); }, }, }); diff --git a/frontend/src/components/OnboardingWizard.escape.dom.test.tsx b/frontend/src/components/OnboardingWizard.escape.dom.test.tsx new file mode 100644 index 00000000..87bb4b78 --- /dev/null +++ b/frontend/src/components/OnboardingWizard.escape.dom.test.tsx @@ -0,0 +1,63 @@ +// @vitest-environment jsdom +// +// Escape closes the topmost layer only — the wizard included (deep-review round 4). Its own Escape +// was a raw `document` listener with no layer check, and document listeners run before the window +// listener ui/Modal uses: a sync that failed inside the wizard opens the global error dialog, and one +// Esc dismissed the error AND ended onboarding, skipping the write-scope step. +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 Modal from "./ui/Modal"; +import { api, type Me } from "../lib/api"; + +const me = { id: 1, email: "a@b", can_read: false, can_write: false } as unknown as Me; + +beforeEach(() => { + vi.spyOn(api, "myStatus").mockResolvedValue({ channels_total: 0 } as never); +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe("the onboarding wizard's Escape", () => { + const mount = (onClose: () => void, dialog: boolean) => + render( + + + {dialog && ( + {}}> +

the sync failed

+ + )} + , + ); + + it("closes the wizard when nothing is open above it", () => { + const onClose = vi.fn(); + mount(onClose, false); + fireEvent.keyDown(document, { key: "Escape" }); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("leaves the wizard alone while a dialog is open over it", () => { + const onClose = vi.fn(); + mount(onClose, true); + fireEvent.keyDown(screen.getByText("the sync failed"), { key: "Escape" }); + expect(onClose).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/components/OnboardingWizard.tsx b/frontend/src/components/OnboardingWizard.tsx index b57c57a5..e8a7de83 100644 --- a/frontend/src/components/OnboardingWizard.tsx +++ b/frontend/src/components/OnboardingWizard.tsx @@ -7,7 +7,8 @@ import { api, type Me } from "../lib/api"; import { beginGrant, endOnboarding } from "../lib/onboarding"; import { Button } from "./ui/Button"; import Overlay from "./ui/Overlay"; -import Toaster from "./ui/Toaster"; +import Toaster, { useOverlayOrder } from "./ui/Toaster"; +import { useLayer } from "../lib/layerStack"; // 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 @@ -103,15 +104,12 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: () endOnboarding(!me.can_read); onClose(); }; + const overlayOrder = useOverlayOrder(); // this overlay's toast-host rank (see ui/Toaster) - useEffect(() => { - function onKey(e: KeyboardEvent) { - if (e.key === "Escape") close(); - } - document.addEventListener("keydown", onKey); - return () => document.removeEventListener("keydown", onKey); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [me.can_read]); + // 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); // Are we currently importing (or about to)? Show a progress screen instead of the // "you're connected" step until the first import settles. @@ -265,7 +263,7 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: () {/* Toasts raised while the wizard is up (the connection notice among them) show above its scrim, not under it — see ui/Toaster. */} - + ); diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 47b73975..6b71e37f 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -48,7 +48,7 @@ import { fullscreenElement } from "../lib/fullscreen"; import { dispatch, isActivationTarget, useKeyScope } from "../lib/keymap"; import { closeThenNavigate, useBackToClose } from "../lib/history"; import ChannelNameLink from "./ChannelNameLink"; -import Toaster from "./ui/Toaster"; +import Toaster, { useOverlayOrder } from "./ui/Toaster"; import { afterRemoval } from "../lib/playerQueue"; import { useNativeControlsYield, @@ -379,6 +379,10 @@ export default function PlayerModal({ staleTime: 5 * 60_000, }); + // Its toast hosts' rank (see ui/Toaster): the player's two hosts — the stage one in true fullscreen, + // the outer one otherwise — swap on every fullscreen change, and must not outrank a dialog opened + // over the player by doing so. + const overlayOrder = useOverlayOrder(); // This player is a dismissable layer: register it so a Modal or popover opened ABOVE it (e.g. the // download dialog, or — after S4 — an anchored menu) takes Escape first and this player defers. const layerIdRef = useRef(null); @@ -583,10 +587,9 @@ export default function PlayerModal({ bar — that stays reachable by hovering the video's bottom edge, just above ours. Always visible (no auto-hide) so there's no guessing whether it will appear and no fight with the seek bar over the same pixels. stopPropagation so a click here doesn't toggle playback; - the exit button also re-arms our shortcuts. Add-to-playlist works in true fullscreen (its - popover portals INTO the fullscreen element); download stays hidden there because it opens - a Modal, which still portals to — outside the fullscreen element. */} - {isFullscreen && } + the exit button also re-arms our shortcuts. Both of its dialogs work in true fullscreen: + add-to-playlist's popover and download's Modal portal INTO the fullscreen element. */} + {isFullscreen && } {(maximized || isFullscreen) && (
e.stopPropagation()} @@ -965,7 +968,7 @@ export default function PlayerModal({ {/* Toasts raised while the player is open (a failed Remove, a lost connection) — the app's own Toaster is underneath this scrim. See ui/Toaster. In true fullscreen it moves into the stage, the element holding the lock: out here it was painted under it, i.e. not at all. */} - {!isFullscreen && } + {!isFullscreen && }
, document.body, ); diff --git a/frontend/src/components/PlexPlayer.tsx b/frontend/src/components/PlexPlayer.tsx index e32b9b0c..a35b9023 100644 --- a/frontend/src/components/PlexPlayer.tsx +++ b/frontend/src/components/PlexPlayer.tsx @@ -43,7 +43,7 @@ import { PlexPlayerSettings } from "./PlexPlayerSettings"; import { PlexStatsOverlay } from "./PlexStatsOverlay"; import { createFragMeter } from "../lib/plexStats"; import { UpNextCard } from "./UpNextCard"; -import Toaster from "./ui/Toaster"; +import Toaster, { useOverlayOrder } from "./ui/Toaster"; import { playhead, useAutoSkip, @@ -793,6 +793,8 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { useKeyScope("plexPlayer"); useHelpConditions({ episode: detail?.kind === "episode" }); + // Its toast host's rank among open overlays (see ui/Toaster). + const overlayOrder = useOverlayOrder(); // This player is a dismissable layer: register it so a Modal or popover opened ABOVE it (its own // settings/tracks menus included) takes Escape first and this player defers (R8 S3). const layerIdRef = useRef(null); @@ -1515,7 +1517,7 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { )} {/* The app's Toaster is underneath this full-screen player (and outside it in native fullscreen), so toasts raised while watching show here instead. See ui/Toaster. */} - + , document.body, ); diff --git a/frontend/src/components/ui/Modal.fullscreen.dom.test.tsx b/frontend/src/components/ui/Modal.fullscreen.dom.test.tsx new file mode 100644 index 00000000..42e1fb87 --- /dev/null +++ b/frontend/src/components/ui/Modal.fullscreen.dom.test.tsx @@ -0,0 +1,52 @@ +// @vitest-environment jsdom +// +// A dialog opened from a player in true fullscreen portals INTO the fullscreen element (a node under +// is painted beneath it, i.e. invisible). Leaving fullscreen changes that container — and a +// React portal whose container changes is torn down and recreated, so everything the dialog held went +// with it: a half-filled download form reset to its defaults and focus fell to (a verifier's +// side finding in deep-review round 4). +import { describe, it, expect, afterEach } from "vitest"; +import { render, cleanup, screen, fireEvent, act } from "@testing-library/react"; +import { vi } from "vitest"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useTranslation: () => ({ t: (key: string) => key }) }; +}); + +import Modal from "./Modal"; + +const setFullscreen = (el: Element | null) => + act(() => { + Object.defineProperty(document, "fullscreenElement", { value: el, configurable: true }); + document.dispatchEvent(new Event("fullscreenchange")); + }); + +afterEach(() => { + cleanup(); + setFullscreen(null); +}); + +describe("a dialog open while fullscreen is left", () => { + it("keeps what the viewer had typed in it, and the focus inside it", async () => { + const stage = document.createElement("div"); + document.body.appendChild(stage); + await setFullscreen(stage); + render( + {}}> + + , + ); + const field = screen.getByLabelText("name") as HTMLInputElement; + fireEvent.change(field, { target: { value: "my cut" } }); + field.focus(); + expect(stage.contains(field)).toBe(true); + + await setFullscreen(null); // the browser's own Esc leaves fullscreen; the dialog stays open + + const after = screen.getByLabelText("name") as HTMLInputElement; + expect(after.value).toBe("my cut"); + expect(document.activeElement).toBe(after); + stage.remove(); + }); +}); diff --git a/frontend/src/components/ui/Modal.tsx b/frontend/src/components/ui/Modal.tsx index fa38737c..4cdc113a 100644 --- a/frontend/src/components/ui/Modal.tsx +++ b/frontend/src/components/ui/Modal.tsx @@ -1,4 +1,4 @@ -import { useEffect, useId, useRef, useState, type ReactNode } from "react"; +import { useEffect, useId, useLayoutEffect, useRef, useState, type ReactNode } from "react"; import { createPortal } from "react-dom"; import { useTranslation } from "react-i18next"; import { X } from "lucide-react"; @@ -6,7 +6,7 @@ import { useBackToClose } from "../../lib/history"; import { pushLayer, removeLayer, isTopmost } from "../../lib/layerStack"; import { useFullscreenElement } from "../../lib/fullscreen"; import { Button } from "./Button"; -import Toaster from "./Toaster"; +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 @@ -45,6 +45,23 @@ export default function Modal({ // 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]); const panelRef = useRef(null); const titleId = useId(); // Capture the trigger to restore focus to, during THIS render — i.e. before commit runs the @@ -52,6 +69,7 @@ export default function Modal({ // 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); + const overlayOrder = useOverlayOrder(); // 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 @@ -170,8 +188,8 @@ export default function Modal({ {/* Toasts raised while a dialog is open show inside it: the app's own host is under this scrim, where a failed share or rename was unreadable and a click on it closed the dialog (see ui/Toaster). */} - + , - fsRoot ?? document.body, + host, ); } diff --git a/frontend/src/components/ui/Toaster.dom.test.tsx b/frontend/src/components/ui/Toaster.dom.test.tsx index ca13a1e0..44a3a53e 100644 --- a/frontend/src/components/ui/Toaster.dom.test.tsx +++ b/frontend/src/components/ui/Toaster.dom.test.tsx @@ -41,7 +41,8 @@ describe("Toaster — one host shows the toasts", () => { expect(screen.getByTestId("app").textContent).not.toContain("Couldn't remove the video."); // Both live regions stay mounted — a polite region has to pre-exist to be announced. expect(hosts()).toHaveLength(2); - expect(hosts().map((h) => h.getAttribute("aria-live"))).toEqual(["off", "polite"]); + // The taking-over host arms itself a task later (see the "inherits" test below). + expect(hosts().map((h) => h.getAttribute("aria-live"))).toEqual(["off", "off"]); overlay.unmount(); expect(screen.getByTestId("app").textContent).toContain("Couldn't remove the video."); @@ -74,6 +75,47 @@ describe("Toaster — handing the toasts to another host", () => { expect(screen.getByText("fresh").closest(".toast-card")!.getAttribute("role")).toBe("alert"); }); + it("keeps its live region off while the inherited toasts go in, and arms it a task later", async () => { + // The other half of "silently": a region that is already polite when the inherited nodes are + // inserted announces them anyway. React flushes effects in the SAME task as that render, so the + // arming has to wait for the next one — a screen reader reads the region's state after the task. + render(); + act(() => void notify({ level: "info", message: "saved" })); + const overlay = render(); + const region = overlay.container.firstElementChild!; + expect(region.textContent).toContain("saved"); // it took the toast over… + expect(region.getAttribute("aria-live")).toBe("off"); // …while still off + await act(async () => void (await new Promise((r) => setTimeout(r, 0)))); + expect(region.getAttribute("aria-live")).toBe("polite"); // and polite for what comes next + }); + + 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. + const player = render( +
+ +
, + ); + render( +
+ +
, + ); + act(() => void notify({ level: "error", message: "boom" })); + expect(screen.getByTestId("dialog").textContent).toContain("boom"); + // The player swaps its host (leaving true fullscreen) while the dialog stays open. + player.rerender( +
+ + +
, + ); + await act(async () => void (await new Promise((r) => setTimeout(r, 0)))); + expect(screen.getByTestId("dialog").textContent).toContain("boom"); + expect(screen.getByTestId("player").textContent).not.toContain("boom"); + }); + it("keeps the countdown where it was, instead of starting it again from full", () => { // The bar is a CSS animation on the node; the dismiss timer was armed once, at notify time. A // new node restarted the bar, and the toast vanished with it still two-thirds full. diff --git a/frontend/src/components/ui/Toaster.hosts.test.ts b/frontend/src/components/ui/Toaster.hosts.test.ts index 874d03b0..e77650f5 100644 --- a/frontend/src/components/ui/Toaster.hosts.test.ts +++ b/frontend/src/components/ui/Toaster.hosts.test.ts @@ -3,31 +3,77 @@ // The app's Toaster lives in the content column, a stacking context: anything that covers the // viewport at z-overlay paints over it, so a toast raised while that overlay is open is unreadable and // a click on it lands on the overlay's backdrop. The fix is a convention — the overlay mounts -// `` inside itself — and a convention stated in a comment is one the next overlay -// forgets (deep-review round 3 found every ui/Modal and the onboarding wizard without one). So it is -// checked here: a component that renders a viewport-covering z-overlay layer mounts an overlay host. +// `` inside itself — and a convention stated in a comment +// is one the next overlay forgets (deep-review round 3 found every ui/Modal and the onboarding wizard +// without one). So it is checked here: a component that renders a viewport-covering layer at +// z-overlay or above mounts an overlay host, ranked by when the overlay opened. +// +// The class TOKENS are matched in any order, and the layer need not be `inset-0`: the first version +// matched the exact string "fixed inset-0 z-overlay", so PlexPlayer's own root (`fixed left-0 top-0 +// z-overlay`) was never checked at all, and a new viewer copying it would have slipped past +// (deep-review round 4). import { describe, it, expect } from "vitest"; -const sources = import.meta.glob("../**/*.tsx", { query: "?raw", import: "default", eager: true }); +// BOTH patterns: `../**/*.tsx` from this directory does not match the siblings in `ui/` itself, so +// ui/Modal.tsx — the overlay behind every dialog in the app — was never checked by this guard at all +// (deep-review round 4). +const sources = import.meta.glob(["../**/*.tsx", "./*.tsx"], { + query: "?raw", + import: "default", + eager: true, +}); -// Covers the viewport for a moment with nothing a toast could be about (a lazy chunk's black -// placeholder while the player loads). -const EXEMPT = new Set(["../PlexBrowse.tsx"]); +// Overlays with nothing a toast could be about: a lazy chunk's black placeholder while the player +// loads, and the signed-out landing page's screenshot zoom (the app's Toaster is not even mounted +// there — App renders Welcome instead of the shell). +const EXEMPT = new Set(["../PlexBrowse.tsx", "../Welcome.tsx"]); + +/** Every class string in the file (quoted or templated), including multi-line ones. */ +const classStrings = (src: string): string[] => + [...src.matchAll(/className=\{?[`"']([^`"']*)[`"']/g)].map((m) => m[1]!); + +const coversTheViewport = (src: string) => + classStrings(src).some( + (cls) => + /(^|\s)fixed(\s|$)/.test(cls) && + /(^|\s)z-(overlay|popover|tooltip)(\s|$)/.test(cls) && + (/(^|\s)inset-0(\s|$)/.test(cls) || + (/(^|\s)left-0(\s|$)/.test(cls) && /(^|\s)top-0(\s|$)/.test(cls))), + ); + +const overlays = () => + Object.entries(sources) + .filter(([path]) => !path.endsWith(".test.tsx") && !EXEMPT.has(path)) + .filter(([, src]) => coversTheViewport(src as string)); describe("full-screen overlays host their own toasts", () => { - it("every one that covers the viewport at z-overlay mounts ", () => { - const missing = Object.entries(sources) - .filter(([path]) => !path.endsWith(".test.tsx") && !EXEMPT.has(path)) - .filter(([, src]) => /fixed inset-0 z-overlay/.test(src as string)) - .filter(([, src]) => !(src as string).includes("")) + it("every one that covers the viewport mounts an overlay host", () => { + const missing = overlays() + .filter(([, src]) => !(src as string).includes(" path); expect(missing).toEqual([]); }); + it("and ranks it by when the overlay opened, not by when the host mounted", () => { + // A host that takes its own rank jumps to the top whenever it remounts — which PlayerModal's does + // on every fullscreen change, putting it back over a dialog opened above the player. + const unranked = overlays() + .filter(([, src]) => !(src as string).includes("useOverlayOrder()")) + .map(([path]) => path); + expect(unranked).toEqual([]); + }); + it("finds the overlays it is meant to check", () => { - const covered = Object.entries(sources).filter(([, src]) => - /fixed inset-0 z-overlay/.test(src as string), + // Modal, PlayerModal, PlexPlayer and the wizard at least — PlexPlayer is the one the exact-string + // check used to miss. + const paths = overlays().map(([p]) => p); + expect(paths).toEqual( + expect.arrayContaining([ + "./Modal.tsx", + "../PlayerModal.tsx", + "../PlexPlayer.tsx", + "../OnboardingWizard.tsx", + ]), ); - expect(covered.length).toBeGreaterThanOrEqual(3); // Modal, PlayerModal, the wizard at least }); }); diff --git a/frontend/src/components/ui/Toaster.tsx b/frontend/src/components/ui/Toaster.tsx index 3e471194..56d52e63 100644 --- a/frontend/src/components/ui/Toaster.tsx +++ b/frontend/src/components/ui/Toaster.tsx @@ -16,24 +16,48 @@ export const LEVEL_STYLE: Record at the // same z-overlay painted over every toast: a failure raised while watching was drawn under an 80% // black, blurred scrim, and a click on it closed the player. So every full-screen overlay mounts its -// OWN `` inside itself — ui/Modal does it for every dialog, the players for -// themselves — and exactly one host shows the toasts: the most recently mounted overlay, else the -// app's. Same component, same look — and inside the overlay's aria-modal dialog, where a screen +// OWN `` inside itself — ui/Modal does it for every dialog, the players +// for themselves — and exactly one host shows the toasts: the one of the overlay OPENED last, else +// the app's. Same component, same look — and inside the overlay's aria-modal dialog, where a screen // reader can still reach it. -const overlayHosts = createStore([]); +// +// "Opened last", not "host mounted last": a host can remount while its overlay stays open — +// PlayerModal's moves into the fullscreen stage and back — and ranked by mount time it came back on +// top of a dialog opened over the player (the download dialog after leaving true fullscreen), so the +// toasts were drawn under that dialog's scrim again (deep-review round 4). So the OVERLAY takes its +// rank once, when it opens (`useOverlayOrder`), and hands it to every host it renders. +let overlaySeq = 0; +/** This overlay's rank among open overlays — taken once, when it mounts. Call it in the overlay + * itself, never in a component that can remount while the overlay stays open. */ +export function useOverlayOrder(): number { + const [order] = useState(() => ++overlaySeq); + return order; +} +type Host = { id: string; order: number }; +const overlayHosts = createStore([]); const topHost = () => { - const hosts = overlayHosts.get(); - return hosts[hosts.length - 1] ?? null; + let top: Host | null = null; + for (const h of overlayHosts.get()) if (!top || h.order >= top.order) top = h; + return top?.id ?? null; }; -export default function Toaster({ overlay = false }: { overlay?: boolean }) { +export default function Toaster({ + overlay = false, + order, +}: { + overlay?: boolean; + /** The owning overlay's `useOverlayOrder()`. Without one, the host ranks by its own mount. */ + order?: number; +}) { const { t } = useTranslation(); const id = useId(); + const ownOrder = useOverlayOrder(); + const rank = order ?? ownOrder; useEffect(() => { if (!overlay) return; - overlayHosts.set((hosts) => [...hosts, id]); - return () => overlayHosts.set((hosts) => hosts.filter((h) => h !== id)); - }, [overlay, id]); + overlayHosts.set((hosts) => [...hosts, { id, order: rank }]); + return () => overlayHosts.set((hosts) => hosts.filter((h) => h.id !== id)); + }, [overlay, id, rank]); const top = useSyncExternalStore(overlayHosts.subscribe, topHost, topHost); const active = useSyncExternalStore(subscribe, getActiveToasts, getActiveToasts); const isTop = top === (overlay ? id : null); @@ -41,14 +65,23 @@ export default function Toaster({ overlay = false }: { overlay?: boolean }) { // A host that takes over (an overlay opening, or the app's own once it closes) receives the toasts // that were already up as NEW nodes — and a screen reader announced every one of them again, an // error assertively. So the ones it inherits are shown silently: no `role="alert"`, and the region - // stays `aria-live="off"` until they are in, turning polite (for the toasts that come after) once - // they have been rendered. + // stays `aria-live="off"` until they are in, turning polite (for the toasts that come after) in a + // 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], ); const [armed, setArmed] = useState(false); - useEffect(() => setArmed(isTop), [isTop]); + useEffect(() => { + if (!isTop) { + setArmed(false); + return; + } + const timer = window.setTimeout(() => setArmed(true), 0); + return () => window.clearTimeout(timer); + }, [isTop]); return ( // Stable, always-mounted live region — a polite region must PRE-EXIST in the DOM for a screen diff --git a/frontend/src/lib/nav.dom.test.ts b/frontend/src/lib/nav.dom.test.ts new file mode 100644 index 00000000..eead88e2 --- /dev/null +++ b/frontend/src/lib/nav.dom.test.ts @@ -0,0 +1,40 @@ +// @vitest-environment jsdom +// +// `navigateTo` is what a toast's action calls ("Download added — View"), and that click happens +// INSIDE an overlay's toast host. The page change unmounts the page-owned overlay (the player), whose +// Back-to-close cleanup then reclaimed the history entry the navigation had just pushed — so the user +// landed back where they started with the player closed instead of on the Downloads page +// (deep-review round 4). Same trap, and the same fix, as the global Shift+W shortcut. +// +// Its own file: `lib/history`'s depth bookkeeping is module state. +import { describe, it, expect, vi } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { useBackToClose } from "./history"; +import { navigateTo, setNavigator } from "./nav"; + +const settle = () => + act(async () => { + await Promise.resolve(); + }); + +describe("navigateTo from inside an overlay", () => { + it("buries the overlay's entry, so closing it does not pop the page just opened", async () => { + const overlay = renderHook(() => useBackToClose(() => {})); + await settle(); + const back = vi.spyOn(window.history, "back"); + try { + setNavigator(() => { + window.history.pushState({ sfPage: "downloads" }, ""); + return true; // as App's setPage reports: it really pushed + }); + act(() => navigateTo("downloads")); + overlay.unmount(); // the page change unmounts the player + await settle(); + expect(back).not.toHaveBeenCalled(); + expect(window.history.state).toMatchObject({ sfPage: "downloads" }); + } finally { + back.mockRestore(); + setNavigator(() => {}); + } + }); +}); diff --git a/frontend/src/lib/nav.ts b/frontend/src/lib/nav.ts index 61f07057..c2d2b622 100644 --- a/frontend/src/lib/nav.ts +++ b/frontend/src/lib/nav.ts @@ -1,16 +1,21 @@ +import { navigateOverOverlays } from "./history"; import type { Page } from "./urlState"; // Tiny decoupled navigator so deep components (a feed card's download toast, the Download // Center) can switch pages without threading setPage through the whole tree. App registers // its setPage on mount; navigateTo is a no-op until then. -let _navigate: ((p: Page) => void) | null = null; +let _navigate: ((p: Page) => boolean | void) | null = null; -export function setNavigator(fn: (p: Page) => void): void { +export function setNavigator(fn: (p: Page) => boolean | void): void { _navigate = fn; } +/** Over whatever overlay is open: the "View" on a "Download added" toast is clicked INSIDE the player + * (its toast host), and the page change unmounts the page-owned player, whose Back-to-close cleanup + * then popped the entry the navigation had just pushed — the user landed back on the feed with the + * player gone (deep-review round 4). Same trap, same fix as Shift+W (see `navigateOverOverlays`). */ export function navigateTo(p: Page): void { - _navigate?.(p); + navigateOverOverlays(() => _navigate?.(p)); } /** Should this click be handled IN-APP, or left to the browser?