Suppress redundant animated scroll in useSearchFocusSync#89498
Suppress redundant animated scroll in useSearchFocusSync#89498
Conversation
When useSearchFocusSync calls scrollToIndex followed by setFocusedIndex, the setFocusedIndex triggers onFocusedIndexChange which fires another scrollToIndex with animation. This causes a visible animated scroll on top of the non-animated scroll that was already performed. Add suppressNextFocusScroll callback to useSearchFocusSync so BaseSelectionListWithSections can set suppressNextFocusScrollRef before setFocusedIndex, preventing the redundant animated scroll. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
| setFocusedIndex, | ||
| }); | ||
|
|
||
| const suppressNextFocusScroll = useCallback(() => { |
There was a problem hiding this comment.
@MelvinBot no need useCallback since this is auto memoized by React Compiler
There was a problem hiding this comment.
Removed the useCallback wrapper — React Compiler handles memoization automatically. Fixed in dc142e7.
React Compiler auto-memoizes this, so the explicit useCallback is unnecessary. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@MelvinBot pull main and fix conflict |
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Merged main and resolved the conflict in |
|
@MelvinBot when arrow key (up or down) is pressed, list should be scrolled to the focused item. How to implement this? |
|
In The fix: suppress the scroll before updating focused index in if (shouldUpdateFocusedIndex && typeof indexToFocus === 'number') {
+ suppressNextFocusScrollRef.current = true;
setFocusedIndex(indexToFocus);
}This way arrow keys still scroll (they go through |
|
@MelvinBot implement this. Also pass |
…archMultipleSelectionPicker Set suppressNextFocusScrollRef before setFocusedIndex in selectRow so mouse clicks update the focused index without triggering auto-scroll. Arrow key navigation continues to scroll normally via onFocusedIndexChange. Also pass shouldUpdateFocusedIndex to SelectionListWithSections in SearchMultipleSelectionPicker so the focused index tracks the selected item. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Done. Pushed both changes:
|
|
@MelvinBot pull main |
Regression Analysis1.
2. Scroll suppression applies to all ~30 The 3. Adding 4.
TL;DR: Fix #1 ( |
|
@MelvinBot add Tests / QA steps and complete author checklist (all items are verified by me) |
…FocusScroll Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Merged main — no conflicts. |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
Done — updated the PR body with:
|
|
@MelvinBot update Tests / QA steps like below:
|
|
Updated the Tests and QA Steps sections with the provided steps. |
|
@MelvinBot mark as ready for review |
|
I'm unable to mark the PR as ready for review due to environment permission restrictions. You can do it yourself by clicking the "Ready for review" button on the PR page, or by running: |
|
@MelvinBot mark as ready for review like you did in #89392 (comment) |
|
Done — PR is now marked as ready for review. |
|
@mkhutornyi Please 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b0c24a968
ℹ️ 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".
| suppressNextFocusScrollRef.current = true; | ||
| setFocusedIndex(indexToFocus); |
There was a problem hiding this comment.
Clear stale focus-scroll suppression on no-op focus updates
Setting suppressNextFocusScrollRef.current = true before setFocusedIndex(indexToFocus) can leave suppression armed when the index is unchanged (for example, clicking a row that is already focused). In that case useArrowKeyFocusManager does not fire onFocusedIndexChange for the no-op update (src/hooks/useArrowKeyFocusManager.ts, prevIsFocusedIndex === focusedIndex guard), so the next real focus move (e.g., first ArrowDown) incorrectly skips scrolling and can move focus out of view. This should only suppress when the focus index will actually change, or explicitly clear the flag for same-index updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 698b951. The suppression flag is now only armed when indexToFocus !== focusedIndex, so clicking an already-focused row no longer leaves it stale for the next arrow-key scroll.
|
The fix is already implemented in this PR — All CI checks pass (the only "failure" is the PR Author Checklist). Is there something else you'd like changed? |
Guard suppressNextFocusScrollRef so it is only armed when the clicked row differs from the current focusedIndex. Previously, clicking the already-focused row would arm the flag without onFocusedIndexChange ever firing to clear it, causing the next arrow-key press to skip scrolling. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@francoisl Please 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 698b9511c6
ℹ️ 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".
| suppressNextFocusScroll?.(); | ||
| setFocusedIndex(firstFocusableIndex); |
There was a problem hiding this comment.
Skip suppression when focused index is already current
Calling suppressNextFocusScroll?.() unconditionally before setFocusedIndex(firstFocusableIndex) can leave suppressNextFocusScrollRef stuck at true when the focused index is already firstFocusableIndex (React ignores same-value state updates, so onFocusedIndexChange never runs to clear the flag). In that case, the next real focus change (for example the first ArrowDown after typing) has its scroll suppressed unexpectedly, which can break the “keep focused item visible” behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac2cf8b. Added a focusedIndex param to useSearchFocusSync and now only arm the suppression flag when the target index differs from the current one, so it cannot get stuck when React skips the no-op state update.
…l change When the focused index is already the target value, React skips the state update and onFocusedIndexChange never fires, leaving the suppression flag stuck at true. This caused the next real arrow-key focus change to skip scrolling. Now we only arm the flag when the index will actually change. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
Explanation of Change
When typing in a selection list's search input,
useSearchFocusSynccallsscrollToIndex(0)followed bysetFocusedIndex(firstFocusableIndex). ThesetFocusedIndexcall triggersonFocusedIndexChangeinBaseSelectionListWithSections, which fires a redundantscrollToIndexcall withanimated=true(the default). This produces a visible animated scroll on top of the non-animated scroll that was already performed.The
suppressNextFocusScrollRefmechanism inBaseSelectionListWithSectionsexists to prevent exactly this kind of redundant scroll, butuseSearchFocusSyncnever used it because it calls rawsetFocusedIndexinstead of going throughupdateAndScrollToFocusedIndex.This PR adds an optional
suppressNextFocusScrollcallback touseSearchFocusSync.BaseSelectionListWithSectionspasses a callback that setssuppressNextFocusScrollRef.current = true, so theonFocusedIndexChangehandler skips the redundant animated scroll.BaseSelectionList(which has no suppress mechanism) is unaffected — the parameter is optional and defaults to undefined.Additionally, in
selectRow, whenshouldUpdateFocusedIndexis true and the focused index is updated after a mouse click, we suppress the focus-triggered scroll so the list doesn't jump to the clicked item.SearchMultipleSelectionPickernow passesshouldUpdateFocusedIndexso arrow-key navigation updates the focused index correctly.Fixed Issues
$ #61414
Tests
Offline tests
N/A — this is a UI scroll behavior fix with no network dependency.
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari