search

radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
15.44k stars 776 forks source link

[Slider] Consider using touch-action rather than preventDefault #570

Open dbismut opened 3 years ago

dbismut commented 3 years ago

Bug report

Current Behavior

The Slider component uses event.preventDefault() when touch starts to prevent scrolling on mobile. https://github.com/radix-ui/primitives/blob/fb723f441f8c03b902f6ee511a017cbcb79060d5/packages/react/slider/src/Slider.tsx#L457

This generates two admittedly minor issues:

  1. It prevents vertical scrolling when the scroll starts from the Slider Thumb. This is acceptable when the slider is vertical, but in the case of an horizontal slider, I don't think the slider should refrain the user from scrolling vertically.

  2. TouchStart events aren't cancellable in Chrome when they're passive, and React doesn't have handlers supporting non passive events. In other words, this line generates the following warning in Chrome (responsive mode):

image

Suggested solution

Option 1 An easy fix to remove the Chrome error would be to check if the event is cancellable. It would be just ignoring the issue, but at least you don't get the red warning in the console ;)

if (event.cancelable) event.preventDefault()

Option 2 A better fix would be to avoid using preventDefault and rather use touch-action css property. You could use touch-action: pan-y on the horizontal slider (ie the browser will allow vertical scrolling) and touch-action: pan-x for the vertical slider (ie the browser will allow horizontal scrolling).

benoitgrelard commented 3 years ago

Thanks again for the suggestion David. I'll let @jjenzz catchup with the discussion and the suggestion here and we'll figure something out. 👍

asherccohen commented 2 years ago

On a side note, while working on this fix, it would be nice to have an additional onFinalChange prop that allows triggering an action only when user ends moving the slider. This acts like a debounced effect, very useful for dispatching redux actions or network mutations.

jjenzz commented 2 years ago

@asherccohen I'm not sure if we would provide an API like that. We try to provide a minimal API surface that gives you all the information you need to be able to achieve the requirements of your product. In your case, you can debounce our onValueChange yourself: https://codesandbox.io/s/wonderful-brown-xrddb?file=/App.js.

Having said that, it does sound a lot like the distinction between onChange and onInput. Our current behaviour is more like native onInput so maybe we could consider onValueInput and change our onValueChange to fire on blur (like native events). If that would be useful, please feel free to create a separate feature request.

asherccohen commented 2 years ago

Thanks @jjenzz, I added a request here https://github.com/radix-ui/primitives/issues/903