diff --git a/frontend/src/components/PlexPlayer.controls.dom.test.tsx b/frontend/src/components/PlexPlayer.controls.dom.test.tsx index 7737765d..2916bc87 100644 --- a/frontend/src/components/PlexPlayer.controls.dom.test.tsx +++ b/frontend/src/components/PlexPlayer.controls.dom.test.tsx @@ -183,6 +183,17 @@ function pressPicture(pointerType: "touch" | "mouse", x = 450) { fireEvent.click(v, { clientX: x, clientY: 250 }); } +// A menu's rows are named by their text: the surface is `visibility: hidden` until floating-ui has +// measured it (never, under jsdom), and a hidden element's accessible name is empty. +const menuItem = (label: string) => + waitFor(() => { + const row = screen + .queryAllByRole("menuitem", { hidden: true }) + .find((b) => b.textContent === label); + expect(row).toBeDefined(); + return row!; + }); + describe("a tap on the picture (C3)", () => { it("a mouse click plays or pauses; a finger's tap only shows or hides the controls", async () => { mount({ details: { A: item("A") } }); @@ -277,13 +288,58 @@ describe("the C5 row", () => { "plex.player.help", "plex.player.download", ]; - for (const r of rows) expect(screen.getByRole("button", { name: r }), r).toBeTruthy(); + const menu = await screen.findByRole("menu", { hidden: true }); + expect( + [...menu.querySelectorAll('[role="menuitem"]')].map( + (b) => b.getAttribute("aria-label") ?? b.textContent, + ), + ).toEqual(rows); // A row closes the menu and opens its tool: Settings → the tuning panel. - fireEvent.click(screen.getByRole("button", { name: "plex.player.settings" })); - expect(screen.queryByRole("button", { name: "plex.player.stats.title" })).toBeNull(); + fireEvent.click(await menuItem("plex.player.settings")); + await screen.findByRole("dialog", { name: "plex.player.settings" }); + expect( + screen + .queryAllByRole("menuitem", { hidden: true }) + .find((b) => b.textContent === "plex.player.stats.title") ?? null, + ).toBeNull(); expect(screen.getByLabelText("plex.player.more").getAttribute("aria-expanded")).toBe("true"); }); + // E28 review D3: the ⋯ was a hand-built
of buttons — no menu role, no arrow keys, and Escape + // closed it with the focus left on . It is a menu on the popover engine now. + it("walks the ⋯ with the arrows, and Escape gives the focus back to the ⋯", async () => { + mount({ details: { A: item("A") } }); + await playingAt("/media/A", 100); + const more = screen.getByLabelText("plex.player.more"); + more.focus(); + fireEvent.click(more); + const menu = await screen.findByRole("menu", { hidden: true }); + const rows = [...menu.querySelectorAll('[role="menuitem"]')]; + await waitFor(() => expect(document.activeElement).toBe(rows[0])); + fireEvent.keyDown(document.activeElement!, { key: "ArrowDown" }); + expect(document.activeElement).toBe(rows[1]); + fireEvent.keyDown(document.activeElement!, { key: "End" }); + expect(document.activeElement).toBe(rows[rows.length - 1]); + fireEvent.keyDown(document.activeElement!, { key: "Escape" }); + await waitFor(() => expect(screen.queryByRole("menu")).toBeNull()); + await waitFor(() => expect(document.activeElement).toBe(more)); + }); + + it("gives the focus back to the ⋯ when the tuning panel closes from inside", async () => { + mount({ details: { A: item("A") } }); + await playingAt("/media/A", 100); + const more = screen.getByLabelText("plex.player.more"); + fireEvent.click(more); + fireEvent.click(await menuItem("plex.player.settings")); + const panel = await screen.findByRole("dialog", { name: "plex.player.settings" }); + // Into the panel on open… + await waitFor(() => expect(panel.contains(document.activeElement)).toBe(true)); + fireEvent.keyDown(document.activeElement!, { key: "Escape" }); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + // …and back on the ⋯ when it goes, not on . + await waitFor(() => expect(document.activeElement).toBe(more)); + }); + it("Next shows only when there is a next item", async () => { mount({ details: { A: item("A") } }); await playingAt("/media/A", 100); diff --git a/frontend/src/components/PlexPlayer.session.dom.test.tsx b/frontend/src/components/PlexPlayer.session.dom.test.tsx index 44dbb8b2..650282ed 100644 --- a/frontend/src/components/PlexPlayer.session.dom.test.tsx +++ b/frontend/src/components/PlexPlayer.session.dom.test.tsx @@ -261,9 +261,9 @@ describe("the subtitle poll and notice", () => { await screen.findByText("Title A"); fireEvent.click(screen.getByLabelText("plex.player.tracks")); const off = await screen.findByText("plex.player.subOff"); - expect(off.closest("button")?.getAttribute("aria-pressed")).toBe("false"); - const pressed = document.querySelectorAll('[aria-pressed="true"]'); - expect([...pressed].map((b) => b.textContent)).toEqual(["English"]); // the saved subtitle + expect(off.closest('[role="menuitemradio"]')?.getAttribute("aria-checked")).toBe("false"); + const checked = document.querySelectorAll('[role="menuitemradio"][aria-checked="true"]'); + expect([...checked].map((b) => b.textContent)).toEqual(["English"]); // the saved subtitle }); }); diff --git a/frontend/src/components/PlexPlayer.tabtrap.dom.test.tsx b/frontend/src/components/PlexPlayer.tabtrap.dom.test.tsx index 3311cfac..e4dabf25 100644 --- a/frontend/src/components/PlexPlayer.tabtrap.dom.test.tsx +++ b/frontend/src/components/PlexPlayer.tabtrap.dom.test.tsx @@ -8,7 +8,7 @@ // the round-3 comment that handed the step prompt's trap to "the dialog below" assumed this one did // too. Its own file because `layerStack` is module state and these tests push into it. import { describe, it, expect, vi, beforeAll, afterEach } from "vitest"; -import { render, cleanup, waitFor, act } from "@testing-library/react"; +import { render, cleanup, waitFor, act, screen } from "@testing-library/react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; vi.mock("react-i18next", async (importOriginal) => ({ @@ -116,6 +116,17 @@ const press = (key: string, shiftKey = false) => ); }); +// A menu's rows are named by their text: the surface is `visibility: hidden` until floating-ui has +// measured it (never, under jsdom), and a hidden element's accessible name is empty. +const menuItem = (label: string) => + waitFor(() => { + const row = screen + .queryAllByRole("menuitem", { hidden: true }) + .find((b) => b.textContent === label); + expect(row).toBeDefined(); + return row!; + }); + describe("PlexPlayer · Tab stays inside the player", () => { it("pulls the focus back in when it is on the page behind", async () => { await mount(); @@ -160,9 +171,10 @@ describe("PlexPlayer · Tab stays inside the player", () => { const more = buttons().find((b) => b.getAttribute("aria-label") === "plex.player.more"); expect(more).toBeDefined(); act(() => more!.click()); - const settings = buttons().find((b) => b.textContent === "plex.player.settings"); - expect(settings).toBeDefined(); - act(() => settings!.click()); + // The ⋯ is a menu on the popover engine: it portals out of the player and traps for itself. + const settings = await menuItem("plex.player.settings"); + act(() => settings.click()); + await screen.findByRole("dialog", { name: "plex.player.settings" }); const items = buttons(); // The panel really opened — the trap now has to span its controls too. expect(items.length).toBeGreaterThan(before + 3); diff --git a/frontend/src/components/PlexPlayer.tsx b/frontend/src/components/PlexPlayer.tsx index 4a95f0a5..d759114d 100644 --- a/frontend/src/components/PlexPlayer.tsx +++ b/frontend/src/components/PlexPlayer.tsx @@ -82,7 +82,8 @@ import { } from "../lib/layerStack"; import { Button } from "./ui/Button"; import { IconButton } from "./ui/IconButton"; -import { Pressable } from "./ui/Pressable"; +import { PopoverItem, PopoverSurface } from "./ui/AnchoredPopover"; +import { closeThenAct, useAnchoredPopover } from "./ui/anchoredPopoverHooks"; import { Slider } from "./ui/NativeControls"; import { formatUnit } from "../lib/format"; @@ -104,6 +105,10 @@ const RECLAIM_WINDOW_MS = 120_000; const BACK_TO_PREV_S = 3; // C5: a touch double-tap on the picture's left or right third skips this far; each further tap // inside the window skips again (the keyboard keeps ← / → at the viewer's own step). +// The player's menus sit on the always-dark media chrome, under white text, in every theme. +const PLAYER_MENU = + "rounded-xl border border-white/15 bg-media-chrome/95 text-sm text-white shadow-2xl"; +const PLAYER_ROW = "block w-full rounded-lg px-2 py-1.5 text-left hover:bg-white/10"; const TAP_SKIP_S = 10; const DOUBLE_TAP_MS = 300; @@ -236,7 +241,6 @@ export default function PlexPlayer({ itemId, onClose, queue, fromStart = false } const [menuOpen, setMenuOpen] = useState(false); // the tuning panel (tabbed), from ⋯ → Settings // The fixed ⋯ (C5): the rare tools — settings, media info, stats, shortcuts, the original file. const [moreOpen, setMoreOpen] = useState(false); - const moreRef = useRef(null); const [tracksOpen, setTracksOpen] = useState(false); // separate quick menu = audio/subtitle tracks const [settingsTab, setSettingsTab] = useState<"sync" | "playback" | "subtitle" | "clock">( "sync", @@ -244,9 +248,7 @@ export default function PlexPlayer({ itemId, onClose, queue, fromStart = false } const audioRef = useRef(null); const menuOpenRef = useRef(false); const menuRef = useRef(null); - const menuTriggerRef = useRef(null); - const tracksRef = useRef(null); - const tracksTriggerRef = useRef(null); + const menuTriggerRef = useRef(null); menuOpenRef.current = menuOpen || tracksOpen || moreOpen; // keep the control bar up while one is open // Auto-hiding control bar (reveal on mouse move / any key; hide after 3s while playing). const { @@ -265,21 +267,40 @@ export default function PlexPlayer({ itemId, onClose, queue, fromStart = false } onPointerEnter: () => holdOnHover(true), onPointerLeave: () => holdOnHover(false), }; - // Each menu stays open until a click/Escape OUTSIDE it (so sliders can be dragged freely). + // The tuning panel stays open until a click/Escape OUTSIDE it (so sliders can be dragged freely). + // It renders inside the player, whose Tab trap spans it; the ⋯ counts as inside, since the ⋯ is + // what closes it (below). useDismiss(menuOpen, () => setMenuOpen(false), [menuRef, menuTriggerRef]); - useDismiss(tracksOpen, () => setTracksOpen(false), [tracksRef, tracksTriggerRef]); - useDismiss(moreOpen, () => setMoreOpen(false), [moreRef, menuTriggerRef]); - // The tracks menu is both dismiss-tracked and edge-faded, so its two refs are merged here. - // Must be stable: an inline arrow would detach (null) and re-attach the node on every render, - // and the fade's node-state ref would re-render in response — a loop. `fade.ref` is a setState. - const tracksFade = useScrollFade(); - const tracksFadeRef = tracksFade.ref; - const setTracksNode = useCallback( - (node: HTMLElement | null) => { - tracksRef.current = node as HTMLDivElement | null; - tracksFadeRef(node); + // The ⋯ and the tracks list are real menus on the popover engine (E28 review D3): roving arrows, + // Home/End, Escape as a layer, and the focus back on the trigger however they close. They were + // hand-built
s — no roles, no arrows, and Escape left the focus on . The engine portals + // them into the fullscreen element when there is one, so they show in fullscreen too. + const morePop = useAnchoredPopover({ + open: moreOpen, + // The ⋯ also CLOSES the tuning panel it opened, rather than stacking the menu over it. + onOpenChange: (open) => { + if (open && menuOpen) setMenuOpen(false); + else setMoreOpen(open); }, - [tracksFadeRef], + placement: "top-end", + offset: 8, + }); + const tracksPop = useAnchoredPopover({ + open: tracksOpen, + onOpenChange: setTracksOpen, + placement: "top-end", + offset: 8, + fitHeight: true, + }); + const tracksFade = useScrollFade(); + // The ⋯ is both the menu's trigger and the tuning panel's dismiss exception and focus return. + const setMoreReference = morePop.refs.setReference; + const moreTriggerRef = useCallback( + (node: HTMLButtonElement | null) => { + menuTriggerRef.current = node; + setMoreReference(node); + }, + [setMoreReference], ); // The keyboard cheat sheet is app-wide now (ui/KeyboardHelp, opened by the global "h"), not this // player's own overlay. Mirror its open state into a ref because usePlayerVolume still needs to @@ -1593,74 +1614,89 @@ export default function PlexPlayer({ itemId, onClose, queue, fromStart = false }
- {/* Quick TRACKS menu (audio / subtitle) — the frequent action, on its own button. */} + {/* Quick TRACKS menu (audio / subtitle) — the frequent action, on its own button. Picking + a track keeps it open: the change is seen where it was made, and the other group is + one arrow away. */} {detail && (detail.audio_streams.length > 1 || textSubs.length > 0) && ( -
+ <> setTracksOpen((o) => !o)} + ref={tracksPop.refs.setReference} + {...tracksPop.getReferenceProps()} aria-expanded={tracksOpen} label={t("plex.player.tracks")} > - {tracksOpen && ( + + {/* Portaled, but a React child of the player: a click or a wheel here must not + reach the stage's play-toggle and volume handlers. */}
e.stopPropagation()} onWheel={(e) => e.stopPropagation()} - className="border border-white/15 bg-media-chrome/95 absolute bottom-full right-0 z-menu mb-2 w-56 max-h-[60vh] overflow-auto no-scrollbar rounded-xl p-2 text-sm text-white shadow-2xl" + style={{ ...tracksFade.style, maxHeight: "min(60dvh, var(--fit-max-h))" }} + className="overflow-auto no-scrollbar p-2" > {detail.audio_streams.length > 1 && ( - <> +
{t("plex.player.audio")} {detail.audio_streams.map((a) => ( - changeAudio(a.ord)} - aria-pressed={(audioOrd ?? 0) === a.ord} - className={`block w-full truncate rounded px-2 py-1 hover:bg-white/10 ${ + tone="media" + role="menuitemradio" + checked={(audioOrd ?? 0) === a.ord} + label={a.label} + onSelect={() => changeAudio(a.ord)} + className={`${PLAYER_ROW} truncate ${ (audioOrd ?? 0) === a.ord ? "text-accent" : "text-white" }`} > {a.label} - + ))} - +
)} {textSubs.length > 0 && ( - <> +
{t("plex.player.subtitles")} - changeSubtitle(null)} - aria-pressed={subOrd == null} - className={`block w-full rounded px-2 py-1 hover:bg-white/10 ${ - subOrd == null ? "text-accent" : "text-white" - }`} + changeSubtitle(null)} + className={`${PLAYER_ROW} ${subOrd == null ? "text-accent" : "text-white"}`} > {t("plex.player.subOff")} - - {textSubs.map((s) => ( - changeSubtitle(s.ord)} - aria-pressed={subOrd === s.ord} - className={`block w-full truncate rounded px-2 py-1 hover:bg-white/10 ${ - subOrd === s.ord ? "text-accent" : "text-white" + + {textSubs.map((sub) => ( + changeSubtitle(sub.ord)} + className={`${PLAYER_ROW} truncate ${ + subOrd === sub.ord ? "text-accent" : "text-white" }`} > - {s.label} - + {sub.label} + ))} - +
)}
- )} -
+ + )} {/* The fixed ⋯ (C5): the rare tools. Settings opens the tuning panel from the same spot, @@ -1670,40 +1706,39 @@ export default function PlexPlayer({ itemId, onClose, queue, fromStart = false } variant="overlay" size="player" round - ref={menuTriggerRef} - onClick={() => (menuOpen ? setMenuOpen(false) : setMoreOpen((o) => !o))} + ref={moreTriggerRef} + {...morePop.getReferenceProps()} aria-expanded={moreOpen || menuOpen} label={t("plex.player.more")} > - {moreOpen && ( -
e.stopPropagation()} - className="border border-white/15 bg-media-chrome/95 absolute bottom-full right-0 z-menu mb-2 flex w-60 flex-col rounded-xl p-1.5 text-sm text-white shadow-2xl" - > + +
e.stopPropagation()} className="flex flex-col"> {moreItems.map((it) => ( - { - setMoreOpen(false); - // Back on the ⋯ before the tool opens: its row is about to unmount, and a - // dialog opened from here returns the focus to where it found it. - menuTriggerRef.current?.focus(); - it.run(); - }} - className="touch-target flex min-h-9 w-full items-center gap-2.5 rounded-lg px-2.5 text-left hover:bg-white/10" + tone="media" + label={it.label} + // Closes first, so the focus is back on the ⋯ before the tool opens: a dialog + // opened from here returns the focus to where it found it. + onSelect={() => closeThenAct(() => setMoreOpen(false), it.run)} + className={`${PLAYER_ROW} flex min-h-9 items-center gap-2.5 text-white`} > {it.icon} {it.label} - + ))}
- )} + {menuOpen && ( ` lied // about that and 18's types let it. menuRef: RefObject; + /** The ⋯ that opened the panel: the focus goes back to it when the panel closes with the focus + * inside — by Escape, Back or the ⋯ itself — instead of falling to (E28 review D3). */ + returnFocusTo: RefObject; settingsTabs: { key: SettingsTab; label: string }[]; activeTab: SettingsTab; setSettingsTab: (tab: SettingsTab) => void; @@ -28,6 +31,7 @@ export function PlexPlayerSettings(props: { }) { const { menuRef, + returnFocusTo, settingsTabs, activeTab, setSettingsTab, @@ -37,9 +41,23 @@ export function PlexPlayerSettings(props: { now, } = props; const { t } = useTranslation(); + // Into the panel on open (its first tab), and back to the ⋯ on close. The cleanup runs while the + // panel is still in the DOM, so "the focus was inside" is read before its removal drops it to + // ; focus the reader moved elsewhere (a click on the video) stays where they put it. + useLayoutEffect(() => { + const panel = menuRef.current; + const back = returnFocusTo.current; // the ⋯ outlives the panel: it is the panel's own trigger + panel?.querySelector("button")?.focus({ preventScroll: true }); + return () => { + if (panel?.contains(document.activeElement)) + queueMicrotask(() => back?.focus({ preventScroll: true })); + }; + }, [menuRef, returnFocusTo]); return (
e.stopPropagation()} onClick={(e) => e.stopPropagation()} className="border border-white/15 bg-media-chrome/95 absolute bottom-full right-0 z-menu mb-2 w-80 rounded-xl p-2 text-sm text-white shadow-2xl" diff --git a/frontend/src/components/ui/AnchoredPopover.tsx b/frontend/src/components/ui/AnchoredPopover.tsx index ca04c75b..f9296b5e 100644 --- a/frontend/src/components/ui/AnchoredPopover.tsx +++ b/frontend/src/components/ui/AnchoredPopover.tsx @@ -104,6 +104,10 @@ const HIGHLIGHT_SELECTED = "data-active:bg-accent/35 data-active:text-fg data-active:ring-2 data-active:ring-inset data-active:ring-accent/80"; const HIGHLIGHT_SELECTED_RING_ONLY = "data-active:ring-2 data-active:ring-inset data-active:ring-accent/70"; +// On the always-dark media chrome (a player's menus, `bg-media-chrome` under white text) the theme's +// `text-fg` is dark in the light theme: the highlighted row would go dark-on-dark. White instead. +const MEDIA_HIGHLIGHT = + "data-active:bg-white/15 data-active:text-white data-active:ring-1 data-active:ring-inset data-active:ring-white/40"; export function PopoverItem({ children, @@ -117,6 +121,7 @@ export function PopoverItem({ elementRef, /** Typeahead match string; defaults to the text child. Set explicitly when children aren't plain text. */ label, + tone, ...rest }: { children: ReactNode; @@ -134,6 +139,8 @@ export function PopoverItem({ * aria-haspopup/aria-expanded belong on that button — not on a wrapper div around it. */ elementRef?: (node: HTMLButtonElement | null) => void; label?: string; + /** `media`: the row sits on the always-dark media chrome (a player's menu), not a theme surface. */ + tone?: "media"; } & Omit, "onSelect" | "role" | "children" | "className">) { const ctx = useContext(PopoverListContext); const virtual = ctx?.virtual ?? false; @@ -212,11 +219,13 @@ export function PopoverItem({ disabled={disabled} // touch-target: a menu row is a control primitive, 44 tall on a coarse pointer (A3). className={`outline-hidden touch-target ${ - checked || selected - ? keepSelectedPaint - ? HIGHLIGHT_SELECTED_RING_ONLY - : HIGHLIGHT_SELECTED - : ROVING_HIGHLIGHT + tone === "media" + ? MEDIA_HIGHLIGHT + : checked || selected + ? keepSelectedPaint + ? HIGHLIGHT_SELECTED_RING_ONLY + : HIGHLIGHT_SELECTED + : ROVING_HIGHLIGHT }${className ? ` ${className}` : ""}`} > {children} diff --git a/frontend/src/components/ui/pressableSelectable.contract.test.ts b/frontend/src/components/ui/pressableSelectable.contract.test.ts index 04c4d3ea..a8537dd6 100644 --- a/frontend/src/components/ui/pressableSelectable.contract.test.ts +++ b/frontend/src/components/ui/pressableSelectable.contract.test.ts @@ -27,7 +27,6 @@ const PLAIN: Record = { "PlaylistsRail.tsx": [1, "the load-error retry"], "PlexCollectionEditor.tsx": [1, "a checkbox row in a picker"], "PlexInfo.tsx": [1, "the rating filter (a number)"], - "PlexPlayer.tsx": [4, "track picker rows and the ⋯ menu's rows"], "PlexPlayerSettings.tsx": [2, "setting options"], "PlexPlaylistAdd.tsx": [1, "a checkbox row in a picker"], "PlexPlaylistView.tsx": [1, "a show's poster stack (aria-hidden)"], diff --git a/frontend/src/lib/zLayer.contract.test.ts b/frontend/src/lib/zLayer.contract.test.ts index 6f0c2f92..c57ef586 100644 --- a/frontend/src/lib/zLayer.contract.test.ts +++ b/frontend/src/lib/zLayer.contract.test.ts @@ -46,8 +46,13 @@ describe("the Plex player's layers", () => { const popovers = classes( /\bbottom-full\b.*\bbg-media-chrome\b|\bbg-media-chrome\b.*\bbottom-full\b/, ); - expect(popovers).toHaveLength(3); // tracks, the ⋯ menu (C5), the settings panel + expect(popovers).toHaveLength(1); // the settings panel, which renders inside the player for (const c of popovers) expect(c).toMatch(/\bz-menu\b/); + // The tracks list and the ⋯ menu are on the popover engine (E28 review D3), which lifts them to + // z-popover — above the player's own z-overlay — and into the fullscreen element. + expect([...src.matchAll(/]*?className=\{`\$\{PLAYER_MENU\}/g)]).toHaveLength( + 2, + ); }); it("keeps the notices a level below", () => {