[ON HOLD — #23546][$1000] Chat - Several images at once are skipped when switching attachments with arrows

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open staging.new.expensify.com with Safari
2. Login in any account
3. Go to any chat with multiple attachments
4. Select any attachment
5. Switch attachments with on-screen arrows

Expected Result:

Only the next attachment is loaded by clicking on the on-screen arrow

Actual Result:

Several images at once are skipped when switching attachments with on-screen arrows

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.37.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug6117796_Safari-Attachments-Preview-Skip.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f9c0565b91f7e6d4
• Upwork Job ID: 1677033938685763584
• Last Price Increase: 2023-08-03

[melvin-bot]

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[CortneyOfstad]

Was able to recreate, so going to get eyes on this 👍

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01f9c0565b91f7e6d4

[melvin-bot]

Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

[univalchemist]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Chat - Several images at once are skipped when switching attachments with arrows

What is the root cause of that problem?

The prop maxToRenderPerBatch of FlastList cause this issue. As mentioned in official React Native documentation, this prop maxToRenderPerBatch is a VicualizedList prop that can be passed through. The current modal attachment list is horizontal, so don't need this one.

What changes do you think we should make in order to solve the problem?

Removing prop maxToRenderPerBatch is a solution here .

What alternative solutions did you explore? (Optional)

N/A

Here is a video link after fixed:
https://www.loom.com/share/bc5bb3fd5ec54a7daa52b405971688f1

[uafrontender]

@lanitochka17 maybe this issue is related to this one #21177

@univalchemist as I understand Removing prop maxToRenderPerBatch is setting it to default value. And after this there is that default Safari bug anyway. You can find it in your Loom video too. When you click on any image, 1st one displayed is image with cat (last image in your attachments) and then loaded correct image.

[univalchemist]

@mkarkachov , removing prop maxToRenderPerBatch is not setting it to the default value. It does not have any default value and removing it means undefined. Yes, this is just issue on Safari and it is related with this prop. Also notice that this prop affects in only the virtual list.

[CortneyOfstad]

Not overdue 👍

[kidroca]

The maxToRenderPerBatch parameter is not added by accident; the chosen value plays a crucial role. Despite the fact that we only see one item at a time, two additional items are rendered on each side (outside of view). This effectively preloads the subsequent and previous attachments, preparing the content that the user is likely to interact with next.

The list's orientation, horizontal in this case, does not provide a valid reason for eliminating this property. The maxToRenderPerBatch parameter is a vital part of virtualization, and it operates irrespective of the type of the list.

Furthermore, this property has a default value of 10 (see maxToRenderPerBatchOrDefault). This implies that removing the property would not solve the issue but merely displace it. It might also degrade loading as we'll start loading 10 images shortly after the carousel is opened

Erasing the property or augmenting the value of maxToRenderPerBatch only masks the issue. This is because the problem would then only surface for conversations encompassing more than 10 attachments.

[CortneyOfstad]

Not overdue

[univalchemist]

The maxToRenderPerBatch parameter is not added by accident; the chosen value plays a crucial role. Despite the fact that we only see one item at a time, two additional items are rendered on each side (outside of view). This effectively preloads the subsequent and previous attachments, preparing the content that the user is likely to interact with next.

The list's orientation, horizontal in this case, does not provide a valid reason for eliminating this property. The maxToRenderPerBatch parameter is a vital part of virtualization, and it operates irrespective of the type of the list.

Furthermore, this property has a default value of 10 (see maxToRenderPerBatchOrDefault). This implies that removing the property would not solve the issue but merely displace it. It might also degrade loading as we'll start loading 10 images shortly after the carousel is opened

Erasing the property or augmenting the value of maxToRenderPerBatch only masks the issue. This is because the problem would then only surface for conversations encompassing more than 10 attachments.

Ok, let's see who suggests another solution then!

[AshenI99]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Chat - Several images at once are skipped when switching attachments with arrows

What is the root cause of that problem?

The pagingEnabled property in FlatList caused the error. According to the react-native docs(Here), when pagingEnabled property is true, the scroll view stops on multiples of the scroll view's size when scrolling. So, it seems like it is automatically scrolling when the next set of images are loaded.

However, pagingEnabled property is necessary for mobile devices to use the paging behavior when swiping through images. So, we cannot completely remove that.

What changes do you think we should make in order to solve the problem?

Setting the pagingEnabled property to false only for non-touch screens will fix the issue. (Here)

pagingEnabled={this.canUseTouchScreen}

What alternative solutions did you explore? (Optional)

None

[melvin-bot]

📣 @AshenI99! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[melvin-bot]

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[CortneyOfstad]

@kidroca any updates?

Also, @Sourcecodedeveloper, we have someone assigned already, but thank you for reaching out!

[kidroca]

Hi team,

I wanted to provide a quick status update on the ticket regarding the carousel bug we've been tackling. Despite implementing the ScrollView strategy I previously proposed, the issue persisted, leading me to conduct a deeper investigation. It became clear that the problem wasn't specific to the use of ScrollView or FlatList; rather, it was rooted in how Safari on iOS manages snap alignment when cycling through attachments. I've shared a detailed analysis on a related ticket, given its relevance to the discussion there.

Ticket History Summary:

• My initial investigation pointed towards react-native-web as the culprit: [ON HOLD — #23546][$1000] Chat - Several images at once are skipped when switching attachments with arrows #22318 (comment)
• Current ticket was linked to several similar issues
  • [HOLD for payment 2024-06-06] [$2000] Wrong Image opens on HT Report in Safari #23546 (comment)
• We experimented with replacing FlatList with FlashList, which introduced a different set of challenges: FlashList Attempt Summary.
• Presently: Substituting FlatList with a plain ScrollView also failed to resolve the bug

The core of the issue has been identified as CSS snap alignment of scroll items, as elaborated in my recent analysis

• [HOLD for payment 2024-06-06] [$2000] Wrong Image opens on HT Report in Safari #23546 (comment)

I'm currently exploring alternatives and keeping an eye on proposals in related tickets. I'll ensure to keep everyone updated with any new developments or insights.

Thank you for your patience and collaboration as we work through this complex issue.

[CortneyOfstad]

Thank you @kidroca for the detailed explanation! This is incredibly thorough, and if there is anything that we can do to help in support, do not hesitate to ask! Within BZ, I will also keep an eye out for anything related that could help 👍

[kidroca]

Hi team,

I'm reaching out to share an important update on the carousel bug across several related tickets. I realized I might not have communicated this breakthrough in all relevant discussions, so I wanted to ensure everyone is on the same page.

Exciting news: I've found and proposed a straightforward solution that effectively tackles the attachment carousel issue and simultaneously addresses the related challenges we encountered. For a detailed account of the steps taken and the rationale behind them, please refer to my comprehensive summary here: Issue Summary and Solution, which is currently pending a C+ review.

The essence of the solution lies in our ability to manually manage scroll and panning interactions, thereby circumventing the pitfalls associated with scroll snap alignment in virtual lists. This approach has shown promising results in our initial tests, offering a solid fix for the issues we've been diligently working to resolve.

Thank you for your continued patience and collaborative spirit as we navigate these challenges together. I'll keep you all updated with any further developments.

[CortneyOfstad]

Thank you so much @kidroca!

[CortneyOfstad]

Bumped the on-hold GH here. Will follow-up as soon as they respond 👍

[CortneyOfstad]

Update was posted here

[CortneyOfstad]

Draft PR was created here! 🎉

[CortneyOfstad]

PR is still in draft but there is a stale PR in the on-hold GH, so reaching out to Scott (engineer on the main GH) to help move this along here

[CortneyOfstad]

Just bumped the on-hold issue, as the PR is ready for review!

[CortneyOfstad]

Bumped here

[CortneyOfstad]

On the on-hold GH, it was indicated that the PR would be merged early next week — will continue to keep an eye on that 👍

[CortneyOfstad]

Bumped the on-hold GH to see if we're still on pace for this week — will follow up as soon as I hear back 👍

[CortneyOfstad]

Merge freeze in place, so having to wait a bit for the on-hold issue

[CortneyOfstad]

PR comment here indicates potential merge today 🎉 Bumped the main GH and will continue to monitor

[CortneyOfstad]

We're in a merge freeze right now, so this will be delayed a bit 👍

[CortneyOfstad]

Merge freeze ends today! 🤞

[CortneyOfstad]

Was deployed to production 4 days ago 😍

[CortneyOfstad]

Set to be out of the 7-day window tomorrow 🥳

[CortneyOfstad]

This can be closed as the on-hold issue has been resolved/completed! Wahooo!