[Slider] Don't move on touch move in opposite direction

[cjoecker]

• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

When a slider is on a device with touch screen, the touch move event for scrolling shouldn't overlap the drag movement of the slider.

Therefore, it will be nice, if the slider drag movement don't get fired, if there is a touch movement in the opposite direction of the slider.

Examples 🌈

Actual Behavior:

[cjoecker]

I checked the code on the slider and I think I could contribute and implement it.

For that, I will need to change the drag behavior on touch. The slider will then fire handleTouchMove only if the getFingerNewValue passes a small threshold in the direction of the slider.

What do you think?

[oliviertassinari]

@cjoecker Solving this issue is far from trivial. However, it seems that we could replicate the behavior of a native slider input on Android. Meaning, if the user tries to scroll, starting the interaction from a slider, the value would change, but the dragging behavior won't trigger, but the touch move will be dedicated to the scrolling of the window.

We would need something in this order:

diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index 7322770ea..66726975e 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -123,6 +123,10 @@ const axisProps = {
   },
 };

+// This value is closed to what browsers are using internally to
+// trigger a native scroll.
+const UNCERTAINTY_THRESHOLD = 3; // px
+
 const Identity = (x) => x;

 export const styles = (theme) => ({
@@ -135,7 +139,7 @@ export const styles = (theme) => ({
     display: 'inline-block',
     position: 'relative',
     cursor: 'pointer',
-    touchAction: 'none',
+    touchAction: 'pan-y',
     color: theme.palette.primary.main,
     WebkitTapHighlightColor: 'transparent',
     '&$disabled': {
@@ -144,6 +148,7 @@ export const styles = (theme) => ({
       color: theme.palette.grey[400],
     },
     '&$vertical': {
+      touchAction: 'pan-x',
       width: 2,
       height: '100%',
       padding: '0 13px',
@@ -395,10 +400,6 @@ const Slider = React.forwardRef(function Slider(props, ref) {
         }))
       : marksProp || [];

-  instanceRef.current = {
-    source: valueDerived, // Keep track of the input value to leverage immutable state comparison.
-  };
-
   const { isFocusVisible, onBlurVisible, ref: focusVisibleRef } = useIsFocusVisible();
   const [focusVisible, setFocusVisible] = React.useState(-1);

@@ -570,6 +571,36 @@ const Slider = React.forwardRef(function Slider(props, ref) {
       return;
     }

+    if (instanceRef.current.isSwiping === null) {
+      const dx = Math.abs(finger.x - instanceRef.current.startFinger.x);
+      const dy = Math.abs(finger.y - instanceRef.current.startFinger.y);
+
+      console.log({ dx, dy })
+
+      // We are likely to be swiping, let's prevent the scroll event on iOS.
+      if (orientation === 'horizontal' ? dx > dy : dy < dx) {
+        if (event.cancelable) {
+          console.log('preventDefault')
+          event.preventDefault();
+        }
+      }
+
+      const definitelySwiping = orientation === 'horizontal'
+      ? dx > dy && dx > UNCERTAINTY_THRESHOLD
+      : dy > dx && dy > UNCERTAINTY_THRESHOLD;
+
+      if (
+        definitelySwiping === true ||
+        (orientation === 'horizontal' ? dy > UNCERTAINTY_THRESHOLD : dx > UNCERTAINTY_THRESHOLD)
+      ) {
+        instanceRef.current.isSwiping = definitelySwiping;
+      }
+    }
+
+    if (!instanceRef.current.isSwiping) {
+      return;
+    }
+
     const { newValue, activeIndex } = getFingerNewValue({
       finger,
       move: true,
@@ -608,13 +639,14 @@ const Slider = React.forwardRef(function Slider(props, ref) {
     const doc = ownerDocument(sliderRef.current);
     doc.removeEventListener('mousemove', handleTouchMove);
     doc.removeEventListener('mouseup', handleTouchEnd);
-    doc.removeEventListener('touchmove', handleTouchMove);
+    doc.removeEventListener('touchmove', handleTouchMove, { passive: false });
     doc.removeEventListener('touchend', handleTouchEnd);
   });

   const handleTouchStart = useEventCallback((event) => {
-    // Workaround as Safari has partial support for touchAction: 'none'.
-    event.preventDefault();
+    // // Workaround as Safari has partial support for touchAction: 'none'.
+    // event.preventDefault();
+
     const touch = event.changedTouches[0];
     if (touch != null) {
       // A number that uniquely identifies the current finger in the touch session.
@@ -624,6 +656,11 @@ const Slider = React.forwardRef(function Slider(props, ref) {
     const { newValue, activeIndex } = getFingerNewValue({ finger, values, source: valueDerived });
     focusThumb({ sliderRef, activeIndex, setActive });

+    instanceRef.current = {
+      startFinger: finger,
+      isSwiping: null,
+    };
+
     setValueState(newValue);

     if (onChange) {

On iOS, the tradeoff is different. A touch start on the slider's rail doesn't trigger it.

[cjoecker]

Wow, that was fast @oliviertassinari !

It looks good to me. Are there plans to implement this in the next update?

[oliviertassinari]

@cjoecker So you confirm that the behavior of a native slider on Android/Chrome, is an acceptable tradeoff?

[cjoecker]

@oliviertassinari: you mean by

the value would change, but the dragging behavior won't trigger

that onChange will be fired but the value sent by the event will remain the same while scrolling?

[oliviertassinari]

@cjoecker Yes, the slider value changes at the touch start and then ignored if the end-user scrolls in the opposite direction of the slider.

[cjoecker]

@oliviertassinari is good for me, thanks! Much better user experience for less developer experience is a good trad-off 😉

[oliviertassinari]

@cjoecker Ok cool, well, I think that the tradeoff is purely for end-users. The options:

• we don't move the slider at the touch start, we wait to determine the intent of the user (scroll vs drag). This avoid any unintended side effect but delay the responsiveness of the slider, especially if dragging is the intent in 90% of the case. It's close to how iOS resolve the problem
• the one we have described above, the Android way. It's also how we solve the ripple issue on a button when scrolling, we interrupt.

Sounds great.

[eps1lon]

We should be very careful with custom implementations of "pan-detection". They are very hard to get right.

It might be more fruitful to check how cross-browser support for pointer-events related features is. If it matches our supported platforms we can fix this feature with minimal effort: https://codesandbox.io/s/pointer-events-are-garbage-h2p0l?file=/src/index.js

[oliviertassinari]

@eps1lon Nice demo, it's the first time I see the API in use. It also seems to behavior work fine on iOS (even without touch-action: pan-y; support). Regarding the platform support, we have https://caniuse.com/#feat=pointer. I'm not sure it's good enough yet.

[eps1lon]

Regarding the platform support, we have caniuse.com/#feat=pointer. I'm not sure it's good enough yet.

This looks fine for v5 to me.

I wouldn't spend time on this in v4 since it's not trivial to detect pan-intention. For v5 we can "use the platform".

[VasilyVP]

It seems in v5 it does not work as expected on touch screens. Is there plan to fix it?