diff --git a/src/components/SelectionList/BaseSelectionList.tsx b/src/components/SelectionList/BaseSelectionList.tsx index 38f899f71f65..c2a8e093ec19 100644 --- a/src/components/SelectionList/BaseSelectionList.tsx +++ b/src/components/SelectionList/BaseSelectionList.tsx @@ -18,7 +18,6 @@ import useSingleExecution from '@hooks/useSingleExecution'; import {focusedItemRef} from '@hooks/useSyncFocus/useSyncFocusImplementation'; import useThemeStyles from '@hooks/useThemeStyles'; import {addKeyDownPressListener, removeKeyDownPressListener} from '@libs/KeyboardShortcut/KeyDownPressListener'; -import {isFocusRestoreInProgress} from '@libs/NavigationFocusReturn'; import type {SkeletonSpanReasonAttributes} from '@libs/telemetry/useSkeletonSpan'; import CONST from '@src/CONST'; import getEmptyArray from '@src/types/utils/getEmptyArray'; @@ -114,7 +113,6 @@ function BaseSelectionList({ const listRef = useRef | null>(null); const itemFocusTimeoutRef = useRef(null); const keyboardListenerRef = useRef | null>(null); - const suppressNextFocusScrollRef = useRef(false); const initialFocusedIndex = useMemo(() => data.findIndex((i) => i.keyForList === initiallyFocusedItemKey), [data, initiallyFocusedItemKey]); const [itemsToHighlight, setItemsToHighlight] = useState | null>(null); @@ -219,12 +217,8 @@ function BaseSelectionList({ maxIndex: data.length - 1, disabledIndexes: dataDetails.disabledArrowKeyIndexes, isActive: isFocused, - onFocusedIndexChange: (index: number) => { - if (suppressNextFocusScrollRef.current) { - suppressNextFocusScrollRef.current = false; - return; - } - if (!shouldScrollToFocusedIndex) { + onFocusedIndexChange: (index, shouldScrollHint) => { + if (!shouldScrollHint || !shouldScrollToFocusedIndex) { return; } @@ -238,17 +232,6 @@ function BaseSelectionList({ }, }); - // Keep the cursor on the restored row so keyboard nav continues from there, but don't scroll to it on the way back. - const setFocusedIndexFromRowFocus = useCallback( - (index: number) => { - if (isFocusRestoreInProgress() && index !== focusedIndex) { - suppressNextFocusScrollRef.current = true; - } - setFocusedIndex(index); - }, - [focusedIndex, setFocusedIndex], - ); - // extraData helps FlashList detect when data changes significantly (e.g., during filtering) // Including data.length ensures FlashList resets its layout cache when the list size changes // This prevents "index out of bounds" errors when filtering reduces the list size @@ -271,9 +254,6 @@ function BaseSelectionList({ } } if (shouldUpdateFocusedIndex && typeof indexToFocus === 'number') { - if (indexToFocus !== focusedIndex) { - suppressNextFocusScrollRef.current = true; - } setFocusedIndex(indexToFocus); } onSelectRow(item); @@ -286,7 +266,6 @@ function BaseSelectionList({ isFocused, canSelectMultiple, shouldUpdateFocusedIndex, - focusedIndex, onSelectRow, shouldShowTextInput, shouldClearInputOnSelect, @@ -396,7 +375,7 @@ function BaseSelectionList({ isSelected: selected, ...item, }} - setFocusedIndex={setFocusedIndexFromRowFocus} + setFocusedIndex={setFocusedIndex} index={index} isFocused={isItemFocused} isFocusVisible={isItemVisuallyFocused} @@ -557,10 +536,6 @@ function BaseSelectionList({ setFocusedIndex, }); - const suppressNextFocusScroll = useCallback(() => { - suppressNextFocusScrollRef.current = true; - }, []); - useSearchFocusSync({ searchValue: syncedSearchValue, data, @@ -570,8 +545,6 @@ function BaseSelectionList({ shouldUpdateFocusedIndex, scrollToIndex, setFocusedIndex, - focusedIndex, - suppressNextFocusScroll, }); useEffect(() => { diff --git a/src/components/SelectionList/SelectionListWithSections/BaseSelectionListWithSections.tsx b/src/components/SelectionList/SelectionListWithSections/BaseSelectionListWithSections.tsx index 342ffc10c15a..22f5030d078a 100644 --- a/src/components/SelectionList/SelectionListWithSections/BaseSelectionListWithSections.tsx +++ b/src/components/SelectionList/SelectionListWithSections/BaseSelectionListWithSections.tsx @@ -29,7 +29,6 @@ import {focusedItemRef} from '@hooks/useSyncFocus/useSyncFocusImplementation'; import useThemeStyles from '@hooks/useThemeStyles'; import {addKeyDownPressListener, removeKeyDownPressListener} from '@libs/KeyboardShortcut/KeyDownPressListener'; import Log from '@libs/Log'; -import {isFocusRestoreInProgress} from '@libs/NavigationFocusReturn'; import type {SkeletonSpanReasonAttributes} from '@libs/telemetry/useSkeletonSpan'; import CONST from '@src/CONST'; import type {FlattenedItem, ListItem, SelectionListWithSectionsProps} from './types'; @@ -95,7 +94,6 @@ function BaseSelectionListWithSections({ const isTextInputFocusedRef = useRef(false); const hasKeyBeenPressed = useRef(false); const [isKeyboardNavigating, setIsKeyboardNavigating] = useState(false); - const suppressNextFocusScrollRef = useRef(false); const activeElementRole = useActiveElementRole(); const {isKeyboardShown} = useKeyboardState(); const {safeAreaPaddingBottomStyle} = useSafeAreaPaddings(); @@ -151,9 +149,8 @@ function BaseSelectionListWithSections({ maxIndex: flattenedData.length - 1, disabledIndexes, isActive: isScreenFocused && itemsCount > 0, - onFocusedIndexChange: (index: number) => { - if (suppressNextFocusScrollRef.current) { - suppressNextFocusScrollRef.current = false; + onFocusedIndexChange: (index, shouldScrollHint) => { + if (!shouldScrollHint) { return; } if (!shouldScrollToFocusedIndex) { @@ -170,23 +167,6 @@ function BaseSelectionListWithSections({ }, }); - // Move the cursor, and skip the scroll the move would otherwise trigger when the index actually changes. - const setFocusedIndexWithoutScrollOnChange = (index: number) => { - if (index !== focusedIndex) { - suppressNextFocusScrollRef.current = true; - } - setFocusedIndex(index); - }; - - // Keep the cursor on the restored row so keyboard nav continues from there, but don't scroll to it on the way back. - const setFocusedIndexFromRowFocus = (index: number) => { - if (isFocusRestoreInProgress()) { - setFocusedIndexWithoutScrollOnChange(index); - } else { - setFocusedIndex(index); - } - }; - const getFocusedItem = (): TItem | undefined => { if (focusedIndex < 0 || focusedIndex >= flattenedData.length) { return; @@ -212,7 +192,7 @@ function BaseSelectionListWithSections({ } } if (shouldUpdateFocusedIndex && typeof indexToFocus === 'number') { - setFocusedIndexWithoutScrollOnChange(indexToFocus); + setFocusedIndex(indexToFocus); } onSelectRow(item); @@ -238,9 +218,6 @@ function BaseSelectionListWithSections({ }; const updateAndScrollToFocusedIndex = (index: number, shouldScroll = true) => { - if (!shouldScroll) { - suppressNextFocusScrollRef.current = true; - } setFocusedIndex(index); if (shouldScroll) { scrollToIndex(index); @@ -312,10 +289,6 @@ function BaseSelectionListWithSections({ setFocusedIndex, }); - const suppressNextFocusScroll = () => { - suppressNextFocusScrollRef.current = true; - }; - useSearchFocusSync({ searchValue: syncedSearchValue, data: flattenedData, @@ -325,9 +298,7 @@ function BaseSelectionListWithSections({ shouldUpdateFocusedIndex, scrollToIndex, setFocusedIndex, - focusedIndex, firstFocusableIndex, - suppressNextFocusScroll, }); const textInputComponent = () => { @@ -402,7 +373,7 @@ function BaseSelectionListWithSections({ shouldSingleExecuteRowSelect={shouldSingleExecuteRowSelect} onDismissError={onDismissError} rightHandSideComponent={rightHandSideComponent} - setFocusedIndex={setFocusedIndexFromRowFocus} + setFocusedIndex={setFocusedIndex} singleExecution={singleExecution} shouldSyncFocus={!isTextInputFocusedRef.current && isKeyboardNavigating} shouldIgnoreFocus={shouldIgnoreFocus} diff --git a/src/components/SelectionList/hooks/useSearchFocusSync.ts b/src/components/SelectionList/hooks/useSearchFocusSync.ts index f11157ad3f62..5cd8b1162cf1 100644 --- a/src/components/SelectionList/hooks/useSearchFocusSync.ts +++ b/src/components/SelectionList/hooks/useSearchFocusSync.ts @@ -27,14 +27,8 @@ type UseSearchFocusSyncParams = { /** Function to set the focused index */ setFocusedIndex: (index: number) => void; - /** The current focused index — needed to avoid arming scroll suppression when the index won't actually change */ - focusedIndex?: number; - /** The first focusable index in the list (useful when index 0 is a header). Defaults to 0. */ firstFocusableIndex?: number; - - /** Optional callback to suppress the scroll that onFocusedIndexChange would otherwise trigger when setFocusedIndex is called */ - suppressNextFocusScroll?: () => void; }; /** @@ -53,9 +47,7 @@ function useSearchFocusSync({ shouldUpdateFocusedIndex, scrollToIndex, setFocusedIndex, - focusedIndex, firstFocusableIndex = 0, - suppressNextFocusScroll, }: UseSearchFocusSyncParams) { const prevSearchValue = usePrevious(searchValue); const prevSelectedOptionsCount = usePrevious(selectedOptionsCount); @@ -80,9 +72,6 @@ function useSearchFocusSync({ if (foundSelectedItemIndex !== -1 && !canSelectMultiple) { scrollToIndex(foundSelectedItemIndex, false); - if (foundSelectedItemIndex !== focusedIndex) { - suppressNextFocusScroll?.(); - } setFocusedIndex(foundSelectedItemIndex); return; } @@ -102,9 +91,6 @@ function useSearchFocusSync({ // Scroll to top of list and focus on first focusable item (not header) scrollToIndex(0, false); - if (firstFocusableIndex !== focusedIndex) { - suppressNextFocusScroll?.(); - } setFocusedIndex(firstFocusableIndex); }, [ canSelectMultiple, @@ -118,9 +104,7 @@ function useSearchFocusSync({ shouldUpdateFocusedIndex, searchValue, isItemSelected, - focusedIndex, firstFocusableIndex, - suppressNextFocusScroll, ]); } diff --git a/src/components/SelectionList/hooks/useSelectedItemFocusSync.ts b/src/components/SelectionList/hooks/useSelectedItemFocusSync.ts index 4eb472a9b0c6..1f7e7cc70775 100644 --- a/src/components/SelectionList/hooks/useSelectedItemFocusSync.ts +++ b/src/components/SelectionList/hooks/useSelectedItemFocusSync.ts @@ -18,7 +18,7 @@ type UseSelectedItemFocusSyncParams = { searchValue: string | undefined; /** Function to set the focused index */ - setFocusedIndex: (index: number) => void; + setFocusedIndex: (index: number, shouldScrollHint?: boolean) => void; }; /** @@ -39,7 +39,7 @@ function useSelectedItemFocusSync({ if (selectedItemIndex === -1 || selectedItemIndex === focusedIndex || searchValue) { return; } - setFocusedIndex(selectedItemIndex); + setFocusedIndex(selectedItemIndex, true); // Only sync focus when selectedItemIndex changes, not when other dependencies update // eslint-disable-next-line react-hooks/exhaustive-deps diff --git a/src/hooks/useArrowKeyFocusManager.ts b/src/hooks/useArrowKeyFocusManager.ts index 56e233266464..8ce9a40632d3 100644 --- a/src/hooks/useArrowKeyFocusManager.ts +++ b/src/hooks/useArrowKeyFocusManager.ts @@ -1,11 +1,19 @@ -import {useCallback, useEffect, useMemo, useState} from 'react'; +import {useEffect, useEffectEvent, useState} from 'react'; import CONST from '@src/CONST'; import useKeyboardShortcut from './useKeyboardShortcut'; import usePrevious from './usePrevious'; type Config = { maxIndex: number; - onFocusedIndexChange?: (index: number) => void; + /** + * Optional callback fired whenever `focusedIndex` changes. The hook itself never + * scrolls — any scroll reaction must live in this callback. `shouldScrollHint` is a + * hint forwarded from whoever wrote the new index; it is only ever set in the same + * commit that writes a new `index`, and is never updated on its own. If you need to + * scroll without changing the focused index, call the underlying list's + * `scrollToIndex` directly rather than going through this hook. + */ + onFocusedIndexChange?: (index: number, shouldScrollHint: boolean) => void; initialFocusedIndex?: number; disabledIndexes?: readonly number[]; captureOnInputs?: boolean; @@ -20,12 +28,15 @@ type Config = { onArrowUpDownCallback?: () => void; }; -type UseArrowKeyFocusManager = [number, (index: number) => void]; +/** + * The setter's optional `shouldScrollHint` flag (default `false`) is forwarded as the + * second argument to `onFocusedIndexChange`; it is not a direct scroll control. + * Same-index writes are dropped — see the comment on `setFocusedIndexExternal` for why. + */ +type UseArrowKeyFocusManager = [number, (index: number, shouldScrollHint?: boolean) => void]; /** - * 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. + * A hook that makes it easy to use the arrow keys to manage focus of items in a list. * * @param config.maxIndex – typically the number of items in your list * @param [config.onFocusedIndexChange] – optional callback to execute when focusedIndex changes @@ -57,41 +68,53 @@ export default function useArrowKeyFocusManager({ setHasKeyBeenPressed, onArrowUpDownCallback = () => {}, }: Config): UseArrowKeyFocusManager { - const [focusedIndex, setFocusedIndex] = useState(initialFocusedIndex); + const [focusState, setFocusState] = useState({index: initialFocusedIndex, shouldScrollHint: false}); + const {index: focusedIndex, shouldScrollHint} = focusState; const prevIsFocusedIndex = usePrevious(focusedIndex); - const arrowConfig = useMemo( - () => ({ - excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [], - isActive, - captureOnInputs, - }), - [captureOnInputs, isActive, shouldExcludeTextAreaNodes], - ); - - const horizontalArrowConfig = useMemo( - () => ({ - excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [], - isActive: isActive && allowHorizontalArrowKeys, - captureOnInputs, - }), - [allowHorizontalArrowKeys, captureOnInputs, isActive, shouldExcludeTextAreaNodes], - ); + + /* + * Same-index bail-out example path: after an arrow-key commit, `useSyncFocus`'s + * layout effect (`src/hooks/useSyncFocus/useSyncFocusImplementation.ts`) calls + * `el.focus()`, the browser fires a synchronous `focus` event, and the `onFocus` + * handler in `src/components/SelectionList/ListItem/ListItemRenderer.tsx` re-enters + * this setter with the same index and no hint. + */ + const setFocusedIndexExternal = (index: number, shouldScrollHintArg = false) => { + setFocusState((prev) => (prev.index === index ? prev : {index, shouldScrollHint: shouldScrollHintArg})); + }; + + const setFocusedIndexInternal = (updater: (prev: number) => number) => { + setFocusState((prev) => ({index: updater(prev.index), shouldScrollHint: true})); + }; + + const arrowConfig = { + excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [], + isActive, + captureOnInputs, + }; + + const horizontalArrowConfig = { + excludedNodes: shouldExcludeTextAreaNodes ? ['TEXTAREA'] : [], + isActive: isActive && allowHorizontalArrowKeys, + captureOnInputs, + }; + + const onFocusedIndexChangeEvent = useEffectEvent(onFocusedIndexChange); useEffect(() => { if (prevIsFocusedIndex === focusedIndex) { return; } - onFocusedIndexChange(focusedIndex); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [focusedIndex, prevIsFocusedIndex]); + onFocusedIndexChangeEvent(focusedIndex, shouldScrollHint); + }, [focusedIndex, prevIsFocusedIndex, shouldScrollHint]); - const arrowUpCallback = useCallback(() => { + const arrowUpCallback = () => { if (maxIndex < 0 || !isFocused) { return; } const nextIndex = disableCyclicTraversal ? -1 : maxIndex; setHasKeyBeenPressed?.(); - setFocusedIndex((actualIndex) => { + setFocusedIndexInternal((actualIndex) => { const currentFocusedIndex = actualIndex > 0 ? actualIndex - (itemsPerRow ?? 1) : nextIndex; let newFocusedIndex = currentFocusedIndex; @@ -114,11 +137,11 @@ export default function useArrowKeyFocusManager({ return newFocusedIndex; }); onArrowUpDownCallback(); - }, [maxIndex, isFocused, disableCyclicTraversal, itemsPerRow, disabledIndexes, allowNegativeIndexes, setHasKeyBeenPressed, onArrowUpDownCallback]); + }; useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_UP, arrowUpCallback, arrowConfig); - const arrowDownCallback = useCallback(() => { + const arrowDownCallback = () => { if (maxIndex < 0 || !isFocused) { return; } @@ -126,7 +149,7 @@ export default function useArrowKeyFocusManager({ const nextIndex = disableCyclicTraversal ? maxIndex : 0; - setFocusedIndex((actualIndex) => { + setFocusedIndexInternal((actualIndex) => { let currentFocusedIndex = -1; if (actualIndex === -1) { @@ -161,18 +184,18 @@ export default function useArrowKeyFocusManager({ return newFocusedIndex; }); onArrowUpDownCallback(); - }, [disableCyclicTraversal, disabledIndexes, isFocused, itemsPerRow, maxIndex, setHasKeyBeenPressed, onArrowUpDownCallback]); + }; useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN, arrowDownCallback, arrowConfig); - const arrowLeftCallback = useCallback(() => { + const arrowLeftCallback = () => { if (maxIndex < 0 || !allowHorizontalArrowKeys) { return; } const nextIndex = disableCyclicTraversal ? -1 : maxIndex; - setFocusedIndex((actualIndex) => { + setFocusedIndexInternal((actualIndex) => { const currentFocusedIndex = actualIndex > 0 ? actualIndex - 1 : nextIndex; let newFocusedIndex = currentFocusedIndex; @@ -187,18 +210,18 @@ export default function useArrowKeyFocusManager({ } return newFocusedIndex; }); - }, [allowHorizontalArrowKeys, disableCyclicTraversal, disabledIndexes, maxIndex]); + }; useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_LEFT, arrowLeftCallback, horizontalArrowConfig); - const arrowRightCallback = useCallback(() => { + const arrowRightCallback = () => { if (maxIndex < 0 || !allowHorizontalArrowKeys) { return; } const nextIndex = disableCyclicTraversal ? maxIndex : 0; - setFocusedIndex((actualIndex) => { + setFocusedIndexInternal((actualIndex) => { const currentFocusedIndex = actualIndex < maxIndex ? actualIndex + 1 : nextIndex; let newFocusedIndex = currentFocusedIndex; @@ -213,10 +236,10 @@ export default function useArrowKeyFocusManager({ } return newFocusedIndex; }); - }, [allowHorizontalArrowKeys, disableCyclicTraversal, disabledIndexes, maxIndex]); + }; useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_RIGHT, arrowRightCallback, horizontalArrowConfig); - // 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]; + // Note: you don't need to manually manage focusedIndex in the parent. setFocusedIndexExternal is only exposed in case you want to reset focusedIndex or focus a specific item + return [focusedIndex, setFocusedIndexExternal]; } diff --git a/tests/unit/BaseSelectionListTest.tsx b/tests/unit/BaseSelectionListTest.tsx index badcdf40013d..f2fd70f4baf7 100644 --- a/tests/unit/BaseSelectionListTest.tsx +++ b/tests/unit/BaseSelectionListTest.tsx @@ -153,14 +153,14 @@ describe('BaseSelectionList', () => { } it('should not trigger item press if screen is not focused', () => { - (NativeNavigation.useIsFocused as jest.Mock).mockReturnValue(false); + jest.mocked(NativeNavigation.useIsFocused).mockReturnValue(false); render(); fireEvent.press(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}1`)); expect(onSelectRowMock).toHaveBeenCalledTimes(0); }); it('should handle item press correctly', () => { - (NativeNavigation.useIsFocused as jest.Mock).mockReturnValue(true); + jest.mocked(NativeNavigation.useIsFocused).mockReturnValue(true); render(); fireEvent.press(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}1`)); @@ -172,7 +172,7 @@ describe('BaseSelectionList', () => { }); it('should update selected item on rerender', () => { - (NativeNavigation.useIsFocused as jest.Mock).mockReturnValue(true); + jest.mocked(NativeNavigation.useIsFocused).mockReturnValue(true); const updatedMockItems = mockItems.map((item) => ({ ...item, isSelected: item.keyForList === '2', @@ -324,8 +324,8 @@ describe('BaseSelectionList', () => { expect(screen.queryByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}0`)).toBeNull(); }); - it('suppresses the scroll on a focus-return restore but still syncs the cursor (no scroll-jump, no stale focusedIndex)', () => { - (NativeNavigation.useIsFocused as jest.Mock).mockReturnValue(true); + it('suppresses the scroll on a focus-return restore', () => { + jest.mocked(NativeNavigation.useIsFocused).mockReturnValue(true); mockIsFocusRestoreInProgress.mockReturnValue(true); render(); mockScrollToIndex.mockClear(); @@ -334,16 +334,10 @@ describe('BaseSelectionList', () => { fireEvent(row, 'focus', {nativeEvent: {sourceCapabilities: null}}); expect(mockScrollToIndex).not.toHaveBeenCalled(); - - // Cursor still moved: a later non-restore focus on another row scrolls, so focusedIndex wasn't left stale. - mockIsFocusRestoreInProgress.mockReturnValue(false); - const otherRow = screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}7`); - fireEvent(otherRow, 'focus', {nativeEvent: {sourceCapabilities: {firesTouchEvents: false}}}); - expect(mockScrollToIndex).toHaveBeenCalledWith(expect.objectContaining({index: 7})); }); - it('still syncs the cursor on genuine keyboard Tab focus (no sourceCapabilities, restore NOT in progress) — regression guard for Codex P2', () => { - (NativeNavigation.useIsFocused as jest.Mock).mockReturnValue(true); + it('does not auto-scroll on genuine keyboard Tab focus (programmatic focus is non-scrolling)', () => { + jest.mocked(NativeNavigation.useIsFocused).mockReturnValue(true); mockIsFocusRestoreInProgress.mockReturnValue(false); render(); mockScrollToIndex.mockClear(); @@ -351,11 +345,11 @@ describe('BaseSelectionList', () => { const row = screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}5`); fireEvent(row, 'focus', {nativeEvent: {sourceCapabilities: null}}); - expect(mockScrollToIndex).toHaveBeenCalledWith(expect.objectContaining({index: 5})); + expect(mockScrollToIndex).not.toHaveBeenCalled(); }); - it('still scrolls to a row on genuine pointer focus (sourceCapabilities present)', () => { - (NativeNavigation.useIsFocused as jest.Mock).mockReturnValue(true); + it('does not auto-scroll on genuine pointer focus (jump-on-click prevented)', () => { + jest.mocked(NativeNavigation.useIsFocused).mockReturnValue(true); mockIsFocusRestoreInProgress.mockReturnValue(false); render(); mockScrollToIndex.mockClear(); @@ -363,6 +357,21 @@ describe('BaseSelectionList', () => { const row = screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}5`); fireEvent(row, 'focus', {nativeEvent: {sourceCapabilities: {firesTouchEvents: false}}}); - expect(mockScrollToIndex).toHaveBeenCalledWith(expect.objectContaining({index: 5})); + expect(mockScrollToIndex).not.toHaveBeenCalled(); + }); + + it('restore-mode suppression does not leak into the next focus event', () => { + jest.mocked(NativeNavigation.useIsFocused).mockReturnValue(true); + mockIsFocusRestoreInProgress.mockReturnValue(true); + render(); + + fireEvent(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}5`), 'focus', {nativeEvent: {sourceCapabilities: null}}); + + mockScrollToIndex.mockClear(); + mockIsFocusRestoreInProgress.mockReturnValue(false); + + fireEvent(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}7`), 'focus', {nativeEvent: {sourceCapabilities: {firesTouchEvents: false}}}); + + expect(mockScrollToIndex).not.toHaveBeenCalled(); }); }); diff --git a/tests/unit/useArrowKeyFocusManagerTest.ts b/tests/unit/useArrowKeyFocusManagerTest.ts new file mode 100644 index 000000000000..0be953fd8cc3 --- /dev/null +++ b/tests/unit/useArrowKeyFocusManagerTest.ts @@ -0,0 +1,176 @@ +import {act, renderHook} from '@testing-library/react-native'; +import useArrowKeyFocusManager from '@hooks/useArrowKeyFocusManager'; + +type ShortcutCallback = () => void; +type ShortcutConfig = {isActive?: boolean}; + +const shortcuts: Record = {}; + +jest.mock('@hooks/useKeyboardShortcut', () => (key: {shortcutKey: string}, callback: ShortcutCallback, config?: ShortcutConfig) => { + shortcuts[key.shortcutKey] = {callback, isActive: config?.isActive ?? true}; +}); + +function pressArrowDown() { + act(() => { + if (!shortcuts.ArrowDown?.isActive) { + return; + } + shortcuts.ArrowDown.callback(); + }); +} + +describe('useArrowKeyFocusManager', () => { + afterEach(() => { + for (const key of Object.keys(shortcuts)) { + delete shortcuts[key]; + } + }); + + it('defaults shouldScrollHint to false when the setter is called without the optional flag', () => { + const onFocusedIndexChange = jest.fn(); + const {result} = renderHook(() => + useArrowKeyFocusManager({ + maxIndex: 5, + initialFocusedIndex: 0, + onFocusedIndexChange, + }), + ); + + act(() => { + result.current[1](2); + }); + + expect(onFocusedIndexChange).toHaveBeenCalledTimes(1); + expect(onFocusedIndexChange).toHaveBeenCalledWith(2, false); + }); + + it('forwards shouldScrollHint: true through to onFocusedIndexChange', () => { + const onFocusedIndexChange = jest.fn(); + const {result} = renderHook(() => + useArrowKeyFocusManager({ + maxIndex: 5, + initialFocusedIndex: 0, + onFocusedIndexChange, + }), + ); + + act(() => { + result.current[1](2, true); + }); + + expect(onFocusedIndexChange).toHaveBeenCalledTimes(1); + expect(onFocusedIndexChange).toHaveBeenCalledWith(2, true); + }); + + it('forwards shouldScrollHint: false through to onFocusedIndexChange', () => { + const onFocusedIndexChange = jest.fn(); + const {result} = renderHook(() => + useArrowKeyFocusManager({ + maxIndex: 5, + initialFocusedIndex: 0, + onFocusedIndexChange, + }), + ); + + act(() => { + result.current[1](2, false); + }); + + expect(onFocusedIndexChange).toHaveBeenCalledTimes(1); + expect(onFocusedIndexChange).toHaveBeenCalledWith(2, false); + }); + + it('arrow key navigation forwards shouldScrollHint: true', () => { + const onFocusedIndexChange = jest.fn(); + renderHook(() => + useArrowKeyFocusManager({ + maxIndex: 5, + initialFocusedIndex: 0, + onFocusedIndexChange, + isActive: true, + }), + ); + + pressArrowDown(); + + expect(onFocusedIndexChange).toHaveBeenCalledTimes(1); + expect(onFocusedIndexChange).toHaveBeenCalledWith(1, true); + }); + + it('arrow keys override a previous shouldScrollHint: false from setFocusedIndex', () => { + const onFocusedIndexChange = jest.fn(); + const {result} = renderHook(() => + useArrowKeyFocusManager({ + maxIndex: 5, + initialFocusedIndex: 0, + onFocusedIndexChange, + isActive: true, + }), + ); + + // Public setter parks the hint at false (e.g., focus-driven update). + act(() => { + result.current[1](2, false); + }); + expect(onFocusedIndexChange).toHaveBeenLastCalledWith(2, false); + + // Arrow key must reassert shouldScrollHint: true; otherwise a focus event + // before an arrow press would silently suppress the next scroll. + pressArrowDown(); + expect(onFocusedIndexChange).toHaveBeenLastCalledWith(3, true); + }); + + it('forwards independent shouldScrollHint values on successive calls', () => { + const onFocusedIndexChange = jest.fn(); + const {result} = renderHook(() => + useArrowKeyFocusManager({ + maxIndex: 5, + initialFocusedIndex: 0, + onFocusedIndexChange, + }), + ); + + act(() => { + result.current[1](2, false); + }); + expect(onFocusedIndexChange).toHaveBeenLastCalledWith(2, false); + + act(() => { + result.current[1](3, true); + }); + expect(onFocusedIndexChange).toHaveBeenLastCalledWith(3, true); + + act(() => { + result.current[1](4, false); + }); + expect(onFocusedIndexChange).toHaveBeenLastCalledWith(4, false); + + expect(onFocusedIndexChange).toHaveBeenCalledTimes(3); + }); + + it('bails out on same-index external writes and preserves the previously-set hint', () => { + // Arrow key sets index=1 with shouldScrollHint=true. Then a focus-event-style + // external write hits with the same index and no hint. The bail-out must prevent + // a second onFocusedIndexChange invocation, so the consumer's scroll for the + // arrow-key transition is not clobbered. + const onFocusedIndexChange = jest.fn(); + const {result} = renderHook(() => + useArrowKeyFocusManager({ + maxIndex: 5, + initialFocusedIndex: 0, + onFocusedIndexChange, + isActive: true, + }), + ); + + pressArrowDown(); + expect(onFocusedIndexChange).toHaveBeenCalledTimes(1); + expect(onFocusedIndexChange).toHaveBeenLastCalledWith(1, true); + + act(() => { + result.current[1](1, false); + }); + + expect(onFocusedIndexChange).toHaveBeenCalledTimes(1); + }); +}); diff --git a/tests/unit/useSelectedItemFocusSyncTest.ts b/tests/unit/useSelectedItemFocusSyncTest.ts new file mode 100644 index 000000000000..c8a31a9f168a --- /dev/null +++ b/tests/unit/useSelectedItemFocusSyncTest.ts @@ -0,0 +1,72 @@ +import {renderHook} from '@testing-library/react-native'; +import useSelectedItemFocusSync from '@components/SelectionList/hooks/useSelectedItemFocusSync'; + +type Item = {keyForList: string; text: string; isSelected: boolean}; + +const isItemSelected = (item: Item) => item.isSelected; + +const buildItems = (length: number, selectedIndex: number): Item[] => + Array.from({length}, (_, i) => ({ + keyForList: `item-${i}`, + text: `Item ${i}`, + isSelected: i === selectedIndex, + })); + +describe('useSelectedItemFocusSync', () => { + it('passes shouldScrollHint: true when the selected item becomes known after mount', () => { + const setFocusedIndex = jest.fn(); + // Mount with the selected key known but data not yet loaded. + // The memo returns -1, so the effect bails on first render. + const initialProps = { + data: [] as Item[], + initiallyFocusedItemKey: 'item-5' as string | undefined, + isItemSelected, + focusedIndex: 0, + searchValue: undefined as string | undefined, + setFocusedIndex, + }; + const {rerender} = renderHook((props: typeof initialProps) => useSelectedItemFocusSync(props), {initialProps}); + + expect(setFocusedIndex).not.toHaveBeenCalled(); + + // Async data arrives: the selected row is now at index 5. + // Regression guard for PR #91605: the sync path must pass shouldScrollHint: true + // so the consumer scrolls — FlashList's initialScrollIndex has already been consumed. + rerender({...initialProps, data: buildItems(10, 5)}); + + expect(setFocusedIndex).toHaveBeenCalledTimes(1); + expect(setFocusedIndex).toHaveBeenCalledWith(5, true); + }); + + it('does not call setFocusedIndex when the cursor is already on the selected item', () => { + const setFocusedIndex = jest.fn(); + renderHook(() => + useSelectedItemFocusSync({ + data: buildItems(10, 3), + initiallyFocusedItemKey: 'item-3', + isItemSelected, + focusedIndex: 3, + searchValue: undefined, + setFocusedIndex, + }), + ); + + expect(setFocusedIndex).not.toHaveBeenCalled(); + }); + + it('does not call setFocusedIndex while a search is active', () => { + const setFocusedIndex = jest.fn(); + renderHook(() => + useSelectedItemFocusSync({ + data: buildItems(10, 5), + initiallyFocusedItemKey: 'item-5', + isItemSelected, + focusedIndex: 0, + searchValue: 'item', + setFocusedIndex, + }), + ); + + expect(setFocusedIndex).not.toHaveBeenCalled(); + }); +});