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

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:

1. Open the app
2. Log in with any account
3. Open any 1:1 chat
4. Send any photo attachment
5. Send any PDF attachment
6. Open the PDF
7. Zoom in
8. 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:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6615307_1727298798512.az_recorder_20240925_230359.mp4

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]

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]

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

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.

App/src/components/Attachments/AttachmentCarousel/useCarouselContextEvents.ts

Line 7 in b655040

const isScrollEnabled = useSharedValue(true);

And we will disable it when the attachment is zoomed in.

App/src/components/Attachments/AttachmentCarousel/useCarouselContextEvents.ts

Lines 29 to 45 in b655040

	const handleScaleChange = useCallback(
	(newScale: number) => {
	if (newScale === scale.current) {
	return;
	}
	scale.current = newScale;
	const newIsScrollEnabled = newScale === 1;
	if (isScrollEnabled.value === newIsScrollEnabled) {
	return;
	}
	// eslint-disable-next-line react-compiler/react-compiler
	isScrollEnabled.value = newIsScrollEnabled;
	onRequestToggleArrows(newIsScrollEnabled);
	},

However, in the PDF attachment (android file only), we have a Pan gesture that enable the scroll when lifting our finger after panning.

App/src/components/Attachments/AttachmentView/AttachmentViewPdf/index.android.tsx

Lines 55 to 61 in b655040

	.onTouchesUp(() => {
	isPanGestureActive.value = false;
	if (!isScrollEnabled) {
	return;
	}
	isScrollEnabled.value = true;
	});

In #30050, we add the Pan gesture to disable the carousel scrolling when scrolling the PDF. Then, #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 #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).

App/src/components/Attachments/AttachmentView/AttachmentViewPdf/index.android.tsx

Line 35 in b655040

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

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

App/src/components/Attachments/AttachmentView/AttachmentViewPdf/index.android.tsx

Lines 55 to 61 in b655040

	.onTouchesUp(() => {
	isPanGestureActive.value = false;
	if (!isScrollEnabled) {
	return;
	}
	isScrollEnabled.value = true;
	});

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

OR

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

[melvin-bot]

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

[melvin-bot]

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

[SecretDev776]

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]

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

1. Make sure you've read and understood the contributing guidelines.
2. 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.
3. 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.
4. 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]

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

[akinwale]

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

🎀👀🎀 C+ reviewed.

[melvin-bot]

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

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

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

PR is ready

cc: @akinwale

[melvin-bot]

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]

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

[melvin-bot]

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

• Fix able to swipe attachment when PDF is zoomed in #50208

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

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]

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]

• [@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:

Regression identified in #33756.

• [@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.

Regression Test Steps

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

Do we agree 👍 or 👎?

[HezekielT]

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

[akinwale]

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]

Payment summary:

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

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj