Skip to content

Commit 1aa66e6

Browse files
majornistaLFDanLusnowystinger
authored
fix(#2659): useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End (#2661)
* useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End * fix(#2659): add onChangeEnd event handler to Slider/RangeSlider stories Per #2661 (review) * Have slider work like color area (#2819) * Match Slider implementation to color area * fix lint * make step optional with a default * add pagesize * Use native slider pagesize calculation * @trivial remove irrelevant comment and use the declared variable for direction === 'rtl' Co-authored-by: Michael Jordan <[email protected]> * fix(#2659): add pageSize stories * simplify slider move logic * a little more cleanup Co-authored-by: Daniel Lu <[email protected]> Co-authored-by: Robert Snow <[email protected]> Co-authored-by: Daniel Lu <[email protected]>
1 parent 47ce78d commit 1aa66e6

File tree

9 files changed

+289
-21
lines changed

9 files changed

+289
-21
lines changed

packages/@react-aria/slider/src/useSliderThumb.ts

Lines changed: 60 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@ import {getSliderThumbId, sliderIds} from './utils';
44
import React, {ChangeEvent, HTMLAttributes, InputHTMLAttributes, LabelHTMLAttributes, RefObject, useCallback, useEffect, useRef} from 'react';
55
import {SliderState} from '@react-stately/slider';
66
import {useFocusable} from '@react-aria/focus';
7+
import {useKeyboard, useMove} from '@react-aria/interactions';
78
import {useLabel} from '@react-aria/label';
89
import {useLocale} from '@react-aria/i18n';
9-
import {useMove} from '@react-aria/interactions';
1010

1111
interface SliderThumbAria {
1212
/** Props for the root thumb element; handles the dragging motion. */
@@ -77,34 +77,82 @@ export function useSliderThumb(
7777
stateRef.current = state;
7878
let reverseX = direction === 'rtl';
7979
let currentPosition = useRef<number>(null);
80+
81+
let {keyboardProps} = useKeyboard({
82+
onKeyDown(e) {
83+
let {
84+
getThumbMaxValue,
85+
getThumbMinValue,
86+
decrementThumb,
87+
incrementThumb,
88+
setThumbValue,
89+
setThumbDragging,
90+
pageSize
91+
} = stateRef.current;
92+
// these are the cases that useMove or useSlider don't handle
93+
if (!/^(PageUp|PageDown|Home|End)$/.test(e.key)) {
94+
e.continuePropagation();
95+
return;
96+
}
97+
// same handling as useMove, stopPropagation to prevent useSlider from handling the event as well.
98+
e.preventDefault();
99+
// remember to set this so that onChangeEnd is fired
100+
setThumbDragging(index, true);
101+
switch (e.key) {
102+
case 'PageUp':
103+
incrementThumb(index, pageSize);
104+
break;
105+
case 'PageDown':
106+
decrementThumb(index, pageSize);
107+
break;
108+
case 'Home':
109+
setThumbValue(index, getThumbMinValue(index));
110+
break;
111+
case 'End':
112+
setThumbValue(index, getThumbMaxValue(index));
113+
break;
114+
}
115+
setThumbDragging(index, false);
116+
}
117+
});
118+
80119
let {moveProps} = useMove({
81120
onMoveStart() {
82121
currentPosition.current = null;
83-
state.setThumbDragging(index, true);
122+
stateRef.current.setThumbDragging(index, true);
84123
},
85-
onMove({deltaX, deltaY, pointerType}) {
124+
onMove({deltaX, deltaY, pointerType, shiftKey}) {
125+
const {
126+
getThumbPercent,
127+
setThumbPercent,
128+
decrementThumb,
129+
incrementThumb,
130+
step,
131+
pageSize
132+
} = stateRef.current;
86133
let size = isVertical ? trackRef.current.offsetHeight : trackRef.current.offsetWidth;
87134

88135
if (currentPosition.current == null) {
89-
currentPosition.current = stateRef.current.getThumbPercent(index) * size;
136+
currentPosition.current = getThumbPercent(index) * size;
90137
}
91138
if (pointerType === 'keyboard') {
92-
// (invert left/right according to language direction) + (according to vertical)
93-
let delta = ((reverseX ? -deltaX : deltaX) + (isVertical ? -deltaY : -deltaY)) * stateRef.current.step;
94-
currentPosition.current += delta * size;
95-
stateRef.current.setThumbValue(index, stateRef.current.getThumbValue(index) + delta);
139+
if ((deltaX > 0 && reverseX) || (deltaX < 0 && !reverseX) || deltaY > 0) {
140+
decrementThumb(index, shiftKey ? pageSize : step);
141+
} else {
142+
incrementThumb(index, shiftKey ? pageSize : step);
143+
}
96144
} else {
97145
let delta = isVertical ? deltaY : deltaX;
98146
if (isVertical || reverseX) {
99147
delta = -delta;
100148
}
101149

102150
currentPosition.current += delta;
103-
stateRef.current.setThumbPercent(index, clamp(currentPosition.current / size, 0, 1));
151+
setThumbPercent(index, clamp(currentPosition.current / size, 0, 1));
104152
}
105153
},
106154
onMoveEnd() {
107-
state.setThumbDragging(index, false);
155+
stateRef.current.setThumbDragging(index, false);
108156
}
109157
});
110158

@@ -161,10 +209,11 @@ export function useSliderThumb(
161209
'aria-invalid': validationState === 'invalid' || undefined,
162210
'aria-errormessage': opts['aria-errormessage'],
163211
onChange: (e: ChangeEvent<HTMLInputElement>) => {
164-
state.setThumbValue(index, parseFloat(e.target.value));
212+
stateRef.current.setThumbValue(index, parseFloat(e.target.value));
165213
}
166214
}),
167215
thumbProps: !isDisabled ? mergeProps(
216+
keyboardProps,
168217
moveProps,
169218
{
170219
onMouseDown: (e: React.MouseEvent<HTMLElement>) => {

packages/@react-aria/slider/stories/Slider.stories.tsx

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ storiesOf('Slider (hooks)', module)
2828
'single with aria label',
2929
() => <StorySlider aria-label="Size" onChange={action('onChange')} onChangeEnd={action('onChangeEnd')} showTip />
3030
)
31+
.add(
32+
'single with pageSize',
33+
() => <StorySlider label="Degrees" onChange={action('onChange')} onChangeEnd={action('onChangeEnd')} minValue={0} maxValue={360} pageSize={15} formatOptions={{style: 'unit', unit: 'degree', unitDisplay: 'narrow'}} showTip />
34+
)
3135
.add(
3236
'range',
3337
() => (<StoryRangeSlider
@@ -56,6 +60,23 @@ storiesOf('Slider (hooks)', module)
5660
unitDisplay: 'narrow'
5761
} as any} />)
5862
)
63+
.add(
64+
'range with pageSize',
65+
() => (<StoryRangeSlider
66+
label="Arc"
67+
defaultValue={[45, 135]}
68+
minValue={0}
69+
maxValue={360}
70+
pageSize={15}
71+
onChange={action('onChange')}
72+
onChangeEnd={action('onChangeEnd')}
73+
showTip
74+
formatOptions={{
75+
style: 'unit',
76+
unit: 'degree',
77+
unitDisplay: 'narrow'
78+
} as any} />)
79+
)
5980
.add(
6081
'3 thumbs',
6182
() => (

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

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,11 +384,60 @@ describe('useSliderThumb', () => {
384384
// Drag thumb
385385
let thumb0 = screen.getByTestId('thumb').firstChild;
386386
fireEvent.keyDown(thumb0, {key: 'ArrowRight'});
387+
fireEvent.keyUp(thumb0, {key: 'ArrowRight'});
387388
expect(onChangeSpy).toHaveBeenLastCalledWith([11]);
388389
expect(onChangeSpy).toHaveBeenCalledTimes(1);
389390
expect(onChangeEndSpy).toHaveBeenLastCalledWith([11]);
390391
expect(onChangeEndSpy).toHaveBeenCalledTimes(1);
391392
expect(stateRef.current.values).toEqual([11]);
393+
394+
fireEvent.keyDown(thumb0, {key: 'ArrowLeft'});
395+
fireEvent.keyUp(thumb0, {key: 'ArrowLeft'});
396+
expect(onChangeSpy).toHaveBeenLastCalledWith([10]);
397+
expect(onChangeSpy).toHaveBeenCalledTimes(2);
398+
expect(onChangeEndSpy).toHaveBeenLastCalledWith([10]);
399+
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
400+
expect(stateRef.current.values).toEqual([10]);
401+
});
402+
403+
it('can be moved with keys at the beginning of the slider', () => {
404+
let onChangeSpy = jest.fn();
405+
let onChangeEndSpy = jest.fn();
406+
render(<Example onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} aria-label="Slider" defaultValue={[0]} />);
407+
408+
let thumb0 = screen.getByTestId('thumb').firstChild;
409+
fireEvent.keyDown(thumb0, {key: 'ArrowLeft'});
410+
fireEvent.keyUp(thumb0, {key: 'ArrowLeft'});
411+
expect(onChangeSpy).not.toHaveBeenCalled();
412+
expect(onChangeEndSpy).toHaveBeenCalledWith([0]);
413+
414+
fireEvent.keyDown(thumb0, {key: 'ArrowRight'});
415+
fireEvent.keyUp(thumb0, {key: 'ArrowRight'});
416+
expect(onChangeSpy).toHaveBeenLastCalledWith([1]);
417+
expect(onChangeSpy).toHaveBeenCalledTimes(1);
418+
expect(onChangeEndSpy).toHaveBeenLastCalledWith([1]);
419+
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
420+
expect(stateRef.current.values).toEqual([1]);
421+
});
422+
423+
it('can be moved with keys at the end of the slider', () => {
424+
let onChangeSpy = jest.fn();
425+
let onChangeEndSpy = jest.fn();
426+
render(<Example onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} aria-label="Slider" defaultValue={[100]} />);
427+
428+
let thumb0 = screen.getByTestId('thumb').firstChild;
429+
fireEvent.keyDown(thumb0, {key: 'ArrowRight'});
430+
fireEvent.keyUp(thumb0, {key: 'ArrowRight'});
431+
expect(onChangeSpy).not.toHaveBeenCalled();
432+
expect(onChangeEndSpy).toHaveBeenCalledWith([100]);
433+
434+
fireEvent.keyDown(thumb0, {key: 'ArrowLeft'});
435+
fireEvent.keyUp(thumb0, {key: 'ArrowLeft'});
436+
expect(onChangeSpy).toHaveBeenLastCalledWith([99]);
437+
expect(onChangeSpy).toHaveBeenCalledTimes(1);
438+
expect(onChangeEndSpy).toHaveBeenLastCalledWith([99]);
439+
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
440+
expect(stateRef.current.values).toEqual([99]);
392441
});
393442

394443
it('can be moved with keys (vertical)', () => {
@@ -399,18 +448,64 @@ describe('useSliderThumb', () => {
399448
// Drag thumb
400449
let thumb0 = screen.getByTestId('thumb').firstChild;
401450
fireEvent.keyDown(thumb0, {key: 'ArrowRight'});
451+
fireEvent.keyUp(thumb0, {key: 'ArrowRight'});
402452
expect(onChangeSpy).toHaveBeenLastCalledWith([11]);
403453
expect(onChangeSpy).toHaveBeenCalledTimes(1);
404454
fireEvent.keyDown(thumb0, {key: 'ArrowUp'});
455+
fireEvent.keyUp(thumb0, {key: 'ArrowUp'});
405456
expect(onChangeSpy).toHaveBeenLastCalledWith([12]);
406457
expect(onChangeSpy).toHaveBeenCalledTimes(2);
407458
fireEvent.keyDown(thumb0, {key: 'ArrowDown'});
459+
fireEvent.keyUp(thumb0, {key: 'ArrowDown'});
408460
expect(onChangeSpy).toHaveBeenLastCalledWith([11]);
409461
expect(onChangeSpy).toHaveBeenCalledTimes(3);
410462
fireEvent.keyDown(thumb0, {key: 'ArrowLeft'});
463+
fireEvent.keyUp(thumb0, {key: 'ArrowLeft'});
411464
expect(onChangeSpy).toHaveBeenLastCalledWith([10]);
412465
expect(onChangeSpy).toHaveBeenCalledTimes(4);
413466
});
467+
468+
it('can be moved with keys (vertical) at the bottom of the slider', () => {
469+
let onChangeSpy = jest.fn();
470+
let onChangeEndSpy = jest.fn();
471+
render(<Example onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} aria-label="Slider" defaultValue={[0]} orientation="vertical" />);
472+
473+
// Drag thumb
474+
let thumb0 = screen.getByTestId('thumb').firstChild;
475+
fireEvent.keyDown(thumb0, {key: 'ArrowDown'});
476+
fireEvent.keyUp(thumb0, {key: 'ArrowDown'});
477+
expect(onChangeSpy).not.toHaveBeenCalled();
478+
expect(onChangeEndSpy).toHaveBeenCalledWith([0]);
479+
480+
fireEvent.keyDown(thumb0, {key: 'ArrowUp'});
481+
fireEvent.keyUp(thumb0, {key: 'ArrowUp'});
482+
expect(onChangeSpy).toHaveBeenLastCalledWith([1]);
483+
expect(onChangeSpy).toHaveBeenCalledTimes(1);
484+
expect(onChangeEndSpy).toHaveBeenLastCalledWith([1]);
485+
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
486+
expect(stateRef.current.values).toEqual([1]);
487+
});
488+
489+
it('can be moved with keys (vertical) at the top of the slider', () => {
490+
let onChangeSpy = jest.fn();
491+
let onChangeEndSpy = jest.fn();
492+
render(<Example onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} aria-label="Slider" defaultValue={[100]} orientation="vertical" />);
493+
494+
// Drag thumb
495+
let thumb0 = screen.getByTestId('thumb').firstChild;
496+
fireEvent.keyDown(thumb0, {key: 'ArrowUp'});
497+
fireEvent.keyUp(thumb0, {key: 'ArrowUp'});
498+
expect(onChangeSpy).not.toHaveBeenCalled();
499+
expect(onChangeEndSpy).toHaveBeenCalledWith([100]);
500+
501+
fireEvent.keyDown(thumb0, {key: 'ArrowDown'});
502+
fireEvent.keyUp(thumb0, {key: 'ArrowDown'});
503+
expect(onChangeSpy).toHaveBeenLastCalledWith([99]);
504+
expect(onChangeSpy).toHaveBeenCalledTimes(1);
505+
expect(onChangeEndSpy).toHaveBeenLastCalledWith([99]);
506+
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
507+
expect(stateRef.current.values).toEqual([99]);
508+
});
414509
});
415510
});
416511
});

packages/@react-spectrum/slider/stories/RangeSlider.stories.tsx

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ storiesOf('Slider/RangeSlider', module)
7171
.add(
7272
'min/max',
7373
() => render({label: 'Label', minValue: 30, maxValue: 70})
74+
)
75+
.add(
76+
'pageSize',
77+
() => render({label: 'Label', minValue: 0, maxValue: 360, pageSize: 15, formatOptions: {style: 'unit', unit: 'degree', unitDisplay: 'narrow'}})
7478
);
7579

7680
function render(props: SpectrumRangeSliderProps = {}) {
@@ -79,5 +83,10 @@ function render(props: SpectrumRangeSliderProps = {}) {
7983
action('change')(v.start, v.end);
8084
};
8185
}
86+
if (props.onChangeEnd == null) {
87+
props.onChangeEnd = (v) => {
88+
action('changeEnd')(v.start, v.end);
89+
};
90+
}
8291
return <RangeSlider {...props} />;
8392
}

packages/@react-spectrum/slider/stories/Slider.stories.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,10 @@ storiesOf('Slider', module)
9696
'step',
9797
() => render({label: 'Label', minValue: 0, maxValue: 100, step: 5})
9898
)
99+
.add(
100+
'pageSize',
101+
() => render({label: 'Label', minValue: 0, maxValue: 360, pageSize: 15, formatOptions: {style: 'unit', unit: 'degree', unitDisplay: 'narrow'}})
102+
)
99103
.add(
100104
'isFilled: true',
101105
() => render({label: 'Label', isFilled: true})
@@ -121,5 +125,8 @@ function render(props: SpectrumSliderProps = {}) {
121125
if (props.onChange == null) {
122126
props.onChange = action('change');
123127
}
128+
if (props.onChangeEnd == null) {
129+
props.onChangeEnd = action('changeEnd');
130+
}
124131
return <Slider {...props} />;
125132
}

packages/@react-spectrum/slider/test/Slider.test.tsx

Lines changed: 48 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -207,13 +207,17 @@ describe('Slider', function () {
207207
});
208208

209209
describe('keyboard interactions', () => {
210-
// Can't test arrow/page up/down, home/end arrows because they are handled by the browser and JSDOM doesn't feel like it.
211-
212210
it.each`
213211
Name | props | commands
214212
${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowRight, result: +1}, {left: press.ArrowLeft, result: -1}]}
215213
${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowRight, result: -1}, {left: press.ArrowLeft, result: +1}]}
216214
${'(left/right arrows, isDisabled)'} | ${{locale: 'de-DE', isDisabled: true}}| ${[{left: press.ArrowRight, result: 0}, {left: press.ArrowLeft, result: 0}]}
215+
${'(up/down arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowUp, result: +1}, {left: press.ArrowDown, result: -1}]}
216+
${'(up/down arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowUp, result: +1}, {left: press.ArrowDown, result: -1}]}
217+
${'(page up/down, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.PageUp, result: +10}, {left: press.PageDown, result: -10}]}
218+
${'(page up/down, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.PageUp, result: +10}, {left: press.PageDown, result: -10}]}
219+
${'(home/end, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.Home, result: -50}, {left: press.End, result: +100}]}
220+
${'(home/end, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.Home, result: -50}, {left: press.End, result: +100}]}
217221
`('$Name moves the slider in the correct direction', function ({props, commands}) {
218222
let tree = render(
219223
<Provider theme={theme} {...props}>
@@ -224,14 +228,53 @@ describe('Slider', function () {
224228
testKeypresses([slider, slider], commands);
225229
});
226230

231+
it.each`
232+
Name | props | commands
233+
${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowRight, result: +1}, {left: press.ArrowLeft, result: -1}]}
234+
${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowRight, result: -1}, {left: press.ArrowLeft, result: +1}]}
235+
${'(left/right arrows, isDisabled)'} | ${{locale: 'de-DE', isDisabled: true}}| ${[{left: press.ArrowRight, result: 0}, {left: press.ArrowLeft, result: 0}]}
236+
${'(up/down arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowUp, result: +1}, {left: press.ArrowDown, result: -1}]}
237+
${'(up/down arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowUp, result: +1}, {left: press.ArrowDown, result: -1}]}
238+
${'(page up/down, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.PageUp, result: +10}, {left: press.PageDown, result: -10}]}
239+
${'(page up/down, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.PageUp, result: +10}, {left: press.PageDown, result: -10}]}
240+
${'(home/end, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.Home, result: -50}, {left: press.End, result: +100}]}
241+
${'(home/end, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.Home, result: -50}, {left: press.End, result: +100}]}
242+
`('$Name moves the slider in the correct direction orientation vertical', function ({props, commands}) {
243+
let tree = render(
244+
<Provider theme={theme} {...props}>
245+
<Slider label="Label" defaultValue={50} minValue={0} maxValue={100} orientation="vertical" />
246+
</Provider>
247+
);
248+
let slider = tree.getByRole('slider');
249+
testKeypresses([slider, slider], commands);
250+
});
251+
227252
it.each`
228253
Name | props | commands
229-
${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowRight, result: +10}, {left: press.ArrowLeft, result: -10}]}
230-
${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowRight, result: -10}, {left: press.ArrowLeft, result: +10}]}
254+
${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowRight, result: +20}, {left: press.ArrowLeft, result: -20}]}
255+
${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowRight, result: -20}, {left: press.ArrowLeft, result: +20}]}
256+
${'(page up/down, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.PageUp, result: +20}, {left: press.PageDown, result: -20}]}
257+
${'(page up/down, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.PageUp, result: +20}, {left: press.PageDown, result: -20}]}
231258
`('$Name respects the step size', function ({props, commands}) {
232259
let tree = render(
233260
<Provider theme={theme} {...props}>
234-
<Slider label="Label" step={10} defaultValue={50} />
261+
<Slider label="Label" step={20} defaultValue={40} />
262+
</Provider>
263+
);
264+
let slider = tree.getByRole('slider');
265+
testKeypresses([slider, slider], commands);
266+
});
267+
268+
it.each`
269+
Name | props | commands
270+
${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowRight, result: +1}, {left: press.ArrowLeft, result: -1}]}
271+
${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowRight, result: -1}, {left: press.ArrowLeft, result: +1}]}
272+
${'(page up/down, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.PageUp, result: +20}, {left: press.PageDown, result: -20}]}
273+
${'(page up/down, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.PageUp, result: +20}, {left: press.PageDown, result: -20}]}
274+
`('$Name respects the page size', function ({props, commands}) {
275+
let tree = render(
276+
<Provider theme={theme} {...props}>
277+
<Slider label="Label" pageSize={20} defaultValue={50} />
235278
</Provider>
236279
);
237280
let slider = tree.getByRole('slider');

0 commit comments

Comments
 (0)