Fix focus not landing on first item when search query is cleared#84597
Fix focus not landing on first item when search query is cleared#84597marcochavezf merged 5 commits intomainfrom
Conversation
When the search query is cleared, the query-change effect was setting focusedIndex to 0, which landed on a disabled section header instead of the first focusable item. Reset hasSetInitialFocusRef so the initial focus effect can re-fire with its header-skipping logic. Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
The failing Analysis: The Evidence: The error logs show only Fix: Please re-run the failed |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@MelvinBot I just tested and the |
Verify that the "Recent searches" section header appears when the search query is empty and recent searches exist in Onyx. Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Added a test case in The test:
Pushed in b3127a2. |
- Type jest.requireActual with generic parameter to avoid no-unsafe-assignment and no-unsafe-member-access errors - Use computed property names for timestamp keys to satisfy naming-convention rule Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Fixed the failing ESLint check. The test file had 5 lint errors:
|
|
@MelvinBot Sorry. I just verified that the |
|
@MelvinBot Please respond to the comment here |
|
I traced the full rendering pipeline from sections data through to the UI. Here's what I found: The data path is structurally correct:
So why might items not appear visually? The most likely explanation is FlashList virtualization. FlashList only renders items that are in or near the visible viewport. The component has Could you clarify what you're observing?
This would help me determine whether it's a FlashList viewport/measurement issue or something deeper in the rendering logic. |
|
@MelvinBot This is via visual inspection of UI in web chrome. The entire section (header + items) missing from the visible area for |
When updateAndScrollToFocusedIndex was called with shouldScroll=false, the direct scrollToIndex call was skipped but the onFocusedIndexChange callback in useArrowKeyFocusManager still triggered a scroll. This caused the list to scroll to the focused item (first recent chat), hiding the "Recent searches" section and "Recent chats" header above the viewport. Add a suppressNextFocusScrollRef that prevents the onFocusedIndexChange callback from scrolling when shouldScroll=false is explicitly passed. Also verify recent search items (not just section header) in test. Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Thanks for the clarification — I found the root cause and pushed a fix in 1c080bc. Root cause: When Fix: Added a Verified in browser: After clearing the query, "Recent searches" section (header + items) is now visible at the top, "Recent chats" header is visible, and focus is correctly on the first recent chat item. Also updated the test to verify that recent search items themselves (not just the section header) are rendered. |
- Add eslint-disable comments for @typescript-eslint/no-unsafe-assignment on jest.requireActual which returns any - Avoid @typescript-eslint/naming-convention violation by building recentSearches object imperatively instead of using timestamp literals as object literal property keys Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Fixed the failing ESLint check. The errors were in
The previous TypeScript check failure was an unrelated CI infrastructure issue (GnuTLS network error during git fetch). |
|
Awesome @MelvinBot. Please mark the PR as |
|
This PR has been marked as ready for review as requested. |
|
@MelvinBot Please resolve the failure of PR Author Checklist |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7eac8ae3b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (autocompleteQueryValue === '') { | ||
| // When query is cleared, reset the initial focus guard so the initial focus | ||
| // effect can re-fire and correctly focus the first focusable item (skipping section headers). | ||
| hasSetInitialFocusRef.current = false; | ||
| } else { |
There was a problem hiding this comment.
Keep list scrolled to top when query is cleared
When autocompleteQueryValue becomes empty, this branch only flips hasSetInitialFocusRef and no longer calls updateAndScrollToFocusedIndex(..., 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.
Flipping hasSetInitialFocusRef will trigger the existing initial focus effect which will call updateAndScrollToFocusedIndex. So, this seems to be a non-issue.
| if (!shouldScroll) { | ||
| suppressNextFocusScrollRef.current = true; | ||
| } |
There was a problem hiding this comment.
Reset suppressed-scroll state on no-op focus updates
Setting suppressNextFocusScrollRef.current = true before setFocusedIndex(index) assumes onFocusedIndexChange will always run next, but useArrowKeyFocusManager skips that callback when the index is unchanged. In that case the suppress flag leaks into the next real focus change and cancels scrolling unexpectedly (e.g., after a no-scroll update to the same index, the next ArrowDown changes focus but does not scroll).
Useful? React with 👍 / 👎.
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp84597-android-hybrid-001.mp4Android: mWeb Chrome84597-mweb-chrome-001.mp4iOS: HybridApp84597-ios-hybrid-001.mp4iOS: mWeb Safari84597-mweb-safari-001.mp4MacOS: Chrome / Safari84597-web-chrome-001.mp4 |
rojiphil
left a comment
There was a problem hiding this comment.
@marcochavezf Changes LGTM and works well too.
All yours. Thanks.
|
@MelvinBot Please address the comment here |
|
Resolved the PR Author Checklist failure — all 10 dynamically-added component checklist items have been checked, and the malformed trailing section has been cleaned up. The checklist check should pass on the next run. |
JmillsExpensify
left a comment
There was a problem hiding this comment.
This change makes sense to me. Product approved.
|
@marcochavezf I think there is no pending task here other than merging with main |
|
🚧 @marcochavezf has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.3.38-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.38-4 🚀
|
Explanation of Change
When the search query is cleared (user deletes all text in the search box), the query-change
useEffectwas callingupdateAndScrollToFocusedIndex(0, true), which setfocusedIndexto 0. But with the query empty,searchQueryItemdisappears and sections rebuild with titled sections ("Recent Searches", "Recent Chats"). TheuseFlattenedSectionshook inserts section headers as the first items in the flattened list, and these headers are disabled/non-focusable. SofocusedIndex=0pointed at a disabled header, and no item appeared visually focused.The fix: when the query transitions to empty, instead of blindly focusing index 0, reset
hasSetInitialFocusRef.current = false. This allows the existing initial focus effect (which already has correct header-skipping logic) to re-fire in the same render cycle and set focus on the first actual focusable item.Fixed Issues
$ #83929
PROPOSAL: #83929 (comment)
Tests
Offline tests
N/A — This is a focus management fix that doesn't involve network requests.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web: Chrome
Tested on Chrome — after clearing the search query, focus correctly lands on the first recent chat item (skipping the "Recent chats" section header).
Android: Native
N/A — focus behavior is desktop only (
shouldUseNarrowLayoutguard)Android: mWeb Chrome
N/A — focus behavior is desktop only
iOS: Native
N/A — focus behavior is desktop only
iOS: mWeb Safari
N/A — focus behavior is desktop only
MacOS: Chrome / Safari
Tested on Chrome — after clearing the search query, focus correctly lands on the first recent chat item (skipping the "Recent chats" section header).