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

[material-ui][Slider] Sometimes the value in onChange and the value in onChangeCommitted are output differently #41739

Open
Jeon-MinGyu opened this issue Apr 2, 2024 · 33 comments · May be fixed by #44795
Assignees
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material

Comments

@Jeon-MinGyu
Copy link

Jeon-MinGyu commented Apr 2, 2024

Steps to reproduce

Steps:

  1. Do a lot of drag on the slider from side to side and then stop dragging.

Current behavior

Sometimes the value in onChange and the value in onChangeCommitted are not the same.

Mui.Slider.mp4

Expected behavior

The value in onChange and the value in onChangeCommitted must be the same

Context

No response

Your environment

OS: Windows 10 10.0.19045
@emotion/react: ^11.11.4 => 11.11.4
@emotion/styled: ^11.11.5 => 11.11.5
@mui/base: 5.0.0-beta.40
@mui/core-downloads-tracker: 5.15.14
@mui/material: ^5.15.14 => 5.15.14
@mui/private-theming: 5.15.14
@mui/styled-engine: 5.15.14
@mui/system: 5.15.14
@mui/types: 7.2.14
@mui/utils: 5.15.14
@types/react: 18.2.73
react: ^18.2.0 => 18.2.0
react-dom: ^18.2.0 => 18.2.0
typescript: 4.9.5

Search keywords: [Material-ui][Slider]

@Jeon-MinGyu Jeon-MinGyu added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Apr 2, 2024
@Jeon-MinGyu Jeon-MinGyu changed the title Sometimes, the value in onChange and the value in onChangeCommitted are output differently. [Material-ui][Slider]Sometimes, the value in onChange and the value in onChangeCommitted are output differently. Apr 2, 2024
@zannager zannager added component: slider This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Apr 2, 2024
@danilo-leal danilo-leal changed the title [Material-ui][Slider]Sometimes, the value in onChange and the value in onChangeCommitted are output differently. [material-ui][Slider] Sometimes the value in onChange and the value in onChangeCommitted are output differently Apr 2, 2024
@Jeon-MinGyu
Copy link
Author

Thank you for your hard work. Please update the progress.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Apr 10, 2024

Seems like a bug. It's possible that the mouseup event, where the onChangeCommitted callback is triggered, occurs after the change event or keydown event, where the onChange callback is fired, resulting in the occasional difference in value.

Also, why do you expect it to be the same though? Do you have any use case?

@ZeeshanTamboli ZeeshanTamboli added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 10, 2024
@Jeon-MinGyu
Copy link
Author

I think the value should be the same because I get the value of onChange and the value of onChangeCommitted in the same slide action.
Also, the value of onChangeCommitted is used to use the value at the end of the drag, and onChange is used to show the value when dragging on the screen.
If it can be modified, by when can you do it?

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Apr 11, 2024
@ZeeshanTamboli
Copy link
Member

Also, the value of onChangeCommitted is used to use the value at the end of the drag, and onChange is used to show the value when dragging on the screen.

I'm not sure how this is impacting you if they're different. It could be due to the order of user events, as mentioned earlier. I am not sure whether to mark it as a bug or not. CC @mnajdova @DiegoAndai

If it can be modified, by when can you do it?

This is open for community contributions if it's a bug. You're welcome to create a pull request. Just keep in mind that this adjustment needs to be made in the useSlider hook within the Base UI repository at https://github.com/mui/base-ui.

@DiegoAndai
Copy link
Member

Hey @Jeon-MinGyu, thanks for the report! Could you share the code in your video as a minimal reproduction? This would help a lot. A live example would be perfect. This StackBlitz sandbox template may be a good starting point. I'm also curious about what's your use case in which this difference is relevant.

I agree with @ZeeshanTamboli that this is probably an edge case in the order of user events.

I am not sure whether to mark it as a bug or not.

Let's wait for the reproduction so we can test it with different browsers and CPU throttling. I would also like to test it with discrete sliders. I would say it's probably an enhancement unless it's really easy to repro, in which case it might be a bug.

@DiegoAndai DiegoAndai added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 12, 2024
@Jeon-MinGyu
Copy link
Author

Jeon-MinGyu commented Apr 16, 2024

@DiegoAndai, I will share the code you requested at the bottom.
The use case of this is to provide only the value of the location on the screen when dragging in the thermostatic UI, and to set the temperature to the value of the location only when dragging is finished.

import * as React from 'react';
import Box from '@mui/material/Box';
import Slider from '@mui/material/Slider';
import {useState} from "react";

export default function App() {
  const [slideValue, setSlideValue] = useState()
  const [newSlideValue, setNewSlideValue] = useState()

  function valuetext(value) {
    setSlideValue(value)
    return `${value}°C`;
  }

  function handleChangeSlider(){
    setNewSlideValue(slideValue)
  }

  function onSlideChange(){
    console.log('onChange Value' + slideValue)
  }

  return (
    <div style={{ width : '100%', paddingTop : '80px'}}>
      <Box sx={{ width: '50%', margin: 'auto' }}>
        <Slider
          aria-label="Always visible"
          defaultValue={80}
          getAriaValueText={valuetext}
          step={1}
          valueLabelDisplay="on"
          onChangeCommitted={handleChangeSlider}
          onChange={onSlideChange}
        />
        <div style={{fontSize: '25px', marginTop: '60px'}}>{"onChange Slider Value : " + slideValue}</div>
        <div style={{fontSize: '25px', marginTop: '20px'}}>{"onChangeCommitted Value : " +  newSlideValue}</div>
      </Box>
    </div>
  );
}

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Apr 16, 2024
@Jeon-MinGyu
Copy link
Author

Also, when dragging for a short time in an IOS environment, it often returns to the value before dragging.
The device used is iPhone X and the IOS version is 16.6.

RPReplay_Final1713418950.mp4

@teddy23-24
Copy link

Can you tell me when this issue will be fixed?

@DiegoAndai
Copy link
Member

I tested it and can confirm that it's easy to reproduce. This is on 4x slowdown throttle:

Screen.Recording.2024-04-24.at.15.03.57.mov

I'm labeling this as a bug and adding the ready to take label. If anyone is interested, feel free to work on it. IF you want this issue fixed, please upvote it by adding a 👍🏼 (thumbs up) reaction to the description.

@DiegoAndai DiegoAndai added bug 🐛 Something doesn't work waiting for 👍 Waiting for upvotes ready to take Help wanted. Guidance available. There is a high chance the change will be accepted and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 24, 2024
@Jeon-MinGyu
Copy link
Author

Jeon-MinGyu commented May 2, 2024

@DiegoAndai Can you tell me when this issue will be fixed?

@DiegoAndai
Copy link
Member

Hey @Jeon-MinGyu, this issue is "waiting for upvotes", so at the moment there's no estimated time. I suggest the following:

  • Anyone that needs this fix, please add an upvote (👍🏼 emoji) on this issue's description so we can track interest
  • If anyone has the time and wants to try and fix it, you're more than welcome and we are happy to provide guidance

In case no one takes this, we might add it to the roadmap if it has enough upvotes, but there's no certainty at the moment.

@mwashief
Copy link

I don't see any issue with the Slider component.

When we call a set state dispatcher, it may not update the state right away. In onChangeCommitted callback, we cannot assume that slideValue state has already updated and contains the newest value. The correct value is always passed as the second argument in the onChangeCommitted callback. Check the api for reference.

onChangeCommitted might be used like this:

import Slider from '@mui/material/Slider';
import {useState} from "react";

export default function App() {

const [slideValue, setSlideValue] = useState();
const [newSlideValue, setNewSlideValue] = useState();

return (
  <div style={{ width : '100%', paddingTop : '80px'}}>
    <Slider
      defaultValue={30}
      onChange={e => setSlideValue(e.target.value)}
      onChangeCommitted={(e, v) => setNewSlideValue(v)}
      /> 
    <div style={{fontSize: '25px', marginTop: '60px'}}>{"onChange Slider Value : " + slideValue}</div>
    <div style={{fontSize: '25px', marginTop: '20px'}}>{"onChangeCommitted Value : " + newSlideValue}</div>
  </div>
  );
};

@Jeon-MinGyu
Copy link
Author

This issue is sometimes reproduced.

@DiegoAndai
Copy link
Member

@mwashief The issue is that onChangeCommitted should match the last dispatched onChange, and this is not always the case. Here's the issue presenting itself when running the code you shared:

Screen.Recording.2024-05-14.at.14.36.29.mov

This didn't even require throttling.

@mwashief
Copy link

mwashief commented May 14, 2024

@DiegoAndai, interesting! I wasn't able to reproduce the issue using the code I shared. Check out this sandbox.

Could you double check, if you actually ran my code?

@LongHaoo
Copy link

LongHaoo commented May 16, 2024

I tried to fix this issue, but I can't reproduce it. Does anyone know how to reproduce it stably?

@Jeon-MinGyu
Copy link
Author

This issue is not reproduced in the sandbox.
It happened frequently when I checked with Ios after code execution.

@kriskate
Copy link

I am also facing the same issue (the one where the values become similar to the one when dragging started)

The easiest way to reproduce for me is:

  • ios > safari (simulator or device)
  • dragging from a small value to a big one (ie: 10>50>100) or the other way around (100>50>10) and NOT small>big>small

I have also reproduced here, using the sliders at "Sizes": https://mui.com/material-ui/react-slider/

@DiegoAndai
Copy link
Member

Now testing with a more powerful computer, it's harder to reproduce. But I was able to reproduce on @mwashief's sandbox on 6x CPU throttle:

Screen.Recording.2024-06-05.at.16.53.38.mov

With a more powerful computer, it isn't as big of a problem. Seems like going outside of the boundaries is a way to trigger it easier.

@Jeon-MinGyu
Copy link
Author

Can this issue be fixed?
If it is fixed, when will it be possible?

@DiegoAndai DiegoAndai added this to the Material UI with Base UI milestone Jul 2, 2024
@DiegoAndai
Copy link
Member

@Jeon-MinGyu, please upvote the issue if you wish this to land. I can't provide an expected timeline at this moment; I'm sorry.

Also, if anyone wants to work on it right now, please go ahead. I would be more than happy to guide it.

@Jeon-MinGyu
Copy link
Author

@DiegoAndai , I'd like to know how many upvotes to need for the issue this to land.
I need a modification schedule for this issue

@DiegoAndai
Copy link
Member

I'd like to know how many upvotes to need for the issue this to land

I have no answer to that. We pick what issues to work on by picking the most upvoted ones, so there's no exact number.

If you need this urgently, I encourage you to look into it and open a PR. I would be happy to guide you.

@oliviertassinari oliviertassinari removed this from the Material UI with Base UI milestone Sep 29, 2024
@Jeon-MinGyu
Copy link
Author

Is it possible to modify it by October 31st??
If it's impossible, we'll use it as it is after organizing it.

@DiegoAndai DiegoAndai added this to the Material UI with Base UI milestone Oct 2, 2024
@DiegoAndai
Copy link
Member

@Jeon-MinGyu this won't be ready by October 31st.
If you need it urgently, I encourage you to propose a solution and open a PR. I would be happy to guide you.

@mj12albert
Copy link
Member

Sorry to come back to this issue so late 🙈

Seems like going outside of the boundaries is a way to trigger it easier

I could never reproduce the issue fully (or easily, even with devtools throttling), but while reworking the Base UI slider I did smooth out some behavior when the thumb is rapidly dragged outside the track area (before releasing the click or touch)

Here's the same repro using the latest Base UI slider (it's basically a heavily reworked version of Material UI slider's internals): https://master--base-ui.netlify.app/experiments/slider-change-committed-lag/

For those encountering this do let me know if there is any noticeably difference 🙏

@Jeon-MinGyu
Copy link
Author

@mj12albert Thank you for answering the issue.

As a result of checking, when you slide quickly, the onValueChange and onValueCommitted values are often displayed differently.

@Jeon-MinGyu
Copy link
Author

May I know the progress on this issue?

@good-jinu
Copy link

good-jinu commented Dec 15, 2024

I reproduced the issued.

I think that sample code @Jeon-MinGyu shared is not a good way to control the Slider.

You may not change the value by valuetext - getAriaValuetext, but change it to onChange.

@Jeon-MinGyu 's code

  //...

  function valuetext(value) {
    setSlideValue(value)
    return `${value}°C`;
  }

  function handleChangeSlider(){
    setNewSlideValue(slideValue)
  }

  function onSlideChange(){
    console.log('onChange Value' + slideValue)
  }

  //...

I would rather suggest this.

import * as React from 'react';
import Box from '@mui/material/Box';
import Slider from '@mui/material/Slider';
import {useState} from "react";

export default function App() {
  const [slideValue, setSlideValue] = useState()
  const [newSlideValue, setNewSlideValue] = useState()

  function valuetext(value) {
    return `${value}°C`;
  }

  function handleChangeSlider(event, value){
    setNewSlideValue(value)
  }

  function onSlideChange(event, value){
    console.log('onChange Value' + value);
    setSlideValue(value)
  }

  return (
    <div style={{ width : '100%', paddingTop : '80px'}}>
      <Box sx={{ width: '50%', margin: 'auto' }}>
        <Slider
          aria-label="Always visible"
          defaultValue={80}
          getAriaValueText={valuetext}
          step={1}
          valueLabelDisplay="on"
          onChangeCommitted={handleChangeSlider}
          onChange={onSlideChange}
        />
        <div style={{fontSize: '25px', marginTop: '60px'}}>{"onChange Slider Value : " + slideValue}</div>
        <div style={{fontSize: '25px', marginTop: '20px'}}>{"onChangeCommitted Value : " +  newSlideValue}</div>
      </Box>
    </div>
  );
}

@Jeon-MinGyu, Please let me know if the issue reoccurs.

CC. @DiegoAndai @mj12albert

@Jeon-MinGyu
Copy link
Author

@good-jinu
Thank you for your answer.
However, the values of 'onChange Slider Value' and 'onChange Committed Value' are still not the same during slide operation.
The attached video is the result of executing the above code.

IMG_0092.MP4

@good-jinu
Copy link

Thanks for your response! @Jeon-MinGyu

It turns out the same issue appears in my suggested code as well.
To reproduce the issue, just move the Slider quickly.

I think I found a fix, so I'll open a PR soon.🔧

@mj12albert
Copy link
Member

mj12albert commented Dec 19, 2024

Thanks @good-jinu for working on this 🙌

Here's a sandbox (updated an earlier one from this thread) with the fix: https://codesandbox.io/p/sandbox/nice-ully-k38gjr

I can confirm that the values reported by both callbacks are much more consistent on iOS when flicking the the slider thumb around

Would be great if somebody could test this out with a mouse and CPU throttling, as I could never reproduce this with a mouse 🙏

@good-jinu
Copy link

Thanks for your response! @mj12albert

For me, CPU throttling did not reproduce the issue. However, using a wireless trackpad did. The built-in trackpad did not exhibit the problem, but the wireless trackpad did.

Here's the list of devices which reproduce the issue for me.

  • MacBook wireless trackpad
  • Android

Here's the video with issue

2024-12-20.8.22.19.mov

And here's the video with the fix

2024-12-20.8.26.31.mov

@mj12albert mj12albert removed waiting for 👍 Waiting for upvotes ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Dec 23, 2024
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! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging a pull request may close this issue.