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

lanitochka17 commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Open staging.new.expensify.com with Safari
Login in any account
Go to any chat with multiple attachments
Select any attachment
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
[x] 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

https://github.com/Expensify/App/assets/78819774/788b44a2-3d0e-4ab4-acab-9894019f42d4

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[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

CortneyOfstad commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

univalchemist commented 1 year ago

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 commented 1 year ago

@lanitochka17 maybe this issue is related to this one https://github.com/Expensify/App/issues/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 commented 1 year ago

@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 commented 1 year ago

Not overdue 👍

kidroca commented 1 year ago

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 commented 1 year ago

Not overdue

univalchemist commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

📣 @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:

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.
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.
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[bot] commented 1 year ago

⚠️ 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[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

kidroca commented 1 year ago

In response to: https://github.com/Expensify/App/issues/22318#issuecomment-1634256318 @AshenI99, we utilize the pagingEnabled property to facilitate the horizontal pagination of attachments, displaying them one at a time.

The existing documentation for this property might be somewhat terse, but it serves a key purpose: it allows for a portion of the view to be scrolled through. In our scenario, we scroll through exactly one page (equivalent to one ScrollView size), and this functions correctly most of the time.

What leads you to the conclusion that removing pagingEnabled would resolve the issue? Wouldn't this modification also disrupt the current paging behavior?

AshenI99 commented 1 year ago

Hi @kidroca,

You are right about that paging behavior when it comes to mobile devices. So, if we completely remove that, it'll affect the paging behavior of the mobile devices. However, when it comes to non-touch screens, paging behavior won't be an issue as a user is switching through attachments with arrows. So, it'll only switch one image at a time.

Therefore, we need to disable the pagingEnabled only when it is a non-touchscreen such as desktops and laptops. So it'll be as follows,

pagingEnabled={this.canUseTouchScreen}

https://github.com/Expensify/App/assets/73756207/8fa9ecdc-eb18-4cdf-afc0-4e8dbd94d28d

AshenI99 commented 1 year ago

Proposal

Updated

kidroca commented 1 year ago

@AshenI99

You are right about that paging behavior when it comes to mobile devices. So, if we completely remove that, it'll affect the paging behavior of the mobile devices. However, when it comes to non-touch screens, paging behavior won't be an issue as a user is switching through attachments with arrows. So, it'll only switch one image at a time.

I appreciate your observations. However, I need a bit more clarification on the specific role of pagingEnabled in causing the issue at hand. You've pinpointed it as the root of the problem, yet an in-depth explanation is missing.

Could you shed light on the exact purpose of pagingEnabled and why you believe it is only relevant to touch screens? Also, it's worth noting that even on touch screen devices, we still offer the arrow buttons to facilitate navigation. Wouldn't those users be susceptible to encountering the same bug as well?

From my perspective, this is beginning to appear like a bug with how pagingEnabled functions in react-native-web, or perhaps it's related to our particular blend of property settings in conjunction with pagingEnabled.

AshenI99 commented 1 year ago

@kidroca

Yes. I think you are right. It could be either bug with how pagingEnabled functions in react-native-web, or it could related to our particular blend of property settings in conjunction with pagingEnabled.

Also this issue is happening in mobile browsers too. That case it is happening when we use swipe action to swipe through images instead of using arrows.

kidroca commented 1 year ago

Seems the following react-native-web bug might be related

https://github.com/necolas/react-native-web/issues/2026

The video provided in the issue description depicts a behavior with content jumping to the start of the list as the user scrolls to the right. I think it's similar to what happens in our case. The content seem to jump to the start of the virtual list

CortneyOfstad commented 1 year ago

@rushatgabhane thoughts on the above? ^^^

CortneyOfstad commented 1 year ago

@rushatgabhane Bump ^^^

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@CortneyOfstad @rushatgabhane this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

CortneyOfstad commented 1 year ago

@rushatgabhane any update on the above?

rushatgabhane commented 1 year ago

Agree! It looks like it's an underlying issue with react native web https://github.com/necolas/react-native-web/issues/2026

CortneyOfstad commented 1 year ago

@rushatgabhane Do we think any of the proposals above could help with this, or do we still need some new ones?

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@CortneyOfstad @rushatgabhane this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

univalchemist commented 1 year ago

See here? No anyone suggests a good solution than this I am sure there is not any good way to fix this issue than my proposal.

CortneyOfstad commented 1 year ago

@rushatgabhane Do we think any of the proposals above could work?

CortneyOfstad commented 1 year ago

Heading OoO until 8/14, so reassigning BZ to keep an eye on this.

@rushatgabhane bump on the proposals above ^^^^

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

rushatgabhane commented 1 year ago

waiting for proposals to fix

https://github.com/necolas/react-native-web/issues/2026

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@CortneyOfstad @rushatgabhane @MitchExpensify this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @rushatgabhane is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

@CortneyOfstad, @rushatgabhane, @MitchExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

MitchExpensify commented 1 year ago

Is this still the latest on this one @rushatgabhane ?

waiting for proposals to fix

https://github.com/necolas/react-native-web/issues/2026

MitchExpensify commented 1 year ago

Friendly bump on this question @rushatgabhane

rushatgabhane commented 1 year ago

waiting for proposals to fix https://github.com/necolas/react-native-web/issues/2026

@MitchExpensify yes! this is the latest - https://github.com/Expensify/App/issues/22318#issuecomment-1660967959

CortneyOfstad commented 1 year ago

Hi @MitchExpensify -- back from OoO so I can take this back over. Thanks for holding down the fort while I was gone 👍

MitchExpensify commented 1 year ago

Sounds gooood! Thanks, @CortneyOfstad

CortneyOfstad commented 1 year ago

No updates yet 👍

CortneyOfstad commented 1 year ago

@rushatgabhane Just checking to see where we're at with this — TIA!

Expensify / App