From e6d4aa745168ceba68864de4f5f344068608b5b5 Mon Sep 17 00:00:00 2001 From: Albert Yu Date: Mon, 23 Dec 2024 16:14:50 +0800 Subject: [PATCH 1/4] Add a test --- .../react/src/slider/root/SliderRoot.test.tsx | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/packages/react/src/slider/root/SliderRoot.test.tsx b/packages/react/src/slider/root/SliderRoot.test.tsx index 400faedbd0..c8200b4587 100644 --- 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')('', () => { 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( + , + ); + + 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', () => { From c8b14fccb25686e5006722def3a3185ae38dc320 Mon Sep 17 00:00:00 2001 From: Albert Yu Date: Tue, 7 Jan 2025 13:54:51 +0800 Subject: [PATCH 2/4] Handle pointerUp at a different location than last value change --- .../src/slider/control/SliderControl.test.tsx | 1 + .../react/src/slider/control/SliderControl.tsx | 8 +++++--- .../react/src/slider/control/useSliderControl.ts | 12 +++++++----- .../slider/indicator/SliderIndicator.test.tsx | 1 + packages/react/src/slider/root/useSliderRoot.ts | 16 +++++++++++----- .../react/src/slider/thumb/SliderThumb.test.tsx | 1 + .../react/src/slider/track/SliderTrack.test.tsx | 1 + .../react/src/slider/value/SliderValue.test.tsx | 1 + 8 files changed, 28 insertions(+), 13 deletions(-) diff --git a/packages/react/src/slider/control/SliderControl.test.tsx b/packages/react/src/slider/control/SliderControl.test.tsx index 4e55a7a590..c8d075ccce 100644 --- 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 index 46e449ee6f..924072a8ba 100644 --- 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,7 +41,7 @@ const SliderControl = React.forwardRef(function SliderControl( disabled, dragging, getFingerState, - setValue, + lastChangedValueRef, minStepsBetweenValues, onValueCommitted, percentageValues, @@ -48,6 +49,7 @@ const SliderControl = React.forwardRef(function SliderControl( 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 index ef6fd13269..a2b38bbe6a 100644 --- 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 index 04eb7806f9..e37fbe7c9f 100644 --- 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/useSliderRoot.ts b/packages/react/src/slider/root/useSliderRoot.ts index 5dcdc0687b..e667f9f939 100644 --- a/packages/react/src/slider/root/useSliderRoot.ts +++ b/packages/react/src/slider/root/useSliderRoot.ts @@ -145,6 +145,8 @@ export function useSliderRoot(parameters: useSliderRoot.Parameters): useSliderRo const controlRef: React.RefObject = React.useRef(null); const thumbRefs = React.useRef<(HTMLElement | null)[]>([]); + const lastChangedValueRef = React.useRef(null); + const [thumbMap, setThumbMap] = React.useState( () => new Map | null>(), ); @@ -226,6 +228,7 @@ export function useSliderRoot(parameters: useSliderRoot.Parameters): useSliderRo value: { value: newValue, name }, }); + lastChangedValueRef.current = newValue; onValueChange(newValue, clonedEvent, thumbIndex); }, ); @@ -257,8 +260,8 @@ export function useSliderRoot(parameters: useSliderRoot.Parameters): useSliderRo } setDirty(newValue !== validityData.initialValue); setTouched(true); - commitValidation(newValue); - onValueCommitted(newValue, event.nativeEvent); + commitValidation(lastChangedValueRef.current ?? newValue); + onValueCommitted(lastChangedValueRef.current ?? newValue, event.nativeEvent); } }, ); @@ -380,6 +383,7 @@ export function useSliderRoot(parameters: useSliderRoot.Parameters): useSliderRo getFingerState, handleInputChange, largeStep, + lastChangedValueRef, max, min, minStepsBetweenValues, @@ -408,6 +412,7 @@ export function useSliderRoot(parameters: useSliderRoot.Parameters): useSliderRo getFingerState, handleInputChange, largeStep, + lastChangedValueRef, max, min, minStepsBetweenValues, @@ -487,7 +492,7 @@ export namespace useSliderRoot { /** * 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. @@ -500,7 +505,7 @@ export namespace useSliderRoot { * @param {Event} event The corresponding event that initiated the change. * **Warning**: This is a generic event not a change event. */ - onValueCommitted: (value: number | number[], event: Event) => void; + onValueCommitted: (value: number | readonly number[], event: Event) => void; /** * The component orientation. * @default 'horizontal' @@ -565,6 +570,7 @@ export namespace useSliderRoot { * @default 10 */ largeStep: number; + lastChangedValueRef: React.RefObject; /** * The maximum allowed value of the slider. */ @@ -578,7 +584,7 @@ export namespace useSliderRoot { */ minStepsBetweenValues: number; name: string; - onValueCommitted: (value: number | number[], event: Event) => void; + onValueCommitted: (value: number | readonly number[], event: Event) => void; /** * The component orientation. * @default 'horizontal' diff --git a/packages/react/src/slider/thumb/SliderThumb.test.tsx b/packages/react/src/slider/thumb/SliderThumb.test.tsx index 55b4d7c727..1f5f1d89cd 100644 --- a/packages/react/src/slider/thumb/SliderThumb.test.tsx +++ b/packages/react/src/slider/thumb/SliderThumb.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/track/SliderTrack.test.tsx b/packages/react/src/slider/track/SliderTrack.test.tsx index 4219709da6..3a4e003c6b 100644 --- a/packages/react/src/slider/track/SliderTrack.test.tsx +++ b/packages/react/src/slider/track/SliderTrack.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/value/SliderValue.test.tsx b/packages/react/src/slider/value/SliderValue.test.tsx index 3d6e62f2e0..632e61484e 100644 --- a/packages/react/src/slider/value/SliderValue.test.tsx +++ b/packages/react/src/slider/value/SliderValue.test.tsx @@ -19,6 +19,7 @@ const testRootContext: SliderRootContext = { }), setValue: NOOP, largeStep: 10, + lastChangedValueRef: { current: null }, thumbMap: new Map(), max: 100, min: 0, From bf0039c9cc26abec631774009f6cbbcf5583fe30 Mon Sep 17 00:00:00 2001 From: Albert Yu Date: Tue, 7 Jan 2025 16:01:42 +0800 Subject: [PATCH 3/4] Remove old experiments --- .../slider-change-committed-lag.tsx | 38 ------- .../(private)/experiments/slider-format.tsx | 56 ----------- .../(private)/experiments/slider.module.css | 99 ------------------- 3 files changed, 193 deletions(-) delete mode 100644 docs/src/app/(private)/experiments/slider-change-committed-lag.tsx delete mode 100644 docs/src/app/(private)/experiments/slider-format.tsx delete mode 100644 docs/src/app/(private)/experiments/slider.module.css diff --git a/docs/src/app/(private)/experiments/slider-change-committed-lag.tsx b/docs/src/app/(private)/experiments/slider-change-committed-lag.tsx deleted file mode 100644 index b8562483a8..0000000000 --- a/docs/src/app/(private)/experiments/slider-change-committed-lag.tsx +++ /dev/null @@ -1,38 +0,0 @@ -'use client'; -// https://github.com/mui/material-ui/issues/41739 -// to cross check whether this issue would still occur in the new API -import * as React from 'react'; -import { Slider } from '@base-ui-components/react/slider'; -import classes from './slider.module.css'; - -export default function App() { - const [val1, setVal1] = React.useState(80); - const [val2, setVal2] = React.useState(null); - - return ( -
- setVal1(newValue as number)} - onValueCommitted={(newValue) => setVal2(newValue as number)} - > - - - - - - - - - - 
onValueChange value: {val1}
- 
onValueCommitted value: {val2}
-
- ); -} diff --git a/docs/src/app/(private)/experiments/slider-format.tsx b/docs/src/app/(private)/experiments/slider-format.tsx deleted file mode 100644 index 26756bcee8..0000000000 --- a/docs/src/app/(private)/experiments/slider-format.tsx +++ /dev/null @@ -1,56 +0,0 @@ -'use client'; -import * as React from 'react'; -import { useTheme } from '@mui/system'; -import { Slider } from '@base-ui-components/react/slider'; -import c from './slider.module.css'; - -export default function UnstyledSliderIntroduction() { - // Replace this with your app logic for determining dark mode - const isDarkMode = useIsDarkMode(); - return ( -
- - - - {(_, values) => `$${values[0].toFixed(2)} - ${values[1].toFixed(2)} USD`} - - - - - - - - - -
- ); -} - -function Label(props: React.LabelHTMLAttributes) { - const { id, htmlFor, ...otherProps } = props; - - return