diff --git a/packages/@react-aria/interactions/src/useInteractOutside.ts b/packages/@react-aria/interactions/src/useInteractOutside.ts index 3a63ada5297..5bc6cd69f7d 100644 --- a/packages/@react-aria/interactions/src/useInteractOutside.ts +++ b/packages/@react-aria/interactions/src/useInteractOutside.ts @@ -20,6 +20,7 @@ import {RefObject, SyntheticEvent, useEffect, useRef} from 'react'; interface InteractOutsideProps { ref: RefObject, onInteractOutside?: (e: SyntheticEvent) => void, + onInteractOutsideStart?: (e: SyntheticEvent) => void, /** Whether the interact outside events should be disabled. */ isDisabled?: boolean } @@ -29,7 +30,7 @@ interface InteractOutsideProps { * when a user clicks outside them. */ export function useInteractOutside(props: InteractOutsideProps) { - let {ref, onInteractOutside, isDisabled} = props; + let {ref, onInteractOutside, isDisabled, onInteractOutsideStart} = props; let stateRef = useRef({ isPointerDown: false, ignoreEmulatedMouseEvents: false @@ -41,7 +42,10 @@ export function useInteractOutside(props: InteractOutsideProps) { if (isDisabled) { return; } - if (isValidEvent(e, ref)) { + if (isValidEvent(e, ref) && onInteractOutside) { + if (onInteractOutsideStart) { + onInteractOutsideStart(e); + } state.isPointerDown = true; } }; diff --git a/packages/@react-aria/menu/stories/useMenu.stories.tsx b/packages/@react-aria/menu/stories/useMenu.stories.tsx index b35e8d58055..29f3505ec70 100644 --- a/packages/@react-aria/menu/stories/useMenu.stories.tsx +++ b/packages/@react-aria/menu/stories/useMenu.stories.tsx @@ -28,7 +28,7 @@ storiesOf('useMenu', module) .add('double menu fires onInteractOutside', () => (
-
This should just be there to show that onInteractOutside fires when clicking on another trigger, don't worry about two open menus at once.
+
This should just be there to show that onInteractOutside fires when clicking on another trigger.
Copy Cut @@ -39,6 +39,7 @@ storiesOf('useMenu', module) Cut Paste +
) ); diff --git a/packages/@react-aria/overlays/src/useOverlay.ts b/packages/@react-aria/overlays/src/useOverlay.ts index 991af37381d..b987345193e 100644 --- a/packages/@react-aria/overlays/src/useOverlay.ts +++ b/packages/@react-aria/overlays/src/useOverlay.ts @@ -89,8 +89,21 @@ export function useOverlay(props: OverlayProps, ref: RefObject): Ov } }; + let onInteractOutsideStart = (e: SyntheticEvent) => { + if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(e.target as HTMLElement)) { + if (visibleOverlays[visibleOverlays.length - 1] === ref) { + e.stopPropagation(); + e.preventDefault(); + } + } + }; + let onInteractOutside = (e: SyntheticEvent) => { if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(e.target as HTMLElement)) { + if (visibleOverlays[visibleOverlays.length - 1] === ref) { + e.stopPropagation(); + e.preventDefault(); + } onHide(); } }; @@ -104,7 +117,7 @@ export function useOverlay(props: OverlayProps, ref: RefObject): Ov }; // Handle clicking outside the overlay to close it - useInteractOutside({ref, onInteractOutside: isDismissable ? onInteractOutside : null}); + useInteractOutside({ref, onInteractOutside: isDismissable ? onInteractOutside : null, onInteractOutsideStart}); let {focusWithinProps} = useFocusWithin({ isDisabled: !shouldCloseOnBlur, diff --git a/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js b/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js index 1742ddafe07..2776b91e440 100644 --- a/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js +++ b/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js @@ -10,15 +10,18 @@ * governing permissions and limitations under the License. */ +import {act, render, within} from '@testing-library/react'; import {Breadcrumbs} from '../'; import {Item} from '@react-stately/collections'; import {Provider} from '@react-spectrum/provider'; import React, {useRef} from 'react'; -import {render, within} from '@testing-library/react'; import {theme} from '@react-spectrum/theme-default'; import {triggerPress} from '@react-spectrum/test-utils'; describe('Breadcrumbs', function () { + beforeAll(() => { + jest.useFakeTimers(); + }); beforeEach(() => { jest.spyOn(HTMLElement.prototype, 'offsetWidth', 'get').mockImplementation(function () { if (this instanceof HTMLUListElement) { @@ -299,6 +302,7 @@ describe('Breadcrumbs', function () { let menuButton = getByRole('button'); triggerPress(menuButton); + act(() => {jest.runAllTimers();}); let menu = getByRole('menu'); expect(menu).toBeTruthy(); @@ -311,6 +315,9 @@ describe('Breadcrumbs', function () { // breadcrumb root item expect(item1[0]).toHaveAttribute('role', 'link'); triggerPress(item1[0]); + // first press closes the menu, second press + act(() => {jest.runAllTimers();}); + triggerPress(item1[0]); expect(onAction).toHaveBeenCalledWith('Folder 1'); // menu item diff --git a/packages/@react-spectrum/combobox/src/ComboBox.tsx b/packages/@react-spectrum/combobox/src/ComboBox.tsx index 8ba2eeef19f..9f2088a1181 100644 --- a/packages/@react-spectrum/combobox/src/ComboBox.tsx +++ b/packages/@react-spectrum/combobox/src/ComboBox.tsx @@ -162,7 +162,8 @@ const ComboBoxBase = React.forwardRef(function ComboBoxBase(pr ref={popoverRef} placement={placement} hideArrow - isNonModal> + isNonModal + isDismissable={false}> ( ) + ) + .add( + '2 comboboxes', + () => ( + + + {(item) => {item.name}} + + + {(item) => {item.name}} + + + ) ); diff --git a/packages/@react-spectrum/combobox/test/ComboBox.test.js b/packages/@react-spectrum/combobox/test/ComboBox.test.js index 60a49abc6c0..3a8fad61293 100644 --- a/packages/@react-spectrum/combobox/test/ComboBox.test.js +++ b/packages/@react-spectrum/combobox/test/ComboBox.test.js @@ -424,6 +424,22 @@ describe('ComboBox', function () { expect(onOpenChange).toHaveBeenCalledWith(true, 'focus'); testComboBoxOpen(combobox, button, listbox); }); + + it('opens menu when combobox is focused by clicking button', function () { + let {getByRole} = renderComboBox({menuTrigger: 'focus'}); + + let button = getByRole('button'); + let combobox = getByRole('combobox'); + act(() => { + triggerPress(button); + jest.runAllTimers(); + }); + + let listbox = getByRole('listbox'); + expect(onOpenChange).toBeCalledTimes(1); + expect(onOpenChange).toHaveBeenCalledWith(true, 'focus'); + testComboBoxOpen(combobox, button, listbox); + }); }); describe('button click', function () { diff --git a/packages/@react-spectrum/dialog/stories/DialogTrigger.stories.tsx b/packages/@react-spectrum/dialog/stories/DialogTrigger.stories.tsx index 3f0f94c121c..88be1688a33 100644 --- a/packages/@react-spectrum/dialog/stories/DialogTrigger.stories.tsx +++ b/packages/@react-spectrum/dialog/stories/DialogTrigger.stories.tsx @@ -335,6 +335,26 @@ storiesOf('DialogTrigger', module) .add( 'trigger visible through underlay', () => renderTriggerNotCentered({}) + ) + .add( + '2 popovers', + () => ( + + + Trigger + + + + + + + + + Trigger + Hi! + + + ) ); function render({width = 'auto', ...props}) { diff --git a/packages/@react-spectrum/dialog/test/DialogTrigger.test.js b/packages/@react-spectrum/dialog/test/DialogTrigger.test.js index 5265e550047..312254ef3ef 100644 --- a/packages/@react-spectrum/dialog/test/DialogTrigger.test.js +++ b/packages/@react-spectrum/dialog/test/DialogTrigger.test.js @@ -56,7 +56,7 @@ describe('DialogTrigger', function () { }); it('should trigger a modal by default', function () { - let {getByRole, getByTestId} = render( + let {queryByRole, getByRole, getByTestId} = render( Trigger @@ -65,9 +65,7 @@ describe('DialogTrigger', function () { ); - expect(() => { - getByRole('dialog'); - }).toThrow(); + expect(queryByRole('dialog')).toBeNull(); let button = getByRole('button'); triggerPress(button); @@ -323,6 +321,47 @@ describe('DialogTrigger', function () { expect(document.activeElement).toBe(button); }); + it('popovers should be closeable by clicking their trigger while they are open', async function () { + let onOpenChange = jest.fn(); + let {getByRole} = render( + + + Trigger + contents + + + ); + + let button = getByRole('button'); + triggerPress(button); + + act(() => { + jest.runAllTimers(); + }); + + let dialog = getByRole('dialog'); + + await waitFor(() => { + expect(dialog).toBeVisible(); + }); // wait for animation + + expect(document.activeElement).toBe(dialog); + expect(onOpenChange).toHaveBeenCalledTimes(1); + + triggerPress(button); + + act(() => { + jest.runAllTimers(); + }); + + await waitFor(() => { + expect(dialog).not.toBeInTheDocument(); + }); // wait for animation + + expect(document.activeElement).toBe(button); + expect(onOpenChange).toHaveBeenCalledTimes(2); + }); + it('should set aria-hidden on parent providers on mount and remove on unmount', async function () { let rootProviderRef = React.createRef(); let {getByRole} = render( @@ -844,4 +883,60 @@ describe('DialogTrigger', function () { }); expect(document.activeElement).toBe(innerInput); }); + + it('input in nested popover should be interactive with a click', async () => { + let {getByRole, getByText, getByLabelText} = render( + + + + Trigger1 + + + + + Trigger2 + + + + + + + + + + + ); + let button = getByRole('button'); + triggerPress(button); + + act(() => { + jest.runAllTimers(); + }); + + let outerDialog = getByRole('dialog'); + + await waitFor(() => { + expect(outerDialog).toBeVisible(); + }); // wait for animation + let outerButton = getByText('Trigger2'); + + + triggerPress(outerButton); + + act(() => { + jest.runAllTimers(); + }); + + let innerDialog = getByRole('dialog'); + + await waitFor(() => { + expect(innerDialog).toBeVisible(); + }); // wait for animation + + let innerInput = getByLabelText('inner input'); + expect(getByLabelText('inner input')).toBeVisible(); + userEvent.click(innerInput); + + expect(document.activeElement).toBe(innerInput); + }); }); diff --git a/packages/@react-spectrum/menu/test/MenuTrigger.test.js b/packages/@react-spectrum/menu/test/MenuTrigger.test.js index 76b4945aeae..4aff365a343 100644 --- a/packages/@react-spectrum/menu/test/MenuTrigger.test.js +++ b/packages/@react-spectrum/menu/test/MenuTrigger.test.js @@ -211,6 +211,7 @@ describe('MenuTrigger', function () { ${'MenuTrigger'} | ${MenuTrigger} | ${{onOpenChange, isOpen: true}} `('$Name supports a controlled open state ', function ({Component, props}) { let tree = renderComponent(Component, props); + act(() => {jest.runAllTimers();}); expect(onOpenChange).toBeCalledTimes(0); let menu = tree.getByRole('menu'); @@ -222,7 +223,7 @@ describe('MenuTrigger', function () { menu = tree.getByRole('menu'); expect(menu).toBeTruthy(); - expect(onOpenChange).toBeCalledTimes(2); // once for press, once for blur :/ + expect(onOpenChange).toBeCalledTimes(1); }); // New functionality in v3 @@ -231,6 +232,7 @@ describe('MenuTrigger', function () { ${'MenuTrigger'} | ${MenuTrigger} | ${{onOpenChange, defaultOpen: true}} `('$Name supports a uncontrolled default open state ', function ({Component, props}) { let tree = renderComponent(Component, props); + act(() => {jest.runAllTimers();}); expect(onOpenChange).toBeCalledTimes(0); let menu = tree.getByRole('menu'); @@ -263,6 +265,7 @@ describe('MenuTrigger', function () { ${'MenuTrigger'} | ${MenuTrigger} | ${{}} `('$Name autofocuses the selected item on menu open', function ({Component, props}) { let tree = renderComponent(Component, props, {selectedKeys: ['Bar']}); + act(() => {jest.runAllTimers();}); let button = tree.getByRole('button'); triggerPress(button); act(() => {jest.runAllTimers();}); @@ -278,6 +281,8 @@ describe('MenuTrigger', function () { // Opening menu via down arrow still autofocuses the selected item fireEvent.keyDown(button, {key: 'ArrowDown', code: 40, charCode: 40}); + fireEvent.keyUp(button, {key: 'ArrowDown', code: 40, charCode: 40}); + act(() => {jest.runAllTimers();}); menu = tree.getByRole('menu'); menuItems = within(menu).getAllByRole('menuitem'); selectedItem = menuItems[1]; @@ -770,4 +775,48 @@ describe('MenuTrigger', function () { let checkmark = queryByRole('img', {hidden: true}); expect(checkmark).toBeNull(); }); + + it('two menus can not be open at the same time', function () { + let {getAllByRole, getByRole, queryByRole} = render( + + + + + Alpha + Bravo + + + + + + Whiskey + Tango + Foxtrot + + + + ); + let [button1, button2] = getAllByRole('button'); + triggerPress(button1); + act(() => jest.runAllTimers()); + let menu = getByRole('menu'); + let menuItem1 = within(menu).getByText('Alpha'); + expect(menuItem1).toBeInTheDocument(); + + // pressing once on button 2 should close menu1, but not open menu2 yet + triggerPress(button2); + act(() => jest.runAllTimers()); + expect(queryByRole('menu')).toBeNull(); + + // second press of button2 should open menu2 + triggerPress(button2); + act(() => jest.runAllTimers()); + let menu2 = getByRole('menu'); + let menu2Item1 = within(menu2).getByText('Whiskey'); + expect(menu2Item1).toBeInTheDocument(); + }); }); diff --git a/packages/@react-spectrum/overlays/src/Popover.tsx b/packages/@react-spectrum/overlays/src/Popover.tsx index ac6a0e8678c..6f6c81c0a5a 100644 --- a/packages/@react-spectrum/overlays/src/Popover.tsx +++ b/packages/@react-spectrum/overlays/src/Popover.tsx @@ -29,7 +29,8 @@ interface PopoverWrapperProps extends HTMLAttributes { onClose?: () => void, shouldCloseOnBlur?: boolean, isKeyboardDismissDisabled?: boolean, - isNonModal?: boolean + isNonModal?: boolean, + isDismissable?: boolean } /** @@ -47,7 +48,18 @@ let arrowPlacement = { }; function Popover(props: PopoverProps, ref: DOMRef) { - let {children, placement, arrowProps, onClose, shouldCloseOnBlur, hideArrow, isKeyboardDismissDisabled, isNonModal, ...otherProps} = props; + let { + children, + placement, + arrowProps, + onClose, + shouldCloseOnBlur, + hideArrow, + isKeyboardDismissDisabled, + isNonModal, + isDismissable = true, + ...otherProps + } = props; let domRef = useDOMRef(ref); let {styleProps} = useStyleProps(props); @@ -62,7 +74,8 @@ function Popover(props: PopoverProps, ref: DOMRef) { shouldCloseOnBlur={shouldCloseOnBlur} isKeyboardDismissDisabled={isKeyboardDismissDisabled} hideArrow={hideArrow} - isNonModal={isNonModal}> + isNonModal={isNonModal} + isDismissable={isDismissable}> {children} @@ -70,9 +83,22 @@ function Popover(props: PopoverProps, ref: DOMRef) { } const PopoverWrapper = forwardRef((props: PopoverWrapperProps, ref: RefObject) => { - // eslint-disable-next-line @typescript-eslint/no-unused-vars - let {children, placement = 'bottom', arrowProps, isOpen, hideArrow, shouldCloseOnBlur, isKeyboardDismissDisabled, isNonModal, ...otherProps} = props; - let {overlayProps} = useOverlay({...props, isDismissable: true}, ref); + let { + children, + placement = 'bottom', + arrowProps, + isOpen, + hideArrow, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + shouldCloseOnBlur, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + isKeyboardDismissDisabled, + isNonModal, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + isDismissable, + ...otherProps + } = props; + let {overlayProps} = useOverlay({...props, isDismissable: isDismissable && isOpen}, ref); let {modalProps} = useModal({ isDisabled: isNonModal }); diff --git a/packages/@react-types/overlays/src/index.d.ts b/packages/@react-types/overlays/src/index.d.ts index e149bf73561..abdb5a79467 100644 --- a/packages/@react-types/overlays/src/index.d.ts +++ b/packages/@react-types/overlays/src/index.d.ts @@ -90,7 +90,8 @@ export interface PopoverProps extends StyleProps, OverlayProps { isOpen?: boolean, onClose?: () => void, shouldCloseOnBlur?: boolean, - isNonModal?: boolean + isNonModal?: boolean, + isDismissable?: boolean } export interface TrayProps extends StyleProps, OverlayProps {