From 2984aa32405caea0b0ae87b0b9fc39e678d6195b Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 31 Jul 2026 02:10:14 +0200 Subject: [PATCH] fix(popover): resolve the R8 platform code-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness: - Escape is now strictly topmost-only for EVERY mode. Hover popovers register in the shared layerStack too (and floating-ui's own Escape is off for all modes), so an open tooltip/hover-card no longer either swallows the Escape from a Modal/player underneath (its document-level stopPropagation vs their window listeners) or double-closes both. A player's own overlay hover-card now takes Escape before the player, keeping it alive. - An invisible (referenceHidden, scrolled-out) surface is no longer a dismissable layer, so it can't silently eat an Escape aimed at what's actually on screen, nor make a player defer. - PopoverSurface splits into a wrapper that returns null before any hook and an inner that holds them, so a mounted-but-closed popover (≈10 per table row) subscribes no fullscreen listeners — was ~1800 document listeners on a 100-row table. - The fullscreen portal no longer remounts on a key: one persistent container is re-parented body↔fullscreen-element with appendChild (atomic move), so the open panel's focus/caret/scroll/animation survive a transition and an element SWAP is handled by the element-identity dependency. - AddToPlaylist drops `disabled={busy}` on its items (it blurred the focused item to and permanently killed roving focus); re-entrancy is already guarded in toggle(), and aria-busy conveys the pending state. - New `hostsInput` option turns off BOTH typeahead and focus-item-on-hover for the two menus with an inline text field, so a pointer passing over a list item no longer steals the field's caret and mis-routes the next Enter. Replaces their `typeahead:false`. - ChannelTags' single-pass fit loop gains a whole-set fast path, so it stops collapsing chips into "+N" when they would all have fitted. - `initialFocus={-1}` is applied only when a `selectedIndex` is set (the sync-focus race it works around); a plain nav menu now lets the focus manager focus normally, so an item-less menu (first-run tag editor) still has a keyboard entry point. a11y: - `ariaLabel` now wins on menu-role panels — the surface strips useRole's competing `aria-labelledby` when an explicit label is given (verified: the filter/View menus are named by their column/label, not the trigger). - PopoverItem routes caller props THROUGH getItemProps and applies its own tabIndex/disabled/data-active/className last, so a caller handler or tabIndex can no longer be silently dropped or clobber the roving model. Regressions / consistency: - Tooltip shows/hides on contact again (explicit 0/0 hover delay) instead of inheriting the platform's 120/140 ms intent delay. - ChannelTags' editor list regains its useScrollFade overflow mask. - ChannelTagsControl takes a `bounded` prop; the channel detail page passes false so tags show inline in the wide header instead of collapsing to "+N". - ChannelPage's tag-catalog query is gated on subscribed && !demo (matches the only consumer's render condition); the manager's tag toggle now also invalidates qk.channel(id) so the detail page's cached tags stay in step. - DetailCustomizeMenu + PrefToggle revert off the platform to the pre-R8 inline `absolute` menu: in the player's overlay it renders inside a transform:scale(1.25) subtree, and portalling out to painted it at 1×. Same reason the player's own menus stay hand-rolled; this also restores PrefToggle to plain buttons (the rows are independent toggles, not a radio group). The stale PlayerModal fullscreen comment is corrected too. Gate green (tsc/eslint/76 vitest). Self-UAT in-browser: hover-over-item no longer steals the tag input's caret; text filter keeps caret while narrowing; menu aria-label is the label not the trigger; selectedIndex lands focus on the checked item; roving + Escape-close+refocus intact; add-to-playlist items never disabled. NOT verified here (needs a device / playing media): the fullscreen portal reparent and the player-overlay paths — owed device UAT, as they already were. --- frontend/src/components/AddToPlaylist.tsx | 18 ++- frontend/src/components/AnchoredPopover.tsx | 144 +++++++++++++------- frontend/src/components/ChannelPage.tsx | 10 +- frontend/src/components/ChannelTags.tsx | 37 +++-- frontend/src/components/Channels.tsx | 3 + frontend/src/components/PlayerModal.tsx | 5 +- frontend/src/components/Tooltip.tsx | 3 + frontend/src/lib/plexDetailUi.tsx | 73 +++++----- 8 files changed, 195 insertions(+), 98 deletions(-) diff --git a/frontend/src/components/AddToPlaylist.tsx b/frontend/src/components/AddToPlaylist.tsx index e289f300..ec0199bb 100644 --- a/frontend/src/components/AddToPlaylist.tsx +++ b/frontend/src/components/AddToPlaylist.tsx @@ -30,7 +30,9 @@ export default function AddToPlaylist({ open, onOpenChange: setOpen, offset: 6, - typeahead: false, + // The menu holds a "new playlist" text field — keep typed chars and pointer-hover out of the + // roving list so they don't steal the caret. + hostsInput: true, }); const membership = useQuery({ @@ -101,7 +103,12 @@ export default function AddToPlaylist({ would otherwise reach the feed card that hosts the trigger and start playback. */}
e.stopPropagation()}>
{t("playlists.addToPlaylist")}
-
+
{lists.length === 0 && !membership.isLoading && (
{t("playlists.noneYet")}
)} @@ -112,9 +119,12 @@ export default function AddToPlaylist({ role="menuitemcheckbox" checked={pl.has_video} onSelect={() => toggle(pl)} - disabled={busy} + // NOT `disabled={busy}`: the native disabled attribute would blur the focused item to + // mid-request and permanently kill roving focus (useListNavigation listens on + // the floating element only). `toggle()` already no-ops while busy, so re-entrancy is + // guarded without touching focus; `aria-busy` on the list conveys the pending state. label={playlistName(pl, t)} - className="w-full flex items-center gap-2 text-sm px-1.5 py-1.5 rounded-lg text-fg outline-none transition hover:bg-card/60 disabled:opacity-50 data-[active]:bg-[color-mix(in_srgb,var(--accent)_24%,transparent)]" + className="w-full flex items-center gap-2 text-sm px-1.5 py-1.5 rounded-lg text-fg outline-none transition hover:bg-card/60 data-[active]:bg-[color-mix(in_srgb,var(--accent)_24%,transparent)]" > opts.onOpenChange(false)); + // `hide()` flips this true once the trigger is scrolled out of its clipping scroller; the surface is + // then painted invisible (PopoverSurface) but stays open so it can reappear on scroll-back. + const referenceHidden = middlewareData.hide?.referenceHidden ?? false; + + // Register the open popover as a dismissable layer so Escape is topmost-only and a player/modal + // underneath defers to it. Hover popovers register too (an open tooltip/hover-card should take + // Escape before the surface beneath it — and, when it is a player's own overlay card, keep the + // player alive). An invisible (referenceHidden) surface is NOT a layer: it must not silently eat + // an Escape the user aimed at whatever is actually on screen. + useLayer(opts.open && !referenceHidden, () => opts.onOpenChange(false)); return { nodeId, mode, isNav, open: opts.open, + selectedIndex: opts.selectedIndex ?? null, refs, floatingStyles, context, isPositioned, - referenceHidden: middlewareData.hide?.referenceHidden ?? false, + referenceHidden, elementsRef, labelsRef, activeIndex, @@ -244,43 +262,37 @@ export function PopoverItem({ ? { role, "aria-selected": !!selected } : { role }; + // Route the caller's own props (onClick, onFocus, …) THROUGH floating-ui's item-prop getter, which + // composes handlers rather than dropping them — spreading `{...rest}` separately would let the + // getter's own onClick/onFocus/onMouseMove silently clobber the caller's. `onClick` here runs the + // caller's first (e.g. a stopPropagation), then the select. + const mergedClick = (e: React.MouseEvent) => { + rest.onClick?.(e); + if (!disabled) onSelect?.(); + }; const handlers = ctx - ? ctx.getItemProps({ onClick: () => !disabled && onSelect?.() }) - : { onClick: () => !disabled && onSelect?.() }; + ? ctx.getItemProps({ ...rest, onClick: mergedClick }) + : { ...rest, onClick: mergedClick }; return ( ); } -/** Renders the popover panel: the FloatingTree node + portal + the positioned surface. Merges the - * library's floating ref/props with the caller's className/style. Menu/listbox surfaces trap+return - * focus and host the list-navigation registry by default; pass `focusManaged={false}` to opt a - * surface out (e.g. a hover card). */ -export function PopoverSurface({ - popover, - className, - style, - children, - focusManaged, - initialFocus, - passthrough, - ariaLabel, -}: { +interface PopoverSurfaceProps { popover: AnchoredPopover; className?: string; style?: CSSProperties; @@ -294,7 +306,30 @@ export function PopoverSurface({ * aimed at whatever it happens to cover during its close delay. Interactive hover cards (scrollable * content, links) must NOT set this. */ passthrough?: boolean; -}) { +} + +/** Renders the popover panel: the FloatingTree node + portal + the positioned surface. Merges the + * library's floating ref/props with the caller's className/style. Menu/listbox surfaces trap+return + * focus and host the list-navigation registry by default; pass `focusManaged={false}` to opt a + * surface out (e.g. a hover card). + * + * A thin wrapper so the closed case runs NO hooks: a mounted-but-closed popover (there can be ~10 + * per table row) must not subscribe fullscreen listeners or build a portal container. */ +export function PopoverSurface(props: PopoverSurfaceProps) { + if (!props.popover.open) return null; + return ; +} + +function PopoverSurfaceInner({ + popover, + className, + style, + children, + focusManaged, + initialFocus, + passthrough, + ariaLabel, +}: PopoverSurfaceProps) { const { context, refs, @@ -304,23 +339,36 @@ export function PopoverSurface({ nodeId, referenceHidden, isPositioned, - open, mode, isNav, + selectedIndex, activeIndex, elementsRef, labelsRef, } = popover; + // A node portaled to is painted UNDERNEATH whatever holds the fullscreen lock, so a popover - // opened from a player's control bar in fullscreen would be invisible. Portal into the fullscreen - // element instead while one exists — this is what let the player's in-bar menus stop being either - // non-portaled (and clipped) or outright disabled in fullscreen. + // opened from a player's control bar in fullscreen would be invisible; it must portal INTO the + // fullscreen element. We own one persistent container div and re-parent it (body ↔ fullscreen + // element) with appendChild, which MOVES the node atomically — no detach, so the open panel's + // focus, caret, scroll offset and animation state survive a fullscreen transition, and an element + // SWAP (A→B without exiting) is handled by the element-identity dependency. (Keying the portal to + // remount instead would rebuild the subtree and lose all of that.) const fsRoot = useFullscreenElement(); + const [container] = useState(() => + typeof document === "undefined" ? null : document.createElement("div"), + ); + useLayoutEffect(() => { + if (container) (fsRoot ?? document.body).appendChild(container); + }, [container, fsRoot]); + useEffect(() => () => container?.remove(), [container]); + const { style: floatingPropStyle, ...floatingProps } = getFloatingProps() as { style?: CSSProperties; } & Record; - - if (!open) return null; + // useRole stamps `aria-labelledby` (the trigger) on a menu-role panel, and it WINS over aria-label + // in the accessible-name computation — so an explicit ariaLabel is dead unless we drop the ref. + if (ariaLabel) delete floatingProps["aria-labelledby"]; const zClass = mode === "hover" ? "z-tooltip" : "z-popover"; // Everything but a passive hover card manages focus by default (trap into the surface, return to @@ -368,22 +416,18 @@ export function PopoverSurface({ return ( - {/* `root` must be undefined (NOT null) to mean "default to " — floating-ui reads a null - root as "the container doesn't exist yet" and creates no portal node at all. And it only - builds that node once, so a root that changes mid-open is ignored; the key remounts the - portal on a fullscreen transition, which is exactly when the popover must move. */} - + {managed ? ( {positioned} diff --git a/frontend/src/components/ChannelPage.tsx b/frontend/src/components/ChannelPage.tsx index d898d2de..2b28dfe6 100644 --- a/frontend/src/components/ChannelPage.tsx +++ b/frontend/src/components/ChannelPage.tsx @@ -171,7 +171,13 @@ export default function ChannelPage({ // Personal tags, editable right here (the same control + flow the channel manager uses). Optimistic // on this channel's detail cache; a failed attach/detach reverts by refetching, and a success // invalidates the manager list so a tag added here shows there too. - const tagsQuery = useQuery({ queryKey: qk.tags(), queryFn: api.tags }); + // Only fetch the tag catalog when the editor will actually render (subscribed, non-demo) — an + // unsubscribed/demo channel page never shows the control, so the request would be thrown away. + const tagsQuery = useQuery({ + queryKey: qk.tags(), + queryFn: api.tags, + enabled: !!ch?.subscribed && !me.is_demo, + }); const userTags = (tagsQuery.data ?? []).filter((tg) => !tg.system); const { toggleTag, createAndAttachTag } = useChannelTagActions({ optimisticToggle: (id, tagId, attach) => @@ -441,6 +447,8 @@ export default function ChannelPage({ attachedIds={ch.tag_ids} onToggle={(tagId) => toggleTag(channelId, tagId, ch.tag_ids.includes(tagId))} onCreate={(name) => createAndAttachTag(channelId, name)} + // The header row has free width — show tags inline instead of collapsing to "+N". + bounded={false} /> )}
diff --git a/frontend/src/components/ChannelTags.tsx b/frontend/src/components/ChannelTags.tsx index e138bb3f..8abed9f4 100644 --- a/frontend/src/components/ChannelTags.tsx +++ b/frontend/src/components/ChannelTags.tsx @@ -3,6 +3,7 @@ import { Check, Plus } from "lucide-react"; import { useTranslation } from "react-i18next"; import { type Tag } from "../lib/api"; import { useAnchoredPopover, PopoverSurface, PopoverItem } from "./AnchoredPopover"; +import { useScrollFade } from "../lib/useScrollFade"; // The channel tag surface, shared by the channel manager (table + cards) and the channel detail // page. Built on the R8 popover platform. Two design goals it fixes over the old inline TagsCell: @@ -53,6 +54,7 @@ function TagEditorMenu({ }) { const { t } = useTranslation(); const [draft, setDraft] = useState(""); + const fade = useScrollFade(); const attached = new Set(attachedIds); const submit = () => { const name = draft.trim(); @@ -62,7 +64,7 @@ function TagEditorMenu({ }; return (
-
+
{userTags.map((tg) => { const on = attached.has(tg.id); return ( @@ -135,6 +137,7 @@ export function ChannelTagsControl({ onToggle, onCreate, onChipClick, + bounded = true, }: { userTags: Tag[]; attachedIds: number[]; @@ -142,6 +145,10 @@ export function ChannelTagsControl({ onCreate: (name: string) => void; /** Manager only: clicking a chip filters the feed by that tag. Omit on the detail page. */ onChipClick?: (tagId: number, name: string) => void; + /** Cap the chip row at a fixed 12rem so it can't widen a `table-layout:auto` column and reflow the + * table (the manager). Set false where there is free width (the channel detail header), so tags + * fit inline instead of needlessly collapsing into "+N". */ + bounded?: boolean; }) { const { t } = useTranslation(); const attached = userTags.filter((tg) => attachedIds.includes(tg.id)); @@ -174,6 +181,16 @@ export function ChannelTagsControl({ ? Math.max(0, pill.offsetLeft - (lastChip.offsetLeft + lastChip.offsetWidth)) : 0; const avail = row.clientWidth; + // Whole-set fast path: if the LAST chip's right edge already fits, every chip fits and no "+N" + // pill is needed. The reservation loop below reserves the pill's width for every chip except + // the last and breaks at the first overflow, so on its own it never reaches the only "does the + // whole set fit?" test — it would collapse chips that would have fitted (e.g. a short trailing + // chip narrower than the pill), leaving "+N" beside empty space. + const lastRight = lastChip ? lastChip.offsetLeft + lastChip.offsetWidth : 0; + if (lastRight <= avail) { + setVisible(chips.length); + return; + } let count = chips.length; for (let i = 0; i < chips.length; i++) { const chip = chips[i]; @@ -202,8 +219,9 @@ export function ChannelTagsControl({ onOpenChange: setEditorOpen, mode: "menu", placement: "bottom-start", - // The menu contains a free-text "create tag" input — type-to-focus would fight the text field. - typeahead: false, + // The menu contains a free-text "create tag" input — keep both typed chars and pointer-hover out + // of the roving list so neither steals the field's caret. + hostsInput: true, }); const [overflowOpen, setOverflowOpen] = useState(false); @@ -223,11 +241,14 @@ export function ChannelTagsControl({ "shrink-0 text-[10px] px-1.5 py-0.5 rounded-full bg-card text-muted border border-border"; return ( - // FIXED width so the cell's footprint is constant under `table-layout:auto`: a content-driven cell - // re-solves EVERY column's width as chips are added, visibly shifting the whole table. A fixed - // width keeps the table stable and centered; the chip row fits what it can and collapses the rest - // into "+N". (max-w-full lets a narrower card shrink it.) -
+ // When `bounded`, a FIXED width keeps the cell's footprint constant under `table-layout:auto`: a + // content-driven cell re-solves EVERY column's width as chips are added, visibly shifting the whole + // table. The fixed width keeps the table stable and centered; the chip row fits what it can and + // collapses the rest into "+N". (max-w-full lets a narrower card shrink it.) Unbounded, the row + // takes its container's width, so a wide header shows the tags inline instead of collapsing them. +
{/* Hidden measurer: all chips + a sample pill, laid out unbounded to read their true widths. */}
qc.invalidateQueries({ queryKey: qk.channel(id) }), onError: () => qc.invalidateQueries({ queryKey: qk.channels() }), }); const syncSubs = useMutation({ diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index b8c7d1dc..ed33fc3f 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -867,8 +867,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 / download are hidden in true - fullscreen — their popovers portal to , outside the fullscreen element. */} + 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. */} {(maximized || isFullscreen) && (
e.stopPropagation()} diff --git a/frontend/src/components/Tooltip.tsx b/frontend/src/components/Tooltip.tsx index 2695d474..2e6831d8 100644 --- a/frontend/src/components/Tooltip.tsx +++ b/frontend/src/components/Tooltip.tsx @@ -25,6 +25,9 @@ export default function Tooltip({ mode: "hover", placement: side, offset: 8, + // Show/hide on contact, as the pre-platform tooltip did — the platform's 120/140 ms hover-intent + // default suits interactive cards but makes a fast sweep across an icon row surface no hints. + hoverDelay: { open: 0, close: 0 }, }); if (!enabled || !hint) return <>{children}; diff --git a/frontend/src/lib/plexDetailUi.tsx b/frontend/src/lib/plexDetailUi.tsx index 6c78ad2d..838e5c06 100644 --- a/frontend/src/lib/plexDetailUi.tsx +++ b/frontend/src/lib/plexDetailUi.tsx @@ -1,10 +1,10 @@ -import { useLayoutEffect, useState, type ReactNode } from "react"; +import { useLayoutEffect, useRef, useState, type ReactNode } from "react"; import { qk } from "./queryKeys"; import { useTranslation } from "react-i18next"; import { useQueryClient } from "@tanstack/react-query"; import { SlidersHorizontal } from "lucide-react"; +import { useDismiss } from "./useDismiss"; import { api } from "./api"; -import { PopoverItem, PopoverSurface, useAnchoredPopover } from "../components/AnchoredPopover"; // Shared "detail page" UI reused by the movie info page (PlexInfo) and the series show/season pages, // so they look consistent (the standing glassy/HTPC brief): the faint fixed art backdrop, the two @@ -131,41 +131,51 @@ export function DetailCustomizeMenu({ }) { const { t } = useTranslation(); const [open, setOpen] = useState(false); - const pop = useAnchoredPopover({ open, onOpenChange: setOpen, placement: "bottom-end" }); + const btnRef = useRef(null); + const menuRef = useRef(null); + useDismiss(open, () => setOpen(false), [menuRef, btnRef]); + // Deliberately NOT on the R8 popover platform. In the player's OVERLAY variant this menu renders + // inside PlexPlayer's `transform: scale(1.25)` windowed subtree, and the platform portals its panel + // out to — which would paint it at 1× while the surrounding player UI is at 1.25×. A plain + // `absolute` child stays in the scaled subtree and matches, in both this variant and the unscaled + // detail-page variant. Same reason the player's own gear/tracks menus stay hand-rolled; migrating + // any of them needs the platform to portal INTO a caller-named (scaled) container. return ( -
+
- - - {hasCast && ( - - )} - {hasRelated && onToggleRelated && ( - - )} - {extra} - + {open && ( +
+ + {hasCast && ( + + )} + {hasRelated && onToggleRelated && ( + + )} + {extra} +
+ )}
); } -/** One switch row inside `DetailCustomizeMenu` — a `PopoverItem`, so the rows callers pass through - * `extra` join the same roving-focus list as the built-in ones. */ +/** One switch row inside `DetailCustomizeMenu`. Independent on/off toggles (all can be on at once), + * so a plain button, not a menuitemradio. */ export function PrefToggle({ label, on, @@ -176,12 +186,9 @@ export function PrefToggle({ onClick: () => void; }) { return ( - {label} - + ); }