From 0e3fa747d9dacf69c31c6f0c8028728380b2525d Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 29 Sep 2026 17:15:39 +1000 Subject: [PATCH 1/4] fix(messages): stop pointer focus pinning the message action bar Clicking a message action such as Copy link left focus on the button, so `.message:focus-within` kept the row's action bar visible after the mouse moved to another row (for example into the thread panel). Closing a mouse-opened overflow menu had the same effect. The bar only went away when something else took focus. Gate the focus reveal for the action bar and the continuation clock on `html[data-keyboard-navigation]`, the same modality flag that already gates focus rings. Hover and keyboard focus still reveal the bar; open popups still keep it visible. Add a regression check that the bar follows hover after a Copy link click and still appears on Tab, and switch two fixtures that relied on pointer or programmatic focus for the reveal to hover the row instead. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Matt Toohey --- src/features/messages/Messages.module.css | 4 ++-- tests/browser/message-actions.spec.mjs | 9 +++++++++ tests/browser/nested-replies.spec.mjs | 4 ++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/features/messages/Messages.module.css b/src/features/messages/Messages.module.css index 2d22bbcdf..e013e0326 100644 --- a/src/features/messages/Messages.module.css +++ b/src/features/messages/Messages.module.css @@ -1558,7 +1558,7 @@ html[data-keyboard-navigation] .imageReviewToolbar :focus-visible { margin-inline-start: 0; } .message:hover .messageActions, - .message:focus-within .messageActions, + html[data-keyboard-navigation] .message:focus-within .messageActions, .messageActions[data-open], .messageActions:has([aria-haspopup][aria-expanded="true"]) { opacity: 1; @@ -1730,7 +1730,7 @@ html[data-keyboard-navigation] .replyBranchRail:focus-visible { opacity: 0; } .message:hover .continuationTime, - .message:focus-within .continuationTime { + html[data-keyboard-navigation] .message:focus-within .continuationTime { opacity: 1; } } diff --git a/tests/browser/message-actions.spec.mjs b/tests/browser/message-actions.spec.mjs index f66b3dae5..59b641a02 100644 --- a/tests/browser/message-actions.spec.mjs +++ b/tests/browser/message-actions.spec.mjs @@ -58,6 +58,7 @@ test("message actions reveal, copy, restore focus and reply across responsive la expect(await page.evaluate(() => window.copiedMessages)).toEqual([ event.content, ]); + await row.hover(); await row.getByRole("button", { name: "Copy link", exact: true }).click(); await expect( page @@ -73,6 +74,14 @@ test("message actions reveal, copy, restore focus and reply across responsive la }); const root = panel.locator(`[data-message-id="${event.id}"]`); await expect(root).toBeVisible(); + // A pointer click leaves focus on the control; the bar still follows hover. + await row.hover(); + await row.getByRole("button", { name: "Copy link", exact: true }).click(); + await root.hover(); + await expect(actions).toHaveCSS("opacity", "0"); + // Keyboard focus still reveals it while the mouse is elsewhere. + await page.keyboard.press("Tab"); + await expect(actions).toHaveCSS("opacity", "1"); await root.hover(); await root .getByRole("button", { name: "React with 👍", exact: true }) diff --git a/tests/browser/nested-replies.spec.mjs b/tests/browser/nested-replies.spec.mjs index a49804d22..4d71608ca 100644 --- a/tests/browser/nested-replies.spec.mjs +++ b/tests/browser/nested-replies.spec.mjs @@ -537,8 +537,9 @@ for (const width of [1492, 1280, 1024, 390]) .first() .getByRole("button", { name: "Collapse this branch" }); const control = (await rail.isVisible()) ? rail : rowControl; + const row = panel.locator(`[data-message-id="${id}"]`); await control.scrollIntoViewIfNeeded(); - if (control === rowControl) await control.focus(); + if (control === rowControl) await row.hover(); await expect .poll(() => control.evaluate((node) => { @@ -557,7 +558,6 @@ for (const width of [1492, 1280, 1024, 390]) }), ) .toBe(true); - const row = panel.locator(`[data-message-id="${id}"]`); await panel.getByRole("textbox", { name: "Reply to thread" }).focus(); await panel.getByRole("heading", { name: "Thread", exact: true }).hover(); await row.scrollIntoViewIfNeeded(); From a667693404e5bb2c29d8bcb9ca8856c5bc0f6e6c Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 29 Sep 2026 18:27:52 +1000 Subject: [PATCH 2/4] test(messages): assert Copy link keeps focus before the Tab reveal check The action-bar regression check presses Tab after a pointer click on Copy link and expects keyboard focus to reveal the bar. That only exercises the keyboard path if focus is still on Copy link, so Tab lands on the row's next control. Copy link is briefly disabled while copying, and only the mocked clipboard resolving in a microtask stops the browser's focus fixup from dropping focus to the body first. Assert that Copy link is focused right before the Tab so a future failure points at the fixture precondition rather than at the CSS. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- tests/browser/message-actions.spec.mjs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/browser/message-actions.spec.mjs b/tests/browser/message-actions.spec.mjs index 59b641a02..78294a21b 100644 --- a/tests/browser/message-actions.spec.mjs +++ b/tests/browser/message-actions.spec.mjs @@ -75,10 +75,15 @@ test("message actions reveal, copy, restore focus and reply across responsive la const root = panel.locator(`[data-message-id="${event.id}"]`); await expect(root).toBeVisible(); // A pointer click leaves focus on the control; the bar still follows hover. + const copyLink = row.getByRole("button", { name: "Copy link", exact: true }); await row.hover(); - await row.getByRole("button", { name: "Copy link", exact: true }).click(); + await copyLink.click(); await root.hover(); await expect(actions).toHaveCSS("opacity", "0"); + // Tab only exercises the keyboard path if focus is still on Copy link, so + // it lands on the row's next control. Copy link is briefly disabled while + // copying, which would otherwise let focus fall back to the body. + await expect(copyLink).toBeFocused(); // Keyboard focus still reveals it while the mouse is elsewhere. await page.keyboard.press("Tab"); await expect(actions).toHaveCSS("opacity", "1"); From f1a3aa729238b42492a3a3d8ff328939c550ca91 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 29 Sep 2026 18:35:21 +1000 Subject: [PATCH 3/4] test(messages): name the keyboard-modality state the clock focus check relies on The nested-replies spec focuses the grandchild row's Reply button with `.focus()` and expects the same-author continuation clock to stay visible with the mouse elsewhere. Since the action bar and clock gate their focus reveal on `html[data-keyboard-navigation]`, programmatic focus only counts as keyboard focus because the earlier ArrowUp edit shortcut switched the flag on and no pointer click has fired since. Assert that the html element carries the flag right before the focus and say so in the comment, so a click added between the two fails on the named precondition rather than looking like a CSS regression. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- tests/browser/nested-replies.spec.mjs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/browser/nested-replies.spec.mjs b/tests/browser/nested-replies.spec.mjs index 4d71608ca..04fd4b0a0 100644 --- a/tests/browser/nested-replies.spec.mjs +++ b/tests/browser/nested-replies.spec.mjs @@ -194,6 +194,14 @@ test("nested replies send, collapse, and reveal through links at readable panel await expect(clock).toHaveCSS("opacity", "1"); await clock.hover(); await expect(page.getByRole("tooltip")).toContainText(/\d{4}/); + // Focus within the row only reveals the clock under keyboard modality. The + // ArrowUp edit shortcut above switched it on and nothing has clicked since + // (hovers leave it alone), so this programmatic focus reads as keyboard + // focus. Any pointer click added between the two would clear the flag. + await expect(page.locator("html")).toHaveAttribute( + "data-keyboard-navigation", + "", + ); await grandchildRow .getByRole("button", { name: "Reply", exact: true }) .focus(); From 95e009e4660a38e666104ce42769edf706ffafa0 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 29 Sep 2026 18:42:25 +1000 Subject: [PATCH 4/4] fix(messages): reveal the action bar for focus-visible focus Gating the action bar and continuation clock focus reveal on `html[data-keyboard-navigation]` hid them whenever focus moved without a navigation keydown, which is how screen readers, switch access and other assistive presses move focus. Their controls stayed focusable but sat at opacity 0. Add `.message:has(:focus-visible)` as a second reveal. Both Chromium and WebKit mark assistive and keyboard focus visible and leave pointer focus (including script focus after a click) non-visible, so the Copy link pinning fixed in 0e3fa747 stays fixed. That reintroduced one pointer path: a mouse-opened overflow menu closed with Escape handed focus back to the trigger as focus-visible and pinned the bar. A pointer-opened menu now returns focus nowhere. Base UI opens on mousedown, so a real press carries a click count while keyboard and assistive presses arrive as a click with 0. `finalFocus` stays a boolean to keep Base UI's safeguard when focus has already moved elsewhere. The edit and delete dialogs pass their own `finalFocus` and are unaffected. Cover both paths in the browser spec (Enter on a script-focused trigger then Escape reveals the bar; a mouse-opened menu closed with Escape leaves the trigger unfocused and the bar hidden) and split the unit test that expected pointer-opened menus to return focus. Each new assertion fails without its half of the change. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Matt Toohey --- .../messages/MessageActionBar.test.tsx | 14 ++++++++++- src/features/messages/MessageActionBar.tsx | 17 ++++++++++--- src/features/messages/Messages.module.css | 4 +++ tests/browser/message-actions.spec.mjs | 25 +++++++++++++++++++ 4 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/features/messages/MessageActionBar.test.tsx b/src/features/messages/MessageActionBar.test.tsx index f1a46c429..2f4277bb6 100644 --- a/src/features/messages/MessageActionBar.test.tsx +++ b/src/features/messages/MessageActionBar.test.tsx @@ -43,12 +43,24 @@ it("opens with keyboard, invokes a sibling action, and returns focus on Escape", ); expect(action).toHaveBeenCalledOnce(); await waitFor(() => expect(screen.queryByRole("menu")).toBeNull()); - await user.click(trigger); + trigger.focus(); + await user.keyboard("{Enter}"); const menu = await screen.findByRole("menu"); await waitFor(() => expect(menu.contains(document.activeElement)).toBe(true)); await user.keyboard("{Escape}"); await waitFor(() => expect(document.activeElement).toBe(trigger)); }); +it("does not hand focus back to the trigger after a pointer-opened menu", async () => { + const user = userEvent.setup(); + render( "Hello"} />); + const trigger = screen.getByRole("button", { name: "More message actions" }); + await user.click(trigger); + const menu = await screen.findByRole("menu"); + await waitFor(() => expect(menu.contains(document.activeElement)).toBe(true)); + await user.keyboard("{Escape}"); + await waitFor(() => expect(screen.queryByRole("menu")).toBeNull()); + expect(document.activeElement).not.toBe(trigger); +}); it("copies the message, shows failure, and allows retry", async () => { const user = userEvent.setup(); const write = vi diff --git a/src/features/messages/MessageActionBar.tsx b/src/features/messages/MessageActionBar.tsx index 08830b82a..350f8d9e5 100644 --- a/src/features/messages/MessageActionBar.tsx +++ b/src/features/messages/MessageActionBar.tsx @@ -55,6 +55,7 @@ export function MessageActionBar({ const busy = useRef(false); const afterClose = useRef<(() => void) | undefined>(undefined); const [handingOffFocus, setHandingOffFocus] = useState(false); + const [openedByPointer, setOpenedByPointer] = useState(false); const copy = async (text: () => string, label: string) => { if (busy.current) return; busy.current = true; @@ -113,8 +114,16 @@ export function MessageActionBar({ { - if (next) setHandingOffFocus(false); + onOpenChange={(next, details) => { + if (next) { + setHandingOffFocus(false); + // Base UI opens on mousedown, so a real pointer press carries a + // click count; keyboard and assistive presses arrive as a click + // with 0. + setOpenedByPointer( + details.event instanceof MouseEvent && details.event.detail > 0, + ); + } setOpen(next); }} onOpenChangeComplete={(opened) => { @@ -141,7 +150,9 @@ export function MessageActionBar({ data-message-id={messageId} // A boolean preserves Base UI's safeguard when focus already moved. // A callback returning true would force focus back over a newer action. - finalFocus={!handingOffFocus} + // A pointer-opened menu hands nothing back: a trigger silently + // holding focus would pin the hover-revealed bar. + finalFocus={!handingOffFocus && !openedByPointer} > { diff --git a/src/features/messages/Messages.module.css b/src/features/messages/Messages.module.css index e013e0326..385ea575b 100644 --- a/src/features/messages/Messages.module.css +++ b/src/features/messages/Messages.module.css @@ -1558,6 +1558,9 @@ html[data-keyboard-navigation] .imageReviewToolbar :focus-visible { margin-inline-start: 0; } .message:hover .messageActions, + /* Assistive tech moves focus without navigation keydowns; the engines still + mark that focus :focus-visible, while pointer focus stays non-visible. */ + .message:has(:focus-visible) .messageActions, html[data-keyboard-navigation] .message:focus-within .messageActions, .messageActions[data-open], .messageActions:has([aria-haspopup][aria-expanded="true"]) { @@ -1730,6 +1733,7 @@ html[data-keyboard-navigation] .replyBranchRail:focus-visible { opacity: 0; } .message:hover .continuationTime, + .message:has(:focus-visible) .continuationTime, html[data-keyboard-navigation] .message:focus-within .continuationTime { opacity: 1; } diff --git a/tests/browser/message-actions.spec.mjs b/tests/browser/message-actions.spec.mjs index 78294a21b..c7038aaaa 100644 --- a/tests/browser/message-actions.spec.mjs +++ b/tests/browser/message-actions.spec.mjs @@ -87,6 +87,31 @@ test("message actions reveal, copy, restore focus and reply across responsive la // Keyboard focus still reveals it while the mouse is elsewhere. await page.keyboard.press("Tab"); await expect(actions).toHaveCSS("opacity", "1"); + // Assistive presses arrive without navigation keydowns; focus alone reveals + // the bar. Clicking plain text first clears the keyboard-modality flag. + const menu = page.getByRole("menu"); + await row.getByText(event.content, { exact: true }).click(); + await page.mouse.move(0, 0); + await trigger.focus(); + // Script focus after a pointer click stays quiet. + await expect(actions).toHaveCSS("opacity", "0"); + await trigger.press("Enter"); + await expect( + page.getByRole("menuitem", { name: "Copy message", exact: true }), + ).toBeVisible(); + await page.keyboard.press("Escape"); + await expect(trigger).toBeFocused(); + await expect(actions).toHaveCSS("opacity", "1"); + // A pointer-opened menu hands focus nowhere, so the bar follows hover again. + // Waiting for the menu's deferred initial focus keeps Escape off the trigger. + await row.hover(); + await trigger.click(); + await expect(menu).toBeFocused(); + await page.keyboard.press("Escape"); + await expect(menu).toHaveCount(0); + await expect(trigger).not.toBeFocused(); + await page.mouse.move(0, 0); + await expect(actions).toHaveCSS("opacity", "0"); await root.hover(); await root .getByRole("button", { name: "React with 👍", exact: true })