fix(keys): epic-end review round 1 — Back, menus and dialogs keep the focus where it belongs
Deep-review of E11 (19 raised, 14 confirmed; 2 refuted, 3 latent test-hardening notes dropped). - Back from a sub-view (a Plex show page, a Messages thread) restores the item it was opened from even when focus was still inside the view: the first attempt now waits one task, past the sub-view's own popstate listener (measured in Chrome: it left focus on <body>). - The restore listener is installed at load, so the first Back after a reload finds it; a push cancels a restore still waiting for the last landing's lists. - Navigating over an overlay asks it again, a render apart, while its entry stays: the Plex player peels one panel per Back step, so Shift+W over info + stats closed one panel and dropped the navigation. - j/k: a native <button> item keeps its own Tab stop; landing twice on the same item no longer leaks its tabindex; after the focused item is removed (a hidden card) the next j goes on from its slot when that slot is on screen. - An abandoned `g` no longer swallows a modifier chord (g, then Mod+K opened the browser search). - A hover card above a dialog no longer switches its Tab trap off (player, Modal, wizard, Lightbox): new layerStack `anyBlockingLayerAbove`. - "Manage views…" closes its menu before opening the dialog, through a shared `closeThenAct` that ActionMenu now uses too. - Copy: the EN welcome says "Feed"; the EN unreachable reason no longer claims a change; stale comments about PlayerModal's scroll lock and dialog contract.
This commit is contained in:
@@ -146,7 +146,7 @@ export default function Feed({
|
||||
(video: Video, startAt: number | null = null) => setActiveVideo({ video, startAt }),
|
||||
[],
|
||||
);
|
||||
// Stable identity, like Playlists' `closePlayer`: PlayerModal's keydown/scroll-lock effect
|
||||
// Stable identity, like Playlists' `closePlayer`: PlayerModal's keydown effect
|
||||
// depends on `onClose`, so a fresh closure every Feed render re-subscribed those listeners,
|
||||
// re-ran focusModal() under the open player, dropped its in-flight timers and flushed the
|
||||
// debounced volume write on each pass.
|
||||
|
||||
@@ -125,8 +125,9 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: ()
|
||||
useDialogChrome(panelRef, rootRef);
|
||||
useEffect(() => {
|
||||
const onKey = (e: KeyboardEvent) => {
|
||||
// Only while nothing is above us: an error dialog opened from inside the wizard owns the Tab.
|
||||
if (e.key !== "Tab" || !layer.isTopmost()) return;
|
||||
// Only while nothing that owns the keyboard is above us: an error dialog opened from inside the
|
||||
// wizard owns the Tab, a hover card under the pointer does not.
|
||||
if (e.key !== "Tab" || layer.blockingAbove()) return;
|
||||
const panel = panelRef.current;
|
||||
// The SAME three arguments `ui/Modal` passes, for the same two reasons — and this call had
|
||||
// neither of them (round 6, on the commit that extracted the helper so the two shells could
|
||||
|
||||
@@ -27,7 +27,13 @@ import {
|
||||
import Avatar from "./ui/Avatar";
|
||||
import AddToPlaylist from "./AddToPlaylist";
|
||||
import { PopoverSurface, useAnchoredPopover } from "./ui/AnchoredPopover";
|
||||
import { pushLayer, removeLayer, anyLayerAbove, escapeHandled } from "../lib/layerStack";
|
||||
import {
|
||||
pushLayer,
|
||||
removeLayer,
|
||||
anyBlockingLayerAbove,
|
||||
anyLayerAbove,
|
||||
escapeHandled,
|
||||
} from "../lib/layerStack";
|
||||
import { trapTab, useDialogChrome } from "./ui/dialogFocus";
|
||||
import DownloadButton from "./DownloadButton";
|
||||
import { api, type Video, type VideoStatus } from "../lib/api";
|
||||
@@ -399,7 +405,7 @@ export default function PlayerModal({
|
||||
|
||||
useKeyScope("ytPlayer");
|
||||
|
||||
// Keyboard shortcuts + background scroll lock. The chords live in the shared keymap (`ytPlayer`
|
||||
// Keyboard shortcuts. The chords live in the shared keymap (`ytPlayer`
|
||||
// scope) so the help sheet lists them; only the behaviour behind each id is here.
|
||||
// These fire only while focus is on our page, not inside the cross-origin player iframe — so
|
||||
// we focus the modal card on open and again on player-ready (autoplay can grab focus).
|
||||
@@ -424,10 +430,11 @@ export default function PlayerModal({
|
||||
nudgeSeek(secs);
|
||||
};
|
||||
const onKey = (e: KeyboardEvent) => {
|
||||
// The shared dialog Tab trap — only while nothing is layered above the player: a Modal opened
|
||||
// over it (the download dialog) traps for itself.
|
||||
// The shared dialog Tab trap — only while nothing that owns the keyboard is layered above the
|
||||
// player: a Modal opened over it (the download dialog) traps for itself. The title's hover card
|
||||
// does not, so it must not switch the trap off.
|
||||
if (e.key === "Tab") {
|
||||
if (layerIdRef.current != null && anyLayerAbove(layerIdRef.current)) return;
|
||||
if (layerIdRef.current != null && anyBlockingLayerAbove(layerIdRef.current)) return;
|
||||
if (dialogRef.current) trapTab(e, dialogRef.current, cardRef.current);
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -689,7 +689,7 @@ export default function Playlists() {
|
||||
.catch((e: unknown) => notifyActionError(e, t("errors.writeFailed.videoState")));
|
||||
}
|
||||
|
||||
// Stable identity: PlayerModal's keydown/scroll-lock effect depends on it, so a fresh closure
|
||||
// Stable identity: PlayerModal's keydown effect depends on it, so a fresh closure
|
||||
// every render re-subscribed those listeners and re-ran focusModal() under the open player.
|
||||
const closePlayer = useCallback(() => setPlaying(null), []);
|
||||
|
||||
|
||||
@@ -0,0 +1,51 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// "Manage views…" opens a dialog from a menu row, so it must close the menu FIRST and open the dialog
|
||||
// on a fresh task (`closeThenAct`): the menu's focus return, queued as it unmounts, otherwise takes the
|
||||
// dialog's focus back to the trigger behind its scrim — measured in Chrome on ActionMenu (E11 S2), and
|
||||
// this row was the sibling that fix missed (deep-review E11 round 1). jsdom does not run that focus
|
||||
// race, so the ORDER is what is pinned: the menu is gone before the dialog is asked to open.
|
||||
import { beforeAll, describe, expect, it, vi } from "vitest";
|
||||
import { fireEvent, render, screen } from "@testing-library/react";
|
||||
import { FloatingTree } from "@floating-ui/react";
|
||||
|
||||
const openModal = vi.fn();
|
||||
vi.mock("react-i18next", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("react-i18next")>();
|
||||
return { ...actual, useTranslation: () => ({ t: (key: string) => key }) };
|
||||
});
|
||||
vi.mock("./SavedViewsProvider", () => ({
|
||||
useSavedViews: () => ({ views: [], activeViewId: null, apply: vi.fn(), openModal }),
|
||||
}));
|
||||
|
||||
import SavedViewsPopover from "./SavedViewsPopover";
|
||||
|
||||
beforeAll(() => {
|
||||
class Stub {
|
||||
observe() {}
|
||||
unobserve() {}
|
||||
disconnect() {}
|
||||
takeRecords() {
|
||||
return [];
|
||||
}
|
||||
}
|
||||
globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver;
|
||||
globalThis.IntersectionObserver ??= Stub as unknown as typeof IntersectionObserver;
|
||||
});
|
||||
|
||||
describe("SavedViewsPopover · Manage views…", () => {
|
||||
it("closes the menu first, then opens the dialog once", async () => {
|
||||
render(
|
||||
<FloatingTree>
|
||||
<SavedViewsPopover trigger="views" ariaLabel="Saved views" />
|
||||
</FloatingTree>,
|
||||
);
|
||||
fireEvent.click(screen.getByRole("button", { name: "Saved views" }));
|
||||
// floating-ui never positions in jsdom, so the surface stays visibility:hidden and the row has no
|
||||
// accessible name to query by: find it by its text.
|
||||
fireEvent.click(screen.getByText("views.manage"));
|
||||
expect(screen.queryByRole("menuitem", { hidden: true })).toBeNull();
|
||||
expect(openModal).not.toHaveBeenCalled();
|
||||
await vi.waitFor(() => expect(openModal).toHaveBeenCalledTimes(1));
|
||||
});
|
||||
});
|
||||
@@ -1,7 +1,12 @@
|
||||
import { useState, type ReactNode } from "react";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { Bookmark, Check, Settings2 } from "lucide-react";
|
||||
import { PopoverItem, PopoverSurface, useAnchoredPopover } from "./ui/AnchoredPopover";
|
||||
import {
|
||||
PopoverItem,
|
||||
PopoverSurface,
|
||||
closeThenAct,
|
||||
useAnchoredPopover,
|
||||
} from "./ui/AnchoredPopover";
|
||||
import { useSavedViews } from "./SavedViewsProvider";
|
||||
|
||||
// The saved-views quick-picker. The filter-panel header's icon (and the toolbar's view chip) use it as
|
||||
@@ -67,10 +72,8 @@ export default function SavedViewsPopover({
|
||||
<div className="mt-1 border-t border-border pt-1">
|
||||
<PopoverItem
|
||||
role="menuitem"
|
||||
onSelect={() => {
|
||||
openModal();
|
||||
setOpen(false);
|
||||
}}
|
||||
// Opens a dialog: close first, or the menu's focus return steals its focus (closeThenAct).
|
||||
onSelect={() => closeThenAct(() => setOpen(false), openModal)}
|
||||
label={t("views.manage")}
|
||||
className="flex w-full items-center gap-2 rounded-md px-2 py-1.5 text-left text-sm text-muted transition hover:text-fg"
|
||||
>
|
||||
|
||||
@@ -30,7 +30,7 @@ import { setLanguage, type LangCode } from "../i18n";
|
||||
import LanguageSwitcher from "./LanguageSwitcher";
|
||||
import { Button } from "./ui/Button";
|
||||
import { trapTab, useDialogChrome } from "./ui/dialogFocus";
|
||||
import { isTopmost, pushLayer, removeLayer } from "../lib/layerStack";
|
||||
import { anyBlockingLayerAbove, isTopmost, pushLayer, removeLayer } from "../lib/layerStack";
|
||||
import { inputCls } from "./ui/form";
|
||||
import { entryHash, entrySearch } from "../lib/entryUrl";
|
||||
import type { VerifyEntry } from "../lib/useVerifyEntry";
|
||||
@@ -275,11 +275,9 @@ function Lightbox({ src, label, onClose }: { src: string; label: string; onClose
|
||||
// and the wizard get theirs; the body-scroll lock moved with it, and round 7 added the layer
|
||||
// membership below.
|
||||
//
|
||||
// It is the THIRD of four to get the contract, not the last: `PlayerModal` is also
|
||||
// `role="dialog" aria-modal="true"` and still hand-rolls its own scroll lock with no accessible
|
||||
// name, no Tab trap and no focus restore. Saying "the fourth shell" here read as though the sweep
|
||||
// were finished, which is the false-completeness that let a demo refusal sit unconverted for a
|
||||
// whole round. It is on ROADMAP.md instead (round 7).
|
||||
// It was the third of four `aria-modal` shells to get the contract; `PlayerModal`, the fourth,
|
||||
// joined in E11 S2, so all four (`ui/Modal`, the wizard, this, the player) now share
|
||||
// `useDialogChrome` and its one counted scroll lock.
|
||||
const panelRef = useRef<HTMLDivElement>(null);
|
||||
useDialogChrome(panelRef);
|
||||
// Keep a stable handler across renders so the stack id is assigned once per mount (as in ui/Modal).
|
||||
@@ -295,14 +293,14 @@ function Lightbox({ src, label, onClose }: { src: string; label: string; onClose
|
||||
// landing page grows (deep-review round 7).
|
||||
const id = pushLayer();
|
||||
const onKey = (e: KeyboardEvent) => {
|
||||
if (!isTopmost(id)) return;
|
||||
if (e.key === "Escape") {
|
||||
if (e.key === "Escape" && isTopmost(id)) {
|
||||
e.stopPropagation();
|
||||
onCloseRef.current();
|
||||
return;
|
||||
}
|
||||
// The trap defers only to a layer that owns the keyboard, not to a hover card under the pointer.
|
||||
const panel = panelRef.current;
|
||||
if (e.key === "Tab" && panel) trapTab(e, panel, panel);
|
||||
if (e.key === "Tab" && panel && !anyBlockingLayerAbove(id)) trapTab(e, panel, panel);
|
||||
};
|
||||
window.addEventListener("keydown", onKey);
|
||||
return () => {
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { Fragment, useState, type ReactNode } from "react";
|
||||
import { ChevronRight, MoreHorizontal } from "lucide-react";
|
||||
import { PopoverItem, PopoverSurface, useAnchoredPopover } from "./AnchoredPopover";
|
||||
import { PopoverItem, PopoverSurface, closeThenAct, useAnchoredPopover } from "./AnchoredPopover";
|
||||
|
||||
// A reusable "⋯ more actions" overflow menu: a compact trigger that opens a list of one-shot actions.
|
||||
// Built on the R8 AnchoredPopover platform (arrows/typeahead/Escape/anchor-flip), same as ViewSwitcher
|
||||
@@ -82,17 +82,8 @@ export function MenuItemList({
|
||||
disabled={it.disabled}
|
||||
label={it.label}
|
||||
title={it.title}
|
||||
onSelect={() => {
|
||||
// Close FIRST, act once the menu has handed the focus back. floating-ui returns the
|
||||
// focus to the trigger from a microtask queued as the menu unmounts, so an action run
|
||||
// in the same batch had its focus taken back by it: measured in Chrome (E11 S2), a
|
||||
// confirm dialog opened from the rail's menu landed its focus on Cancel and lost it
|
||||
// 7 ms later to the row BEHIND its scrim, where Enter activated the row. A fresh task
|
||||
// runs after that microtask — and a dialog opened there captures the trigger, not a
|
||||
// dying menu item, as the element to restore the focus to.
|
||||
onClose();
|
||||
setTimeout(it.onSelect, 0);
|
||||
}}
|
||||
// Any row may open a dialog (a confirm), so every row closes first — see closeThenAct.
|
||||
onSelect={() => closeThenAct(onClose, it.onSelect)}
|
||||
className={`${ROW}${it.danger ? " text-danger" : ""}`}
|
||||
>
|
||||
{it.icon && (
|
||||
|
||||
@@ -393,6 +393,18 @@ const HIGHLIGHT_SELECTED =
|
||||
const HIGHLIGHT_SELECTED_RING_ONLY =
|
||||
"data-active:ring-2 data-active:ring-inset data-active:ring-accent/70";
|
||||
|
||||
/** A menu row whose action opens a dialog or moves the focus: close the menu FIRST and act once it
|
||||
* has handed the focus back. floating-ui returns the focus to the trigger from a microtask queued as
|
||||
* the menu unmounts, so an action run in the same batch had its focus taken back by it: measured in
|
||||
* Chrome (E11 S2), a confirm dialog opened from the rail's menu landed its focus on Cancel and lost it
|
||||
* 7 ms later to the row BEHIND its scrim, where Enter activated the row. A fresh task runs after that
|
||||
* microtask — and a dialog opened there captures the trigger, not a dying menu item, as the element
|
||||
* to restore the focus to. Every such row goes through here (`ActionMenu`, "Manage views…"). */
|
||||
export function closeThenAct(close: () => void, act: () => void): void {
|
||||
close();
|
||||
setTimeout(act, 0);
|
||||
}
|
||||
|
||||
export function PopoverItem({
|
||||
children,
|
||||
onSelect,
|
||||
|
||||
@@ -22,6 +22,7 @@ vi.mock("../../lib/history", () => ({ useBackToClose: () => {} }));
|
||||
|
||||
import Modal from "./Modal";
|
||||
import { clearAll, notify } from "../../lib/notifications";
|
||||
import { pushLayer, removeLayer } from "../../lib/layerStack";
|
||||
|
||||
// Fire a keydown on window (where Modal installs its handler), inside act.
|
||||
const press = (key: string, shiftKey = false) =>
|
||||
@@ -93,6 +94,24 @@ describe("Modal focus trap (offsetParent stubbed 'visible')", () => {
|
||||
expect(document.activeElement).toBe(screen.getByText("beta"));
|
||||
});
|
||||
|
||||
// A tooltip or hover card open under a resting pointer is a (passive) layer above the dialog. It
|
||||
// takes Escape first, but deferring the Tab trap to it let Shift+Tab walk out behind the scrim.
|
||||
it("keeps trapping while only a hover card is open above it", () => {
|
||||
render(
|
||||
<Modal title="t" onClose={() => {}}>
|
||||
<Body />
|
||||
</Modal>,
|
||||
);
|
||||
const hover = pushLayer({ passive: true });
|
||||
try {
|
||||
screen.getByText("beta").focus(); // last
|
||||
press("Tab");
|
||||
expect(document.activeElement).toBe(closeBtn());
|
||||
} finally {
|
||||
removeLayer(hover);
|
||||
}
|
||||
});
|
||||
|
||||
it("Tab reaches a toast shown inside the dialog, and wraps from it", () => {
|
||||
// The dialog's toast host sits beside the panel; trapped to the panel, a toast's Undo or dismiss
|
||||
// could not be reached from the keyboard at all.
|
||||
|
||||
@@ -3,7 +3,7 @@ import { createPortal } from "react-dom";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { X } from "lucide-react";
|
||||
import { useBackToClose } from "../../lib/history";
|
||||
import { pushLayer, removeLayer, isTopmost } from "../../lib/layerStack";
|
||||
import { anyBlockingLayerAbove, pushLayer, removeLayer, isTopmost } from "../../lib/layerStack";
|
||||
import { useFullscreenPortalHost } from "../../lib/fullscreen";
|
||||
import { trapTab, useDialogChrome } from "./dialogFocus";
|
||||
import { Button } from "./Button";
|
||||
@@ -61,15 +61,15 @@ export default function Modal({
|
||||
useEffect(() => {
|
||||
const id = pushLayer();
|
||||
const onKey = (e: KeyboardEvent) => {
|
||||
if (!isTopmost(id)) return;
|
||||
if (e.key === "Escape") {
|
||||
if (e.key === "Escape" && isTopmost(id)) {
|
||||
e.stopPropagation();
|
||||
onCloseRef.current();
|
||||
return;
|
||||
}
|
||||
// Focus trap: keep Tab / Shift+Tab cycling inside the dialog instead of reaching the inert
|
||||
// page behind the backdrop. Only the topmost modal traps (a nested one defers to its child).
|
||||
if (e.key === "Tab") {
|
||||
// page behind the backdrop. A nested modal or a click-opened menu above traps for itself; a hover
|
||||
// card open under the pointer (a passive layer) does not, so it must not switch the trap off.
|
||||
if (e.key === "Tab" && !anyBlockingLayerAbove(id)) {
|
||||
const panel = panelRef.current;
|
||||
if (!panel) return;
|
||||
// The whole dialog, not just the panel: its toast host sits beside the panel (see ui/Toaster),
|
||||
|
||||
Reference in New Issue
Block a user