Moving default focus behavior from Menu into useSelectableCollection

packages/@react-aria/menu-trigger/src/useMenu.ts

@@ -32,14 +32,20 @@ export function useMenu<T>(props: MenuProps<T>, state: MenuState<T>, layout: Men
     'aria-orientation': ariaOrientation = 'vertical' as Orientation,
     role = 'menu',
     id,
-    selectionMode
+    selectionMode,
+    autoFocus,
+    wrapAround,
+    focusStrategy
   } = props;
 
   let menuId = useId(id);
 
   let {listProps} = useSelectableCollection({
     selectionManager: state.selectionManager,
-    keyboardDelegate: layout
+    keyboardDelegate: layout,
+    autoFocus,
+    focusStrategy,
+    wrapAround
   });
 
   let menuItemRole = 'menuitem';

packages/@react-aria/menu-trigger/src/useMenuTrigger.ts

@@ -38,6 +38,7 @@ export function useMenuTrigger(props: MenuTriggerProps, state: MenuTriggerState)
 
   let onPress = () => {
     if (!isDisabled) {
+      state.setFocusStrategy('first');
       state.setOpen(!state.isOpen);
     }
   };

packages/@react-aria/menu-trigger/test/useMenuTrigger.test.js

@@ -10,31 +10,34 @@
  * governing permissions and limitations under the License.
  */
 
-import React, {createRef} from 'react';
+import React from 'react';
 import {renderHook} from 'react-hooks-testing-library';
 import {useMenuTrigger} from '../';
 
 describe('useMenuTrigger', function () {
   let state = {};
   let setOpen = jest.fn();
-  let focusStrategy = createRef();
+  let setFocusStrategy = jest.fn();
 
-  let renderMenuTriggerHook = (menuProps, menuTriggerProps, isOpen) => {
-    let {result} = renderHook(() => useMenuTrigger(menuProps, menuTriggerProps, isOpen));
+  let renderMenuTriggerHook = (menuTriggerProps, menuTriggerState) => {
+    let {result} = renderHook(() => useMenuTrigger(menuTriggerProps, menuTriggerState));
     return result.current;
   };
 
   beforeEach(() => {
     state.isOpen = false;
     state.setOpen = setOpen;
+    state.focusStrategy = 'first';
+    state.setFocusStrategy = setFocusStrategy;
   });
 
   afterEach(() => {
     setOpen.mockClear();
+    setFocusStrategy = jest.fn();
   });
 
   it('should return default props for menu and menu trigger', function () {
-    let {menuTriggerProps, menuProps} = renderMenuTriggerHook({focusStrategy}, state);
+    let {menuTriggerProps, menuProps} = renderMenuTriggerHook({}, state);
     expect(menuTriggerProps['aria-controls']).toBeFalsy();
     expect(menuTriggerProps['aria-expanded']).toBeFalsy();
     expect(menuTriggerProps['aria-haspopup']).toBeFalsy();
@@ -45,7 +48,7 @@ describe('useMenuTrigger', function () {
   it('should return proper aria props for menu and menu trigger if menu is open', function () {
     state.isOpen = true;
 
-    let {menuTriggerProps, menuProps} = renderMenuTriggerHook({focusStrategy}, state);
+    let {menuTriggerProps, menuProps} = renderMenuTriggerHook({}, state);
     expect(menuTriggerProps['aria-controls']).toBe(menuProps.id);
     expect(menuTriggerProps['aria-expanded']).toBeTruthy();
     expect(menuProps['aria-labelledby']).toBe(menuTriggerProps.id);
@@ -54,8 +57,7 @@ describe('useMenuTrigger', function () {
 
   it('returns the proper aria-haspopup based on the menu\'s type', function () {
     let props = {
-      type: 'menu',
-      focusStrategy
+      type: 'menu'
     };
 
     let {menuTriggerProps} = renderMenuTriggerHook(props, state);
@@ -65,23 +67,23 @@ describe('useMenuTrigger', function () {
   // Comprehensive onPress functionality is tested in MenuTrigger test
   it('returns a onPress for the menuTrigger', function () {
     let props = {
-      type: 'menu',
-      focusStrategy
+      type: 'menu'
     };
 
     let {menuTriggerProps} = renderMenuTriggerHook(props, state);
     expect(typeof menuTriggerProps.onPress).toBe('function');
     menuTriggerProps.onPress();
     expect(setOpen).toHaveBeenCalledTimes(1);
     expect(setOpen).toHaveBeenCalledWith(!state.isOpen);
+    expect(setFocusStrategy).toHaveBeenCalledTimes(1);
+    expect(setFocusStrategy).toHaveBeenCalledWith('first');
   });
 
   // Comprehensive onKeyDown functionality is tested in MenuTrigger test
   it('returns a onKeyDown that toggles the menu open state for specific key strokes', function () {
     let props = {
       type: 'menu',
-      ref: {current: true},
-      focusStrategy
+      ref: {current: true}
     };
 
     let preventDefault = jest.fn();
@@ -94,29 +96,33 @@ describe('useMenuTrigger', function () {
     menuTriggerProps.onKeyDown({
       pointerType: 'not keyboard', 
       isDefaultPrevented: () => true,
-      key: 'ArrowDown'
+      key: 'ArrowUp'
     });
     expect(setOpen).toHaveBeenCalledTimes(0);
+    expect(setFocusStrategy).toHaveBeenCalledTimes(0);
 
     // doesn't trigger event if defaultPrevented is true
     menuTriggerProps.onKeyDown({
       pointerType: 'not keyboard', 
       defaultPrevented: true,
-      key: 'ArrowDown'
+      key: 'ArrowUp'
     });
     expect(setOpen).toHaveBeenCalledTimes(0);
+    expect(setFocusStrategy).toHaveBeenCalledTimes(0);
 
      // triggers event if defaultPrevented is not true and it matches one of the keys
     menuTriggerProps.onKeyDown({
       pointerType: 'not keyboard', 
       defaultPrevented: false,
-      key: 'ArrowDown',
+      key: 'ArrowUp',
       preventDefault,
       stopPropagation
     });
     expect(setOpen).toHaveBeenCalledTimes(1);
     expect(setOpen).toHaveBeenCalledWith(!state.isOpen);
     expect(stopPropagation).toHaveBeenCalledTimes(1);
     expect(preventDefault).toHaveBeenCalledTimes(1);
+    expect(setFocusStrategy).toHaveBeenCalledTimes(2);
+    expect(setFocusStrategy).toHaveBeenLastCalledWith('last');
   });
 });

packages/@react-aria/selection/src/useSelectableCollection.ts

@@ -10,10 +10,12 @@
  * governing permissions and limitations under the License.
  */
 
-import {FocusEvent, HTMLAttributes, KeyboardEvent} from 'react';
+import {FocusEvent, HTMLAttributes, KeyboardEvent, useEffect} from 'react';
 import {KeyboardDelegate} from '@react-types/shared';
 import {MultipleSelectionManager} from '@react-stately/selection';
 
+type FocusStrategy = 'first' | 'last';
+
 const isMac =
   typeof window !== 'undefined' && window.navigator != null
     ? /^Mac/.test(window.navigator.platform)
@@ -29,7 +31,10 @@ function isCtrlKeyPressed(e: KeyboardEvent) {
 
 interface SelectableListOptions {
   selectionManager: MultipleSelectionManager,
-  keyboardDelegate: KeyboardDelegate
+  keyboardDelegate: KeyboardDelegate,
+  autoFocus?: boolean,
+  focusStrategy?: FocusStrategy,
+  wrapAround?: boolean 
 }
 
 interface SelectableListAria {
@@ -39,7 +44,10 @@ interface SelectableListAria {
 export function useSelectableCollection(options: SelectableListOptions): SelectableListAria {
   let {
     selectionManager: manager,
-    keyboardDelegate: delegate
+    keyboardDelegate: delegate,
+    autoFocus = false,
+    focusStrategy,
+    wrapAround = false
   } = options;
 
   let onKeyDown = (e: KeyboardEvent) => {
@@ -50,7 +58,7 @@ export function useSelectableCollection(options: SelectableListOptions): Selecta
           let nextKey = delegate.getKeyBelow(manager.focusedKey);
           if (nextKey) {
             manager.setFocusedKey(nextKey);
-          } else {
+          } else if (wrapAround) {
             manager.setFocusedKey(delegate.getFirstKey());
           }
           if (e.shiftKey && manager.selectionMode === 'multiple') {
@@ -65,7 +73,7 @@ export function useSelectableCollection(options: SelectableListOptions): Selecta
           let nextKey = delegate.getKeyAbove(manager.focusedKey);
           if (nextKey) {
             manager.setFocusedKey(nextKey);
-          } else {
+          } else if (wrapAround) {
             manager.setFocusedKey(delegate.getLastKey());
           }
           if (e.shiftKey && manager.selectionMode === 'multiple') {
@@ -177,6 +185,28 @@ export function useSelectableCollection(options: SelectableListOptions): Selecta
     manager.setFocused(false);
   };
 
+  useEffect(() => {
+    if (autoFocus) {
+      manager.setFocused(true);
+
+      // By default, select first item for focus target
+      let focusedKey = delegate.getFirstKey();
+      let selectedKeys = manager.selectedKeys;
+
+      // Set the last item as the new focus target if focusStrategy is 'last' (i.e. ArrowUp opening the menu)
+      if (focusStrategy && focusStrategy === 'last') {
+        focusedKey = delegate.getLastKey();
+      }
+
+      // If there are any selected keys, make the first one the new focus target
+      if (selectedKeys.size) {
+        focusedKey = selectedKeys.values().next().value;
+      }
+
+      manager.setFocusedKey(focusedKey);
+    }
+  }, []);
+
   return {
     listProps: {
       onKeyDown,

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

@@ -16,7 +16,7 @@ import {Item, ListLayout, Node, Section} from '@react-stately/collections';
 import {MenuContext} from './context';
 import {MenuDivider, MenuHeading, MenuItem} from './';
 import {mergeProps} from '@react-aria/utils';
-import React, {Fragment, useContext, useEffect, useMemo} from 'react';
+import React, {Fragment, useContext, useMemo} from 'react';
 import {SpectrumMenuProps} from '@react-types/menu';
 import styles from '@adobe/spectrum-css-temp/components/menu/vars.css';
 import {useMenu} from '@react-aria/menu-trigger';
@@ -43,43 +43,10 @@ export function Menu<T>(props: SpectrumMenuProps<T>) {
   let {styleProps} = useStyleProps(completeProps);
   let menuContext = mergeProps(menuProps, completeProps);
 
-  let {
-    focusStrategy,
-    setFocusStrategy,
-    autoFocus = true,
-    ...otherProps
-  } = completeProps;
-
-  useEffect(() => {
-    // By default, attempt to focus first item upon opening menu
-    let focusedKey = layout.getFirstKey();
-    let selectionManager = state.selectionManager;
-    let selectedKeys = selectionManager.selectedKeys;
-    selectionManager.setFocused(true);
-
-    // Focus last item if focusStrategy is 'last' (i.e. ArrowUp opening the menu)
-    if (focusStrategy && focusStrategy === 'last') {
-      focusedKey = layout.getLastKey();
-
-      // Reset focus strategy so it doesn't get applied to future menu openings
-      setFocusStrategy('first');
-    }
-
-    // TODO: add other default focus behaviors https://jira.corp.adobe.com/browse/RSP-1399
-    // Focus the first selected key (if any)
-    if (selectedKeys.size) {
-      focusedKey = selectedKeys.values().next().value;
-    }
-
-    if (autoFocus) {
-      selectionManager.setFocusedKey(focusedKey);
-    }
-  }, []);
-
   return (
     <MenuContext.Provider value={menuContext}>
       <CollectionView
-        {...filterDOMProps(otherProps)}
+        {...filterDOMProps(completeProps)}
         {...styleProps}
         {...menuProps}
         focusedKey={state.selectionManager.focusedKey}

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

@@ -70,9 +70,10 @@ export function MenuTrigger(props: SpectrumMenuTriggerProps) {
   let menuContext = {
     ...menuProps,
     focusStrategy,
-    setFocusStrategy,
     onClose,
-    closeOnSelect
+    closeOnSelect,
+    autoFocus: true,
+    wrapAround: true
   };
 
   let triggerProps = {

packages/@react-spectrum/menu-trigger/src/context.ts

@@ -18,7 +18,8 @@ export interface MenuContextValue extends DOMProps {
   onClose?: () => void,
   closeOnSelect?: boolean,
   focusStrategy?: FocusStrategy,
-  setFocusStrategy?: (value: FocusStrategy) => void,
+  wrapAround?: boolean,
+  autoFocus?: boolean
 }
 
 export const MenuContext = React.createContext<MenuContextValue>({});