mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.08k stars 32.05k forks source link

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

Open robwheatley opened 2 years ago

robwheatley commented 2 years ago

Duplicates

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

mnajdova commented 2 years ago

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
robwheatley commented 2 years ago

@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

github-actions[bot] commented 2 years ago

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.

fuyouqie commented 2 years ago

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.

mnajdova commented 2 years ago

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

konnextadmin commented 2 years ago

@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.

michaldudak commented 2 years ago

@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.

oliviertassinari commented 2 years ago

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.

michaldudak commented 2 years ago

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?

robwheatley commented 2 years ago

@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.

michaldudak commented 2 years ago

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.

coder-lcn commented 2 years ago

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

michaldudak commented 1 year ago

@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?

ConnorsApps commented 1 year ago

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

const handleChange = (event, newValue) => {
        if (event.type === 'mousedown') {
            return;
        }
....
}
Sweater-Baron commented 1 year ago

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
}
ConnorsApps commented 1 year ago

@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
    };