diff --git a/src/components/PopoverMenu/index.js b/src/components/PopoverMenu/index.js index 4de4a2341283..1cafa9e12664 100644 --- a/src/components/PopoverMenu/index.js +++ b/src/components/PopoverMenu/index.js @@ -41,6 +41,7 @@ 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]; @@ -48,7 +49,6 @@ function PopoverMenu(props) { setSelectedItemIndex(index); }; - const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({initialFocusedIndex: -1, maxIndex: props.menuItems.length - 1}); useKeyboardShortcut( CONST.KEYBOARD_SHORTCUTS.ENTER, () => { diff --git a/src/hooks/useArrowKeyFocusManager.js b/src/hooks/useArrowKeyFocusManager.js index 09c3f03d488b..fdfdf07c99a7 100644 --- a/src/hooks/useArrowKeyFocusManager.js +++ b/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]; diff --git a/src/hooks/useKeyboardShortcut.js b/src/hooks/useKeyboardShortcut.js index 434daeda1921..9b7244176864 100644 --- a/src/hooks/useKeyboardShortcut.js +++ b/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]); }