[Slider] Ensure onValueCommitted is called with the same value as l…

…atest `onValueChange` (#1296)
mui · Jan 7, 2025 · 2e87d21 · 2e87d21
1 parent 5d0cd32
commit 2e87d21
Show file tree

Hide file tree

Showing 13 changed files with 67 additions and 207 deletions.
diff --git a/docs/src/app/(private)/experiments/slider-change-committed-lag.tsx b/docs/src/app/(private)/experiments/slider-change-committed-lag.tsx
diff --git a/docs/src/app/(private)/experiments/slider-format.tsx b/docs/src/app/(private)/experiments/slider-format.tsx
diff --git a/docs/src/app/(private)/experiments/slider.module.css b/docs/src/app/(private)/experiments/slider.module.css
diff --git a/packages/react/src/slider/control/SliderControl.test.tsx b/packages/react/src/slider/control/SliderControl.test.tsx
@@ -17,6 +17,7 @@ const testRootContext: SliderRootContext = {
   }),
   setValue: NOOP,
   largeStep: 10,
+  lastChangedValueRef: { current: null },
   thumbMap: new Map(),
   max: 100,
   min: 0,

diff --git a/packages/react/src/slider/control/SliderControl.tsx b/packages/react/src/slider/control/SliderControl.tsx
@@ -24,14 +24,15 @@ const SliderControl = React.forwardRef(function SliderControl(
     disabled,
     dragging,
     getFingerState,
-    setValue,
+    lastChangedValueRef,
     minStepsBetweenValues,
     onValueCommitted,
-    state,
     percentageValues,
     registerSliderControl,
     setActive,
     setDragging,
+    setValue,
+    state,
     step,
     thumbRefs,
   } = useSliderRootContext();
@@ -40,14 +41,15 @@ const SliderControl = React.forwardRef(function SliderControl(
     disabled,
     dragging,
     getFingerState,
-    setValue,
+    lastChangedValueRef,
     minStepsBetweenValues,
     onValueCommitted,
     percentageValues,
     registerSliderControl,
     rootRef: forwardedRef,
     setActive,
     setDragging,
+    setValue,
     step,
     thumbRefs,
   });

diff --git a/packages/react/src/slider/control/useSliderControl.ts b/packages/react/src/slider/control/useSliderControl.ts
@@ -49,14 +49,15 @@ export function useSliderControl(
     disabled,
     dragging,
     getFingerState,
-    setValue,
-    onValueCommitted,
+    lastChangedValueRef,
     minStepsBetweenValues,
+    onValueCommitted,
     percentageValues,
     registerSliderControl,
     rootRef: externalRef,
     setActive,
     setDragging,
+    setValue,
     step,
     thumbRefs,
   } = parameters;
@@ -126,9 +127,9 @@ export function useSliderControl(
 
     setActive(-1);
 
-    commitValidation(finger.value);
+    commitValidation(lastChangedValueRef.current ?? finger.value);
 
-    onValueCommitted(finger.value, nativeEvent);
+    onValueCommitted(lastChangedValueRef.current ?? finger.value, nativeEvent);
 
     touchIdRef.current = null;
 
@@ -277,13 +278,14 @@ export namespace useSliderControl {
       | 'disabled'
       | 'dragging'
       | 'getFingerState'
-      | 'setValue'
+      | 'lastChangedValueRef'
       | 'minStepsBetweenValues'
       | 'onValueCommitted'
       | 'percentageValues'
       | 'registerSliderControl'
       | 'setActive'
       | 'setDragging'
+      | 'setValue'
       | 'step'
       | 'thumbRefs'
     > {

diff --git a/packages/react/src/slider/indicator/SliderIndicator.test.tsx b/packages/react/src/slider/indicator/SliderIndicator.test.tsx
@@ -17,6 +17,7 @@ const testRootContext: SliderRootContext = {
   }),
   setValue: NOOP,
   largeStep: 10,
+  lastChangedValueRef: { current: null },
   thumbMap: new Map(),
   max: 100,
   min: 0,

diff --git a/packages/react/src/slider/root/SliderRoot.test.tsx b/packages/react/src/slider/root/SliderRoot.test.tsx
@@ -1293,6 +1293,44 @@ describeSkipIf(typeof Touch === 'undefined')('<Slider.Root />', () => {
         value: 4,
       });
     });
+
+    it('onValueCommitted is called with the same value as the latest onValueChange when pointerUp occurs at a different location than onValueChange', async () => {
+      const handleValueChange = spy();
+      const handleValueCommitted = spy();
+
+      await render(
+        <TestSlider
+          onValueChange={handleValueChange}
+          onValueCommitted={handleValueCommitted}
+          defaultValue={0}
+        />,
+      );
+
+      const sliderControl = screen.getByTestId('control');
+
+      stub(sliderControl, 'getBoundingClientRect').callsFake(
+        () => GETBOUNDINGCLIENTRECT_HORIZONTAL_SLIDER_RETURN_VAL,
+      );
+
+      fireEvent.pointerDown(sliderControl, {
+        buttons: 1,
+        clientX: 10,
+      });
+      fireEvent.pointerMove(sliderControl, {
+        buttons: 1,
+        clientX: 15,
+      });
+      fireEvent.pointerUp(sliderControl, {
+        buttons: 1,
+        clientX: 20,
+      });
+
+      expect(handleValueChange.callCount).to.equal(2);
+      expect(handleValueChange.args[0][0]).to.equal(10);
+      expect(handleValueChange.args[1][0]).to.equal(15);
+      expect(handleValueCommitted.callCount).to.equal(1);
+      expect(handleValueCommitted.args[0][0]).to.equal(15);
+    });
   });
 
   describe('keyboard interactions', () => {

diff --git a/packages/react/src/slider/root/SliderRoot.tsx b/packages/react/src/slider/root/SliderRoot.tsx
@@ -288,7 +288,7 @@ SliderRoot.propTypes /* remove-proptypes */ = {
   /**
    * Callback function that is fired when the slider's value changed.
    *
-   * @param {number | number[]} value The new value.
+   * @param {number | readonly number[]} value The new value.
    * @param {Event} event The corresponding event that initiated the change.
    * You can pull out the new value by accessing `event.target.value` (any).
    * @param {number} activeThumbIndex Index of the currently moved thumb.