[slider] Use pointer events instead of mouse events#48164
[slider] Use pointer events instead of mouse events#48164mj12albert merged 4 commits intomui:masterfrom
Conversation
Netlify deploy previewBundle size report
|
mj12albert
force-pushed
the
slider-fixes
branch
4 times, most recently
from
9e34510 to
4b2a79f
Compare
mj12albert
added
the
breaking change
mj12albert
changed the title
|
|
||
| const getFingerNewValue = ({ | ||
| finger, | ||
| move = false, |
There was a problem hiding this comment.
This move param was previously used as part of the logic to determine the active thumb for range sliders
But both pointer(mouse)Down and Move events call this, pointerDown passes move: false, and pointerMove passes move: true which is a bit indirect and hard to understand
Now we use a ref to track the pressed (or closest) thumb on pointerDown
| export type UseSliderRootSlotOwnProps = { | ||
| onMouseDown: React.MouseEventHandler; | ||
| onPointerDown: React.PointerEventHandler; | ||
| ref: React.RefCallback<Element> | null; | ||
| }; |
There was a problem hiding this comment.
Please check if the type is public, if yes, it should be documented in the breaking change doc.
There was a problem hiding this comment.
It's not, but I documented the minor breaking behavior in the doc (only applicable if using preventDefault to cancel slider drags)
| zIndex = 1; | ||
| } | ||
| } else if (active === index) { | ||
| zIndex = 1; |
There was a problem hiding this comment.
Suggestion: Worth double-checking the interaction between getThumbStyle z-index and findClosest for overlapping range thumbs.
When a range slider's thumbs sit at the same value, getThumbStyle elevates lastUsedThumbIndexRef with zIndex: 1 (visually on top). But findClosest with <= always picks the highest index on a tie. These two can disagree:
- User drags thumb 0 onto thumb 1's value, releases → thumb 0 gets
zIndex: 1(on top) - User clicks the overlap →
findClosestpicks thumb 1 (higher index wins with<=) - Wrong thumb is activated vs. what's visually on top
Might be worth checking if this is noticeable in practice, and if so whether findClosest should use lastUsedThumbIndexRef as a tiebreaker.
There was a problem hiding this comment.
Makes sense ~ fixed and added a test in 1704f14
| export const Identity = (x: number) => x; | ||
|
|
||
| const EMPTY_MARKS: readonly Mark[] = []; | ||
| const EMPTY_OBJ: EventHandlers = {}; |
There was a problem hiding this comment.
Nit: EMPTY_OBJ appears unused at runtime — extractEventHandlers() always returns an object ({}), never null/undefined, so the || EMPTY_OBJ fallback in getRootProps, getThumbProps, getHiddenInputProps is unreachable.
Could simplify to just const externalHandlers = extractEventHandlers(externalProps); and drop EMPTY_OBJ.
There was a problem hiding this comment.
Fixed, the const still has a use here
LukasTy
left a comment
LukasTy
left a comment
There was a problem hiding this comment.
Leaving a few personal nitpicks.
Looks and works great overall. 👍
Claude Opus 4.6 review
Overview
Migrates the Slider component from mouse events to pointer events, fixing several longstanding iOS/touch issues (#35887, #32737, #31869, #26754). Also modernizes internal helpers (positional args instead of objects), removes the obsolete touch-action support check, memoizes derived values, and adds pointer capture for reliable drag tracking.
Positives
- Solid modernization. Pointer events unify mouse+touch+pen handling and are well-supported in all target browsers. Removing the
doesSupportTouchActionNonefallback is the right call. - Pointer ID tracking (
activePointerIdRef) correctly prevents stray pointerup/pointermove from a second pointer from interfering with the active drag — a common multi-touch edge case. - Dual-fire prevention (
pointerDownHandledRef) is a clean approach to handling the pointerdown+touchstart double-fire on touch devices without removing the touchstart path entirely. - Memoization of
values,marks, andmarksValuesis a welcome performance improvement. - Good test coverage. New tests for unmount-during-drag, dual-fire prevention, pointer ID filtering,
getThumbStylez-index, andstep="any"handling. Identitytyped properly as(x: number) => numberinstead of(x: any) => x.
Issues & Suggestions
1. findClosest — tie-breaking with <= favors last index (behavioral change)
The change from distance < acc.distance || distance === acc.distance to distance <= acc.distance is semantically identical, but the original also had a subtle special case: when distances are equal, it picked the higher index. The new preferredIndex parameter partially addresses this for range sliders, but for initial presses without a lastUsedThumbIndex, the tie-break still always picks the last thumb. This is the same behavior as before, so it's fine — just worth a comment noting the intent.
2. UseSliderRootSlotOwnProps type is too broad
export type UseSliderRootSlotOwnProps = React.DOMAttributes<Element> & {
ref: React.RefCallback<Element> | null;
};React.DOMAttributes<Element> includes every event handler (onClick, onDrag, onScroll, etc.), which makes the type essentially unreadable for consumers who want to know what the slider actually attaches. The previous version explicitly listed onMouseDown. Consider narrowing to just onPointerDown:
export type UseSliderRootSlotOwnProps = {
onPointerDown: React.PointerEventHandler;
ref: React.RefCallback<Element> | null;
};3. Pointer capture try/catch is fine, but the empty catch could swallow real bugs
The try { event.currentTarget.setPointerCapture(...) } catch {} block silently swallows all errors. Consider at minimum a console.warn in dev mode, or at least a comment scoping the expected failure modes more precisely (the comment is good but a narrower catch would be better):
catch (err) {
// Only expected for synthetic events in tests or already-released pointers
if (process.env.NODE_ENV !== 'production') {
console.warn('MUI: setPointerCapture failed', err);
}
}4. Event cloning changed subtly
// Before:
const clonedEvent = new nativeEvent.constructor(nativeEvent.type, nativeEvent);
// After:
const clonedEvent = new Event(nativeEvent.type, nativeEvent);Using new Event(...) instead of new nativeEvent.constructor(...) means PointerEvent-specific properties (like pointerId, pointerType, pressure) won't be on the cloned event. If any consumer inspects the event type or pointer-specific fields in their onChange handler, this could be surprising. Consider using new PointerEvent(...) when the native event is a PointerEvent, or document that onChange always receives a plain Event.
5. beforeEach placement in useSlider.test.js
The beforeEach that stubs setPointerCapture/releasePointerCapture/hasPointerCapture is placed after the first describe block (line ~423 in the diff), making it run for all tests in the file but placed in an odd position. Consider moving it to the top of the describe('useSlider', ...) block for clarity.
6. Many tests now skipIf(isJsdom())
A significant number of existing tests now skip in jsdom. This reduces coverage in the fast jsdom test runner. The root cause is that jsdom doesn't support pointer capture. Since you're already stubbing the capture API in beforeEach, is there a reason these tests can't run in jsdom? If the issue is that fireEvent.pointerDown doesn't properly trigger in jsdom, that's worth documenting.
7. Minor: EMPTY_OBJ as default parameter
const EMPTY_OBJ = {};
// Used as:
externalProps: ExternalProps = EMPTY_OBJ as ExternalPropsThis is a nice optimization to avoid per-render allocations, but the as ExternalProps cast could mask type errors if ExternalProps has required properties. Low risk since these are spread into handlers, but worth noting.
Breaking Change Assessment
The migration guide entry is clear and appropriate. The only breaking change is that onMouseDown + preventDefault() no longer prevents drag — consumers must switch to onPointerDown. This is well-documented and tested.
Verdict
Well-executed modernization with good test coverage and attention to edge cases (multi-pointer, dual-fire, unmount-during-drag). The main items to address are #2 (overly broad type) and #4 (event cloning losing PointerEvent properties). The rest are minor suggestions.
|
|
||
| ### Slider | ||
|
|
||
| The `Slider` component uses pointer events instead of mouse events. Previously `onMouseDown={(event) => event.preventDefault()}` will cancel a drag from starting, now `onPointerDown` must be used instead. |
There was a problem hiding this comment.
| The `Slider` component uses pointer events instead of mouse events. Previously `onMouseDown={(event) => event.preventDefault()}` will cancel a drag from starting, now `onPointerDown` must be used instead. | |
| The `Slider` component uses pointer events instead of mouse events. | |
| Previously `onMouseDown={(event) => event.preventDefault()}` would cancel a drag from starting, now `onPointerDown` must be used instead. |
|
|
||
| if (event.defaultPrevented) { | ||
| return; | ||
| // On touch devices, the browser fires both pointerdown and touchstart for the |
There was a problem hiding this comment.
Nitpick: Wouldn't using only pointer events avoid this issue?
Or are there reasons preventing pure usage of PointerEvent?
There was a problem hiding this comment.
Wasn't 100% confident in doing this now
Technically only pointer events could work, but dropping the touch fallback could potentially break user code that sometimes expects touch events... maybe v10 😅
4 participants
Minor breaking change
Previously
onMouseDown={(event) => event.preventDefault()}will cancel the drag from starting, now can only be achieved withonPointerDown.This PR changes slider to use pointer events instead of mouse events, which is fully compatible with our browser support today, and fixes a number of issues with iOS
Other fixes:
fn(a, b)instead objectsfn({ a, b })to avoid destructuringtouch-actionsupport, no longer needed todaygetActiveElementutil to checkdoc.activeElementFixes #35887
Fixes #32737
Fixes #31869
Fixes #26754