[HOLD for payment 2024-07-22] [HOLD for payment 2024-07-17] [$1000] mWeb - Chat – Unable to zoom the image with pinching

kbecciv commented 7 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.42-1 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Interna; Team Slack conversation:

Issue found when executing PR https://github.com/Expensify/App/pull/34080

Action Performed:

Go to https://staging.new.expensify.com/
Login
Go to a report with attachments
Open an (image) attachment
Check that image gestures/transformations are working (pinching)

Expected Result:

Able to zoom the image with pinching

Actual Result:

Unable to zoom the image with pinching

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[x] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/ce464be6-14ca-4d7a-a2eb-598f8ae21ba6

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~016170309acaa20014
Upwork Job ID: 1758158238713884672
Last Price Increase: 2024-06-04
Automatic offers:

situchan | Contributor | 0
dukenv0307 | Reviewer | 102684851
ZhenjaHorbach | Contributor | 102711497

Issue Owner

Current Issue Owner: @dukenv0307

situchan commented 6 months ago

@jeremy-croff zoomScale is necessary to prevent tiny images from looking like this

@andreasnw your solution still doesn't fix this.

Please check this video:

https://github.com/Expensify/App/assets/108292595/402a6333-c5d3-49cd-8406-ffdebbacfef1

situchan commented 6 months ago

@jeremy-croff same feedback ^ unless this isn't your up-to-date branch

situchan commented 6 months ago

@laurenreidexpensify can we double the bounty? We should consider many cases to avoid any regressions.

andreasnw commented 6 months ago

@situchan , can you confirm whether you've defined zoomScale and applied validation to resizeMode in your code?

const zoomScale = Math.min((canvasSize?.width ?? 0) / (contentSize?.width ?? 0), (canvasSize?.height?? 0) / (contentSize?.height ?? 0));

 <MultiGestureCanvas
                                isActive={isActive}
                                canvasSize={canvasSize}
                                contentSize={contentSize}
                                zoomRange={zoomRange}
                                pagerRef={pagerRef}
                                shouldDisableTransformationGestures={isPagerScrolling}
                                onTap={onTap}
                                onScaleChanged={scaleChange}
                            >
                                <Image
                                    source={{uri}}
                                    style={contentSize ?? DEFAULT_IMAGE_DIMENSION}
                                    isAuthTokenRequired={isAuthTokenRequired}
                                    onError={onError}
                                    onLoad={updateContentSize}
                                    onLoadEnd={() => {
                                        setLightboxImageLoaded(true);
                                    }}
                                    resizeMode={zoomScale > 1 ? RESIZE_MODES.center : RESIZE_MODES.contain}
                                />
                            </MultiGestureCanvas>

The problem seems to have been resolved on my device after both these changes.

https://github.com/Expensify/App/assets/60419079/0991b2c8-740b-4043-b0e2-fea8bc0b23ea

Do you mind to share image you used for testing?

situchan commented 6 months ago

@andreasnw yes, I tested https://github.com/Expensify/App/compare/main...andreasnw:Expensify:feat/web_app_gesture_handler

Here's image:

andreasnw commented 6 months ago

sorry @situchan , still unable to reproduce this on android native with latest main branch. it has the same behavior as the mweb.

https://github.com/Expensify/App/assets/60419079/0c1fd027-3084-4710-b5d6-dfe83dc6d4d5

situchan commented 6 months ago

@andreasnw my video is from your solution. So it causes regression - small size on native. If you consider this as new feature, make mWeb consistent. Also try dragging image vertically and horizontally. There are another regressions. Please try to find yourself.

melvin-bot[bot] commented 6 months ago

Upwork job price has been updated to $1000

laurenreidexpensify commented 6 months ago

Increased bounty

jeremy-croff commented 6 months ago

@jeremy-croff same feedback ^ unless this isn't your up-to-date branch

In regards to the "swiping-up" for closing the attachment preview, that was implemented for native approx a month ago: https://github.com/Expensify/App/pull/37080 It's not really related to pinching or lightbox, but if we want to include that feature as well, we just need to pass in the onClose call back context for the web flow to the pan gestures that trigger the closing of the modal from the original implementation https://github.com/Expensify/App/assets/157416545/415063c5-9a3a-4d6e-9921-96f7599b6d9d I pushed that to my branch so that it starts behaving the same.

https://github.com/Expensify/App/assets/157416545/699c666c-1e84-4173-b750-86e6f85cc9c8

In regards to testing everything else, I'm not seeing any major issues or gaps. I've tested it thoroughly. However, I found that PDF's have separate implementations for mobile and web, so I see that pinching to zoom doesn't work there yet on my branch either.

situchan commented 6 months ago

@jeremy-croff please check this video. Not able to move to prev/next image via horizontal swipe when not zoomed.

https://github.com/Expensify/App/assets/108292595/4acf8830-18d1-4621-8a12-c1ec4daeaffe

dukenv0307 commented 6 months ago

Happy to take over as C+ since @situchan is OOO in April

melvin-bot[bot] commented 6 months ago

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

mallenexpensify commented 6 months ago

@situchan is out OOO, reassigning a C+ via the auto-assigner for fairness.

melvin-bot[bot] commented 6 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

abdulrahuman5196 commented 5 months ago

Reviewing now

abdulrahuman5196 commented 5 months ago

There are 2 proposals here 1) @andreasnw 's proposal here https://github.com/Expensify/App/issues/36597#issuecomment-1947749686 2) @jeremy-croff 's proposal here https://github.com/Expensify/App/issues/36597#issuecomment-1963077617

On first look, both the proposals are more than 1 month old. And the second proposal is almost similar to 1st.

I also saw multiple discussions around potential solution.

I would request the contributors to update the proposal as per your latest suggestion and add the custom branches which they have already build.

melvin-bot[bot] commented 5 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

laurenreidexpensify commented 5 months ago

Waiting for proposal updates

laurenreidexpensify commented 5 months ago

Waiting for proposals

melvin-bot[bot] commented 5 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

laurenreidexpensify commented 5 months ago

@abdulrahuman5196 bump for review ^^

laurenreidexpensify commented 5 months ago

@abdulrahuman5196 bump for review ^^

melvin-bot[bot] commented 5 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

laurenreidexpensify commented 5 months ago

@abdulrahuman5196 bump for review ^^

badeggg commented 5 months ago

After reading through the whole thread and digging some related code, I don't think it's a good idea to reimplement pinch-to-room feature dedicated for mWeb. Lightbox is an implementation currently used on native, we should use it on mWeb too. While some bugs must be fixed when used on mWeb.

proposal here and here are doing the similar thing just in difference that "some bugs must be fixed when used on mWeb". The feedback from c+ is that they are similar and both have potential regressions.

I don't dare to put more effort into fixing "bugs that must be fixed when used on mWeb" because I'm afraid getting feedback like "you proposal only has minor changes compared to others and those minor changes and code refactor could be addressed in PR review and testing" like comment in here

Can I get clarification about this?

abdulrahuman5196 commented 5 months ago

Checking now

abdulrahuman5196 commented 5 months ago

@jsdev2547 Could you share the test code you had used in a separate branch? It would be time consuming to re-create the proposal for testing.

melvin-bot[bot] commented 5 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

badeggg commented 5 months ago

Proposal

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

On mWeb, we can not pinch-to-zoom picture attachments.

What is the root cause of that problem?

new feature

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

First of all, LightBox (src/components/Lightbox/index.tsx) has implemented pinch-image feature, and native platforms are using it. We definitely don't want to re-create a wheel for mWeb.

In file src/components/ImageView/index.tsx, when touch screen is usable, we should use LightBox instead of Image
We should upgrade react-native-gesture-handler from 2.14.1 --> 2.16.2
About react-native-gesture-handler:
- Like I said in comment, react-native-gesture-handler@2.16.2 leads to iOS build crash. I find out the causing line is here. Change this line to
```
auto shadowNode = arguments[0].asObject(runtime).getHostObject<ShadowNodeWrapper>(runtime)->shadowNode;
```
  which is identical in version 2.14.1 here, iOS builds ok. I have reported a bug here. Before this react-native-gesture-handler bug is fixed, we can use patch-package to workaround.
- react-native-gesture-handler@2.16.2 has fixed this problem that I mentioned in comment
  
  Unable to make FlatList to be part of react-native-gesture-handler's gesture system. That's why we can't swipe to switch after change Image to Lightbox, like this I have tried every possible way I can imagine. So we have to do scrolling by self
- After upgrade react-native-gesture-handler, image zoom works fine on safari/iOS and chrome/android. Double tap to zoom to scale 1 still not working for chrome/iOS, If we want to be compatible with chrome for iOS, we can count tap number by hand.

Check my test branch here

What alternative solutions did you explore? (Optional)

N/A

badeggg commented 5 months ago

LightBox has implemented pinch-image feature, and native platforms are using it. We definitely don't want to re-create a wheel for mWeb.
Since my concern here, I must say that my proposal is not just similar to proposal here and here , I tuned several non-negligible situation that must be considered when pinch-image feature is up for mWeb
I can create a test branch if needed

melvin-bot[bot] commented 4 months ago

@abdulrahuman5196, @laurenreidexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

abdulrahuman5196 commented 4 months ago

Hi will check the test branch and new proposal in my morning

abdulrahuman5196 commented 4 months ago

Hi, I don't have bandwidth to work on this issue. Unassigning myself.

@laurenreidexpensify Kindly reassign the external label, so that a C+ is assigned randomly.

melvin-bot[bot] commented 4 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

Upwork job price has been updated to $1000

laurenreidexpensify commented 4 months ago

@dukenv0307 ypu've been assigned as new C+ - pls confirm if you good to take over. thanks

dukenv0307 commented 4 months ago

I can take over @laurenreidexpensify

laurenreidexpensify commented 4 months ago

Thanks @dukenv0307 pls review latest proposals and provide an update

badeggg commented 4 months ago

I noticed that some other changes need be made besides the changes listed in my proposal. I will update my proposal in a few days and provide a test branch. Fixing this issue involves too much.

dukenv0307 commented 4 months ago

I'm still reviewing proposals

dukenv0307 commented 4 months ago

@andreasnw @jeremy-croff I tested your proposals. But I faced with the following bug (that didn't happen on latest main). Pls let me know if I missed sth

https://github.com/Expensify/App/assets/129500732/01c2c295-32d4-40c7-b33b-13e19b19674c

@badeggg Can you estimate when you finish updating your proposal?

dukenv0307 commented 4 months ago

@jsdev2547 i'm checking...

badeggg commented 4 months ago

I am canceling my proposal, because I have run into severe struggling to do lots of workaround. I can share my work-in-pregress code branch if it help the issue.

Unexpected situations(if I'm correct):

Unable to make FlatList to be part of react-native-gesture-handler's gesture system. That's why we can't swipe to switch after change Image to Lightbox, like this I have tried every possible way I can imagine. So we have to do scrolling by self
manualActivation(true) of Gesture.Pan() not working on mWeb, react-native-gesture-handler version 2.14.1. Later versions(2.15.0, 2.16.0, 2.16.1, 2.16.2) lead to ios build crash. We have to manage active state by self.
double tap .numberOfTaps(2) not working on chrome for iOS, does work on safari on iOS If we want to be compatible with chrome for iOS, one solution is to count tap number by hand. Or maybe a latter version of react-native-gesture-handler will be ok, current react-native-gesture-handler version is 2.14.1. Also notice that few later versions(2.15.0, 2.16.0, 2.16.1, 2.16.2) lead to iOS build crash.
react-native-gesture-handler@2.14.1 may not reporting 'allTouches' when onTouchesMove correctly on mWeb, stale touch may exist.
maybe more...

cc @dukenv0307 and everyone here

dukenv0307 commented 4 months ago

@jsdev2547 I checked your proposal. It worked well when I zoomed the image with pinching but I can't drag the image

https://github.com/Expensify/App/assets/129500732/4e77ef8a-bfa9-4e7a-86ac-3138ec9f17bd

dukenv0307 commented 4 months ago

It worked well on native, so we need to fix that

https://github.com/Expensify/App/assets/129500732/70b503f1-2a3b-4fa4-9d07-e450f94c1c93

dukenv0307 commented 4 months ago

@jsdev2547 Thanks for your update. I'll verify your branch in a few hours

melvin-bot[bot] commented 4 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

dukenv0307 commented 4 months ago

@jsdev2547 Your proposal looks promising, I can drag and zoom but we need to improve

We should have the limitation for zoom in/out
At the original state we shouldn't allow users to drag the image

Pls refer the behavior on Android/iOS native. We want to make it consistency

https://github.com/Expensify/App/assets/129500732/92be3663-45f5-4551-b7ae-4cf277150c9e

Expensify / App