diff --git a/packages/wds/src/components/slider/index.test.tsx b/packages/wds/src/components/slider/index.test.tsx index 8ce38d01e..40639b3b4 100644 --- a/packages/wds/src/components/slider/index.test.tsx +++ b/packages/wds/src/components/slider/index.test.tsx @@ -1,4 +1,10 @@ -import { cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { + act, + cleanup, + fireEvent, + render, + screen, +} from '@testing-library/react'; import { axe } from 'vitest-axe'; import { @@ -17,8 +23,7 @@ const TRACK_WIDTH = 100; * coordinates `fireEvent.pointerMove` passes in — dispatch a `MouseEvent` so * `clientX` survives. */ -const firePointer = ( - element: Element, +const makePointerEvent = ( type: 'pointerdown' | 'pointermove' | 'pointerup' | 'pointercancel', clientX: number, { @@ -33,7 +38,16 @@ const firePointer = ( buttons, }); Object.defineProperty(event, 'pointerId', { value: pointerId }); - fireEvent(element, event); + return event; +}; + +const firePointer = ( + element: Element, + type: 'pointerdown' | 'pointermove' | 'pointerup' | 'pointercancel', + clientX: number, + options: { pointerId?: number; buttons?: number } = {}, +) => { + fireEvent(element, makePointerEvent(type, clientX, options)); }; const getTrack = (container: HTMLElement) => @@ -576,6 +590,82 @@ describe('when operating a slider with a pointer', () => { expect(getValues(container)).toEqual(['80']); }); + it('should report completion when the release outruns the re-render', () => { + const onValueChangeComplete = vi.fn(); + const { container } = render( + , + ); + stubTrackRect(container); + + const thumb = getThumbs(container)[0]!; + firePointer(thumb, 'pointerdown', 20); + fireEvent.focus(thumb); + + /** + * A fast drag lands its last move and its release in the same batch, so + * the pointerup handler still closes over the values from before the move. + */ + act(() => { + thumb.dispatchEvent(makePointerEvent('pointermove', 70)); + thumb.dispatchEvent(makePointerEvent('pointerup', 70)); + }); + + expect(getValues(container)).toEqual(['70']); + expect(onValueChangeComplete).toHaveBeenCalledTimes(1); + expect(onValueChangeComplete).toHaveBeenCalledWith([70]); + }); + + it('should land on the last position when a drag is batched into one frame', () => { + const { container } = render( + , + ); + stubTrackRect(container); + + const thumb = getThumbs(container)[0]!; + firePointer(thumb, 'pointerdown', 20); + fireEvent.focus(thumb); + + act(() => { + thumb.dispatchEvent(makePointerEvent('pointermove', 40)); + thumb.dispatchEvent(makePointerEvent('pointermove', 60)); + thumb.dispatchEvent(makePointerEvent('pointermove', 85)); + }); + + expect(getValues(container)).toEqual(['85']); + }); + + it('should survive a controlled re-render that lands mid-drag', () => { + const onValueChangeComplete = vi.fn(); + const props = { + min: 0, + max: 100, + onValueChangeComplete, + }; + const { container, rerender } = render(); + stubTrackRect(container); + + const thumb = getThumbs(container)[0]!; + firePointer(thumb, 'pointerdown', 20); + fireEvent.focus(thumb); + firePointer(thumb, 'pointermove', 70); + + /** + * A controlled parent has not applied the change yet, but re-renders for + * its own reasons — handing back a fresh array holding the old value. + */ + rerender(); + + firePointer(thumb, 'pointerup', 70); + + expect(onValueChangeComplete).toHaveBeenCalledTimes(1); + expect(onValueChangeComplete).toHaveBeenCalledWith([70]); + }); + it('should still run the consumer pointer handlers', () => { const onPointerDown = vi.fn(); const onPointerMove = vi.fn(); diff --git a/packages/wds/src/components/slider/index.tsx b/packages/wds/src/components/slider/index.tsx index 3ee07c7b2..2c34fb39d 100644 --- a/packages/wds/src/components/slider/index.tsx +++ b/packages/wds/src/components/slider/index.tsx @@ -87,6 +87,15 @@ const Slider = forwardRef< const slideStartValues = useRef(values); + /** + * `values` is a render closure, and a fast drag lands several moves — and + * its release — inside one batch, before React re-renders. Every one of + * those would compute from, and compare against, the values the drag + * started at. This ref carries what was last committed so each step sees + * the one before it. + */ + const committedValues = useRef(values); + const handleValueChange = useCallback( (nextValue: number, index: number, isCompleted = false) => { const decimalCount = nextValue.toString().split('.')[1]?.length ?? 0; @@ -98,44 +107,48 @@ const Slider = forwardRef< const calculatedNextValue = clamp(snapToStep, [min, max]); - setValues((prevValues = []) => { - const nextValues = [ - ...prevValues.slice(0, index), - calculatedNextValue, - ...prevValues.slice(index + 1), - ]; - - const stepsBetweenValue = Math.min( - ...nextValues.slice(0, -1).map((v, i) => nextValues[i + 1]! - v), - ); - - if ( - (disableSwapThumbs && stepsBetweenValue < minStepBetweenThumbs) || - (minStepBetweenThumbs > 0 && - stepsBetweenValue < minStepBetweenThumbs) - ) { - return prevValues; - } - - const sortedNextValues = [...nextValues].sort((a, b) => a - b); - - /** - * `indexOf` resolves to the leftmost slot when thumbs share a value, - * which would hand the focus over to a thumb the user never touched. - * Keep the dragged thumb where it is unless sorting actually moved it. - */ - currentFocusedIndex.current = - sortedNextValues[index] === calculatedNextValue - ? index - : sortedNextValues.indexOf(calculatedNextValue); - - const hasChanged = - sortedNextValues.toString() !== prevValues.toString(); - if (hasChanged && isCompleted) { - onValueChangeComplete?.(sortedNextValues); - } - return hasChanged ? sortedNextValues : prevValues; - }); + const prevValues = committedValues.current; + + const nextValues = [ + ...prevValues.slice(0, index), + calculatedNextValue, + ...prevValues.slice(index + 1), + ]; + + const stepsBetweenValue = Math.min( + ...nextValues.slice(0, -1).map((v, i) => nextValues[i + 1]! - v), + ); + + if ( + (disableSwapThumbs && stepsBetweenValue < minStepBetweenThumbs) || + (minStepBetweenThumbs > 0 && stepsBetweenValue < minStepBetweenThumbs) + ) { + return; + } + + const sortedNextValues = [...nextValues].sort((a, b) => a - b); + + /** + * `indexOf` resolves to the leftmost slot when thumbs share a value, + * which would hand the focus over to a thumb the user never touched. + * Keep the dragged thumb where it is unless sorting actually moved it. + */ + currentFocusedIndex.current = + sortedNextValues[index] === calculatedNextValue + ? index + : sortedNextValues.indexOf(calculatedNextValue); + + if (sortedNextValues.toString() === prevValues.toString()) { + return; + } + + committedValues.current = sortedNextValues; + + if (isCompleted) { + onValueChangeComplete?.(sortedNextValues); + } + + setValues(sortedNextValues); }, [ max, @@ -181,14 +194,27 @@ const Slider = forwardRef< * Comparing a single index misses changes whenever thumbs end up * stacked or swapped, so compare the whole set instead. */ + const finalValues = committedValues.current; const hasChanged = - slideStartValues.current.toString() !== values.toString(); + slideStartValues.current.toString() !== finalValues.toString(); if (hasChanged) { - onValueChangeComplete?.(values); + onValueChangeComplete?.(finalValues); } }; + /** + * Outside a drag the ref only mirrors what is rendered, so a controlled + * update or a form reset reaches it. During one it must not: a controlled + * parent can re-render with the value it has not applied yet, which would + * hand the drag back its own starting point and swallow the completion. + */ + useEffect(() => { + if (activePointerId.current !== null) return; + + committedValues.current = values; + }); + const initialValuesRef = useRef(values); useEffect(() => { @@ -280,6 +306,7 @@ const Slider = forwardRef< activePointerId.current = event.pointerId; slideStartValues.current = values; + committedValues.current = values; const target = event.target as HTMLElement; target.setPointerCapture(event.pointerId);