diff --git a/desktop/src/features/channels/ui/markdownDocFocus.test.mjs b/desktop/src/features/channels/ui/markdownDocFocus.test.mjs new file mode 100644 index 0000000000..70613f8fcf --- /dev/null +++ b/desktop/src/features/channels/ui/markdownDocFocus.test.mjs @@ -0,0 +1,142 @@ +/** + * Opener-identity tests for the markdown panel focus restore (PR #6731 P2 + * follow-up). The same attachment can appear in several messages — several + * cards sharing one `data-doc-url` — so restoring by URL alone always lands + * on the first DOM match. These tests pin the recorded per-invocation + * identity: the invoking card wins, the record is consumed after one + * restore, a missing record falls back to the first match, and a claimed + * focus target aborts the restore entirely. + */ + +import assert from "node:assert/strict"; +import { after, before, beforeEach, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +// pretendToBeVisual gives the module its requestAnimationFrame loop. +const dom = new JSDOM("", { + pretendToBeVisual: true, + url: "http://localhost", +}); + +before(() => { + Object.assign(globalThis, { + // This jsdom build has no window.CSS; quote-safe escaping is all the + // module's attribute selectors need. + CSS: { escape: (value) => String(value).replace(/["\\]/g, "\\$&") }, + cancelAnimationFrame: dom.window.cancelAnimationFrame.bind(dom.window), + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + requestAnimationFrame: dom.window.requestAnimationFrame.bind(dom.window), + window: dom.window, + }); +}); + +after(() => dom.window.close()); + +const DOC_URL = "http://localhost:3000/media/deadbeef.bin"; + +function addCard(url = DOC_URL) { + const card = dom.window.document.createElement("button"); + card.setAttribute("data-testid", "file-card"); + card.setAttribute("data-doc-url", url); + dom.window.document.body.appendChild(card); + return card; +} + +function settleFrames(count = 3) { + let done = Promise.resolve(); + for (let i = 0; i < count; i += 1) { + done = done.then( + () => new Promise((resolve) => dom.window.requestAnimationFrame(resolve)), + ); + } + return done; +} + +async function loadModule() { + return import("./markdownDocFocus.ts"); +} + +beforeEach(async () => { + dom.window.document.body.innerHTML = ""; + // Park focus on (focus is "free") and clear any leftover record. + const { recordMarkdownDocOpener } = await loadModule(); + recordMarkdownDocOpener(DOC_URL, null); +}); + +test("restores the recorded invoking card, not the first URL match", async () => { + const { recordMarkdownDocOpener, restoreFocusToMarkdownDocOpener } = + await loadModule(); + addCard(); + const second = addCard(); + + recordMarkdownDocOpener(DOC_URL, second); + restoreFocusToMarkdownDocOpener(DOC_URL); + await settleFrames(); + + assert.equal(dom.window.document.activeElement, second); +}); + +test("consumes the record: a later restore without one takes the first match", async () => { + const { recordMarkdownDocOpener, restoreFocusToMarkdownDocOpener } = + await loadModule(); + const first = addCard(); + const second = addCard(); + + recordMarkdownDocOpener(DOC_URL, second); + restoreFocusToMarkdownDocOpener(DOC_URL); + await settleFrames(); + assert.equal(dom.window.document.activeElement, second); + + // Free the focus again, then restore with no fresh record (deep link / + // reload open): the stale index must not survive the first consumption. + second.blur(); + restoreFocusToMarkdownDocOpener(DOC_URL); + await settleFrames(); + assert.equal(dom.window.document.activeElement, first); +}); + +test("clamps to the last remaining card when the recorded one is gone", async () => { + const { recordMarkdownDocOpener, restoreFocusToMarkdownDocOpener } = + await loadModule(); + const first = addCard(); + const second = addCard(); + + recordMarkdownDocOpener(DOC_URL, second); + second.remove(); + restoreFocusToMarkdownDocOpener(DOC_URL); + await settleFrames(); + + assert.equal(dom.window.document.activeElement, first); +}); + +test("ignores a record made for a different document URL", async () => { + const { recordMarkdownDocOpener, restoreFocusToMarkdownDocOpener } = + await loadModule(); + const first = addCard(); + addCard(); + const otherCard = addCard("http://localhost:3000/media/cafe.bin"); + + recordMarkdownDocOpener("http://localhost:3000/media/cafe.bin", otherCard); + restoreFocusToMarkdownDocOpener(DOC_URL); + await settleFrames(); + + assert.equal(dom.window.document.activeElement, first); +}); + +test("aborts when another control already claimed focus", async () => { + const { recordMarkdownDocOpener, restoreFocusToMarkdownDocOpener } = + await loadModule(); + addCard(); + const second = addCard(); + const claimed = dom.window.document.createElement("button"); + dom.window.document.body.appendChild(claimed); + + recordMarkdownDocOpener(DOC_URL, second); + claimed.focus(); + restoreFocusToMarkdownDocOpener(DOC_URL); + await settleFrames(); + + assert.equal(dom.window.document.activeElement, claimed); +}); diff --git a/desktop/src/features/channels/ui/markdownDocFocus.ts b/desktop/src/features/channels/ui/markdownDocFocus.ts index 8615493763..36fb186377 100644 --- a/desktop/src/features/channels/ui/markdownDocFocus.ts +++ b/desktop/src/features/channels/ui/markdownDocFocus.ts @@ -6,13 +6,47 @@ * panel that held focus — in both directions focus falls to `` and * keyboard/screen-reader users lose their place. On open, focus moves to the * panel's close control; on close, it returns to the attachment card that - * opened the document (matched by URL, since the original element was - * unmounted meanwhile). + * opened the document (by recorded identity, since the original element was + * unmounted meanwhile and the URL alone can match several cards). */ const PANEL_CLOSE_SELECTOR = '[data-testid="markdown-doc-panel"] [data-testid="auxiliary-panel-close"]'; +/** + * Identity of the card that invoked the current open, captured while it was + * still mounted. The same attachment can appear in several messages — several + * cards, one URL — so the opener is remembered as its DOM-order index among + * the cards sharing that URL. The index survives the narrow-layout swap (the + * message list remounts in the same order) where an element reference or a + * URL-only selector cannot single out the invoking card. + */ +type OpenerRecord = { index: number; url: string }; + +let lastOpenerRecord: OpenerRecord | null = null; + +function findOpenerCards(url: string): HTMLElement[] { + return Array.from( + document.querySelectorAll( + `[data-testid="file-card"][data-doc-url="${CSS.escape(url)}"]`, + ), + ); +} + +/** + * Remember which card invoked the open for `url`. Call at open time, while + * the clicked element is still in the document; a null/detached opener (e.g. + * a deep-link restore with no invoking card) clears the record and the + * eventual restore falls back to the first matching card. + */ +export function recordMarkdownDocOpener( + url: string, + opener: HTMLElement | null, +): void { + const index = opener ? findOpenerCards(url).indexOf(opener) : -1; + lastOpenerRecord = index >= 0 ? { index, url } : null; +} + /** Frames to wait for the target to (re)mount before giving up. */ const FOCUS_SEARCH_FRAMES = 12; @@ -63,16 +97,22 @@ export function focusMarkdownDocPanelClose(): () => void { /** * After the panel closes, return focus to the attachment card that opened - * `url` once the channel section has remounted. Aborts if focus has already + * `url` once the channel section has remounted — the recorded invoking card + * when one exists, the first URL match otherwise. Aborts if focus has already * landed on a real control (e.g. a different panel claimed it), and gives up - * quietly when the card no longer exists. + * quietly when no card exists anymore. */ export function restoreFocusToMarkdownDocOpener(url: string): void { scheduleFocusSearch( - () => - document.querySelector( - `[data-testid="file-card"][data-doc-url="${CSS.escape(url)}"]`, - ), + () => { + const cards = findOpenerCards(url); + if (cards.length === 0) return null; + const record = lastOpenerRecord?.url === url ? lastOpenerRecord : null; + // One record per invocation: consume it so a later open without a + // recorded card (deep link, reload) does not inherit a stale index. + if (record) lastOpenerRecord = null; + return cards[Math.min(record?.index ?? 0, cards.length - 1)] ?? null; + }, () => !focusIsFree(), ); } diff --git a/desktop/src/features/channels/ui/useChannelPaneOpeners.ts b/desktop/src/features/channels/ui/useChannelPaneOpeners.ts index 98cd0fb066..b1ecb45e0a 100644 --- a/desktop/src/features/channels/ui/useChannelPaneOpeners.ts +++ b/desktop/src/features/channels/ui/useChannelPaneOpeners.ts @@ -2,6 +2,8 @@ import * as React from "react"; import type { MarkdownDocTarget } from "@/shared/ui/markdown/markdownDocViewerContext"; +import { recordMarkdownDocOpener } from "./markdownDocFocus"; + type UseChannelPaneOpenersOptions = { channelType: string | undefined; channelManagementOpen: boolean; @@ -81,6 +83,10 @@ export function useChannelPaneOpeners({ // Opening a doc closes the thread pane, so an in-progress thread edit // must be resolved first — same contract as the sibling pane openers. if (!requireThreadEditResolution()) return; + // Capture the invoking card's identity before the panel swap unmounts + // it, so close can restore focus to this card and not merely the first + // card sharing the document URL. + recordMarkdownDocOpener(doc.url, doc.opener ?? null); clearCompetingPanes(); setChannelManagementOpen(false); openMarkdownDoc(doc.url, doc.filename); diff --git a/desktop/src/shared/ui/markdown/FileCard.tsx b/desktop/src/shared/ui/markdown/FileCard.tsx index ba0555d25b..764a2bc4c3 100644 --- a/desktop/src/shared/ui/markdown/FileCard.tsx +++ b/desktop/src/shared/ui/markdown/FileCard.tsx @@ -71,7 +71,7 @@ export function FileCard({ type="button" onClick={() => { if (opensInViewer) { - openMarkdownDoc({ url: href, filename }); + openMarkdownDoc({ url: href, filename, opener: cardRef.current }); return; } invokeTauri("download_file", { url: href, filename }).catch( diff --git a/desktop/src/shared/ui/markdown/markdownDocViewerContext.ts b/desktop/src/shared/ui/markdown/markdownDocViewerContext.ts index 8195d93f35..ef3914a7da 100644 --- a/desktop/src/shared/ui/markdown/markdownDocViewerContext.ts +++ b/desktop/src/shared/ui/markdown/markdownDocViewerContext.ts @@ -6,6 +6,12 @@ export type MarkdownDocTarget = { url: string; /** Human-readable filename from the message's imeta `filename` field. */ filename: string; + /** + * The card element that invoked the open, for focus restoration when the + * panel closes. The URL alone cannot identify the opener: the same + * attachment can appear in several messages, giving several cards one URL. + */ + opener?: HTMLElement | null; }; /** diff --git a/desktop/tests/e2e/markdown-doc-viewer.spec.ts b/desktop/tests/e2e/markdown-doc-viewer.spec.ts index e3f35039ee..d2aeb4f8b0 100644 --- a/desktop/tests/e2e/markdown-doc-viewer.spec.ts +++ b/desktop/tests/e2e/markdown-doc-viewer.spec.ts @@ -62,7 +62,12 @@ async function sendMarkdownAttachment(page: Page) { await page.goto("/"); await page.getByTestId("channel-general").click(); await expect(page.getByTestId("chat-title")).toHaveText("general"); + await attachAndSendMarkdown(page); +} +/** Attach + send in the already-open channel (mock re-serves one descriptor, + * so every send yields a card with the same document URL). */ +async function attachAndSendMarkdown(page: Page) { const [chooser] = await Promise.all([ page.waitForEvent("filechooser"), page.getByRole("button", { name: "Attach file" }).click(), @@ -152,6 +157,34 @@ test("narrow layout moves focus into the panel and returns it to the card", asyn await expect(page.getByTestId("file-card").last()).toBeFocused(); }); +test("close restores focus to the invoking card when the document appears twice", async ({ + page, +}) => { + // The same attachment in two messages gives two cards with one URL, so a + // URL-only anchor would always restore the first DOM match. The invoking + // card's recorded identity must win. + await sendMarkdownAttachment(page); + await attachAndSendMarkdown(page); + + await page.setViewportSize({ width: 560, height: 720 }); + + const docCards = page.locator( + `[data-testid="file-card"][data-doc-url="${DOC_URL}"]`, + ); + await expect(docCards).toHaveCount(2); + + // Open from the SECOND card. + await docCards.nth(1).focus(); + await page.keyboard.press("Enter"); + await expect(page.getByTestId("auxiliary-panel-close")).toBeFocused(); + + await page.keyboard.press("Escape"); + await expect(page.getByTestId("markdown-doc-panel")).toHaveCount(0); + await expect(docCards).toHaveCount(2); + await expect(docCards.nth(1)).toBeFocused(); + await expect(docCards.nth(0)).not.toBeFocused(); +}); + test("open document survives reload and back/forward navigation", async ({ page, }) => {