From fd415a9df9e1cb6579ad088f554fa5cf2b1cac55 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Tue, 10 Mar 2026 10:03:10 -0700 Subject: [PATCH] fix: Make ColorPicker properly commit its new value upon clicking outside the dialog (#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 --- .../@react-aria/combobox/src/useComboBox.ts | 15 --------------- .../@react-aria/dialog/docs/useDialog.mdx | 2 +- .../@react-aria/overlays/src/useOverlay.ts | 13 +------------ .../overlays/test/useOverlay.test.js | 11 ----------- .../combobox/test/ComboBox.test.js | 17 +++++++++-------- .../menu/test/MenuTrigger.test.js | 5 +++-- .../@react-spectrum/s2/test/Combobox.test.tsx | 19 +++++-------------- .../test/Popover.test.js | 14 +++++++++----- 8 files changed, 28 insertions(+), 68 deletions(-) diff --git a/packages/@react-aria/combobox/src/useComboBox.ts b/packages/@react-aria/combobox/src/useComboBox.ts index bb511e156d7..9c4548ae47c 100644 --- a/packages/@react-aria/combobox/src/useComboBox.ts +++ b/packages/@react-aria/combobox/src/useComboBox.ts @@ -25,7 +25,6 @@ import {getChildNodes, getItemCount} from '@react-stately/collections'; import intlMessages from '../intl/*.json'; import {ListKeyboardDelegate, useSelectableCollection} from '@react-aria/selection'; import {privateValidationStateProp} from '@react-stately/form'; -import {useInteractOutside} from '@react-aria/interactions'; import {useLocalizedStringFormatter} from '@react-aria/i18n'; import {useMenuTrigger} from '@react-aria/menu'; import {useTextField} from '@react-aria/textfield'; @@ -366,20 +365,6 @@ export function useComboBox(props: AriaCo state.close(); } : undefined); - // usePopover -> useOverlay calls useInteractOutside, but ComboBox is non-modal, so `isDismissable` is false - // Because of this, onInteractOutside is not passed to useInteractOutside, so we need to call it here. - useInteractOutside({ - ref: popoverRef, - onInteractOutside: (e) => { - let target = getEventTarget(e) as Element; - if (nodeContains(buttonRef?.current, target) || nodeContains(inputRef.current, target)) { - return; - } - state.close(); - }, - isDisabled: !state.isOpen - }); - return { labelProps, buttonProps: { diff --git a/packages/@react-aria/dialog/docs/useDialog.mdx b/packages/@react-aria/dialog/docs/useDialog.mdx index 3da064b7955..e5294ea9339 100644 --- a/packages/@react-aria/dialog/docs/useDialog.mdx +++ b/packages/@react-aria/dialog/docs/useDialog.mdx @@ -135,7 +135,7 @@ import {useViewportSize} from '@react-aria/utils'; function Modal({state, children, ...props}) { let ref = React.useRef(null); - let {modalProps, underlayProps} = useModalOverlay(props, state, ref); + let {modalProps, underlayProps} = useModalOverlay({...props, isDismissable: true}, state, ref); return ( diff --git a/packages/@react-aria/overlays/src/useOverlay.ts b/packages/@react-aria/overlays/src/useOverlay.ts index 46497cf2729..08a129880bc 100644 --- a/packages/@react-aria/overlays/src/useOverlay.ts +++ b/packages/@react-aria/overlays/src/useOverlay.ts @@ -99,7 +99,6 @@ export function useOverlay(props: AriaOverlayProps, ref: RefObject { - // fixes a firefox issue that starts text selection https://bugzilla.mozilla.org/show_bug.cgi?id=1675846 - if (getEventTarget(e) === e.currentTarget) { - e.preventDefault(); - } - }; - return { overlayProps: { onKeyDown, ...focusWithinProps }, - underlayProps: { - onPointerDown: onPointerDownUnderlay - } + underlayProps: {} }; } diff --git a/packages/@react-aria/overlays/test/useOverlay.test.js b/packages/@react-aria/overlays/test/useOverlay.test.js index 2f686f0f287..7535d0554a5 100644 --- a/packages/@react-aria/overlays/test/useOverlay.test.js +++ b/packages/@react-aria/overlays/test/useOverlay.test.js @@ -128,15 +128,4 @@ describe('useOverlay', function () { fireEvent.keyDown(el, {key: 'Escape'}); expect(onClose).toHaveBeenCalledTimes(1); }); - - describe('firefox bug', () => { - installPointerEvent(); - it('should prevent default on pointer down on the underlay', function () { - let underlayRef = React.createRef(); - render(); - let isPrevented = fireEvent.pointerDown(underlayRef.current, {button: 0, pointerId: 1}); - fireEvent.pointerUp(document.body); - expect(isPrevented).toBeFalsy(); // meaning the event had preventDefault called - }); - }); }); diff --git a/packages/@react-spectrum/combobox/test/ComboBox.test.js b/packages/@react-spectrum/combobox/test/ComboBox.test.js index 245127dd922..3d7ee5ac14c 100644 --- a/packages/@react-spectrum/combobox/test/ComboBox.test.js +++ b/packages/@react-spectrum/combobox/test/ComboBox.test.js @@ -4362,7 +4362,8 @@ describe('ComboBox', function () { let dismissButtons = within(tray).getAllByRole('button'); switch (Method) { case 'clicking outside tray': - await user.click(document.body); + fireEvent.mouseDown(document.body, {button: 0}); + fireEvent.mouseUp(document.body, {button: 0}); break; case 'dismiss button': // 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 () { if (parseInt(React.version, 10) >= 19) { it('resets to defaultSelectedKey when submitting form action', async () => { - function Test(props) { + function Test(props) { const [value, formAction] = React.useActionState(() => '2', '1'); - + return (
@@ -5280,11 +5281,11 @@ describe('ComboBox', function () { ); } - + let {getByTestId, getByRole, rerender} = render(); let input = getByRole('combobox'); expect(input).toHaveValue('One'); - + let button = getByTestId('submit'); // For some reason, user.click() causes act warnings related to suspense... await act(() => button.click()); @@ -5604,7 +5605,7 @@ describe('ComboBox', function () { it('resets to defaultSelectedKey when submitting form action', async () => { function Test(props) { const [value, formAction] = React.useActionState(() => '2', '1'); - + return ( @@ -5614,11 +5615,11 @@ describe('ComboBox', function () { ); } - + let {getByTestId, rerender} = render(); let input = document.querySelector('input[name=combobox]'); expect(input).toHaveValue('One'); - + let button = getByTestId('submit'); await act(async () => await user.click(button)); expect(input).toHaveValue('Two'); diff --git a/packages/@react-spectrum/menu/test/MenuTrigger.test.js b/packages/@react-spectrum/menu/test/MenuTrigger.test.js index 2ce5eab7717..80f6d9c0de7 100644 --- a/packages/@react-spectrum/menu/test/MenuTrigger.test.js +++ b/packages/@react-spectrum/menu/test/MenuTrigger.test.js @@ -154,7 +154,7 @@ describe('MenuTrigger', function () { it.each` Name | Component | props ${'MenuTrigger'} | ${MenuTrigger} | ${{onOpenChange, isOpen: true}} - `('$Name supports a controlled open state ', async function ({Component, props}) { + `('$Name supports a controlled open state ', function ({Component, props}) { let tree = renderComponent(Component, props); act(() => {jest.runAllTimers();}); expect(onOpenChange).toBeCalledTimes(0); @@ -163,7 +163,8 @@ describe('MenuTrigger', function () { expect(menu).toBeInTheDocument(); let triggerButton = tree.getByText('Menu Button'); - await user.click(triggerButton); + fireEvent.mouseDown(triggerButton, {button: 0}); + fireEvent.mouseUp(triggerButton, {button: 0}); act(() => {jest.runAllTimers();}); expect(menu).toBeInTheDocument(); diff --git a/packages/@react-spectrum/s2/test/Combobox.test.tsx b/packages/@react-spectrum/s2/test/Combobox.test.tsx index fe3beaac752..c6d188c6416 100644 --- a/packages/@react-spectrum/s2/test/Combobox.test.tsx +++ b/packages/@react-spectrum/s2/test/Combobox.test.tsx @@ -11,7 +11,7 @@ */ jest.mock('@react-aria/live-announcer'); -import {act, fireEvent, pointerMap, render, setupIntersectionObserverMock, within} from '@react-spectrum/test-utils-internal'; +import {act, pointerMap, render, setupIntersectionObserverMock, waitFor, within} from '@react-spectrum/test-utils-internal'; import {announce} from '@react-aria/live-announcer'; import {Button, ComboBox, ComboBoxItem, Content, ContextualHelp, Dialog, DialogTrigger, Heading, Text} from '../src'; import React from 'react'; @@ -214,6 +214,7 @@ describe('Combobox', () => { }); it('should close the combobox when clicking outside the combobox on a dialog backdrop', async () => { + let user = userEvent.setup({delay: null, pointerMap}); let tree = render( @@ -247,20 +248,10 @@ describe('Combobox', () => { jest.runAllTimers(); }); let backdrop = document.querySelector('[style*="--visual-viewport-height"]'); - // can't use userEvent here for some reason - fireEvent.mouseDown(backdrop!, {button: 0}); - fireEvent.mouseUp(backdrop!, {button: 0}); - act(() => { - jest.runAllTimers(); - }); - expect(comboboxTester.listbox).toBeNull(); + await user.click(backdrop!); - - fireEvent.mouseDown(backdrop!, {button: 0}); - fireEvent.mouseUp(backdrop!, {button: 0}); - act(() => { - jest.runAllTimers(); - }); + await waitFor(() => expect(comboboxTester.listbox).toBeNull()); + await user.click(backdrop!); expect(dialogTester.dialog).toBeNull(); }); }); diff --git a/packages/react-aria-components/test/Popover.test.js b/packages/react-aria-components/test/Popover.test.js index 957468b0f24..2d8e288a65c 100644 --- a/packages/react-aria-components/test/Popover.test.js +++ b/packages/react-aria-components/test/Popover.test.js @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -import {act, pointerMap, render} from '@react-spectrum/test-utils-internal'; +import {act, fireEvent, pointerMap, render} from '@react-spectrum/test-utils-internal'; import {Button, Dialog, DialogTrigger, OverlayArrow, Popover, Pressable} from '../'; import React, {useRef} from 'react'; import {UNSAFE_PortalProvider} from '@react-aria/overlays'; @@ -102,7 +102,7 @@ describe('Popover', () => { expect(dialog).toHaveAttribute('data-custom', 'true'); }); - it('should support being used standalone', async () => { + it('should support being used standalone', () => { let triggerRef = React.createRef(); let onOpenChange = jest.fn(); let {getByRole} = render(<> @@ -115,12 +115,14 @@ describe('Popover', () => { let dialog = getByRole('dialog'); expect(dialog).toHaveTextContent('A popover'); - await user.click(document.body); + // userEvent seems to trigger a double close event + fireEvent.mouseDown(document.body, {button: 0}); + fireEvent.mouseUp(document.body, {button: 0}); expect(onOpenChange).toHaveBeenCalledTimes(1); expect(onOpenChange).toHaveBeenCalledWith(false); }); - it('isOpen and defaultOpen should override state from context', async () => { + it('isOpen and defaultOpen should override state from context', () => { let onOpenChange = jest.fn(); let {getByRole} = render(<> @@ -134,7 +136,9 @@ describe('Popover', () => { let dialog = getByRole('dialog'); expect(dialog).toHaveTextContent('A popover'); - await user.click(document.body); + // userEvent seems to trigger a double close event + fireEvent.mouseDown(document.body, {button: 0}); + fireEvent.mouseUp(document.body, {button: 0}); expect(onOpenChange).toHaveBeenCalledTimes(1); expect(onOpenChange).toHaveBeenCalledWith(false); });