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 <body> 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.
This commit is contained in:
@@ -152,6 +152,7 @@ function MembershipPanel({
|
||||
popover={pop}
|
||||
className={`${SURFACE.popover} w-60 p-2`}
|
||||
ariaLabel={t("playlists.addToPlaylistTitle")}
|
||||
listInside
|
||||
>
|
||||
<MembershipMenu
|
||||
heading={t("playlists.addToPlaylistTitle")}
|
||||
|
||||
@@ -0,0 +1,61 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// The channel tag editor says a failed load of the tag catalog as one, with a retry — not as "no tags
|
||||
// yet, create one below", which invited a duplicate of a tag that exists. i18n is key-passthrough.
|
||||
import { describe, it, expect, afterEach, beforeAll, vi } from "vitest";
|
||||
import { render, screen, fireEvent, cleanup } from "@testing-library/react";
|
||||
import { FloatingTree } from "@floating-ui/react";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
|
||||
vi.mock("react-i18next", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("react-i18next")>();
|
||||
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(
|
||||
<QueryClientProvider client={qc}>
|
||||
<FloatingTree>
|
||||
<ChannelTagsControl
|
||||
userTags={[]}
|
||||
attachedIds={[]}
|
||||
onToggle={() => {}}
|
||||
onCreate={() => {}}
|
||||
/>
|
||||
</FloatingTree>
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
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();
|
||||
});
|
||||
});
|
||||
@@ -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 (
|
||||
<MembershipMenu
|
||||
dense
|
||||
rows={userTags.map((tg) => ({ 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({
|
||||
<PopoverSurface
|
||||
popover={editPop}
|
||||
className={`${SURFACE.menu} w-56 animate-[popIn_0.16s_ease]`}
|
||||
listInside
|
||||
>
|
||||
<TagEditorMenu
|
||||
userTags={userTags}
|
||||
|
||||
@@ -30,7 +30,7 @@ vi.mock("../lib/notifications", async (importOriginal) => ({
|
||||
}));
|
||||
|
||||
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(
|
||||
<QueryClientProvider client={client()}>
|
||||
<AddToPlaylistAt videoId="v1" anchor={anchor} onClose={() => {}} />
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
// 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);
|
||||
|
||||
@@ -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 <body> 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<HTMLDivElement | null>(null);
|
||||
const rootRef = useRef<HTMLDivElement | null>(null);
|
||||
const [draft, setDraft] = useState("");
|
||||
|
||||
const shownStatus = useRef(status);
|
||||
const placed = useRef<HTMLElement | null>(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<HTMLElement>("[role=menuitemcheckbox]") ??
|
||||
root.querySelector<HTMLElement>("input") ??
|
||||
root.querySelector<HTMLElement>("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.
|
||||
<div className="flex flex-col" onClick={(e) => e.stopPropagation()}>
|
||||
<div ref={rootRef} className="flex flex-col" onClick={(e) => e.stopPropagation()}>
|
||||
{heading && <div className="text-xs text-muted px-1.5 pb-1.5">{heading}</div>}
|
||||
<div
|
||||
ref={setList}
|
||||
@@ -103,24 +130,26 @@ export function MembershipMenu({
|
||||
) : rows.length === 0 ? (
|
||||
<p className={`text-xs text-muted py-2 ${dense ? "px-2" : "px-1.5"}`}>{emptyText}</p>
|
||||
) : null}
|
||||
{status === "ready" &&
|
||||
rows.map((row) => (
|
||||
<PopoverItem
|
||||
key={row.id}
|
||||
// A thing can sit in several sets at once — checkboxes, not one choice.
|
||||
role="menuitemcheckbox"
|
||||
checked={row.checked}
|
||||
onSelect={() => 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"
|
||||
}`}
|
||||
>
|
||||
<MenuCheck kind="many" on={row.checked} />
|
||||
<span className="truncate flex-1 text-left">{row.label}</span>
|
||||
{row.extra}
|
||||
</PopoverItem>
|
||||
))}
|
||||
<PopoverList>
|
||||
{status === "ready" &&
|
||||
rows.map((row) => (
|
||||
<PopoverItem
|
||||
key={row.id}
|
||||
// A thing can sit in several sets at once — checkboxes, not one choice.
|
||||
role="menuitemcheckbox"
|
||||
checked={row.checked}
|
||||
onSelect={() => 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"
|
||||
}`}
|
||||
>
|
||||
<MenuCheck kind="many" on={row.checked} />
|
||||
<span className="truncate flex-1 text-left">{row.label}</span>
|
||||
{row.extra}
|
||||
</PopoverItem>
|
||||
))}
|
||||
</PopoverList>
|
||||
</div>
|
||||
{create && (
|
||||
<div
|
||||
|
||||
Reference in New Issue
Block a user