fix(messages): float hover actions above scroll containers - #393
Conversation
Promote desktop action bars to manual popovers while retaining their DOM position, keyboard focus targets, and reserved thread/continuation space. Track hover, focus and popup state; follow scrolling/resizing and hide offscreen bars. Preserve static mobile controls and continuation timestamp visibility. Add reveal lifecycle tests and browser coverage for clipping, growing emoji, keyboard order, RTL positioning, popup retention and modal stacking. Validation: 64 focused Vitest tests and 36 Chromium/WebKit browser cases; TypeScript, design checks and repository pre-commit checks. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Promote shared preview positioners to manual top-layer popovers while retaining Base UI placement, hover timing, and keyboard behavior. Replace the inline video speed fieldset with the shared non-modal radio menu so options escape thread clipping, with selection and focus return preserved. Remove the obsolete speed-menu styles and media clipping exception. Add three browser cases for stacking/hit testing, thread and fullscreen speed-menu interactions, and responsive chip previews. Both geometry regressions fail against the previous implementation in Chromium/WebKit. No browser cases removed; update component assertions for radio-menu roles. Validation: 20 focused Chromium/WebKit cases, 19 component tests, 104 design system tests, TypeScript, design checks and design builds. Native-app and human acceptance testing remain deferred. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
4bd3493 to
f2978f4
Compare
Suppress top-layer toolbars outside the active modal even when the pointer stays on Reply. Observe modal changes only while reveal is requested, preserving actions inside modal viewers and recovery after dismissal. Add keyboard Search coverage for hover with focus in the composer or toolbar, backdrop hit testing, focus return, and Reply recovery. The regression fails before the fix in Chromium and WebKit. Add unit coverage for modal lifecycle and nested confirmations; no browser cases removed. Validation: 73 focused unit tests, 8 Chromium/WebKit browser cases, TypeScript, and pre-commit checks. Native-app and human testing remain deferred. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Track slot geometry on animation frames while actions are revealed so edits or attachment growth in preceding messages cannot leave the toolbar over another message. Reuse the positioning lifecycle and skip positioning work when geometry and observed inputs are unchanged; cancel tracking on hide, mode changes, and unmount. Add one browser case for sibling growth and shrinkage without scroll events or target/scroller resizing. It fails at the previous head in Chromium and WebKit and passes with the fix. Add lifecycle coverage for frame cleanup and avoiding redundant positioning. No browser cases removed. Validation: 9 focused unit tests, 16 Chromium/WebKit cases across both affected browser files, and TypeScript. Native-app and human acceptance testing remain deferred. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Shared preview cards joined the top layer unconditionally, so a destination preview hovered before a modal opened stayed above the backdrop and still took clicks. Leave the top layer while the active modal excludes the preview's trigger, and rejoin it when the modal closes. Previews whose triggers are inside the active modal keep their top-layer placement. Share the modal check and observer with the floating message actions instead of duplicating them. Add an isolated PreviewCard + Dialog fixture and one browser case covering a background preview after keyboard modal opening and the close delay, backdrop hit testing, an in-modal preview, and recovery after dismissal. It fails at the previous head in Chromium and WebKit. No browser cases removed. Validation: 22 Chromium/WebKit cases across the overlay, floating-action, entity-destination and design-system browser files; 984 focused unit tests, 104 design-system tests, TypeScript, design checks and Biome. Native-app and human acceptance testing remain deferred. 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 automated source review
Published through Wes’s account (wesbillman). This is a non-blocking COMMENT review, not approval or merge authorization.
Reviewed head: 70c38b73c825666d95e86dde3c62964c1eec8a76
Reviewed base: c6b47a5837fd8912dc84d98bb959714a23d70818
One actionable finding (P2), inline: the new offscreen-toolbar guard is not wired into the supported selected-session-message scroller. That caller needs the same scroller marker as the timeline, thread and media-comment surfaces.
Reviewed the changed source/test files and relevant callers for reveal/cleanup, scrolling, modal layering, shared previews, video-speed menus and focus handoffs, including clipboard/report/deletion error and retry paths. No additional actionable finding from that source analysis. The public description, commit messages and changed publication material were also inspected; no additional privacy finding, and the description has no attached images.
Validation limits: source-only review of immutable snapshots; no PR code, tests, builds or app workflows executed. A one-time hosted-check snapshot at this head reports required CI, JavaScript, browser journeys/measurements and other completed checks successful; Windows native validation is skipped. These checks are not evidence that the missing selected-session scroller case works. Native/human acceptance and live focus/scroll behavior remain unverified. Existing lower-level coverage is retained and the added browser cases have layout/top-layer justifications; later commits record fail-before/pass-after claims, which I did not independently reproduce. The description’s validation paragraph still names an older revision.
| const bar = barRef.current; | ||
| const slot = slotRef.current; | ||
| if (!floating || !revealed || !row || !bar || !slot) return; | ||
| const scroller = row.closest<HTMLElement>("[data-message-scroller]"); |
There was a problem hiding this comment.
[P2] Wire the selected-session-message scroller into this visibility guard
row.closest("[data-message-scroller]") returns null for the production SessionMessageTarget caller: its SelectedMessage renders MessageRow inside the scrollable messages.feed section (src/features/sessions/SessionMessageTarget.tsx:158–177), but that section has not received the new marker. ChannelsPage.tsx:993–1013 uses this path when opening a session message outside the loaded timeline.
For a long selected message, keep its toolbar revealed by hover/focus and scroll its section: positioning still follows the slot and promotes the toolbar to the top layer, but the viewport && ... check below is skipped entirely. The actions can therefore paint and take clicks over the selected-message navigation/header after their anchor has left the message pane, rather than hiding like the other message surfaces.
Add data-message-scroller to that existing section (or pass its scroll owner through the hook) and cover this selected-session path with an offscreen/return regression.
There was a problem hiding this comment.
Fixed in 52b625b and preserved through the current-main integration at 87a808e. SessionMessageTarget now marks the existing scroll section with data-message-scroller; mounted coverage checks the marker, and the selected-session browser case covers the toolbar leaving and returning to its pane. No replacement scroll owner was added.
The selected session message pane scrolls but lacked the message scroller marker, so its floating toolbar skipped the offscreen guard. A long selected message kept revealed by hover or focus could paint its actions over the "Back to latest" navigation once the anchor left the pane, and those controls still took clicks there. Mark the pane as a message scroller so the shared positioning hides the toolbar when its anchor scrolls out and restores it on return. Add one browser case on the session-navigation fixture that grows the selected message, reveals the toolbar by hover and focus, scrolls its anchor under the navigation, and checks it is hidden, unclickable and back in place after scrolling home. It fails at the previous head in Chromium and WebKit. Assert the pane contract in the component test. Validation: 6 Chromium/WebKit cases in the session-navigation spec, 6 component tests, Biome and TypeScript. Native-app and human acceptance testing remain deferred. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
|
No remaining blocking findings after integration with main |
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
|
CI follow-up pushed as
Required hooks passed; full Vitest passed (448 files / 5,411 tests) on the resulting code before commit. Current hosted CI and human acceptance remain pending. No approval or merge is asserted. |
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
|
Pushed All 5,417 Vitest tests and 38 local Chromium/WebKit cases across layout/new-message/floating-actions pass; required gates pass. Hosted CI and human/native acceptance remain pending. #393 and #397 still require the documented focus-modality integration repair before a combined verdict; neither is approved. |
The WebKit new-message journey failed on this branch because the separate send-reveal frame could run before a restore frame on DM remount. Main already moved reveal into the restore effect (#393). Signed-off-by: Rocket <0c41329ade659e01801116549bc617351689892a382bc864a1186dc9191293a7@buzz.block.builderlab.xyz>
Integrate main through #440. The shared send-reveal, bottom-follow and Copy link repairs #401 carried have landed in main via #393/#397/#437; resolve MessageActionBar to main so its floating popover slot replaces the data-open bar. #440 sidebar preferences do not overlap #401. Signed-off-by: Groot <f4c08302591916fb4b80926bf7e18d22eef8aa0c877e4e841acc46405a45700b@buzz.block.builderlab.xyz>
Keep desktop message actions visible beyond scroll-container edges using native manual popovers, while preserving keyboard order, coarse-pointer controls, and current main's compact message-header spacing. Follow scrolling, resizing and sibling growth; hide offscreen actions and background controls behind modals; retain controls while menus or pickers are open.
Integrated through main
f3fe889e; conflict resolutions preserve the inline Copy link action, grouped-message author-line anchoring, and PreviewCard focus/identity APIs. The selected-session scroller finding is fixed with the marker and offscreen/return coverage. Added jsdom opening shims and updated geometry assertions without removing prose clearance, hit-testing or no-layout-shift checks.Review and validation
6349e6ce: integrated ci: fix two main-branch vitest failures #437 and restored its convergence/removal semantics, superseding the earlier flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) #407-only fixture expectations described below. Carried the shared timeline repair from fix(messages): stop pointer focus pinning the message action bar #397: retain launch bottom-follow until reader input, and let one rows/height observer own send reveal through delayed measurements. Newer reader input permanently cancels a pending reveal. No new timers/retries; no geometry budgets relaxed.6349e6ce, 38 Chromium/WebKit cases pass across the complete layout, new-message and floating-action files, including the hosted-failing panel resize case. Required format/lint, secret scan, types, related-unit and design gates pass. Hosted CI, native and human acceptance remain pending.Historical evidence (not current CI)
Follow-up at
755bf49d: removed the redundant Copy link span to offset the extra floating-action slot. The original cursor-paging measurement reproduced locally at 1,792 DOM nodes / 54 mounted rows, under the unchanged 1,800 limit (atcc1e55d6, before the latest main merge).Integrated Replace fixed browser-test waits with conditions, gates and the clock #373 and flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) #407. Updated only the restoration fixture to match flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) #407: pending/converged/removed anchors do not enable bottom follow without reader input, and a subsequent gesture still enables it. Independent source review clear; no production restoration change.
At
017fa28bplus that fixture change: full Vitest passed (448 files / 5,411 tests). At committed755bf49d: required format/lint, TypeScript, related-unit and design gates passed. Latest hosted CI and human acceptance remain open; earlier results below are historical, not evidence of green current CI.Independent source review found no remaining blockers. The optional timestamp tooltip overlap risk remains unverified; stale layer/branch comments were corrected.
At
87a808e6: required format/lint, TypeScript, related-unit and design checks passed. Full Vitest passed (448 files / 5,407 tests).At
10fba1d3: 76 Chromium/WebKit cases passed across floating actions, overlays, session navigation, action/management flows, nested replies, reactions, thread video and account profile. The only later change was the account-profile test fixture's missing matchMedia stub.Browser-only justification: native top-layer paint/hit testing, real scroll geometry, portal stacking and focus restoration cannot be established in jsdom. Existing cases were retained. Merge-time old-anchor/position assertions failed before updates; these runs are not a before-fix reproduction of every original feature defect.
Broad browser, native/Rust and integration suites remain with CI; human acceptance is not recorded. No approval or merge is asserted.
Try it
Hover and keyboard-focus messages at timeline/thread edges, scroll with actions open, expand a reaction, open a menu/picker, and open Search or a report/delete dialog. Actions should stay reachable without moving prose, then yield to the modal. Open a selected session message and scroll its anchor out/back: controls should hide and return.
Before / After
Before: hover toolbar clipped by scroll containers (merge base)
Watch the toolbar on the last thread reply get cut in half at the list's bottom edge, then the toolbar and the 3× quick-reaction glyph get clipped at the feed's top edge.
buzz-393-before.mp4
After: toolbar, reaction previews and video speed menu all escape their containers (branch head)
Watch the same edges with the toolbar and grown glyph fully visible and following a scroll, a reaction hover card painting above the toolbar in a narrow column, the playback-speed menu (including 2× and 1.75×) opening past the thread header, and keyboard Search hiding the toolbar behind its backdrop.
buzz-393-after.mp4