Repository navigation
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
|
|
Kindly bump @eh2077 for review. Thanks |
|
thanks for the bump! Reviewing |
Screen.Recording.2026-04-28.at.8.46.09.PM.mov@NJ-2020 It doesn't work well
|
|
@eh2077 As mentioned here, the current chat |
|
Oh I see, thanks! Then I think we should hold for the other PR. |
|
@NJ-2020 The FlashList DOM order patch in question is specifically aimed at accessibility issues with tab navigation and screen readers. I'm not sure it will help here, at least not by itself. My fix works by sorting immediately in case of navigation by tabbing (it catches focusIn events), so as a workaround here you could call It works in a pretty hacky way. The DOM sorting is not run immediately, but delayed with setTimeouts (it's necessary because of stale hover issues otherwise). |
|
@sharabai Thank you for informing us, I'll try to look into it soon. |
|
Sorry for the delay Regarding the FlashList package, we have some solutions to fix it:
Pros: Fast
Pros: Work even while scrolling
Pros: Fast |
@NJ-2020 hi, the FlashList patch I'm working on does exactly that. If you want to know if my patch is going to work for your needs, the simplest way - copy the patch file to the shopify/flash-list patch folder, run My approach to the visual bug problem was to limit the sorting - only do it when necessary. |
|
Oh, thank you, hopefully your next PR/patch will also resolve this issue - in the meanwhile I'll try also to look for another solution. |
|
@NJ-2020 Let's update to move it forward, thx! |
|
@NJ-2020 The short summary: my patch was aimed to solve accessibility issues and it does that, but it would not help in your case. I went with the least amount of changing the behavior of the FlashList, which in turn seems to not be enough for your needs. Although, imo, if we're alright with not having any visible hover in chats, you could go with the second approach. Or, like you'd said, you'd need to diverge heavily from the current FlashList architecture (3. Instead of updating/recycling the old item, remove the old item), which would work, but it'd heavily impact the performance. The longer version: The reason why my patch likely won't help your case is that it works similarly to this:
with a caveat that it does behave like this when it detects focus events: Your second proposed approach (2. Sort the rendered items) would have the bugs that I mentioned.
The issues I mentioned (stale hover) seem to be not resolvable without some drastic change in architecture of the FlashList itself. Alternatively, you third outlined approach, "3. Instead of updating/recycling the old item, remove the old item", would work but would also tank the performance of the FlashList. |
Okay
Thanks for the info. |
|
Hi − the bug is no longer reproducible for me: Brave Screen.Recording.2026-07-05.at.13.57.58.movOne thing I notice on Brave is where we need to explicitly select the message text when selecting multiple messages, else it wont' work. Safari Screen.Recording.2026-07-05.at.13.57.25.movcc: @eh2077 Thanks. |
|
Bump @eh2077 |
|
Hmm QA is still able to reproduce, but to confirm were you testing prod env on your branch? Or real prod |
|
Yes, as you can see from this video, the QA is able to select multiple messages when starting from text properly, not the container/div But if you select like from the message row/similar, it won't work
cc: @Julesssss |
|
Bump @eh2077 |
Hmm, it seems bad UX to select multiple messages in this strict way. We still can't copy messages properly. Screen.Recording.2026-07-27.at.10.06.37.PM.mov |
@NJ-2020 any new finding about the solution? The instant sorting method looks promising (without performance issue) but it seems not good to sacrifice hover feature of charts. |
|
@NJ-2020 gentle bump |
|
@NJ-2020 bump! |
|
Apologies for the delay For that bug, we need to update this condition to disable the role button if it's web, i.e not limited to safari-only accessibilityRole={Platform.OS === CONST.PLATFORM.WEB && hasHoverSupport() ? undefined : CONST.ROLE.BUTTON}I'll try to post more update soon. Sorry for the inconvenience. |
|
hey @NJ-2020, any updates? |
Explanation of Change
Fix
accessibilityRoleforPureReportActionItemby passingundefinedfor web onlyFixed Issues
$ #37447 (comment)
PROPOSAL: #37447 (comment)
Tests
Verify both Chrome & Safari
Offline tests
Same as tests
QA Steps
Same as tests
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
21.mp4
Android: mWeb Chrome
22.mp4
iOS: Native
Screen.Recording.2026-04-21.at.14.09.59.mov
iOS: mWeb Safari
Screen.Recording.2026-04-21.at.14.19.26.mov
MacOS: Chrome / Safari
Screen.Recording.2026-04-21.at.11.09.31.mov