Skip to content

fix(#2659): useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End #2661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Feb 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
353e08f
useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End
majornista Dec 8, 2021
ec21016
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
majornista Dec 20, 2021
6b8f72c
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
majornista Jan 5, 2022
8c26ed7
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
LFDanLu Jan 25, 2022
69630ce
fix(#2659): add onChangeEnd event handler to Slider/RangeSlider stories
majornista Jan 26, 2022
af705c4
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
snowystinger Feb 1, 2022
b50f3c7
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
snowystinger Feb 1, 2022
2e7cd03
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
snowystinger Feb 1, 2022
3150e7e
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
majornista Feb 3, 2022
92cb6ee
Have slider work like color area (#2819)
snowystinger Feb 11, 2022
df0c129
fix(#2659): add pageSize stories
majornista Feb 11, 2022
5117a7d
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
majornista Feb 15, 2022
26b1e55
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
LFDanLu Feb 17, 2022
b08be0d
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
snowystinger Feb 17, 2022
ed3e55a
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
snowystinger Feb 17, 2022
f9e6854
simplify slider move logic
snowystinger Feb 17, 2022
52af63a
a little more cleanup
snowystinger Feb 17, 2022
86f8a39
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
snowystinger Feb 17, 2022
b194983
Merge branch 'main' into Issue-2659-useSliderThumb-onChangeEnd
snowystinger Feb 17, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 60 additions & 11 deletions packages/@react-aria/slider/src/useSliderThumb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import {getSliderThumbId, sliderIds} from './utils';
import React, {ChangeEvent, HTMLAttributes, InputHTMLAttributes, LabelHTMLAttributes, RefObject, useCallback, useEffect, useRef} from 'react';
import {SliderState} from '@react-stately/slider';
import {useFocusable} from '@react-aria/focus';
import {useKeyboard, useMove} from '@react-aria/interactions';
import {useLabel} from '@react-aria/label';
import {useLocale} from '@react-aria/i18n';
import {useMove} from '@react-aria/interactions';

interface SliderThumbAria {
/** Props for the root thumb element; handles the dragging motion. */
Expand Down Expand Up @@ -77,34 +77,82 @@ export function useSliderThumb(
stateRef.current = state;
let reverseX = direction === 'rtl';
let currentPosition = useRef<number>(null);

let {keyboardProps} = useKeyboard({
onKeyDown(e) {
let {
getThumbMaxValue,
getThumbMinValue,
decrementThumb,
incrementThumb,
setThumbValue,
setThumbDragging,
pageSize
} = stateRef.current;
// these are the cases that useMove or useSlider don't handle
if (!/^(PageUp|PageDown|Home|End)$/.test(e.key)) {
e.continuePropagation();
return;
}
// same handling as useMove, stopPropagation to prevent useSlider from handling the event as well.
e.preventDefault();
// remember to set this so that onChangeEnd is fired
setThumbDragging(index, true);
switch (e.key) {
case 'PageUp':
incrementThumb(index, pageSize);
break;
case 'PageDown':
decrementThumb(index, pageSize);
break;
case 'Home':
setThumbValue(index, getThumbMinValue(index));
break;
case 'End':
setThumbValue(index, getThumbMaxValue(index));
break;
}
setThumbDragging(index, false);
}
});

let {moveProps} = useMove({
onMoveStart() {
currentPosition.current = null;
state.setThumbDragging(index, true);
stateRef.current.setThumbDragging(index, true);
},
onMove({deltaX, deltaY, pointerType}) {
onMove({deltaX, deltaY, pointerType, shiftKey}) {
const {
getThumbPercent,
setThumbPercent,
decrementThumb,
incrementThumb,
step,
pageSize
} = stateRef.current;
let size = isVertical ? trackRef.current.offsetHeight : trackRef.current.offsetWidth;

if (currentPosition.current == null) {
currentPosition.current = stateRef.current.getThumbPercent(index) * size;
currentPosition.current = getThumbPercent(index) * size;
}
if (pointerType === 'keyboard') {
// (invert left/right according to language direction) + (according to vertical)
let delta = ((reverseX ? -deltaX : deltaX) + (isVertical ? -deltaY : -deltaY)) * stateRef.current.step;
currentPosition.current += delta * size;
stateRef.current.setThumbValue(index, stateRef.current.getThumbValue(index) + delta);
if ((deltaX > 0 && reverseX) || (deltaX < 0 && !reverseX) || deltaY > 0) {
decrementThumb(index, shiftKey ? pageSize : step);
} else {
incrementThumb(index, shiftKey ? pageSize : step);
}
} else {
let delta = isVertical ? deltaY : deltaX;
if (isVertical || reverseX) {
delta = -delta;
}

currentPosition.current += delta;
stateRef.current.setThumbPercent(index, clamp(currentPosition.current / size, 0, 1));
setThumbPercent(index, clamp(currentPosition.current / size, 0, 1));
}
},
onMoveEnd() {
state.setThumbDragging(index, false);
stateRef.current.setThumbDragging(index, false);
}
});

Expand Down Expand Up @@ -161,10 +209,11 @@ export function useSliderThumb(
'aria-invalid': validationState === 'invalid' || undefined,
'aria-errormessage': opts['aria-errormessage'],
onChange: (e: ChangeEvent<HTMLInputElement>) => {
state.setThumbValue(index, parseFloat(e.target.value));
stateRef.current.setThumbValue(index, parseFloat(e.target.value));
}
}),
thumbProps: !isDisabled ? mergeProps(
keyboardProps,
moveProps,
{
onMouseDown: (e: React.MouseEvent<HTMLElement>) => {
Expand Down
21 changes: 21 additions & 0 deletions packages/@react-aria/slider/stories/Slider.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ storiesOf('Slider (hooks)', module)
'single with aria label',
() => <StorySlider aria-label="Size" onChange={action('onChange')} onChangeEnd={action('onChangeEnd')} showTip />
)
.add(
'single with pageSize',
() => <StorySlider label="Degrees" onChange={action('onChange')} onChangeEnd={action('onChangeEnd')} minValue={0} maxValue={360} pageSize={15} formatOptions={{style: 'unit', unit: 'degree', unitDisplay: 'narrow'}} showTip />
)
.add(
'range',
() => (<StoryRangeSlider
Expand Down Expand Up @@ -56,6 +60,23 @@ storiesOf('Slider (hooks)', module)
unitDisplay: 'narrow'
} as any} />)
)
.add(
'range with pageSize',
() => (<StoryRangeSlider
label="Arc"
defaultValue={[45, 135]}
minValue={0}
maxValue={360}
pageSize={15}
onChange={action('onChange')}
onChangeEnd={action('onChangeEnd')}
showTip
formatOptions={{
style: 'unit',
unit: 'degree',
unitDisplay: 'narrow'
} as any} />)
)
.add(
'3 thumbs',
() => (
Expand Down
95 changes: 95 additions & 0 deletions packages/@react-aria/slider/test/useSliderThumb.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -384,11 +384,60 @@ describe('useSliderThumb', () => {
// Drag thumb
let thumb0 = screen.getByTestId('thumb').firstChild;
fireEvent.keyDown(thumb0, {key: 'ArrowRight'});
fireEvent.keyUp(thumb0, {key: 'ArrowRight'});
expect(onChangeSpy).toHaveBeenLastCalledWith([11]);
expect(onChangeSpy).toHaveBeenCalledTimes(1);
expect(onChangeEndSpy).toHaveBeenLastCalledWith([11]);
expect(onChangeEndSpy).toHaveBeenCalledTimes(1);
expect(stateRef.current.values).toEqual([11]);

fireEvent.keyDown(thumb0, {key: 'ArrowLeft'});
fireEvent.keyUp(thumb0, {key: 'ArrowLeft'});
expect(onChangeSpy).toHaveBeenLastCalledWith([10]);
expect(onChangeSpy).toHaveBeenCalledTimes(2);
expect(onChangeEndSpy).toHaveBeenLastCalledWith([10]);
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
expect(stateRef.current.values).toEqual([10]);
});

it('can be moved with keys at the beginning of the slider', () => {
let onChangeSpy = jest.fn();
let onChangeEndSpy = jest.fn();
render(<Example onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} aria-label="Slider" defaultValue={[0]} />);

let thumb0 = screen.getByTestId('thumb').firstChild;
fireEvent.keyDown(thumb0, {key: 'ArrowLeft'});
fireEvent.keyUp(thumb0, {key: 'ArrowLeft'});
expect(onChangeSpy).not.toHaveBeenCalled();
expect(onChangeEndSpy).toHaveBeenCalledWith([0]);

fireEvent.keyDown(thumb0, {key: 'ArrowRight'});
fireEvent.keyUp(thumb0, {key: 'ArrowRight'});
expect(onChangeSpy).toHaveBeenLastCalledWith([1]);
expect(onChangeSpy).toHaveBeenCalledTimes(1);
expect(onChangeEndSpy).toHaveBeenLastCalledWith([1]);
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
expect(stateRef.current.values).toEqual([1]);
});

it('can be moved with keys at the end of the slider', () => {
let onChangeSpy = jest.fn();
let onChangeEndSpy = jest.fn();
render(<Example onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} aria-label="Slider" defaultValue={[100]} />);

let thumb0 = screen.getByTestId('thumb').firstChild;
fireEvent.keyDown(thumb0, {key: 'ArrowRight'});
fireEvent.keyUp(thumb0, {key: 'ArrowRight'});
expect(onChangeSpy).not.toHaveBeenCalled();
expect(onChangeEndSpy).toHaveBeenCalledWith([100]);

fireEvent.keyDown(thumb0, {key: 'ArrowLeft'});
fireEvent.keyUp(thumb0, {key: 'ArrowLeft'});
expect(onChangeSpy).toHaveBeenLastCalledWith([99]);
expect(onChangeSpy).toHaveBeenCalledTimes(1);
expect(onChangeEndSpy).toHaveBeenLastCalledWith([99]);
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
expect(stateRef.current.values).toEqual([99]);
});

it('can be moved with keys (vertical)', () => {
Expand All @@ -399,18 +448,64 @@ describe('useSliderThumb', () => {
// Drag thumb
let thumb0 = screen.getByTestId('thumb').firstChild;
fireEvent.keyDown(thumb0, {key: 'ArrowRight'});
fireEvent.keyUp(thumb0, {key: 'ArrowRight'});
expect(onChangeSpy).toHaveBeenLastCalledWith([11]);
expect(onChangeSpy).toHaveBeenCalledTimes(1);
fireEvent.keyDown(thumb0, {key: 'ArrowUp'});
fireEvent.keyUp(thumb0, {key: 'ArrowUp'});
expect(onChangeSpy).toHaveBeenLastCalledWith([12]);
expect(onChangeSpy).toHaveBeenCalledTimes(2);
fireEvent.keyDown(thumb0, {key: 'ArrowDown'});
fireEvent.keyUp(thumb0, {key: 'ArrowDown'});
expect(onChangeSpy).toHaveBeenLastCalledWith([11]);
expect(onChangeSpy).toHaveBeenCalledTimes(3);
fireEvent.keyDown(thumb0, {key: 'ArrowLeft'});
fireEvent.keyUp(thumb0, {key: 'ArrowLeft'});
expect(onChangeSpy).toHaveBeenLastCalledWith([10]);
expect(onChangeSpy).toHaveBeenCalledTimes(4);
});

it('can be moved with keys (vertical) at the bottom of the slider', () => {
let onChangeSpy = jest.fn();
let onChangeEndSpy = jest.fn();
render(<Example onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} aria-label="Slider" defaultValue={[0]} orientation="vertical" />);

// Drag thumb
let thumb0 = screen.getByTestId('thumb').firstChild;
fireEvent.keyDown(thumb0, {key: 'ArrowDown'});
fireEvent.keyUp(thumb0, {key: 'ArrowDown'});
expect(onChangeSpy).not.toHaveBeenCalled();
expect(onChangeEndSpy).toHaveBeenCalledWith([0]);

fireEvent.keyDown(thumb0, {key: 'ArrowUp'});
fireEvent.keyUp(thumb0, {key: 'ArrowUp'});
expect(onChangeSpy).toHaveBeenLastCalledWith([1]);
expect(onChangeSpy).toHaveBeenCalledTimes(1);
expect(onChangeEndSpy).toHaveBeenLastCalledWith([1]);
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
expect(stateRef.current.values).toEqual([1]);
});

it('can be moved with keys (vertical) at the top of the slider', () => {
let onChangeSpy = jest.fn();
let onChangeEndSpy = jest.fn();
render(<Example onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} aria-label="Slider" defaultValue={[100]} orientation="vertical" />);

// Drag thumb
let thumb0 = screen.getByTestId('thumb').firstChild;
fireEvent.keyDown(thumb0, {key: 'ArrowUp'});
fireEvent.keyUp(thumb0, {key: 'ArrowUp'});
expect(onChangeSpy).not.toHaveBeenCalled();
expect(onChangeEndSpy).toHaveBeenCalledWith([100]);

fireEvent.keyDown(thumb0, {key: 'ArrowDown'});
fireEvent.keyUp(thumb0, {key: 'ArrowDown'});
expect(onChangeSpy).toHaveBeenLastCalledWith([99]);
expect(onChangeSpy).toHaveBeenCalledTimes(1);
expect(onChangeEndSpy).toHaveBeenLastCalledWith([99]);
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
expect(stateRef.current.values).toEqual([99]);
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ storiesOf('Slider/RangeSlider', module)
.add(
'min/max',
() => render({label: 'Label', minValue: 30, maxValue: 70})
)
.add(
'pageSize',
() => render({label: 'Label', minValue: 0, maxValue: 360, pageSize: 15, formatOptions: {style: 'unit', unit: 'degree', unitDisplay: 'narrow'}})
);

function render(props: SpectrumRangeSliderProps = {}) {
Expand All @@ -79,5 +83,10 @@ function render(props: SpectrumRangeSliderProps = {}) {
action('change')(v.start, v.end);
};
}
if (props.onChangeEnd == null) {
props.onChangeEnd = (v) => {
action('changeEnd')(v.start, v.end);
};
}
return <RangeSlider {...props} />;
}
7 changes: 7 additions & 0 deletions packages/@react-spectrum/slider/stories/Slider.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ storiesOf('Slider', module)
'step',
() => render({label: 'Label', minValue: 0, maxValue: 100, step: 5})
)
.add(
'pageSize',
() => render({label: 'Label', minValue: 0, maxValue: 360, pageSize: 15, formatOptions: {style: 'unit', unit: 'degree', unitDisplay: 'narrow'}})
)
.add(
'isFilled: true',
() => render({label: 'Label', isFilled: true})
Expand All @@ -121,5 +125,8 @@ function render(props: SpectrumSliderProps = {}) {
if (props.onChange == null) {
props.onChange = action('change');
}
if (props.onChangeEnd == null) {
props.onChangeEnd = action('changeEnd');
}
return <Slider {...props} />;
}
53 changes: 48 additions & 5 deletions packages/@react-spectrum/slider/test/Slider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -207,13 +207,17 @@ describe('Slider', function () {
});

describe('keyboard interactions', () => {
// Can't test arrow/page up/down, home/end arrows because they are handled by the browser and JSDOM doesn't feel like it.

it.each`
Name | props | commands
${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowRight, result: +1}, {left: press.ArrowLeft, result: -1}]}
${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowRight, result: -1}, {left: press.ArrowLeft, result: +1}]}
${'(left/right arrows, isDisabled)'} | ${{locale: 'de-DE', isDisabled: true}}| ${[{left: press.ArrowRight, result: 0}, {left: press.ArrowLeft, result: 0}]}
${'(up/down arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowUp, result: +1}, {left: press.ArrowDown, result: -1}]}
${'(up/down arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowUp, result: +1}, {left: press.ArrowDown, result: -1}]}
${'(page up/down, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.PageUp, result: +10}, {left: press.PageDown, result: -10}]}
${'(page up/down, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.PageUp, result: +10}, {left: press.PageDown, result: -10}]}
${'(home/end, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.Home, result: -50}, {left: press.End, result: +100}]}
${'(home/end, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.Home, result: -50}, {left: press.End, result: +100}]}
`('$Name moves the slider in the correct direction', function ({props, commands}) {
let tree = render(
<Provider theme={theme} {...props}>
Expand All @@ -224,14 +228,53 @@ describe('Slider', function () {
testKeypresses([slider, slider], commands);
});

it.each`
Name | props | commands
${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowRight, result: +1}, {left: press.ArrowLeft, result: -1}]}
${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowRight, result: -1}, {left: press.ArrowLeft, result: +1}]}
${'(left/right arrows, isDisabled)'} | ${{locale: 'de-DE', isDisabled: true}}| ${[{left: press.ArrowRight, result: 0}, {left: press.ArrowLeft, result: 0}]}
${'(up/down arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowUp, result: +1}, {left: press.ArrowDown, result: -1}]}
${'(up/down arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowUp, result: +1}, {left: press.ArrowDown, result: -1}]}
${'(page up/down, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.PageUp, result: +10}, {left: press.PageDown, result: -10}]}
${'(page up/down, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.PageUp, result: +10}, {left: press.PageDown, result: -10}]}
${'(home/end, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.Home, result: -50}, {left: press.End, result: +100}]}
${'(home/end, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.Home, result: -50}, {left: press.End, result: +100}]}
`('$Name moves the slider in the correct direction orientation vertical', function ({props, commands}) {
let tree = render(
<Provider theme={theme} {...props}>
<Slider label="Label" defaultValue={50} minValue={0} maxValue={100} orientation="vertical" />
</Provider>
);
let slider = tree.getByRole('slider');
testKeypresses([slider, slider], commands);
});

it.each`
Name | props | commands
${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowRight, result: +10}, {left: press.ArrowLeft, result: -10}]}
${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowRight, result: -10}, {left: press.ArrowLeft, result: +10}]}
${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowRight, result: +20}, {left: press.ArrowLeft, result: -20}]}
${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowRight, result: -20}, {left: press.ArrowLeft, result: +20}]}
${'(page up/down, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.PageUp, result: +20}, {left: press.PageDown, result: -20}]}
${'(page up/down, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.PageUp, result: +20}, {left: press.PageDown, result: -20}]}
`('$Name respects the step size', function ({props, commands}) {
let tree = render(
<Provider theme={theme} {...props}>
<Slider label="Label" step={10} defaultValue={50} />
<Slider label="Label" step={20} defaultValue={40} />
</Provider>
);
let slider = tree.getByRole('slider');
testKeypresses([slider, slider], commands);
});

it.each`
Name | props | commands
${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.ArrowRight, result: +1}, {left: press.ArrowLeft, result: -1}]}
${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.ArrowRight, result: -1}, {left: press.ArrowLeft, result: +1}]}
${'(page up/down, ltr)'} | ${{locale: 'de-DE'}} | ${[{left: press.PageUp, result: +20}, {left: press.PageDown, result: -20}]}
${'(page up/down, rtl)'} | ${{locale: 'ar-AE'}} | ${[{left: press.PageUp, result: +20}, {left: press.PageDown, result: -20}]}
`('$Name respects the page size', function ({props, commands}) {
let tree = render(
<Provider theme={theme} {...props}>
<Slider label="Label" pageSize={20} defaultValue={50} />
</Provider>
);
let slider = tree.getByRole('slider');
Expand Down
Loading