From 684982a6e8425c4e13abc965a5fa8e079b62911a Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 28 May 2021 15:09:42 -0500 Subject: [PATCH 01/16] Fix Popover not closing from pressing trigger --- packages/@react-aria/combobox/src/useComboBox.ts | 1 + .../interactions/src/useInteractOutside.ts | 6 ++++++ .../combobox/stories/ComboBox.stories.tsx | 13 +++++++++++++ 3 files changed, 20 insertions(+) diff --git a/packages/@react-aria/combobox/src/useComboBox.ts b/packages/@react-aria/combobox/src/useComboBox.ts index 1785be53af0..5faa6d41e9e 100644 --- a/packages/@react-aria/combobox/src/useComboBox.ts +++ b/packages/@react-aria/combobox/src/useComboBox.ts @@ -170,6 +170,7 @@ export function useComboBox(props: AriaComboBoxProps, state: ComboBoxState }; let onPressStart = (e: PressEvent) => { + console.log('on press start useCombobox') if (e.pointerType !== 'touch') { inputRef.current.focus(); state.toggle((e.pointerType === 'keyboard' || e.pointerType === 'virtual') ? 'first' : null, 'manual'); diff --git a/packages/@react-aria/interactions/src/useInteractOutside.ts b/packages/@react-aria/interactions/src/useInteractOutside.ts index 3a63ada5297..e397c8beb97 100644 --- a/packages/@react-aria/interactions/src/useInteractOutside.ts +++ b/packages/@react-aria/interactions/src/useInteractOutside.ts @@ -41,6 +41,7 @@ export function useInteractOutside(props: InteractOutsideProps) { if (isDisabled) { return; } + e.stopPropagation(); if (isValidEvent(e, ref)) { state.isPointerDown = true; } @@ -52,7 +53,9 @@ export function useInteractOutside(props: InteractOutsideProps) { if (isDisabled) { return; } + console.log('on pointer up outside'); if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) { + e.stopPropagation(); state.isPointerDown = false; onInteractOutside(e); } @@ -71,9 +74,11 @@ export function useInteractOutside(props: InteractOutsideProps) { if (isDisabled) { return; } + e.stopPropagation(); if (state.ignoreEmulatedMouseEvents) { state.ignoreEmulatedMouseEvents = false; } else if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) { + e.stopPropagation(); state.isPointerDown = false; onInteractOutside(e); } @@ -85,6 +90,7 @@ export function useInteractOutside(props: InteractOutsideProps) { } state.ignoreEmulatedMouseEvents = true; if (onInteractOutside && state.isPointerDown && isValidEvent(e, ref)) { + e.stopPropagation(); state.isPointerDown = false; onInteractOutside(e); } diff --git a/packages/@react-spectrum/combobox/stories/ComboBox.stories.tsx b/packages/@react-spectrum/combobox/stories/ComboBox.stories.tsx index c13efdfaf8c..a5e52dc7308 100644 --- a/packages/@react-spectrum/combobox/stories/ComboBox.stories.tsx +++ b/packages/@react-spectrum/combobox/stories/ComboBox.stories.tsx @@ -454,6 +454,19 @@ storiesOf('ComboBox', module) () => ( ) + ) + .add( + '2 comboboxes', + () => ( + + + {(item) => {item.name}} + + + {(item) => {item.name}} + + + ) ); From 1fe1c078caa58364508a359aa6da9e23bd299b27 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 28 May 2021 16:35:48 -0500 Subject: [PATCH 02/16] Pushing some more changes --- .../interactions/src/useInteractOutside.ts | 5 +-- .../@react-spectrum/combobox/src/ComboBox.tsx | 3 +- .../combobox/stories/ComboBox.stories.tsx | 4 +- .../dialog/stories/DialogTrigger.stories.tsx | 20 ++++++++++ .../dialog/test/DialogTrigger.test.js | 8 ++-- .../@react-spectrum/overlays/src/Popover.tsx | 38 ++++++++++++++++--- packages/@react-types/overlays/src/index.d.ts | 3 +- 7 files changed, 64 insertions(+), 17 deletions(-) diff --git a/packages/@react-aria/interactions/src/useInteractOutside.ts b/packages/@react-aria/interactions/src/useInteractOutside.ts index e397c8beb97..aa8b193dc7f 100644 --- a/packages/@react-aria/interactions/src/useInteractOutside.ts +++ b/packages/@react-aria/interactions/src/useInteractOutside.ts @@ -41,8 +41,8 @@ export function useInteractOutside(props: InteractOutsideProps) { if (isDisabled) { return; } - e.stopPropagation(); - if (isValidEvent(e, ref)) { + if (isValidEvent(e, ref) && onInteractOutside) { + e.stopPropagation(); state.isPointerDown = true; } }; @@ -53,7 +53,6 @@ export function useInteractOutside(props: InteractOutsideProps) { if (isDisabled) { return; } - console.log('on pointer up outside'); if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) { e.stopPropagation(); state.isPointerDown = false; 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}> ( + '2 comboboxes', + () => ( {(item) => {item.name}} 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..14c4fed2370 100644 --- a/packages/@react-spectrum/dialog/test/DialogTrigger.test.js +++ b/packages/@react-spectrum/dialog/test/DialogTrigger.test.js @@ -55,8 +55,8 @@ describe('DialogTrigger', function () { window.requestAnimationFrame.mockRestore(); }); - it('should trigger a modal by default', function () { - let {getByRole, getByTestId} = render( + it.only('should trigger a modal by default', function () { + 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); diff --git a/packages/@react-spectrum/overlays/src/Popover.tsx b/packages/@react-spectrum/overlays/src/Popover.tsx index ac6a0e8678c..81c94824845 100644 --- a/packages/@react-spectrum/overlays/src/Popover.tsx +++ b/packages/@react-spectrum/overlays/src/Popover.tsx @@ -29,7 +29,9 @@ interface PopoverWrapperProps extends HTMLAttributes { onClose?: () => void, shouldCloseOnBlur?: boolean, isKeyboardDismissDisabled?: boolean, - isNonModal?: boolean + isNonModal?: boolean, + shouldCloseOnInteractOutside?: (element: HTMLElement) => boolean, + isDismissable?: boolean } /** @@ -47,7 +49,19 @@ 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, + shouldCloseOnInteractOutside, + isDismissable = true, + ...otherProps + } = props; let domRef = useDOMRef(ref); let {styleProps} = useStyleProps(props); @@ -60,9 +74,11 @@ function Popover(props: PopoverProps, ref: DOMRef) { arrowProps={arrowProps} onClose={onClose} shouldCloseOnBlur={shouldCloseOnBlur} + shouldCloseOnInteractOutside={shouldCloseOnInteractOutside} isKeyboardDismissDisabled={isKeyboardDismissDisabled} hideArrow={hideArrow} - isNonModal={isNonModal}> + isNonModal={isNonModal} + isDismissable={isDismissable}> {children} @@ -71,8 +87,20 @@ 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, + shouldCloseOnBlur, + isKeyboardDismissDisabled, + isNonModal, + shouldCloseOnInteractOutside, + isDismissable, + ...otherProps + } = props; + let {overlayProps} = useOverlay({...props, isDismissable}, 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..db92951d83a 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, + shouldCloseOnInteractOutside?: (element: HTMLElement) => boolean } export interface TrayProps extends StyleProps, OverlayProps { From d2a3eeed9c3d27d56c532baf71186a88b07661e0 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 28 May 2021 16:36:01 -0500 Subject: [PATCH 03/16] remove console log --- packages/@react-aria/combobox/src/useComboBox.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@react-aria/combobox/src/useComboBox.ts b/packages/@react-aria/combobox/src/useComboBox.ts index 5faa6d41e9e..1785be53af0 100644 --- a/packages/@react-aria/combobox/src/useComboBox.ts +++ b/packages/@react-aria/combobox/src/useComboBox.ts @@ -170,7 +170,6 @@ export function useComboBox(props: AriaComboBoxProps, state: ComboBoxState }; let onPressStart = (e: PressEvent) => { - console.log('on press start useCombobox') if (e.pointerType !== 'touch') { inputRef.current.focus(); state.toggle((e.pointerType === 'keyboard' || e.pointerType === 'virtual') ? 'first' : null, 'manual'); From c6cda12d175bb4bc00f632cbdb40eb3f28e50e77 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 28 May 2021 16:37:16 -0500 Subject: [PATCH 04/16] pushing a fix --- packages/@react-aria/interactions/src/useInteractOutside.ts | 1 - packages/@react-spectrum/dialog/test/DialogTrigger.test.js | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/@react-aria/interactions/src/useInteractOutside.ts b/packages/@react-aria/interactions/src/useInteractOutside.ts index aa8b193dc7f..2db0b20f062 100644 --- a/packages/@react-aria/interactions/src/useInteractOutside.ts +++ b/packages/@react-aria/interactions/src/useInteractOutside.ts @@ -73,7 +73,6 @@ export function useInteractOutside(props: InteractOutsideProps) { if (isDisabled) { return; } - e.stopPropagation(); if (state.ignoreEmulatedMouseEvents) { state.ignoreEmulatedMouseEvents = false; } else if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) { diff --git a/packages/@react-spectrum/dialog/test/DialogTrigger.test.js b/packages/@react-spectrum/dialog/test/DialogTrigger.test.js index 14c4fed2370..f75ee79458e 100644 --- a/packages/@react-spectrum/dialog/test/DialogTrigger.test.js +++ b/packages/@react-spectrum/dialog/test/DialogTrigger.test.js @@ -55,7 +55,7 @@ describe('DialogTrigger', function () { window.requestAnimationFrame.mockRestore(); }); - it.only('should trigger a modal by default', function () { + it('should trigger a modal by default', function () { let {queryByRole, getByRole, getByTestId} = render( From b9a411b41a5cc97ee98b32fb3139fee708d3cf9b Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 28 May 2021 17:19:50 -0500 Subject: [PATCH 05/16] fix broken tests --- .../@react-aria/overlays/test/useOverlay.test.js | 16 ++++------------ .../breadcrumbs/test/Breadcrumbs.test.js | 3 ++- .../menu/test/MenuTrigger.test.js | 7 ++++++- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/packages/@react-aria/overlays/test/useOverlay.test.js b/packages/@react-aria/overlays/test/useOverlay.test.js index 3da346c4bba..41273660811 100644 --- a/packages/@react-aria/overlays/test/useOverlay.test.js +++ b/packages/@react-aria/overlays/test/useOverlay.test.js @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -import {fireEvent, render} from '@testing-library/react'; +import {act, fireEvent, render} from '@testing-library/react'; import {installMouseEvent, installPointerEvent} from '@react-spectrum/test-utils'; import {mergeProps} from '@react-aria/utils'; import React, {useRef} from 'react'; @@ -29,6 +29,9 @@ function Example(props) { } describe('useOverlay', function () { + beforeAll(() => { + jest.useFakeTimers(); + }); describe.each` type | prepare | actions ${'Mouse Events'} | ${installMouseEvent} | ${[ @@ -123,15 +126,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/breadcrumbs/test/Breadcrumbs.test.js b/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js index 1742ddafe07..e44cb79f451 100644 --- a/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js +++ b/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js @@ -14,7 +14,7 @@ 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 {act, render, within} from '@testing-library/react'; import {theme} from '@react-spectrum/theme-default'; import {triggerPress} from '@react-spectrum/test-utils'; @@ -299,6 +299,7 @@ describe('Breadcrumbs', function () { let menuButton = getByRole('button'); triggerPress(menuButton); + act(() => {jest.runAllTimers();}); let menu = getByRole('menu'); expect(menu).toBeTruthy(); diff --git a/packages/@react-spectrum/menu/test/MenuTrigger.test.js b/packages/@react-spectrum/menu/test/MenuTrigger.test.js index cdc4c1f0770..2377694e2d5 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]; From 9ad50ea60f7bfa92221682882e63860c567d2ee8 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 28 May 2021 17:23:32 -0500 Subject: [PATCH 06/16] fix lint --- packages/@react-aria/overlays/test/useOverlay.test.js | 5 +---- packages/@react-spectrum/overlays/src/Popover.tsx | 4 ---- packages/@react-types/overlays/src/index.d.ts | 2 +- 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/packages/@react-aria/overlays/test/useOverlay.test.js b/packages/@react-aria/overlays/test/useOverlay.test.js index 41273660811..c13fa77067b 100644 --- a/packages/@react-aria/overlays/test/useOverlay.test.js +++ b/packages/@react-aria/overlays/test/useOverlay.test.js @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -import {act, fireEvent, render} from '@testing-library/react'; +import {fireEvent, render} from '@testing-library/react'; import {installMouseEvent, installPointerEvent} from '@react-spectrum/test-utils'; import {mergeProps} from '@react-aria/utils'; import React, {useRef} from 'react'; @@ -29,9 +29,6 @@ function Example(props) { } describe('useOverlay', function () { - beforeAll(() => { - jest.useFakeTimers(); - }); describe.each` type | prepare | actions ${'Mouse Events'} | ${installMouseEvent} | ${[ diff --git a/packages/@react-spectrum/overlays/src/Popover.tsx b/packages/@react-spectrum/overlays/src/Popover.tsx index 81c94824845..58489d1290f 100644 --- a/packages/@react-spectrum/overlays/src/Popover.tsx +++ b/packages/@react-spectrum/overlays/src/Popover.tsx @@ -30,7 +30,6 @@ interface PopoverWrapperProps extends HTMLAttributes { shouldCloseOnBlur?: boolean, isKeyboardDismissDisabled?: boolean, isNonModal?: boolean, - shouldCloseOnInteractOutside?: (element: HTMLElement) => boolean, isDismissable?: boolean } @@ -58,7 +57,6 @@ function Popover(props: PopoverProps, ref: DOMRef) { hideArrow, isKeyboardDismissDisabled, isNonModal, - shouldCloseOnInteractOutside, isDismissable = true, ...otherProps } = props; @@ -74,7 +72,6 @@ function Popover(props: PopoverProps, ref: DOMRef) { arrowProps={arrowProps} onClose={onClose} shouldCloseOnBlur={shouldCloseOnBlur} - shouldCloseOnInteractOutside={shouldCloseOnInteractOutside} isKeyboardDismissDisabled={isKeyboardDismissDisabled} hideArrow={hideArrow} isNonModal={isNonModal} @@ -96,7 +93,6 @@ const PopoverWrapper = forwardRef((props: PopoverWrapperProps, ref: RefObject void, shouldCloseOnBlur?: boolean, isNonModal?: boolean, - shouldCloseOnInteractOutside?: (element: HTMLElement) => boolean + isDismissable?: boolean } export interface TrayProps extends StyleProps, OverlayProps { From d6647290ae5ae3548e175d57d52e072fb82ddd64 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 28 May 2021 17:27:13 -0500 Subject: [PATCH 07/16] Moar lint fixes --- .../@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js | 5 ++++- packages/@react-spectrum/overlays/src/Popover.tsx | 6 ++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js b/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js index e44cb79f451..a925e8a1389 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 {act, 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) { diff --git a/packages/@react-spectrum/overlays/src/Popover.tsx b/packages/@react-spectrum/overlays/src/Popover.tsx index 58489d1290f..db3d188916f 100644 --- a/packages/@react-spectrum/overlays/src/Popover.tsx +++ b/packages/@react-spectrum/overlays/src/Popover.tsx @@ -83,20 +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, + // 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}, ref); + let {overlayProps} = useOverlay(props, ref); let {modalProps} = useModal({ isDisabled: isNonModal }); From bd1532a30bc0e9138ca659be51f93df1bd222f45 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 28 May 2021 17:35:28 -0500 Subject: [PATCH 08/16] fix last test --- packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js b/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js index a925e8a1389..2776b91e440 100644 --- a/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js +++ b/packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js @@ -315,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 From 7550f3b06eac253096d99ba344c41f05ebb44333 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 28 May 2021 18:15:09 -0500 Subject: [PATCH 09/16] fix firefox, again --- .../interactions/src/useInteractOutside.ts | 1 + packages/@react-aria/menu/stories/useMenu.stories.tsx | 1 + packages/@react-aria/overlays/test/useOverlay.test.js | 11 +++++++++++ 3 files changed, 13 insertions(+) diff --git a/packages/@react-aria/interactions/src/useInteractOutside.ts b/packages/@react-aria/interactions/src/useInteractOutside.ts index 2db0b20f062..4bf51d7ffc2 100644 --- a/packages/@react-aria/interactions/src/useInteractOutside.ts +++ b/packages/@react-aria/interactions/src/useInteractOutside.ts @@ -42,6 +42,7 @@ export function useInteractOutside(props: InteractOutsideProps) { return; } if (isValidEvent(e, ref) && onInteractOutside) { + e.preventDefault(); e.stopPropagation(); 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..2c49461d967 100644 --- a/packages/@react-aria/menu/stories/useMenu.stories.tsx +++ b/packages/@react-aria/menu/stories/useMenu.stories.tsx @@ -39,6 +39,7 @@ storiesOf('useMenu', module) Cut Paste + ) ); diff --git a/packages/@react-aria/overlays/test/useOverlay.test.js b/packages/@react-aria/overlays/test/useOverlay.test.js index c13fa77067b..3da346c4bba 100644 --- a/packages/@react-aria/overlays/test/useOverlay.test.js +++ b/packages/@react-aria/overlays/test/useOverlay.test.js @@ -123,4 +123,15 @@ 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 + }); + }); }); From a272b1ef49ecbefb336a433e351fced202b10226 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 28 May 2021 18:25:05 -0500 Subject: [PATCH 10/16] being consistent --- packages/@react-aria/interactions/src/useInteractOutside.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/@react-aria/interactions/src/useInteractOutside.ts b/packages/@react-aria/interactions/src/useInteractOutside.ts index 4bf51d7ffc2..f4796f8b936 100644 --- a/packages/@react-aria/interactions/src/useInteractOutside.ts +++ b/packages/@react-aria/interactions/src/useInteractOutside.ts @@ -55,6 +55,7 @@ export function useInteractOutside(props: InteractOutsideProps) { return; } if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) { + e.preventDefault(); e.stopPropagation(); state.isPointerDown = false; onInteractOutside(e); @@ -77,6 +78,7 @@ export function useInteractOutside(props: InteractOutsideProps) { if (state.ignoreEmulatedMouseEvents) { state.ignoreEmulatedMouseEvents = false; } else if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) { + e.preventDefault(); e.stopPropagation(); state.isPointerDown = false; onInteractOutside(e); @@ -89,6 +91,7 @@ export function useInteractOutside(props: InteractOutsideProps) { } state.ignoreEmulatedMouseEvents = true; if (onInteractOutside && state.isPointerDown && isValidEvent(e, ref)) { + e.preventDefault(); e.stopPropagation(); state.isPointerDown = false; onInteractOutside(e); From 158e8ea8e1fb7c07c0b8f215a8639e36d77f2e6a Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Mon, 7 Jun 2021 11:41:44 -0700 Subject: [PATCH 11/16] adding onInteractOutsideStart move stoppropagation and preventdefault to useoverlay --- .../interactions/src/useInteractOutside.ts | 14 +++++--------- packages/@react-aria/overlays/src/useOverlay.ts | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/packages/@react-aria/interactions/src/useInteractOutside.ts b/packages/@react-aria/interactions/src/useInteractOutside.ts index f4796f8b936..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 @@ -42,8 +43,9 @@ export function useInteractOutside(props: InteractOutsideProps) { return; } if (isValidEvent(e, ref) && onInteractOutside) { - e.preventDefault(); - e.stopPropagation(); + if (onInteractOutsideStart) { + onInteractOutsideStart(e); + } state.isPointerDown = true; } }; @@ -55,8 +57,6 @@ export function useInteractOutside(props: InteractOutsideProps) { return; } if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) { - e.preventDefault(); - e.stopPropagation(); state.isPointerDown = false; onInteractOutside(e); } @@ -78,8 +78,6 @@ export function useInteractOutside(props: InteractOutsideProps) { if (state.ignoreEmulatedMouseEvents) { state.ignoreEmulatedMouseEvents = false; } else if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) { - e.preventDefault(); - e.stopPropagation(); state.isPointerDown = false; onInteractOutside(e); } @@ -91,8 +89,6 @@ export function useInteractOutside(props: InteractOutsideProps) { } state.ignoreEmulatedMouseEvents = true; if (onInteractOutside && state.isPointerDown && isValidEvent(e, ref)) { - e.preventDefault(); - e.stopPropagation(); state.isPointerDown = false; onInteractOutside(e); } 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, From a6443d32a5d5d1255d21b8b856d0e7009310d978 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Mon, 7 Jun 2021 12:48:47 -0700 Subject: [PATCH 12/16] write tests for the cases we had fix overlaytrigger state so it uses previous state --- .../dialog/test/DialogTrigger.test.js | 97 +++++++++++++++++++ .../menu/test/MenuTrigger.test.js | 46 ++++++++- .../overlays/src/useOverlayTriggerState.ts | 2 +- 3 files changed, 143 insertions(+), 2 deletions(-) diff --git a/packages/@react-spectrum/dialog/test/DialogTrigger.test.js b/packages/@react-spectrum/dialog/test/DialogTrigger.test.js index f75ee79458e..312254ef3ef 100644 --- a/packages/@react-spectrum/dialog/test/DialogTrigger.test.js +++ b/packages/@react-spectrum/dialog/test/DialogTrigger.test.js @@ -321,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( @@ -842,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 91b14d32fd4..52fbdad828f 100644 --- a/packages/@react-spectrum/menu/test/MenuTrigger.test.js +++ b/packages/@react-spectrum/menu/test/MenuTrigger.test.js @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -import {act, fireEvent, render, within} from '@testing-library/react'; +import {act, fireEvent, queryByRole, render, within} from '@testing-library/react'; import {Button} from '@react-spectrum/button'; import {Item, Menu, MenuTrigger, Section} from '../'; import {Provider} from '@react-spectrum/provider'; @@ -775,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-stately/overlays/src/useOverlayTriggerState.ts b/packages/@react-stately/overlays/src/useOverlayTriggerState.ts index 8e1f7c2b28c..4f9abbd5669 100644 --- a/packages/@react-stately/overlays/src/useOverlayTriggerState.ts +++ b/packages/@react-stately/overlays/src/useOverlayTriggerState.ts @@ -40,7 +40,7 @@ export function useOverlayTriggerState(props: OverlayTriggerProps): OverlayTrigg setOpen(false); }, toggle() { - setOpen(!isOpen); + setOpen(wasOpen => !wasOpen); } }; } From 650675de171e86e960f1b4c06d4093de73a61fa4 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Mon, 7 Jun 2021 13:16:33 -0700 Subject: [PATCH 13/16] fix lint --- packages/@react-spectrum/menu/test/MenuTrigger.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-spectrum/menu/test/MenuTrigger.test.js b/packages/@react-spectrum/menu/test/MenuTrigger.test.js index 52fbdad828f..4aff365a343 100644 --- a/packages/@react-spectrum/menu/test/MenuTrigger.test.js +++ b/packages/@react-spectrum/menu/test/MenuTrigger.test.js @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -import {act, fireEvent, queryByRole, render, within} from '@testing-library/react'; +import {act, fireEvent, render, within} from '@testing-library/react'; import {Button} from '@react-spectrum/button'; import {Item, Menu, MenuTrigger, Section} from '../'; import {Provider} from '@react-spectrum/provider'; From fa41cda32da288b233c9e06e90ea86bc7467ffe7 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Mon, 7 Jun 2021 15:26:27 -0700 Subject: [PATCH 14/16] fix comobobox --- packages/@react-aria/menu/stories/useMenu.stories.tsx | 2 +- packages/@react-stately/overlays/src/useOverlayTriggerState.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@react-aria/menu/stories/useMenu.stories.tsx b/packages/@react-aria/menu/stories/useMenu.stories.tsx index 2c49461d967..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 diff --git a/packages/@react-stately/overlays/src/useOverlayTriggerState.ts b/packages/@react-stately/overlays/src/useOverlayTriggerState.ts index 4f9abbd5669..8e1f7c2b28c 100644 --- a/packages/@react-stately/overlays/src/useOverlayTriggerState.ts +++ b/packages/@react-stately/overlays/src/useOverlayTriggerState.ts @@ -40,7 +40,7 @@ export function useOverlayTriggerState(props: OverlayTriggerProps): OverlayTrigg setOpen(false); }, toggle() { - setOpen(wasOpen => !wasOpen); + setOpen(!isOpen); } }; } From f230f14eb7e62ce69d600359f274bc9b73ce2a14 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Mon, 7 Jun 2021 15:40:39 -0700 Subject: [PATCH 15/16] Fix safari menu double tap to open leads to hover opening --- packages/@react-spectrum/overlays/src/Popover.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-spectrum/overlays/src/Popover.tsx b/packages/@react-spectrum/overlays/src/Popover.tsx index db3d188916f..6f6c81c0a5a 100644 --- a/packages/@react-spectrum/overlays/src/Popover.tsx +++ b/packages/@react-spectrum/overlays/src/Popover.tsx @@ -98,7 +98,7 @@ const PopoverWrapper = forwardRef((props: PopoverWrapperProps, ref: RefObject Date: Mon, 7 Jun 2021 15:49:47 -0700 Subject: [PATCH 16/16] Add combobox test for clicking button for focus --- .../combobox/test/ComboBox.test.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) 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 () {