-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
Comments
Thank you for your hard work. Please update the progress. |
Seems like a bug. It's possible that the Also, why do you expect it to be the same though? Do you have any use case? |
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. |
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
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. |
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.
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, I will share the code you requested at the bottom. 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>
);
} |
Also, when dragging for a short time in an IOS environment, it often returns to the value before dragging. RPReplay_Final1713418950.mp4 |
Can you tell me when this issue will be fixed? |
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.movI'm labeling this as a bug and adding the |
@DiegoAndai Can you tell me when this issue will be fixed? |
Hey @Jeon-MinGyu, this issue is "waiting for upvotes", so at the moment there's no estimated time. I suggest the following:
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. |
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
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>
);
}; |
This issue is sometimes reproduced. |
@mwashief The issue is that Screen.Recording.2024-05-14.at.14.36.29.movThis didn't even require throttling. |
@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? |
I tried to fix this issue, but I can't reproduce it. Does anyone know how to reproduce it stably? |
This issue is not reproduced in the sandbox. |
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:
I have also reproduced here, using the sliders at "Sizes": https://mui.com/material-ui/react-slider/ |
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.movWith 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. |
Can this issue be fixed? |
@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. |
@DiegoAndai , 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. |
Is it possible to modify it by October 31st?? |
@Jeon-MinGyu this won't be ready by October 31st. |
Sorry to come back to this issue so late 🙈
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 🙏 |
@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. |
May I know the progress on this issue? |
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 @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. |
@good-jinu IMG_0092.MP4 |
Thanks for your response! @Jeon-MinGyu It turns out the same issue appears in my suggested code as well. I think I found a fix, so I'll open a PR soon.🔧 |
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 🙏 |
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.
Here's the video with issue 2024-12-20.8.22.19.movAnd here's the video with the fix 2024-12-20.8.26.31.mov |
Steps to reproduce
Steps:
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]
The text was updated successfully, but these errors were encountered: