diff --git a/backend/app/routes/messages.py b/backend/app/routes/messages.py index d7502f75..eb7463f0 100644 --- a/backend/app/routes/messages.py +++ b/backend/app/routes/messages.py @@ -46,7 +46,7 @@ WELCOME = { "This is your private message inbox. Conversations with other members are end-to-end " "encrypted β€” only you and the person you're chatting with can read them, not even an " "admin. To start messaging someone, set up secure messaging with a passphrase first.\n\n" - "Enjoy your feed!" + "Enjoy your Feed!" ), "hu": ( "Üdv a Siftlode-on! πŸ‘‹\n\n" diff --git a/frontend/src/components/Feed.tsx b/frontend/src/components/Feed.tsx index e4a8d84c..7a113dc2 100644 --- a/frontend/src/components/Feed.tsx +++ b/frontend/src/components/Feed.tsx @@ -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. diff --git a/frontend/src/components/OnboardingWizard.tsx b/frontend/src/components/OnboardingWizard.tsx index ab2db044..339c9e15 100644 --- a/frontend/src/components/OnboardingWizard.tsx +++ b/frontend/src/components/OnboardingWizard.tsx @@ -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 diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 5de0b221..10f30288 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -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; } diff --git a/frontend/src/components/Playlists.tsx b/frontend/src/components/Playlists.tsx index 4d787d7a..8851d548 100644 --- a/frontend/src/components/Playlists.tsx +++ b/frontend/src/components/Playlists.tsx @@ -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), []); diff --git a/frontend/src/components/SavedViewsPopover.dom.test.tsx b/frontend/src/components/SavedViewsPopover.dom.test.tsx new file mode 100644 index 00000000..ba28aeaa --- /dev/null +++ b/frontend/src/components/SavedViewsPopover.dom.test.tsx @@ -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(); + 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( + + + , + ); + 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)); + }); +}); diff --git a/frontend/src/components/SavedViewsPopover.tsx b/frontend/src/components/SavedViewsPopover.tsx index 877fea69..b51e99bb 100644 --- a/frontend/src/components/SavedViewsPopover.tsx +++ b/frontend/src/components/SavedViewsPopover.tsx @@ -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({
{ - 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" > diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index 70d57391..8736e330 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -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(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 () => { diff --git a/frontend/src/components/ui/ActionMenu.tsx b/frontend/src/components/ui/ActionMenu.tsx index c311557f..d643ce33 100644 --- a/frontend/src/components/ui/ActionMenu.tsx +++ b/frontend/src/components/ui/ActionMenu.tsx @@ -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 && ( diff --git a/frontend/src/components/ui/AnchoredPopover.tsx b/frontend/src/components/ui/AnchoredPopover.tsx index 1b87842f..fbd6e08e 100644 --- a/frontend/src/components/ui/AnchoredPopover.tsx +++ b/frontend/src/components/ui/AnchoredPopover.tsx @@ -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, diff --git a/frontend/src/components/ui/Modal.dom.test.tsx b/frontend/src/components/ui/Modal.dom.test.tsx index 853c2891..26597680 100644 --- a/frontend/src/components/ui/Modal.dom.test.tsx +++ b/frontend/src/components/ui/Modal.dom.test.tsx @@ -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( + {}}> + + , + ); + 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. diff --git a/frontend/src/components/ui/Modal.tsx b/frontend/src/components/ui/Modal.tsx index 068baf70..2695089b 100644 --- a/frontend/src/components/ui/Modal.tsx +++ b/frontend/src/components/ui/Modal.tsx @@ -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), diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index 47a56ce9..0f2326ac 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -21,7 +21,7 @@ "job": "This job doesn't exist any more." }, "reason": { - "unreachable": "The server couldn't be reached, so the change didn't go through. Try again.", + "unreachable": "The server couldn't be reached. Try again.", "forbidden": "You don't have permission to do that.", "rateLimited": "Too many requests at once β€” wait a moment and try again.", "unexpected": "Something went wrong in the app. Try again." diff --git a/frontend/src/lib/history.dom.test.ts b/frontend/src/lib/history.dom.test.ts index f34f9907..380d81d9 100644 --- a/frontend/src/lib/history.dom.test.ts +++ b/frontend/src/lib/history.dom.test.ts @@ -163,6 +163,36 @@ describe("closeOverlaysThenNavigate", () => { } }); + // An overlay's Back callback is a Back STEP: the Plex player keeps its panels on one entry and peels + // one per call. Called once, the entry stayed and the navigation was dropped 1.5 s later β€” `i`, `d`, + // Shift+W in the player closed the info panel and nothing else (deep-review E11 round 1). + it("asks again, a render apart, when one call peels only a step off the overlay", async () => { + const back = stubBack(); + try { + let steps = 2; + const calls: number[] = []; + const holder: { unmount?: () => void } = {}; + const r = renderHook(() => + useBackToClose(() => { + calls.push(steps); + if (--steps === 0) holder.unmount?.(); + }), + ); + holder.unmount = r.unmount; + await settle(); + const navigate = vi.fn(); + act(() => closeOverlaysThenNavigate(navigate)); + expect(calls).toEqual([2]); + await act(async () => { + await new Promise((res) => setTimeout(res, 300)); + }); + expect(calls).toEqual([2, 1]); // never called again once the entry is gone + expect(navigate).toHaveBeenCalledTimes(1); + } finally { + back.mockRestore(); + } + }); + // The old helper ran after "a popstate" OR a 250 ms fallback, whichever came first β€” so a reclaim // slower than the fallback undid the navigation. It must wait for the history, not the clock. it("waits for a back() still in flight instead of pushing under it", async () => { diff --git a/frontend/src/lib/history.navPosition.dom.test.tsx b/frontend/src/lib/history.navPosition.dom.test.tsx index 53d4887b..33fe6ffe 100644 --- a/frontend/src/lib/history.navPosition.dom.test.tsx +++ b/frontend/src/lib/history.navPosition.dom.test.tsx @@ -5,7 +5,7 @@ // stamped onto the entry being left (`rememberNavPosition`) and a popstate landing on it focuses the item // once the page's lists are back. A landing is simulated the way the browser delivers one: the entry's // state is current and a popstate carries it. Its own file: history.ts and listNav keep module state. -import { useRef } from "react"; +import { useEffect, useRef, useState } from "react"; import { afterEach, describe, expect, it } from "vitest"; import { act, cleanup, render, waitFor } from "@testing-library/react"; import { rememberNavPosition } from "./history"; @@ -92,6 +92,44 @@ describe("rememberNavPosition Β· Back focuses the item a view was left from", () await waitFor(() => expect(focused()).toBe("a1")); }); + // A sub-view (a Plex show page) switches back in its OWN popstate listener, registered after this + // module's: while ours runs, focus still sits in the view being left. Measured in Chrome (deep-review + // E11 round 1) β€” Discover card β†’ show β†’ `j` on the show page β†’ Back left focus on . + it("restores when a later popstate listener switches the view the focus was still in", async () => { + const { unmount } = render(); + const stamped = leaveFrom("b1", 4); + unmount(); + function Views() { + const [view, setView] = useState<"sub" | "page">("sub"); + useEffect(() => { + const onPop = () => setView("page"); + window.addEventListener("popstate", onPop); + return () => window.removeEventListener("popstate", onPop); + }, []); + return view === "page" ? : ; + } + render(); + press("j"); + expect(focused()).toBe("s0"); + landOn(stamped); + await waitFor(() => expect(focused()).toBe("b1")); + }); + + // A restore still waiting for the landed page's lists must not outlive the next push: it would + // focus "list 1, item 1" of whatever page that push opened. + it("is cancelled by the next push", async () => { + const { unmount } = render(); + const stamped = leaveFrom("b1", 4); + unmount(); + landOn(stamped); + render(); // one list of the two: the restore keeps waiting + await sleep(120); + window.history.pushState({ ...rememberNavPosition(), view: "elsewhere" }, ""); + render(); // enough lists now, on the page the push opened + await sleep(150); + expect(focused()).toBeUndefined(); + }); + it("leaves focus where it is when something already has it", async () => { const { unmount } = render(); const stamped = leaveFrom("b1", 4); diff --git a/frontend/src/lib/history.navRestore.reload.dom.test.tsx b/frontend/src/lib/history.navRestore.reload.dom.test.tsx new file mode 100644 index 00000000..78f888d6 --- /dev/null +++ b/frontend/src/lib/history.navRestore.reload.dom.test.tsx @@ -0,0 +1,50 @@ +// @vitest-environment jsdom +// +// The stamp `rememberNavPosition` leaves in history.state survives a reload, but module state does not: +// the listener that reads it was installed by the first push, so after F5 the first Back onto a +// stamped entry restored nothing (deep-review E11 round 1). Its own file, because the point is a +// module that has made NO push yet β€” any other test in history.navPosition would have made one. +import { useRef } from "react"; +import { afterEach, expect, it } from "vitest"; +import { act, cleanup, render, waitFor } from "@testing-library/react"; +import "./history"; +import { navItemAttrs, useListNav } from "./listNav"; + +function List({ id, count }: { id: string; count: number }) { + const ref = useRef(null); + useListNav(ref, { enabled: true }); + return ( +
+ {Array.from({ length: count }, (_, i) => ( +
+ {id} + {i} +
+ ))} +
+ ); +} + +afterEach(() => { + cleanup(); + window.history.replaceState(null, ""); +}); + +it("restores on a landing before this load has pushed anything", async () => { + // An entry stamped before the reload: list 1 of 2, item 1. + const state = { sfPage: "home", sfNav: { list: 1, lists: 2, index: 1 } }; + window.history.replaceState(state, ""); + render( + <> + + + , + ); + act(() => { + window.dispatchEvent(new PopStateEvent("popstate", { state })); + }); + await waitFor(() => + expect((document.activeElement as HTMLElement | null)?.dataset.testid).toBe("b1"), + ); + expect(window.history.state.sfNav).toBeUndefined(); +}); diff --git a/frontend/src/lib/history.ts b/frontend/src/lib/history.ts index d8deaf31..9f53e238 100644 --- a/frontend/src/lib/history.ts +++ b/frontend/src/lib/history.ts @@ -69,11 +69,10 @@ export function subscribeNav(cb: () => void): () => void { // See lib/listNav "The item a view was left from". Stamped onto the entry being LEFT, right before a // push opens a new view over it; read when Back/Forward lands on that entry, and dropped from it there, // so a later landing on the same entry β€” an overlay closing over it β€” cannot pull focus to an old item. +// Installed at load, not by the first push: a stamp survives a reload in history.state, and the first +// Back after one must still find a listener (deep-review E11 round 1). let cancelNavRestore = () => {}; -let navRestoreInstalled = false; -function ensureNavRestore(): void { - if (navRestoreInstalled) return; - navRestoreInstalled = true; +if (typeof window !== "undefined") { window.addEventListener("popstate", (e) => { cancelNavRestore(); const pos = (e.state as { sfNav?: NavPosition } | null)?.sfNav; @@ -88,7 +87,9 @@ function ensureNavRestore(): void { * channel, a tab). Returns the current state WITHOUT the stamp: a push that spreads the current state * must spread this, so the new entry does not inherit a position it was never left from. */ export function rememberNavPosition(): Record { - ensureNavRestore(); + // A restore still waiting for the last landing's lists belongs to the page being left: left + // running, it would focus "list N, item M" of whatever page this push opens. + cancelNavRestore(); const { sfNav: _stale, ...st } = (window.history.state ?? {}) as Record; const pos = navPosition(); window.history.replaceState(pos ? { ...st, sfNav: pos } : st, ""); @@ -251,6 +252,11 @@ function historyInSync(): boolean { // render (measured back()β†’popstate: 1–13 ms in Chrome); nothing in the app refuses a close today, // but a future "discard your changes?" guard must not be navigated out from under. const CLOSE_TIMEOUT_MS = 1500; +// An overlay's Back callback is a Back STEP, and one entry can hold several steps: the Plex player's +// panels share a single entry and Back peels one per press. Navigating over it asks again, a render +// apart (the callback reads what the last one closed), until the entry is gone or the timeout drops +// the command. +const RECLOSE_MS = 50; /** Close `targets` (via `close`), then `run` once they have unmounted AND the history has caught up. * @@ -267,7 +273,12 @@ const CLOSE_TIMEOUT_MS = 1500; * `run` goes out on a fresh task, never inside the popstate dispatch that settled it: listeners * registered before ours (NavigationProvider's) have read that event already, but ones after it * have not, and a page applied from the OLD entry after our push would undo the navigation. */ -function afterClosing(targets: readonly Overlay[], close: () => void, run: () => void): void { +function afterClosing( + targets: readonly Overlay[], + close: () => void, + run: () => void, + reclose = false, +): void { let done = false; let queued = false; const gone = () => targets.every((o) => !overlayStack.includes(o)); @@ -277,7 +288,13 @@ function afterClosing(targets: readonly Overlay[], close: () => void, run: () => syncWaiters.delete(check); window.removeEventListener("popstate", check); clearTimeout(timeout); + clearInterval(again); }; + const again = reclose + ? setInterval(() => { + if (!done && !gone()) close(); + }, RECLOSE_MS) + : undefined; function check(): void { if (queued || !ready()) return; queued = true; @@ -318,7 +335,12 @@ export function closeOverlaysThenNavigate(navigate: () => void): void { return; } const targets = [...overlayStack]; - afterClosing(targets, () => [...targets].reverse().forEach((o) => o.cb()), navigate); + const close = () => + [...targets] + .reverse() + .filter((o) => overlayStack.includes(o)) + .forEach((o) => o.cb()); + afterClosing(targets, close, navigate, true); } /** Close the overlay a control lives in, then run its command once that overlay's entry is back β€” diff --git a/frontend/src/lib/keymap.sequence.test.ts b/frontend/src/lib/keymap.sequence.test.ts index 4d31b5aa..cf32f0d5 100644 --- a/frontend/src/lib/keymap.sequence.test.ts +++ b/frontend/src/lib/keymap.sequence.test.ts @@ -87,6 +87,22 @@ describe("two-step sequences", () => { expect(calls).toEqual(["goDownloads"]); }); + // No second step needs a modifier (D1), so a chord with one is the reader changing their mind: it + // must reach its own row. Swallowed, `g` then Ctrl+K opened the browser's search, not the palette. + it("lets a modifier chord abandon the sequence and match as itself", () => { + const calls: string[] = []; + const global = { + help: () => calls.push("help"), + palette: () => calls.push("palette"), + home: () => calls.push("home"), + }; + dispatch(press({ key: "g" }), "page", pageHandlers(calls)); + const k = press({ key: "k", ctrlKey: true, metaKey: true }); + expect(dispatch(k, "global", global)).toBe(true); + expect(dispatch(press({ key: "f" }), "page", pageHandlers(calls))).toBe(false); // not `g f` + expect(calls).toEqual(["palette"]); + }); + it("does not start or finish inside a text field", () => { const calls: string[] = []; const input = document.createElement("input"); diff --git a/frontend/src/lib/keymap.ts b/frontend/src/lib/keymap.ts index 413d1e47..19501eb7 100644 --- a/frontend/src/lib/keymap.ts +++ b/frontend/src/lib/keymap.ts @@ -297,7 +297,14 @@ function liveSequence(e: KeyboardEvent): typeof pending { pending = null; return null; } - return MODIFIER_KEYS.has(e.key) ? null : pending; + if (MODIFIER_KEYS.has(e.key)) return null; + // A chord with a modifier cannot be a second step (no binding may need one, D1): it abandons the + // sequence and is matched as itself, or `g` then Mod+K would open the browser's search, not ours. + if (e.ctrlKey || e.metaKey || e.altKey || e.shiftKey) { + pending = null; + return null; + } + return pending; } const stepsOf = (chord: string): string[] => chord.split(" "); diff --git a/frontend/src/lib/layerStack.ts b/frontend/src/lib/layerStack.ts index d56d8512..573a61fa 100644 --- a/frontend/src/lib/layerStack.ts +++ b/frontend/src/lib/layerStack.ts @@ -51,6 +51,14 @@ export function anyLayerAbove(id: number): boolean { return i !== -1 && i < stack.length - 1; } +/** True when a layer that owns the keyboard sits above `id` β€” the predicate for a dialog's Tab trap. + * A tooltip or hover card open under a resting pointer takes Escape first, but not the Tab: deferring + * the trap to it let Shift+Tab walk out of the dialog into the page behind its scrim. */ +export function anyBlockingLayerAbove(id: number): boolean { + const i = stack.indexOf(id); + return i !== -1 && stack.slice(i + 1).some((x) => !passive.has(x)); +} + // UX-8 β€” a layer that ACTS on an Escape stamps the event here; a lower layer with its own key handler // (the players, listening on `window`) reads the stamp instead of re-reading the stack. This closes // the "one Escape closes BOTH the popover and the player" bug: the popover's Escape listener is on @@ -76,7 +84,7 @@ export function useLayer( active: boolean, onEscape?: () => void, opts?: { passive?: boolean }, -): { isTopmost: () => boolean; anyAbove: () => boolean } { +): { isTopmost: () => boolean; anyAbove: () => boolean; blockingAbove: () => boolean } { const idRef = useRef(null); const escRef = useRef(onEscape); escRef.current = onEscape; @@ -110,5 +118,6 @@ export function useLayer( return { isTopmost: () => idRef.current != null && isTopmost(idRef.current), anyAbove: () => idRef.current != null && anyLayerAbove(idRef.current), + blockingAbove: () => idRef.current != null && anyBlockingLayerAbove(idRef.current), }; } diff --git a/frontend/src/lib/listNav.dom.test.tsx b/frontend/src/lib/listNav.dom.test.tsx index 34ea2e0d..fffc797d 100644 --- a/frontend/src/lib/listNav.dom.test.tsx +++ b/frontend/src/lib/listNav.dom.test.tsx @@ -3,9 +3,9 @@ // `lib/listNav` against real DOM: `j`/`k` from item to item and across lists in reading order, the // arrows around a grid, Enter as a click on the item's own "open" element β€” and the places where the // keys must stay out of the way (a field, an overlay, a control inside the card). -import { useRef } from "react"; +import { useRef, useState } from "react"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { act, cleanup, render, screen, waitFor } from "@testing-library/react"; +import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; import { navItemAttrs, useListNav } from "./listNav"; import { pushLayer, removeLayer } from "./layerStack"; @@ -349,3 +349,83 @@ describe("useListNav Β· keys and lists it must leave alone", () => { expect(focused()).toBe("a1"); }); }); + +describe("useListNav Β· the roving tabindex and a removed item (deep-review E11 round 1)", () => { + // A tile that is a native + ))} +
+ ); + } + render(); + press("j"); + expect(focused()).toBe("t0"); + expect(screen.getByTestId("t0").hasAttribute("tabindex")).toBe(false); + press("j"); + expect(focused()).toBe("t1"); + expect(screen.getByTestId("t1").hasAttribute("tabindex")).toBe(false); + }); + + // Landing again on the item that already holds the roved tabindex (focus went to in between) + // forgot it, so it kept tabindex="-1" for good β€” and a click on its blank area focused the card. + it("gives the tabindex back after landing twice on the same item", () => { + render(); + press("j"); + expect(focused()).toBe("a0"); + act(() => (document.activeElement as HTMLElement).blur()); + press("j"); + expect(focused()).toBe("a0"); + press("j"); + expect(focused()).toBe("a1"); + expect(screen.getByTestId("a0").hasAttribute("tabindex")).toBe(false); + }); + + // Hiding the focused feed card unmounts it and drops focus to ; the next `j` started over at + // the first visible card instead of going on from the hidden card's slot. + it("goes on from the slot of an item that was removed under the focus", () => { + function Removable() { + const [ids, setIds] = useState([0, 1, 2, 3]); + const ref = useRef(null); + useListNav(ref, { enabled: true }); + return ( +
+ {ids.map((id, i) => ( +
+ +
+ ))} +
+ ); + } + withLayout( + (_, n) => ({ left: 0, top: n * 100 }), + () => { + render(); + press("j"); + press("j"); + press("j"); + expect(focused()).toBe("r2"); + act(() => screen.getByTestId("r2-hide").focus()); + fireEvent.click(screen.getByTestId("r2-hide")); + expect(document.activeElement).toBe(document.body); + press("j"); + expect(focused()).toBe("r3"); + }, + ); + }); +}); diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts index 1e1a1bdd..fa3cf1ec 100644 --- a/frontend/src/lib/listNav.ts +++ b/frontend/src/lib/listNav.ts @@ -120,13 +120,44 @@ function firstVisible(): { list: NavList; index: number } | null { return fallback; } +/** The item focus was last inside, so a `j` after that item was REMOVED (a feed card hidden, a + * download deleted β€” focus falls to ) goes on from its slot instead of starting over. */ +let last: { list: NavList; index: number; el: HTMLElement } | null = null; + +function onFocusIn(e: FocusEvent): void { + const item = (e.target as HTMLElement | null)?.closest?.(`[${INDEX}]`); + const list = + item && lists.get(item.parentElement?.closest(`[${LIST}]`) as HTMLElement); + if (item && list) last = { list, index: indexOf(item), el: item }; +} + +/** Where `j` (`k`: the item before) lands after the last-focused item left the DOM: the item now in + * its slot, if that slot is on screen. An item that only scrolled out of a virtualized list has left + * the DOM too, but its slot is off screen then, so the reader's viewport (`firstVisible`) wins. */ +function slotOfRemoved(delta: 1 | -1): { list: NavList; index: number } | null { + if (!last || last.el.isConnected || lists.get(last.list.el) !== last.list) return null; + const n = last.list.count(); + const index = Math.min(Math.max(0, delta > 0 ? last.index : last.index - 1), n - 1); + const el = index >= 0 ? itemAt(last.list, index) : null; + if (!el) return null; + const box = scrollParent(last.list.el)?.getBoundingClientRect(); + const r = el.getBoundingClientRect(); + const band = (box?.top ?? 0) + (parseFloat(getComputedStyle(el).scrollMarginTop) || 0); + return r.bottom > band && r.top < (box?.bottom ?? window.innerHeight) + ? { list: last.list, index } + : null; +} + function focusItem(el: HTMLElement, reveal = true): void { - if (roved && roved !== el) roved.removeAttribute("tabindex"); - roved = null; - // An item that is focusable in its own right (a Plex card is a role=button) keeps its tabindex. - if (!el.hasAttribute("tabindex")) { - el.setAttribute("tabindex", "-1"); - roved = el; + if (roved !== el) { + roved?.removeAttribute("tabindex"); + roved = null; + // An item that is a Tab stop in its own right (a native