-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix focus not landing on first item when search query is cleared #84597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a269d7f
b3127a2
f9eef66
1c080bc
c7eac8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,7 @@ function BaseSelectionListWithSections<TItem extends ListItem>({ | |
| const innerTextInputRef = useRef<BaseTextInputRef | null>(null); | ||
| const isTextInputFocusedRef = useRef<boolean>(false); | ||
| const hasKeyBeenPressed = useRef(false); | ||
| const suppressNextFocusScrollRef = useRef(false); | ||
| const activeElementRole = useActiveElementRole(); | ||
| const {isKeyboardShown} = useKeyboardState(); | ||
| const {safeAreaPaddingBottomStyle} = useSafeAreaPaddings(); | ||
|
|
@@ -125,6 +126,10 @@ function BaseSelectionListWithSections<TItem extends ListItem>({ | |
| disabledIndexes, | ||
| isActive: isScreenFocused && itemsCount > 0, | ||
| onFocusedIndexChange: (index: number) => { | ||
| if (suppressNextFocusScrollRef.current) { | ||
| suppressNextFocusScrollRef.current = false; | ||
| return; | ||
| } | ||
| if (!shouldScrollToFocusedIndex) { | ||
| return; | ||
| } | ||
|
|
@@ -183,6 +188,9 @@ function BaseSelectionListWithSections<TItem extends ListItem>({ | |
| }; | ||
|
|
||
| const updateAndScrollToFocusedIndex = (index: number, shouldScroll = true) => { | ||
| if (!shouldScroll) { | ||
| suppressNextFocusScrollRef.current = true; | ||
| } | ||
|
Comment on lines
+191
to
+193
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Setting Useful? React with 👍 / 👎. |
||
| setFocusedIndex(index); | ||
| if (shouldScroll) { | ||
| scrollToIndex(index); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,202 @@ | ||
| import type * as NativeNavigation from '@react-navigation/native'; | ||
| import {act, render, screen, waitFor} from '@testing-library/react-native'; | ||
| import React, {useMemo} from 'react'; | ||
| import Onyx from 'react-native-onyx'; | ||
| import {LocaleContextProvider} from '@components/LocaleContextProvider'; | ||
| import OnyxListItemProvider from '@components/OnyxListItemProvider'; | ||
| import {OptionsListActionsContext, OptionsListStateContext} from '@components/OptionListContextProvider'; | ||
| import SearchRouter from '@components/Search/SearchRouter/SearchRouter'; | ||
| import type {PrivateIsArchivedMap} from '@hooks/usePrivateIsArchivedMap'; | ||
| import {createOptionList} from '@libs/OptionsListUtils'; | ||
| import ComposeProviders from '@src/components/ComposeProviders'; | ||
| import CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import type {PersonalDetails, Report} from '@src/types/onyx'; | ||
| import createCollection from '../utils/collections/createCollection'; | ||
| import createPersonalDetails from '../utils/collections/personalDetails'; | ||
| import {createRandomReport} from '../utils/collections/reports'; | ||
| import * as TestHelper from '../utils/TestHelper'; | ||
| import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; | ||
| import wrapOnyxWithWaitForBatchedUpdates from '../utils/wrapOnyxWithWaitForBatchedUpdates'; | ||
|
|
||
| jest.mock('lodash/debounce', () => | ||
| jest.fn((fn: Record<string, jest.Mock>) => { | ||
| // eslint-disable-next-line no-param-reassign | ||
| fn.cancel = jest.fn(); | ||
| return fn; | ||
| }), | ||
| ); | ||
|
|
||
| jest.mock('@src/libs/Log'); | ||
|
|
||
| jest.mock('@src/libs/API', () => ({ | ||
| write: jest.fn(), | ||
| makeRequestWithSideEffects: jest.fn(), | ||
| read: jest.fn(), | ||
| })); | ||
|
|
||
| // The jest-expo preset resolves to the .native.tsx file which defers rendering via onLayout (which never fires in tests). | ||
| // Mock the deferred wrapper to directly render SearchAutocompleteList. | ||
| jest.mock('@components/Search/DeferredSearchAutocompleteList', () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| const module = jest.requireActual<{default: React.ComponentType}>('@components/Search/SearchAutocompleteList'); | ||
| return { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| __esModule: true, | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| default: module.default, | ||
| }; | ||
| }); | ||
|
|
||
| jest.mock('@src/libs/Navigation/Navigation', () => ({ | ||
| dismissModalWithReport: jest.fn(), | ||
| getTopmostReportId: jest.fn(), | ||
| isNavigationReady: jest.fn(() => Promise.resolve()), | ||
| isDisplayedInModal: jest.fn(() => false), | ||
| navigate: jest.fn(), | ||
| })); | ||
|
|
||
| jest.mock('@src/hooks/useRootNavigationState', () => ({ | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| __esModule: true, | ||
| default: () => ({contextualReportID: undefined, isSearchRouterScreen: false}), | ||
| })); | ||
|
|
||
| jest.mock('@hooks/useExportedToFilterOptions', () => ({ | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| __esModule: true, | ||
| default: () => ({ | ||
| exportedToFilterOptions: [], | ||
| combinedUniqueExportTemplates: [], | ||
| connectedIntegrationNames: new Set<string>(), | ||
| }), | ||
| })); | ||
|
|
||
| jest.mock('@react-navigation/native', () => { | ||
| const actualNav = jest.requireActual<typeof NativeNavigation>('@react-navigation/native'); | ||
| return { | ||
| ...actualNav, | ||
| useFocusEffect: jest.fn(), | ||
| useIsFocused: () => true, | ||
| useRoute: () => jest.fn(), | ||
| usePreventRemove: () => jest.fn(), | ||
| useNavigation: () => ({ | ||
| navigate: jest.fn(), | ||
| addListener: () => jest.fn(), | ||
| }), | ||
| createNavigationContainerRef: () => ({ | ||
| addListener: () => jest.fn(), | ||
| removeListener: () => jest.fn(), | ||
| isReady: () => jest.fn(), | ||
| getCurrentRoute: () => jest.fn(), | ||
| getState: () => jest.fn(), | ||
| }), | ||
| useNavigationState: () => ({ | ||
| routes: [], | ||
| }), | ||
| }; | ||
| }); | ||
|
|
||
| jest.mock('@src/components/ConfirmedRoute.tsx'); | ||
|
|
||
| const getMockedReports = (length = 10) => | ||
| createCollection<Report>( | ||
| (item) => `${ONYXKEYS.COLLECTION.REPORT}${item.reportID}`, | ||
| (index) => createRandomReport(index, undefined), | ||
| length, | ||
| ); | ||
|
|
||
| const getMockedPersonalDetails = (length = 10) => | ||
| createCollection<PersonalDetails>( | ||
| (item) => item.accountID, | ||
| (index) => createPersonalDetails(index), | ||
| length, | ||
| ); | ||
|
|
||
| const MOCK_CURRENT_USER_ACCOUNT_ID = 1; | ||
|
|
||
| const mockedReports = getMockedReports(10); | ||
| const mockedBetas = Object.values(CONST.BETAS); | ||
| const mockedPersonalDetails = getMockedPersonalDetails(10); | ||
| const EMPTY_PRIVATE_IS_ARCHIVED_MAP: PrivateIsArchivedMap = {}; | ||
| const mockedOptions = createOptionList(mockedPersonalDetails, MOCK_CURRENT_USER_ACCOUNT_ID, EMPTY_PRIVATE_IS_ARCHIVED_MAP, mockedReports); | ||
|
|
||
| const mockOnClose = jest.fn(); | ||
|
|
||
| function SearchRouterWrapper() { | ||
| return ( | ||
| <ComposeProviders components={[OnyxListItemProvider, LocaleContextProvider]}> | ||
| <OptionsListStateContext.Provider value={useMemo(() => ({options: mockedOptions, areOptionsInitialized: true}), [])}> | ||
| <OptionsListActionsContext.Provider value={useMemo(() => ({initializeOptions: () => {}, resetOptions: () => {}}), [])}> | ||
| <SearchRouter onRouterClose={mockOnClose} /> | ||
| </OptionsListActionsContext.Provider> | ||
| </OptionsListStateContext.Provider> | ||
| </ComposeProviders> | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Helper to flush all pending React state updates and Onyx callbacks. | ||
| * With fake timers we need multiple rounds of timer advancement + microtask flushing. | ||
| */ | ||
| async function flushAllUpdates() { | ||
| for (let i = 0; i < 10; i++) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await act(async () => { | ||
| jest.advanceTimersByTime(100); | ||
| await waitForBatchedUpdates(); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| describe('SearchAutocompleteList', () => { | ||
| beforeAll(() => { | ||
| Onyx.init({ | ||
| keys: ONYXKEYS, | ||
| evictableKeys: [ONYXKEYS.COLLECTION.REPORT], | ||
| }); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| global.fetch = TestHelper.getGlobalFetchMock(); | ||
| wrapOnyxWithWaitForBatchedUpdates(Onyx); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await act(async () => { | ||
| await Onyx.clear(); | ||
| }); | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('should display Recent searches section when query is empty and recent searches exist', async () => { | ||
| const timestampOne = '2024-01-01T00:00:00'; | ||
| const timestampTwo = '2024-01-02T00:00:00'; | ||
| const recentSearches: Record<string, {query: string; timestamp: string}> = {}; | ||
| recentSearches[timestampOne] = {query: 'type:expense status:approved', timestamp: timestampOne}; | ||
| recentSearches[timestampTwo] = {query: 'type:chat', timestamp: timestampTwo}; | ||
|
|
||
| await waitForBatchedUpdates(); | ||
| await Onyx.multiSet({ | ||
| ...mockedReports, | ||
| [ONYXKEYS.PERSONAL_DETAILS_LIST]: mockedPersonalDetails, | ||
| [ONYXKEYS.BETAS]: mockedBetas, | ||
| [ONYXKEYS.IS_SEARCHING_FOR_REPORTS]: true, | ||
| [ONYXKEYS.RECENT_SEARCHES]: recentSearches, | ||
| }); | ||
|
|
||
| render(<SearchRouterWrapper />); | ||
|
|
||
| // Flush all pending updates (Onyx subscriptions, useEffect, re-renders) | ||
| await flushAllUpdates(); | ||
|
|
||
| // Verify "Recent searches" section header is visible when query is empty and recent searches exist | ||
| await waitFor(() => { | ||
| expect(screen.getByText('Recent searches')).toBeTruthy(); | ||
| }); | ||
|
|
||
| // Verify the recent search items themselves are also displayed | ||
| expect(screen.getByText('type:expense status:approved')).toBeTruthy(); | ||
| expect(screen.getByText('type:chat')).toBeTruthy(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
autocompleteQueryValuebecomes empty, this branch only flipshasSetInitialFocusRefand no longer callsupdateAndScrollToFocusedIndex(..., true), so the list can stay at a previously scrolled position while focus is moved to an item near the top by the initial-focus effect (updateAndScrollToFocusedIndex(flatIndex, false)later in the same component). In the common case where a user scrolls down search results and then clears the query, the highlighted item is off-screen, which makes keyboard focus appear lost again.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flipping
hasSetInitialFocusRefwill trigger the existing initial focus effect which will callupdateAndScrollToFocusedIndex. So, this seems to be a non-issue.