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

@@ -28,7 +28,7 @@ storiesOf('useMenu', module)
   .add('double menu fires onInteractOutside',
     () => (
       <div>
-        <div>This should just be there to show that onInteractOutside fires when clicking on another trigger, don't worry about two open menus at once.</div>
+        <div>This should just be there to show that onInteractOutside fires when clicking on another trigger.</div>
         <MenuButton label="Actions">
           <Item key="copy">Copy</Item>
           <Item key="cut">Cut</Item>
@@ -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/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 () {

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);
+  });
 });