Skip to content

[scroll area] End thumb drag when the primary button is no longer held - #5374

Merged
atomiks merged 6 commits into
mui:masterfrom
atomiks:claude/scroll-thumb-buggy-issue-1e9184
Jul 30, 2026
Merged

[scroll area] End thumb drag when the primary button is no longer held#5374
atomiks merged 6 commits into
mui:masterfrom
atomiks:claude/scroll-thumb-buggy-issue-1e9184

Conversation

@atomiks

@atomiks atomiks commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #5371

Dragging the thumb latches an internal drag flag that only a pointerup/pointercancel reaching the thumb or scrollbar clears. When pointer capture is lost mid-drag, the release is delivered to whatever element is under the cursor instead, so the flag survives the gesture and the next buttonless hover over the thumb drags it along with the cursor (the drag's scroll-snap disable also leaks).

handlePointerMove now treats a move without the primary button held as the missed release and runs the full pointerup cleanup. This mirrors the existing defense in useSwipeDismiss, which ends the gesture on buttons: 0 moves with the same "the gesture is over even if no pointerup reached us" reasoning, and the equivalent guard in SliderControl; nothing in the codebase relies on lostpointercapture for this. buttons % 2 follows the hasPrimaryMouseButton idiom since no-bitwise is enforced.

The regression test is Chromium-only because the drag math reads logical properties through getComputedStyle, which jsdom reports as empty strings. Existing raw fireEvent.pointerMove drag simulations now pass buttons: 1, matching real held-button moves.

@atomiks atomiks added component: scroll area Changes related to the scroll area component. type: bug It doesn't behave as expected. labels Jul 29, 2026
@pkg-pr-new

pkg-pr-new Bot commented Jul 29, 2026

Copy link
Copy Markdown

commit: ff8e2ee

@code-infra-dashboard

code-infra-dashboard Bot commented Jul 29, 2026

Copy link
Copy Markdown

Bundle size

Bundle Parsed size Gzip size
@base-ui/react 🔺+125B(+0.03%) ▼-1B(0.00%)

Details of bundle changes

Performance

Total duration: 1,245.23 ms -90.20 ms(-6.8%) | Renders: 78 (+0) | Paint: 1,970.27 ms -135.69 ms(-6.4%)

No significant changes — details


Check out the code infra dashboard for more information about this PR.

@netlify

netlify Bot commented Jul 29, 2026

Copy link
Copy Markdown

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit ff8e2ee
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/6a6ad15967f6960008a7978b
😎 Deploy Preview https://deploy-preview-5374--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@atomiks
atomiks force-pushed the claude/scroll-thumb-buggy-issue-1e9184 branch 2 times, most recently from 655d2d3 to 61ef255 Compare July 30, 2026 02:45
@atomiks
atomiks marked this pull request as ready for review July 30, 2026 03:02
@atomiks

atomiks commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

PR review

The fix is sound and well-tested: the buttons % 2 === 0 guard correctly treats a buttonless move as the missed release, matching the existing useSwipeDismiss/SliderControl idiom, and handlePointerUp is a byte-identical move (no behavior change from reordering). Pointer-type semantics check out — mouse, touch, and pen drags all carry bit 1 on legitimate moves, chorded release (buttons: 2) correctly ends the drag, and the double-release path is idempotent. Nothing is merge-blocking; two low-severity nits and a note below.

Bugs (2)

1. 🟡 Buttonless move from a different pointer hard-cancels an active drag

Location: packages/react/src/scroll-area/root/ScrollAreaRoot.tsx:175

if (event.buttons % 2 === 0) {
  handlePointerUp(event);
  return;
}

The guard never compares event.pointerId against the pointer that started the drag (the dragging pointerId isn't stored). Pointer capture is per-pointer, so while a touch pointer is captured and dragging the thumb, a mouse pointermove (buttons: 0) that crosses the thumb still hit-tests it and reaches this guard. It clears thumbDraggingRef, restores scrollSnapType mid-drag, and skips the capture release (the thumb holds capture for the touch pointer, not the mouse), so subsequent touch moves hit the !thumbDraggingRef.current early return and the drag freezes under the user's finger until they lift and press again.

The base code also mishandled this input, but as a transient jump the next real move corrected — the guard converts it into an un-self-correcting cancellation.

Failure scenario: On a touchscreen laptop, the user touch-drags the vertical thumb while the mouse cursor gets nudged across it (or a hovering pen passes over it during a mouse drag) — the drag dies mid-gesture.

Fix: Record the dragging pointerId in handlePointerDown and ignore moves (including the buttons guard) from other pointers.

2. ℹ️ Synthetic release skips the thumb's immediate setScrolling(false)

Location: packages/react/src/scroll-area/root/ScrollAreaRoot.tsx:176

The thumb's real release path (endDrag in ScrollAreaThumb.tsx) clears the scrolling state immediately before calling handlePointerUp; the guard calls bare handlePointerUp, so [data-scrolling] styling lingers until the SCROLL_TIMEOUT (500 ms) armed by the last real drag move fires. It cannot get stuck and matches the scrollbar track path's existing behavior — noting it only as an observable difference between the synthetic and real release paths, in case you want the guard to route through the same cleanup.

Tests (1)

1. 🟡 Scroll-snap restore on the missed-release path is untested

Location: packages/react/src/scroll-area/thumb/ScrollAreaThumb.test.tsx:306

The snap-disable leak is half of the reported bug (the drag's scrollSnapType = 'none' survives the missed release), but the new regression test only asserts viewport.scrollTop stays put. If the handlePointerUp call in the guard were later replaced with just thumbDraggingRef.current = false, the scroll assertions would still pass while the snap leak regressed.

Failure scenario: A regression that ends the drag without running the full handlePointerUp cleanup (snap restore, capture release) passes the suite.

Fix: In the new test, set an inline scroll-snap-type on the viewport and assert it is restored after the buttons: 0 move.

Verdict

Approve after nits — the fix is correct for all single-pointer cases; the multi-pointer cancellation and the missing snap-restore assertion are worth a look but neither blocks merge.


🤖 Review generated with Claude Code

@atomiks
atomiks merged commit f5a1dff into mui:master Jul 30, 2026
23 checks passed
@atomiks
atomiks deleted the claude/scroll-thumb-buggy-issue-1e9184 branch July 30, 2026 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: scroll area Changes related to the scroll area component. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[scrollarea] Dragging the thumb is frequently buggy

1 participant