Repository navigation
Limit message action hover to the bubble - #1306
Conversation
Reveal message actions over message content and the toolbar instead of the full row. Preserve keyboard, touch, and open-menu behavior, with a token-based pointer bridge to the actions. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security), and Groot (product/accessibility) reviews.
Reviewed head 888790b861ea1405e38dcf596a6a2db8256c3d8e against base f81387b496b71ae8fa339804a2891f0c7dd013dd.
8/10 — one actionable pointer hit-testing defect, detailed inline and already acknowledged in the PR’s “Before review-ready” section. The change otherwise stays with the existing toolbar owner and shared IconButton/Menu controls. Listener cleanup, supported message/attachment producers, keyboard and open-menu retention, and copy failure/retry focus paths were source-reviewed; no additional actionable defect found.
Validation remains incomplete: the new component test checks delegated events, but not actual geometry; assert retention immediately after pointerOut before a compensating pointerOver. Existing browser cases such as message-actions.spec.mjs:235 still hover the full row and need to target content under the new contract. Exercise empty-space rejection, gap traversal, and first-line link/selection access in Chromium and WebKit after the bridge fix. These are validation follow-ups, not claims that those tests failed.
Inspected the public PR description and commit message; no attached images or additional disclosure finding. This was source-only: no PR code, tests, builds, or app were run. CI was not independently assessed. Rendered hit-testing, native behavior, screen-reader behavior, theme/viewport/text-size checks, performance and merged behavior with #1298/#1300 remain unverified. The PR reports human testing; that is author-provided evidence. This COMMENT review is not approval or merge authorization.
Preserve shared overlay priority while retaining bubble-only hover activation. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Restrict the bridge to beside placement and target painted message content in browser journeys. Cover first-line clicks and selection, whitespace rejection, and actual gap traversal. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Keep reaction exclusion under the narrower message-content hover rule and preserve the new reaction and overlay regression coverage. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
A closed toolbar remains in tab order with zero opacity. Assert the user-facing contract rather than requiring visibility:hidden after focus switches between reaction pickers. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head d0ea44dde895ac3bf18af0f32ee9a08bb527becd against base a3752f05cee2e48960a5b52d34356ffea868cf73. 8/10 — one remaining P2 pointer-travel defect, detailed inline. The previous above-placement bridge interception is fixed; the beside bridge still needs to account for the toolbar’s border.
The production change remains with the existing hover owner and CSS recipe, reusing shared IconButton/Menu controls and overlay priority. Reviewed the 19-file PR diff, relevant callers, the prior hover-owner/bridge delta, reaction/overlay integration, cleanup, and success/error/retry focus paths. The revised tests add meaningful first-line click/selection and gap-travel checks without adding browser cases; the inline finding explains the edge those checks miss. Unrelated changes brought in from main were not exhaustively re-reviewed.
Inspected the public description, five commit messages, and all three attached fixture screenshots; no disclosure finding. A read-only snapshot of hosted checks on this head shows success, including CI required; Windows native validation was skipped. This was source-only: no PR code, tests, builds, or app were run. The finding is derived from CSS geometry, not a runtime reproduction. Fresh human testing, packaged desktop behavior, screen-reader behavior, and independent live validation of themes/viewports/enlarged text remain unverified. This COMMENT is not approval or merge authorization.
Use the hovered content as the toolbar anchor, retain it through menu travel, and include the existing border in the beside bridge. Cover exact edge crossing and multi-line captioned attachments in both directions. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Retain current image geometry and message behavior alongside the hover correction. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Retain both imports and current Settings test navigation; hover behavior is unchanged. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Apply the text bubble padding overlap only to text. Attachments have no equivalent inset, so their raised toolbar meets the preview edge instead of obscuring small images. The unchanged image-strip regression now passes in both engines. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
|
🤖 Commented on and resolved both fixed review threads at Morgan’s request. Investigated the remaining CI failures: the image-strip case reproduced in both browsers because the raised toolbar reused text-bubble padding over tiny attachment previews. Fixed in 1ab9474; the unchanged regression and both complete message-action files now pass, 42/42 cases across Chromium and WebKit. Text placement and beside travel are unchanged; independent review, TypeScript, and mandatory hooks passed. The other failure was HTTP 502 downloading pinned pnpm before tests started; its job was rerun. The normal push succeeded on retry without force or bypass. New-head CI is pending; the PR summary records the evidence and remaining human acceptance. |
Let the virtual list settle after text scaling before binding scrollIntoViewIfNeeded to a row. Preserve every interaction assertion; no retries, delays, or production changes. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
|
🤖 Pushed test-only synchronization correction 30e93e3. All required CI checks passed on the prior production head; the remaining advisory WebKit repeat failure tried to scroll a row while text scaling replaced it. The test now waits for the existing layout-settlement condition before scrolling. Ten repetitions in each browser passed (20/20), followed by the entire message-actions file in both engines (20/20). All assertions, retry settings, and production code are unchanged. Independent test-honesty review and mandatory hooks passed. Both review threads remain resolved; watching the new workflow run for the latest head. |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 30e93e306ccf4df8f488d72342e0e4838683e7c3 against base f31fa12dc872f2c18f3e3820903b8e6231a25596.
8/10 — one new P2 pointer hit-testing regression, detailed inline. The prior beside-border gap is corrected, and the hovered attachment now owns beside placement. The remaining issue concerns raised placement over a caption.
Follow-up review covered the corrections since d0ea44dde, the 19-file PR diff, relevant message/attachment producers, popup retention, observer/frame cleanup, and separate success/error/retry focus paths. The implementation retains the existing toolbar owner, shared IconButton/Menu controls and overlay policy. The new edge/attachment browser probes exercise meaningful native geometry; the text-scale follow-up uses observed layout settlement without weakening assertions. Unchanged areas brought in from main were not exhaustively re-reviewed.
Inspected the public description, feature commit messages and all three attached fixture screenshots; no disclosure finding. The screenshots are correctly labelled as earlier-head captures. The author updated the description during review to report completed CI. A refreshed read-only snapshot on this head shows 62 successful checks and 1 skipped (Windows native validation).
Source-only: no PR code, tests, builds or app were run. The finding is inferred from source geometry, not a runtime reproduction. Rendered hit testing, native behavior, screen-reader behavior, performance and fresh human acceptance remain unverified. This COMMENT is not approval or merge authorization.
Beside placement follows hovered content; raised placement clears the full painted message. Retain an activated hover through its content envelope without activating row whitespace or intercepting clicks. Cover narrow caption links, gap travel, exit/re-entry, and menu dismissal in both browsers. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Seed the touch-menu message before opening rather than tapping during live-append scrolling. Preserve a single native tap, no-hover activation, and unchanged menu assertions. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Integrate current sidebar, harness, Huddles, and activity changes; retain the verified message hover owner and regression coverage. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
|
🤖 Updated to f1169c4, integrating main through 2085d6d without changing the verified hover owner. No unresolved review threads remain. The touch-only setup correction now seeds its message before startup settlement; ten repetitions per engine and the full action file pass, preserving the single native tap and no-hover menu assertion. The merged branch also passed all 42 cases across the full action/floating/image-strip files in Chromium and WebKit, then mandatory push hooks. A stale local Vitest module cache briefly hid the existing Huddles guide; clearing generated cache made the unchanged test and full hook pass. Fresh CI is running on the actual pushed head; no force or hook bypass used. |
Preserve main's consistent raised placement, shared popover surface, and header clipping. Keep whole-message clearance and narrow activation; remove beside placement, pointer-anchor state, and CSS bridges. Retain only active pointer travel through measured geometry to the toolbar. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
|
🤖 Pushed conflict resolution 752b07a, merged through main 9012da3. Morgan approved main’s always-above placement combined with this branch’s bubble-only activation and whole-message clearance. Removed obsolete beside placement, pointer-anchor state, and CSS hover bridges; retained main’s shared popover surface and clipping beneath the pane header. Short-message diagonal travel was reproduced failing in both engines, then corrected using cached retention geometry—not a click-intercepting overlay or timer. Focused browser travel/caption/negative cases and all 164 component tests pass; mandatory hooks passed. The final all-affected browser run and latest hosted CI are still running on the actual pushed head. No unresolved merge conflicts remain; fresh human acceptance is not claimed. |
* origin/main: Limit message action hover to the bubble (#1306) Add chat and project creation to the Solo sidebar (#1338) Give chartreuse clear color rules for links, mentions, selection and unread (#1309) Let agent media tools read and attach any type Buzz supports (#1278) Signed-off-by: jtennant <jtennant@squareup.com> # Conflicts: # tests/browser/inbox-edit.spec.mjs
…-session-preview-live * commit 'f14605aa7aec82bd407900a9d217deb8c9f46322': Limit message action hover to the bubble (#1306) Add chat and project creation to the Solo sidebar (#1338) fix(inbox): keep tab ownership for focus in a portalled dialog test(inbox): assert settled successor focus after Archive advances Give chartreuse clear color rules for links, mentions, selection and unread (#1309) Let agent media tools read and attach any type Buzz supports (#1278) fix(channels): focus the composer of a tab a page opens (#1336) fix(inbox): focus the successor's tab when Archive advances fix(sidebar): stop the sidebar jumping and re-sorting when a channel is deleted (#1327) fix(inbox): keep focus when Inbox closes a tab in the main pane fix(inbox): keep focus when a retained tab's room becomes a Huddle feat(search): sort conversation search by newest and page results (#1206) Polish appearance controls, panel dividers, and message actions (#1337) Stop saving agent activity and metrics locally (#1347) fix(composer): place the caret after retained mentions on explicit focus (#1344)
What this does
Reveals message actions from the bubble or attachment—not the empty space across the row. Actions consistently sit above the complete message, clearing its caption and media, while clipped portions scroll beneath the pane header rather than intercepting header clicks.
Once revealed, actions remain reachable through the message's content and the narrow route to the toolbar. Entering that whitespace alone cannot reveal them, and it adds no invisible click surface. Keyboard, touch, menu behavior, and available actions remain unchanged.
How it works
Conflict resolution adopts main's always-above placement and shared popover surface, as Morgan approved. It removes the obsolete beside-placement branch, pointer-anchor state, and CSS hover bridges. Whole-message bounds and a tapered route to the visible toolbar are calculated in the existing positioning pass; pointer movement reads cached geometry without new layout measurements, timers, or overlays. Empty geometry cannot retain hover.
Merged main through
9012da3ba, preserving current message widths, workflow alignment, header clipping, appearance controls, and tab-composer focus behavior. Initial activation is still limited to painted message content, with bylines, reactions, accessories, and full-row whitespace excluded.Verification
752b07a6a: 164/164 focused component tests passed, plus TypeScript, scoped formatting, and diff checks. This corrects the zero-size geometry boundary caught by the real mounted-component tests. Mandatory commit and push hooks passed: formatting/icons, secret scan, TypeScript and related unit tests, design-system type/static guards, and destination policy. The Rust lane correctly skipped because this PR changes no Rust inputs. DCO passed. Hosted CI run 39321373458 completed successfully on exact head752b07a6a: all 63 executed check entries passed, including all browser journey/repetition shards. Windows native validation is the existing manual-only skipped check.752b07a6a: 190/190 browser cases passed across all 18 affected files in Chromium/WebKit (6.5 minutes, no retries), including short-message travel to both toolbar ends, theme/RTL, negative whitespace activation, pass-through hit testing, caption links, content gaps, menus, header clipping, image previews, nested replies, and Solo navigation. Short-message diagonal travel failed in both engines before its geometric-retention correction; failure artifacts are preserved locally.Remaining acceptance
Morgan approved combining main's always-above treatment with bubble-only activation and whole-message clearance. Fresh human testing and required reviewer approval remain pending. Latest-head hosted CI is green, the branch is mergeable, and no unresolved review threads remain. No review-completion marker, merge readiness, or packaged-desktop acceptance is claimed.
Suggested retest: hover beside a short message (quiet), hover the bubble and move diagonally onto either end of its actions, hover a PDF below a long caption and click its final-line link, and scroll actions under the pane header. The existing policy still hides actions once the message's top leaves the visible scroll area; viewport-pinned controls for arbitrarily tall messages are outside this change.
Fixture screenshots — earlier d0ea44d correction
Light and dark screenshots show the existing beside placement; the enlarged-text screenshot shows the above placement and keyboard focus. These are synthetic-data captures from the earlier passing WebKit run on d0ea44d, not evidence of the subsequent border or attachment-anchor correction.