Merge pull request #20756 from gedu/edu/19700_arrow_subscription

19700: Arrow key shortcut stops working

src/components/PopoverMenu/index.js

@@ -41,14 +41,14 @@ const defaultProps = {
 function PopoverMenu(props) {
     const {isSmallScreenWidth} = useWindowDimensions();
     const [selectedItemIndex, setSelectedItemIndex] = useState(null);
+    const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({initialFocusedIndex: -1, maxIndex: props.menuItems.length - 1, isActive: props.isVisible});
 
     const selectItem = (index) => {
         const selectedItem = props.menuItems[index];
         props.onItemSelected(selectedItem);
         setSelectedItemIndex(index);
     };
 
-    const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({initialFocusedIndex: -1, maxIndex: props.menuItems.length - 1});
     useKeyboardShortcut(
         CONST.KEYBOARD_SHORTCUTS.ENTER,
         () => {

src/hooks/useArrowKeyFocusManager.js

@@ -1,71 +1,86 @@
-import {useState, useEffect} from 'react';
+import {useState, useEffect, useCallback, useMemo} from 'react';
 import useKeyboardShortcut from './useKeyboardShortcut';
 import CONST from '../CONST';
 
+// Creating a default array this way because objects ({}) and arrays ([]) are not stable types.
+// The "disabledIndexes" array needs to be stable to prevent the "useCallback" hook from being recreated unnecessarily.
+// Freezing the array ensures that it cannot be unintentionally modified.
+const EMPTY_ARRAY = Object.freeze([]);
 /**
  * A hook that makes it easy to use the arrow keys to manage focus of items in a list
  *
+ * Recommendation: To ensure stability, wrap the `onFocusedIndexChange` function with the useCallback hook before using it with this hook.
+ *
  * @param {Object} config
  * @param {Number} config.maxIndex – typically the number of items in your list
  * @param {Function} [config.onFocusedIndexChange] – optional callback to execute when focusedIndex changes
  * @param {Number} [config.initialFocusedIndex] – where to start in the list
  * @param {Array} [config.disabledIndexes] – An array of indexes to disable + skip over
  * @param {Boolean} [config.shouldExcludeTextAreaNodes] – Whether arrow keys should have any effect when a TextArea node is focused
+ * @param {Boolean} [config.isActive] – Whether the component is ready and should subscribe to KeyboardShortcut
  * @returns {Array}
  */
-export default function useArrowKeyFocusManager({maxIndex, onFocusedIndexChange = () => {}, initialFocusedIndex = 0, disabledIndexes = [], shouldExcludeTextAreaNodes = true}) {
+export default function useArrowKeyFocusManager({
+    maxIndex,
+    onFocusedIndexChange = () => {},
+    initialFocusedIndex = 0,
+    disabledIndexes = EMPTY_ARRAY,
+    shouldExcludeTextAreaNodes = true,
+    isActive,
+}) {
     const [focusedIndex, setFocusedIndex] = useState(initialFocusedIndex);
+    const arrowConfig = useMemo(
+        () => ({
+            excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [],
+            isActive,
+        }),
+        [isActive, shouldExcludeTextAreaNodes],
+    );
+
     useEffect(() => onFocusedIndexChange(focusedIndex), [focusedIndex, onFocusedIndexChange]);
 
-    useKeyboardShortcut(
-        CONST.KEYBOARD_SHORTCUTS.ARROW_UP,
-        () => {
-            if (maxIndex < 0) {
-                return;
-            }
+    const arrowUpCallback = useCallback(() => {
+        if (maxIndex < 0) {
+            return;
+        }
 
-            const currentFocusedIndex = focusedIndex > 0 ? focusedIndex - 1 : maxIndex;
+        setFocusedIndex((actualIndex) => {
+            const currentFocusedIndex = actualIndex > 0 ? actualIndex - 1 : maxIndex;
             let newFocusedIndex = currentFocusedIndex;
 
             while (disabledIndexes.includes(newFocusedIndex)) {
                 newFocusedIndex = newFocusedIndex > 0 ? newFocusedIndex - 1 : maxIndex;
                 if (newFocusedIndex === currentFocusedIndex) {
                     // all indexes are disabled
-                    return; // no-op
+                    return actualIndex; // no-op
                 }
             }
+            return newFocusedIndex;
+        });
+    }, [disabledIndexes, maxIndex]);
+    useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_UP, arrowUpCallback, arrowConfig);
 
-            setFocusedIndex(newFocusedIndex);
-        },
-        {
-            excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [],
-        },
-    );
-
-    useKeyboardShortcut(
-        CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN,
-        () => {
-            if (maxIndex < 0) {
-                return;
-            }
+    const arrowDownCallback = useCallback(() => {
+        if (maxIndex < 0) {
+            return;
+        }
 
-            const currentFocusedIndex = focusedIndex < maxIndex ? focusedIndex + 1 : 0;
+        setFocusedIndex((actualIndex) => {
+            const currentFocusedIndex = actualIndex < maxIndex ? actualIndex + 1 : 0;
             let newFocusedIndex = currentFocusedIndex;
 
             while (disabledIndexes.includes(newFocusedIndex)) {
                 newFocusedIndex = newFocusedIndex < maxIndex ? newFocusedIndex + 1 : 0;
                 if (newFocusedIndex === currentFocusedIndex) {
                     // all indexes are disabled
-                    return; // no-op
+                    return actualIndex;
                 }
             }
 
-            setFocusedIndex(newFocusedIndex);
-        },
-        {
-            excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [],
-        },
-    );
+            return newFocusedIndex;
+        });
+    }, [disabledIndexes, maxIndex]);
+    useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN, arrowDownCallback, arrowConfig);
 
     // Note: you don't need to manually manage focusedIndex in the parent. setFocusedIndex is only exposed in case you want to reset focusedIndex or focus a specific item
     return [focusedIndex, setFocusedIndex];

src/hooks/useKeyboardShortcut.js

@@ -1,20 +1,25 @@
-import {useEffect, useRef, useCallback} from 'react';
+import {useEffect} from 'react';
 import KeyboardShortcut from '../libs/KeyboardShortcut';
 
+// Creating a default array this way because objects ({}) and arrays ([]) are not stable types.
+// The "excludedNodes" array needs to be stable to prevent the "useEffect" hook from being recreated unnecessarily.
+// Freezing the array ensures that it cannot be unintentionally modified.
+const EMPTY_ARRAY = Object.freeze([]);
+
 /**
  * Register a keyboard shortcut handler.
+ * Recommendation: To ensure stability, wrap the `callback` function with the useCallback hook before using it with this hook.
  *
  * @param {Object} shortcut
  * @param {Function} callback
  * @param {Object} [config]
  */
 export default function useKeyboardShortcut(shortcut, callback, config = {}) {
-    const {captureOnInputs = true, shouldBubble = false, priority = 0, shouldPreventDefault = true, excludedNodes = [], isActive = true} = config;
+    const {captureOnInputs = true, shouldBubble = false, priority = 0, shouldPreventDefault = true, excludedNodes = EMPTY_ARRAY, isActive = true} = config;
 
-    const subscription = useRef(null);
-    const subscribe = useCallback(
-        () =>
-            KeyboardShortcut.subscribe(
+    useEffect(() => {
+        if (isActive) {
+            return KeyboardShortcut.subscribe(
                 shortcut.shortcutKey,
                 callback,
                 shortcut.descriptionKey,
@@ -24,14 +29,8 @@ export default function useKeyboardShortcut(shortcut, callback, config = {}) {
                 priority,
                 shouldPreventDefault,
                 excludedNodes,
-            ),
-        [callback, captureOnInputs, excludedNodes, priority, shortcut.descriptionKey, shortcut.modifiers, shortcut.shortcutKey, shouldBubble, shouldPreventDefault],
-    );
-
-    useEffect(() => {
-        const unsubscribe = subscription.current || (() => {});
-        unsubscribe();
-        subscription.current = isActive ? subscribe() : null;
-        return isActive ? subscription.current : () => {};
-    }, [isActive, subscribe]);
+            );
+        }
+        return () => {};
+    }, [isActive, callback, captureOnInputs, excludedNodes, priority, shortcut.descriptionKey, shortcut.modifiers, shortcut.shortcutKey, shouldBubble, shouldPreventDefault]);
 }