[$500] Attachment - Navigation arrows not disabled and PDF can be swiped while zoomed in

[lanitochka17]

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: 1.4.42.1
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/4318973
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Open any chat
2. Send some PDF files
3. Open any PDF
4. Double-tap or pinch to zoom the PDF
5. Tap once again and try to swipe

Expected Result:

PDF is now swiped and the navigation arrows are disabled while zoomed in

Actual Result:

PDF can be swiped using the navigation arrows while zoomed in

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6381181_1708028243865.video_2024-02-15_15-17-03.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b58822d8679c7299
• Upwork Job ID: 1759630579153809408
• Last Price Increase: 2024-02-19
• Automatic offers:
• DylanDylann | Reviewer | 0
• tienifr | Contributor | 0

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp
CC @quinthar

[lanitochka17]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[puneetlath]

Making external as this does seem like a bug.

[tienifr]

Proposal

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

PDF can be swiped using the navigation arrows while zoomed in

What is the root cause of that problem?

We're not setting isUsedInCarousel to true for AttachmentView in CarouselItem. So this line will never be run and the arrows won't hide because this logic to set the arrows visibility to the correct value will never be run for PDF

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

Set isUsedInCarousel to true for AttachmentView in CarouselItem here

What alternative solutions did you explore? (Optional)

We can set the isUsedInCarousel of only AttachmentViewPdf to the isUsedInCarousel of AttachmentView if we want to apply isUsedInCarousel only for PDF, but I think it makes sense to keep the isUsedInCarousel of AttachmentViewImage and AttachmentViewVideo as is, because it's true in this case that they are being used in the carousel.

[DylanDylann]

@puneetlath I also see this bug happen on the web

Screen.Recording.2024-02-20.at.15.29.43.mov

Should we fix this issue on all platforms or only for native mobile? I suggest we only fix this issue on the native mobile app because I don't think It's a problem on web

[DylanDylann]

@tienifr Your proposal looks promising but It only fix this issue on android

[hassanShahzad]

Proposal

Please re-state the problem that we are trying to solve in this issue.
PDF can be swiped using the navigation arrows while zoomed in

What is the root cause of that problem?
Not setting isScrollEnabled = false for zoom in in onScaleChanged method in src/components/Attachments/AttachmentView/AttachmentViewPdf/BaseAttachmentViewPdf.js file

What changes do you think we should make in order to solve the problem?
In src/components/Attachments/AttachmentView/AttachmentViewPdf/BaseAttachmentViewPdf.js file

1. We have to use let instead of count for isScrollEnabled:
Current code:
const isScrollEnabled = attachmentCarouselPagerContext === null ? undefined : attachmentCarouselPagerContext.isScrollEnabled;

Updated code:
let isScrollEnabled = attachmentCarouselPagerContext === null ? undefined : attachmentCarouselPagerContext.isScrollEnabled;

2. We have to change code for onScaleChanged for if condition for When a pdf is shown in a carousel, we want to disable the pager scroll when the pdf is zoomed in:
Current code:

// When a pdf is shown in a carousel, we want to disable the pager scroll when the pdf is zoomed in
            if (isUsedInCarousel && attachmentCarouselPagerContext) {
                attachmentCarouselPagerContext.onScaleChanged(newScale);
            }

Updated code:

// When a pdf is shown in a carousel, we want to disable the pager scroll when the pdf is zoomed in
            if (attachmentCarouselPagerContext) {
                attachmentCarouselPagerContext.onScaleChanged(newScale);
                newScale > 1 ? isScrollEnabled = false : null;
            }

Then it will work fine with onPress in src/components/Attachments/AttachmentView/AttachmentViewPdf/BaseAttachmentViewPdf.js file. Which is:

  const onPress = useCallback(
       (e) => {
           if (onPressProp !== undefined) {
               onPressProp(e);
           }

           if (attachmentCarouselPagerContext !== null && isScrollEnabled.value) {
               attachmentCarouselPagerContext.onTap(e);
           }
       },
       [attachmentCarouselPagerContext, isScrollEnabled, onPressProp],
   );

Screen.Recording.2024-02-20.at.15.08.30.mov

What alternative solutions did you explore? (Optional)
None

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

[tienifr]

@DylanDylann

@tienifr Your proposal looks promising but It only fix this issue on android

It fixes for both Android, iOS

I suggest we only fix this issue on the native mobile app because I don't think It's a problem on web

It's intentional to only hide the arrows on native mobile and mobile web because those arrows will take a significant amount of space on small screen. On desktop web the arrows are very small so it should show.

If we look at this line, we can see that it's intentional not to hide the arrows on non-touch-screen (aka desktop) web.

[DylanDylann]

@tienifr's proposal looks good to me. I also think we only need to fix on mobile app and mobile web.
One more thing, this issue also resolved this deploy blocker

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 @DylanDylann 🎉 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

[melvin-bot]

📣 @tienifr 🎉 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 📖

[puneetlath]

Actually, let's consolidate conversation here: #36873 (comment)

[tienifr]

@puneetlath The issue was fixed it #37202. We can proceed next steps. Since we (I and C+) have comitted efforts for PR and the delay was mostly on internal side, I believe we can still be compensated for this. Thanks.

[puneetlath]

Ok I think that's fair, since the full PR and review was done.

Paid it, thanks everyone.