[Slider] Set onChangeCommitted to receive the last argument passed to onChange #44795
Conversation
Netlify deploy previewhttps://deploy-preview-44795--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
mnajdova
added
bug 🐛
good-jinu
force-pushed
the
slider-on-change-on-change-committed
branch
3 times, most recently
from
7e07151 to
ab1e076
Compare
- Add test case for differing values passed to onChange and onChangeCommitted in Slider
- Set onChangeCommitted callback in Slider to receive the last argument passed to onChange
good-jinu
force-pushed
the
slider-on-change-on-change-committed
branch
from
ab1e076 to
51f1489
Compare
github-actions
bot
added
the
PR: out-of-date
|
Here's the list of devices which reproduce the issue for me.
Before; with issue2024-12-20.8.22.19.movAfter; with fix2024-12-20.8.26.31.movAny review please 🙏 |
This comment was marked as outdated.
This comment was marked as outdated.
…on-change-committed
github-actions
bot
removed
the
PR: out-of-date
Looks good 👍 Thanks @DiegoAndai as well for finding an easier repro of the case that happens in the CSB iframe - this was the last thing on my mind about this issue - seems the way to repro is to drag the thumb outside the slider (the left edge) before releasing the pointer, then circling back to the slider on the other side (the right edge) Before: before.movAfter: after.mov |
| @@ -1665,4 +1665,38 @@ describe('<Slider />', () => { | |||
| width: '10px', | |||
| }); | |||
| }); | |||
|
|
|||
| describe('When the onMouseUp event occurs at a different location than the last onChange event', () => { | |||
| it('should pass onChangeCommitted the same value that was passed to the last onChange event', () => { | |||
There was a problem hiding this comment.
Based on my findings, the root cause of this issue is that the last argument passed to onChange is different from the argument passed to onChangeCommitted. This happens when the slider slides fast quickly.
The event flow is as follows:
- MouseDown to 10 => onChange(event, 10)
- MouseMove to 15 => onChange(event, 15)
- MouseUp to 20 => onChangeCommit(event, 20)
I couldn't find any tests for this. So I propose this test case.
Demo: https://codesandbox.io/p/sandbox/nice-ully-k38gjr
The value is determined by the mouse position at the time onChange and onChangeCommitted are called. The discrepancy in the mouse position between the last onChange call and the onChangeCommitted call led to different argument values.
To address this, I've modified onChangeCommitted to always receive the last argument passed to onChange.
Closes #41739