mirror of
https://github.com/block/buzz.git
synced 2026-10-29 21:41:33 +00:00
fix(desktop): restore focus to the invoking card, not the first URL match
The panel-close focus restore located the opener card by document URL alone, but the same attachment can appear in several messages — several cards, one URL — so querySelector always returned the first DOM match and closing a panel opened from a later duplicate focused the wrong card. FileCard now passes the clicked element through the open call, and the channel pane opener records it as its DOM-order index among the cards sharing that URL — a per-invocation identity that survives the narrow-layout unmount/remount where an element reference cannot. The restore consumes the record (deep-link/reload opens fall back to the first match, as before), clamps when the recorded card is gone, and still yields to any control that already claimed focus. Covered by a JSDOM unit suite for the record/restore contract and a duplicate-URL narrow-layout keyboard e2e. Signed-off-by: Riley Crane <rileycrane@squareup.com> Co-authored-by: Fizz <d4ec90109b43092b0ec94cf136850915a3732a29ab3089a1dec43d2ed43dd6eb@buzz.block.builderlab.xyz> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
142
desktop/src/features/channels/ui/markdownDocFocus.test.mjs
Normal file
142
desktop/src/features/channels/ui/markdownDocFocus.test.mjs
Normal file
@@ -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("<!doctype html><html><body></body></html>", {
|
||||
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 <body> (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);
|
||||
});
|
||||
@@ -6,13 +6,47 @@
|
||||
* panel that held focus — in both directions focus falls to `<body>` 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<HTMLElement>(
|
||||
`[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<HTMLElement>(
|
||||
`[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(),
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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;
|
||||
};
|
||||
|
||||
/**
|
||||
|
||||
@@ -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,
|
||||
}) => {
|
||||
|
||||
Reference in New Issue
Block a user