[$2000] Android- PDF Scrolling Issue: Difficulty in Navigating Through Pages

[kavimuru]

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:

1 Initiate a chat with another user 2 Go to + > add attachment >choose document > upload a multipage pdf file 3 Open the pdf file and scroll down and up 4 Verify if its smooth 5 Now, upload an image 6 Go back to the pdf file and try to scroll up-down 7 Verify if the image opens (Next image is selected when swiping the screen up and down)

Expected Result:

Smooth interaction with pdf. No difficulty in scrolling pdf pages

Actual Result:

Difficulty in scrolling through pdf. Sometimes another document opens instead of scrolling

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• [x] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [ ] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.3.44-0 Reproducible in staging?: y Reproducible in production?: y 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/43996225/319bec3f-9bdd-4090-a0a5-f4b038e23d5e

https://github.com/Expensify/App/assets/43996225/90582094-35e4-448d-8293-f2cd77bd6710

Expensify/Expensify Issue URL: Issue reported by: @avi-shek-jha Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690006696574999

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ef34bf6d94d34d3b
• Upwork Job ID: 1686884315196669952
• Last Price Increase: 2023-10-18
• Automatic offers:
• HezekielT | Contributor | 27284015
• avi-shek-jha | Reporter | 27284018

[melvin-bot[bot]]

📣 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]]

@johncschuster, @mollfpr Eep! 4 days overdue now. Issues have feelings too...

[mollfpr]

Not overdue, will review the latest proposal in the morning.

[melvin-bot[bot]]

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

[studentofcoding]

Hey @mollfpr, do you have any feedback on my proposal?

[mollfpr]

Sorry for the delay!

@studentofcoding I tried your solution but didn't find it working. Also, the issue in upstream you attached has a different problem with this.

Upon checking and add <ScrollView contentContainerStyle={{ flex: 1 }}> for wrapping PDFView it fixed, the problem

You should have specified the reason why this can fix the problem.

[melvin-bot[bot]]

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

[mollfpr]

Not overdue, still waiting for other proposals.

[melvin-bot[bot]]

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

[johncschuster]

What are your thoughts on this one, @mollfpr?

[melvin-bot[bot]]

@johncschuster, @mollfpr Huh... This is 4 days overdue. Who can take care of this?

[mollfpr]

Still looking for a better solution here.

[melvin-bot[bot]]

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

[johncschuster]

Posted in #expensify-open-source to get more proposals

[HezekielT]

Proposal

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

Android- PDF Scrolling Issue: Difficulty in Navigating Through Pages

What is root cause of that problem?

We show attachments using PagerView for native devices.

https://github.com/Expensify/App/blob/62cc10934aff490028c9b794ae0e8631fff2df9a/src/components/Attachments/AttachmentCarousel/index.native.js#L146

https://github.com/Expensify/App/blob/62cc10934aff490028c9b794ae0e8631fff2df9a/src/components/Attachments/AttachmentCarousel/Pager/index.js#L13

This PagerView scroll event is enabled(disabled) depending on the value of e.offset as shown below

https://github.com/Expensify/App/blob/62cc10934aff490028c9b794ae0e8631fff2df9a/src/components/Attachments/AttachmentCarousel/Pager/index.js#L96

This means an offset value of as small as 0.001 is enough to set isScrolling to true enabling the user to swipe left or right and open different attachments.

Now when the user scrolls up and down, even if the scroll is in Y direction, a small change in the offset value of X causes the PagerView to be active resulting in a conflict between the pager scroll event and react-native-pdf’s scroll event.

This issue is reproducible only on android because of the fact that e.offset is easily changed on android even while scrolling in the Y direction. However, on ios, the value doesn't change easily unless we scroll in X direction to trigger it. We can see this difference by console.log(e.offset) inside pagerScrollHandler while scrolling a pdf attachement up and down.

This is the root cause cause.

The issue is also not reproducible once we zoom in on the pdf attachment. This is due to the fact that we disable PagerView scroll by setting shouldPagerScroll to false inside onScaleChanged method which in turn disables scrolling by setting scrollEnabled to false.

https://github.com/Expensify/App/blob/fe282b45cb13e01519282ccc023e5bfbd7714158/src/components/Attachments/AttachmentView/AttachmentViewPdf/index.native.js#L22

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

We can solve this issue simply by setting attachmentCarouselPagerContext.shouldPagerScroll.value = false when the user scrolls up and down and set it to true again if the user is swiping in the x direction and if the scale is 1(not zoomed in) for android since the issue occurs only on android.

In order to implement the above solution, We first need to rename the AttachmentViewPdf(index.native.js) file to BaseAttachmentViewPdf.js and then create index.ios.js and index.android.js files.

• For iOS(index.ios.js), Simply call BaseAttachmentViewPdf

  return (
  <BaseAttachmentViewPdf
      // eslint-disable-next-line react/jsx-props-no-spreading
      {...props}
  />
  )

• For android(index.android.js), do the following

1. We need to use Gesture.Pan() so that we can detect the direction of swipe and enable(disable) shouldPagerScroll accordingly.

Code reference

``` const scaleRef = useSharedValue(1) const offsetX = useSharedValue(0); const offsetY = useSharedValue(0); .... const panGesture = Gesture.Pan() .manualActivation(true) .onTouchesMove((evt) => { if(offsetX.value !== 0 && offsetY.value !== 0) { // if the value of X is greater than Y and the pdf is not zoomed in, enable the pager scroll so that the user // can swipe to the next attachment otherwise disable it. if((Math.abs(evt.allTouches[0].absoluteX - offsetX.value) > Math.abs(evt.allTouches[0].absoluteY - offsetY.value)) && scaleRef.value === 1){ attachmentCarouselPagerContext.shouldPagerScroll.value = true } else { attachmentCarouselPagerContext.shouldPagerScroll.value = false } } offsetX.value = evt.allTouches[0].absoluteX; offsetY.value = evt.allTouches[0].absoluteY; }) ``` - We need to update the value of `scaleRef` inside `onScaleChanged` so that we can have the correct of value of the pdf's scale value. - There are different methods we can use instead of `onTouchesMove` such as `onChange`, `onUpdate` etc but I think `onTouchesMove` is enough since it does the job.

3. Next, we need to wrap <BaseAttachmentViewPdf> component by GestureDetector so that we can detect the gesture.

Code reference

``` ```

There are different ways of detecting the gesture. Regardless of the approach we use, setting shouldPagerScroll to false when the swipe is in Y direction and setting it true when it is in X direction solves this issue.

What alternative solutions did you explore? (Optional)

Result

https://github.com/Expensify/App/assets/39636266/4e1f35b4-2b19-4b3d-a4c0-db91f3fdc205

[johncschuster]

@mollfpr what do you think of the above proposal?

[mollfpr]

@HezekielT I just tested your proposal on an Android physical device, and it's working well, but on the iOS simulator, I can swipe left/right on the PDF to navigate to other attachments.

https://github.com/Expensify/App/assets/25520267/a6537a34-9a07-4d6c-9b62-ef9d1e121e99

[HezekielT]

Proposal

Updated

cc @mollfpr

[mollfpr]

@HezekielT Can we move the current index.native.js to a new base component so we don't need to do the same twice for index.ios.js and index.android.js?

[HezekielT]

@mollfpr Yes, we can do that. We can put the current logic of index.native.js in a file called BaseAttachmentViewPdf.js and then for iOS, we simply call

<BaseAttachmentViewPdf
        // eslint-disable-next-line react/jsx-props-no-spreading
        {...props}
/>

while for android we will wrap it by GestureDetector

...
<GestureDetector gesture={Pan}>
...
        <BaseAttachmentViewPdf
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...props}
        />
</GestureDetector>

Another approach would be to conditionally render the component in BaseAttachmentViewPdf.js

shouldDetectGesture ? (
<GestureDetector gesture={Pan}>
...
        <PDFView />
</GestureDetector>
) : (
<PDFView />
)

Where shouldDetectGesture will be true for android and false for ios.

What do you think?

[melvin-bot[bot]]

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

[HezekielT]

@mollfpr I have updated my proposal based on your suggestion. Please take a look again when you get the chance.Thanks!

Proposal

UPDATED

[mollfpr]

Okay, the proposal looks good, and we can improve the code while reviewing the PR so it will be easier. Let's go with @HezekielT proposal, then.

🎀 👀 🎀 C+ reviewed!

@johncschuster Could you help add the Engineering label? Thanks!

[melvin-bot[bot]]

Triggered auto assignment to @Gonals (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[melvin-bot[bot]]

📣 @HezekielT 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot[bot]]

📣 @avi-shek-jha 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[avi-shek-jha]

Hello, This issue was reported on July 23 before the new changes took place. Can someone please send me the corrected offer as I received the one for 50 dollars instead of 250 for older issues?

[mollfpr]

@HezekielT Let me know when the PR is ready because Melvin will not request my review. Thank you!

@avi-shek-jha I think @johncschuster will take care of that.

[HezekielT]

@mollfpr PR is ready for review.

[johncschuster]

@avi-shek-jha, I've adjusted your offer to account for the previous Bug Reporter amount. Thanks for calling that out!

[avi-shek-jha]

@avi-shek-jha, I've adjusted your offer to account for the previous Bug Reporter amount. Thanks for calling that out!

Thank you John. Accepted.

[melvin-bot[bot]]

@johncschuster, @Gonals, @mollfpr, @HezekielT Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

@johncschuster, @Gonals, @mollfpr, @HezekielT 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot[bot]]

@johncschuster, @Gonals, @mollfpr, @HezekielT Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[melvin-bot[bot]]

@johncschuster, @Gonals, @mollfpr, @HezekielT 12 days overdue. Walking. Toward. The. Light...

[mollfpr]

Not overdue. The PR is merged.

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[johncschuster]

Oh boy, I literally just paid this out too 😅.

@Gonals, I'll follow your lead on this one.

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @johncschuster, @Gonals, @mollfpr, @HezekielT eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[mollfpr]

@johncschuster I think this was ready for payment. Could you give the payment summary to make a manual request? Thanks!

[mollfpr]

The PR that introduced the bug has been identified. Link to the PR: The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

No offending PR. This difference in behavior between Android and iOS is causing the issue.

A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

The regression step should be good.

Determine if we should create a regression test for this bug. If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

1. Open any report
2. Click on + of the composer.
3. Select Add attachment > Choose document > upload a multi-page pdf file.
4. Open the pdf file and scroll up and down.
5. Verify the scroll is smooth.
6. Close the attachment and Click on + of the composer again.
7. Upload an image.
8. Open the image and swipe to the left.
9. Verify the previously sent pdf is opened.
10. Scroll through the pdf up and down.
11. Verify the scroll is smooth.

---

Friendly bump @johncschuster

[mollfpr]

Friendly bump @johncschuster

[mollfpr]

Friendly bump @johncschuster

[johncschuster]

Payment Summary

Bug reporter @avi-shek-jha - $250 - PAID via Upwork ✅ Contributor that fixed the issue: @HezekielT - $2000 - PAID via Upwork ✅ C+ that reviewed the issue: @mollfpr - $2000 - needs to be paid via manual request

[JmillsExpensify]

$2,000 payment approved for @mollfpr based on summary above.