Skip to content

Commit 4b5f4a9

Browse files
reidbarbersnowystinger
reidbarber
and
snowystinger
authored
Fix double onChangeEnd in ColorWheel (#2448)
* use ref for useColorWheel dragging state * conditionally add pointer event listeners * update value ref on hue change * add controlled hsl story * add test for clicking on track * fix click test to start and end at same position Co-authored-by: Robert Snow <[email protected]>
1 parent 74e31fa commit 4b5f4a9

File tree

4 files changed

+97
-34
lines changed

4 files changed

+97
-34
lines changed

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

Lines changed: 43 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -116,9 +116,13 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState
116116
currentPointer.current = id;
117117
focusInput();
118118
state.setDragging(true);
119-
addGlobalListener(window, 'mouseup', onThumbUp, false);
120-
addGlobalListener(window, 'touchend', onThumbUp, false);
121-
addGlobalListener(window, 'pointerup', onThumbUp, false);
119+
120+
if (typeof PointerEvent !== 'undefined') {
121+
addGlobalListener(window, 'pointerup', onThumbUp, false);
122+
} else {
123+
addGlobalListener(window, 'mouseup', onThumbUp, false);
124+
addGlobalListener(window, 'touchend', onThumbUp, false);
125+
}
122126
}
123127
};
124128

@@ -130,9 +134,12 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState
130134
currentPointer.current = undefined;
131135
isOnTrack.current = false;
132136

133-
removeGlobalListener(window, 'mouseup', onThumbUp, false);
134-
removeGlobalListener(window, 'touchend', onThumbUp, false);
135-
removeGlobalListener(window, 'pointerup', onThumbUp, false);
137+
if (typeof PointerEvent !== 'undefined') {
138+
removeGlobalListener(window, 'pointerup', onThumbUp, false);
139+
} else {
140+
removeGlobalListener(window, 'mouseup', onThumbUp, false);
141+
removeGlobalListener(window, 'touchend', onThumbUp, false);
142+
}
136143
}
137144
};
138145

@@ -149,9 +156,12 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState
149156
focusInput();
150157
state.setDragging(true);
151158

152-
addGlobalListener(window, 'mouseup', onTrackUp, false);
153-
addGlobalListener(window, 'touchend', onTrackUp, false);
154-
addGlobalListener(window, 'pointerup', onTrackUp, false);
159+
if (typeof PointerEvent !== 'undefined') {
160+
addGlobalListener(window, 'pointerup', onTrackUp, false);
161+
} else {
162+
addGlobalListener(window, 'mouseup', onTrackUp, false);
163+
addGlobalListener(window, 'touchend', onTrackUp, false);
164+
}
155165
}
156166
};
157167

@@ -163,9 +173,13 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState
163173
state.setDragging(false);
164174
focusInput();
165175

166-
removeGlobalListener(window, 'mouseup', onTrackUp, false);
167-
removeGlobalListener(window, 'touchend', onTrackUp, false);
168-
removeGlobalListener(window, 'pointerup', onTrackUp, false);
176+
177+
if (typeof PointerEvent !== 'undefined') {
178+
removeGlobalListener(window, 'pointerup', onTrackUp, false);
179+
} else {
180+
removeGlobalListener(window, 'mouseup', onTrackUp, false);
181+
removeGlobalListener(window, 'touchend', onTrackUp, false);
182+
}
169183
}
170184
};
171185

@@ -185,21 +199,23 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState
185199
});
186200

187201
let trackInteractions = isDisabled ? {} : mergeProps({
188-
onMouseDown: (e: React.MouseEvent) => {
189-
if (e.button !== 0 || e.altKey || e.ctrlKey || e.metaKey) {
190-
return;
191-
}
192-
onTrackDown(e.currentTarget, undefined, e.clientX, e.clientY);
193-
},
194-
onPointerDown: (e: React.PointerEvent) => {
195-
if (e.pointerType === 'mouse' && (e.button !== 0 || e.altKey || e.ctrlKey || e.metaKey)) {
196-
return;
197-
}
198-
onTrackDown(e.currentTarget, e.pointerId, e.clientX, e.clientY);
199-
},
200-
onTouchStart: (e: React.TouchEvent) => {
201-
onTrackDown(e.currentTarget, e.changedTouches[0].identifier, e.changedTouches[0].clientX, e.changedTouches[0].clientY);
202-
}
202+
...(typeof PointerEvent !== 'undefined' ? {
203+
onPointerDown: (e: React.PointerEvent) => {
204+
if (e.pointerType === 'mouse' && (e.button !== 0 || e.altKey || e.ctrlKey || e.metaKey)) {
205+
return;
206+
}
207+
onTrackDown(e.currentTarget, e.pointerId, e.clientX, e.clientY);
208+
}} : {
209+
onMouseDown: (e: React.MouseEvent) => {
210+
if (e.button !== 0 || e.altKey || e.ctrlKey || e.metaKey) {
211+
return;
212+
}
213+
onTrackDown(e.currentTarget, undefined, e.clientX, e.clientY);
214+
},
215+
onTouchStart: (e: React.TouchEvent) => {
216+
onTrackDown(e.currentTarget, e.changedTouches[0].identifier, e.changedTouches[0].clientX, e.changedTouches[0].clientY);
217+
}
218+
})
203219
}, movePropsContainer);
204220

205221
let thumbInteractions = isDisabled ? {} : mergeProps({

‎packages/@react-spectrum/color/stories/ColorWheel.stories.tsx

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,4 +54,27 @@ storiesOf('ColorWheel', module)
5454
<div style={{width: '50px', height: '50px', backgroundColor: colorCSS, border: '1px solid black'}} />
5555
</Flex>);
5656
}
57+
)
58+
.add(
59+
'controlled hsl',
60+
() => <Flex gap={'size-500'} direction="row" alignItems="center"><ControlledHSL onChangeEnd={action('onChangeEnd')} /></Flex>
61+
);
62+
63+
export function ControlledHSL(props) {
64+
let [color, setColor] = useState(props.defaultValue || parseColor('hsl(0, 100%, 50%)'));
65+
let colorCSS = color.toString('css');
66+
let onChangeEnd = (color) => {
67+
props.onChangeEnd && props.onChangeEnd(color);
68+
setColor(color);
69+
};
70+
let onChange = (color) => {
71+
props.onChange && props.onChange(color);
72+
setColor(color);
73+
};
74+
return (
75+
<>
76+
<ColorWheel onChange={onChange} onChangeEnd={onChangeEnd} value={color.toString('hsl')} />
77+
<div style={{width: '50px', height: '50px', backgroundColor: colorCSS, border: '1px solid black'}} />
78+
</>
5779
);
80+
}

‎packages/@react-spectrum/color/test/ColorWheel.test.tsx

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
import {act, fireEvent, render} from '@testing-library/react';
1414
import {ColorWheel} from '../';
15+
import {ControlledHSL} from '../stories/ColorWheel.stories';
1516
import {installMouseEvent, installPointerEvent} from '@react-spectrum/test-utils';
1617
import {parseColor} from '@react-stately/color';
1718
import React from 'react';
@@ -329,5 +330,24 @@ describe('ColorWheel', () => {
329330
end(thumb, {pageX: CENTER, pageY: CENTER - THUMB_RADIUS});
330331
expect(onChangeSpy).toHaveBeenCalledTimes(2);
331332
});
333+
334+
it('clicking on the track works', () => {
335+
let defaultColor = parseColor('hsl(0, 100%, 50%)');
336+
let {container: _container, getByRole} = render(<ControlledHSL defaultValue={defaultColor} onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} />);
337+
let slider = getByRole('slider');
338+
let container = _container.firstChild.firstChild as HTMLElement;
339+
container.getBoundingClientRect = getBoundingClientRect;
340+
341+
expect(document.activeElement).not.toBe(slider);
342+
start(container, {pageX: CENTER, pageY: CENTER + THUMB_RADIUS});
343+
expect(onChangeSpy).toHaveBeenCalledTimes(1);
344+
expect(onChangeSpy.mock.calls[0][0].toString('hsla')).toBe(defaultColor.withChannelValue('hue', 90).toString('hsla'));
345+
expect(document.activeElement).toBe(slider);
346+
347+
end(container, {pageX: CENTER, pageY: CENTER + THUMB_RADIUS});
348+
expect(onChangeSpy).toHaveBeenCalledTimes(1);
349+
expect(document.activeElement).toBe(slider);
350+
expect(onChangeEndSpy).toHaveBeenCalledTimes(1);
351+
});
332352
});
333353
});

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

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
108108
valueRef.current = value;
109109

110110
let [isDragging, setDragging] = useState(false);
111+
let isDraggingRef = useRef(false).current;
111112

112113
let hue = value.getChannelValue('hue');
113114
function setHue(v: number) {
@@ -117,7 +118,9 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
117118
}
118119
v = roundToStep(mod(v, 360), step);
119120
if (hue !== v) {
120-
setValue(value.withChannelValue('hue', v));
121+
let color = value.withChannelValue('hue', v);
122+
valueRef.current = color;
123+
setValue(color);
121124
}
122125
}
123126

@@ -155,13 +158,14 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
155158
}
156159
},
157160
setDragging(isDragging) {
158-
setDragging(wasDragging => {
159-
if (onChangeEnd && !isDragging && wasDragging) {
160-
onChangeEnd(valueRef.current);
161-
}
161+
let wasDragging = isDraggingRef;
162+
isDraggingRef = isDragging;
163+
164+
if (onChangeEnd && !isDragging && wasDragging) {
165+
onChangeEnd(valueRef.current);
166+
}
162167

163-
return isDragging;
164-
});
168+
setDragging(isDragging);
165169
},
166170
isDragging,
167171
getDisplayColor() {

0 commit comments

Comments
 (0)