[HOLD for payment 2024-11-22] [$250] Attachment - Image moves to the bottom when swiping down slightly

[izarutskaya]

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.54-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Launch ND app.
2. Go to DM.
3. Upload an image.
4. Tap on the image preview.
5. Swipe down from the top slightly.

Expected Result:

The image will be dismissed (production behavior).

Actual Result:

The image moves to the bottom.

Workaround:

Unknown

Platforms:

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

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6645372_1729849782889.swiipe.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021849822965045405568
• Upwork Job ID: 1849822965045405568
• Last Price Increase: 2024-10-25
• Automatic offers:
• brunovjk | Reviewer | 104693785

Issue Owner

Current Issue Owner: @CortneyOfstad

[melvin-bot]

Triggered auto assignment to @Julesssss (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

Triggered auto assignment to @CortneyOfstad (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.

[melvin-bot]

💬 A slack conversation has been started in #expensify-open-source

[github-actions]

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

[izarutskaya]

Production

P.2.MP4

[Julesssss]

Thanks! This is annoying, and I will seek out the cause, but I won't mark this as a blocker.

[Julesssss]

Reverting the 'Swipe attatchement' PR didn't resolve the issue

[melvin-bot]

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

[melvin-bot]

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

[brunovjk]

Hi @Anaslancer,Thank you for the proposal. I reproduced the issue, but I'm unclear about your suggestion. Did you mean we should completely disable the swipe feature in this scenario? From my understanding, we want the image to fade away after a swipe, like in production.

The problem currently is that when you swipe slightly, the image doesn’t close or return to the center; instead, it gets left in an awkward position:

Screen.Recording.2024-10-28.at.14.16.07.mov

[brunovjk]

Update: Still looking for proposals.

[brunovjk]

👋 @Anaslancer, have you had a chance to investigate this issue a bit further? Thanks.

[bernhardoj]

Edited by proposal-police: This proposal was edited at 2024-10-30 12:25:29 UTC.

Proposal

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

Swiping image doesn't close the attachment and instead moves the image permanently.

What is the root cause of that problem?

The logic to bring the image back to center is handled inside finishPanGesture based on the boundary that we calculate in getBounds.

App/src/components/MultiGestureCanvas/usePanGesture.ts

Lines 105 to 164 in f806997

	const finishPanGesture = useWorkletCallback(() => {
	// If the content is centered within the canvas, we don't need to run any animations
	if (offsetX.value === 0 && offsetY.value === 0 && panTranslateX.value === 0 && panTranslateY.value === 0) {
	return;
	}
	const {clampedOffset, isInHoriztontalBoundary, isInVerticalBoundary, horizontalBoundaries, verticalBoundaries} = getBounds();
	// If the content is within the horizontal/vertical boundaries of the canvas, we can smoothly phase out the animation
	// If not, we need to snap back to the boundaries
	if (isInHoriztontalBoundary) {
	// If the (absolute) velocity is 0, we don't need to run an animation
	if (Math.abs(panVelocityX.value) !== 0) {
	// Phase out the pan animation
	// eslint-disable-next-line react-compiler/react-compiler
	offsetX.value = withDecay({
	velocity: panVelocityX.value,
	clamp: [horizontalBoundaries.min, horizontalBoundaries.max],
	deceleration: PAN_DECAY_DECELARATION,
	rubberBandEffect: false,
	});
	}
	} else {
	// Animated back to the boundary
	offsetX.value = withSpring(clampedOffset.x, SPRING_CONFIG);
	}
	if (isInVerticalBoundary) {
	// If the (absolute) velocity is 0, we don't need to run an animation
	if (Math.abs(panVelocityY.value) !== 0) {
	// Phase out the pan animation
	offsetY.value = withDecay({
	velocity: panVelocityY.value,
	clamp: [verticalBoundaries.min, verticalBoundaries.max],
	deceleration: PAN_DECAY_DECELARATION,
	});
	}
	} else {
	const finalTranslateY = offsetY.value + panVelocityY.value * 0.2;
	if (finalTranslateY > SNAP_POINT && zoomScale.value <= 1) {
	offsetY.value = withSpring(SNAP_POINT_HIDDEN, SPRING_CONFIG, () => {
	isSwipingDownToClose.value = false;
	if (onSwipeDown) {
	runOnJS(onSwipeDown)();
	}
	});
	} else {
	// Animated back to the boundary
	offsetY.value = withSpring(clampedOffset.y, SPRING_CONFIG, () => {
	isSwipingDownToClose.value = false;
	});
	}
	}
	// Reset velocity variables after we finished the pan gesture
	panVelocityX.value = 0;
	panVelocityY.value = 0;
	});

getBounds calculate the bound from the canvas size of the view.

App/src/components/MultiGestureCanvas/usePanGesture.ts

Lines 69 to 100 in f806997

	const getBounds = useWorkletCallback(() => {
	let horizontalBoundary = 0;
	let verticalBoundary = 0;
	if (canvasSize.width < zoomedContentWidth.value) {
	horizontalBoundary = Math.abs(canvasSize.width - zoomedContentWidth.value) / 2;
	}
	if (canvasSize.height < zoomedContentHeight.value) {
	verticalBoundary = Math.abs(zoomedContentHeight.value - canvasSize.height) / 2;
	}
	const horizontalBoundaries = {min: -horizontalBoundary, max: horizontalBoundary};
	const verticalBoundaries = {min: -verticalBoundary, max: verticalBoundary};
	const clampedOffset = {
	x: MultiGestureCanvasUtils.clamp(offsetX.value, horizontalBoundaries.min, horizontalBoundaries.max),
	y: MultiGestureCanvasUtils.clamp(offsetY.value, verticalBoundaries.min, verticalBoundaries.max),
	};
	// If the horizontal/vertical offset is the same after clamping to the min/max boundaries, the content is within the boundaries
	const isInHoriztontalBoundary = clampedOffset.x === offsetX.value;
	const isInVerticalBoundary = clampedOffset.y === offsetY.value;
	return {
	horizontalBoundaries,
	verticalBoundaries,
	clampedOffset,
	isInHoriztontalBoundary,
	isInVerticalBoundary,
	};
	}, [canvasSize.width, canvasSize.height]);

The problem is, canvas size is initially 0. getBounds is already recreated when canvas size is updated, but not with finishPanGesture.

We get the canvas size from the view onLayout and we render nothing when the canvas size is still undefined.

App/src/components/Lightbox/index.tsx

Lines 204 to 213 in f806997

	<View
	style={[StyleSheet.absoluteFill, style]}
	onLayout={updateCanvasSize}
	>
	{!isCanvasLoading && (
	<>
	{isLightboxVisible && (
	<View style={[StyleUtils.getFullscreenCenteredContentStyles(), StyleUtils.getOpacityStyle(Number(shouldShowLightbox))]}>
	<MultiGestureCanvas
	isActive={isActive}

App/src/components/Lightbox/index.tsx

Lines 94 to 95 in f806997

	const [canvasSize, setCanvasSize] = useState<CanvasSize>();
	const isCanvasLoading = canvasSize === undefined;

onLayout is called with 0 width and height initially and then the correct value. This can be observed on any modal. You can add onLayout here and see the width will be 0 and height will be 0 too if you remove the padding.

App/src/components/Modal/BaseModal.tsx

Lines 272 to 277 in f806997

	<View
	style={[styles.defaultModalContainer, modalPaddingStyles, modalContainerStyle, !isVisible && styles.pointerEventsNone]}
	ref={ref}
	>
	<ColorSchemeWrapper>{children}</ColorSchemeWrapper>
	</View>

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

I think this happens after we enable bridgeless mode, but nevertheless, we need to fix the issue where the finishPanGesture is not recreated when canvas size is updated by adding getBounds as the useWorkletCallback deps.

App/src/components/MultiGestureCanvas/usePanGesture.ts

Lines 105 to 164 in f806997

	const finishPanGesture = useWorkletCallback(() => {
	// If the content is centered within the canvas, we don't need to run any animations
	if (offsetX.value === 0 && offsetY.value === 0 && panTranslateX.value === 0 && panTranslateY.value === 0) {
	return;
	}
	const {clampedOffset, isInHoriztontalBoundary, isInVerticalBoundary, horizontalBoundaries, verticalBoundaries} = getBounds();
	// If the content is within the horizontal/vertical boundaries of the canvas, we can smoothly phase out the animation
	// If not, we need to snap back to the boundaries
	if (isInHoriztontalBoundary) {
	// If the (absolute) velocity is 0, we don't need to run an animation
	if (Math.abs(panVelocityX.value) !== 0) {
	// Phase out the pan animation
	// eslint-disable-next-line react-compiler/react-compiler
	offsetX.value = withDecay({
	velocity: panVelocityX.value,
	clamp: [horizontalBoundaries.min, horizontalBoundaries.max],
	deceleration: PAN_DECAY_DECELARATION,
	rubberBandEffect: false,
	});
	}
	} else {
	// Animated back to the boundary
	offsetX.value = withSpring(clampedOffset.x, SPRING_CONFIG);
	}
	if (isInVerticalBoundary) {
	// If the (absolute) velocity is 0, we don't need to run an animation
	if (Math.abs(panVelocityY.value) !== 0) {
	// Phase out the pan animation
	offsetY.value = withDecay({
	velocity: panVelocityY.value,
	clamp: [verticalBoundaries.min, verticalBoundaries.max],
	deceleration: PAN_DECAY_DECELARATION,
	});
	}
	} else {
	const finalTranslateY = offsetY.value + panVelocityY.value * 0.2;
	if (finalTranslateY > SNAP_POINT && zoomScale.value <= 1) {
	offsetY.value = withSpring(SNAP_POINT_HIDDEN, SPRING_CONFIG, () => {
	isSwipingDownToClose.value = false;
	if (onSwipeDown) {
	runOnJS(onSwipeDown)();
	}
	});
	} else {
	// Animated back to the boundary
	offsetY.value = withSpring(clampedOffset.y, SPRING_CONFIG, () => {
	isSwipingDownToClose.value = false;
	});
	}
	}
	// Reset velocity variables after we finished the pan gesture
	panVelocityX.value = 0;
	panVelocityY.value = 0;
	});

We can optionally replace useWorkletCallback to useCallback as it's deprecated and also the deprecation message suggesting to replace it with useCallback. The current useWorkletCallback implementation is just a useCallback.
https://github.com/software-mansion/react-native-reanimated/blob/main/packages/react-native-reanimated/src/hook/useWorkletCallback.ts

we can update isCanvasLoading to return it as true if the canvas size is 0, but I don't think it fixes the root cause

[brunovjk]

Thank you for the proposal bernhardoj.

I tested it and @bernhardoj's proposal seems good to me. The RCA analysis seems correct to me too.

Main	Proposal
main.mov	proposal.mov

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[brunovjk]

@justinpersaud and @bernhardoj do you think it's worth replacing all useWorkletCallback with useCallback in the app? Thanks.

[bernhardoj]

I think it should be save to replace them all.

[Julesssss]

Nice, yeah lets replace all uses please.

[melvin-bot]

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

[CortneyOfstad]

PR actively being worked on so no update

[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.62-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix image moves permanently when swipe down #51921

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

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

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

[melvin-bot]

@brunovjk @CortneyOfstad @brunovjk The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]