From 3cd0c525e2947c8c07d21a4b8fbce5be644d863f Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 13 Sep 2026 19:18:04 +0200 Subject: [PATCH] =?UTF-8?q?fix(keys):=20epic-end=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20focus=20comes=20back=20to=20the=20card,=20the=20rig?= =?UTF-8?q?ht=20rail=20and=20the=20reason?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whole-epic deep-review of E11 (18 raised, 12 kept; B and G refuted, four latent test-hardening notes dropped), plus the admin refusal copy gap round 1 surfaced. - A context menu opened from a card the keyboard is on gives the focus back to the card. floating-ui returns focus through the opener's first tabbable descendant even when handed a ref, so for a non-tabbable opener the surface restores it itself (measured in Chrome: j, Shift+F10, Escape left it on the thumbnail link). - Back finds a named list (a rail) by its name: a rail that appeared above it after the cache expired sent the focus into the wrong rail. An unnamed list is matched only when the page has the same number of lists. - The slot kept for a removed item is forgotten once focus moves on to page content (search, a chip), so j after new results starts at the first visible item; a menu or dialog opened from the item keeps it. - The list keys reach the cheat sheet only while a list has an item. - A j-focused card root is a named group while it holds the focus. - The channel page's Block and Hide toggles keep the focus while their request runs. - A split export that fails part-way says how many clips were added and deselects them, so a retry adds only the rest. - Config save, the one-click share link and "log out other sessions" report failures with their reason. - VirtualGrid uses the one scroll-parent walk listNav measures against. - Admin refusals (activate, resend, invite, set-password link) say why in HU and EN instead of "try again"; the email-off special cases collapse into that one reason. The contract reads the codes from localizeDetail. HU copy points to Konfiguráció → E-mail / SMTP; HU "unreachable" matches the EN change of round 1. --- frontend/src/components/AccountPanel.tsx | 9 +- frontend/src/components/AdminUsers.tsx | 22 +---- .../ChannelPage.busyFocus.dom.test.tsx | 93 +++++++++++++++++++ frontend/src/components/ChannelPage.tsx | 14 +-- frontend/src/components/ConfigPanel.tsx | 4 +- frontend/src/components/SettingsPanel.tsx | 4 +- .../VideoEditor.partial.dom.test.tsx | 81 ++++++++++++++++ frontend/src/components/VideoEditor.tsx | 32 +++++-- .../src/components/accountPanel.dom.test.tsx | 12 ++- .../src/components/adminInvites.dom.test.tsx | 8 +- .../adminPendingAccess.dom.test.tsx | 10 +- .../src/components/ui/AnchoredPopover.tsx | 38 +++++++- .../components/ui/ContextMenu.dom.test.tsx | 36 +++++++ frontend/src/components/ui/VirtualGrid.tsx | 18 +--- frontend/src/i18n/backendIds.contract.test.ts | 24 +++++ frontend/src/i18n/locales/en/editor.json | 1 + frontend/src/i18n/locales/en/errors.json | 8 ++ frontend/src/i18n/locales/en/settings.json | 1 - frontend/src/i18n/locales/en/users.json | 3 +- frontend/src/i18n/locales/hu/editor.json | 1 + frontend/src/i18n/locales/hu/errors.json | 10 +- frontend/src/i18n/locales/hu/settings.json | 1 - frontend/src/i18n/locales/hu/users.json | 3 +- frontend/src/lib/adminUsers.test.ts | 20 +--- frontend/src/lib/adminUsers.ts | 10 -- frontend/src/lib/api/core.test.ts | 18 ++++ frontend/src/lib/api/core.ts | 9 ++ frontend/src/lib/downloads.test.ts | 5 +- frontend/src/lib/downloads.ts | 5 +- .../src/lib/history.navPosition.dom.test.tsx | 47 +++++++++- frontend/src/lib/listNav.dom.test.tsx | 83 +++++++++++++++++ frontend/src/lib/listNav.scope.dom.test.tsx | 54 +++++++++++ frontend/src/lib/listNav.ts | 69 +++++++++++--- 33 files changed, 640 insertions(+), 113 deletions(-) create mode 100644 frontend/src/components/ChannelPage.busyFocus.dom.test.tsx create mode 100644 frontend/src/components/VideoEditor.partial.dom.test.tsx create mode 100644 frontend/src/lib/listNav.scope.dom.test.tsx diff --git a/frontend/src/components/AccountPanel.tsx b/frontend/src/components/AccountPanel.tsx index 42298122..7f91ac25 100644 --- a/frontend/src/components/AccountPanel.tsx +++ b/frontend/src/components/AccountPanel.tsx @@ -7,9 +7,9 @@ import { LoadingState, StateMessage } from "./ui/QueryState"; import { api } from "../lib/api"; import { qk } from "../lib/queryKeys"; import { notify } from "../lib/notifications"; -import { notifyActionError, notifyKnownFailure } from "../lib/actionErrors"; +import { notifyActionError } from "../lib/actionErrors"; import { copyText } from "../lib/clipboard"; -import { invalidateUserAccess, isEmailOff } from "../lib/adminUsers"; +import { invalidateUserAccess } from "../lib/adminUsers"; import type { AdminUserAccess, AdminUserRow } from "../lib/api/admin"; // The admin's answer to "why can this person not get in?", and the place every recovery action for @@ -73,10 +73,7 @@ export default function AccountPanel({ const resend = useMutation({ mutationFn: () => api.resendUserVerification(user.id), onSuccess: () => notify({ level: "success", message: t("users.account.resendSent") }), - onError: (err) => - isEmailOff(err) - ? notifyKnownFailure(err, t("users.pendingAccess.resendEmailOff")) - : notifyActionError(err, t("users.pendingAccess.resendFailed")), + onError: (err) => notifyActionError(err, t("users.pendingAccess.resendFailed")), }); const activate = useMutation({ diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index 2bd5a7d2..916d5508 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -24,7 +24,7 @@ import { type PendingAccessRow, } from "../lib/api"; import { notify } from "../lib/notifications"; -import { invalidateUserAccess, isEmailOff } from "../lib/adminUsers"; +import { invalidateUserAccess } from "../lib/adminUsers"; import { ADMIN_USERS_TAB_KEY } from "../lib/adminUsersTab"; import { Button } from "./ui/Button"; import Tooltip from "./ui/Tooltip"; @@ -319,12 +319,6 @@ function UsersRoles({ me }: { me: Me }) { ); } -/** Did the invitation fail because this instance has no working SMTP? */ -function inviteEmailOff(err: HttpError): boolean { - const d = err.detailData as { code?: string; reason?: string } | null | undefined; - return d?.code === "invite_send" && d.reason === "email_off"; -} - // --- Sign-ups that are still outside (roadmap UX-25) -------------------------------------------- // // The gap this closes: approve an address BEFORE the person registers and their sign-up produces no @@ -360,12 +354,9 @@ function PendingAccess() { refresh(); notify({ level: "success", message: t("users.pendingAccess.resent", { email: u.email }) }); }, - // The public resend is deliberately silent; this one is not, so say WHY it failed — an instance - // with no working SMTP would otherwise show a success toast for a mail that was never sent. - onError: (err) => - isEmailOff(err) - ? notifyKnownFailure(err, t("users.pendingAccess.resendEmailOff")) - : notifyActionError(err, t("users.pendingAccess.resendFailed")), + // The public resend is deliberately silent; this one is not, and the server's reason says WHY + // (no working SMTP, a refused send) — a success toast for a mail never sent would be the lie. + onError: (err) => notifyActionError(err, t("users.pendingAccess.resendFailed")), }); const list = rows.data ?? []; @@ -591,10 +582,7 @@ function AdminInvites() { refresh(); notify({ level: "success", message: t("settings.invites.invited", { email: v.inv.email }) }); }, - onError: (err) => - err instanceof HttpError && inviteEmailOff(err) - ? notifyKnownFailure(err, t("settings.invites.inviteEmailOff")) - : notifyActionError(err, t("settings.invites.inviteFailed")), + onError: (err) => notifyActionError(err, t("settings.invites.inviteFailed")), }); // The one recipient the app has no language signal about: the admin typed this address in, so // nobody ever told us how to write to its owner. The dialog is where that gets decided — and it diff --git a/frontend/src/components/ChannelPage.busyFocus.dom.test.tsx b/frontend/src/components/ChannelPage.busyFocus.dom.test.tsx new file mode 100644 index 00000000..e676af89 --- /dev/null +++ b/frontend/src/components/ChannelPage.busyFocus.dom.test.tsx @@ -0,0 +1,93 @@ +// @vitest-environment jsdom +// +// The channel page's Block and Hide toggles while their request runs (deep-review E11 round 2). A +// focused button that turns natively `disabled` drops the keyboard to in Chrome (measured for +// ui/Button in E11 S2), so a busy toggle blocks the click with aria-disabled instead — focusable, and +// still pressed only once. +import type { ReactNode } from "react"; +import { describe, it, expect, vi, beforeAll, afterEach } from "vitest"; +import { render, screen, fireEvent, waitFor, cleanup, act } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => ({ + ...(await importOriginal()), + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), +})); +vi.mock("./NavigationProvider", () => ({ + useNavigationActions: () => ({ closeChannel: vi.fn(), openChannel: vi.fn() }), +})); +vi.mock("./ConfirmProvider", () => ({ useConfirm: () => async () => true })); +vi.mock("./FeedFiltersProvider", () => ({ + useFeedFilters: () => ({ includeNormal: true, includeShorts: false, includeLive: false }), +})); +vi.mock("./Feed", () => ({ default: () => null })); +vi.mock("./ChannelAbout", () => ({ default: () => null })); +// The page's chrome portals into the shell's fixed toolbar band, which this test has no shell for. +vi.mock("./ui/PageShell", async (importOriginal) => ({ + ...(await importOriginal()), + PageToolbar: ({ children }: { children?: ReactNode }) => <>{children}, +})); + +import ChannelPage from "./ChannelPage"; +import { api, type ChannelDetail, type Me } from "../lib/api"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; + globalThis.IntersectionObserver ??= Stub as unknown as typeof IntersectionObserver; +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +const CH = "UCsXVk37bltHxD1rDPwtNM8Q"; + +describe("ChannelPage — a busy toggle keeps the keyboard", () => { + it.each([ + ["channel.block", "blockChannel"], + ["channel.hide", "updateChannel"], + ] as const)("%s stays focusable and fires once while its request runs", async (label, call) => { + vi.spyOn(api, "channelDetail").mockResolvedValue({ + id: CH, + title: "LEMMiNO", + subscribed: true, + blocked: false, + hidden: false, + deep_requested: false, + backfill_done: true, + stored_videos: 10, + } as unknown as ChannelDetail); + vi.spyOn(api, "tags").mockResolvedValue([] as never); + const pending = vi.spyOn(api, call).mockReturnValue(new Promise(() => {}) as never); + render( + + {}} + onShowInFeed={() => {}} + /> + , + ); + const button = await screen.findByRole("button", { name: label }); + act(() => button.focus()); + fireEvent.click(button); + await waitFor(() => expect(button.getAttribute("aria-disabled")).toBe("true")); + expect(button.hasAttribute("disabled")).toBe(false); + expect(document.activeElement).toBe(button); + fireEvent.click(button); + expect(pending).toHaveBeenCalledTimes(1); + }); +}); diff --git a/frontend/src/components/ChannelPage.tsx b/frontend/src/components/ChannelPage.tsx index fc7bca47..5982111f 100644 --- a/frontend/src/components/ChannelPage.tsx +++ b/frontend/src/components/ChannelPage.tsx @@ -479,11 +479,13 @@ export default function ChannelPage({ )} {!me.is_demo && ( + + ))} + + ); + } + + // `s`, a query, Escape: the feed stays mounted with new results and the old card is gone. The slot + // kept for a REMOVED card sent the next `j` to the third new result instead of the first. + it("starts from the first visible item after focus went on to a field and the items changed", () => { + withLayout( + (_, n) => ({ left: 0, top: n * 100 }), + () => { + const page = (ids: number[]) => ( + <> + + + + ); + const { rerender } = render(page([0, 1, 2, 3])); + press("j"); + press("j"); + press("j"); + expect(focused()).toBe("r2"); + act(() => screen.getByTestId("field").focus()); + rerender(page([4, 5, 6, 7])); + act(() => screen.getByTestId("field").blur()); + press("j"); + expect(focused()).toBe("r4"); + }, + ); + }); + + // A confirm (or the item's own menu) takes the focus before the delete it confirms: back from it, the + // next `j` still goes on from the deleted row's slot. + it("keeps the slot when the removal was confirmed in a dialog", () => { + withLayout( + (_, n) => ({ left: 0, top: n * 100 }), + () => { + const page = (ids: number[]) => ( + <> +
+ +
+ + + ); + const { rerender } = render(page([0, 1, 2, 3])); + press("j"); + press("j"); + press("j"); + act(() => screen.getByTestId("ok").focus()); + rerender(page([0, 1, 3])); + act(() => screen.getByTestId("ok").blur()); + press("j"); + expect(focused()).toBe("r3"); + }, + ); + }); + + // A card root is a plain
: focused by `j` it had no role and no name, so a screen reader read + // an unlabelled run of text. + it("names a focused card after what it opens while it holds the focus, then gives the name back", () => { + render(); + press("j"); + const a0 = screen.getByTestId("a0"); + expect(a0.getAttribute("role")).toBe("group"); + expect(a0.getAttribute("aria-label")).toBe("title"); + press("j"); + expect(a0.hasAttribute("role")).toBe(false); + expect(a0.hasAttribute("aria-label")).toBe(false); + expect(screen.getByTestId("a1").getAttribute("aria-label")).toBe("title"); + }); +}); diff --git a/frontend/src/lib/listNav.scope.dom.test.tsx b/frontend/src/lib/listNav.scope.dom.test.tsx new file mode 100644 index 00000000..49af4087 --- /dev/null +++ b/frontend/src/lib/listNav.scope.dom.test.tsx @@ -0,0 +1,54 @@ +// @vitest-environment jsdom +// +// The `list` key scope — what puts j/k, the arrows and Enter on the cheat sheet — is live only while a +// registered list has an item to land on. An empty download queue, a list still loading or one that +// failed mounts its container all the same, and the sheet listed the keys over nothing (deep-review +// E11 round 2). Its own file: listNav keeps module state. +import { useRef } from "react"; +import { afterEach, describe, expect, it } from "vitest"; +import { cleanup, render, screen, waitFor } from "@testing-library/react"; +import { navItemAttrs, useListNav } from "./listNav"; +import { useActiveScopes } from "./keymap"; + +afterEach(cleanup); + +function Scopes() { + return {useActiveScopes().join(" ")}; +} + +function Queue({ count }: { count: number }) { + const ref = useRef(null); + useListNav(ref, { enabled: true }); + return ( +
+ {count === 0 ? ( +

Nothing queued

+ ) : ( + Array.from({ length: count }, (_, i) => ( +
+ item +
+ )) + )} +
+ ); +} + +const page = (count: number) => ( + <> + + + +); +const scopes = () => screen.getByTestId("scopes").textContent!.split(" "); + +describe("useListNav · the list key scope", () => { + it("is not live over a list with nothing in it, and is while it has items", async () => { + const { rerender } = render(page(0)); + expect(scopes()).not.toContain("list"); + rerender(page(2)); + await waitFor(() => expect(scopes()).toContain("list")); + rerender(page(0)); + await waitFor(() => expect(scopes()).not.toContain("list")); + }); +}); diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts index fa3cf1ec..dda79144 100644 --- a/frontend/src/lib/listNav.ts +++ b/frontend/src/lib/listNav.ts @@ -1,4 +1,4 @@ -import { useEffect, useRef, type RefObject } from "react"; +import { useEffect, useRef, useState, type RefObject } from "react"; import { dispatch, useKeyScope } from "./keymap"; import { scrollParent } from "./listPaging"; @@ -35,8 +35,9 @@ interface NavList { } const lists = new Map(); -// The item we gave a tabindex to, so the next move can take it back. +// The item we gave a tabindex to, so the next move can take it back — and whether we named it too. let roved: HTMLElement | null = null; +let named = false; const INDEX = "data-nav-index"; const LIST = "data-nav-list"; @@ -125,10 +126,17 @@ function firstVisible(): { list: NavList; index: number } | null { 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 target = e.target as HTMLElement | null; + const item = target?.closest?.(`[${INDEX}]`); const list = item && lists.get(item.parentElement?.closest(`[${LIST}]`) as HTMLElement); if (item && list) last = { list, index: indexOf(item), el: item }; + // Focus went on to page content that is not an item — the search box, a filter chip: the reader + // left the list, and whatever it shows next (new results) is not "the slot they were in" (deep-review + // E11 round 2). A layer opened FROM the item — its menu, a confirm — does not count: focus comes + // back from it, and what it did (a hidden card, a deleted row) is the removal the slot is kept for. + // Removing the focused node itself fires no focusin. + else if (!target?.closest?.('[data-floating-ui-portal], [role="dialog"]')) last = null; } /** Where `j` (`k`: the item before) lands after the last-focused item left the DOM: the item now in @@ -150,13 +158,31 @@ function slotOfRemoved(delta: 1 | -1): { list: NavList; index: number } | null { function focusItem(el: HTMLElement, reveal = true): void { if (roved !== el) { + if (named) roved?.removeAttribute("role"); + if (named) roved?.removeAttribute("aria-label"); roved?.removeAttribute("tabindex"); roved = null; + named = false; // An item that is a Tab stop in its own right (a native