Refactor: flash list dom order improvement for simple lists - #91605
chuckdries merged 11 commits into
Conversation
|
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
6fb480e to
befd57b
Compare
…der-improvement/unselectable-lists
…der-improvement/unselectable-lists
93b944d to
434c5b4
Compare
|
@TaduJR, could you take a look if I'd introduced any regression unknowingly to your changes from this PR? I've removed lots of the things you added in that PR. I've tried to check the things your PR aimed to fix for regressions and haven't found anything. Still, a second opinion is welcome. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ 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". |
|
@sharabai Do we need to create a new ticket for this PR? Is this refactor related to the ticket mentioned above? |
|
@huult |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
@MelvinBot Please help generate test steps for this ticket. |
|
Here are test steps for this PR,
Tests1. Currency picker — continuous Tab nav + arrow-key scroll (the fix)
2. Search sync — typing scrolls to top once, no double-bounce
3. Multi-select — selecting items doesn't scroll or reorder
4. Focus restore — closing the detail RHP doesn't scroll the list
5. Console — throughout all flows, verify no errors appear in the JS console. OfflineN/A — focus-management refactor only. No new API calls, Onyx writes, or offline-relevant paths. Automated coverage (already in the PR — run before manual QA)These assert the new
|
|
Ah, I think I understand. Can we solve it simply here or will it require another patch? |
|
Ah, since it's a separate patch, and doesn't really meet the description of the original issue linked from this PR, let's handle it as a separate issue. At the very least, I don't want to increase the chance we revert this improvement for web users. @sharabai feel free to file it and assign @TaduJR to review |
@chuckdries I'm not fully sure what @TaduJR meant by it. I'll build main and compare it to this branch to see the difference. |
@TaduJR I'm having trouble finding that. 91605.main.mp4And here's one from the branch build: 91605.local.mp4What am I missing? |
…der-improvement/unselectable-lists
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ 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". |
|
I did some testing of main versus this branch, and I concur I didn't notice a difference between what items are selectable with talkback. I was just tabbing through a concierge chat. @TaduJR can you clarify what use case or situation you found in this comment
|
I think it's another PR that was offending during that time. It was not like that previously. I am seeing same behaviour as you stated on the demo now.
We can leave this now. Thanks! |
chuckdries
left a comment
There was a problem hiding this comment.
Ok! Thanks for sticking with this everyone. I would always rather be extra cautious at review time than have to revert due to a deployblocker later!
|
@huult looks like we changed the checklist between when you filled it out and now. Would you kindly update your checklist to match the latest https://raw.githubusercontent.com/Expensify/App/main/contributingGuides/REVIEWER_CHECKLIST.md then we're finally good to merge! |
|
sure |
|
@TaduJR I'm with @chuckdries on this one, it's better to be safe than sorry. |
|
Oh speaking of, did that sibling issue ever get filed or was I supposed to do it? |
Yea @sharabai, I am already fully in support of handling separately.
I think it's this one #94165 Should I comment one the issue? |
Yeah, that's the one. I left a comment explaining the origin, but I can't assign anyone directly. |
|
@huult reviewer checklist is still failing https://github.com/Expensify/App/actions/runs/28180818828/job/83535773117 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 chuckdries 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/chuckdries in version: 9.4.21-2 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.21-9 🚀
Bundle Size Analysis (Sentry): |
Note
This PR depends on the FlashList natural-DOM-order patch PR (#91328) — that PR must be merged first.
Explanation of Change
What this fixes (user-visible). Continuous Tab navigation through long single-column selection lists (e.g. the currency picker reached via
+ → Create expense → tap the currency button) didn't work correctly together with the FlashList natural-DOM-order patch. Each Tab firedListItemRenderer.onFocus→setFocusedIndex(N)(no scroll opt-out) →useArrowKeyFocusManager'sonFocusedIndexChangeeffect → a programmaticscrollToIndex(N). From FlashList's perspective the resulting movement looked like keyboard-driven scrolling throughuseArrowKeyFocusManager, not Tab traversal — so the natural-DOM-order patch couldn't reliably treat the interaction as tabbing and DOM order drifted out of step with what Tab was doing. After this PR,ListItemRenderer.onFocuspasses{shouldScroll: false}, so Tab no longer fires a programmaticscrollToIndex; FlashList sees the actual DOM Tab traversal and the natural-DOM-order patch behaves correctly across continuous tabbing.Implementation note. Internally, the previous focus-management code suppressed unwanted scrolls via an imperative
suppressNextFocusScrollRefflag that consumers had to flip at the right moment. This PR replaces that with an explicitsetFocusedIndex(index, {shouldScroll})option onuseArrowKeyFocusManager, threaded throughBaseSelectionList,BaseSelectionListWithSections, anduseSearchFocusSync.ListItemRenderer.onFocusnow passes{shouldScroll: false}so programmatic focus events never auto-scroll, and internal arrow-key handlers default to{shouldScroll: true}. The new declarative API should cover everything the suppress-ref pattern covered before — and may even be a touch more robust in corner cases where the imperative flag could fall out of sync with React state updates.The primary target is
CurrencySelectionList— a canonical single-column picker (no right-hand-side button, simpleSingleSelectListItemrows). The same change benefits every similar single-column picker that usesSelectionListWithSectionswith a plain list item — currency picker, country picker, language picker, priority mode picker, tax picker, category/tag pickers, destination picker, debug constant picker, report-field initial-value picker, and the various workspace settings pickers (settlement frequency, statement close date, layout, etc.).Fixed Issues
$ #86126
PROPOSAL:
Tests
Each test below exercises a specific code path that this PR rewrites. Test 1 is the user-visible fix; Tests 2–4 are regression checks on the other behaviors the old
suppressNextFocusScrollRefflag was serving (their original test steps come from the merged PRs that introduced each behavior, linked inline).Prerequisites: run these commands from project root before testing
Currency picker — continuous Tab navigation + arrow-key scroll into view (exercises
ListItemRenderer.onFocusanduseArrowKeyFocusManager's scroll gate — the actual fix).USD,EUR). The currency picker opens.Spend > Filters > Currency — typing in search scrolls to the top once, no double-bounce (exercises
useSearchFocusSync+BaseSelectionListWithSections.onFocusedIndexChange, regression for #89498).Spend > Filters > Workspace — selecting items doesn't scroll or reorder (exercises
BaseSelectionListWithSections.selectRow's{shouldScroll: false}, regression for #90121).Precondition: account with multiple workspaces.
Workspace > Categories — closing the category-detail RHP doesn't scroll the list (exercises
BaseSelectionList.setFocusedIndexFromRowFocus+isFocusRestoreInProgress(), regression for #90869).Offline tests
N/A — this is a focus-management refactor. No new API calls, Onyx writes, or offline-relevant code paths.
QA Steps
Same as Tests. macOS Chrome is the primary platform (keyboard-driven flows are where this refactor matters most); iOS Native, Android Native, and mWeb are regression-only — tap-to-focus rows should still highlight the tapped row without unexpected scrolling, and selection should still work.
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
Android: mWeb Chrome
iOS: Native
ios.regression.tests.91605.mp4
iOS: mWeb Safari
ios.mWeb.regression.tests.91605.mp4
MacOS: Chrome / Safari
Working.Currency.List.mp4