From e09d613b04b0834ce87c2c1f99e1b5d4f3705c15 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 4 Oct 2026 02:29:03 +0200 Subject: [PATCH] fix(ui): a reviewed log takes the focus back once, and a dialog's focus survives a Retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit E32 review round 1 (F1, F2). - Issue composer: the review's focus-return token became state in S4 and was never spent, so every Preview → Edit round trip remounted the attachments and moved the focus from the toggle to the log's Review button. The token is cleared once it has been used, as the ref was. - Modal (initialFocus "field"): the late-form watch moved the focus to the first control once and stopped. A failed load's Retry then gave way to the spinner, its focus fell to inside the aria-modal dialog, and the form's first field never got it. The watch now re-arms when the focus falls to : it waits on the ✕ and moves to the first field when the form arrives. Focus in a dialog stacked on top is left alone. --- .../src/components/IssueComposer.dom.test.tsx | 16 ++++++++++++++++ frontend/src/components/IssueComposer.tsx | 11 ++++++++--- .../src/components/downloadAdmin.dom.test.tsx | 15 +++++++++++++++ frontend/src/components/ui/Modal.tsx | 14 +++++++++++--- 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/frontend/src/components/IssueComposer.dom.test.tsx b/frontend/src/components/IssueComposer.dom.test.tsx index b1a0ec0a..be3aba63 100644 --- a/frontend/src/components/IssueComposer.dom.test.tsx +++ b/frontend/src/components/IssueComposer.dom.test.tsx @@ -539,6 +539,22 @@ describe("the issue composer", () => { expect(api.saved).toHaveLength(0); // sent: nothing to save, only the rest to send }); + it("returns the focus to a reviewed log once, not on every way back from the preview", async () => { + mount(); + fireEvent.click( + (await screen.findAllByRole("button", { name: "recorder.composer.review" }))[0]!, + ); + fireEvent.click(screen.getByRole("button", { name: "recorder.composer.reviewDone" })); + const review = screen.getAllByRole("button", { name: "recorder.composer.review" })[0]; + expect(document.activeElement).toBe(review); + fireEvent.click(screen.getByRole("button", { name: "recorder.composer.preview" })); + const edit = screen.getByRole("button", { name: "recorder.composer.edit" }); + edit.focus(); + fireEvent.click(edit); + await screen.findAllByRole("button", { name: "recorder.composer.review" }); + expect(document.activeElement).toBe(edit); // the toggle keeps it; the log's button does not take it + }); + it("previews through the tracker in a frame where no script runs", async () => { tracker = { repo: "o/r" }; mount(); diff --git a/frontend/src/components/IssueComposer.tsx b/frontend/src/components/IssueComposer.tsx index 37b46ba7..3a36f963 100644 --- a/frontend/src/components/IssueComposer.tsx +++ b/frontend/src/components/IssueComposer.tsx @@ -412,6 +412,7 @@ function ComposerBody({ }} onReview={setReviewing} reviewed={reviewed} + setReviewed={setReviewed} /> )} @@ -596,6 +597,7 @@ function Attachments({ onAdd, onReview, reviewed, + setReviewed, }: { session: DebugSession; shotsQ: { data?: { items: DebugScreenshot[] }; isError: boolean; refetch: () => unknown }; @@ -605,6 +607,7 @@ function Attachments({ onAdd: (files: Iterable) => void; onReview: (source: Source) => void; reviewed: Source | null; + setReviewed: (spent: null) => void; }) { const { t, i18n } = useTranslation(); const lang = i18n.language; @@ -614,10 +617,12 @@ function Attachments({ const pick = useRef(null); const reviewButtons = useRef>>({}); // On mount after a review closed (this list unmounts while one is open), that log's Review button - // takes the focus back. Only a mount or a new review moves it, so no later render steals it. + // takes the focus back — once: the spent token keeps a later remount (back from the preview) off it. useEffect(() => { - if (reviewed) reviewButtons.current[reviewed]?.focus(); - }, [reviewed]); + if (!reviewed) return; + reviewButtons.current[reviewed]?.focus(); + setReviewed(null); + }, [reviewed, setReviewed]); const remove = useMutation({ mutationFn: (shotId: number) => api.removeDebugScreenshot(session.id, shotId), meta: { failTitle: "recorder.composer.removeShotFailed" }, diff --git a/frontend/src/components/downloadAdmin.dom.test.tsx b/frontend/src/components/downloadAdmin.dom.test.tsx index 1fcd3a56..575de8e9 100644 --- a/frontend/src/components/downloadAdmin.dom.test.tsx +++ b/frontend/src/components/downloadAdmin.dom.test.tsx @@ -53,6 +53,21 @@ describe("the download-quota editor", () => { expect(get).toHaveBeenCalledTimes(2); }); + it("gives the first field the focus when the form arrives after a retry", async () => { + // jsdom lays nothing out, so every element reads as hidden to the dialog's focus rules. + vi.spyOn(HTMLElement.prototype, "offsetParent", "get").mockReturnValue(document.body); + vi.spyOn(api, "adminGetDownloadQuota") + .mockRejectedValueOnce(new Error("down")) + .mockResolvedValueOnce(QUOTA as never); + open(); + await screen.findByRole("alert"); + const retry = screen.getByText("common.retry").closest("button")!; + await waitFor(() => expect(document.activeElement).toBe(retry)); + fireEvent.click(retry); // the Retry gives way to the spinner: its focus must not fall to + const first = await screen.findByDisplayValue("5"); + await waitFor(() => expect(document.activeElement).toBe(first)); + }); + it("saves the GB field as bytes, and the reset drops the override", async () => { vi.spyOn(api, "adminGetDownloadQuota").mockResolvedValue(QUOTA as never); const set = vi.spyOn(api, "adminSetDownloadQuota").mockResolvedValue(QUOTA as never); diff --git a/frontend/src/components/ui/Modal.tsx b/frontend/src/components/ui/Modal.tsx index c0fa8009..68a86364 100644 --- a/frontend/src/components/ui/Modal.tsx +++ b/frontend/src/components/ui/Modal.tsx @@ -106,14 +106,22 @@ export default function Modal({ // A form that loads after the dialog opens (the issue composer shows a spinner first) has no field // to take the focus at open, so it lands on the ✕. When the first control arrives the focus moves // there — but only while it still rests on the ✕: a user who has moved it keeps it where it is. + // The watch re-arms when the focused control is taken away (a failed load's Retry giving way to + // the spinner again): the focus fell to , so it waits on the ✕ for the form (E32 review). + // Only : focus in a dialog stacked on top is not this one's to take. useEffect(() => { const body = bodyRef.current; if (initialFocus !== "field" || !body || focusablesIn(body).length > 0) return; + let armed = true; const watch = new MutationObserver(() => { + if (document.activeElement === document.body && closeRef.current) { + closeRef.current.focus(); + armed = true; + } const first = focusablesIn(body)[0]; - if (!first) return; - watch.disconnect(); - if (document.activeElement === closeRef.current) first.focus(); + if (!armed || !first || document.activeElement !== closeRef.current) return; + armed = false; + first.focus(); }); watch.observe(body, { childList: true, subtree: true }); return () => watch.disconnect();