fix(#2664): useColorSlider: refine PageUp/PageDown behaviors

packages/@react-aria/color/src/useColorField.ts

@@ -85,7 +85,7 @@ export function useColorField(
     } else if (e.deltaY < 0) {
       decrement();
     }
-  }, [isReadOnly, isDisabled, decrement, increment]);
+  }, [decrement, increment]);
   // If the input isn't supposed to receive input, disable scrolling.
   let scrollingDisabled = isDisabled || isReadOnly || !focusWithin;
   useScrollWheel({onScroll: onWheel, isDisabled: scrollingDisabled}, ref);

packages/@react-aria/color/src/useColorWheel.ts

@@ -255,8 +255,8 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState
     ...props,
     'aria-label': ariaLabel
   });
-  let step = stateRef.current.step;
 
+  let {minValue, maxValue, step} = state.value.getChannelRange('hue');
   return {
     trackProps: {
       ...trackInteractions,
@@ -300,8 +300,8 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState
       inputLabellingProps,
       {
         type: 'range',
-        min: '0',
-        max: '360',
+        min: String(minValue),
+        max: String(maxValue),
         step: String(step),
         'aria-valuetext': state.value.formatChannelValue('hue', locale),
         disabled: isDisabled,

packages/@react-spectrum/color/stories/ColorArea.stories.tsx

@@ -84,7 +84,7 @@ XBlueYRed.storyName = 'RGB xChannel="blue", yChannel="red"';
 XBlueYRed.args = {...XBlueYGreen.args, xChannel: 'blue', yChannel: 'red'};
 
 export let XRedYBlue = Template.bind({});
-XRedYBlue.storyName = 'GB xChannel="red", yChannel="blue"';
+XRedYBlue.storyName = 'RGB xChannel="red", yChannel="blue"';
 XRedYBlue.args = {...XBlueYGreen.args, xChannel: 'red', yChannel: 'blue'};
 
 export let XRedYGreen = Template.bind({});

packages/@react-spectrum/color/stories/ColorField.stories.tsx

@@ -70,10 +70,6 @@ storiesOf('ColorField', module)
     'with placeholder',
     () => render({placeholder: 'Enter a hex color'})
   )
-  .add(
-    'step = 16',
-    () => render({step: 16})
-  )
   .add(
     'controlled value',
     () => (

packages/@react-spectrum/color/stories/ColorSlider.stories.tsx

@@ -20,23 +20,19 @@ import {Text} from '@react-spectrum/text';
 storiesOf('ColorSlider', module)
   .add(
     'default',
-    () => <ColorSlider defaultValue="#7f0000" channel={'red'} />
+    () => <ColorSlider defaultValue="#800000" channel={'red'} />
   )
   .add(
     'no label, default aria-label',
-    () => <ColorSlider defaultValue="#7f0000" channel={'red'} label={null} />
+    () => <ColorSlider defaultValue="#800000" channel={'red'} label={null} />
   )
   .add(
     'no value label',
-    () => <ColorSlider defaultValue="#7f0000" channel={'red'} showValueLabel={false} />
+    () => <ColorSlider defaultValue="#800000" channel={'red'} showValueLabel={false} />
   )
   .add(
     'custom aria-label',
-    () => <ColorSlider defaultValue="#7f0000" channel={'red'} aria-label="Color Picker Channel: Red" />
-  )
-  .add(
-    'step',
-    () => <ColorSlider defaultValue="hsl(0, 100%, 50%)" channel={'hue'} step={72} />
+    () => <ColorSlider defaultValue="#800000" channel={'red'} aria-label="Color Picker Channel: Red" />
   )
   .add(
     'disabled',
@@ -52,11 +48,11 @@ storiesOf('ColorSlider', module)
   )
   .add(
     'custom width',
-    () => <ColorSlider defaultValue="#7f0000" channel={'red'} width={300} />
+    () => <ColorSlider defaultValue="#800000" channel={'red'} width={300} />
   )
   .add(
     'custom height',
-    () => <ColorSlider defaultValue="#7f0000" channel={'red'} orientation="vertical" height={300} />
+    () => <ColorSlider defaultValue="#800000" channel={'red'} orientation="vertical" height={300} />
   )
   .add(
     'rgba',

packages/@react-spectrum/color/test/ColorField.test.js

@@ -283,14 +283,13 @@ describe('ColorField', function () {
 
   it.each`
     Name                                | expected                 | key
-    ${'increment with arrow up key'}    | ${parseColor('#AAAAAE')}  | ${'ArrowUp'}
-    ${'decrement with arrow down key'}  | ${parseColor('#AAAAA6')}  | ${'ArrowDown'}
+    ${'increment with arrow up key'}    | ${parseColor('#AAAAAB')}  | ${'ArrowUp'}
+    ${'decrement with arrow down key'}  | ${parseColor('#AAAAA9')}  | ${'ArrowDown'}
   `('should handle $Name event', function ({expected, key}) {
     let onChangeSpy = jest.fn();
     let {getByLabelText} = renderComponent({
       defaultValue: '#aaa',
-      onChange: onChangeSpy,
-      step: 4
+      onChange: onChangeSpy
     });
     let colorField = getByLabelText('Primary Color');
     expect(colorField.value).toBe('#AAAAAA');
@@ -303,14 +302,13 @@ describe('ColorField', function () {
 
   it.each`
     Name                                | expected                        | deltaY
-    ${'increment with mouse wheel'}     | ${parseColor('#AAAAAE')}  | ${10}
-    ${'decrement with mouse wheel'}     | ${parseColor('#AAAAA6')}  | ${-10}
+    ${'increment with mouse wheel'}     | ${parseColor('#AAAAAB')}  | ${10}
+    ${'decrement with mouse wheel'}     | ${parseColor('#AAAAA9')}  | ${-10}
   `('should handle $Name event', function ({expected, deltaY}) {
     let onChangeSpy = jest.fn();
     let {getByLabelText} = renderComponent({
       defaultValue: '#aaa',
-      onChange: onChangeSpy,
-      step: 4
+      onChange: onChangeSpy
     });
     let colorField = getByLabelText('Primary Color');
     expect(colorField.value).toBe('#AAAAAA');

packages/@react-spectrum/color/test/ColorSlider.test.tsx

@@ -19,6 +19,7 @@ import userEvent from '@testing-library/user-event';
 
 describe('ColorSlider', () => {
   let onChangeSpy = jest.fn();
+  let onChangeEndSpy = jest.fn();
 
   beforeAll(() => {
     jest.spyOn(window.HTMLElement.prototype, 'offsetWidth', 'get').mockImplementation(() => 100);
@@ -31,6 +32,7 @@ describe('ColorSlider', () => {
 
   afterEach(() => {
     onChangeSpy.mockClear();
+    onChangeEndSpy.mockClear();
     // for restoreTextSelection
     act(() => {jest.runAllTimers();});
   });
@@ -264,16 +266,65 @@ describe('ColorSlider', () => {
   describe('keyboard events', () => {
     it('works', () => {
       let defaultColor = parseColor('#000000');
-      let {getByRole} = render(<ColorSlider defaultValue={defaultColor} onChange={onChangeSpy} channel="red" />);
+      let {getByRole} = render(<ColorSlider defaultValue={defaultColor} onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} channel="red" />);
       let slider = getByRole('slider');
       act(() => {slider.focus();});
 
       fireEvent.keyDown(slider, {key: 'Right'});
+      act(() => {jest.runAllTimers();});
       expect(onChangeSpy).toHaveBeenCalledTimes(1);
       expect(onChangeSpy.mock.calls[0][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa'));
+      expect(onChangeEndSpy).toHaveBeenCalledTimes(1);
+      expect(onChangeEndSpy.mock.calls[0][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa'));
+
       fireEvent.keyDown(slider, {key: 'Left'});
+      act(() => {jest.runAllTimers();});
       expect(onChangeSpy).toHaveBeenCalledTimes(2);
       expect(onChangeSpy.mock.calls[1][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
+      expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
+      expect(onChangeEndSpy.mock.calls[1][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
+
+      fireEvent.keyDown(slider, {key: 'PageUp'});
+      act(() => {jest.runAllTimers();});
+      expect(onChangeSpy).toHaveBeenCalledTimes(3);
+      expect(onChangeSpy.mock.calls[2][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 17).toString('hexa'));
+      expect(onChangeEndSpy).toHaveBeenCalledTimes(3);
+      expect(onChangeEndSpy.mock.calls[2][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 17).toString('hexa'));
+
+      fireEvent.keyDown(slider, {key: 'Right'});
+      act(() => {jest.runAllTimers();});
+      expect(onChangeSpy).toHaveBeenCalledTimes(4);
+      expect(onChangeSpy.mock.calls[3][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 18).toString('hexa'));
+      expect(onChangeEndSpy).toHaveBeenCalledTimes(4);
+      expect(onChangeEndSpy.mock.calls[3][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 18).toString('hexa'));
+
+      fireEvent.keyDown(slider, {key: 'PageDown'});
+      act(() => {jest.runAllTimers();});
+      expect(onChangeSpy).toHaveBeenCalledTimes(5);
+      expect(onChangeSpy.mock.calls[4][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa'));
+      expect(onChangeEndSpy).toHaveBeenCalledTimes(5);
+      expect(onChangeEndSpy.mock.calls[4][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa'));
+
+      fireEvent.keyDown(slider, {key: 'End'});
+      act(() => {jest.runAllTimers();});
+      expect(onChangeSpy).toHaveBeenCalledTimes(6);
+      expect(onChangeSpy.mock.calls[5][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 255).toString('hexa'));
+      expect(onChangeEndSpy).toHaveBeenCalledTimes(6);
+      expect(onChangeEndSpy.mock.calls[5][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 255).toString('hexa'));
+
+      fireEvent.keyDown(slider, {key: 'PageDown'});
+      act(() => {jest.runAllTimers();});
+      expect(onChangeSpy).toHaveBeenCalledTimes(7);
+      expect(onChangeSpy.mock.calls[6][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 238).toString('hexa'));
+      expect(onChangeEndSpy).toHaveBeenCalledTimes(7);
+      expect(onChangeEndSpy.mock.calls[6][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 238).toString('hexa'));
+
+      fireEvent.keyDown(slider, {key: 'Home'});
+      act(() => {jest.runAllTimers();});
+      expect(onChangeSpy).toHaveBeenCalledTimes(8);
+      expect(onChangeSpy.mock.calls[7][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
+      expect(onChangeEndSpy).toHaveBeenCalledTimes(8);
+      expect(onChangeEndSpy.mock.calls[7][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
     });
 
     it('doesn\'t work when disabled', () => {
@@ -287,20 +338,6 @@ describe('ColorSlider', () => {
       fireEvent.keyDown(slider, {key: 'Left'});
       expect(onChangeSpy).toHaveBeenCalledTimes(0);
     });
-
-    it('respects step', () => {
-      let defaultColor = parseColor('#000000');
-      let {getByRole} = render(<ColorSlider defaultValue={defaultColor} onChange={onChangeSpy} channel="red" step={10} />);
-      let slider = getByRole('slider');
-      act(() => {slider.focus();});
-
-      fireEvent.keyDown(slider, {key: 'Right'});
-      expect(onChangeSpy).toHaveBeenCalledTimes(1);
-      expect(onChangeSpy.mock.calls[0][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 10).toString('hexa'));
-      fireEvent.keyDown(slider, {key: 'Left'});
-      expect(onChangeSpy).toHaveBeenCalledTimes(2);
-      expect(onChangeSpy.mock.calls[1][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
-    });
   });
 
   describe.each`
@@ -385,21 +422,6 @@ describe('ColorSlider', () => {
       expect(document.activeElement).not.toBe(slider);
     });
 
-    it('dragging the thumb respects the step', () => {
-      let defaultColor = parseColor('hsl(0, 100%, 50%)');
-      let {getByRole} = render(<ColorSlider channel="hue" defaultValue={defaultColor} onChange={onChangeSpy} step={120} />);
-      let slider = getByRole('slider');
-      let thumb = slider.parentElement;
-
-      start(thumb, {pageX: 0});
-      expect(onChangeSpy).toHaveBeenCalledTimes(0);
-      move(thumb, {pageX: 20});
-      expect(onChangeSpy).toHaveBeenCalledTimes(1);
-      expect(onChangeSpy.mock.calls[0][0].toString('hsla')).toBe(defaultColor.withChannelValue('hue', 120).toString('hsla'));
-      end(thumb, {pageX: 20});
-      expect(onChangeSpy).toHaveBeenCalledTimes(1);
-    });
-
     it('clicking and dragging on the track works', () => {
       let defaultColor = parseColor('hsl(0, 100%, 50%)');
       let {getByRole} = render(<ColorSlider channel="hue" defaultValue={defaultColor} onChange={onChangeSpy} />);
@@ -465,22 +487,5 @@ describe('ColorSlider', () => {
       expect(onChangeSpy).toHaveBeenCalledTimes(0);
       expect(document.activeElement).not.toBe(slider);
     });
-
-    it('clicking and dragging on the track respects the step', () => {
-      let defaultColor = parseColor('hsl(0, 100%, 50%)');
-      let {getByRole} = render(<ColorSlider channel="saturation" defaultValue={defaultColor} onChange={onChangeSpy} step={25} />);
-      let slider = getByRole('slider');
-      let thumb = slider.parentElement;
-      let container = getByRole('group');
-
-      start(container, {pageX: 30});
-      expect(onChangeSpy).toHaveBeenCalledTimes(1);
-      expect(onChangeSpy.mock.calls[0][0].toString('hsla')).toBe(defaultColor.withChannelValue('saturation', 25).toString('hsla'));
-      move(thumb, {pageX: 50});
-      expect(onChangeSpy).toHaveBeenCalledTimes(2);
-      expect(onChangeSpy.mock.calls[1][0].toString('hsla')).toBe(defaultColor.withChannelValue('saturation', 50).toString('hsla'));
-      end(thumb, {pageX: 50});
-      expect(onChangeSpy).toHaveBeenCalledTimes(2);
-    });
   });
 });

packages/@react-stately/color/src/useColorFieldState.ts

@@ -14,7 +14,7 @@ import {Color, ColorFieldProps} from '@react-types/color';
 import {parseColor} from './Color';
 import {useColor} from './useColor';
 import {useControlledState} from '@react-stately/utils';
-import {useEffect, useMemo, useRef, useState} from 'react';
+import {useMemo, useRef, useState} from 'react';
 
 export interface ColorFieldState {
   /**
@@ -63,11 +63,11 @@ export function useColorFieldState(
   props: ColorFieldProps
 ): ColorFieldState {
   let {
-    step = 1,
     value,
     defaultValue,
     onChange
   } = props;
+  let {step} = MIN_COLOR.getChannelRange('red');
 
   let initialValue = useColor(value);
   let initialDefaultValue = useColor(defaultValue);
@@ -85,9 +85,12 @@ export function useColorFieldState(
     }
   };
 
-  useEffect(() => {
+  let prevValue = useRef(colorValue);
+  if (prevValue.current !== colorValue) {
     setInputValue(colorValue ? colorValue.toString('hex') : '');
-  }, [colorValue, setInputValue]);
+    prevValue.current = colorValue;
+  }
+
 
   let parsedValue = useMemo(() => {
     let color;

packages/@react-stately/color/src/useColorSliderState.ts

@@ -51,7 +51,7 @@ export function useColorSliderState(props: ColorSliderStateOptions): ColorSlider
       setColor(color.withChannelValue(channel, v));
     },
     onChangeEnd([v]) {
-      // onChange will have already been called with the right value, this is just to trigger onChangEnd
+      // onChange will have already been called with the right value, this is just to trigger onChangeEnd
       if (props.onChangeEnd) {
         props.onChangeEnd(color.withChannelValue(channel, v));
       }

packages/@react-stately/color/src/useColorWheelState.ts

@@ -101,6 +101,8 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
   let valueRef = useRef(value);
   valueRef.current = value;
 
+  let channelRange = value.getChannelRange('hue');
+  let {minValue: minValueX, maxValue: maxValueX, step: step, pageSize: pageStep} = channelRange;
   let [isDragging, setDragging] = useState(false);
   let isDraggingRef = useRef(false).current;
 
@@ -118,9 +120,6 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
     }
   }
 
-  let channelRange = value.getChannelRange('hue');
-  let {minValue: minValueX, maxValue: maxValueX, step: step, pageSize: pageStep} = channelRange;
-
   return {
     value,
     step,
@@ -139,12 +138,13 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
       return angleToCartesian(value.getChannelValue('hue'), radius);
     },
     increment(stepSize = 1) {
-      let newValue = hue + Math.max(stepSize, step);
-      if (newValue > maxValueX) {
+      let s = Math.max(stepSize, step);
+      let newValue = hue + s;
+      if (newValue >= maxValueX) {
         // Make sure you can always get back to 0.
         newValue = minValueX;
       }
-      setHue(newValue);
+      setHue(roundToStep(mod(newValue, 360), s));
     },
     decrement(stepSize = 1) {
       let s = Math.max(stepSize, step);
@@ -153,7 +153,7 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
         // |(previous step) - 0| < step size
         setHue(roundDown(360 / s) * s);
       } else {
-        setHue(hue - s);
+        setHue(roundToStep(mod(hue - s, 360), s));
       }
     },
     setDragging(isDragging) {

packages/@react-stately/color/test/useColorFieldState.test.js

@@ -42,20 +42,6 @@ describe('useColorFieldState tests', function () {
     expect(result.current.inputValue).toBe('#AABBCC');
   });
 
-  it.each`
-    action          | props
-    ${'increment'}  | ${{defaultValue: '#000008', step: 4}}
-    ${'decrement'}  | ${{defaultValue: '#000010', step: 4}}
-  `('should $action', function ({action, props}) {
-    let {result} = renderHook(() => useColorFieldState(props));
-    act(() => result.current[action]());
-    expect(result.current.colorValue.getChannelValue('red')).toBe(0);
-    expect(result.current.colorValue.getChannelValue('green')).toBe(0);
-    expect(result.current.colorValue.getChannelValue('blue')).toBe(12);
-    expect(result.current.colorValue.getChannelValue('alpha')).toBe(1);
-    expect(result.current.inputValue).toBe('#00000C');
-  });
-
   it.each`
     name                                 | action               | props
     ${'not increment beyond max value'}  | ${'increment'}       | ${{defaultValue: '#ffffff'}}