Skip to content

Fix: force FlashList to use natural DOM order on web#85825

Open
sharabai wants to merge 1 commit intoExpensify:mainfrom
software-mansion-labs:fix/flash-list/DOM-order
Open

Fix: force FlashList to use natural DOM order on web#85825
sharabai wants to merge 1 commit intoExpensify:mainfrom
software-mansion-labs:fix/flash-list/DOM-order

Conversation

@sharabai
Copy link
Copy Markdown
Contributor

@sharabai sharabai commented Mar 19, 2026

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/chat wrapping SectionListWithSections into Profiler , there's not much of a measurable impact in such a heavy list, which is expected.
Screenshot 2026-03-25 at 19 56 57
First #1 - #5 are tests with the patch. Last #1, #3 #4 are 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

rm -rf node_modules
npm i

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.tsx

  • Go to Inbox.
  • Use Screen reader to navigate the list up and down
  • Use JAWS on Windows or VoiceOver on MacOS to move around in the list
  • Go forward so the list goes over the buffer and (if you can see 20 elements, scroll to element # 40)
  • Go back to start
  • It should follow natural order of items in the list
  • Verify that no errors appear in the JS console

Offline 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

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I verified there are no new alerts related to the canBeMissing param for useOnyx
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
correct.FlashList.DOM.order.mp4

@github-actions
Copy link
Copy Markdown
Contributor

⚠️ This PR is possibly changing native code and/or updating libraries, it may cause problems with HybridApp. Please check if any patch updates are required in the HybridApp repo and run an AdHoc build to verify that HybridApp will not break. Ask Contributor Plus for help if you are not sure how to handle this. ⚠️

@sharabai sharabai changed the title Add patch Fix: force FlashList to use natural DOM order on web Mar 19, 2026
@sharabai
Copy link
Copy Markdown
Contributor Author

This patch starts with 003 because another one of my PRs takes 002.

@sharabai
Copy link
Copy Markdown
Contributor Author

⚠️ This PR is possibly changing native code and/or updating libraries, it may cause problems with HybridApp. Please check if any patch updates are required in the HybridApp repo and run an AdHoc build to verify that HybridApp will not break. Ask Contributor Plus for help if you are not sure how to handle this. ⚠️

FlashList isn't used in Mobile-Expensify, we're good here.

@sharabai sharabai marked this pull request as ready for review March 19, 2026 17:26
@sharabai sharabai requested a review from a team as a code owner March 19, 2026 17:26
@melvin-bot melvin-bot Bot requested review from eVoloshchak and removed request for a team March 19, 2026 17:26
@melvin-bot
Copy link
Copy Markdown

melvin-bot Bot commented Mar 19, 2026

@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 rushatgabhane requested review from shubham1206agra and removed request for eVoloshchak March 19, 2026 17:28
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +18 to +19
+ if (Platform.OS === "web") {
+ renderEntries.sort(([, a], [, b]) => a.index - b.index);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle inverted lists when forcing natural DOM order

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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VickyStash Can you please look into this when you use this prop?

Copy link
Copy Markdown
Contributor

@VickyStash VickyStash Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@sharabai sharabai Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested the patch over the main chat that uses FlashList with inverted flag and here are the results:

  1. 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
  1. But other than that, I've noticed nothing. Everything works the same way as before

Copy link
Copy Markdown
Contributor Author

@sharabai sharabai Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shubham1206agra
Copy link
Copy Markdown
Contributor

Screen.Recording.2026-03-19.at.11.35.42.PM.mov

BUG: Multiple items in LHN gets highlighted when scrolling fast (Not repro on staging)

@github-actions
Copy link
Copy Markdown
Contributor

🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here.

@sharabai sharabai force-pushed the fix/flash-list/DOM-order branch from 2ca24ea to c41dca9 Compare March 23, 2026 12:38
@sharabai sharabai requested a review from a team as a code owner March 24, 2026 16:58
@melvin-bot melvin-bot Bot requested review from joekaufmanexpensify and removed request for a team March 24, 2026 16:58
@sharabai sharabai changed the title Fix: force FlashList to use natural DOM order on web [WIP] Fix: force FlashList to use natural DOM order on web Mar 24, 2026
@sharabai sharabai marked this pull request as draft March 24, 2026 17:54
@sharabai sharabai changed the title [WIP] Fix: force FlashList to use natural DOM order on web Fix: force FlashList to use natural DOM order on web Mar 24, 2026
@sharabai sharabai force-pushed the fix/flash-list/DOM-order branch 3 times, most recently from 7dc3360 to 8640360 Compare March 25, 2026 18:18
@joekaufmanexpensify
Copy link
Copy Markdown
Contributor

No product review needed

@joekaufmanexpensify joekaufmanexpensify removed their request for review March 25, 2026 21:13
@sharabai sharabai force-pushed the fix/flash-list/DOM-order branch from 8640360 to 1f0f379 Compare March 30, 2026 18:08
@sharabai
Copy link
Copy Markdown
Contributor Author

sharabai commented Apr 2, 2026

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:

@VickyStash
if you haven't, first read this. With that in mind, yeah it's that 1000ms delay. if your scroll is so slow that it DOM sort happens after this delay - it does result in those stale hovers.
You can make the sort delay even longer, but then you come across the accessibility issue affecting people who use sreen readers: they can use scrolling (by some voice commands) and then ask screen readers to read the list or use the shortcut - and the list would be unsorted if they do it quickly enough. So 1000ms was kinda in place where most hovers wouldn't be stale and people using assistive technology would be able to navigate correctly.

@sharabai
Copy link
Copy Markdown
Contributor Author

sharabai commented Apr 2, 2026

It would be good to add to PR recordings a recording from the E/App, not from the expo snack example app.

@VickyStash
Oh, snap. I uploaded wrong video. It's from the FlashList issue I opened. Gotta change it. Thx for catching that!

@sharabai
Copy link
Copy Markdown
Contributor Author

sharabai commented Apr 2, 2026

@VickyStash
In general, the patch has the potential to introduce some of those stale hovers, but it solves the big issue of not having accessible navigation on web at all. On top of that, the stale hover issues are very marginal. You have to really try to cause them.
If you see a better approach or a completely different solution, I'm open to suggestions!

@sharabai
Copy link
Copy Markdown
Contributor Author

sharabai commented Apr 2, 2026

@VickyStash

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

Increasing the SORT_DELAY_MS would solve it, but would make the screenreaders to wait longer for the sort to read out the correct order of elements.
It's a trade-off between providing a quick enough update for screen readers and cause as few as possible of those stale hovers.

@sharabai
Copy link
Copy Markdown
Contributor Author

sharabai commented Apr 2, 2026

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

The first part about hovering effect doesn't follow - I'm sure it's the same on staging too, the second part that introduces a stale hover - that's just part of the drawback fo thhe current approach.

@VickyStash
Copy link
Copy Markdown
Contributor

Thank you for the explanations!
If it's known and expected, I'm okay with that!
cc @shubham1206agra for thoughts as he is a C+ here

@VickyStash
Copy link
Copy Markdown
Contributor

Let's just make sure there are no other side effects we're unaware of.

@mountiny
Copy link
Copy Markdown
Contributor

@shubham1206agra can you prioritize this PR please?

@sharabai sharabai force-pushed the fix/flash-list/DOM-order branch from 30e5ba1 to 5e07bb1 Compare April 13, 2026 13:47
@mountiny
Copy link
Copy Markdown
Contributor

@shubham1206agra bump

@shubham1206agra
Copy link
Copy Markdown
Contributor

@mountiny Can you pass this PR off to someone else for review?

@mountiny mountiny requested review from huult and removed request for shubham1206agra April 15, 2026 05:43
@huult
Copy link
Copy Markdown
Contributor

huult commented Apr 16, 2026

@sharabai please resovle the conflict

@huult
Copy link
Copy Markdown
Contributor

huult commented Apr 16, 2026

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@huult
Copy link
Copy Markdown
Contributor

huult commented Apr 16, 2026

Screen.Recording.2026-04-16.at.12.12.29.mov

BUG: UI issue in chat search

@huult
Copy link
Copy Markdown
Contributor

huult commented Apr 16, 2026

Screen.Recording.2026-04-16.at.12.12.29.mov
BUG: UI issue in chat search

After clearing and rebuilding, the issue is gone.

@huult
Copy link
Copy Markdown
Contributor

huult commented Apr 16, 2026

Screen.Recording.2026-04-16.at.15.01.23.mov

@sharabai When moving down the list using the arrow keys, I see flickering and items jumping positions. The order doesn't remain stable.

@huult
Copy link
Copy Markdown
Contributor

huult commented Apr 16, 2026

Screen.Recording.2026-04-16.at.15.09.40.mov

@sharabai When I use arrow up to reach the first item and press arrow up again, the focus moves to the last item. After that, the list becomes unstable and items jump around.

@huult
Copy link
Copy Markdown
Contributor

huult commented Apr 16, 2026

Screen.Recording.2026-04-16.at.15.15.39.mov

@sharabai The list behaves incorrectly when using Arrow Down/Up to navigate from the first item to the last item. This issue does not occur on staging.

@huult
Copy link
Copy Markdown
Contributor

huult commented Apr 16, 2026

@sharabai If you want to test with a high-traffic account like I did, you can refer to this guide: https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#high-traffic-accounts

@huult
Copy link
Copy Markdown
Contributor

huult commented Apr 20, 2026

@sharabai Do you have any updates on this?

@sharabai
Copy link
Copy Markdown
Contributor Author

@huult I'll get back to you shortly 😃.

@sharabai sharabai force-pushed the fix/flash-list/DOM-order branch from 65df099 to aeaf3ca Compare April 20, 2026 17:03
@huult
Copy link
Copy Markdown
Contributor

huult commented Apr 21, 2026

Screenshot 2026-04-21 at 09 18 27

@sharabai Please avoid using force push anymore

Screenshot 2026-04-21 at 09 20 04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants