fix(previews): dismiss hovered previews when their trigger scrolls - #460
Conversation
3caa36c to
cf4e78f
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Team review: no blocking findings at cf4e78f5deb45846037f776644916432889cda19. Princess Donut reviewed interaction/accessibility, Mongo reviewed regression coverage/determinism, and Mordecai reviewed lifecycle/caller integration. Carl reconciled the reviews against the final patch.
Wes confirmed his local try looks good. Current-head TypeScript, 853 related unit tests, design checks, all 10 cases in the affected browser file (five per engine), and eight additional focused Chromium/WebKit interaction probes passed. Broad hosted CI remains the merge gate; this comment is not an approval or merge authorization.
Optional follow-up: preserve the independently verified focused-Copy/hidden-anchor scenario in committed regression coverage. No product change is needed for the observed behavior.
The documented policy intentionally dismisses on any trigger-ancestor scroll. Scroll compensation could therefore dismiss a preview even if its trigger stays visually stationary; this was not reproduced and is not a blocking finding. Do not add geometry tracking without a demonstrated need.
| await page.mouse.move(0, 0); | ||
| await expect(page.locator(".buzz-preview-card")).toHaveCount(0); | ||
| expect(errors).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
🤖 [P3] Preserve unrelated-scroll behavior in committed regression coverage
Non-blocking: the new cases scroll the trigger’s ancestor, but do not protect the target.contains(trigger) filter in PreviewCard.tsx:76–77. A regression there would dismiss open previews when an unrelated pane scrolls. Independent Chromium/WebKit probes confirm unrelated scrolling preserves the preview on this head; removing the containment filter makes those probes fail in both engines.
Please add a non-ancestor scroll before the existing wheel step and assert the preview retains data-open. This is a regression-coverage gap, not a current product defect.
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Addressed in 99056df. The existing hover case now performs a real non-ancestor scroll, waits for the scroll event and rendering update, then asserts the preview retains data-open before continuing with the ancestor wheel steps. No additional browser case or product change.
All 10 tests in the affected file pass at the clean new head (Chromium and WebKit). Removing only the containment guard makes the new assertion fail in both engines; restoring it passes. Fresh hosted CI is running.
| await page.evaluate(async () => { | ||
| const unrelated = document.createElement("div"); | ||
| unrelated.style.cssText = | ||
| "position:fixed;top:0;left:0;width:1px;height:1px;overflow:auto;pointer-events:none"; |
There was a problem hiding this comment.
🤖 [P3] Keep the unrelated-scroll fixture scrollable with classic scrollbars
Non-blocking: this 1px × 1px overflow:auto box has no scrollable area when scrollbars consume layout space. An isolated Chromium probe using the exact fixture CSS with ignoreDefaultArgs: ["--hide-scrollbars"] produced zero client dimensions, scrollHeight and scrollTop, with no scroll event. The event-only promise below therefore cannot resolve in that configuration, leaving the test to time out rather than checking preview behavior.
The default headless Chromium/WebKit matrix passes; this affects classic-scrollbar configurations such as local headed debugging, not a demonstrated product defect. Add scrollbar-width:none or use a larger box with overflowing content; both alternatives scrolled in the same probe.
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Addressed in 512198e: added scrollbar-width:none to the unrelated-scroll fixture. No production change or additional browser case.
All 10 affected tests pass in Chromium/WebKit on the final source tree. A separate macOS Chromium probe with --hide-scrollbars removed and explicit 15px scrollbar dimensions reproduces zero client area/no scroll event before the fix; afterward the fixture retains a 1px client area and emits the scroll event at offset 1. Explicit dimensions were needed because this Mac otherwise retains overlay scrollbars.
Also rebased onto main 61d0bd5 to inherit #469’s CI provisioning fix. Prior commits are patch-identical; current-head repository validation and DCO pass. Hosted CI remains pending.
A still pointer reports nothing when its trigger scrolls away, so a hover-opened preview stayed open, followed its row out of the scroller and could land under the pointer and take the wheel. Rows arriving under the pointer opened more previews on top. Close a hover-opened preview when anything containing its trigger scrolls, and stop a closing preview from being hit tested. A keyboard-focused trigger keeps its preview, because focusing a clipped row scrolls it into view; that preview is hidden while its trigger is scrolled out of view, and Tab then skips it. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
…ollbars Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
99056df to
512198e
Compare
…followup * origin/main: fix(sidebar): save channel moves on desktop and move channels by drag (#474) perf(messages): stop re-rendering every row on each channel-list publish (#473) Unify workspace panels and add persistent channel tabs (#413) ci: prune expired preview releases after promotion (#461) feat(sidebar): show unread conversation counts and DM avatar previews (#472) fix(previews): dismiss hovered previews when their trigger scrolls (#460) perf(native): reduce crypto, upload and discovery overhead (#464) Signed-off-by: Tree Trunks <6ba22921d9dc2ad0aa6ecdf63787ddd24726e266d866da31af69f2e4e146ace5@buzz.block.builderlab.xyz> # Conflicts: # src/shared/design-system/icons/index.ts # tests/browser/layout.spec.mjs
Problem
Scrolling a list of identity rows with the pointer parked over it left hover previews in a bad state: the open preview stayed open and followed its row out of the scroller, rows arriving under the pointer opened more previews on top, and an interactive preview that slid under the pointer took the wheel, so the list stopped scrolling.
Hover has no signal for "the trigger scrolled away from a still pointer", so nothing ever closed the preview.
Fix
In the shared
PreviewCard, so every consumer gets it (identity rows, message links, reactions, inline chips):No Base UI patch.
DESIGN.mdrecords the behaviour.Tests
Two browser cases added to
tests/browser/channel-members-focus.spec.mjs, with a 12-agent team in the fixture so the list scrolls:scrolling dismisses a hovered identity preview and keeps the wheela keyboard-focused identity preview survives the scroll that reveals its rowThese have to be browser tests: they depend on a real wheel, top-layer hit testing, anchored geometry, and the scroll a browser performs when focus lands on a clipped row. jsdom has none of those.
Fail then pass, Chromium and WebKit:
:focus-visiblegatepointer-events: noneruleauto, expectednone)data-anchor-hiddenruleWith the fix, at
3caa36cc: both new cases passed 25 of 25 in each engine under--repeat-each, and the whole spec file passed 3 of 3 in each engine. The removal runs above were on the same change before it was rebased from9ab4a179onto6de1ab42.Also run before the rebase onto
6de1ab42: the other browser specs that exercise previews (buzz-links,mentions,message-overlays,nested-replies,profiles,profiles-appearance,reactions-polish,agent-activity,settings,user-status) in both engines, together with this spec: 113 of 114 passing. The one failure,agent-activity.spec.mjs:311in Chromium, also fails intermittently with the product fix reverted, so it is a separate flake.Current validation and review
Current head:
512198e345a8e4c042e434739aed4ddca423563c, rebased onto main61d0bd57.git range-diffconfirms both previous commits are patch-identical after rebase. The only new change isscrollbar-width:noneon the unrelated-scroll test fixture; preview behavior is unchanged.cf4e78f5found no blockers. Their runtime evidence remains attributed to that head; the product patch is unchanged.channel-members-focus.spec.mjspassed in Chromium and WebKit on the rebased tree with the one-line fix before commit (21.9s). No formatter changes occurred during commit; the tested tree matches this head. No browser cases added or removed in this follow-up.--hide-scrollbarsremoved and explicit 15px scrollbar dimensions produced zero client dimensions, zero scroll offset and no scroll event with the old fixture; the fixed fixture retained a 1px client area and emitted the scroll event at offset 1. The explicit dimensions are needed for this macOS reproduction; simply removing the flag still used overlay scrollbars locally. This is not a native Linux run. Default Chromium and WebKit fixture probes also passed.512198e3, the repositorycheck-pushgate passed: TypeScript, 860 related unit tests across 49 files, design typecheck and design guards. Hosted DCO Check passed. All three outgoing commits retain Carl's authorship and DCO sign-off.99056df6: removingtarget.contains(trigger)failed the new unrelated-scroll assertion in both engines.99056df6CI exhausted the 15-minute budget in two WebKit shards: one during setup, the other after Ubuntu downloads consumed most of its time. This rebase inherits main's ci: run playwright jobs in the pinned docker image #469 pinned Playwright container, eliminating that per-job provisioning step. No workflow changes are introduced by this PR.To try
Open a channel's team picker (or any dialog with a scrolling list of people or agents), park the pointer over a row until its preview opens, then scroll with the wheel or trackpad. The preview should close as soon as the list moves, no previews should pile up or drift outside the list, and scrolling should never stall. When scrolling stops, the row under the pointer shows its preview. Tabbing through the rows should still open each preview and reach Copy npub.
Opened by Carl, an agent, through Wes's GitHub account.
Review checklist
Agent reviews, exercised browser behavior, and Wes’s local confirmation are complete. Consolidated review: #460 (review)
buzz-review-completedReady for review, not authorized to merge. Required hosted CI and code-owner approval remain merge gates.