Stop scroll jump when selecting items in category filter#89392
Stop scroll jump when selecting items in category filter#89392
Conversation
Merge selected and remaining items into a single sorted section instead of splitting them across two sections. This prevents the scroll-to-top triggered by BaseSelectionListWithSections when sections.length > 1. Also set initiallyFocusedItemKey to the first selected item so the list scrolls to the right position on initial render. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| <SelectionListWithSections | ||
| sections={sections} | ||
| ListItem={MultiSelectListItem} | ||
| initiallyFocusedItemKey={firstSelectedKey} |
There was a problem hiding this comment.
@MelvinBot do we also need to set shouldUpdateFocusedIndex true?
There was a problem hiding this comment.
@MelvinBot Also, we need to set shouldClearInputOnSelect false
There was a problem hiding this comment.
No — these two props are independent and serve different purposes:
initiallyFocusedItemKey— one-time setup: scrolls to the first selected item when the list mounts.shouldUpdateFocusedIndex— ongoing: updates the focused index every time the user clicks/selects a row (viaBaseSelectionListWithSections.tsx:171-173).
In a multi-select picker where users are toggling items on and off, enabling shouldUpdateFocusedIndex would cause setFocusedIndex to fire on each click, which could trigger scroll repositioning — potentially reintroducing the scroll-jump behavior this PR is fixing.
There was a problem hiding this comment.
Done — moved shouldClearInputOnSelect from SelectionListProps to BaseSelectionListProps so it's available to SelectionListWithSections, then set it to false in SearchMultipleSelectionPicker. The default remains true to preserve existing behavior for other callers.
Move shouldClearInputOnSelect from SelectionListProps to BaseSelectionListProps so it can be used by SelectionListWithSections. Set it to false in SearchMultipleSelectionPicker to preserve the search filter text when selecting/deselecting items. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
The The runner failed to download the Fix: Re-run the failed job. This should resolve on its own. |
|
The Evidence: The error is The two You can re-run the failed job to resolve this. |
|
@MelvinBot The selected item should not be auto scrolled to the top of the viewport on first selection after opening page first time Screen.Recording.2026-05-03.at.10.55.50.pm.mov |
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Removed |
@MelvinBot we still need this to auto scroll to the preselected item when opening page. |
Re-adds initiallyFocusedItemKey so the list scrolls to the first preselected item when opening the page. This does not cause scroll jumps on item selection because FlashList's initialScrollIndex only applies on mount and useArrowKeyFocusManager ignores prop changes after its initial useState call. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Restored |
|
@MelvinBot check off all items in author checklist with this screen recording: demo.mov |
|
I'm unable to process video content, so I can't verify the screen recording to check off the author checklist items. Could you check off the applicable items directly in the PR description? You can edit the PR body and toggle the checkboxes yourself. |
|
@MelvinBot C+ doesn't have permission to edit author checklist. Check off all items as I already verified myself. |
|
Done — all PR Author Checklist items have been checked off. |
|
@MelvinBot add Tests, QA Steps |
|
Added Tests, Offline tests, and QA Steps to the PR description. |
|
@Expensify/design please check if this behavior is fine. Selected item is auto scrolled to the top of the viewport first time. first.selection.movSimilar to scrolling to the preselected first item when open page. first.open.movFrom 2nd selection, auto scroll never happens. Auto scrolled only when search input changes. 2nd.selection.mov |
I don't think we want this cause it's quite jarring :/
On one hand I like this, but it feels overly clever to me given that it's not super obvious there's more items above. I just think we don't want any scrolling or have I missed something here. Curious what @Expensify/design thinks too |
|
ok after disabling scroll in both cases: disable.mov |
|
Curious what @Expensify/design thinks as well, but that feels a bit better to me. I understand the reasoning for wanting the selected options at the top when you go back into the flow though so I can see that still being enabled, but I still don't think we should have any auto-scrolling |
|
Why aren't we solving this everywhere instead of just the filters list? Remember, we have this issue that we were planning to use to fix this behavior across the whole app: #61414 I think the idea is quite simple:
|
|
We'll solve this everywhere eventually. |
| <SelectionListWithSections | ||
| sections={sections} | ||
| ListItem={MultiSelectListItem} | ||
| initiallyFocusedItemKey={firstSelectedKey} |
There was a problem hiding this comment.
initiallyFocusedItemKey={firstSelectedKey}
@MelvinBot while keeping this, we should not update initialScrollIndex of FlashList to prevent auto scroll whenever item is selected. But still update when page is first rendered.
(Scroll happens when this prop value of FlashList is changed from -1)
There was a problem hiding this comment.
Fixed — initiallyFocusedItemKey is now computed once on mount via a useState lazy initializer (from initiallySelectedItems and the sorted items), so it stays stable across re-renders. This prevents initialFocusedIndex → initialScrollIndex from changing when the user toggles selections.
There was a problem hiding this comment.
@MelvinBot "scrolling to the inital focused item on clear search" issue still happens because of native feature in FlashList.
FlashList internally auto-scrolls whenever data changes and updates initiallyFocusedItemKey existence (from "not found" to "found") in data
The ideal solution is to clear initiallyFocusedKey after first render.
There was a problem hiding this comment.
Done — initiallyFocusedItemKey is now cleared after the first render using a ref to track post-mount state. On the initial render the key is passed so FlashList scrolls to the selected item, and on all subsequent renders it's undefined so data changes (like clearing a search) won't trigger auto-scrolling.
Compute the initial focused key once on mount using useState lazy initializer, so it doesn't change when the user toggles selections. This prevents FlashList's initialScrollIndex from updating and triggering auto-scroll on each item click. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
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. |
| const [initiallyFocusedKey] = useState(() => { | ||
| const initialIDs = new Set((initiallySelectedItems ?? []).map((item) => item.value.toString())); | ||
| const sorted = [...items].sort((a, b) => sortOptionsWithEmptyValue(a.value.toString(), b.value.toString(), localeCompare)); | ||
| return sorted.find((item) => initialIDs.has(item.value.toString()))?.name; |
There was a problem hiding this comment.
@MelvinBot sort is run twice (also here), and this will cause performance impact?
Also let's avoid duplicated code like new Set((initiallySelectedItems ?? []).map((item) => item.value.toString()))
There was a problem hiding this comment.
Fixed in 962eda6 — eliminated both issues:
- Duplicate sort removed:
initiallyFocusedKeyis now computed from the already-sortedsectionDatainstead of sorting a second time in a separateuseStateinitializer. - Duplicate Set removed: Extracted
initialSelectedIDsinto its ownuseStateand reuse it for bothselectedItemIDsinitialization and theinitiallyFocusedKeylookup.
Store initialSelectedIDs once in state and reuse it for both selectedItemIDs initialization and initiallyFocusedKey lookup. Compute initiallyFocusedKey from the already-sorted sectionData instead of sorting a second time in a separate useState initializer. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Please check this behavior now: new.movAuto scroll happens when
Otherwise never scrolls. |
|
Would it be possible to not have the animation of scroll when typing in search? Given the list filters I think it's actually better for that specifically to just be instant. Again curious if @Expensify/design is aligned to all this |
| const [searchTerm, debouncedSearchTerm, setSearchTerm] = useDebouncedState(''); | ||
|
|
||
| const [selectedItemIDs, setSelectedItemIDs] = useState(() => new Set((initiallySelectedItems ?? []).map((item) => item.value.toString()))); | ||
| const [initialSelectedIDs] = useState(() => new Set((initiallySelectedItems ?? []).map((item) => item.value.toString()))); |
There was a problem hiding this comment.
@MelvinBot when search something to have no results and clear search, scroll still happens because initiallyFocusedKey is set to undefined(computed here) and then set to as stale initial selected item as initialSelectedIDs never changes after first render.
search.mov
There was a problem hiding this comment.
Good catch. Fixed in b797082 — initiallyFocusedKey is now computed once on mount via a useState lazy initializer (from items + initialSelectedIDs), so it no longer depends on sectionData and stays stable when search results change.
Compute initiallyFocusedKey once on mount via useState lazy initializer instead of recomputing from sectionData every render. This prevents the key from flipping between undefined (empty search) and a real key (search cleared), which was triggering initialScrollIndex changes and scroll jumps. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Agree with that! |
| const [initialSelectedIDs] = useState(() => new Set((initiallySelectedItems ?? []).map((item) => item.value.toString()))); | ||
| const [selectedItemIDs, setSelectedItemIDs] = useState(() => initialSelectedIDs); | ||
| const [initiallyFocusedKey] = useState(() => { | ||
| const sorted = [...items].sort((a, b) => sortOptionsWithEmptyValue(a.value.toString(), b.value.toString(), localeCompare)); |
There was a problem hiding this comment.
@MelvinBot can you avoid sorting twice for performance and also not re-introduce this bug?
There was a problem hiding this comment.
Fixed in 230c1c6 — replaced the full [...items].sort(...) in the useState initializer with a linear scan that finds the minimum selected item using the comparator directly. This eliminates the duplicate sort while keeping initiallyFocusedKey stable across re-renders (computed once on mount via useState, so no search-clear scroll bug).
Replace the full array sort in the useState initializer with a linear scan that finds the minimum selected item using the comparator directly. This eliminates the duplicate sort while keeping initiallyFocusedKey stable across re-renders (no search-clear scroll bug). Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@MelvinBot disable scroll animation when typing in search as per #89392 (comment) |
Search-triggered scrolls in useSearchFocusSync now pass animated: false so the list jumps instantly instead of smooth-scrolling when filtering. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Disabled scroll animation when typing in search. The |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
… auto-scroll FlashList internally auto-scrolls when initialScrollIndex changes from -1 to a valid index. This happens when search is cleared and the focused item key transitions from "not found" to "found" in the data. Using a ref to track post-mount state ensures the key is only passed on the initial render. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Explanation of Change
When selecting a category in the Spend > Filters > Category picker, the selected item jumps to the top of the list and the view scrolls up, disorienting the user.
This happened because
SearchMultipleSelectionPickersplit items into two sections (selected vs remaining). When an item was newly selected,BaseSelectionListWithSections.selectRowdetectedsections.length > 1and calledscrollToIndex(0), scrolling back to the top.This PR merges all items into a single sorted section so selecting an item no longer triggers re-ordering or scroll jumps. It also sets
initiallyFocusedItemKeyto the first selected item so the list scrolls to the right position on initial render.Fixed Issues
$ #61414
Tests
Offline tests
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