Skip to content

Commit fd415a9

Browse files
authored
fix: Make ColorPicker properly commit its new value upon clicking outside the dialog (adobe#9768)
* fix: remove old FireFox workaround so ComboBox closes overlay when clicking on backdrop * forgot that colorpicker needs the additional prevent defaults removed * fix 17/16
1 parent 7967385 commit fd415a9

File tree

8 files changed

+28
-68
lines changed

8 files changed

+28
-68
lines changed

packages/@react-aria/combobox/src/useComboBox.ts

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import {getChildNodes, getItemCount} from '@react-stately/collections';
2525
import intlMessages from '../intl/*.json';
2626
import {ListKeyboardDelegate, useSelectableCollection} from '@react-aria/selection';
2727
import {privateValidationStateProp} from '@react-stately/form';
28-
import {useInteractOutside} from '@react-aria/interactions';
2928
import {useLocalizedStringFormatter} from '@react-aria/i18n';
3029
import {useMenuTrigger} from '@react-aria/menu';
3130
import {useTextField} from '@react-aria/textfield';
@@ -366,20 +365,6 @@ export function useComboBox<T, M extends SelectionMode = 'single'>(props: AriaCo
366365
state.close();
367366
} : undefined);
368367

369-
// usePopover -> useOverlay calls useInteractOutside, but ComboBox is non-modal, so `isDismissable` is false
370-
// Because of this, onInteractOutside is not passed to useInteractOutside, so we need to call it here.
371-
useInteractOutside({
372-
ref: popoverRef,
373-
onInteractOutside: (e) => {
374-
let target = getEventTarget(e) as Element;
375-
if (nodeContains(buttonRef?.current, target) || nodeContains(inputRef.current, target)) {
376-
return;
377-
}
378-
state.close();
379-
},
380-
isDisabled: !state.isOpen
381-
});
382-
383368
return {
384369
labelProps,
385370
buttonProps: {

packages/@react-aria/dialog/docs/useDialog.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ import {useViewportSize} from '@react-aria/utils';
135135

136136
function Modal({state, children, ...props}) {
137137
let ref = React.useRef(null);
138-
let {modalProps, underlayProps} = useModalOverlay(props, state, ref);
138+
let {modalProps, underlayProps} = useModalOverlay({...props, isDismissable: true}, state, ref);
139139

140140
return (
141141
<Overlay>

packages/@react-aria/overlays/src/useOverlay.ts

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,6 @@ export function useOverlay(props: AriaOverlayProps, ref: RefObject<Element | nul
9999
if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(getEventTarget(e) as Element)) {
100100
if (topMostOverlay === ref) {
101101
e.stopPropagation();
102-
e.preventDefault();
103102
}
104103
}
105104
};
@@ -108,7 +107,6 @@ export function useOverlay(props: AriaOverlayProps, ref: RefObject<Element | nul
108107
if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(getEventTarget(e) as Element)) {
109108
if (visibleOverlays[visibleOverlays.length - 1] === ref) {
110109
e.stopPropagation();
111-
e.preventDefault();
112110
}
113111
if (lastVisibleOverlay.current === ref) {
114112
onHide();
@@ -151,20 +149,11 @@ export function useOverlay(props: AriaOverlayProps, ref: RefObject<Element | nul
151149
}
152150
});
153151

154-
let onPointerDownUnderlay = e => {
155-
// fixes a firefox issue that starts text selection https://bugzilla.mozilla.org/show_bug.cgi?id=1675846
156-
if (getEventTarget(e) === e.currentTarget) {
157-
e.preventDefault();
158-
}
159-
};
160-
161152
return {
162153
overlayProps: {
163154
onKeyDown,
164155
...focusWithinProps
165156
},
166-
underlayProps: {
167-
onPointerDown: onPointerDownUnderlay
168-
}
157+
underlayProps: {}
169158
};
170159
}

packages/@react-aria/overlays/test/useOverlay.test.js

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -128,15 +128,4 @@ describe('useOverlay', function () {
128128
fireEvent.keyDown(el, {key: 'Escape'});
129129
expect(onClose).toHaveBeenCalledTimes(1);
130130
});
131-
132-
describe('firefox bug', () => {
133-
installPointerEvent();
134-
it('should prevent default on pointer down on the underlay', function () {
135-
let underlayRef = React.createRef();
136-
render(<Example isOpen isDismissable underlayProps={{ref: underlayRef}} />);
137-
let isPrevented = fireEvent.pointerDown(underlayRef.current, {button: 0, pointerId: 1});
138-
fireEvent.pointerUp(document.body);
139-
expect(isPrevented).toBeFalsy(); // meaning the event had preventDefault called
140-
});
141-
});
142131
});

packages/@react-spectrum/combobox/test/ComboBox.test.js

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4362,7 +4362,8 @@ describe('ComboBox', function () {
43624362
let dismissButtons = within(tray).getAllByRole('button');
43634363
switch (Method) {
43644364
case 'clicking outside tray':
4365-
await user.click(document.body);
4365+
fireEvent.mouseDown(document.body, {button: 0});
4366+
fireEvent.mouseUp(document.body, {button: 0});
43664367
break;
43674368
case 'dismiss button':
43684369
// TODO: not entirely sure why using user.click here breaks the test... It seems to cause the selectedKey
@@ -5268,9 +5269,9 @@ describe('ComboBox', function () {
52685269

52695270
if (parseInt(React.version, 10) >= 19) {
52705271
it('resets to defaultSelectedKey when submitting form action', async () => {
5271-
function Test(props) {
5272+
function Test(props) {
52725273
const [value, formAction] = React.useActionState(() => '2', '1');
5273-
5274+
52745275
return (
52755276
<Provider theme={theme}>
52765277
<form action={formAction}>
@@ -5280,11 +5281,11 @@ describe('ComboBox', function () {
52805281
</Provider>
52815282
);
52825283
}
5283-
5284+
52845285
let {getByTestId, getByRole, rerender} = render(<Test />);
52855286
let input = getByRole('combobox');
52865287
expect(input).toHaveValue('One');
5287-
5288+
52885289
let button = getByTestId('submit');
52895290
// For some reason, user.click() causes act warnings related to suspense...
52905291
await act(() => button.click());
@@ -5604,7 +5605,7 @@ describe('ComboBox', function () {
56045605
it('resets to defaultSelectedKey when submitting form action', async () => {
56055606
function Test(props) {
56065607
const [value, formAction] = React.useActionState(() => '2', '1');
5607-
5608+
56085609
return (
56095610
<Provider theme={theme}>
56105611
<form action={formAction}>
@@ -5614,11 +5615,11 @@ describe('ComboBox', function () {
56145615
</Provider>
56155616
);
56165617
}
5617-
5618+
56185619
let {getByTestId, rerender} = render(<Test />);
56195620
let input = document.querySelector('input[name=combobox]');
56205621
expect(input).toHaveValue('One');
5621-
5622+
56225623
let button = getByTestId('submit');
56235624
await act(async () => await user.click(button));
56245625
expect(input).toHaveValue('Two');

packages/@react-spectrum/menu/test/MenuTrigger.test.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ describe('MenuTrigger', function () {
154154
it.each`
155155
Name | Component | props
156156
${'MenuTrigger'} | ${MenuTrigger} | ${{onOpenChange, isOpen: true}}
157-
`('$Name supports a controlled open state ', async function ({Component, props}) {
157+
`('$Name supports a controlled open state ', function ({Component, props}) {
158158
let tree = renderComponent(Component, props);
159159
act(() => {jest.runAllTimers();});
160160
expect(onOpenChange).toBeCalledTimes(0);
@@ -163,7 +163,8 @@ describe('MenuTrigger', function () {
163163
expect(menu).toBeInTheDocument();
164164

165165
let triggerButton = tree.getByText('Menu Button');
166-
await user.click(triggerButton);
166+
fireEvent.mouseDown(triggerButton, {button: 0});
167+
fireEvent.mouseUp(triggerButton, {button: 0});
167168
act(() => {jest.runAllTimers();});
168169

169170
expect(menu).toBeInTheDocument();

packages/@react-spectrum/s2/test/Combobox.test.tsx

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
*/
1212

1313
jest.mock('@react-aria/live-announcer');
14-
import {act, fireEvent, pointerMap, render, setupIntersectionObserverMock, within} from '@react-spectrum/test-utils-internal';
14+
import {act, pointerMap, render, setupIntersectionObserverMock, waitFor, within} from '@react-spectrum/test-utils-internal';
1515
import {announce} from '@react-aria/live-announcer';
1616
import {Button, ComboBox, ComboBoxItem, Content, ContextualHelp, Dialog, DialogTrigger, Heading, Text} from '../src';
1717
import React from 'react';
@@ -214,6 +214,7 @@ describe('Combobox', () => {
214214
});
215215

216216
it('should close the combobox when clicking outside the combobox on a dialog backdrop', async () => {
217+
let user = userEvent.setup({delay: null, pointerMap});
217218
let tree = render(
218219
<DialogTrigger>
219220
<Button>Open</Button>
@@ -247,20 +248,10 @@ describe('Combobox', () => {
247248
jest.runAllTimers();
248249
});
249250
let backdrop = document.querySelector('[style*="--visual-viewport-height"]');
250-
// can't use userEvent here for some reason
251-
fireEvent.mouseDown(backdrop!, {button: 0});
252-
fireEvent.mouseUp(backdrop!, {button: 0});
253-
act(() => {
254-
jest.runAllTimers();
255-
});
256-
expect(comboboxTester.listbox).toBeNull();
251+
await user.click(backdrop!);
257252

258-
259-
fireEvent.mouseDown(backdrop!, {button: 0});
260-
fireEvent.mouseUp(backdrop!, {button: 0});
261-
act(() => {
262-
jest.runAllTimers();
263-
});
253+
await waitFor(() => expect(comboboxTester.listbox).toBeNull());
254+
await user.click(backdrop!);
264255
expect(dialogTester.dialog).toBeNull();
265256
});
266257
});

packages/react-aria-components/test/Popover.test.js

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
* governing permissions and limitations under the License.
1111
*/
1212

13-
import {act, pointerMap, render} from '@react-spectrum/test-utils-internal';
13+
import {act, fireEvent, pointerMap, render} from '@react-spectrum/test-utils-internal';
1414
import {Button, Dialog, DialogTrigger, OverlayArrow, Popover, Pressable} from '../';
1515
import React, {useRef} from 'react';
1616
import {UNSAFE_PortalProvider} from '@react-aria/overlays';
@@ -102,7 +102,7 @@ describe('Popover', () => {
102102
expect(dialog).toHaveAttribute('data-custom', 'true');
103103
});
104104

105-
it('should support being used standalone', async () => {
105+
it('should support being used standalone', () => {
106106
let triggerRef = React.createRef();
107107
let onOpenChange = jest.fn();
108108
let {getByRole} = render(<>
@@ -115,12 +115,14 @@ describe('Popover', () => {
115115
let dialog = getByRole('dialog');
116116
expect(dialog).toHaveTextContent('A popover');
117117

118-
await user.click(document.body);
118+
// userEvent seems to trigger a double close event
119+
fireEvent.mouseDown(document.body, {button: 0});
120+
fireEvent.mouseUp(document.body, {button: 0});
119121
expect(onOpenChange).toHaveBeenCalledTimes(1);
120122
expect(onOpenChange).toHaveBeenCalledWith(false);
121123
});
122124

123-
it('isOpen and defaultOpen should override state from context', async () => {
125+
it('isOpen and defaultOpen should override state from context', () => {
124126
let onOpenChange = jest.fn();
125127
let {getByRole} = render(<>
126128
<DialogTrigger>
@@ -134,7 +136,9 @@ describe('Popover', () => {
134136
let dialog = getByRole('dialog');
135137
expect(dialog).toHaveTextContent('A popover');
136138

137-
await user.click(document.body);
139+
// userEvent seems to trigger a double close event
140+
fireEvent.mouseDown(document.body, {button: 0});
141+
fireEvent.mouseUp(document.body, {button: 0});
138142
expect(onOpenChange).toHaveBeenCalledTimes(1);
139143
expect(onOpenChange).toHaveBeenCalledWith(false);
140144
});

0 commit comments

Comments
 (0)