Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
isagoico opened this issue Apr 20, 2021 · 55 comments
Closed

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

isagoico opened this issue Apr 20, 2021 · 55 comments
Labels
Engineering Improvement Item broken or needs improvement. Monthly KSv2 Not a priority

Comments

@isagoico
Copy link

isagoico commented Apr 20, 2021 •
edited by kbecciv
Loading

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

  1. Launch the app and login
  2. Send an image attachment
  3. After the attachment uploads, tap on it, confirm an attachment modal shows
  4. 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 #2442 (comment).

Bug5030887_20210420_162938_1_.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork
@isagoico isagoico changed the title Android/iOS - Chat - Image does not zoom in when two fingers are used Android/iOS - Attachment - Image does not zoom in when two fingers are used Apr 20, 2021
@Jag96 Jag96 added Weekly KSv2 Improvement Item broken or needs improvement. labels Apr 20, 2021
@Jag96
Copy link
Contributor

Jag96 commented Apr 27, 2021 •
edited
Loading

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
Copy link
Contributor

Jag96 commented May 4, 2021

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
Copy link
Author

isagoico commented May 9, 2021

Issue reproducible during today's KI retests

2 similar comments
@isagoico
Copy link
Author

isagoico commented May 17, 2021 •
edited
Loading

Issue reproducible during today's KI retests

@isagoico
Copy link
Author

Issue reproducible during today's KI retests

@isagoico
Copy link
Author

Issue reproducible today during KI retests

2 similar comments
@isagoico
Copy link
Author

isagoico commented Jun 7, 2021

Issue reproducible today during KI retests

@isagoico
Copy link
Author

Issue reproducible today during KI retests

@isagoico
Copy link
Author

Issue reproducible today during KI retests.

@isagoico
Copy link
Author

Issue reproducible during KI retests.

1 similar comment
@isagoico
Copy link
Author

isagoico commented Jul 5, 2021

Issue reproducible during KI retests.

@isagoico
Copy link
Author

Issue reproducible during KI retests

3 similar comments
@isagoico
Copy link
Author

Issue reproducible during KI retests

@isagoico
Copy link
Author

Issue reproducible during KI retests

@isagoico
Copy link
Author

isagoico commented Aug 2, 2021

Issue reproducible during KI retests

@Jag96 Jag96 added the Reviewing Has a PR in review label Aug 6, 2021
@MelvinBot MelvinBot removed the Overdue label Aug 6, 2021
@Jag96
Copy link
Contributor

Jag96 commented Aug 6, 2021

Missed this one! PR in review

@Jag96 Jag96 mentioned this issue Aug 6, 2021
Merged
5 tasks
@isagoico
Copy link
Author

isagoico commented Aug 9, 2021

Issue reproducible during KI retests

@isagoico isagoico added the DeployBlockerCash This issue or pull request should block deployment label Aug 14, 2021
@isagoico
Copy link
Author

isagoico commented Aug 14, 2021 •
edited
Loading

Issue is still reproducible and failing #4440 CC @Jag96

@OSBotify OSBotify mentioned this issue Aug 14, 2021
Closed
53 tasks
@github-actions github-actions bot added Engineering and removed Weekly KSv2 labels Aug 14, 2021
@kbecciv
Copy link

kbecciv commented May 11, 2022

@Jag96 Issue is reproduced during Regression.

screen-20220511-224934.mp4

@kbecciv kbecciv reopened this May 11, 2022
@melvin-bot melvin-bot bot added the Overdue label May 11, 2022
@melvin-bot melvin-bot bot closed this as completed Jul 25, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 25, 2022

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

@melvin-bot melvin-bot bot removed the Overdue label Jul 25, 2022
@thesahindia
Copy link
Member

The issue is reproducible. Can we reopen this?

cc: @Jag96 @kadiealexander

@kbecciv kbecciv changed the title Android/iOS - Attachment - Image does not zoom in when two fingers are used Attachment - Image does not zoom in when two fingers are used Jan 30, 2023
@kbecciv
Copy link

kbecciv commented Jan 30, 2023

Issue is reproductible on build 1.2.62.0

Screen.Recording.20230130.232854.Chrome.mp4

@kbecciv kbecciv reopened this Jan 30, 2023
@melvin-bot melvin-bot bot added the Overdue label Jan 30, 2023
@ArekChr ArekChr mentioned this issue Mar 30, 2023
Closed
@cristipaval cristipaval mentioned this issue Mar 30, 2023
Merged
55 tasks
@melvin-bot melvin-bot bot closed this as completed Apr 14, 2023
@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot removed the Overdue label Apr 14, 2023
This was referenced Apr 26, 2023
Closed
Closed
@kbecciv
Copy link

kbecciv commented Jul 28, 2023

Issue is reproduced in build 1.3.47.3

Screen.Recording.20230728.174309.Chrome.mp4

@kbecciv kbecciv reopened this Jul 28, 2023
@NajiNazzal
Copy link

I would like to contribute please

@melvin-bot
Copy link

melvin-bot bot commented Aug 6, 2023

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

  1. 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.
  2. 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.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@NajiNazzal
Copy link

Contributor details
Your Expensify account email: najinazzal98@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01a352a7b4bf7c67cd

@melvin-bot
Copy link

melvin-bot bot commented Aug 6, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@melvin-bot
Copy link

melvin-bot bot commented Oct 10, 2023

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

@melvin-bot melvin-bot bot closed this as completed Oct 10, 2023
@lanitochka17
Copy link

Issue is reproduced in build 1.3.83-1

Zoom_New.mp4

@lanitochka17 lanitochka17 reopened this Oct 12, 2023
@melvin-bot melvin-bot bot closed this as completed Dec 25, 2023
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Dec 25, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Engineering Improvement Item broken or needs improvement. Monthly KSv2 Not a priority
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@Jag96 @MelvinBot @mvtglobally @isagoico @kadiealexander @OSBotify @lanitochka17 @thesahindia @kbecciv @NajiNazzal