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.
Round 2 reviewed round 1's own fix diff, which is where a fix round does
its damage. It found 15 issues in 792 lines — a much higher density than
round 1 found in the whole epic — and most were regressions round 1
introduced. So this follows the rule rather than fighting it: revert the
fixes whose mechanism was wrong, narrow the ones whose guard was too
broad, and stop reaching for a third variant.
REVERTED, because the mechanism does not work:
- The `inert` menu item. Round 1 swapped a native `disabled` for
`aria-disabled` so a row's "why is this off" tooltip could be reached.
floating-ui's list navigation skips `aria-disabled="true"` exactly as it
skips `disabled` (isListIndexDisabled) — and AnchoredPopover's own
comment, written before this branch, already said so. The swap fixed
nothing and made the mouse worse: without native `disabled` the row took
pointer events, the full active highlight and DOM focus, then no-opped
on click. The underlying finding is logged where the prop was, unfixed
and honest: the reason has to go somewhere a disabled row is not.
- `useContextMenu` owning the focus restore. Turning floating-ui's
returnFocus off platform-wide traded one bug for two: Playlists spreads
triggerProps on a bare <tr>, so restoring to it is a spec no-op and
Escape stranded focus on <body>; and with dismissOnScroll now on, the
restore's plain focus() scrolled the row back into view, undoing the
scroll that closed the menu. Patching that needs three more guards on
one fix. The original bug — the rail's rename field dying on arrival —
is fixed at the field instead: it focuses from an effect, which is
scheduled after the microtask that was blurring it.
NARROWED, because the fix was right and the guard was not:
- PlexPlayer's Backspace deferred to `anyLayerAbove`, but the player's own
gear and tracks menus register layers via useDismiss — so Backspace
stopped closing them and did nothing at all. Only the cheat sheet and
the palette own that key, so it names them.
- `historyDepth = 0` also desynced the counter from overlays the
navigation does NOT unmount (About and ReleaseNotes live outside the
page column), leaving one Back to both navigate and close them. The
entries are marked buried and consumed silently by reconcile instead.
- The move's error toast fired for every status, so a transport fault
surfaced twice on top of req's own dialog. Scoped to 403/404.
- The palette's moved-pointer guard could not judge the FIRST move of a
session and went stale across a close. `e.movementX/Y` asks the browser
directly and needs no state.
- `useKeyScope("contextMenu")` runs once per hook, and the Playlists page
mounts two — so the cheat sheet rendered the chords twice under a
duplicated React key. The scope stack dedupes.
- The `enabled:hover:` sweep missed the half that lives in CSS, so a
disabled glass button still lifted. `.glass-hover:not(:disabled):hover`.
- ShareDialog's migrated buttons passed a colour className that the
variant's own colour class outranks under clsx, so Revoke hovered
neutral and a password-protected link looked unprotected. The colour
goes on the icon, as the Copy button beside them already did.
- `matchesShortcut` took an untyped id and ids are only unique within a
scope, so `back` would have answered for the wrong player. Keyed on
scope + id, both literal.
And the systemic one: round 2 reverted eight of round 1's behaviour fixes
as a single mutation and all 763 tests still passed. Round 1 tested the
three fixes that were easy to test and none of the rest. So the guards
with the widest blast radius are pinned now, each mutation-checked to
fail for its own reason: the global keymap's repeat/allowInInput/disabled
handling (a new dom test), the cheat sheet bound to the real
useContextMenu rather than a scope stub, the scope dedupe, the history
burial, and the Button accessible-name union — whose @ts-expect-error
fixtures turn a widened type into a red typecheck lane.
The chipLane contract test that round 1 rewrote to count per lane counted
the import line as a lane, giving every file one free mirror — so the
exact hole it was rewritten to close was still open. Mutation-checked
both ways this time.
S1c, the last part of S1. The contract now lives in components/ui/ and
in one CSS recipe instead of in ~160 call sites:
- Minimum target 2.75rem, on coarse pointers only (.touch-target). A
32px icon button is fine with a mouse -- precise pointer, hover to
confirm the aim -- and not fine with a thumb, so it grows exactly where
there is no precise pointer, which on this app's real second device is
the tablet. It grows the real box rather than painting an invisible
::after hit area, because two expanded hit areas on icon buttons 4px
apart would overlap and the overlap fires the WRONG button. `link` is
excluded: it is text inside a sentence.
- 2.75rem rather than 44px so the target obeys the text-size slider like
everything else; verified that the type/geometry ladder was already
rem-derived off --font-scale, so nothing else was needed there.
- The ladder is documented as a closed set in Button.tsx, with the site
counts it was derived from, and ui/README.md states the contract.
Measured in the browser: fine pointer 36x36 and 34px-tall buttons
(unchanged), coarse pointer the same buttons at 44x44 and 44px tall.
1024x768 renders identically. 3x tsc, eslint, knip, prettier and 690
vitest green.
Counting the un-migrated call sites showed the primitive and the app had
drifted into two different looks: the majority of hand-rolled buttons
hover to accent (outline) and fill with card (ghost), while Button
hovered to fg and filled with surface. That split is the inconsistency
this epic exists to remove, so the primitive follows the majority rather
than the other way round -- 16 existing call sites shift slightly, ~90
hand-rolled ones stop being special.
Adds the variants the call sites prove are missing (outline-accent,
outline-danger, outline-warn, link), an icon-sm size for the p-1.5 icon
family, and a size=none so a link owns no button padding; renames
danger-outline to danger-soft so it cannot be confused with
outline-danger (red at rest vs red on hover). Two new tests lock the
link-size default and the rule that a variant never adds a second
font-weight utility -- without tailwind-merge, two font-* utilities are
decided by stylesheet order, not class order, so that is a real hazard.
Migrates AdminUsers, AuditLog, Channels, TagManager, Playlists,
PlaylistsRail and SettingsPanel (Button call sites 37 -> 52). Left alone
deliberately: conditional-paint sites, video-overlay white buttons, drag
handles, chip removers and card-click targets -- the families Button's
own docstring excludes.
3x tsc, eslint, knip, prettier and 689 vitest green; visual UAT on Users,
Access requests and Playlists at 1280x900, no console errors.
- Button: `loading` always disables (|| not ??) — stops a double-submit
when a call-site passes an explicit boolean `disabled` (VideoEditor).
- playlists: track un-ingestable-but-live YouTube ids in the sync baseline
so a linked playlist containing a private/deleted video converges instead
of re-classifying it as a pending pull forever; re-attempt ingestion each
sync so a restored (private->public) video heals, and prune ids that
leave YouTube.
- SettingsPanel: download-auth section renders an honest error/loading
state (StateMessage + retry) instead of a bare empty section on a failed
credentials fetch; HU+EN loadError string.
- tests: reorder-order, partial-failure baseline hold, un-ingestable
converge/heal/prune, Button loading-disables, download-auth error/retry.
Introduce frontend/src/components/ui/Button.tsx — the one action-button primitive
(variants primary|danger|outline|ghost|glass|danger-outline; sizes xs|sm|md|lg|icon;
icon/iconOnly/label/loading/fullWidth/forwardRef). No focus-visible (already centralized
in index.css @layer base). Migrate the cleanest proof sites off btnCls, and flip the
channel Subscribe button to the outline variant so it matches Full history (the user's ask).
peter