From 33fe895539727559a3931877d880ed16617bc808 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 11 Sep 2026 18:04:54 +0200 Subject: [PATCH] fix(ui): every full-screen overlay hosts its own toasts, above the players' controls, without re-announcing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deep-review round 3 of bug hunt 2026-09 (the round-2 overlay toast host, finished). - Inside an overlay the toast stack sits above the bottom band (bottom-24, the UpNextCard rung) and lets clicks between cards through: at the app's own spot it covered the Plex transport row and PlayerModal's docked bar and took their clicks while a toast was up (measured: play/pause, ±10, stop, prev/next, mute all hit the toast). - ui/Modal and the onboarding wizard mount an overlay host too, so a toast raised under any dialog is readable and a click on it no longer closes the dialog; the Modal's Tab trap spans the whole dialog, so a toast's action and dismiss are keyboard-reachable. A test now enforces the convention: a component that covers the viewport at z-overlay must mount . - PlayerModal's host moves into the stage in true fullscreen (outside the element holding the lock it was painted under it). - A host that takes over shows the toasts it inherits silently (no second role=alert, its region aria-live=off until they are in), and each countdown bar starts as far in as its toast already is instead of restarting while the real dismiss timer does not. - The overlay-host registry uses lib/store instead of a hand-rolled listener triad. --- frontend/src/components/OnboardingWizard.tsx | 4 + .../src/components/PlayerModal.dom.test.tsx | 19 ++++ frontend/src/components/PlayerModal.tsx | 6 +- .../src/components/UploadDialog.dom.test.tsx | 6 +- frontend/src/components/ui/Modal.dom.test.tsx | 46 +++++++++ frontend/src/components/ui/Modal.tsx | 14 ++- .../src/components/ui/Toaster.dom.test.tsx | 43 +++++++- .../src/components/ui/Toaster.hosts.test.ts | 33 +++++++ frontend/src/components/ui/Toaster.tsx | 98 ++++++++++++------- 9 files changed, 228 insertions(+), 41 deletions(-) create mode 100644 frontend/src/components/ui/Toaster.hosts.test.ts diff --git a/frontend/src/components/OnboardingWizard.tsx b/frontend/src/components/OnboardingWizard.tsx index abbf3f6d..b57c57a5 100644 --- a/frontend/src/components/OnboardingWizard.tsx +++ b/frontend/src/components/OnboardingWizard.tsx @@ -7,6 +7,7 @@ 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"; // 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 @@ -262,6 +263,9 @@ 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.dom.test.tsx b/frontend/src/components/PlayerModal.dom.test.tsx index 08c07545..e24c3c67 100644 --- a/frontend/src/components/PlayerModal.dom.test.tsx +++ b/frontend/src/components/PlayerModal.dom.test.tsx @@ -270,4 +270,23 @@ describe("PlayerModal — toasts raised while it is open", () => { act(() => void notify({ level: "error", message: "Couldn't remove the video." })); expect(screen.getByTestId("player-modal").textContent).toContain("Couldn't remove the video."); }); + + it("shows them inside the stage in true fullscreen — the element holding the lock", async () => { + // Anything outside the fullscreen element is painted under it. The player's host sat beside the + // card, so in fullscreen a lost connection was reported to nobody. + vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {})); + const A = vid("A", 0); + renderModal( + {}} onState={() => {}} />, + ); + const stage = document.querySelector(".player-stage")!; + Object.defineProperty(document, "fullscreenElement", { configurable: true, get: () => stage }); + try { + act(() => void document.dispatchEvent(new Event("fullscreenchange"))); + act(() => void notify({ level: "error", message: "Connection lost" })); + await waitFor(() => expect(stage.textContent).toContain("Connection lost")); + } finally { + delete (document as { fullscreenElement?: unknown }).fullscreenElement; + } + }); }); diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index d0ac068c..47b73975 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -586,6 +586,7 @@ export default function PlayerModal({ 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 && } {(maximized || isFullscreen) && (
e.stopPropagation()} @@ -962,8 +963,9 @@ 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. */} - + 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 && } , document.body, ); diff --git a/frontend/src/components/UploadDialog.dom.test.tsx b/frontend/src/components/UploadDialog.dom.test.tsx index 4259b031..ccbecf77 100644 --- a/frontend/src/components/UploadDialog.dom.test.tsx +++ b/frontend/src/components/UploadDialog.dom.test.tsx @@ -20,7 +20,11 @@ vi.mock("react-i18next", async (importOriginal) => { }); vi.mock("../lib/api", () => ({ api: { uploadDownload: h.uploadDownload } })); vi.mock("../lib/downloads", () => ({ invalidateDownloads: vi.fn() })); -vi.mock("../lib/notifications", () => ({ notify: h.notify })); +// The rest of the module stays real: the dialog is a ui/Modal, which hosts its own toasts. +vi.mock("../lib/notifications", async (importOriginal) => ({ + ...(await importOriginal()), + notify: h.notify, +})); vi.mock("../lib/nav", () => ({ navigateTo: vi.fn() })); import UploadDialog from "./UploadDialog"; diff --git a/frontend/src/components/ui/Modal.dom.test.tsx b/frontend/src/components/ui/Modal.dom.test.tsx index ef886c69..7e5571ce 100644 --- a/frontend/src/components/ui/Modal.dom.test.tsx +++ b/frontend/src/components/ui/Modal.dom.test.tsx @@ -20,6 +20,7 @@ vi.mock("react-i18next", async (importOriginal) => { vi.mock("../../lib/history", () => ({ useBackToClose: () => {} })); import Modal from "./Modal"; +import { clearAll, notify } from "../../lib/notifications"; // Fire a keydown on window (where Modal installs its handler), inside act. const press = (key: string, shiftKey = false) => @@ -91,6 +92,32 @@ describe("Modal focus trap (offsetParent stubbed 'visible')", () => { expect(document.activeElement).toBe(screen.getByText("beta")); }); + it("Tab reaches a toast shown inside the dialog, and wraps from it", () => { + // The dialog's toast host sits beside the panel; trapped to the panel, a toast's Undo or dismiss + // could not be reached from the keyboard at all. + render( + {}}> + + , + ); + act(() => void notify({ level: "info", message: "Saved" })); + try { + const dismissToast = screen.getByTitle("notifications.dismiss"); + // The panel's last control is no longer the trap's edge: Tab carries on (natively) to the toast. + screen.getByText("beta").focus(); + press("Tab"); + expect(document.activeElement).toBe(screen.getByText("beta")); // not wrapped back to the top + // The toast is the edge now: past it, Tab wraps to the first control, and Shift+Tab reaches it. + dismissToast.focus(); + press("Tab"); + expect(document.activeElement).toBe(closeBtn()); + press("Tab", true); + expect(document.activeElement).toBe(dismissToast); + } finally { + act(() => clearAll()); + } + }); + it("Tab pulls focus back inside when it has escaped to the page behind", () => { const outside = document.createElement("button"); document.body.appendChild(outside); @@ -189,3 +216,22 @@ describe("Modal focus trap (raw jsdom — every offsetParent null)", () => { expect(document.activeElement).toBe(panelEl()); }); }); + +describe("Modal — toasts raised while it is open", () => { + afterEach(() => { + cleanup(); + act(() => clearAll()); + }); + + it("shows them inside the dialog, above its scrim", () => { + // The app's Toaster is in a stacking context under this body-level portal: a failed share or + // rename was drawn under the scrim, and clicking it landed on the backdrop and closed the dialog. + render( + {}}> + + , + ); + act(() => void notify({ level: "error", message: "Couldn't share it." })); + expect(screen.getByRole("dialog").textContent).toContain("Couldn't share it."); + }); +}); diff --git a/frontend/src/components/ui/Modal.tsx b/frontend/src/components/ui/Modal.tsx index a38857bf..fa38737c 100644 --- a/frontend/src/components/ui/Modal.tsx +++ b/frontend/src/components/ui/Modal.tsx @@ -6,6 +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"; // 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 @@ -56,6 +57,7 @@ export default function Modal({ // drag that begins inside the dialog (e.g. selecting text in an input) and is released out on // the backdrop wrongly dismisses — the click event bubbles to the common ancestor (the backdrop). const pressedOnBackdrop = useRef(false); + const rootRef = useRef(null); // Keep a stable handler across renders so the stack id is assigned once per mount. const onCloseRef = useRef(onClose); @@ -74,7 +76,10 @@ export default function Modal({ if (e.key === "Tab") { const panel = panelRef.current; if (!panel) return; - const items = focusablesIn(panel); + // 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) { @@ -82,7 +87,7 @@ export default function Modal({ return; } const active = document.activeElement; - if (!panel.contains(active) || active === panel) { + 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. @@ -119,6 +124,7 @@ export default function Modal({ const top = placement === "top"; return createPortal(
{children}
+ {/* 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, ); diff --git a/frontend/src/components/ui/Toaster.dom.test.tsx b/frontend/src/components/ui/Toaster.dom.test.tsx index ab67ce1b..ca13a1e0 100644 --- a/frontend/src/components/ui/Toaster.dom.test.tsx +++ b/frontend/src/components/ui/Toaster.dom.test.tsx @@ -20,7 +20,7 @@ afterEach(() => { act(() => clearAll()); }); -const hosts = () => Array.from(document.querySelectorAll("[aria-live='polite']")); +const hosts = () => Array.from(document.querySelectorAll("[aria-live]")); describe("Toaster — one host shows the toasts", () => { it("moves the toasts into an open overlay, and back when it closes", () => { @@ -41,6 +41,7 @@ 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"]); overlay.unmount(); expect(screen.getByTestId("app").textContent).toContain("Couldn't remove the video."); @@ -59,3 +60,43 @@ describe("Toaster — one host shows the toasts", () => { expect(backdrop).not.toHaveBeenCalled(); }); }); + +describe("Toaster — handing the toasts to another host", () => { + it("does not announce the toasts it inherits again, only the ones that come after", () => { + // The new host gets them as NEW nodes: a screen reader read every one out again, an error + // assertively, each time a player (or any dialog) opened or closed. + render(); + act(() => void notify({ level: "error", message: "old news" })); + render(); + const old = screen.getByText("old news").closest(".toast-card")!; + expect(old.getAttribute("role")).toBeNull(); + act(() => void notify({ level: "error", message: "fresh" })); + expect(screen.getByText("fresh").closest(".toast-card")!.getAttribute("role")).toBe("alert"); + }); + + 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. + const t0 = Date.now(); + const now = vi.spyOn(Date, "now").mockReturnValue(t0); + render(); + act(() => void notify({ level: "info", message: "saved" })); + now.mockReturnValue(t0 + 4000); + render(); + const bar = screen.getByText("saved").closest(".toast-card")!.lastElementChild as HTMLElement; + expect(bar.style.animation).toContain("-4000ms"); + }); + + it("keeps the players' control band free inside an overlay", () => { + // At the app's own spot (bottom-4) the stack sat on the Plex transport row and PlayerModal's + // docked bar, above them at z-overlay, and took their clicks for as long as a toast was up. + render(); + act(() => void notify({ level: "error", message: "boom" })); + const host = hosts()[0]!; + expect(host.className).toContain("bottom-24"); + expect(host.className).toContain("pointer-events-none"); + expect(screen.getByText("boom").closest(".toast-card")!.className).toContain( + "pointer-events-auto", + ); + }); +}); diff --git a/frontend/src/components/ui/Toaster.hosts.test.ts b/frontend/src/components/ui/Toaster.hosts.test.ts new file mode 100644 index 00000000..874d03b0 --- /dev/null +++ b/frontend/src/components/ui/Toaster.hosts.test.ts @@ -0,0 +1,33 @@ +// Every full-screen overlay hosts its own toasts (see ui/Toaster). +// +// 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. +import { describe, it, expect } from "vitest"; + +const sources = import.meta.glob("../**/*.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"]); + +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("")) + .map(([path]) => path); + expect(missing).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), + ); + 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 2720feeb..3e471194 100644 --- a/frontend/src/components/ui/Toaster.tsx +++ b/frontend/src/components/ui/Toaster.tsx @@ -1,7 +1,8 @@ -import { useEffect, useId, useSyncExternalStore } from "react"; +import { useEffect, useId, useMemo, useState, useSyncExternalStore } from "react"; import { useTranslation } from "react-i18next"; import { AlertCircle, AlertTriangle, CheckCircle2, Info, X, XCircle } from "lucide-react"; import { dismiss, getActiveToasts, subscribe, type NotifLevel } from "../../lib/notifications"; +import { createStore } from "../../lib/store"; export const LEVEL_STYLE: Record = { info: { icon: Info, color: "text-info", bar: "bg-info" }, @@ -12,22 +13,17 @@ 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. Such an overlay -// mounts its OWN `` inside itself instead, 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 reader can still reach it. -let overlayHosts: string[] = []; -const hostListeners = new Set<() => void>(); -const subscribeHosts = (l: () => void) => { - hostListeners.add(l); - return () => void hostListeners.delete(l); -}; -const topHost = () => overlayHosts[overlayHosts.length - 1] ?? null; -const setHosts = (next: string[]) => { - overlayHosts = next; - hostListeners.forEach((l) => l()); +// context (its `@container` implies layout containment) — so anything portalled to 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 +// reader can still reach it. +const overlayHosts = createStore([]); +const topHost = () => { + const hosts = overlayHosts.get(); + return hosts[hosts.length - 1] ?? null; }; export default function Toaster({ overlay = false }: { overlay?: boolean }) { @@ -35,23 +31,43 @@ export default function Toaster({ overlay = false }: { overlay?: boolean }) { const id = useId(); useEffect(() => { if (!overlay) return; - setHosts([...overlayHosts, id]); - return () => setHosts(overlayHosts.filter((h) => h !== id)); + overlayHosts.set((hosts) => [...hosts, id]); + return () => overlayHosts.set((hosts) => hosts.filter((h) => h !== id)); }, [overlay, id]); - const top = useSyncExternalStore(subscribeHosts, topHost, topHost); + const top = useSyncExternalStore(overlayHosts.subscribe, topHost, topHost); const active = useSyncExternalStore(subscribe, getActiveToasts, getActiveToasts); - const toasts = top === (overlay ? id : null) ? active : []; + const isTop = top === (overlay ? id : null); + const toasts = isTop ? active : []; + // 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. + const inherited = useMemo( + () => new Set(isTop ? getActiveToasts().map((n) => n.id) : []), + [isTop], + ); + const [armed, setArmed] = useState(false); + useEffect(() => setArmed(isTop), [isTop]); return ( - // Stable, always-mounted polite live region — a polite region must PRE-EXIST in the DOM for a - // screen reader to announce content later inserted into it (a region created together with its - // first message is unreliably announced). `aria-live` (not role="status") on purpose: - // role="status" implies aria-atomic="true", which would re-read the WHOLE stack on every toast; - // aria-live defaults atomic=false, so only the newly-added toast is spoken. Error/fatal toasts - // additionally carry role="alert", which announces assertively on insertion regardless. + // Stable, always-mounted live region — a polite region must PRE-EXIST in the DOM for a screen + // reader to announce content later inserted into it (a region created together with its first + // message is unreliably announced). `aria-live` (not role="status") on purpose: role="status" + // implies aria-atomic="true", which would re-read the WHOLE stack on every toast; aria-live + // defaults atomic=false, so only the newly-added toast is spoken. Error/fatal toasts additionally + // carry role="alert", which announces assertively on insertion regardless. + // + // Inside an overlay it sits ABOVE the bottom band: that is where the players keep their control + // bars (the Plex transport row, PlayerModal's docked bar), and the toast stack — on top of them at + // z-overlay, and catching the click — took play/pause, ±10, stop, prev/next and mute for as long + // as a toast was up (the connection notice: until the server answered). The same rung UpNextCard + // uses for the same reason. Clicks between the cards go through to what is underneath.
e.stopPropagation() : undefined} > @@ -61,8 +77,8 @@ export default function Toaster({ overlay = false }: { overlay?: boolean }) { return (
@@ -95,10 +111,8 @@ export default function Toaster({ overlay = false }: { overlay?: boolean }) { {toast.duration && ( -
+ // Keyed by `ts`: a coalesced repeat re-arms the dismiss timer, so its bar restarts too. + )}
); @@ -106,3 +120,17 @@ export default function Toaster({ overlay = false }: { overlay?: boolean }) {
); } + +/** The auto-dismiss countdown, started as far in as the toast already is. A toast handed to another + * host is a new node, and its bar used to start again from full while the dismiss timer — armed + * once, at notify time — did not: the toast vanished with the bar still two-thirds full. The offset + * is taken once, at mount; recomputed on a later render it would jump the running animation ahead. */ +function CountdownBar({ since, duration, bar }: { since: number; duration: number; bar: string }) { + const [elapsed] = useState(() => Math.min(duration, Math.max(0, Date.now() - since))); + return ( +
+ ); +}