fix(menu): prevents focus from moving out of the menu unless it is ac…

…tually closed The keydown handler on menu items has an over-eager trigger focus condition. This has been moved to a centralized conditional in a `useEffect` to wait until the menu is truly unexpanded before allowing focus to go back to the menu button.
zendeskgarden · Oct 4, 2023 · 61687b8 · 61687b8
1 parent 207880d
commit 61687b8
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 16 deletions.
diff --git a/packages/menu/.size-snapshot.json b/packages/menu/.size-snapshot.json
@@ -1,13 +1,13 @@
 {
   "index.cjs.js": {
-    "bundled": 26382,
-    "minified": 13587,
-    "gzipped": 3731
+    "bundled": 26587,
+    "minified": 13631,
+    "gzipped": 3764
   },
   "index.esm.js": {
-    "bundled": 25440,
-    "minified": 12644,
-    "gzipped": 3713,
+    "bundled": 25647,
+    "minified": 12689,
+    "gzipped": 3744,
     "treeshaked": {
       "rollup": {
         "code": 386,

diff --git a/packages/menu/src/useMenu.ts b/packages/menu/src/useMenu.ts
@@ -12,6 +12,7 @@ import React, {
   useEffect,
   useMemo,
   useReducer,
+  useRef,
   useState
 } from 'react';
 import { useSelection } from '@zendeskgarden/container-selection';
@@ -92,6 +93,7 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
    */
 
   const [menuVisible, setMenuVisible] = useState<boolean>(false);
+  const focusTriggerRef = useRef<boolean>(false);
 
   const [state, dispatch] = useReducer(stateReducer, {
     focusedValue: focusedValue || defaultFocusedValue,
@@ -316,14 +318,14 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
 
         const type = StateChangeTypes[key === KEYS.ESCAPE ? 'MenuKeyDownEscape' : 'MenuKeyDownTab'];
 
-        closeMenu(type);
-
-        if (triggerRef?.current && KEYS.ESCAPE === key) {
-          triggerRef.current.focus();
+        if (KEYS.ESCAPE === key) {
+          focusTriggerRef.current = true;
         }
+
+        closeMenu(type);
       }
     },
-    [closeMenu, triggerRef]
+    [closeMenu, focusTriggerRef]
   );
 
   const handleMenuBlur = useCallback(
@@ -421,8 +423,8 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
           ...(nextSelection && { selectedItems: nextSelection })
         };
 
-        if (triggerRef?.current && !isTransitionItem) {
-          triggerRef.current.focus();
+        if (!isTransitionItem) {
+          focusTriggerRef.current = true;
         }
       } else if (key === KEYS.RIGHT) {
         if (rtl && isPrevious) {
@@ -480,11 +482,11 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
     },
     [
       rtl,
-      triggerRef,
       state.nestedPathIds,
       isExpandedControlled,
       isFocusedValueControlled,
       isSelectedItemsControlled,
+      focusTriggerRef,
       getNextFocusedValue,
       getSelectedItems,
       onChange
@@ -543,7 +545,12 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
   }, [controlledIsExpanded, handleMenuBlur, handleMenuKeyDown, environment]);
 
   /**
-   * Focus initial item when menu opens or changes due to sub-menu transition
+   * Handles focus depending on menu state:
+   * - When opened, focus the menu via `focusedValue`
+   * - When closed, focus the trigger via `triggerRef`
+   *
+   * This effect is intended to prevent focusing the trigger or menu
+   * unless the menu is in the right expansion state.
    */
   useEffect(() => {
     if (state.focusOnOpen && menuVisible && controlledFocusedValue && controlledIsExpanded) {
@@ -558,13 +565,20 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
 
       ref && ref.focus();
     }
+
+    if (!menuVisible && !controlledIsExpanded && focusTriggerRef.current) {
+      triggerRef?.current?.focus();
+      focusTriggerRef.current = false;
+    }
   }, [
+    focusTriggerRef,
     values,
     menuVisible,
     itemRefs,
     controlledFocusedValue,
     state.focusOnOpen,
-    controlledIsExpanded
+    controlledIsExpanded,
+    triggerRef
   ]);
 
   /**