[HOLD for payment 2023-07-24] [$1000] Android - Attachment - Unable to scroll PDF files in preview

[lanitochka17]

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. Launch New Expensify app
2. Go to any chat
3. Click on the PDF file
4. Scroll down the file

Expected Result:

User is able to swipe to scroll down the file.

Actual Result:

User is unable to scroll down the file, unless pinching in while scrolling is performed.

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

Reproducible in staging?: Yes

Reproducible in production?: No

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

https://expensify.testrail.io/index.php?/tests/view/3497642

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

Screen_Recording_20230616_002453_New.Expensify.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/~019bb267c27b2966de
• Upwork Job ID: 1669733353046675456
• Last Price Increase: 2023-06-23

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

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

[melvin-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.

[mountiny]

given the size of the deploy and the urgency to get this out this is one of two/three remaining blockers. I am not sure if its big enough to hold on this if its just android and user can downlaod the PDF

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019bb267c27b2966de

[melvin-bot]

Triggered auto assignment to @greg-schroeder (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[hoangzinh]

Proposal

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

Android - Attachment - Unable to scroll PDF files in preview

What is the root cause of that problem?

• When we render a PDFView component, if we have props.onPress, we will wrap it into a Pressable component. Also disable Pressable when loadComplete
  

  App/src/components/AttachmentView.js

  Lines 96 to 99 in 83ab016

  	return props.onPress ? (
  	<PressableWithoutFeedback
  	onPress={props.onPress}
  	disabled={loadComplete}

  
  This logic is introduced since this commit. It's a refactoring commit so there are not much info for me to understand reasons behind. But I think the reason is, from beginning, the PDF file is not load yet, we need Pressable to show arrow button, but when PDF file is loaded completely, we need to disable Pressable to prevent PanResponder conflict between Pressable and PDFView.

• There is a change in this PR, it changed from Pressable to PressableWithoutFeedback component. Unluckily, in PressableWithoutFeedback component, we spread out disabled out of rest props, so when we pass it to the Pressable component inside, we didn't pass disabled props to Pressable => The logic to disable PressableWithoutFeedback when PDF file loaded doesn't work.

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

I think instead of disable PressableWithoutFeedback when PDF file is loaded completely, we can don't wrap PDFView inside PressableWithoutFeedback. That mean in this line, we can change to

props.onPress && !loadComplete ? <PressableWithoutFeedback ... /> : children

What alternative solutions did you explore? (Optional)

If the PressableWithoutFeedback is needed to handle PanResponder in some cases, we can leave as it is. And we should pass disabled={isDisabled} to Pressable component before this line

App/src/components/Pressable/GenericPressable/BaseGenericPressable.js

Line 165 in 83ab016

{...rest}

So it will keep old logic work as expected.

[robertKozik]

I want to clarify the reasoning behind not passing the disabled state into react-native Pressable component. When pressable gets disabled={true} we are loosing the possibility to alter the cursor appearance on hovering (so we cannot show disabled cursor). This said I was opting for not passing disabled state directly, but to check the disabled state both, when passing, and also inside, the event handlers.

[abdulrahuman5196]

Will review this today

[greg-schroeder]

Bump @abdulrahuman5196!

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @hoangzinh got assigned: 2023-06-27 16:08:01 Z
• when the PR got merged: 2023-07-13 20:02:01 UTC
• days elapsed: 12

On to the next one 🚀

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.41-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix unable to scroll PDF files in preview #21714

If no regressions arise, payment will be issued on 2023-07-24. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[abdulrahuman5196]

I don't think this issue should have penalty. We found an another issue during implementation and was discussing and waiting on @deetergp 's confirmation here - #21714 (comment)

[abdulrahuman5196]

BZ Checklist:

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

#20671 (comment)

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.

No

[abdulrahuman5196]

@greg-schroeder BZ checklist is complete here #20867 (comment)

[greg-schroeder]

Thanks @abdulrahuman5196! Taking care of payment now (not applying penalty/bonus)

[greg-schroeder]

Offer sent @hoangzinh @abdulrahuman5196

[greg-schroeder]

No regression test required, so closing this. Will finalize payments as soon as you both accept. :)

[abdulrahuman5196]

Accepted the offer @greg-schroeder

[hoangzinh]

Accepted the offer. Thanks @greg-schroeder

[hoangzinh]

Waiting on payment.

[greg-schroeder]

Payments complete:

@hoangzinh $1000 - C
@abdulrahuman5196 $1000 - C+

[lanitochka17]

Issue reproducible with Build 1.3.48-0

Screen_Recording_20230801_231349_New.Expensify.mp4

[greg-schroeder]

@hoangzinh @abdulrahuman5196 thoughts on the above?

[abdulrahuman5196]

@greg-schroeder This is not that the PDF not able to scroll., but the scrolling is not smooth. This issue of scroll being not smooth has been present for long time. I also noticed and mentioned the same during the review. The PDF is scrolling but clashing with horizontal and vertical scroll. We should create a separate issue to improve the scroll experience.

[greg-schroeder]

Okay, I thought as much. thanks