Attachment - Image does not zoom in when two fingers are used

isagoico commented 3 years 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!

Expected Result

Confirm the image zooms in

Actual Result

Image does not zoom in

Action Performed

Launch the app and login
Send an image attachment
After the attachment uploads, tap on it, confirm an attachment modal shows
Use two fingers to zoom in

Platform

iOS ✔️ Android ✔️ mWeb/Chrome ✔️

Workaround:

User can zoom with double tap

Platform:

Where is this issue occurring?

Web iOS ✔️ Android ✔️ Desktop App Mobile Web

Version Number: 1.0.27-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Images/Videos Issue is reproducible in production but it's failing step 5 of this PR https://github.com/Expensify/Expensify.cash/pull/2442#issuecomment-823623629.

https://user-images.githubusercontent.com/44479856/115468224-c4c6e180-a200-11eb-8a4e-9e751b56a07f.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

Jag96 commented 3 years ago

Had a look at this today and this is definitely a regression, at least on iOS, on the Android emulator the pinch-zoom didn't work even on an old version without the recent changes. The main issue is that the event/gestureState being passed into the onStartShouldSetPanResponder callback is showing 1 touch, rather than 2 touches. On the simulator this works fine, and when testing on a physical device the pinch to zoom works about 50% of the time, so I believe this is related to the PanResponder trying to prevent multiple taps as stated in the docs.

PanResponder reconciles several touches into a single gesture. It makes single-touch gestures resilient to extra touches, and can be used to recognize basic multi-touch gestures.

The reason this was working before was that in the library code, the zoom functionality was happening inside the onPanResponderMove function even after 1 tap, however, with the recent changes the override of onStartShouldSetPanResponder now prevents that logic from being hit. For context, the reason for this change was to enable the swipe event to be passed to the Attachment Modal if the tap event was just one touchpoint or we were zoomed in.

I tried a couple of things here to enable the zoom to work consistently but ultimately couldn't retain both behaviors

I tried overriding the onStart to always return false, and then moving the logic from onStart to onMove. The issue here was that as soon as the ImageView component passes on the onStart, the Modal grabs the event and the ImageZoom onMove and onMoveShouldSetPanResponder functions never get hit
I tried updating the onStartShouldSetPanResponder to return true, passing down a function as a prop to close the AttachmentModal, and calling that function inside the onMove/responderRelease which works, but with two issues; the animation for dismissing the modal is not the same as the other modals (it doesn't animate as you swipe), and the library does not pass any data letting us determine the direction/distance the user swiped which means the modal dismisses regardless of the direction you swipe in. We'd need to update the library to pass the event data to the callbacks to make this work.

Some potential solutions, from easiest to more complicated:

Figure out some way to get the PanResponder event to be more accurate for multi-touch gestures (if possible)
Update the code to pass down a callback and dismiss on any onPanResponderRelease event where the scale is set to 1 (not zoomed), sacrificing the consistent modal animation
Write our own version of the ImageZoom library that works with our use case

cc @marcaaron since you are familiar w/ this feature

Jag96 commented 3 years ago

Had another look at this today but didn't find any valid solutions, and it doesn't seem like there's a way to make the PanResponder event more accurate for multi-touch. I also tried using https://github.com/aMarCruz/react-native-photo-view-ex and https://github.com/ascoders/react-native-image-viewer#readme but nothing super helpful in those either. I've pushed this branch with my testing diff, I'll get more eyes on this in #expensify-open-source.

isagoico commented 3 years ago

Issue reproducible during today's KI retests

isagoico commented 3 years ago

Issue reproducible during today's KI retests

isagoico commented 3 years ago

Issue reproducible during today's KI retests

isagoico commented 3 years ago

Issue reproducible today during KI retests

isagoico commented 3 years ago

Issue reproducible today during KI retests

isagoico commented 3 years ago

Issue reproducible today during KI retests

isagoico commented 3 years ago

Issue reproducible today during KI retests.

isagoico commented 3 years ago

Issue reproducible during KI retests.

isagoico commented 3 years ago

Issue reproducible during KI retests.

isagoico commented 3 years ago

Issue reproducible during KI retests

isagoico commented 3 years ago

Issue reproducible during KI retests

isagoico commented 3 years ago

Issue reproducible during KI retests

isagoico commented 3 years ago

Issue reproducible during KI retests

Jag96 commented 3 years ago

Missed this one! PR in review

isagoico commented 3 years ago

Issue reproducible during KI retests

isagoico commented 3 years ago

Issue is still reproducible and failing https://github.com/Expensify/App/pull/4440 CC @Jag96

OSBotify commented 3 years ago

:wave: 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:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

isagoico commented 3 years ago

Issue reproducible during KI retests

Jag96 commented 3 years ago

Removing the blocker label since it's the same behavior on production

isagoico commented 3 years ago

When retesting this. I noticed that if you zoom in the picture first, then you're able to use the pinch gesture to zoom in and out, but when you return the picture to the original size the pinch gesture won't work

https://user-images.githubusercontent.com/44479856/130476765-07484610-67c6-498e-a48b-6c1a4a2252e5.mp4

PD: Enjoy the pic of my chair stealing cats, I just have to leave the chair to get some water for them to take over like this.

Jag96 commented 3 years ago

Haven't gotten a chance to look at this yet due to other priorities

isagoico commented 3 years ago

Issue reproducible during KI retests

Jag96 commented 3 years ago

Had another look at this and it's the same issue as https://github.com/Expensify/App/issues/2499#issuecomment-827228825, the same problem exists for the invisible view where numberActiveTouches is only set to 2 if both fingers hit the view at the same exact time, which isn't realistic.

The next steps here would be to look into a couple of libraries to see if there's any other way to get this working, otherwise, we'll need to look into a custom solution which will likely involve replacing ImageZoom (and possibly react-native-modal) with our own implementation that handles both the modal animation swipe and all zoom/pan functionality.

isagoico commented 3 years ago

Issue reproducible during KI retests

isagoico commented 3 years ago

Issue reproducible during KI retests.

Jag96 commented 3 years ago

Haven't gotten to this, not a priority atm

isagoico commented 3 years ago

Issue reproducible during KI retests.

Jag96 commented 3 years ago

Still low priority

isagoico commented 3 years ago

Issue reproducible during KI retests.

mvtglobally commented 3 years ago

Issue reproducible during KI retests.

mvtglobally commented 3 years ago

Issue not reproducible during KI retests. (First Week)

mvtglobally commented 3 years ago

Issue reproducible during KI retests.

mvtglobally commented 3 years ago

Issue reproducible during KI retests.

Jag96 commented 3 years ago

This one hasn't been a priority for quite some time, and likely won't be a priority soon. Unassigning myself.

mvtglobally commented 2 years ago

Issue not reproducible during KI retests. (Second Week)

mvtglobally commented 2 years ago

Issue not reproducible during KI retests. (Third week) Keeping open for another check due to this https://expensify.slack.com/archives/C01GTK53T8Q/p1639445152215200

mvtglobally commented 2 years ago

Team was able to reproduce again this week.

https://user-images.githubusercontent.com/43995119/146866966-d4551476-c999-4d5a-923e-edd1d6e7838b.mp4

mvtglobally commented 2 years ago

Issue not reproducible during KI retests. (Fourth week). It seems very inconsistent. Should we close this?

Jag96 commented 2 years ago

@mvtglobally I believe it will only work properly if both fingers hit the screen at exactly the same time. If you put one finger on the screen and then add the second and pinch to zoom, I believe it will be reproducible every time.

MelvinBot commented 2 years ago

@isagoico, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

kbecciv commented 2 years ago

@Jag96 Issue is reproduced during Regression.

https://user-images.githubusercontent.com/93399543/167934826-fe07f60a-b295-4ade-9d0d-f3b6b1bd3c05.mp4

melvin-bot[bot] commented 2 years ago

@isagoico, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

thesahindia commented 2 years ago

The issue is reproducible. Can we reopen this?

cc: @Jag96 @kadiealexander

kbecciv commented 1 year ago

Issue is reproductible on build 1.2.62.0

https://user-images.githubusercontent.com/93399543/215602333-9872c2d8-b1ca-4488-b579-66e7f3136a13.mp4

MelvinBot commented 1 year ago

@isagoico, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

kbecciv commented 1 year ago

Issue is reproduced in build 1.3.47.3

https://github.com/Expensify/App/assets/93399543/76c63dfb-2b87-4070-b521-6cee819d9cc1

NajiNazzal commented 1 year ago

I would like to contribute please

melvin-bot[bot] commented 1 year ago

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

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

Expensify / App