From b38a339ca7a3dc171baf845ff7d4aa98caa8e821 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Fri, 7 Aug 2020 19:49:38 -0700 Subject: [PATCH 1/6] Fix double menu open This moves useInteractOutside to fire on mouse down instead of on up, this means menus won't reopen after they've been closed by the blur It also takes advantage of shouldCloseOnInteractOutside, which lets us detect if the the interaction was on the trigger, which will automatically toggle the menu when clicked. If we couldn't filter that event out, then the menu would just be closed from outside interaction and toggled open again immediately cleans up trigger toggle state so it always uses the latest state --- .../interactions/src/useInteractOutside.ts | 43 ++++++------------- .../@react-aria/overlays/src/useOverlay.ts | 12 +++--- packages/@react-aria/select/src/useSelect.ts | 2 +- .../@react-spectrum/menu/src/MenuTrigger.tsx | 10 ++++- .../menu/test/MenuTrigger.test.js | 2 +- .../@react-spectrum/overlays/src/Popover.tsx | 10 +++-- .../@react-spectrum/picker/src/Picker.tsx | 11 ++++- .../picker/stories/Picker.stories.tsx | 2 +- .../picker/test/Picker.test.js | 5 ++- .../overlays/src/useOverlayTriggerState.ts | 2 +- packages/@react-types/overlays/src/index.d.ts | 3 +- 11 files changed, 53 insertions(+), 49 deletions(-) diff --git a/packages/@react-aria/interactions/src/useInteractOutside.ts b/packages/@react-aria/interactions/src/useInteractOutside.ts index 5c91732d9b5..5ba764857d1 100644 --- a/packages/@react-aria/interactions/src/useInteractOutside.ts +++ b/packages/@react-aria/interactions/src/useInteractOutside.ts @@ -29,65 +29,48 @@ interface InteractOutsideProps { export function useInteractOutside(props: InteractOutsideProps) { let {ref, onInteractOutside} = props; let stateRef = useRef({ - isPointerDown: false, ignoreEmulatedMouseEvents: false }); let state = stateRef.current; useEffect(() => { + if (!onInteractOutside) { + return; + } let onPointerDown = (e) => { + if (state.ignoreEmulatedMouseEvents) { + state.ignoreEmulatedMouseEvents = false; + return; + } if (isValidEvent(e, ref)) { - state.isPointerDown = true; + onInteractOutside(e); } }; // Use pointer events if available. Otherwise, fall back to mouse and touch events. if (typeof PointerEvent !== 'undefined') { - let onPointerUp = (e) => { - if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) { - state.isPointerDown = false; - onInteractOutside(e); - } - }; - document.addEventListener('pointerdown', onPointerDown, false); - document.addEventListener('pointerup', onPointerUp, false); return () => { document.removeEventListener('pointerdown', onPointerDown, false); - document.removeEventListener('pointerup', onPointerUp, false); }; } else { - let onMouseUp = (e) => { - if (state.ignoreEmulatedMouseEvents) { - state.ignoreEmulatedMouseEvents = false; - } else if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) { - state.isPointerDown = false; - onInteractOutside(e); - } - }; - - let onTouchEnd = (e) => { - state.ignoreEmulatedMouseEvents = true; - if (onInteractOutside && state.isPointerDown && isValidEvent(e, ref)) { - state.isPointerDown = false; + let onTouchStart = (e) => { + if (isValidEvent(e, ref)) { + state.ignoreEmulatedMouseEvents = true; onInteractOutside(e); } }; document.addEventListener('mousedown', onPointerDown, false); - document.addEventListener('mouseup', onMouseUp, false); - document.addEventListener('touchstart', onPointerDown, false); - document.addEventListener('touchend', onTouchEnd, false); + document.addEventListener('touchstart', onTouchStart, false); return () => { document.removeEventListener('mousedown', onPointerDown, false); - document.removeEventListener('mouseup', onMouseUp, false); document.removeEventListener('touchstart', onPointerDown, false); - document.removeEventListener('touchend', onTouchEnd, false); }; } - }, [onInteractOutside, ref, state.ignoreEmulatedMouseEvents, state.isPointerDown]); + }, [onInteractOutside, ref, state]); } function isValidEvent(event, ref) { diff --git a/packages/@react-aria/overlays/src/useOverlay.ts b/packages/@react-aria/overlays/src/useOverlay.ts index b0a431087ad..51db8a31be4 100644 --- a/packages/@react-aria/overlays/src/useOverlay.ts +++ b/packages/@react-aria/overlays/src/useOverlay.ts @@ -29,8 +29,8 @@ interface OverlayProps { /** Whether the overlay should close when focus is lost or moves outside it. */ shouldCloseOnBlur?: boolean, - /** - * Whether pressing the escape key to close the overlay should be disabled. + /** + * Whether pressing the escape key to close the overlay should be disabled. * @default false */ isKeyboardDismissDisabled?: boolean, @@ -38,7 +38,7 @@ interface OverlayProps { /** * When user interacts with the argument element outside of the overlay ref, * return true if onClose should be called. This gives you a chance to filter - * out interaction with elements that should not dismiss the overlay. + * out interaction with elements that should not dismiss the overlay. * By default, onClose will always be called on interaction outside the overlay ref. */ shouldCloseOnInteractOutside?: (element: HTMLElement) => boolean @@ -80,11 +80,11 @@ export function useOverlay(props: OverlayProps, ref: RefObject): Ov } }; - let onInteractOutside = (e: SyntheticEvent) => { + let onInteractOutside = isDismissable && isOpen ? (e: SyntheticEvent) => { if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(e.target as HTMLElement)) { onHide(); } - }; + } : undefined; // Handle the escape key let onKeyDown = (e) => { @@ -95,7 +95,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}); let {focusWithinProps} = useFocusWithin({ isDisabled: !shouldCloseOnBlur, diff --git a/packages/@react-aria/select/src/useSelect.ts b/packages/@react-aria/select/src/useSelect.ts index 8220d5696ad..ea286cf50c5 100644 --- a/packages/@react-aria/select/src/useSelect.ts +++ b/packages/@react-aria/select/src/useSelect.ts @@ -81,7 +81,7 @@ export function useSelect(props: AriaSelectOptions, state: SelectState, }); let domProps = filterDOMProps(props, {labelable: true}); - let triggerProps = mergeProps(mergeProps(menuTriggerProps, fieldProps), typeSelectProps); + let triggerProps = mergeProps(menuTriggerProps, fieldProps, typeSelectProps); let valueId = useId(); return { diff --git a/packages/@react-spectrum/menu/src/MenuTrigger.tsx b/packages/@react-spectrum/menu/src/MenuTrigger.tsx index 7169d013599..b400a21b725 100644 --- a/packages/@react-spectrum/menu/src/MenuTrigger.tsx +++ b/packages/@react-spectrum/menu/src/MenuTrigger.tsx @@ -73,6 +73,13 @@ function MenuTrigger(props: SpectrumMenuTriggerProps, ref: DOMRef) ); + let shouldCloseOnInteractOutside = (element) => { + if (menuTriggerRef.current && menuTriggerRef.current.contains(element)) { + return false; + } + return true; + }; + // On small screen devices, the menu is rendered in a tray, otherwise a popover. let overlay; if (isMobile) { @@ -90,7 +97,8 @@ function MenuTrigger(props: SpectrumMenuTriggerProps, ref: DOMRef) placement={placement} hideArrow onClose={state.close} - shouldCloseOnBlur> + shouldCloseOnBlur + shouldCloseOnInteractOutside={shouldCloseOnInteractOutside}> {contents} ); diff --git a/packages/@react-spectrum/menu/test/MenuTrigger.test.js b/packages/@react-spectrum/menu/test/MenuTrigger.test.js index 75774af956b..62db5a41b5a 100644 --- a/packages/@react-spectrum/menu/test/MenuTrigger.test.js +++ b/packages/@react-spectrum/menu/test/MenuTrigger.test.js @@ -221,7 +221,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 diff --git a/packages/@react-spectrum/overlays/src/Popover.tsx b/packages/@react-spectrum/overlays/src/Popover.tsx index 56c9d5b8cd8..28951316205 100644 --- a/packages/@react-spectrum/overlays/src/Popover.tsx +++ b/packages/@react-spectrum/overlays/src/Popover.tsx @@ -28,7 +28,8 @@ interface PopoverWrapperProps extends HTMLAttributes { isOpen?: boolean, onClose?: () => void, shouldCloseOnBlur?: boolean, - isKeyboardDismissDisabled?: boolean + isKeyboardDismissDisabled?: boolean, + shouldCloseOnInteractOutside?: (element: HTMLElement) => boolean } /** @@ -46,7 +47,7 @@ let arrowPlacement = { }; function Popover(props: PopoverProps, ref: DOMRef) { - let {children, placement, arrowProps, onClose, shouldCloseOnBlur, hideArrow, isKeyboardDismissDisabled, ...otherProps} = props; + let {children, placement, arrowProps, onClose, shouldCloseOnBlur, hideArrow, isKeyboardDismissDisabled, shouldCloseOnInteractOutside, ...otherProps} = props; let domRef = useDOMRef(ref); let {styleProps} = useStyleProps(props); @@ -60,7 +61,8 @@ function Popover(props: PopoverProps, ref: DOMRef) { onClose={onClose} shouldCloseOnBlur={shouldCloseOnBlur} isKeyboardDismissDisabled={isKeyboardDismissDisabled} - hideArrow={hideArrow}> + hideArrow={hideArrow} + shouldCloseOnInteractOutside={shouldCloseOnInteractOutside}> {children} @@ -69,7 +71,7 @@ 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, ...otherProps} = props; + let {children, placement = 'bottom', arrowProps, isOpen, hideArrow, shouldCloseOnBlur, isKeyboardDismissDisabled, shouldCloseOnInteractOutside, ...otherProps} = props; let {overlayProps} = useOverlay({...props, isDismissable: true}, ref); let {modalProps} = useModal(); diff --git a/packages/@react-spectrum/picker/src/Picker.tsx b/packages/@react-spectrum/picker/src/Picker.tsx index 1bb7a171938..c74ff1f434e 100644 --- a/packages/@react-spectrum/picker/src/Picker.tsx +++ b/packages/@react-spectrum/picker/src/Picker.tsx @@ -149,6 +149,14 @@ function Picker(props: SpectrumPickerProps, ref: DOMRef { + if (triggerRef.current && triggerRef.current?.UNSAFE_getDOMNode().contains(element)) { + return false; + } + return true; + }; + + overlay = ( (props: SpectrumPickerProps, ref: DOMRef + onClose={state.close} + shouldCloseOnInteractOutside={shouldCloseOnInteractOutside}> {listbox} ); diff --git a/packages/@react-spectrum/picker/stories/Picker.stories.tsx b/packages/@react-spectrum/picker/stories/Picker.stories.tsx index ab6fed1aafd..d2895e2e2e2 100644 --- a/packages/@react-spectrum/picker/stories/Picker.stories.tsx +++ b/packages/@react-spectrum/picker/stories/Picker.stories.tsx @@ -54,7 +54,7 @@ storiesOf('Picker', module) .add( 'default', () => ( - + One Two Three diff --git a/packages/@react-spectrum/picker/test/Picker.test.js b/packages/@react-spectrum/picker/test/Picker.test.js index a937c215d8a..5c9025867d7 100644 --- a/packages/@react-spectrum/picker/test/Picker.test.js +++ b/packages/@react-spectrum/picker/test/Picker.test.js @@ -23,6 +23,7 @@ import React from 'react'; import {Text} from '@react-spectrum/text'; import {theme} from '@react-spectrum/theme-default'; import {triggerPress} from '@react-spectrum/test-utils'; +import userEvent from '@testing-library/user-event'; describe('Picker', function () { let offsetWidth, offsetHeight; @@ -365,7 +366,7 @@ describe('Picker', function () { expect(() => getByRole('listbox')).toThrow(); let picker = getByRole('button'); - act(() => triggerPress(picker)); + act(() => userEvent.click(picker)); act(() => jest.runAllTimers()); let listbox = getByRole('listbox'); @@ -375,7 +376,7 @@ describe('Picker', function () { expect(picker).toHaveAttribute('aria-expanded', 'true'); expect(picker).toHaveAttribute('aria-controls', listbox.id); - act(() => triggerPress(picker)); + act(() => userEvent.click(picker)); act(() => jest.runAllTimers()); expect(listbox).not.toBeInTheDocument(); diff --git a/packages/@react-stately/overlays/src/useOverlayTriggerState.ts b/packages/@react-stately/overlays/src/useOverlayTriggerState.ts index 8e1f7c2b28c..3ca89ded7c9 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(prev => !prev); } }; } diff --git a/packages/@react-types/overlays/src/index.d.ts b/packages/@react-types/overlays/src/index.d.ts index a8ab9e416b7..d19a7abc2fe 100644 --- a/packages/@react-types/overlays/src/index.d.ts +++ b/packages/@react-types/overlays/src/index.d.ts @@ -89,7 +89,8 @@ export interface PopoverProps extends StyleProps, OverlayProps { hideArrow?: boolean, isOpen?: boolean, onClose?: () => void, - shouldCloseOnBlur?: boolean + shouldCloseOnBlur?: boolean, + shouldCloseOnInteractOutside?: (element: HTMLElement) => boolean } export interface TrayProps extends StyleProps, OverlayProps { From cc23b8f1e5f14f1cafce2fc1b3dcd32039611324 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Mon, 10 Aug 2020 22:45:38 -0700 Subject: [PATCH 2/6] Reduce logic in expression --- packages/@react-spectrum/menu/src/MenuTrigger.tsx | 5 +---- packages/@react-spectrum/picker/src/Picker.tsx | 5 +---- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/packages/@react-spectrum/menu/src/MenuTrigger.tsx b/packages/@react-spectrum/menu/src/MenuTrigger.tsx index b400a21b725..aa03dc7f1bd 100644 --- a/packages/@react-spectrum/menu/src/MenuTrigger.tsx +++ b/packages/@react-spectrum/menu/src/MenuTrigger.tsx @@ -74,10 +74,7 @@ function MenuTrigger(props: SpectrumMenuTriggerProps, ref: DOMRef) ); let shouldCloseOnInteractOutside = (element) => { - if (menuTriggerRef.current && menuTriggerRef.current.contains(element)) { - return false; - } - return true; + return !(menuTriggerRef.current?.contains(element)); }; // On small screen devices, the menu is rendered in a tray, otherwise a popover. diff --git a/packages/@react-spectrum/picker/src/Picker.tsx b/packages/@react-spectrum/picker/src/Picker.tsx index c74ff1f434e..2c383c4f064 100644 --- a/packages/@react-spectrum/picker/src/Picker.tsx +++ b/packages/@react-spectrum/picker/src/Picker.tsx @@ -150,10 +150,7 @@ function Picker(props: SpectrumPickerProps, ref: DOMRef { - if (triggerRef.current && triggerRef.current?.UNSAFE_getDOMNode().contains(element)) { - return false; - } - return true; + return !(triggerRef.current?.UNSAFE_getDOMNode()?.contains(element)); }; From d47129a0290f299ab09da36b87e28389f84e71c7 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Tue, 11 Aug 2020 10:41:56 -0700 Subject: [PATCH 3/6] Add back the story with two menus --- .../menu/stories/MenuTrigger.stories.tsx | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx b/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx index 8cbbabce0f5..10748538ca3 100644 --- a/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx +++ b/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx @@ -19,6 +19,7 @@ import Blower from '@spectrum-icons/workflow/Blower'; import Book from '@spectrum-icons/workflow/Book'; import Copy from '@spectrum-icons/workflow/Copy'; import Cut from '@spectrum-icons/workflow/Cut'; +import {Flex} from '@react-spectrum/layout'; import {Item, Menu, MenuTrigger, Section} from '../'; import {Keyboard, Text} from '@react-spectrum/text'; import Paste from '@spectrum-icons/workflow/Paste'; @@ -477,7 +478,32 @@ storiesOf('MenuTrigger', module) ) - ); + ) + .add('double menu', () => { + return ( + + + + Menu Button 1 + + {defaultMenu} + + + + Menu Button 2 + + {defaultMenu} + + + ); + }); + let customMenuItem = (item) => { let Icon = iconMap[item.icon]; From e46b2ffb69909c5b04f42e8a831dc93bc5abd6ad Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Tue, 11 Aug 2020 14:25:43 -0700 Subject: [PATCH 4/6] fix lint --- .../@react-spectrum/menu/src/MenuTrigger.tsx | 4 +- .../menu/stories/MenuTrigger.stories.tsx | 46 +++++++++---------- .../@react-spectrum/picker/src/Picker.tsx | 5 +- 3 files changed, 24 insertions(+), 31 deletions(-) diff --git a/packages/@react-spectrum/menu/src/MenuTrigger.tsx b/packages/@react-spectrum/menu/src/MenuTrigger.tsx index aa03dc7f1bd..29900a5011e 100644 --- a/packages/@react-spectrum/menu/src/MenuTrigger.tsx +++ b/packages/@react-spectrum/menu/src/MenuTrigger.tsx @@ -73,9 +73,7 @@ function MenuTrigger(props: SpectrumMenuTriggerProps, ref: DOMRef) ); - let shouldCloseOnInteractOutside = (element) => { - return !(menuTriggerRef.current?.contains(element)); - }; + let shouldCloseOnInteractOutside = (element) => !menuTriggerRef.current?.contains(element); // On small screen devices, the menu is rendered in a tray, otherwise a popover. let overlay; diff --git a/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx b/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx index 10748538ca3..71ab61968dd 100644 --- a/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx +++ b/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx @@ -479,30 +479,28 @@ storiesOf('MenuTrigger', module) ) ) - .add('double menu', () => { - return ( - - - - Menu Button 1 - - {defaultMenu} - - - - Menu Button 2 - - {defaultMenu} - - - ); - }); + .add('double menu', () => ( + + + + Menu Button 1 + + {defaultMenu} + + + + Menu Button 2 + + {defaultMenu} + + + ); let customMenuItem = (item) => { diff --git a/packages/@react-spectrum/picker/src/Picker.tsx b/packages/@react-spectrum/picker/src/Picker.tsx index 2c383c4f064..b0855d7dc57 100644 --- a/packages/@react-spectrum/picker/src/Picker.tsx +++ b/packages/@react-spectrum/picker/src/Picker.tsx @@ -149,10 +149,7 @@ function Picker(props: SpectrumPickerProps, ref: DOMRef { - return !(triggerRef.current?.UNSAFE_getDOMNode()?.contains(element)); - }; - + let shouldCloseOnInteractOutside = (element) => !triggerRef.current?.UNSAFE_getDOMNode()?.contains(element); overlay = ( Date: Tue, 11 Aug 2020 15:33:07 -0700 Subject: [PATCH 5/6] missed a paren --- packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx b/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx index 71ab61968dd..4873211ea55 100644 --- a/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx +++ b/packages/@react-spectrum/menu/stories/MenuTrigger.stories.tsx @@ -500,7 +500,7 @@ storiesOf('MenuTrigger', module) {defaultMenu} - ); + )); let customMenuItem = (item) => { From 61babe3eb13e3a1fa6b3684a9bbd313d366d1288 Mon Sep 17 00:00:00 2001 From: Robert Snow Date: Tue, 11 Aug 2020 18:02:35 -0700 Subject: [PATCH 6/6] move code to an easier to read spot --- packages/@react-aria/overlays/src/useOverlay.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/@react-aria/overlays/src/useOverlay.ts b/packages/@react-aria/overlays/src/useOverlay.ts index 51db8a31be4..951a187abdb 100644 --- a/packages/@react-aria/overlays/src/useOverlay.ts +++ b/packages/@react-aria/overlays/src/useOverlay.ts @@ -80,11 +80,11 @@ export function useOverlay(props: OverlayProps, ref: RefObject): Ov } }; - let onInteractOutside = isDismissable && isOpen ? (e: SyntheticEvent) => { + let onInteractOutside = (e: SyntheticEvent) => { if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(e.target as HTMLElement)) { onHide(); } - } : undefined; + }; // Handle the escape key let onKeyDown = (e) => { @@ -95,7 +95,7 @@ export function useOverlay(props: OverlayProps, ref: RefObject): Ov }; // Handle clicking outside the overlay to close it - useInteractOutside({ref, onInteractOutside}); + useInteractOutside({ref, onInteractOutside: isDismissable && isOpen ? onInteractOutside : undefined}); let {focusWithinProps} = useFocusWithin({ isDisabled: !shouldCloseOnBlur,