peter/siftlode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(popover): resolve the R8 platform code-review findings

Browse Source 
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
  <body> 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 <body> 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.
This commit is contained in:
peter
2026-07-31 02:10:14 +02:00
parent 2727b59fa5
commit 2984aa3240
8 changed files with 195 additions and 98 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/components/AnchoredPopover.tsx
+94 -50
View File
@@ -2,6 +2,7 @@ import {
createContext,
useContext,
useEffect,
useLayoutEffect,
useRef,
useState,
type ButtonHTMLAttributes,
@@ -82,6 +83,10 @@ export interface AnchoredPopoverOptions {
/** Type-to-focus a matching item by label (menu/listbox only). Default on; turn OFF for menus that
* contain a free-text input, where character keys belong to the text field. */
typeahead?: boolean;
/** This menu hosts a free-text input (an inline "create" field). Turns off BOTH typeahead AND
* focus-item-on-hover, so neither typed characters nor a pointer passing over a list item steals
* focus/keystrokes from the field. Set it instead of `typeahead:false` for such menus. */
hostsInput?: boolean;
/** Index of the item that is currently CHOSEN, for a single-choice menu/listbox. Opening then
* lands the roving focus on it instead of the first item — the behaviour a "pick one of these
* modes" menu needs. Leave unset for action menus and multi-select toggle lists. */
@@ -142,11 +147,12 @@ export function useAnchoredPopover(opts: AnchoredPopoverOptions) {
handleClose: safePolygon(),
});
const focus = useFocus(context, { enabled: isHover });
// Escape for an interactive (non-hover) popover is dispatched through the shared `layerStack`
// (below) so it coordinates with Modals and players, not just other floating nodes — so turn
// floating-ui's own Escape off there. Outside-press stays with floating-ui (tree-aware). Hover
// tooltips keep floating-ui's Escape and are NOT layers (they mustn't make a player defer).
const dismiss = useDismiss(context, { escapeKey: isHover });
// Escape for EVERY mode (hover included) is dispatched through the shared `layerStack` below, so it
// is strictly topmost-only across popovers, Modals and players — so turn floating-ui's own Escape
// off entirely. (Leaving it on for hover made an open tooltip's document-level handler either
// stopPropagation the Escape away from a Modal/player underneath, or double-close both.) Outside-
// press stays with floating-ui (tree-aware).
const dismiss = useDismiss(context, { escapeKey: false });
const role = useRole(context, { role: opts.role ?? (isHover ? "tooltip" : "menu") });
const listNav = useListNavigation(context, {
enabled: isNav,
@@ -158,9 +164,13 @@ export function useAnchoredPopover(opts: AnchoredPopoverOptions) {
// Focus the first item on open for BOTH click and keyboard (the default 'auto' skips pointer-opens,
// which strands focus on the menu container — arrows then can't enter the roving list at all).
focusItemOnOpen: true,
// A menu that hosts a free-text input must NOT pull focus onto a list item when the pointer merely
// passes over it (the library default) — that blurs the input mid-typing and mis-routes the next
// Enter to the hovered item. Turned off together with typeahead for such menus.
focusItemOnHover: !opts.hostsInput,
});
const typeahead = useTypeahead(context, {
enabled: isNav && (opts.typeahead ?? true),
enabled: isNav && (opts.typeahead ?? true) && !opts.hostsInput,
listRef: labelsRef,
activeIndex,
onMatch: setActiveIndex,
@@ -168,20 +178,28 @@ export function useAnchoredPopover(opts: AnchoredPopoverOptions) {
const interactions = useInteractions([click, hover, focus, dismiss, role, listNav, typeahead]);
// Register the open interactive popover as a dismissable layer (topmost-only Escape, and so a
// player/modal underneath defers to it). Hover tooltips are excluded on purpose.
useLayer(opts.open && !isHover, () => 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<HTMLButtonElement>) => {
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 (
<button
ref={ref}
type="button"
// Roving tabindex: only the active item is in the Tab order; arrows move focus between them.
{...ariaProps}
{...handlers}
// These win over anything in `handlers` — the roving tabindex must not be a caller override,
// and disabled/className/data-active are the platform's to set.
tabIndex={active ? 0 : -1}
// Highlight hook: reliable across click- vs key-open (see the note above), unlike :focus-visible.
data-active={active ? "true" : undefined}
disabled={disabled}
className={className}
{...ariaProps}
{...rest}
{...handlers}
>
{children}
</button>
);
}
/** 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 <PopoverSurfaceInner {...props} />;
}
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 <body> 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<HTMLDivElement | null>(() =>
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<string, unknown>;
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 (
<FloatingNode id={nodeId}>
{/* `root` must be undefined (NOT null) to mean "default to <body>" — 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. */}
<FloatingPortal key={fsRoot ? "fullscreen" : "body"} root={fsRoot ?? undefined}>
<FloatingPortal root={container ?? undefined}>
{managed ? (
<FloatingFocusManager
context={context}
modal={false}
// On a roving list, useListNavigation owns the initial focus — so tell the focus manager
// NOT to also focus the panel itself (-1). Without this, a `selectedIndex` menu ends up
// with focus stranded on the container: that path focuses its item SYNCHRONOUSLY in a
// layout effect, and the manager's own initial focus then runs after it and wins. The
// no-selectedIndex path only escapes the same fate because it defers its focus to a
// microtask/rAF while it waits for the item list to populate.
initialFocus={initialFocus ?? (isNav ? -1 : undefined)}
// Only when a `selectedIndex` is set does useListNavigation focus its item SYNCHRONOUSLY
// (in a layout effect) and race the focus manager's own initial focus — so ONLY then do we
// suppress the manager's initial focus with -1. For a plain nav menu we let the manager
// focus normally: it lands on the first tabbable child, which is the first roving item, or
// — in an item-less menu (e.g. a first-run tag editor with only a create input) — the
// input, so such a menu still has a keyboard entry point instead of stranding focus.
initialFocus={initialFocus ?? (isNav && selectedIndex != null ? -1 : undefined)}
returnFocus
>
{positioned}