[HOLD for payment 2024-11-05] [$250] Android - Attachment - Android - I can to switch to another attachment in PDF zoomed state

IuliiaHerets commented 1 month 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!

Version Number: 9.0.40-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: https://expensify.testrail.io/index.php?/tests/view/5010669&group_by=cases:section_id&group_id=292107&group_order=asc Issue reported by: Applause Internal Team

Action Performed:

Open the app
Log in with any account
Open any 1:1 chat
Send any photo attachment
Send any PDF attachment
Open the PDF
Zoom in
Swipe to the right multiple times quickly

Expected Result:

I shouldn't be able to to switch to another attachment in PDF zoomed state.

Actual Result:

I can to switch to another attachment in PDF zoomed state.

Workaround:

Unknown

Platforms:

[x] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/1bdf1a25-bc75-47ac-9f15-558d26c2b8fe

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021839226463966939125
Upwork Job ID: 1839226463966939125
Last Price Increase: 2024-10-03
Automatic offers:

akinwale | Reviewer | 104253547

Issue Owner

Current Issue Owner: @bfitzexpensify

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets commented 1 month ago

@bfitzexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

bernhardoj commented 1 month ago

Edited by proposal-police: This proposal was edited at 2023-10-02 12:00:00.

Proposal

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

Carousel is scrollable when PDF is zoomed in.

What is the root cause of that problem?

We have a shared value to determine whether the carousel is scrollable or not. https://github.com/Expensify/App/blob/b655040afb3d5a482fd0c295dda4a6d8802c3df6/src/components/Attachments/AttachmentCarousel/useCarouselContextEvents.ts#L7

And we will disable it when the attachment is zoomed in. https://github.com/Expensify/App/blob/b655040afb3d5a482fd0c295dda4a6d8802c3df6/src/components/Attachments/AttachmentCarousel/useCarouselContextEvents.ts#L29-L45

However, in the PDF attachment (android file only), we have a Pan gesture that enable the scroll when lifting our finger after panning. https://github.com/Expensify/App/blob/b655040afb3d5a482fd0c295dda4a6d8802c3df6/src/components/Attachments/AttachmentView/AttachmentViewPdf/index.android.tsx#L55-L61

In https://github.com/Expensify/App/pull/30050, we add the Pan gesture to disable the carousel scrolling when scrolling the PDF. Then, https://github.com/Expensify/App/pull/33756 added the onTouchesUp code to re-enable it after we are done panning.

The problem is, we don't always want to re-enable the scroll after panning, especially when we are already zoomed in.

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

The problem that https://github.com/Expensify/App/pull/30050 solves is that the carousel/pager scroll conflicted with the PDF scroll, so we disable the carousel scroll when scrolling the PDF. But in our case, the PDF is zoomed in, which means the carousel/pager scroll is already disabled, so we don't need the logic to disable/re-enable it again.

To solve this, only execute the logic if the zoom scale is 1 (not zoomed in). https://github.com/Expensify/App/blob/b655040afb3d5a482fd0c295dda4a6d8802c3df6/src/components/Attachments/AttachmentView/AttachmentViewPdf/index.android.tsx#L35

if (offsetX.value !== 0 && offsetY.value !== 0 && isScrollEnabled && scale.value === 1) {

https://github.com/Expensify/App/blob/b655040afb3d5a482fd0c295dda4a6d8802c3df6/src/components/Attachments/AttachmentView/AttachmentViewPdf/index.android.tsx#L55-L61

if (!isScrollEnabled || scale.value !== 1) {
    return;
}

OR

isScrollEnabled.value = scale.value === 1;

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

SecretDev776 commented 1 month ago

To address the issue of navigating attachments while a PDF is zoomed in, several key solutions are proposed. First, the app should disable navigation when the PDF viewer is in a zoomed state, preventing users from accidentally switching attachments. Visual feedback, like a message or overlay, should indicate this restriction to help manage user expectations. Additionally, gesture handling needs to be modified to ignore swipes while in zoom mode, allowing only pinch gestures for zooming.

Implementing a state management system will track whether the PDF is zoomed in, controlling navigation functions accordingly. Comprehensive testing is essential to ensure that the changes work across various scenarios and devices. A user settings option could also be considered, allowing users to toggle navigation while zoomed in for added flexibility. Finally, after deployment, monitoring user feedback will be crucial to assess how effective the solution is and to make further adjustments as needed. These steps aim to enhance the overall user experience when interacting with PDF attachments.

melvin-bot[bot] commented 1 month ago

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

Make sure you've read and understood the contributing guidelines.
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 month ago

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

akinwale commented 1 month ago

We can move forward with @bernhardoj's proposal here.

🎀👀🎀 C+ reviewed.

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

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

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

Offer link Upwork job

bernhardoj commented 1 month ago

PR is ready

cc: @akinwale

melvin-bot[bot] commented 2 weeks ago

This issue has not been updated in over 15 days. @akinwale, @youssef-lr, @bfitzexpensify, @bernhardoj 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!

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

https://github.com/Expensify/App/pull/50208

If no regressions arise, payment will be issued on 2024-11-05. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@akinwale requires payment automatic offer (Reviewer)
@bernhardoj requires payment through NewDot Manual Requests

melvin-bot[bot] commented 2 weeks ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@akinwale] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@akinwale] 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:
[ ] [@akinwale] 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:
[ ] [@akinwale] Determine if we should create a regression test for this bug.
[ ] [@akinwale] 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.
[ ] [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

akinwale commented 1 week ago

[x] [@akinwale] The PR that introduced the bug has been identified. Link to the PR:

[x] [@akinwale] 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:

[x] [@akinwale] 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:

Regression identified in https://github.com/Expensify/App/pull/33756.

[x] [@akinwale] Determine if we should create a regression test for this bug.

[x] [@akinwale] 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.

Regression Test Steps

Launch Expensify on Android native
Open any report with multiple attachments including a PDF (or create multiple attachments including a PDF in the report)
Select the PDF attachment to open it
Zoom in on the PDF
Verify that you cannot swipe left or right to change the currently displayed attachment

Do we agree 👍 or 👎?

HezekielT commented 1 week ago

I think this was caused by https://github.com/Expensify/App/pull/33756 => by this change to be exact not by #30050 @akinwale. Could you please update it? Thanks.

akinwale commented 1 week ago

I think this was caused by #33756 => by this change to be exact not by #30050 @akinwale. Could you please update it? Thanks.

You're right. I'll update the comments.

bfitzexpensify commented 1 week ago

Payment summary:

@bernhardoj due $250 for contributor work - to be paid via manual request @akinwale due $250 for C+ work - paid via Upwork ✅

bernhardoj commented 1 week ago

Requested in ND.

JmillsExpensify commented 1 week ago

$250 approved for @bernhardoj

Expensify / App