[HOLD for payment 2024-09-03][$250] iOS - Attachment - 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 contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.23-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/4879183&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. Send any photo attachment
7. Open the PDF
8. Switch to the image on the right after the PDF finished loading
9. Switch back to the PDF
10. Zoom in on the PDF with the pinch method
11. Switch to the image on the right while the PDF is still zoomed

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 after swiping to an image and back to the PDF.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6578108_1724244156588.JCEQ1898.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010723f9a2c0dc6d78
• Upwork Job ID: 1827109849447510261
• Last Price Increase: 2024-08-23

Issue Owner

Current Issue Owner: @s77rt

[melvin-bot]

Triggered auto assignment to @johncschuster (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]

We think that this bug might be related to #vip-vsb

[IuliiaHerets]

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

Proposal

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

We can swipe between attachment even though the PDF or image is zoomed in.

What is the root cause of that problem?

When the attachment scale is changed, we call the carousel pager onScaleChanged which will disable the scrolling/swiping (isScrollEnabled.value).

App/src/components/Attachments/AttachmentView/AttachmentViewPdf/BaseAttachmentViewPdf.tsx

Lines 43 to 45 in 12037ee

	if (isUsedInCarousel && attachmentCarouselPagerContext) {
	attachmentCarouselPagerContext.onScaleChanged(newScale);
	}

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

Lines 29 to 46 in 12037ee

	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);
	},
	[isScrollEnabled, onRequestToggleArrows],

App/src/components/Attachments/AttachmentCarousel/Pager/index.tsx

Lines 105 to 107 in 12037ee

	const animatedProps = useAnimatedProps(() => ({
	scrollEnabled: isScrollEnabled.value,
	}));

But it doesn't work for both PDF and image. For the PDF, we only call the scale change callback if isUsedInCarousel is true, but we never pass the props.

App/src/components/Attachments/AttachmentView/AttachmentViewPdf/BaseAttachmentViewPdf.tsx

Lines 43 to 45 in 12037ee

	if (isUsedInCarousel && attachmentCarouselPagerContext) {
	attachmentCarouselPagerContext.onScaleChanged(newScale);
	}

For both image and PDF, even though the scale change callback is called, the scroll is never disabled because the isScrollEnabled.value here

App/src/components/Attachments/AttachmentCarousel/Pager/index.tsx

Lines 105 to 107 in 12037ee

	const animatedProps = useAnimatedProps(() => ({
	scrollEnabled: isScrollEnabled.value,
	}));

is coming from the local shared value.

App/src/components/Attachments/AttachmentCarousel/Pager/index.tsx

Line 61 in 12037ee

const isScrollEnabled = useSharedValue(true);

But the scale change callback changes the useCarouselContextEvents shared value.

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

Lines 5 to 7 in 12037ee

	function useCarouselContextEvents(setShouldShowArrows: (show?: SetStateAction<boolean>) => void) {
	const scale = useRef(1);
	const isScrollEnabled = useSharedValue(true);

This happens after #43620.

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

In pdf attachment component, we can check for pagerRef context, just like we did with image.

App/src/components/Attachments/AttachmentView/AttachmentViewPdf/BaseAttachmentViewPdf.tsx

Lines 42 to 45 in 12037ee

	// 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);
	}

if (!!attachmentCarouselPagerContext?.pagerRef) {

App/src/components/Lightbox/index.tsx

Line 84 in 12037ee

isUsedInCarousel: !!attachmentCarouselPagerContext.pagerRef,

To fix the double isScrollEnabled, we can remove isScrollEnabled from AttachmentCarouselPager/Pager,

App/src/components/Attachments/AttachmentCarousel/Pager/index.tsx

Line 61 in 12037ee

const isScrollEnabled = useSharedValue(true);

then use it from useCarouselContextEvents.

const {handleTap, handleScaleChange, isScrollEnabled} = useCarouselContextEvents(setShouldShowArrows);

useCarouselContextEvents will return the isScrollEnabled.

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

Line 7 in 12037ee

const isScrollEnabled = useSharedValue(true);

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

Line 61 in 12037ee

return {handleTap, handleScaleChange, scale};

return {handleTap, handleScaleChange, scale, isScrollEnabled};

[melvin-bot]

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

[melvin-bot]

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

[s77rt]

Not reproducible

Screen.Recording.2024-08-24.at.8.34.49.PM.mov

[bernhardoj]

Hmm, your video shows that it's reproducible.

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 after swiping to an image and back to the PDF.

[s77rt]

@bernhardoj Oh I misunderstood the issue, you are right! Your RCA makes sense but regarding the solution:

we only call the scale change callback if isUsedInCarousel is true, but we never pass the props.

Shouldn't we just pass the prop instead of checking against !!attachmentCarouselPagerContext.pagerRef (DRY)

(Using isScrollEnabled from useCarouselContextEvents is 👍)

[bernhardoj]

Shouldn't we just pass the prop instead of checking against !!attachmentCarouselPagerContext.pagerRef

We already have the information from attachmentCarouselPagerContext.pagerRef (which we already use in image component, Lightbox), so relying on another source (isUsedInCarousel) won't make it DRY I guess.

[s77rt]

@bernhardoj For this case the prop is always passed as true from CarouselItem to AttachmentView and since it's available in AttachmentView we should just pass it down to the AttachmentViewPdf

[bernhardoj]

We can always remove isUsedInCarousel but we can't remove thepagerRef. Instead of doing props drilling while we already have the information from a context, I prefer to get it from the context.

[s77rt]

@bernhardoj Removing isUsedInCarousel sounds good to me. As long as we keep only one source of truth.

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

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

[bernhardoj]

PR is ready

cc: @s77rt

[johncschuster]

Payment Summary:

Contributor: @bernhardoj owed $250 via NewDot
Contributor+: @s77rt owed $250 via NewDot

@bernhardoj / @s77rt do you feel this requires regression test steps? If so, can you please provide them? Thank you!

[s77rt]

@johncschuster This already has a regression test.

[johncschuster]

Awesome. Thanks, @s77rt! In that case, we're all done here. Please make your manual requests!

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @s77rt

[JmillsExpensify]

$250 approved for @bernhardoj