Fix: force FlashList to use natural DOM order on web#85825
Fix: force FlashList to use natural DOM order on web#85825sharabai wants to merge 1 commit intoExpensify:mainfrom
Conversation
|
|
sharabai
changed the title
|
This patch starts with 003 because another one of my PRs takes 002. |
FlashList isn't used in Mobile-Expensify, we're good here. |
melvin-bot
bot
requested review from
eVoloshchak
and removed request for
a team
|
@eVoloshchak 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] |
rushatgabhane
requested review from
shubham1206agra
and removed request for
eVoloshchak
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ca24ea86e
ℹ️ 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 (Platform.OS === "web") { | ||
| + renderEntries.sort(([, a], [, b]) => a.index - b.index); |
There was a problem hiding this comment.
This sort always uses ascending index, but FlashList still supports inverted on web (RecyclerView/ViewHolder reverse the visual order via the inverted transform helpers). In that mode the DOM will now be deterministically ordered opposite to what users see, so keyboard and screen-reader traversal stays backwards for any inverted FlashList even though this patch is meant to fix accessibility ordering. The web sort needs to account for inverted (and horizontal inverted) instead of always using a.index - b.index.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We don't have yet any components that are using inverted prop with FlashList. I did test it on FlashList 2.3.0 without the patch and let's just say inverted does a lot of things at once and navigation there looks weird in general. For example, list starts from the bottom and tabbing goes up, not down (without the patch). The patch does not change that behavior. Maybe it's warranted to come back to it when we have some components that do use that prop and reevaluate, but the behavior looks good to me at this point.
There was a problem hiding this comment.
@VickyStash Can you please look into this when you use this prop?
There was a problem hiding this comment.
Here is the PR #85114 where I'm working on migration main chat to FlashList with the inverted flag (P.S. I have three patches there as well). So what should I test specifically?
There was a problem hiding this comment.
FYI I've also tested this PR over the scenario when main chat (so expense details too) are migrated to FlashList, and I see some wrong ordering when I use voice over:
Monosnap.screencast.2026-03-20.09-32-05.mp4
But since FlashList is not used for the main chat in the current prod, I guess it can be solved later.
Just one question from me, does this PR then fixes the original issue for production? Since at this moment FlatList is used for main chat/expense details on prod?
There was a problem hiding this comment.
@VickyStash
This PR aimed to fix the issue of elements in the list being often in 'random' order when scrolling through it.
It was created with the assumption that the main chat would use FlashList instead, which is not true anymore as understand it.
Let's focus on what's actually needed for my PR and why @VickyStash was tagged in the first place.
It all comes down to if you apply my PR to your PR, would it break anything in FlashList's behavior with inverted prop used? I think that was what @shubham1206agra was asking. Basically, could you look at how navigating with screen reader works without my patch on a FlashList with inverted prop, then apply my patch and see if it broke anything.
There was a problem hiding this comment.
I've tested the patch over the main chat that uses FlashList with inverted flag and here are the results:
- I've followed the test steps from the original issue and i see the same issue:
Monosnap.screencast.2026-03-23.17-22-15.mp4
- But other than that, I've noticed nothing. Everything works the same way as before
There was a problem hiding this comment.
Great, it means it doesn't come in conflict with inverted prop. Since the original issue is going to be fixed with the original approach I'm changing the issue to a broader one I created.
Screen.Recording.2026-03-19.at.11.35.42.PM.movBUG: Multiple items in LHN gets highlighted when scrolling fast (Not repro on staging) |
|
🚧 @luacmartins 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! 🧪🧪
|
sharabai
force-pushed
the
fix/flash-list/DOM-order
branch
from
2ca24ea to
c41dca9
Compare
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
sharabai
changed the title
sharabai
changed the title
sharabai
force-pushed
the
fix/flash-list/DOM-order
branch
3 times, most recently
from
7dc3360 to
8640360
Compare
|
No product review needed |
sharabai
force-pushed
the
fix/flash-list/DOM-order
branch
from
8640360 to
1f0f379
Compare
sharabai
force-pushed
the
fix/flash-list/DOM-order
branch
from
5778fd7 to
d97f44f
Compare
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d97f44f415
ℹ️ 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".
patches/@shopify/flash-list/@shopify+flash-list+2.3.0+003+sort-for-natural-DOM-order.patch
Outdated
Show resolved
Hide resolved
sharabai
force-pushed
the
fix/flash-list/DOM-order
branch
from
d97f44f to
c248d32
Compare
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@codex review |
sharabai
force-pushed
the
fix/flash-list/DOM-order
branch
from
c248d32 to
d8d677f
Compare
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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
force-pushed
the
fix/flash-list/DOM-order
branch
from
d8d677f to
eef0c40
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eef0c40481
ℹ️ 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".
patches/@shopify/flash-list/@shopify+flash-list+2.3.0+003+sort-for-natural-DOM-order.patch
Show resolved
Hide resolved
sharabai
force-pushed
the
fix/flash-list/DOM-order
branch
from
eef0c40 to
30e5ba1
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30e5ba180d
ℹ️ 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 (isSorted) { | ||
| + return; | ||
| + } | ||
| + entries.sort(([, a], [, b]) => a.index - b.index); |
There was a problem hiding this comment.
The web reorder logic still hardcodes ascending index order (a.index - b.index), so any FlashList rendered with inverted will get DOM order opposite to its visual order and keyboard/screen-reader traversal will remain backwards. Fresh evidence in this revision is that the comparator is still fixed to ascending and no inverted (or equivalent) flag is read before sorting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This was covered here already.
#85825 (comment)
VickyStash
left a comment
VickyStash
left a comment
There was a problem hiding this comment.
Overall tests well, just a couple of notes.
- I've noticed an issue, that if scroll slowly through the Inbox the hovering effect dosen't follow, I've ended up with a couple of items hovered:
bug_example_1.mp4
- It would be good to add to PR recordings a recording from the E/App, not from the expo snack example app.
| +const SORT_DELAY_MS = 1000; | ||
| +const TAB_SCROLL_THRESHOLD_MS = 400; |
There was a problem hiding this comment.
Why are these specific numbers used?
There was a problem hiding this comment.
@VickyStash The approach is described in more detail in details.md, but for a quick overview - we sort the DOM nodes FlashList is using underneath. The SORT_DELAY_MS is the delay before the we do the sort of the DOM nodes. It's needed exactly to avoid stale hover states that were reported here.
the question then becomes how long do we wait before sorting?
Long enough so that hover states get their mouseleave event fired. With momentum scrolling on Macs for example you have to wait about that time so that the correct element gets the hover.
Example of a late hover (it took about 500ms after scroll completely stopped for hover to change)
late.hover.example.mp4
And it can happen to be even longer, so 1000ms was kind of a good spot to stop to avoid most of state hover states.
There was a problem hiding this comment.
@VickyStash
The second TAB_SCROLL_THRESHOLD_MS is used when user navigates using tabs or screen readers that focus elements. In that case we need to immediately sort the DOM nodes (because tabbing works on DOM node order in web).
This var is used to control the following behavior: we are in the list, we are tabbing - tabbing normally means that we don't scroll, but when we reach an element that is outside of a viewport (browser sees we're focusing on an element that it outside of the FlashList's viewport), the list scrolls so that element is in the middle.
Normally we would defer sorting because it'd introduce late hovers, but since we need it ASAP for navigation, we don't defer scroll.
The TAB_SCROLL_THRESHOLD_MS is used to answer the question: when have we last tabbed? If it was more recent than this threshold - do DOM sort immediately because we're likely causing scroll by browser focusing on an element outside of the viewport, meaning we're using tab navigation - so sort immediately.
There was a problem hiding this comment.
The timing itself was chosen so that it works well enough under most conditions.
@VickyStash |
@VickyStash |
|
@VickyStash |
Increasing the |
The first part about |
|
The data preparation in NewChatPage was not looked at closely for a long time. Lots of things are happening that don't make sense at this point. I'd be modifying it in chunks so that it's easier to track progress/ merge PRs. The list of current PRs that aim to clean up data preparation there: |
|
Thank you for the explanations! |
|
Let's just make sure there are no other side effects we're unaware of. |
4 participants
Explanation of Change
FlashList uses absolute positioning, so the DOM order of elements doesn’t affect layout from its perspective. However, this creates an accessibility issue: mobile screen readers navigate based on visual position, while web screen readers follow the DOM order.
This patch ensures that list items are rendered in a natural DOM order, aligning behavior across platforms.
Performance
The performance impact is minimal. Sorting only occurs for the pool of reusable DOM elements changes. Even in a heavy scenario, say, 30 visible items plus a buffer above and below (around 50 elements total) the cost of sorting is negligible.
From my testing on a heavy AppleSauce account at
https://dev.new.expensify.com:8082/new/chatwrappingSectionListWithSectionsintoProfiler, there's not much of a measurable impact in such a heavy list, which is expected.First
#1 - #5are tests with the patch. Last#1, #3 #4are from main without the patch.As you can see, there's greater variability between measurements than between main and this patch.
Fixed Issues
$ #86126
It also should potentinally prevent a similar issue to #37447 with FlashList
Tests
Prerequisites: run those commands from project root before testing
You can test in on any list in the app that uses FlashList and has lots of elements.
Example test for
src/pages/iou/request/MoneyRequestParticipantsSelector.tsxOffline tests
QA Steps
Same as tests
I'm open to suggestions what type of QA testing is best here. In my opinion a general regression testing to check for bugs on pages that use FlashList.
A good place to check different places in the app that use FlashList can be found here
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: mWeb Safari
MacOS: Chrome / Safari
correct.FlashList.DOM.order.mp4