Fix incorrect result selection when navigating Search with keyboard#82098
Fix incorrect result selection when navigating Search with keyboard#82098marufsharifi wants to merge 0 commit intoExpensify:mainfrom
Conversation
|
@dannymcclain @linhvovan29546 One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
The failed actions are not related to our changes. btw. |
src/components/SelectionListWithSections/BaseSelectionListWithSections.tsx
Outdated
Show resolved
Hide resolved
| /** | ||
| * Handles isTextInputFocusedRef value when using external TextInput, so external TextInput does not lose focus when typing in it. | ||
| * | ||
| * @param isTextInputFocused - Is external TextInput focused. |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The updateExternalTextInputFocus callback wrapper is unnecessary. It creates a new function that simply calls another function, adding an extra layer of indirection without any benefit.
Suggested fix: Directly expose updateTextInputFocusState or inline the logic:
// Option 1: Remove updateExternalTextInputFocus and expose updateTextInputFocusState via useImperativeHandle
useImperativeHandle(
ref,
() => ({
// ... other methods
updateExternalTextInputFocus: updateTextInputFocusState,
}),
[/* dependencies */],
);
// Option 2: If updateExternalTextInputFocus needs to remain for API compatibility,
// make updateTextInputFocusState a regular function (not useCallback) as suggested in the previous commentThe current pattern creates unnecessary function allocations and adds cognitive overhead.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
src/components/SelectionListWithSections/BaseSelectionListWithSections.tsx
Show resolved
Hide resolved
src/components/SelectionListWithSections/BaseSelectionListWithSections.tsx
Outdated
Show resolved
Hide resolved
linhvovan29546
left a comment
There was a problem hiding this comment.
@marufsharifi Please remove shouldIgnoreHoverIndex and the hover next/previous handling from useArrowKeyFocusManager. We only need the change to disable hover via shouldDisableHoverStyle
|
@marufsharifi Any updates? |
|
@linhvovan29546 I can no longer reproduce this issue on latest main. I retested the OP steps and keyboard navigation now behaves as expected. It appears this was already resolved by #81293 79395.mp4 |
|
Although the main issue is resolved, we still see two hover states. Please apply your proposed fix to disable the hover state when navigate via arrow button |
|
@marufsharifi Could you please remove the |
|
@linhvovan29546, thanks for your patience. Please take another look when you get a chance. thanks. |
linhvovan29546
left a comment
There was a problem hiding this comment.
@marufsharifi thanks for the update! Could you also remove the shouldStopMouseLeavePropagation flag from the codebase? Since we removed onMouseLeave, that flag is no longer needed.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
| const handleMouseLeave = () => { | ||
| bind.onMouseLeave(); | ||
| if (shouldStopMouseLeavePropagation) { | ||
| e.stopPropagation(); |
There was a problem hiding this comment.
@marufsharifi This is incorrect. We still need e.stopPropagation(); we only need to remove the conditional, which is sufficient.
| const handleMouseLeave = (e: React.MouseEvent<Element, MouseEvent>) => { | ||
| const handleMouseLeave = () => { | ||
| bind.onMouseLeave(); | ||
| if (shouldStopMouseLeavePropagation) { |
There was a problem hiding this comment.
Could you please update the test to add step |
9262bcd to
13c1c76
Compare
|
|
30da612 to
d600d30
Compare
Explanation of Change
When a user is typing in search, keyboard navigation now prioritizes what the keyboard is doing, not where the mouse happens to be hovering.
Fixed Issues
$ #79395
PROPOSAL: #79395 (comment)
Tests
Offline tests
Same as the tests.
QA Steps
Same as the tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: Native
Screen.Recording.2026-02-13.at.7.13.16.PM.mov
Android: mWeb Chrome
Screen.Recording.2026-02-13.at.7.15.53.PM.mov
iOS: Native
Screen.Recording.2026-02-13.at.7.19.44.PM.mov
iOS: mWeb Safari
Screen.Recording.2026-02-13.at.7.22.29.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-02-13.at.4.24.57.PM.mov