fix(ui): dialogs, menus and busy buttons keep the keyboard focus where it belongs

Measured in Chrome before each change:

- A dialog opened from a menu lost its focus: the menu row ran its action
  and closed in one batch, and floating-ui's focus return (a microtask)
  moved the focus to the row BEHIND the confirm dialog's scrim 7 ms after
  it opened, where Enter activated the row. MenuItemList now closes first
  and runs the action a task later; the dialog also captures the row, not
  a dying menu item, as its restore target.
- A side panel opened as a flyout collapsed the moment a dialog opened
  from inside it, unmounting the row the dialog hands the focus back to.
  useHoverFocusWithin holds while the focus is inside an aria-modal dialog.
- ui/Button's loading state set native disabled; Chrome moved the focus of
  the pressed button to <body> and never gave it back. Loading is now
  aria-disabled with a click guard that also cancels implicit submission.
- Escape/Enter in the playlist rail's rename field dropped the focus on
  <body>. The row refocuses from its ref callback when it re-attaches (a
  post-commit focus sticks; the earlier attempt ran before the button
  existed).
- PlayerModal was the one aria-modal shell without the dialog contract:
  it is now named by its title, traps Tab, restores the focus to its
  opener and shares one counted body scroll lock with every dialog.
This commit is contained in:
2026-09-13 01:04:11 +02:00
parent 92c305bab9
commit b4aea7fa5b
14 changed files with 480 additions and 86 deletions
+28 -30
View File
@@ -167,35 +167,20 @@ export default function PlaylistsRail({
},
});
// 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.
// Enter and Escape unmount the focused <input> and re-render the row as a NEW <button>, so without
// this a keyboard user was dropped on <body> and lost their place in the rail (E10 review round 1).
// The row is focused from its ref callback, i.e. as that button attaches: measured in Chrome (E11
// S2), a focus issued after the commit sticks and nothing takes it back — the round-1 attempt that
// "never worked" focused before the replacement button existed. Set by Enter/Escape only: a
// blur-commit means the user put the focus somewhere else on purpose.
const refocusRowRef = useRef<number | null>(null);
const renameInputRef = useRef<HTMLInputElement | null>(null);
// 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.
// Opening this field from the context menu used to race the menu's own focus return (floating-ui
// hands the focus back to the trigger from a microtask queued as the popover unmounts, and the field
// mounted in that same commit). `MenuItemList` now runs the item's action on a fresh task AFTER that
// return, so the field simply mounts and is focused here; measured in Chrome (E11 S2), the field is
// focused and survives.
useEffect(() => {
if (renamingId !== null) renameInputRef.current?.focus();
}, [renamingId]);
@@ -338,9 +323,15 @@ export default function PlaylistsRail({
onChange={(e) => setRenameValue(e.target.value)}
onBlur={() => void commitRename(pl)}
onKeyDown={(e) => {
if (e.key === "Enter") void commitRename(pl);
if (e.key === "Enter") {
refocusRowRef.current = pl.id;
void commitRename(pl);
}
// Unmounting the input fires no blur in React, so Escape really does cancel.
if (e.key === "Escape") setRenamingId(null);
if (e.key === "Escape") {
refocusRowRef.current = pl.id;
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"
@@ -349,7 +340,14 @@ export default function PlaylistsRail({
) : (
<button
key={pl.id}
ref={pl.id === selectedId ? selectedRef : null}
ref={(el) => {
if (pl.id === selectedId) selectedRef.current = el;
// Back from the rename field by Enter/Escape — see refocusRowRef.
if (el && refocusRowRef.current === pl.id) {
refocusRowRef.current = null;
el.focus();
}
}}
onClick={() => setSelectedId(pl.id)}
{...railMenu.triggerProps(pl)}
className={`${ROW_BASE} text-left ${