material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[Slider] Firing extra `onChange` with incorrect value after `ChangeCommitted` on mobile

Open robwheatley opened this issue 3 years ago • 15 comments

Duplicates

  • [X] I have searched the existing issues

Latest version

  • [X] I have tested the latest version

Current behavior 😯

Tested on iOS 15.3 and 15.4. The bug appears in the latest Chrome and Safari apps

When using the Slider component, if you only slide by a few steps, then after the onChangeCommit an 'extra' onChange is triggered with the original start value in, resetting the slider back to where it was before the user interaction. Larger changes don't trigger this error.

Here's a very simple demo: https://5xdrfw.csb.app/ There's a bit of logging so you can see the extra onChange trigger. To see the console logs on mobile chrome, open a new tab and go to chrome://inspect and click the button to start logging.

Note that you have to be using a mobile device to trigger this. If you make a large change (e.g. swipe all the way to the left/right) then it works fine, but if you make a smaller change, then the slider 'springs back' to where you started due to an erroneous onChange firing after the onChangeCommitted.

Expected behavior 🤔

No matter how big the change, it should 'stick.

Steps to reproduce 🕹

To reproduce, use the demo above and start swiping. You can even go to the MUI demo page here as it suffers the same problem https://mui.com/components/slider/#main-content

Context 🔦

No response

Your environment 🌎

No response

robwheatley avatar Mar 18 '22 09:03 robwheatley

I can't seem to reproduce. I am trying Safari on iPhone XS Max, and this is the output:

change value 46
8demo.js:13 change value 46
demo.js:13 change value 47
demo.js:16 Commit value 47
demo.js:13 change value 49
15demo.js:13 change value 48
demo.js:16 Commit value 48
demo.js:13 change value 50
5demo.js:13 change value 50
4demo.js:13 change value 51
demo.js:16 Commit value 51

mnajdova avatar Apr 05 '22 09:04 mnajdova

@mnajdova Thanks for looking.

I just gave it another go and can still reproduce it on my iPhone 12 Pro in Safari and Chrome using my demo - although it's more reliably reproduced using Chrome.

Did you try the Docs page too or just my demo. It seems to be more obvious using that page: https://mui.com/components/slider/#main-content

robwheatley avatar Apr 05 '22 10:04 robwheatley

Since the issue is missing key information, and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

github-actions[bot] avatar Apr 13 '22 00:04 github-actions[bot]

Just by putting the demo in documentation to sandbox it clearly reproduces the issue, same exact behavior on latest ios 15.4, while there is no such issue on android. Have also tested slider with the old mui4, it works fine on both ios and android.

fuyouqie avatar Apr 13 '22 04:04 fuyouqie

@mui/core can anyone with iPhone try to reproduce this?

mnajdova avatar Apr 15 '22 10:04 mnajdova

@mnajdova Hi, may I know which IOS version is running on your iphone XS? I have tested the demo app on IOS 12.5 and no such flickering issue happens. It is probably due to the newer IOS version somehow changing support for a certain feature resulting different behavior in differnt IOS version and also android. This is not something new in IOS though, have encountered similar issues back when developing webrtc-streamer. Meanwhile I will also find a mac pc or laptop in the next couple of days to debug IOS safari and reproduce it with debug outputs.

konnextadmin avatar Apr 19 '22 01:04 konnextadmin

@mnajdova I was able to reproduce it on Edge on my 11 Pro running iOS 15.4.1 I'll take a look at what may be the cause here.

michaldudak avatar Apr 26 '22 09:04 michaldudak

On this topic of change, I would also like to fix this regression: https://github.com/mui/material-ui/pull/28472#pullrequestreview-758136704. I had it on my to-do list for a long time but never found the time.

oliviertassinari avatar Apr 26 '22 12:04 oliviertassinari

The problem exists because mobile Safari fires mousedown as a way to emulate mouse events. The createHandleMouseDown function registers the handleTouchEnd as an event handler for the second time (first for touchend, and now mouseup). We could try using pointer events instead of mouse and touch events and make sure they are registered just once. Would anyone here be interested in giving it a try?

michaldudak avatar Apr 26 '22 12:04 michaldudak

@michaldudak Where you asked if people wanted to give it a try, did you mean other MUI contributors who could hack the change in locally, or 'anyone'?

I'd be happy to give it a go, but I would need pointers on how to install the modified version (or on how to modify it myself).

My app is built on Meteor, but things are installed using NPM (if that helps in anyway!).

Rob.

robwheatley avatar Apr 26 '22 12:04 robwheatley

Anyone can be a contributor and we encourage community members to work on issues. Take a look at the contributing guide and let me know if anything is unclear. As to how to build a custom version and use it in your app - don't do it at first. Create a small reproduction or just observe the demos as you make changes to the code.

Later, if you want to test the changes in your app, you may build the Material UI packages locally by running yarn release:build. Take the build output from /packages/mui-material/build and replace the copy from your app's node_modules with it.

michaldudak avatar Apr 26 '22 17:04 michaldudak

The problem exists because mobile Safari fires mousedown as a way to emulate mouse events. The createHandleMouseDown function registers the handleTouchEnd as an event handler for the second time (first for touchend, and now mouseup). We could try using pointer events instead of mouse and touch events and make sure they are registered just once. Would anyone here be interested in giving it a try?

Hi, I am trying to solve this problem. I found that it is possible to prevent mousedown from triggering by disabling the default behavior in the createHandleMouseDown function. But this way has a new problem, that is, when the browser page size changes and switches to the mobile terminal or the computer terminal. This logic does't change in real time, it only takes effect on the first page load. Then to judge whether it is suitable for mobile devices to solve?

https://github.com/coder-lcn/material-ui/blob/aa3ad2fc71fe2822141af411a47615c013f8f34a/packages/mui-base/src/SliderUnstyled/useSlider.ts#L557

coder-lcn avatar Aug 24 '22 05:08 coder-lcn

@coder-lcn, sorry for the late response. Have you tried using pointer events as I suggested in https://github.com/mui/material-ui/issues/31869#issuecomment-1109716626?

michaldudak avatar Oct 11 '22 15:10 michaldudak

My workaround for my application was to return on mousedown events.

const handleChange = (event, newValue) => {
        if (event.type === 'mousedown') {
            return;
        }
....
}

ConnorsApps avatar Dec 06 '22 03:12 ConnorsApps

The workaround suggested in @ConnorsApps comment above will break click functionality for normal mouse users, which my application needs to work, so I added detection of iOS to the below code, and only discard the click event if a device is iOS. (iOS detection code from https://stackoverflow.com/questions/9038625/detect-if-device-is-ios/9039885#9039885 )

function iOS() {
  return [
    'iPad Simulator',
    'iPhone Simulator',
    'iPod Simulator',
    'iPad',
    'iPhone',
    'iPod'
  ].includes(navigator.platform)
  // iPad on iOS 13 detection
  || (navigator.userAgent.includes("Mac") && "ontouchend" in document)
}

const isIOS = iOS();

const handleChange = (event, newValue) => {
        if (isIOS && event.type === 'mousedown') {
            return;
        }
  // Otherwise handle your change event as normal
}

Sweater-Baron avatar Dec 12 '22 21:12 Sweater-Baron

@Sweater-Baron, Great catch, however navigator.platform is Deprecated. I'm going to use the following

// FROM https://stackoverflow.com/questions/9038625/detect-if-device-is-ios
const iOS = () => {
    const platform = navigator.userAgentData?.platform || navigator.platform;

    return [
      'iPad Simulator',
      'iPhone Simulator',
      'iPod Simulator',
      'iPad',
      'iPhone',
      'iPod'
    ].includes(platform)
    // iPad on iOS 13 detection
    || (navigator.userAgent.includes("Mac") && "ontouchend" in document)
}
const isIOS = iOS();
.....
    const handleChange = (event, newValue) => {
        // Slider component workaround for Safari https://github.com/mui/material-ui/issues/31869
        if (event.type === 'mousedown' && isIOS) {
            return;
        }
       // Do Stuff
    };

ConnorsApps avatar Dec 19 '22 14:12 ConnorsApps