Material-ui: [Slider] Touch in SwipeableDrawer

[daniel-rabe]

When the Slider is in a SwipeableDrawer the drawer moves on slider-movement on touch devices.

https://codesandbox.io/s/great-tesla-0kjx1?fontsize=14

[oliviertassinari]

Similar issue with a scrolling content: https://codesandbox.io/s/keen-cache-c7pzc.

[oliviertassinari]

@daniel-rabe What do you think of this diff?

diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index f96acaf3a..aa6fefc3a 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -552,6 +552,10 @@ const Slider = React.forwardRef(function Slider(props, ref) {
   const handleTouchStart = useEventCallback(event => {
     // Workaround as Safari has partial support for touchAction: 'none'.
     event.preventDefault();
+
+    // The slider is going to take care of this interaction. Dibs!
+    event.stopPropagation();
+
     const touch = event.changedTouches[0];
     if (touch != null) {
       // A number that uniquely identifies the current finger in the touch session.

[oliviertassinari]

However, reading https://css-tricks.com/dangers-stopping-event-propagation/, I'm wondering if we shouldn't rely on something else cc @eps1lon.

[daniel-rabe]

i tested this in our project with mui-core 3.9.3 and lab 3.0.0-alpha30 and it does not solve the problem.

i think preventDefault should already fix the drawer-sliding, but the touchEvent is passive by default and preventDefault has no effect

[oliviertassinari]

Only works with v4.

[oliviertassinari]

Alternatively:

diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index f96acaf3a..f7db34fcf 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -552,6 +552,10 @@ const Slider = React.forwardRef(function Slider(props, ref) {
   const handleTouchStart = useEventCallback(event => {
     // Workaround as Safari has partial support for touchAction: 'none'.
     event.preventDefault();
+
+    // The slider is going to take care of this interaction. Dibs!
+    event.__MUI_HANDLED__ = true;
+
     const touch = event.changedTouches[0];
     if (touch != null) {
       // A number that uniquely identifies the current finger in the touch session.
diff --git a/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js b/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
index b04e4b021..3991c1184 100644
--- a/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
+++ b/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
@@ -293,7 +293,10 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(props, ref) {
   const handleBodyTouchStart = React.useCallback(
     event => {
       // We are not supposed to handle this touch move.
-      if (nodeThatClaimedTheSwipe !== null && nodeThatClaimedTheSwipe !== swipeInstance.current) {
+      if (
+        (nodeThatClaimedTheSwipe !== null && nodeThatClaimedTheSwipe !== swipeInstance.current) ||
+        event.__MUI_HANDLED__
+      ) {
         return;
       }

[jayesh-kaza]

@oliviertassinari is this resolved? Can i work on this?

[oliviertassinari]

@jayesh-kaza Thanks for giving a start at this issue. I want to make is clear that the diff I have showcased isn't meant to be the actual solution to the problem, rather something we could explore. I believe the related issue is part of a broader problem:

1. #16565
2. #17408
3. #16942
4. #17190

I think that we should consider a solution that closes these 4 issues at the same time. It would ensure we have something that is globally optimum.

I have created a simple reproduction for issues 1, 2, and 3:

import React from 'react';
import SwipeableDrawer from '@material-ui/core/SwipeableDrawer';
import Slider from '@material-ui/core/Slider';
import Dialog from '@material-ui/core/Dialog';

export default function App() {
  const [dialogOpen, setDialogOpen] = React.useState(false);
  const [drawerOpen, setDrawerOpen] = React.useState(false);

  return (
    <div>
      <button
        onClick={() => {
          setDrawerOpen(true);
        }}
      >
        open drawer
      </button>
      <SwipeableDrawer
        open={drawerOpen}
        onClose={() => {
          setDrawerOpen(false);
        }}
        onOpen={() => {
          setDrawerOpen(true);
        }}
        anchor="right"
        variant="temporary"
      >
        <div style={{ width: 200, padding: '50px 20px' }}>
          slider issue #16565
          <Slider defaultValue={20} aria-labelledby="continuous-slider" />
          <button
            onClick={event => {
              setDialogOpen(true);
            }}
          >
            Dialog issue #17408 open dialog
          </button>
          <div style={{ marginTop: 20, overflow: 'auto' }}>
            <div style={{ width: 400, backgroundColor: 'red' }}>
              scroll container scroll containerscroll container scroll container scroll container
              scroll containerscroll container scroll container scroll container scroll
              containerscroll container scroll container
            </div>
          </div>
        </div>
      </SwipeableDrawer>
      <Dialog
        open={dialogOpen}
        onClose={() => {
          setDialogOpen(false);
        }}
      >
        <div>dialog body dialog body dialog body dialog body</div>
      </Dialog>
    </div>
  );
}

[oliviertassinari]

I propose the following solution to problem n°1 (this issue). It's based on https://css-tricks.com/dangers-stopping-event-propagation/ recommandations. I like how it reduces the bundle size and give developers some control with event.muiHandled:

diff --git a/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js b/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
index 862aaf47e..a1d63ee4e 100644
--- a/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
+++ b/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
@@ -13,16 +13,6 @@ import SwipeArea from './SwipeArea';
 // trigger a native scroll.
 const UNCERTAINTY_THRESHOLD = 3; // px

-// We can only have one node at the time claiming ownership for handling the swipe.
-// Otherwise, the UX would be confusing.
-// That's why we use a singleton here.
-let nodeThatClaimedTheSwipe = null;
-
-// Exported for test purposes.
-export function reset() {
-  nodeThatClaimedTheSwipe = null;
-}
-
 function calculateCurrentX(anchor, touches) {
   return anchor === 'right' ? document.body.offsetWidth - touches[0].pageX : touches[0].pageX;
 }
@@ -147,7 +137,6 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(props, ref) {
       if (!touchDetected.current) {
         return;
       }
-      nodeThatClaimedTheSwipe = null;
       touchDetected.current = false;
       setMaybeSwiping(false);

@@ -302,8 +291,13 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(props, ref) {

   const handleBodyTouchStart = React.useCallback(
     event => {
-      // We are not supposed to handle this touch move.
-      if (nodeThatClaimedTheSwipe !== null && nodeThatClaimedTheSwipe !== swipeInstance.current) {
+      // Example of use case: ignore the event if there is a Slider.
+      if (event.defaultPrevented) {
+        return;
+      }
+
+      // We can only have one node at the time claiming ownership for handling the swipe.
+      if (event.muiHandled) {
         return;
       }

@@ -326,7 +320,7 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(props, ref) {
         }
       }

-      nodeThatClaimedTheSwipe = swipeInstance.current;
+      event.muiHandled = true;
       swipeInstance.current.startX = currentX;
       swipeInstance.current.startY = currentY;

@@ -367,16 +361,6 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(props, ref) {
     return undefined;
   }, [variant, handleBodyTouchStart, handleBodyTouchMove, handleBodyTouchEnd]);

-  React.useEffect(
-    () => () => {
-      // We need to release the lock.
-      if (nodeThatClaimedTheSwipe === swipeInstance.current) {
-        nodeThatClaimedTheSwipe = null;
-      }
-    },
-    [],
-  );
-
   React.useEffect(() => {
     if (!open) {
       setMaybeSwiping(false);

[leMaik]

Regarding problem No 2: We should add the move listener to the paper only. That would solve the issue and also I just compared MUI's swipeable drawer with Android's native one:

MUI: swipes while swiping on the backdrop
Android: swipes only when touching the paper, not while swiping on the backdrop, but the swipe can start on the backdrop, though

[oliviertassinari]

@leMaik If you don't mind, let's move the discussion for point 2 in #17408.