fix: Pressing Enter key to trigger Menu item links (#8721)

Author: devongovett

packages/@react-aria/menu/src/useMenuItem.ts

@@ -111,7 +111,7 @@ export function useMenuItem<T>(props: AriaMenuItemProps, state: TreeState<T>, re
     closeOnSelect,
     isVirtualized,
     'aria-haspopup': hasPopup,
-    onPressStart: pressStartProp,
+    onPressStart,
     onPressUp: pressUpProp,
     onPress,
     onPressChange: pressChangeProp,
@@ -188,21 +188,18 @@ export function useMenuItem<T>(props: AriaMenuItemProps, state: TreeState<T>, re
     ariaProps['aria-setsize'] = getItemCount(state.collection);
   }
 
-  let onPressStart = (e: PressEvent) => {
-    // Trigger native click event on keydown unless this is a link (the browser will trigger onClick then).
-    if (e.pointerType === 'keyboard' && !selectionManager.isLink(key)) {
-      (e.target as HTMLElement).click();
-    }
-
-    pressStartProp?.(e);
-  };
   let isPressedRef = useRef(false);
   let onPressChange = (isPressed: boolean) => {
     pressChangeProp?.(isPressed);
     isPressedRef.current = isPressed;
   };
 
+  let interaction = useRef<{pointerType: string, key?: string} | null>(null);
   let onPressUp = (e: PressEvent) => {
+    if (e.pointerType !== 'keyboard') {
+      interaction.current = {pointerType: e.pointerType};
+    }
+
     // If interacting with mouse, allow the user to mouse down on the trigger button,
     // drag, and release over an item (matching native behavior).
     if (e.pointerType === 'mouse') {
@@ -211,19 +208,26 @@ export function useMenuItem<T>(props: AriaMenuItemProps, state: TreeState<T>, re
       }
     }
 
-    // Pressing a menu item should close by default in single selection mode but not multiple
-    // selection mode, except if overridden by the closeOnSelect prop.
-    if (e.pointerType !== 'keyboard' && !isTrigger && onClose && (closeOnSelect ?? (selectionManager.selectionMode !== 'multiple' || selectionManager.isLink(key)))) {
-      onClose();
-    }
-
     pressUpProp?.(e);
   };
 
   let onClick = (e: MouseEvent<FocusableElement>) => {
     onClickProp?.(e);
     performAction();
     handleLinkClick(e, router, item!.props.href, item?.props.routerOptions);
+
+    let shouldClose = interaction.current?.pointerType === 'keyboard'
+      // Always close when pressing Enter key, or if item is not selectable.
+      ? interaction.current?.key === 'Enter' || selectionManager.selectionMode === 'none' || selectionManager.isLink(key)
+      // Close except if multi-select is enabled.
+      : selectionManager.selectionMode !== 'multiple' || selectionManager.isLink(key);
+    
+    shouldClose = closeOnSelect ?? shouldClose;
+    if (onClose && !isTrigger && shouldClose) {
+      onClose();
+    }
+
+    interaction.current = null;
   };
 
   let {itemProps, isFocused} = useSelectableItem({
@@ -274,14 +278,15 @@ export function useMenuItem<T>(props: AriaMenuItemProps, state: TreeState<T>, re
 
       switch (e.key) {
         case ' ':
-          if (!isDisabled && selectionManager.selectionMode === 'none' && !isTrigger && closeOnSelect !== false && onClose) {
-            onClose();
-          }
+          interaction.current = {pointerType: 'keyboard', key: ' '};
+          (e.target as HTMLElement).click();
           break;
         case 'Enter':
-          // The Enter key should always close on select, except if overridden.
-          if (!isDisabled && closeOnSelect !== false && !isTrigger && onClose) {
-            onClose();
+          interaction.current = {pointerType: 'keyboard', key: 'Enter'};
+
+          // Trigger click unless this is a link. Links trigger click natively.
+          if ((e.target as HTMLElement).tagName !== 'A') {
+            (e.target as HTMLElement).click();
           }
           break;
         default:

packages/@react-spectrum/menu/test/Menu.test.js

@@ -12,8 +12,9 @@
 
 import {act, fireEvent, mockClickDefault, pointerMap, render, within} from '@react-spectrum/test-utils-internal';
 import Bell from '@spectrum-icons/workflow/Bell';
+import {Button} from '@react-spectrum/button';
 import {Dialog, DialogTrigger} from '@react-spectrum/dialog';
-import {Item, Menu, Section} from '../';
+import {Item, Menu, MenuTrigger, Section} from '../';
 import {Keyboard, Text} from '@react-spectrum/text';
 import {MenuContext} from '../src/context';
 import {Provider} from '@react-spectrum/provider';
@@ -789,20 +790,31 @@ describe('Menu', function () {
   });
 
   describe('supports links', function () {
-    describe.each(['mouse', 'keyboard'])('%s', (type) => {
+    describe.each(['mouse', 'Enter', 'Space'])('%s', (type) => {
       it.each(['none', 'single', 'multiple'])('with selectionMode = %s', async function (selectionMode) {
         let user = userEvent.setup({delay: null, pointerMap});
         let onAction = jest.fn();
         let onSelectionChange = jest.fn();
         let tree = render(
           <Provider theme={theme}>
-            <Menu aria-label="menu" selectionMode={selectionMode} onSelectionChange={onSelectionChange} onAction={onAction}>
-              <Item href="https://google.com">One</Item>
-              <Item href="https://adobe.com">Two</Item>
-            </Menu>
+            <MenuTrigger>
+              <Button>Button</Button>
+              <Menu aria-label="menu" selectionMode={selectionMode} onSelectionChange={onSelectionChange} onAction={onAction}>
+                <Item href="https://google.com">One</Item>
+                <Item href="https://adobe.com">Two</Item>
+              </Menu>
+            </MenuTrigger>
           </Provider>
         );
 
+        let button = tree.getByRole('button');
+        if (type === 'mouse') {
+          await user.click(button);
+        } else {
+          await user.tab();
+          await user.keyboard('{Enter}');
+        }
+
         let role = {
           none: 'menuitem',
           single: 'menuitemradio',
@@ -820,15 +832,43 @@ describe('Menu', function () {
         if (type === 'mouse') {
           await user.click(items[1]);
         } else {
-          fireEvent.keyDown(items[1], {key: 'Enter'});
+          fireEvent.keyDown(items[1], {key: type});
           fireEvent.click(items[1]);
-          fireEvent.keyUp(items[1], {key: 'Enter'});
+          fireEvent.keyUp(items[1], {key: type});
         }
         expect(onAction).toHaveBeenCalledTimes(1);
         expect(onSelectionChange).not.toHaveBeenCalled();
         expect(onClick).toHaveBeenCalledTimes(1);
         window.removeEventListener('click', onClick);
       });
     });
+
+    it('should support dragging and releasing', async () => {
+      let user = userEvent.setup({delay: null, pointerMap});
+      let onAction = jest.fn();
+      let tree = render(
+        <Provider theme={theme}>
+          <MenuTrigger>
+            <Button>Button</Button>
+            <Menu aria-label="menu" onAction={onAction}>
+              <Item href="https://google.com">One</Item>
+              <Item href="https://adobe.com">Two</Item>
+            </Menu>
+          </MenuTrigger>
+        </Provider>
+      );
+
+      let button = tree.getByRole('button');
+      await user.pointer({target: button, keys: '[MouseLeft>]'});
+
+      let items = tree.getAllByRole('menuitem');
+      let onClick = mockClickDefault({capture: true});
+
+      await user.pointer({target: items[0], keys: '[/MouseLeft]'});
+
+      expect(onAction).toHaveBeenCalledTimes(1);
+      expect(onClick).toHaveBeenCalledTimes(1);
+      window.removeEventListener('click', onClick);
+    });
   });
 });