Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[$500] [MEDIUM] Zoomed large image disappears from preview if triple tap on the bottom of the screen #35383

Closed kavimuru closed 8 months ago

kavimuru commented 9 months 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: v1.4.33-3 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/4253754 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. Go to https://staging.new.expensify.com/
  2. On a conversation, click on the + button in the compose box
  3. Upload this long image to a conversation
  4. Tap on the preview
  5. Double tap image > Scroll down > Triple tap on the bottom

    Expected Result:

    Zoomed large image has become smaller

Actual Result:

Zoomed large image disappears from preview if triple tap on the bottom of the screen.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6360736_1706612036657!long_image

https://github.com/Expensify/App/assets/43996225/7e346ccc-d0b0-43d7-9b2d-2f2c0fbf0081

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018c8406d6a29ea8b8
  • Upwork Job ID: 1752291085958225920
  • Last Price Increase: 2024-01-30
  • Automatic offers:
    • andreasnw | Contributor | 28145607
melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

andreasnw commented 9 months ago

Contributor details Your Expensify account email: andreasnw93@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0111c6bff2681e8236

melvin-bot[bot] commented 9 months ago

โœ… Contributor details stored successfully. Thank you for contributing to Expensify!

dylanexpensify commented 9 months ago

hmm I think this is a dupe, cc @eVoloshchak can you confirm?

andreasnw commented 9 months ago

Proposal

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

Triple-clicking causes the image to zoom out, but it fails to reposition the image, leading it to move out of the visible area.

What is the root cause of that problem?

Upon a double click by the user, the reset() function is triggered, animating the offsetY.value to 0 https://github.com/Expensify/App/blob/49fbaa035150260033424bf284343a05c3213fb5/src/components/MultiGestureCanvas/index.tsx#L126-L133

while the one remaining click is captured by the pan gesture, and the single tap listener invokes the stopAnimation() method before offsetY.value successfully set to 0.

https://github.com/Expensify/App/blob/49fbaa035150260033424bf284343a05c3213fb5/src/components/MultiGestureCanvas/usePanGesture.ts#L114-L127

https://github.com/Expensify/App/blob/49fbaa035150260033424bf284343a05c3213fb5/src/components/MultiGestureCanvas/useTapGestures.ts#L131-L135

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

Decrease spring duration to 0 on reset function, transforming offsetY.value before pan gesture, and the single tap listener invokes the stopAnimation()

https://github.com/Expensify/App/blob/49fbaa035150260033424bf284343a05c3213fb5/src/components/MultiGestureCanvas/index.tsx#L126-L141

offsetY.value = withSpring(0, {...SPRING_CONFIG, duration: 0});

What alternative solutions did you explore? (Optional)

  1. use reduceMotion

    offsetY.value = withSpring(0, {...SPRING_CONFIG, reduceMotion: ReduceMotion.Always});
  2. Only use animation for zoom

aswin-s commented 9 months ago

Proposal

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

Zoomed in extra long image disappear when tripple tapped at bottom of screen.

What is the root cause of that problem?

Zoom, Pan & Pich gestures on the ImageView is handled by the MultiGestureCanvas component. It simultaneously listen to all three gestures and triggers the first one that is detected.

https://github.com/Expensify/App/blob/49fbaa035150260033424bf284343a05c3213fb5/src/components/MultiGestureCanvas/index.tsx#L246

When user double taps on an image it gets zoomed in or gets zoomed back out based on the current state of the image.

https://github.com/Expensify/App/blob/49fbaa035150260033424bf284343a05c3213fb5/src/components/MultiGestureCanvas/useTapGestures.ts#L122-L128

If image is already zoomed, double tap gesture resets the zoomScale and translate offsets of the image. To give a smooth transition the values are reset to zero using withSpring animation. This interpolates the zoomScale and translate offsets gradually to zero.

https://github.com/Expensify/App/blob/49fbaa035150260033424bf284343a05c3213fb5/src/components/MultiGestureCanvas/index.tsx#L126-L141

When user triple taps on the screen, the first two taps trigger the doubleTapGesture and the third tap triggers a panGesture. To prevent race conditions, each gesture handler resets the animations before it gets invoked.

https://github.com/Expensify/App/blob/49fbaa035150260033424bf284343a05c3213fb5/src/components/MultiGestureCanvas/usePanGesture.ts#L127

But the reset method only cancels animations on translateX & translateY shared variables. And animation on zoomScale variable continues to run.

https://github.com/Expensify/App/blob/49fbaa035150260033424bf284343a05c3213fb5/src/components/MultiGestureCanvas/index.tsx#L118-L121

Result is that zoom gets reset to zero, but translateX & translateY stop mid way. This results in the image getting hidden.

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

Along with translateX & translateY variables, reset the zoomScale shared variable as well. This makes sure that when the panGesture gets activated it will cancel out the doubleTap animation.

const stopAnimation = useWorkletCallback(() => {
        cancelAnimation(offsetX);
        cancelAnimation(offsetY);
        // cancel animation on zoomScale as well
        cancelAnimation(zoomScale);
});

Result

Before After

What alternative solutions did you explore? (Optional)

None

eVoloshchak commented 9 months ago

hmm I think this is a dupe, cc @eVoloshchak can you confirm?

@dylanexpensify, cannot find a dupe of this issue. I remember seeing similar ones (this exact image was causing other issues), but not with triple-tapping

@andreasnw's proposal looks good to me. This is kind of an edge case, so we have to make sure we don't break the main functionality when testing the PR

๐ŸŽ€๐Ÿ‘€๐ŸŽ€ C+ reviewed!

melvin-bot[bot] commented 9 months ago

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

rlinoz commented 9 months ago

@dylanexpensify can you think of a wave to put this on?

dylanexpensify commented 9 months ago

I'd say wave5 for now @rlinoz!

rlinoz commented 9 months ago

Ok!

Let's go with @andreasnw's proposal and and take care to not break the main functionality as @eVoloshchak said.

melvin-bot[bot] commented 9 months ago

๐Ÿ“ฃ @andreasnw ๐ŸŽ‰ 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 ๐Ÿ“–

andreasnw commented 9 months ago

PR ready for review https://github.com/Expensify/App/pull/35931

dylanexpensify commented 9 months ago

Nice!

dylanexpensify commented 9 months ago

review in progress

dylanexpensify commented 8 months ago

@eVoloshchak update?

rlinoz commented 8 months ago

PR is merged!

melvin-bot[bot] commented 8 months ago

โš ๏ธ 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.

andreasnw commented 8 months ago

not a regression https://github.com/Expensify/App/issues/37248#issuecomment-1965808408

dylanexpensify commented 8 months ago

Nice! Payout coming soon if no regression!

dylanexpensify commented 8 months ago

ah, payment didn't autogenerate, but been 7 days, so will work on this now!

andreasnw commented 8 months ago

appreciate your help, @dylanexpensify, thank you!

andreasnw commented 8 months ago

hi @dylanexpensify any updates on this payment issue?

dylanexpensify commented 8 months ago

Payment summary:

Please apply/request!

andreasnw commented 8 months ago

How do i apply/ request @dylanexpensify ?

dylanexpensify commented 8 months ago

Ah, that's for our records @andreasnw! You've now been paid! ๐ŸŽ‰

rlinoz commented 8 months ago

Since everyone has been paid we can close this one out.

JmillsExpensify commented 7 months ago

$500 approved for @eVoloshchak based on summary.