Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Slider] Improve touch passive event handling #13623

Closed
rockmandash opened this issue Nov 17, 2018 · 12 comments · Fixed by #15703 or #22269
Closed

[Slider] Improve touch passive event handling #13623

rockmandash opened this issue Nov 17, 2018 · 12 comments · Fixed by #15703 or #22269
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@rockmandash
Copy link

https://material-ui.com/lab/slider/
You can just open chrome developer console and drag the slider, you will see all the warnings.

@oliviertassinari oliviertassinari added the component: slider This is the name of the generic UI component, not the React module! label Nov 17, 2018
@eps1lon eps1lon added the status: waiting for author Issue with insufficient information label Nov 18, 2018
@eps1lon
Copy link
Member

eps1lon commented Nov 18, 2018

Well I don't. What is your browser/os version?

I'm using Chrome Version 70.0.3538.102 (Official Build) (64-bit) on Ubuntu 18.04.1 LTS

@rockmandash
Copy link
Author

@eps1lon I have recorded the video https://youtu.be/tVmOlY-7UaE

@eps1lon eps1lon added bug 🐛 Something doesn't work and removed status: waiting for author Issue with insufficient information labels Nov 19, 2018
@eps1lon
Copy link
Member

eps1lon commented Nov 19, 2018

You forgot to mention that this is happening when the device toolbar is enabled.

This is caused by https://www.chromestatus.com/features/5093566007214080 and https://github.com/mui-org/material-ui/blob/master/packages/material-ui-lab/src/Slider/Slider.js#L326

Not sure how to resolve this yet. I guess we can remove this since we're have another non-passive listener that calls preventDefault to prevent scrolling.

@csellis
Copy link

csellis commented Feb 13, 2019

https://stackoverflow.com/questions/42101723/unable-to-preventdefault-inside-passive-event-listener

This fixed it for me
.sortable-handler { touch-action: none; }

@golestanirad
Copy link

@csellis could you PLEASE let me know where/how to add that .sortable-handler { touch-action: none; } ?

@oliviertassinari
Copy link
Member

The issue was solved, upgrade to the latest version, it should be free from this problem.

@Misiu
Copy link

Misiu commented Feb 3, 2020

@oliviertassinari the issue still exists.
Open sandbox with slider demos (https://5r8pi.csb.app/) and run audit in google chrome.
You will get this:
image

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 3, 2020

@Misiu Not possible, the blocker:

// Workaround as Safari has partial support for touchAction: 'none'.

https://caniuse.com/#search=touch-action

@Misiu
Copy link

Misiu commented Feb 3, 2020

@oliviertassinari so we must wait for Safari support? maybe leave passive only for Safari (is that possible?)
As a workaround, I switched to rc-slider but I'd like to get back to MUI

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Feb 3, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 3, 2020

As a workaround, I switched to rc-slider but I'd like to get back to MUI

@Misiu If you don't need to support iOS, sure, it's great 👌

rc-slider

In this context, I would propose that we take a step toward "touch-action: none":

diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index 3d3473c83..9d92ead64 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -125,6 +125,8 @@ const axisProps = {

 const Identity = x => x;

+const iOS = typeof navigator !== 'undefined' && /iPad|iPhone|iPod/.test(navigator.userAgent);
+
 export const styles = theme => ({
   /* Styles applied to the root element. */
   root: {
@@ -138,6 +140,8 @@ export const styles = theme => ({
     touchAction: 'none',
     color: theme.palette.primary.main,
     WebkitTapHighlightColor: 'transparent',
+    // Disable scroll capabilities.
+    touchAction: 'none',
     '&$disabled': {
       pointerEvents: 'none',
       cursor: 'default',
@@ -603,8 +607,11 @@ const Slider = React.forwardRef(function Slider(props, ref) {
   });

   const handleTouchStart = useEventCallback(event => {
-    // Workaround as Safari has partial support for touchAction: 'none'.
-    event.preventDefault();
+    if (event.cancelable) {
+      // 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.
@@ -627,7 +634,11 @@ const Slider = React.forwardRef(function Slider(props, ref) {

   React.useEffect(() => {
     const { current: slider } = sliderRef;
-    slider.addEventListener('touchstart', handleTouchStart);
+    // TODO: replace with a synthetic event, like onMouseDown.
+    // https://caniuse.com/#search=touch-action
+    slider.addEventListener('touchstart', handleTouchStart, {
+      // Workaround as Safari has partial support for touchAction: 'none'.
+      passive: !iOS,
+    });
     const doc = ownerDocument(slider);

     return () => {
diff --git a/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js b/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
index ddd794460..3645c2372 100644
--- a/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
+++ b/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
@@ -115,8 +115,7 @@ function findNativeHandler({ domTreeShapes, start, current, anchor }) {
   });
 }

-const disableSwipeToOpenDefault =
-  typeof navigator !== 'undefined' && /iPad|iPhone|iPod/.test(navigator.userAgent);
+const iOS = typeof navigator !== 'undefined' && /iPad|iPhone|iPod/.test(navigator.userAgent);
 const transitionDurationDefault = { enter: duration.enteringScreen, exit: duration.leavingScreen };

 const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect;
@@ -126,7 +125,7 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(props, ref) {
     anchor = 'left',
     disableBackdropTransition = false,
     disableDiscovery = false,
-    disableSwipeToOpen = disableSwipeToOpenDefault,
+    disableSwipeToOpen = iOS,
     hideBackdrop,
     hysteresis = 0.52,
     minFlingVelocity = 450,

@oliviertassinari
Copy link
Member

@Misiu feel free to work on it :)

@oliviertassinari oliviertassinari changed the title <Slider /> [Intervention] Unable to preventDefault inside passive event listener due to target being treated as passive. See <URL> [Slider] Improve touch passive event handling Feb 3, 2020
@oliviertassinari oliviertassinari removed their assignment May 16, 2020
mikhalev-im added a commit to mikhalev-im/material-ui that referenced this issue Aug 18, 2020
mikhalev-im added a commit to mikhalev-im/material-ui that referenced this issue Aug 18, 2020
@pierreminiggio

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
7 participants