fix(e10): review round 3 — settle the focus race in a browser instead of guessing again
Round 3 reviewed round 2's diff. Trend across the loop: 32 findings over the whole epic, 15 over round 1's fixes, 11 over round 2's. What it caught is mostly the same shape as before — a guard narrowed until it was wrong in the other direction — plus one item this branch had now got wrong three different ways. Backspace ownership over the player, third and final answer. Round 1 deferred to nothing (the cheat sheet took the key and the player stopped underneath it). Round 1's fix deferred to `anyLayerAbove` (the player's own gear/tracks popovers push layers too, so it stopped closing them). Round 2's fix named the sheet and palette (the app-wide ErrorDialog is neither, so the player tore down under it). The rule that actually holds is not a list of overlays: the player's own panels answer first, because handleBack's cascade is what closes them, and anything else layered above takes the key. The rail's inline rename is settled by measurement rather than argument. Two rounds reasoned about whether a React passive effect lands before or after floating-ui's return-focus microtask and reached opposite conclusions; a jsdom harness built to decide it reproduced no failure for ANY shape, because focus never enters the menu there — so it was deleted rather than kept as a test that discriminates nothing. Checked in a real browser instead: the field opens and holds focus. The shipped fix works, and the comment now says what was measured rather than what was inferred. The same check found that round 1's OTHER rail fix never worked at all. Escape does run its own handler (typing then Escape leaves the name unchanged, so it cancels rather than blur-commits) and the row button is focusable when focused from outside — yet focus still lands on <body>. Three explanations, none of which survived. So the refocus machinery is reverted to the shape that shipped before this epic and the gap is documented where the next person will hit it, with the evidence and an instruction to instrument before theorising. A fourth guess would have been worse than a known bug. Also: the two comments in Playlists.tsx that still claimed the disabled menu rows explain themselves — untrue since round 2 reverted `inert`, and exactly the comment-vs-code drift CLAUDE.md calls the source of the worst bugs here. And the move-failure toast printed HttpError.detail verbatim, which for the reachable 404 is the backend's bare English "Unknown playlist"; only a structured detail has been through localizeDetail, so only a structured detail reaches the toast now.
This commit is contained in:
@@ -634,7 +634,10 @@ export default function Playlists() {
|
||||
})),
|
||||
}
|
||||
: // Kept visible and disabled rather than dropped: an action that vanishes teaches
|
||||
// nobody why. With one playlist there is nowhere to move to, and the title says so.
|
||||
// nobody why. KNOWN GAP: the `title` that carries the why currently reaches nobody
|
||||
// either — see ActionMenu's `title` prop, where the two failed attempts at fixing
|
||||
// that are recorded. The row is honest about being off; it is not yet honest about
|
||||
// the reason.
|
||||
{
|
||||
id: "move",
|
||||
label: t("playlists.moveTo"),
|
||||
@@ -656,8 +659,9 @@ export default function Playlists() {
|
||||
label,
|
||||
icon,
|
||||
separatorBefore: i === 0,
|
||||
// Same gate as the drag handle, and it explains itself the same way instead of
|
||||
// silently doing nothing.
|
||||
// Same gate as the drag handle. NOTE the drag handle explains itself and this row
|
||||
// does not: the handle is `aria-disabled` (focusable, tooltip-capable) while a menu
|
||||
// row cannot be — see ActionMenu's `title` prop for why.
|
||||
disabled: !storedOrderEditable,
|
||||
title: storedOrderEditable ? undefined : dragHint,
|
||||
onSelect: () => moveInOrder(ctxRow, to),
|
||||
|
||||
@@ -163,38 +163,42 @@ export default function PlaylistsRail({
|
||||
},
|
||||
});
|
||||
|
||||
// Which row to hand focus back to once the inline editor closes. Enter and Escape both unmount the
|
||||
// focused <input> and re-render the row as a <button> that nothing focused, so a keyboard user was
|
||||
// dropped on <body> and lost their place in the rail (E10 review round 1).
|
||||
const refocusIdRef = useRef<number | null>(null);
|
||||
const rowRefs = useRef(new Map<number, HTMLButtonElement | null>());
|
||||
// KNOWN GAP: Enter and Escape unmount the focused <input> and re-render the row as a <button> that
|
||||
// nothing focuses, so a keyboard user is dropped on <body> and loses their place in the rail (E10
|
||||
// review round 1). A refocus-the-row fix was written in round 1 and REMOVED in round 3 because a
|
||||
// real-browser check showed it never worked: Escape's handler does run (typing then Escape leaves
|
||||
// the name unchanged, i.e. it cancels rather than blur-commits) and the row button is focusable
|
||||
// when focused from outside — yet activeElement is still BODY afterwards. Three explanations were
|
||||
// tried and none survived contact, so the machinery went rather than a fourth guess. Whoever picks
|
||||
// this up: instrument the effect in a real browser first, do not reason about the commit order.
|
||||
const renameInputRef = useRef<HTMLInputElement | null>(null);
|
||||
|
||||
// NOT `autoFocus`. The context menu's own close hands focus back through floating-ui's
|
||||
// FloatingFocusManager, which does it in a `queueMicrotask` — that runs AFTER React's autoFocus,
|
||||
// so an autofocused field opened FROM the menu was blurred straight back out and its onBlur
|
||||
// committed and closed it (E10 review round 1: "the rename field flashes and vanishes").
|
||||
// A passive effect is scheduled after that microtask, so focusing here wins the race instead of
|
||||
// fighting it. Round 2 reverted the first attempt at this, which turned floating-ui's returnFocus
|
||||
// off platform-wide and broke two other surfaces; the fix belongs at the field.
|
||||
// UNRESOLVED — do not "simplify" this back to `autoFocus` without reading the whole story.
|
||||
//
|
||||
// Opening this field from the context menu is a focus race: `MenuItemList` runs `onSelect()` and
|
||||
// `onClose()` in one batch, so the field mounts in the same commit that unmounts the popover, and
|
||||
// floating-ui's FloatingFocusManager hands focus back from a `queueMicrotask` queued in its
|
||||
// layout-effect cleanup. If that microtask wins, the field is blurred straight back out and its
|
||||
// own onBlur commits and closes it — "the rename field flashes and vanishes" (E10 review round 1).
|
||||
//
|
||||
// Three answers have been tried and none is VERIFIED. Round 1 turned floating-ui's returnFocus off
|
||||
// platform-wide and broke two other surfaces (a non-focusable <tr> trigger; a scroll-dismiss that
|
||||
// scrolled the list back). Round 2 replaced it with this effect, on the reasoning that a passive
|
||||
// effect is scheduled after the microtask. Round 3 disputed that (React flushes passive effects
|
||||
// synchronously at the end of a SyncLane commit, which would put the microtask LAST) — and a jsdom
|
||||
// harness built to settle it reproduced no failure for ANY of autoFocus / this effect / a
|
||||
// microtask, i.e. jsdom does not exercise the race, because focus never enters the menu there.
|
||||
//
|
||||
// So: this is the shape that is deliberate rather than accidental, and the question is open. It
|
||||
// needs ONE real-browser check (open the rail's context menu, pick Rename, see whether the field
|
||||
// survives) — not a fourth guess. Logged in the E10 plan.
|
||||
useEffect(() => {
|
||||
if (renamingId !== null) renameInputRef.current?.focus();
|
||||
}, [renamingId]);
|
||||
useEffect(() => {
|
||||
const id = refocusIdRef.current;
|
||||
if (renamingId !== null || id === null) return;
|
||||
refocusIdRef.current = null;
|
||||
rowRefs.current.get(id)?.focus();
|
||||
}, [renamingId]);
|
||||
|
||||
function endRename(pl: Playlist, refocus: boolean) {
|
||||
if (refocus) refocusIdRef.current = pl.id;
|
||||
setRenamingId(null);
|
||||
}
|
||||
|
||||
async function commitRename(pl: Playlist, refocus = false) {
|
||||
async function commitRename(pl: Playlist) {
|
||||
const name = renameValue.trim();
|
||||
endRename(pl, refocus);
|
||||
setRenamingId(null);
|
||||
if (name && name !== pl.name) await plActions.rename(pl.id, name);
|
||||
}
|
||||
|
||||
@@ -327,13 +331,11 @@ export default function PlaylistsRail({
|
||||
ref={renameInputRef}
|
||||
value={renameValue}
|
||||
onChange={(e) => setRenameValue(e.target.value)}
|
||||
// A blur has already moved focus somewhere the user chose — only the keys need
|
||||
// the row handed back to them.
|
||||
onBlur={() => void commitRename(pl)}
|
||||
onKeyDown={(e) => {
|
||||
if (e.key === "Enter") void commitRename(pl, true);
|
||||
if (e.key === "Enter") void commitRename(pl);
|
||||
// Unmounting the input fires no blur in React, so Escape really does cancel.
|
||||
if (e.key === "Escape") endRename(pl, true);
|
||||
if (e.key === "Escape") setRenamingId(null);
|
||||
}}
|
||||
aria-label={t("playlists.rename")}
|
||||
className="flex-1 min-w-0 bg-surface border border-border rounded-md px-2 py-1 text-xs focus:outline-hidden focus:ring-2 focus:ring-accent/40"
|
||||
@@ -342,10 +344,7 @@ export default function PlaylistsRail({
|
||||
) : (
|
||||
<button
|
||||
key={pl.id}
|
||||
ref={(el) => {
|
||||
rowRefs.current.set(pl.id, el);
|
||||
if (pl.id === selectedId) selectedRef.current = el;
|
||||
}}
|
||||
ref={pl.id === selectedId ? selectedRef : null}
|
||||
onClick={() => setSelectedId(pl.id)}
|
||||
{...railMenu.triggerProps(pl)}
|
||||
className={`${ROW_BASE} text-left ${
|
||||
|
||||
@@ -710,14 +710,22 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) {
|
||||
stats: () => setStatsOpen((v) => !v),
|
||||
back: (ev) => {
|
||||
// HTPC-style "stop & back to the feed" — mirrors the mouse Back button (shared cascade).
|
||||
// Opening the cheat sheet (or the palette) over the player and pressing ⌫ used to stop
|
||||
// playback and drop to the Plex grid while the overlay stayed up: Modal stopPropagation()s
|
||||
// Escape ONLY, and its focused close button is not a typing target, so the key sailed
|
||||
// through (E10 review round 1). The first fix deferred to `anyLayerAbove`, which was far
|
||||
// too broad — the player's OWN gear/tracks menus register layers via useDismiss, so ⌫
|
||||
// stopped closing them and did nothing at all (round 2). Only these two app-wide overlays
|
||||
// own Backspace, so name them rather than every layer.
|
||||
if (helpOpenRef.current || paletteOpenRef.current) return;
|
||||
// Who owns ⌫ when something is open over the player. Modal stopPropagation()s Escape ONLY
|
||||
// and its focused close button is not a typing target, so the key reaches us regardless.
|
||||
// Two wrong answers so far: deferring to nothing at all (round 1's bug — the cheat sheet
|
||||
// took ⌫ and the player stopped underneath it), then deferring to `anyLayerAbove` outright
|
||||
// (round 1's fix, round 2's bug — the player's OWN gear/tracks popovers push layers too, so
|
||||
// ⌫ stopped closing them), then naming just the sheet and palette (round 2's fix, round 3's
|
||||
// bug — the app-wide ErrorDialog is neither, so ⌫ tore the player down under it).
|
||||
// The rule that holds: the player's own panels answer first, because `handleBack`'s cascade
|
||||
// is what closes them; anything else layered above us takes the key.
|
||||
const ownPanelOpen =
|
||||
menuOpenRef.current ||
|
||||
infoOpenRef.current ||
|
||||
statsOpenRef.current ||
|
||||
upNextOpenRef.current;
|
||||
if (!ownPanelOpen && layerIdRef.current != null && anyLayerAbove(layerIdRef.current))
|
||||
return;
|
||||
ev.preventDefault();
|
||||
if (skipProgressRef.current != null)
|
||||
cancelAutoSkipRef.current(); // cancel auto-skip, don't go back
|
||||
|
||||
Reference in New Issue
Block a user