Skip to content

Commit c166369

Browse files
reidbarberLFDanLu
reidbarber
and
LFDanLu
authored
Update RAC Submenu to close all submenus when interacting outside of root submenu (#5827)
* close all submenus when interacting outside of root submenu * Get rid of underlay pointer down hack in favor of useInteractOutside * Make sure to only call root menu close all via useInteractOutside if it has a open submenu prevents double onOpenChange calls when a user clicks outside of controlled open=true menu * fix use in RAC * fix test and add one to RAC submenu for checking for click on submenu trigger * update logic for Menu useInteractOutside * fix case where clicking on the root menu item sub menu trigger closes everything * fix lint --------- Co-authored-by: Daniel Lu <[email protected]>
1 parent 2c229fd commit c166369

File tree

7 files changed

+109
-25
lines changed

7 files changed

+109
-25
lines changed

‎packages/@react-spectrum/menu/src/Menu.tsx

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import {mergeProps, useSlotId, useSyncRef} from '@react-aria/utils';
2424
import React, {ReactElement, useContext, useEffect, useRef, useState} from 'react';
2525
import {SpectrumMenuProps} from '@react-types/menu';
2626
import styles from '@adobe/spectrum-css-temp/components/menu/vars.css';
27+
import {useInteractOutside} from '@react-aria/interactions';
2728
import {useLocale, useLocalizedStringFormatter} from '@react-aria/i18n';
2829
import {useMenu} from '@react-aria/menu';
2930
import {useTreeState} from '@react-stately/tree';
@@ -59,6 +60,16 @@ function Menu<T extends object>(props: SpectrumMenuProps<T>, ref: DOMRef<HTMLDiv
5960

6061
let menuLevel = contextProps.submenuLevel ?? -1;
6162
let hasOpenSubmenu = state.collection.getItem(rootMenuTriggerState?.UNSTABLE_expandedKeysStack[menuLevel + 1]) != null;
63+
useInteractOutside({
64+
ref: domRef,
65+
onInteractOutside: (e) => {
66+
if (!popoverContainer?.contains(e.target) && !trayContainerRef.current?.contains(e.target)) {
67+
rootMenuTriggerState.close();
68+
}
69+
},
70+
isDisabled: isSubmenu || !hasOpenSubmenu
71+
});
72+
6273
// TODO: add slide transition
6374
return (
6475
<MenuStateContext.Provider value={{popoverContainer, trayContainerRef, menu: domRef, submenu: submenuRef, rootMenuTriggerState, state}}>

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1213,8 +1213,8 @@ describe('MenuTrigger', function () {
12131213
expect(document.activeElement).toBe(dialog);
12141214

12151215
let underlay = tree.getByTestId('underlay', {hidden: true});
1216-
fireEvent.pointerDown(underlay);
1217-
fireEvent.pointerUp(underlay);
1216+
fireEvent.mouseDown(underlay);
1217+
fireEvent.mouseUp(underlay);
12181218
act(() => {jest.runAllTimers();});
12191219
act(() => {jest.runAllTimers();});
12201220
expect(dialog).not.toBeInTheDocument();

‎packages/@react-spectrum/menu/test/SubMenuTrigger.test.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -332,8 +332,9 @@ describe('Submenu', function () {
332332

333333
// @ts-ignore
334334
let underlay = tree.getByTestId('underlay', {hidden: true});
335-
fireEvent.pointerDown(underlay);
336-
fireEvent.pointerUp(underlay);
335+
// TODO: use mouseDown for now, adding installPointerEvents makes some the other tests break
336+
fireEvent.mouseDown(underlay);
337+
fireEvent.mouseUp(underlay);
337338
act(() => {jest.runAllTimers();});
338339
act(() => {jest.runAllTimers();});
339340
menus = tree.queryAllByRole('menu', {hidden: true});

‎packages/@react-spectrum/overlays/src/Popover.tsx

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -117,14 +117,10 @@ const PopoverWrapper = forwardRef((props: PopoverWrapperProps, ref: RefObject<HT
117117
}, state);
118118
let {focusWithinProps} = useFocusWithin(props);
119119

120-
let onPointerDown = () => {
121-
state.close();
122-
};
123-
124120
// Attach Transition's nodeRef to outermost wrapper for node.reflow: https://github.com/reactjs/react-transition-group/blob/c89f807067b32eea6f68fd6c622190d88ced82e2/src/Transition.js#L231
125121
return (
126122
<div ref={wrapperRef}>
127-
{!isNonModal && <Underlay isTransparent {...mergeProps(underlayProps, {onPointerDown})} isOpen={isOpen} /> }
123+
{!isNonModal && <Underlay isTransparent {...mergeProps(underlayProps)} isOpen={isOpen} /> }
128124
<div
129125
{...styleProps}
130126
{...mergeProps(popoverProps, focusWithinProps)}

‎packages/react-aria-components/src/Menu.tsx

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ import {Header} from './Header';
2020
import {Key, LinkDOMProps} from '@react-types/shared';
2121
import {KeyboardContext} from './Keyboard';
2222
import {OverlayTriggerStateContext} from './Dialog';
23-
import {PopoverContext} from './Popover';
24-
import {PressResponder, useHover} from '@react-aria/interactions';
23+
import {PopoverContext, PopoverProps} from './Popover';
24+
import {PressResponder, useHover, useInteractOutside} from '@react-aria/interactions';
2525
import React, {createContext, ForwardedRef, forwardRef, ReactElement, ReactNode, RefObject, useCallback, useContext, useRef, useState} from 'react';
2626
import {RootMenuTriggerState, UNSTABLE_useSubmenuTriggerState} from '@react-stately/menu';
2727
import {Separator, SeparatorContext} from './Separator';
@@ -147,6 +147,8 @@ function MenuInner<T extends object>({props, collection, menuRef: ref}: MenuInne
147147
});
148148
let [popoverContainer, setPopoverContainer] = useState<HTMLDivElement | null>(null);
149149
let {menuProps} = useMenu(props, state, ref);
150+
let rootMenuTriggerState = useContext(RootMenuTriggerStateContext)!;
151+
let popoverContext = useContext(PopoverContext)!;
150152

151153
let children = useCachedChildren({
152154
items: state.collection,
@@ -166,6 +168,17 @@ function MenuInner<T extends object>({props, collection, menuRef: ref}: MenuInne
166168
}
167169
});
168170

171+
let isSubmenu = (popoverContext as PopoverProps)?.trigger === 'SubmenuTrigger';
172+
useInteractOutside({
173+
ref,
174+
onInteractOutside: (e) => {
175+
if (rootMenuTriggerState && !popoverContainer?.contains(e.target as HTMLElement)) {
176+
rootMenuTriggerState.close();
177+
}
178+
},
179+
isDisabled: isSubmenu || rootMenuTriggerState?.UNSTABLE_expandedKeysStack.length === 0
180+
});
181+
169182
return (
170183
<FocusScope>
171184
<div

‎packages/react-aria-components/src/Popover.tsx

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -150,13 +150,9 @@ function PopoverInner({state, isExiting, UNSTABLE_portalContainer, ...props}: Po
150150

151151
let style = {...popoverProps.style, ...renderProps.style};
152152

153-
let onPointerDown = () => {
154-
state.close();
155-
};
156-
157153
return (
158154
<Overlay isExiting={isExiting} portalContainer={UNSTABLE_portalContainer}>
159-
{!props.isNonModal && state.isOpen && <div data-testid="underlay" {...mergeProps(underlayProps, {onPointerDown})} style={{position: 'fixed', inset: 0}} />}
155+
{!props.isNonModal && state.isOpen && <div data-testid="underlay" {...underlayProps} style={{position: 'fixed', inset: 0}} />}
160156
<div
161157
{...mergeProps(filterDOMProps(props as any), popoverProps)}
162158
{...renderProps}

‎packages/react-aria-components/test/Menu.test.js

Lines changed: 76 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,7 @@ describe('Menu', () => {
409409
expect(triggerItem).toHaveAttribute('data-open', 'true');
410410
let submenu = getAllByRole('menu')[1];
411411
expect(submenu).toBeInTheDocument();
412-
412+
413413
let submenuPopover = submenu.closest('.react-aria-Popover');
414414
expect(submenuPopover).toBeInTheDocument();
415415
expect(submenuPopover).toHaveAttribute('data-trigger', 'SubmenuTrigger');
@@ -482,7 +482,7 @@ describe('Menu', () => {
482482
expect(triggerItem).toHaveAttribute('data-open', 'true');
483483
let submenu = getAllByRole('menu')[1];
484484
expect(submenu).toBeInTheDocument();
485-
485+
486486
let submenuPopover = submenu.closest('.react-aria-Popover');
487487
expect(submenuPopover).toBeInTheDocument();
488488
expect(submenuPopover).toHaveAttribute('data-trigger', 'SubmenuTrigger');
@@ -513,7 +513,7 @@ describe('Menu', () => {
513513
expect(nestedSubmenu).not.toBeInTheDocument();
514514
expect(submenu).not.toBeInTheDocument();
515515
});
516-
it('should close all submenus if underlay is clicked', async () => {
516+
it('should close all submenus if interacting outside root submenu', async () => {
517517
let onAction = jest.fn();
518518
let {getByRole, getAllByRole, getByTestId} = render(
519519
<MenuTrigger>
@@ -575,7 +575,7 @@ describe('Menu', () => {
575575
expect(triggerItem).toHaveAttribute('data-open', 'true');
576576
let submenu = getAllByRole('menu')[1];
577577
expect(submenu).toBeInTheDocument();
578-
578+
579579
let submenuPopover = submenu.closest('.react-aria-Popover');
580580
expect(submenuPopover).toBeInTheDocument();
581581
expect(submenuPopover).toHaveAttribute('data-trigger', 'SubmenuTrigger');
@@ -603,7 +603,7 @@ describe('Menu', () => {
603603
let underlay = getByTestId('underlay');
604604
expect(underlay).toBeInTheDocument();
605605
expect(underlay).toHaveAttribute('aria-hidden', 'true');
606-
await user.click(underlay);
606+
await user.click(document.body);
607607
expect(nestedSubmenu).not.toBeInTheDocument();
608608
expect(submenu).not.toBeInTheDocument();
609609
expect(menu).not.toBeInTheDocument();
@@ -662,7 +662,7 @@ describe('Menu', () => {
662662
expect(triggerItem).toHaveAttribute('data-open', 'true');
663663
let submenu = getAllByRole('menu')[1];
664664
expect(submenu).toBeInTheDocument();
665-
665+
666666
let submenuItems = within(submenu).getAllByRole('menuitem');
667667
expect(submenuItems).toHaveLength(3);
668668

@@ -673,7 +673,7 @@ describe('Menu', () => {
673673
fireEvent.keyDown(document.activeElement, {key: 'Escape'});
674674
fireEvent.keyUp(document.activeElement, {key: 'Escape'});
675675
act(() => {jest.runAllTimers();});
676-
676+
677677
expect(submenu).not.toBeInTheDocument();
678678
expect(menu).not.toBeInTheDocument();
679679
expect(document.activeElement).toBe(button);
@@ -740,7 +740,7 @@ describe('Menu', () => {
740740
expect(triggerItem).toHaveAttribute('data-open', 'true');
741741
let submenu = getAllByRole('menu')[1];
742742
expect(submenu).toBeInTheDocument();
743-
743+
744744
let submenuItems = within(submenu).getAllByRole('menuitem');
745745
expect(submenuItems).toHaveLength(3);
746746

@@ -760,11 +760,78 @@ describe('Menu', () => {
760760
fireEvent.keyDown(document.activeElement, {key: 'Escape'});
761761
fireEvent.keyUp(document.activeElement, {key: 'Escape'});
762762
act(() => {jest.runAllTimers();});
763-
763+
764764
expect(nestedSubmenu).not.toBeInTheDocument();
765765
expect(submenu).not.toBeInTheDocument();
766766
expect(menu).not.toBeInTheDocument();
767767
expect(document.activeElement).toBe(button);
768768
});
769+
it('should not close the menu when clicking on a element within the submenu tree', async () => {
770+
let onAction = jest.fn();
771+
let {getByRole, getAllByRole, queryAllByRole} = render(
772+
<MenuTrigger>
773+
<Button aria-label="Menu">☰</Button>
774+
<Popover>
775+
<Menu onAction={onAction}>
776+
<MenuItem id="open">Open</MenuItem>
777+
<MenuItem id="rename">Rename…</MenuItem>
778+
<MenuItem id="duplicate">Duplicate</MenuItem>
779+
<SubmenuTrigger>
780+
<MenuItem id="share">Share…</MenuItem>
781+
<Popover>
782+
<Menu onAction={onAction}>
783+
<SubmenuTrigger>
784+
<MenuItem id="email">Email…</MenuItem>
785+
<Popover>
786+
<Menu onAction={onAction}>
787+
<MenuItem id="work">Work</MenuItem>
788+
<MenuItem id="personal">Personal</MenuItem>
789+
</Menu>
790+
</Popover>
791+
</SubmenuTrigger>
792+
<MenuItem id="sms">SMS</MenuItem>
793+
<MenuItem id="twitter">Twitter</MenuItem>
794+
</Menu>
795+
</Popover>
796+
</SubmenuTrigger>
797+
<MenuItem id="delete">Delete…</MenuItem>
798+
</Menu>
799+
</Popover>
800+
</MenuTrigger>
801+
);
802+
803+
let button = getByRole('button');
804+
expect(button).not.toHaveAttribute('data-pressed');
805+
806+
await user.click(button);
807+
expect(button).toHaveAttribute('data-pressed');
808+
809+
let menu = getAllByRole('menu')[0];
810+
expect(getAllByRole('menuitem')).toHaveLength(5);
811+
812+
let popover = menu.closest('.react-aria-Popover');
813+
expect(popover).toBeInTheDocument();
814+
815+
let triggerItem = getAllByRole('menuitem')[3];
816+
817+
// Open the submenu
818+
await user.pointer({target: triggerItem});
819+
act(() => {jest.runAllTimers();});
820+
let submenu = getAllByRole('menu')[1];
821+
expect(submenu).toBeInTheDocument();
822+
823+
let nestedTriggerItem = getAllByRole('menuitem')[5];
824+
825+
// Click a nested submenu item trigger
826+
await user.click(nestedTriggerItem);
827+
act(() => {jest.runAllTimers();});
828+
let menus = getAllByRole('menu', {hidden: true});
829+
expect(menus).toHaveLength(3);
830+
831+
await user.click(getAllByRole('menuitem')[6]);
832+
menus = queryAllByRole('menu', {hidden: true});
833+
expect(menus).toHaveLength(0);
834+
expect(menu).not.toBeInTheDocument();
835+
});
769836
});
770837
});

0 commit comments

Comments
 (0)