Repository navigation
fix(messages): stop pointer focus pinning the message action bar - #397
Conversation
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) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review (via Wes’s account)
No actionable findings in this revision. Non-blocking COMMENT only; not approval.
Reviewed head 0e3fa747d9dacf69c31c6f0c8028728380b2525d against base c6b47a5837fd8912dc84d98bb959714a23d70818 using a Git-blob-verified source snapshot, with no dirty source inputs.
- The two selector changes reuse the existing app-root input-modality owner: navigation keys enable keyboard reveal, pointer input clears it, and ordinary hover/open-popup selectors still keep actions available. Narrow/coarse-pointer action-bar behavior is unchanged (
Messages.module.css:1535–1566,1728–1735;useKeyboardFocusVisibility.ts:21–46). - Reviewed success/cancel focus restoration separately from error/retry behavior. Menu return and dialog focus targets remain intact. Clipboard failures retain the error toast and release the busy state for retry; report/delete errors keep their own dialog recovery. The change only stops pointer-restored focus from holding the row controls visibly open, while keyboard navigation restores reveal. These are source conclusions, not observed workflow results.
- The added browser regression checks pointer-click → hover another row → hidden actions, then Tab → visible actions. The nested-branch fixture now uses hover for pointer hit-testing without removing its collapse/expand focus assertions.
- Public-material check covered the PR description, complete three-file diff and commit message: no internal URLs/deployment identifiers or exposed secrets found in those surfaces. The description has no attached images to inspect.
Validation limits: no PR code, tests, builds or app workflows executed in this review. A one-time hosted check-runs snapshot at this head showed CI required, all twelve Chromium/WebKit browser shards, JavaScript, Rust/tool integration and DCO successful; Windows native validation was skipped. This is not independent runtime/native verification, human acceptance or merge authorization.
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 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…k 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 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
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 0e3fa74 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) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review (via Wes’s account)
One P2 correctness finding in the new menu-focus policy, inline below. Non-blocking COMMENT only; not approval.
Reviewed head 95e009e4660a38e666104ce42769edf706ffafa0 against base c6b47a5837fd8912dc84d98bb959714a23d70818. This follow-up reviews the actual code changes since the earlier reviewed head 0e3fa747d9dacf69c31c6f0c8028728380b2525d, with particular attention to the newly introduced focus-restoration override. The five changed files were checked against their Git blobs; no dirty source inputs or product edits.
Scope and evidence
- Traced the action-bar selectors, app input-modality owner, menu wrapper/callers, and Base UI 1.8.0 menu/focus implementation. The repository’s Base UI patch changes form-reset handling, not this menu behavior. The
:focus-visiblefallback and existing hover/open-popup reveals are retained. - Checked success/cancel separately from clipboard failure/retry and edit/delete/report handoffs. Clipboard rejection still clears busy state and displays the recovery notice, but mixed-input users are affected by the same focus-loss finding when trying to return to the menu. Explicit downstream dialog/editor handoffs must remain intact in the fix.
- Public-material review covered the current PR description, all five changed files, four commit messages and extracted frames across both attached demo videos. No additional internal-link, deployment-identifier or secret exposure found in those inspected surfaces. The demos use fixture content and show the Copy link hover case; they do not establish the mixed-input menu behavior.
Validation limits
Source-only: no PR code, tests, builds or app workflows were executed. The one-time hosted check-runs snapshot for this head showed JavaScript, Rust/tool integration, measurements, DCO, security checks, all six Chromium shards and four WebKit shards successful; two WebKit shards were still in progress and Windows native validation was skipped. No CI polling or wait. Script focus + Enter/Escape coverage is not an actual screen-reader or switch-access acceptance check; native/assistive behavior remains unverified. This is not human acceptance or merge authorization.
| finalFocus={!handingOffFocus} | ||
| // A pointer-opened menu hands nothing back: a trigger silently | ||
| // holding focus would pin the hover-revealed bar. | ||
| finalFocus={!handingOffFocus && !openedByPointer} |
There was a problem hiding this comment.
[P2] Preserve focus return when a pointer-opened menu switches to keyboard use
openedByPointer is latched only on open, so finalFocus remains false even after the user clicks the overflow button, navigates its items with ArrowDown, and presses Escape (or activates Copy message with Enter). Base UI initially moves focus into the popup; with finalFocus={false}, its close/unmount path deliberately selects no return target. Removing the focused popup therefore loses the user's focused message control instead of returning to the trigger. Enter can no longer reopen that message's menu, including for a clipboard retry. The ArrowDown already set data-keyboard-navigation, so this is active keyboard use, not stale pointer focus that should be hidden.
Please retain the existing boolean/no-focus-stealing and explicit-handoff safeguards, but allow return focus after keyboard interaction/dismissal rather than deciding the entire menu lifetime from the opening event. Keep the pointer-only hover fix, and add click → ArrowDown → Escape → focused trigger → Enter reopens coverage (plus the keyboard-selected clipboard failure/retry path). The new negative-focus test currently locks in the loss instead of checking a usable keyboard continuation. This also preserves the focus-restoration contract in src/shared/design-system/DESIGN.md:142–148.
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
|
Repaired the prior keyboard focus-return blocker in Independent source review clear for this repair; 28 Chromium/WebKit cases passed and required hooks pass. CI and human acceptance remain open. Integration with #393 is still pending: its JS floating-action reveal must use the same modality rule. No approval or merge asserted. |
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
|
Pushed All 5,415 Vitest tests and 60 local Chromium/WebKit cases across the five affected files pass; required hooks pass. No waits, retries or geometry budgets were loosened. Hosted checks and human/native acceptance remain pending; this is not an approval. |
Integrate main #437, keeping its restoration convergence and removal tests. Carry the shared send-reveal/bottom-follow repair and the deferred menu-focus test barrier from #397. Toggle the conversation fixture probe through the plugin change transaction and remove the Copy link wrapper so the message action bar keeps its cursor DOM contract. Signed-off-by: Groot <f4c08302591916fb4b80926bf7e18d22eef8aa0c877e4e841acc46405a45700b@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Not ready to approve/merge against current main. Reviewed head 71aece46cb2485d44cfa10c18f85f21a32383ad5 against its integrated base f3fe889eec574a4ecee9b3dfa10697378ef0faa9. During review, #393 merged as 5b6bd71d389c8171652bbc009341a2e9a3b55031. GitHub reports conflicts in MessageActionBar.tsx and Messages.module.css; independently reproduced with git merge-tree.
- Original-base result: the previous mixed-input focus finding is repaired, including keyboard clipboard retry and explicit focus handoffs. No additional material source defect found in that diff. The timeline repair is already on main through #393; preserve it rather than layering another repair.
- Integration gate: preserve #393’s floating structure and #397’s menu-close modality, and move the keyboard/
:focus-visiblereveal rule intouseFloatingActionBar. That hook treats any retained focus as revealed, so resolving only the CSS conflict leaves the original pointer-pinning defect. Keep continuation timestamps consistent. Check the combined pointer-leave, mixed-input close/reopen, clipboard retry and floating-toolbar workflows in Chromium/WebKit, then obtain fresh required CI and human acceptance. - Evidence/limits: required CI/DCO and all 12 browser shards passed for the old-base head. This review used source/caller/test inspection and merge reproduction, not new local suites or native/assistive testing. Windows native validation was skipped; two documented WebKit scroll cases remain local-only. Full diff, commit messages, PR text and sampled demo frames showed no public-material exposure in those inspected surfaces.
This is a merge/integration hold, not a newly found defect in the original-base revision. COMMENT only; no approval or merge performed.
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
The earlier integration hold is cleared. No remaining code blockers found in this follow-up. Reviewed head b1d0ab944cfea0542c35faa3cc78a3ec0fd0b4be against integrated base 5b6bd71d389c8171652bbc009341a2e9a3b55031.
- The floating hook now rechecks keyboard/
:focus-visiblemodality instead of retaining pointer focus, with matching continuation timestamps. Mixed-input menu close/retry and explicit focus-handoff protections remain intact; the timeline diff is gone. Regression coverage checks pointer leave and keyboard continuation across the combined floating-toolbar behavior. - Current main
59e87ce5de792737a93d335ea345f7d829bc61acmerges without conflicts. Localgit merge-treeproduced the same tree as GitHub’s tested merge51ec9b8aa82fba07b3fa70ae374db1ed9aee4534. Hosted JavaScript logs confirm 448 files / 5,436 tests passed on that merge; all 12 Chromium/WebKit shards, CI required, Rust/tool integration, security and DCO checks passed. - This is source and existing-CI verification, not a new local/native run. Windows native validation was skipped. Human app acceptance is not established by this review; if that smoke test is accepted, I see no further engineering gate to merging.
COMMENT only. No approval or merge performed.
Forward could leave you stuck in two ways. In its To field, the first Escape closed the people list, and every later Escape did nothing, so Forward stayed open until you used Cancel or Close. And after More message actions (⋯) → **Forward message**, closing Forward with Cancel, Close or Escape left focus on the page body instead of returning it to ⋯. With this change, the first Escape in To closes the people list and the next one closes Forward. That retires its visit, as Cancel does. Closing a Forward opened from ⋯ returns focus to ⋯. ## Before and after Left: main @ `3bcf1c4bb`. Right: this branch. Same keystrokes on both. Step 2 (⋯ → Forward → Close): on the left the focus ring is gone because focus fell to the page body; on the right it's back on ⋯. Step 3: the toolbar Forward button returns focus on both sides (unchanged). Step 6 (second Escape): on the left nothing happens and the dialog stays open; on the right it closes and focus is back on ⋯. https://github.com/user-attachments/assets/cfa6d2ad-a00e-4e52-9adf-2de1ee271b0e  | Flow | `main` | This branch | |---|---|---| | Forward, To field: Escape with the people list open | Closes the list | Closes the list (unchanged) | | Forward, To field: Escape with the list closed | Nothing happens; Forward stays open | Closes Forward; Back then Forward doesn't bring it back | | ⋯ → Forward message → Cancel, Close or Escape | Focus on the page body | Focus on ⋯ | | Message context menu → Forward message → Cancel | Focus on the page body | Focus on the menu's target: the row's first button after a right-click, or the focused button after Shift+F10 | | Toolbar Forward button → Cancel | Focus on the Forward button | Unchanged | | New message, To field: Escape | Closes an open list | Unchanged | ## The change The production diff is +22/−8 across two files. `ForwardMessage.tsx` is unchanged. **Escape (`RecipientPicker.tsx`).** The To field's wrapper and its input both handled Escape, and both swallowed it even with the list closed. Each now acts only while the list is shown: `open && !disabled`, the same condition as `Popover.Root`'s `open` and the input's `aria-expanded`. Otherwise Escape reaches the host dialog, which closes through `close()` → `navigation.dismiss`, as Cancel does. Escape counts as an explicit close under the [navigation history contract](https://github.com/block/buzz-app/blob/main/docs/navigation-history.md#passive-navigation-preserves-explicit-close-dismisses), so the visit is retired. Both gates are needed, because either handler alone still swallows the second Escape. An Escape during IME composition still can't close Forward: Base UI's `useDismiss` ignores Escape while `isComposing`. **Focus (`MessageActionBar.tsx`).** Forward returns focus to whatever had focus when it opened. From the ⋯ menu, that was the Forward menu item, which unmounts with the menu, so focus fell to the page body. The Forward menu item now focuses the menu's target (with `preventScroll`) just before it calls `onForward`: - ⋯, however its menu was opened; - on the context-menu path, the context menu's own target (`contextFocus`), if it's still connected. This mirrors the toolbar Forward button, which focuses itself, and Report in the same menu, which returns focus to ⋯ (`finalFocus={menuTrigger}` in `MessageRow.tsx`). `MessageActionBar` keeps its own ref to ⋯ when no `menuTriggerRef` is passed. `menuTriggerRef` is narrowed to a `RefObject`, which its only caller, `MessageRow`, already passes. With no connected return target, `ForwardMessage.tsx`'s `finalFocus` still returns `false`, so focus goes to the page body rather than to an unrelated control. ## Why not `finalFocus → true` The first version made Forward's `finalFocus` return `true`, and review showed that this can put focus on an unrelated control. `true` hands the choice to Base UI's fallback list, which every Base UI popup in the app shares. Each popup adds the element that had focus when it opened, and Base UI focuses the newest entry still in the page, even if it has nothing to do with this message. So ⋯ got focus back only if ⋯ itself had focus when its menu opened. Two real paths miss that: - **A pointer press on ⋯ in WebKit**, which is the macOS app's webview. WebKit doesn't focus buttons on click, which is why the toolbar's Reply and Forward buttons focus themselves. - **The message context menu.** Right-clicking text focuses no button, and the hand-off dropped the context menu's own return target. In both paths, focus after Cancel or Send jumped to an earlier control, so the next Enter or Space would activate it. In the test, that's a button called "Elsewhere" from an unrelated menu. Focusing the menu's target explicitly avoids the fallback list. ## Shift+Escape in New message The picker's modifier guard doesn't exclude Shift. With the list closed, Shift+Escape in New message's To field now reaches the app-wide Mark all as read shortcut, as it does in other fields. Plain Escape there has no ancestor handler, so New message behaves as before. Inside Forward, Mark all as read isn't available in a modal visit, so Shift+Escape only closes the dialog. ## Tests `src/features/messages/ForwardMessage.test.tsx` adds six tests. All of them fail on `main` and pass with this change: - Escape closes the people list, then the dialog, and Back then Forward doesn't bring the dialog back. On `main`, the dialog stays open after the second Escape. Reverting either picker gate on its own also fails this test. - ⋯ opened from the keyboard → Forward message → Cancel, and → Escape, Escape (`it.each`): focus returns to ⋯. On `main`, it's on `<body>`. - A menu on a button called "Elsewhere" is opened and closed. Then ⋯ is pressed without being focused (`fireEvent` with `detail: 1`, standing in for the WebKit press), then Forward message → Cancel. Focus must land on ⋯. It's on `<body>` on `main` and on "Elsewhere" with the `finalFocus → true` version. - Context menu → Forward message → Cancel, opened by right-click and by Shift+F10 (`it.each`). Focus returns to the row's first button after a right-click, and to the button that had focus after Shift+F10. | Check | Result | |---|---| | `ForwardMessage`, `NewMessage`, `NewMessage.recovery` and `MessageActionBar` tests | 150/150 | | Those four files plus the `MessageActionBar`/`MessageRow` importers (`MessageRow`, `ReportMessageDialog`, `ActivityGroupPreview`, `ThreadPanel`, `ThreadPanel.nesting`) | 470/470 at `85f2e372c` on `3bcf1c4bb`; 472/472 at this head, `00875cfd0`, where #2587 adds two `ThreadPanel` tests | | The five new focus tests, 5 runs in a row | 5/5 on each run | | `tsc --noEmit` (`tsconfig.json` and `tsconfig.design.json`), `biome check`, `git diff --check` | Pass on both heads | No browser journeys were added or removed. The component tests cover the behaviour, and the browser checks below used scratch specs that aren't committed. ## Browser checks Scratch Playwright specs ran against the channel fixture workspace in Chromium and WebKit (Linux WebKit during review, macOS Playwright WebKit locally). The recording above uses Chromium. - **Pointer: ⋯ → Forward message → Cancel.** Focus ends on ⋯ without `:focus-visible`, and the action bar hides once the pointer leaves, so #397's action-bar pinning doesn't come back. On `main`, focus lands on `<body>`. - **Keyboard close: Escape, Escape, after the menu was opened by pointer or keyboard.** Forward closes and focus returns to ⋯. Base UI marks a keyboard close as focus-visible, so after a pointer open the bar stays shown. That matches #397's own case for a keyboard dismissal of a pointer-opened menu, and the toolbar Forward button behaves the same way. On `main`, the second Escape leaves Forward open. - **A no-focus press on ⋯ (dispatched `pointerdown`/`mousedown`) and the right-click coordination row.** Focus returns to the menu's target, while on `main` it lands on `<body>`. - **Scroll.** The hand-off and Base UI's focus return both use `preventScroll`, and the scroller didn't move. No visual design changes: the diff changes only key handling in the feature-owned `RecipientPicker` and the focus hand-off in `MessageActionBar`'s existing menu. Because nothing renders differently, themes, narrow widths and enlarged text weren't re-checked. `docs/buzz-help.md` doesn't describe Escape or focus return for Forward, so it needs no change. ## Deferred checks - **Native macOS app (WKWebView): a pointer press on ⋯ → Forward message → Cancel.** That press doesn't focus ⋯, and no automated engine here reproduces it: Linux WebKit focuses buttons on click, and so did Playwright's macOS WebKit. The jsdom "Elsewhere" test stands in for it. - **Bestie conversation fixture.** Forward → Cancel already drops focus to `<body>` on `main` from every Forward entry point, including the toolbar button, and it still does on this branch. Its provider records the return target in one owner, and a different owner renders the dialog. This isn't a regression, and this PR doesn't claim to fix every Forward entry point. How the real app mounts Bestie wasn't checked. - **Not run:** Firefox and screen readers. - **Human test:** still to do, so this PR stays a draft. ## Agent review Two agent reviews ran: - **First review (the `finalFocus → true` version).** It confirmed the Escape fix, including the one-person limit, no matches, the disabled state, IME and New message. It flagged the `true` fallback, which led to the explicit hand-off above. - **Second review (the reworked version).** It found no blockers: - no #397 pinning and no scroll jump; - no conflict with `handingOffFocus` or row retirement; - the `RefObject` narrowing is safe; - the jsdom press is a fair stand-in for the WebKit press. All three of its suggestions were applied: clearer comment wording, an `it.each` over right-click and Shift+F10, and one commit with an accurate message. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: mtoohey <mtoohey@squareup.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
If you clicked a message action like Copy link, focus stayed on the button. Because of that,
.message:focus-withinkept the row's action bar visible after the mouse moved to another row, for example into the thread panel. Closing an overflow menu opened with the mouse did the same thing. The bar only went away once something else took focus.:focus-visible, not lingering pointer focus. Hovering and keyboard focus still show the bar, and an open popup still keeps it visible.message-actions.spec.mjshas a new regression check: after a Copy link click the bar follows hover, and it still appears on Tab.message-actions.spec.mjsandnested-replies.spec.mjshad steps that relied on pointer or programmatic focus to show the controls. Those steps now hover the row instead.Integration review
At
b1d0ab9, integrated main5b6bd71d, including the landed #393 floating toolbar. Resolved both merge conflicts while preserving the slot/top-layer layout, pointer-open → keyboard-close focus return, and existing handoff safeguards. The floating hook now rechecks keyboard/focus-visible modality on each update instead of treating all focused descendants as a reveal. Continuation timestamps use the same modality rules.c2bbff39: full Vitest 448 files / 5,427 tests passed; 40 browser cases passed across complete message-actions, message-actions-floating, message-overlays and message-management files in Chromium/WebKit. These checks exercise real focus, popover painting, layout and hit testing, not native desktop acceptance.59e87ce5during delivery (feat(agents): port bundled agent lifecycle to Windows #417/feat: enable packaged channel writes, encrypted recipes, and direct messages #436, agent lifecycle/native writes). A merge-tree check is conflict-free; those newer changes are not part of the locally tested snapshot. Hosted CI validates the resulting merge tree.Demo
Recorded with a scratch Playwright script against the built app in headless Chromium; the pointer is drawn in by the script because Playwright recordings do not include it. Steps: hover Thread root 1, click Reply to open the thread panel, hover the row again, click Copy link, then move the mouse onto the messages in the thread panel.
Before (
main): after the Copy link click, the left row's action bar stays pinned while the mouse hovers messages in the thread panel.before.mp4
After (this branch): the left row's action bar hides once the mouse leaves it, and only the hovered thread message shows its actions.
after.mp4
🤖 Generated with Claude Code