From 7541b1b0164a469be63c7475b425c1b998b58a1c Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 8 Oct 2026 22:12:43 +0200 Subject: [PATCH] fix(ui): membership menus keep their lines beside the list and focus in the panel The loading / failed-load lines and their Retry sat inside role=menu, which may own its items only: the three surfaces now open with `listInside` and the rows sit in a PopoverList. A keyboard Retry unmounted the focused button and left focus on with the menu open, the arrows dead: focus is put back on the first row, else the field, else the new Retry. The channel tag editor said a failed tag load as "no tags yet"; it reads the catalog query's state now. The playlist panel's name and the tag editor's error state are held by tests. --- frontend/src/components/AddToPlaylist.tsx | 1 + .../ChannelTags.editor.dom.test.tsx | 61 ++++++++++++++++ frontend/src/components/ChannelTags.tsx | 9 ++- .../PlaylistEditFailures.dom.test.tsx | 26 ++++++- frontend/src/components/ui/MembershipMenu.tsx | 71 +++++++++++++------ 5 files changed, 145 insertions(+), 23 deletions(-) create mode 100644 frontend/src/components/ChannelTags.editor.dom.test.tsx diff --git a/frontend/src/components/AddToPlaylist.tsx b/frontend/src/components/AddToPlaylist.tsx index 6055a434..8ad527a0 100644 --- a/frontend/src/components/AddToPlaylist.tsx +++ b/frontend/src/components/AddToPlaylist.tsx @@ -152,6 +152,7 @@ function MembershipPanel({ popover={pop} className={`${SURFACE.popover} w-60 p-2`} ariaLabel={t("playlists.addToPlaylistTitle")} + listInside > { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), + }; +}); + +import { ChannelTagsControl } from "./ChannelTags"; +import { api } from "../lib/api"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe("the channel tag editor", () => { + it("says a failed load of the tags as one, with a retry — not as no tags yet", async () => { + vi.spyOn(api, "tags").mockRejectedValue(new Error("down")); + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + + {}} + onCreate={() => {}} + /> + + , + ); + fireEvent.click(screen.getByLabelText("channels.row.editTags")); + const alert = await screen.findByRole("alert", { hidden: true }); + expect(alert.textContent).toContain("common.loadError"); + expect(screen.queryByText("channels.tags.noneYet")).toBeNull(); + expect(alert.closest("[role=menu]")).toBeNull(); + expect(await screen.findByText("common.retry")).toBeTruthy(); + }); +}); diff --git a/frontend/src/components/ChannelTags.tsx b/frontend/src/components/ChannelTags.tsx index c78bffa4..6df99b54 100644 --- a/frontend/src/components/ChannelTags.tsx +++ b/frontend/src/components/ChannelTags.tsx @@ -9,6 +9,7 @@ import { chipLaneClass, useChipOverflowFit } from "../lib/useChipOverflowFit"; import { Chip, DismissibleChip } from "./ui/Chip"; import { chipClass } from "./ui/chipStyles"; import { MembershipMenu } from "./ui/MembershipMenu"; +import { useTags } from "../lib/useTags"; import { SURFACE } from "./ui/surfaces"; // The channel tag surface, shared by the channel manager (table + cards) and the channel detail @@ -191,7 +192,9 @@ function SuggestedChipsLane({ } /** The editor body: toggle the user's existing tags on/off for this channel, plus an inline - * "create + attach" input. Rendered inside an anchored `menu` popover. */ + * "create + attach" input. Rendered inside an anchored `menu` popover. The rows are the caller's + * `userTags`; whether that list has loaded is read from the catalog query itself (mounted only while + * the editor is open), so a failed or pending load is said as such, not as "no tags yet". */ function TagEditorMenu({ userTags, attachedIds, @@ -204,12 +207,15 @@ function TagEditorMenu({ onCreate: (name: string) => void; }) { const { t } = useTranslation(); + const tags = useTags(); const attached = new Set(attachedIds); return ( ({ id: tg.id, label: tg.name, checked: attached.has(tg.id) }))} onToggle={(row) => onToggle(row.id as number)} + status={tags.isError && !tags.data ? "error" : tags.isPending ? "loading" : "ready"} + onRetry={() => void tags.refetch()} emptyText={t("channels.tags.noneYet")} errorText={t("common.loadError")} create={{ @@ -422,6 +428,7 @@ export function ChannelTagsControl({ ({ })); import PlexPlaylistView from "./PlexPlaylistView"; -import AddToPlaylist from "./AddToPlaylist"; +import AddToPlaylist, { AddToPlaylistAt } from "./AddToPlaylist"; import PlexPlaylistAdd from "./PlexPlaylistAdd"; import TagManager from "./TagManager"; import { api, HttpError, type PlexCard } from "../lib/api"; @@ -170,6 +170,30 @@ describe("the add-to-playlist popover", () => { expect(screen.queryByText("playlists.noneYet")).toBeNull(); }); + it("opened from a card's menu, is a menu named for itself, its lines beside the list", async () => { + vi.spyOn(api, "playlists").mockRejectedValue(new Error("down")); + const anchor = document.createElement("button"); + document.body.append(anchor); + render( + + {}} /> + , + ); + // The menu row that opened it is gone: its own name, not a label pointing at that row. jsdom + // computes no name for the unpositioned surface, so read what names it. + const menu = await waitFor(() => { + const found = document.querySelector( + '[role="menu"][aria-label="playlists.addToPlaylistTitle"]', + ); + expect(found).not.toBeNull(); + return found!; + }); + expect(menu.hasAttribute("aria-labelledby")).toBe(false); + const alert = await screen.findByRole("alert", { hidden: true }); + expect(alert.closest("[role=menu]")).toBeNull(); + anchor.remove(); + }); + it("flips the tick as soon as an add lands, so the next tap takes it out again", async () => { const lists = vi.spyOn(api, "playlists"); lists.mockResolvedValueOnce([{ id: 3, name: "Mix", kind: "user", has_video: false }] as never); diff --git a/frontend/src/components/ui/MembershipMenu.tsx b/frontend/src/components/ui/MembershipMenu.tsx index caa3a8b0..2ab11c5d 100644 --- a/frontend/src/components/ui/MembershipMenu.tsx +++ b/frontend/src/components/ui/MembershipMenu.tsx @@ -1,8 +1,8 @@ -import { useRef, useState, type ReactNode } from "react"; +import { useEffect, useRef, useState, type ReactNode } from "react"; import { useTranslation } from "react-i18next"; import { Plus } from "lucide-react"; import { useScrollFade } from "../../lib/useScrollFade"; -import { PopoverItem } from "./AnchoredPopover"; +import { PopoverItem, PopoverList } from "./AnchoredPopover"; import { IconButton } from "./IconButton"; import { Input } from "./Input"; import { MenuCheck } from "./MenuCheck"; @@ -17,6 +17,13 @@ import { StateMessage } from "./QueryState"; // name field keeps its keys to itself — except Escape, which closes the menu, and the up/down arrows, // which step from the field into the rows: the rows may arrive after the surface opened, when the // popover's own first-row focus has already given up and left the focus in the field. +// +// The surface is opened with `listInside` too: the menu role sits on the rows' own box, and the lines +// about them (loading, a failed load with its Retry, none yet), the heading and the field sit beside +// it — a menu may own its items only. And when the state line swaps for the rows (or back), focus that +// was on its Retry would be left on with the panel still open, the arrows dead: it is put back +// in the panel — on the first row, else the field, else the new Retry. A spot it was put on only +// because the rows were not there yet (the field, while a retry loads) hands on to the first row. export interface MembershipRow { id: string | number; @@ -64,8 +71,28 @@ export function MembershipMenu({ const { t } = useTranslation(); const { ref: fadeRef, style: fadeStyle } = useScrollFade(); const listRef = useRef(null); + const rootRef = useRef(null); const [draft, setDraft] = useState(""); + const shownStatus = useRef(status); + const placed = useRef(null); + useEffect(() => { + if (shownStatus.current === status) return; + shownStatus.current = status; + const root = rootRef.current; + const at = document.activeElement; + // …unless a name is being typed into it meanwhile. + const parked = at === placed.current && !(at instanceof HTMLInputElement && at.value); + const lost = at == null || at === document.body || parked; + if (!root || !lost) return; + const target = + root.querySelector("[role=menuitemcheckbox]") ?? + root.querySelector("input") ?? + root.querySelector("button"); + target?.focus(); + placed.current = target; + }, [status]); + async function submit() { const name = draft.trim(); if (!name) return; @@ -86,7 +113,7 @@ export function MembershipMenu({ const rowText = dense ? "text-xs px-2" : "text-sm px-1.5"; return ( // A portal still bubbles through the React tree: keep a click here off the card that opened it. -
e.stopPropagation()}> +
e.stopPropagation()}> {heading &&
{heading}
}
{emptyText}

) : null} - {status === "ready" && - rows.map((row) => ( - onToggle(row)} - label={row.label} - className={`w-full flex items-center gap-2 ${rowText} py-1.5 rounded-lg outline-hidden transition ${ - dense && !row.checked ? "text-muted" : "text-fg" - }`} - > - - {row.label} - {row.extra} - - ))} + + {status === "ready" && + rows.map((row) => ( + onToggle(row)} + label={row.label} + className={`w-full flex items-center gap-2 ${rowText} py-1.5 rounded-lg outline-hidden transition ${ + dense && !row.checked ? "text-muted" : "text-fg" + }`} + > + + {row.label} + {row.extra} + + ))} +
{create && (