Fix updating stale state in Slider

packages/@react-aria/slider/test/useSliderThumb.test.js

@@ -161,6 +161,7 @@ describe('useSliderThumb', () => {
       expect(onChangeEndSpy).not.toHaveBeenCalled();
       expect(stateRef.current.values).toEqual([30, 80]);
 
+      fireEvent.mouseMove(thumb0, {clientX: 40});
       fireEvent.mouseUp(thumb0, {clientX: 40});
       expect(onChangeSpy).toHaveBeenLastCalledWith([40, 80]);
       expect(onChangeEndSpy).toHaveBeenLastCalledWith([40, 80]);
@@ -233,7 +234,7 @@ describe('useSliderThumb', () => {
       // Drag thumb
       let thumb0 = screen.getByTestId('thumb');
       fireEvent.mouseDown(thumb0, {clientX: 10});
-      expect(onChangeSpy).not.toHaveBeenLastCalledWith([10]);
+      expect(onChangeSpy).not.toHaveBeenCalled();
       expect(onChangeEndSpy).not.toHaveBeenCalled();
       expect(stateRef.current.values).toEqual([10]);
 
@@ -242,6 +243,7 @@ describe('useSliderThumb', () => {
       expect(onChangeEndSpy).not.toHaveBeenCalled();
       expect(stateRef.current.values).toEqual([20]);
 
+      fireEvent.mouseMove(thumb0, {clientX: 40});
       fireEvent.mouseUp(thumb0, {clientX: 40});
       expect(onChangeSpy).toHaveBeenLastCalledWith([40]);
       expect(onChangeEndSpy).toHaveBeenLastCalledWith([40]);

packages/@react-aria/utils/src/useDrag1D.ts

@@ -50,16 +50,21 @@ export function useDrag1D(props: UseDrag1DProps): HTMLAttributes<HTMLElement> {
   let dragging = useRef(false);
   let prevPosition = useRef(0);
 
+  // Keep track of the current handlers in a ref so that the events can access them.
+  let handlers = useRef({onPositionChange, onDrag});
+  handlers.current.onDrag = onDrag;
+  handlers.current.onPositionChange = onPositionChange;
+
   let onMouseDragged = (e: MouseEvent) => {
     e.preventDefault();
     let nextOffset = getNextOffset(e);
     if (!dragging.current) {
       dragging.current = true;
-      if (onDrag) {
-        onDrag(true);
+      if (handlers.current.onDrag) {
+        handlers.current.onDrag(true);
       }
-      if (onPositionChange) {
-        onPositionChange(nextOffset);
+      if (handlers.current.onPositionChange) {
+        handlers.current.onPositionChange(nextOffset);
       }
     }
     if (prevPosition.current === nextOffset) {
@@ -75,11 +80,11 @@ export function useDrag1D(props: UseDrag1DProps): HTMLAttributes<HTMLElement> {
     const target = e.target as HTMLElement;
     dragging.current = false;
     let nextOffset = getNextOffset(e);
-    if (onDrag) {
-      onDrag(false);
+    if (handlers.current.onDrag) {
+      handlers.current.onDrag(false);
     }
-    if (onPositionChange) {
-      onPositionChange(nextOffset);
+    if (handlers.current.onPositionChange) {
+      handlers.current.onPositionChange(nextOffset);
     }
 
     draggingElements.splice(draggingElements.indexOf(target), 1);
@@ -89,7 +94,7 @@ export function useDrag1D(props: UseDrag1DProps): HTMLAttributes<HTMLElement> {
 
   let onMouseDown = (e: React.MouseEvent<HTMLElement>) => {
     const target = e.currentTarget;
-    // If we're already handling dragging on a descendant with useDrag1D, then 
+    // If we're already handling dragging on a descendant with useDrag1D, then
     // we don't want to handle the drag motion on this target as well.
     if (draggingElements.some(elt => target.contains(elt))) {
       return;

packages/@react-stately/slider/src/useSliderState.ts

@@ -75,10 +75,6 @@ export function useSliderState(props: SliderProps): SliderState {
   const isEditablesRef = useRef<boolean[]>(new Array(values.length).fill(true));
   const [focusedIndex, setFocusedIndex] = useState<number|undefined>(undefined);
 
-  // We keep some of the dragging state on refs as well, because they are read by event
-  // handlers.  In useDrag1D, the same drag event handler is used for the entire drag motion,
-  // so the state object within their closure is already stale.
-  const realTimeDragging = useRef(false);
   const formatter = useNumberFormatter(formatOptions);
 
   function getValuePercent(value: number) {
@@ -109,28 +105,21 @@ export function useSliderState(props: SliderProps): SliderState {
 
     // Round value to multiple of step, clamp value between min and max
     value = clamp(getRoundedValue(value), thisMin, thisMax);
-
-    // Do nothing if slider hasn't moved
-    if (value === values[index]) {
-      return;
-    }
-
-    const newValues = replaceIndex(values, index, value);
-    setValues(newValues);
-
-    if (props.onChangeEnd && !realTimeDragging.current) {
-      // If not in the middle of dragging, call onChangeEnd
-      props.onChangeEnd(newValues);
-    }
+    setValues(values => replaceIndex(values, index, value));
   }
 
   function updateDragging(index: number, dragging: boolean) {
     if (isReadOnly || isDisabled || !isThumbEditable(index)) {
       return;
     }
+
     const newDraggings = replaceIndex(isDraggings, index, dragging);
     setDraggings(newDraggings);
-    realTimeDragging.current = newDraggings.some(Boolean);
+
+    // Call onChangeEnd if no handles are dragging.
+    if (props.onChangeEnd && isDraggings[index] && !newDraggings.some(Boolean)) {
+      props.onChangeEnd(values);
+    }
   }
 
   function getFormattedValue(value: number) {
@@ -173,5 +162,9 @@ export function useSliderState(props: SliderProps): SliderState {
 }
 
 function replaceIndex<T>(array: T[], index: number, value: T) {
+  if (array[index] === value) {
+    return array;
+  }
+
   return [...array.slice(0, index), value, ...array.slice(index + 1)];
 }

packages/@react-stately/slider/test/useSliderState.test.js

@@ -81,7 +81,9 @@ describe('useSliderState', () => {
     })).result;
 
     expect(result.current.values).toEqual([0]);
+    act(() => result.current.setThumbDragging(0, true));
     act(() => result.current.setThumbValue(0, 50));
+    act(() => result.current.setThumbDragging(0, false));
     expect(onChangeSpy).toHaveBeenLastCalledWith([50]);
     expect(onChangeEndSpy).toHaveBeenLastCalledWith([50]);
 
@@ -95,8 +97,8 @@ describe('useSliderState', () => {
     act(() => result.current.setThumbValue(0, 60));
     expect(onChangeSpy).toHaveBeenLastCalledWith([60]);
     expect(onChangeEndSpy).not.toHaveBeenCalled();
-    act(() => result.current.setThumbDragging(0, false));
     act(() => result.current.setThumbValue(0, 65));
+    act(() => result.current.setThumbDragging(0, false));
     expect(onChangeSpy).toHaveBeenLastCalledWith([65]);
     expect(onChangeEndSpy).toHaveBeenLastCalledWith([65]);
   });

packages/@react-stately/splitview/src/useSplitViewState.ts

@@ -39,7 +39,7 @@ export function useSplitViewState(props: SplitViewStatelyProps): SplitViewState
     }
   };
   let callOnResizeEnd = (value) => {
-    if (onResizeEnd && value !== offset) {
+    if (onResizeEnd) {
       onResizeEnd(value);
     }
   };
@@ -76,30 +76,38 @@ export function useSplitViewState(props: SplitViewStatelyProps): SplitViewState
 
   let increment = () => setOffset(prevHandleOffset => {
     let nextOffset = boundOffset(prevHandleOffset + 10);
-    callOnResize(nextOffset);
-    callOnResizeEnd(nextOffset);
+    if (nextOffset !== offset) {
+      callOnResize(nextOffset);
+      callOnResizeEnd(nextOffset);
+    }
     return nextOffset;
   });
 
   let decrement = () => setOffset(prevHandleOffset => {
     let nextOffset = boundOffset(prevHandleOffset - 10);
-    callOnResize(nextOffset);
-    callOnResizeEnd(nextOffset);
+    if (nextOffset !== offset) {
+      callOnResize(nextOffset);
+      callOnResizeEnd(nextOffset);
+    }
     return nextOffset;
   });
 
   let decrementToMin = () => {
     let nextOffset = allowsCollapsing ? 0 : minPos;
-    callOnResize(nextOffset);
-    callOnResizeEnd(nextOffset);
-    setOffset(nextOffset);
+    if (nextOffset !== offset) {
+      callOnResize(nextOffset);
+      callOnResizeEnd(nextOffset);
+      setOffset(nextOffset);
+    }
   };
 
   let incrementToMax = () => {
     let nextOffset = maxPos;
-    callOnResize(nextOffset);
-    callOnResizeEnd(nextOffset);
-    setOffset(nextOffset);
+    if (nextOffset !== offset) {
+      callOnResize(nextOffset);
+      callOnResizeEnd(nextOffset);
+      setOffset(nextOffset);
+    }
   };
 
   let collapseToggle = () => setOffset(prevHandleOffset => {