Fix MenuTrigger closing immediately (#2175)

Author: devongovett

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

@@ -33,18 +33,23 @@ export function useInteractOutside(props: InteractOutsideProps) {
   let {ref, onInteractOutside, isDisabled, onInteractOutsideStart} = props;
   let stateRef = useRef({
     isPointerDown: false,
-    ignoreEmulatedMouseEvents: false
+    ignoreEmulatedMouseEvents: false,
+    onInteractOutside,
+    onInteractOutsideStart
   });
   let state = stateRef.current;
+  state.onInteractOutside = onInteractOutside;
+  state.onInteractOutsideStart = onInteractOutsideStart;
 
   useEffect(() => {
+    if (isDisabled) {
+      return;
+    }
+
     let onPointerDown = (e) => {
-      if (isDisabled) {
-        return;
-      }
-      if (isValidEvent(e, ref) && onInteractOutside) {
-        if (onInteractOutsideStart) {
-          onInteractOutsideStart(e);
+      if (isValidEvent(e, ref) && state.onInteractOutside) {
+        if (state.onInteractOutsideStart) {
+          state.onInteractOutsideStart(e);
         }
         state.isPointerDown = true;
       }
@@ -53,12 +58,9 @@ export function useInteractOutside(props: InteractOutsideProps) {
     // Use pointer events if available. Otherwise, fall back to mouse and touch events.
     if (typeof PointerEvent !== 'undefined') {
       let onPointerUp = (e) => {
-        if (isDisabled) {
-          return;
-        }
-        if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) {
+        if (state.isPointerDown && state.onInteractOutside && isValidEvent(e, ref)) {
           state.isPointerDown = false;
-          onInteractOutside(e);
+          state.onInteractOutside(e);
         }
       };
 
@@ -72,25 +74,19 @@ export function useInteractOutside(props: InteractOutsideProps) {
       };
     } else {
       let onMouseUp = (e) => {
-        if (isDisabled) {
-          return;
-        }
         if (state.ignoreEmulatedMouseEvents) {
           state.ignoreEmulatedMouseEvents = false;
-        } else if (state.isPointerDown && onInteractOutside && isValidEvent(e, ref)) {
+        } else if (state.isPointerDown && state.onInteractOutside && isValidEvent(e, ref)) {
           state.isPointerDown = false;
-          onInteractOutside(e);
+          state.onInteractOutside(e);
         }
       };
 
       let onTouchEnd = (e) => {
-        if (isDisabled) {
-          return;
-        }
         state.ignoreEmulatedMouseEvents = true;
-        if (onInteractOutside && state.isPointerDown && isValidEvent(e, ref)) {
+        if (state.onInteractOutside && state.isPointerDown && isValidEvent(e, ref)) {
           state.isPointerDown = false;
-          onInteractOutside(e);
+          state.onInteractOutside(e);
         }
       };
 
@@ -106,7 +102,7 @@ export function useInteractOutside(props: InteractOutsideProps) {
         document.removeEventListener('touchend', onTouchEnd, true);
       };
     }
-  }, [onInteractOutside, ref, state.ignoreEmulatedMouseEvents, state.isPointerDown, isDisabled]);
+  }, [ref, state, isDisabled]);
 }
 
 function isValidEvent(event, ref) {

packages/@react-spectrum/menu/src/MenuTrigger.tsx

@@ -82,7 +82,7 @@ function MenuTrigger(props: SpectrumMenuTriggerProps, ref: DOMRef<HTMLElement>)
   };
 
   let contents = (
-    <FocusScope restoreFocus>
+    <FocusScope restoreFocus contain={isMobile}>
       <DismissButton onDismiss={state.close} />
       {menu}
       <DismissButton onDismiss={state.close} />
@@ -93,7 +93,7 @@ function MenuTrigger(props: SpectrumMenuTriggerProps, ref: DOMRef<HTMLElement>)
   let overlay;
   if (isMobile) {
     overlay = (
-      <Tray isOpen={state.isOpen} onClose={state.close} shouldCloseOnBlur>
+      <Tray isOpen={state.isOpen} onClose={state.close}>
         {contents}
       </Tray>
     );

packages/@react-spectrum/picker/src/Picker.tsx

@@ -117,7 +117,7 @@ function Picker<T extends object>(props: SpectrumPickerProps<T>, ref: DOMRef<HTM
 
   // On small screen devices, the listbox is rendered in a tray, otherwise a popover.
   let listbox = (
-    <FocusScope restoreFocus>
+    <FocusScope restoreFocus contain={isMobile}>
       <DismissButton onDismiss={() => state.close()} />
       <ListBoxBase
         {...menuProps}
@@ -158,7 +158,7 @@ function Picker<T extends object>(props: SpectrumPickerProps<T>, ref: DOMRef<HTM
   let overlay;
   if (isMobile) {
     overlay = (
-      <Tray isOpen={state.isOpen} onClose={state.close} shouldCloseOnBlur>
+      <Tray isOpen={state.isOpen} onClose={state.close}>
         {listbox}
       </Tray>
     );

packages/@react-spectrum/table/test/Table.test.js

@@ -91,7 +91,6 @@ describe('TableView', function () {
   beforeAll(function () {
     offsetWidth = jest.spyOn(window.HTMLElement.prototype, 'clientWidth', 'get').mockImplementation(() => 1000);
     offsetHeight = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementation(() => 1000);
-    jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => cb());
     jest.useFakeTimers();
   });
 
@@ -100,6 +99,10 @@ describe('TableView', function () {
     offsetHeight.mockReset();
   });
 
+  beforeEach(() => {
+    jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => cb());
+  });
+
   afterEach(() => {
     act(() => {jest.runAllTimers();});
   });
@@ -2628,13 +2631,18 @@ describe('TableView', function () {
       let menuItems = within(menu).getAllByRole('menuitem');
       expect(menuItems.length).toBe(2);
 
+      // Need requestAnimationFrame to actually be async for this test to work.
+      jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => setTimeout(cb, 0));
+
       triggerPress(menuItems[1]);
+      act(() => jest.runAllTimers());
       expect(menu).not.toBeInTheDocument();
 
       let dialog = tree.getByRole('alertdialog', {hidden: true});
       let deleteButton = within(dialog).getByRole('button', {hidden: true});
 
       triggerPress(deleteButton);
+      act(() => jest.runAllTimers());
       expect(dialog).not.toBeInTheDocument();
 
       act(() => jest.runAllTimers());
@@ -2828,15 +2836,23 @@ describe('TableView', function () {
       expect(document.activeElement).toBe(within(rows[1]).getByRole('button'));
 
       fireEvent.keyDown(document.activeElement, {key: 'ArrowDown', altKey: true});
+      fireEvent.keyUp(document.activeElement, {key: 'ArrowDown', altKey: true});
 
       let menu = tree.getByRole('menu');
       expect(menu).toBeInTheDocument();
       expect(document.activeElement).toBe(within(menu).getAllByRole('menuitem')[0]);
 
       fireEvent.keyDown(document.activeElement, {key: 'ArrowDown'});
+      fireEvent.keyUp(document.activeElement, {key: 'ArrowDown'});
       expect(document.activeElement).toBe(within(menu).getAllByRole('menuitem')[1]);
 
-      act(() => table.focus());
+      // Need requestAnimationFrame to actually be async for this test to work.
+      jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => setTimeout(cb, 0));
+
+      fireEvent.keyDown(document.activeElement, {key: 'Escape'});
+      fireEvent.keyUp(document.activeElement, {key: 'Escape'});
+
+      act(() => jest.runAllTimers());
 
       expect(menu).not.toBeInTheDocument();
       expect(document.activeElement).toBe(within(rows[1]).getByRole('button'));