Fix Popover not closing from pressing trigger

packages/@react-aria/interactions/src/useInteractOutside.ts

@@ -20,6 +20,7 @@ import {RefObject, SyntheticEvent, useEffect, useRef} from 'react';
 interface InteractOutsideProps {
   ref: RefObject<Element>,
   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;
       }
     };

packages/@react-aria/menu/stories/useMenu.stories.tsx

@@ -39,6 +39,7 @@ storiesOf('useMenu', module)
           <Item key="cut">Cut</Item>
           <Item key="paste">Paste</Item>
         </MenuButton>
+        <input />
       </div>
     )
   );

packages/@react-aria/overlays/src/useOverlay.ts

@@ -89,8 +89,21 @@ export function useOverlay(props: OverlayProps, ref: RefObject<HTMLElement>): Ov
     }
   };
 
+  let onInteractOutsideStart = (e: SyntheticEvent<HTMLElement>) => {
+    if (!shouldCloseOnInteractOutside || shouldCloseOnInteractOutside(e.target as HTMLElement)) {
+      if (visibleOverlays[visibleOverlays.length - 1] === ref) {
+        e.stopPropagation();
+        e.preventDefault();
+      }
+    }
+  };
+
   let onInteractOutside = (e: SyntheticEvent<HTMLElement>) => {
     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<HTMLElement>): 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,

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

packages/@react-spectrum/combobox/src/ComboBox.tsx

@@ -162,7 +162,8 @@ const ComboBoxBase = React.forwardRef(function ComboBoxBase<T extends object>(pr
         ref={popoverRef}
         placement={placement}
         hideArrow
-        isNonModal>
+        isNonModal
+        isDismissable={false}>
         <ListBoxBase
           ref={listBoxRef}
           domProps={listBoxProps}

packages/@react-spectrum/combobox/stories/ComboBox.stories.tsx

@@ -454,6 +454,19 @@ storiesOf('ComboBox', module)
     () => (
       <AsyncLoadingExampleControlledKeyWithReset />
     )
+  )
+  .add(
+    '2 comboboxes',
+    () => (
+      <Flex gap="size-100">
+        <ComboBox defaultItems={items} label="Combobox1">
+          {(item) => <Item>{item.name}</Item>}
+        </ComboBox>
+        <ComboBox defaultItems={items} label="Combobox2">
+          {(item) => <Item>{item.name}</Item>}
+        </ComboBox>
+      </Flex>
+    )
   );
 
 

packages/@react-spectrum/dialog/stories/DialogTrigger.stories.tsx

@@ -335,6 +335,26 @@ storiesOf('DialogTrigger', module)
   .add(
     'trigger visible through underlay',
     () => renderTriggerNotCentered({})
+  )
+  .add(
+    '2 popovers',
+    () => (
+      <Flex gap="size-200">
+        <DialogTrigger type="popover">
+          <ActionButton>Trigger</ActionButton>
+          <Dialog>
+            <Content>
+              <input />
+              <input />
+            </Content>
+          </Dialog>
+        </DialogTrigger>
+        <DialogTrigger type="popover">
+          <ActionButton>Trigger</ActionButton>
+          <Dialog><Content>Hi!</Content></Dialog>
+        </DialogTrigger>
+      </Flex>
+    )
   );
 
 function render({width = 'auto', ...props}) {

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(
       <Provider theme={theme}>
         <DialogTrigger>
           <ActionButton>Trigger</ActionButton>
@@ -65,9 +65,7 @@ describe('DialogTrigger', function () {
       </Provider>
     );
 
-    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(
+      <Provider theme={theme}>
+        <DialogTrigger onOpenChange={onOpenChange} type="popover">
+          <ActionButton>Trigger</ActionButton>
+          <Dialog>contents</Dialog>
+        </DialogTrigger>
+      </Provider>
+    );
+
+    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(
+      <Provider theme={theme}>
+        <TextField id="document-input" aria-label="document input" />
+        <DialogTrigger type="popover">
+          <ActionButton id="outer-trigger">Trigger1</ActionButton>
+          <Dialog id="outer-dialog">
+            <Content>
+              <TextField id="outer-input" aria-label="outer input" />
+              <DialogTrigger type="popover">
+                <ActionButton id="inner-trigger">Trigger2</ActionButton>
+                <Dialog id="inner-dialog">
+                  <Content>
+                    <TextField id="inner-input" label="inner input" />
+                  </Content>
+                </Dialog>
+              </DialogTrigger>
+            </Content>
+          </Dialog>
+        </DialogTrigger>
+      </Provider>
+    );
+    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);
+  });
 });

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';
@@ -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(
+      <Provider theme={theme}>
+        <MenuTrigger>
+          <Button>
+            {triggerText}
+          </Button>
+          <Menu items={withSection}>
+            <Item key="alpha">Alpha</Item>
+            <Item key="bravo">Bravo</Item>
+          </Menu>
+        </MenuTrigger>
+        <MenuTrigger>
+          <Button>
+            {triggerText}
+          </Button>
+          <Menu items={withSection}>
+            <Item key="whiskey">Whiskey</Item>
+            <Item key="tango">Tango</Item>
+            <Item key="foxtrot">Foxtrot</Item>
+          </Menu>
+        </MenuTrigger>
+      </Provider>
+    );
+    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();
+  });
 });