App icon indicating copy to clipboard operation
App copied to clipboard

[$1000] mWeb - Chat – Unable to zoom the image with pinching

Open kbecciv opened this issue 1 year ago • 62 comments

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

  1. Go to https://staging.new.expensify.com/
  2. Login
  3. Go to a report with attachments
  4. Open an (image) attachment
  5. 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-03-25
  • Automatic offers:
    • situchan | Contributor | 0

kbecciv avatar Feb 15 '24 15:02 kbecciv

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

melvin-bot[bot] avatar Feb 15 '24 15:02 melvin-bot[bot]

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

melvin-bot[bot] avatar Feb 15 '24 15:02 melvin-bot[bot]

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

melvin-bot[bot] avatar Feb 15 '24 15:02 melvin-bot[bot]

We think that this bug might be related #vip-vsb CC @quinthar

kbecciv avatar Feb 15 '24 15:02 kbecciv

Proposal

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

gestures won't work when viewing image attachments on the web app

What is the root cause of that problem?

new feature

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

on web ImageView component, if the device has a touch screen (canUseTouchScreen is true), https://github.com/Expensify/App/blob/8bdb753045cf0eee6cf1d342adf8b76040cabac4/src/components/ImageView/index.tsx#L194-L215

we display images using the Lightbox component instead of regular images. This will allow users to interact with the images using touch gestures.

 if (canUseTouchScreen) {
        return (
            <Lightbox
                uri={url}
                zoomRange={zoomRange}
                isAuthTokenRequired={isAuthTokenRequired}
                onError={onError}
                style={style}
            />
        );
    }

Lightbox component also requires additional zoomScale to define how images are resized.

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

this controls how the image fits within the frame when its original size differs from the frame's dimensions.

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

result:

https://github.com/Expensify/App/assets/60419079/6daf30ef-b1e4-4d73-8b11-1469b4046c81

https://github.com/Expensify/App/assets/60419079/33ef5846-5069-4caa-ae78-995cd4c799db

What alternative solutions did you explore? (Optional)

andreasnw avatar Feb 16 '24 04:02 andreasnw

Please reassign. I can't take it!

thesahindia avatar Feb 16 '24 21:02 thesahindia

I can take over.

We are not yet supporting zoom image in mobile web. Is it right time to implement this new feature?

situchan avatar Feb 18 '24 15:02 situchan

📣 @situchan 🎉 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 📖

melvin-bot[bot] avatar Feb 19 '24 16:02 melvin-bot[bot]

Asking internally here https://expensify.slack.com/archives/C066HJM2CAZ/p1708364190263029 whether we need to fix this

laurenreidexpensify avatar Feb 19 '24 17:02 laurenreidexpensify

Still chatting internally

laurenreidexpensify avatar Feb 22 '24 10:02 laurenreidexpensify

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

melvin-bot[bot] avatar Feb 23 '24 15:02 melvin-bot[bot]

Bumped thread internally for further clarity

laurenreidexpensify avatar Feb 23 '24 16:02 laurenreidexpensify

@situchan confirmed internally - let's do it!

laurenreidexpensify avatar Feb 23 '24 16:02 laurenreidexpensify

@laurenreidexpensify thanks

situchan avatar Feb 23 '24 16:02 situchan

@andreasnw thanks for the proposal. Please test your solution yourself on android chrome and iOS safari.

situchan avatar Feb 23 '24 16:02 situchan

Waiting proposals

situchan avatar Feb 23 '24 16:02 situchan

📣 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] avatar Feb 24 '24 16:02 melvin-bot[bot]

Proposal

Updated

andreasnw avatar Feb 25 '24 17:02 andreasnw

Proposal

What is the root cause of that problem?

We only have gestures like pinch on native because of the lightbox implementation here: https://github.com/Expensify/App/blob/8bdb753045cf0eee6cf1d342adf8b76040cabac4/src/components/ImageView/index.native.tsx

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

  1. Similar to @andreasnw, we want to replace the Image component in https://github.com/Expensify/App/blob/8bdb753045cf0eee6cf1d342adf8b76040cabac4/src/components/ImageView/index.tsx#L200

However additionally: With the lightbox implementation exactly as defined in https://github.com/Expensify/App/blob/8bdb753045cf0eee6cf1d342adf8b76040cabac4/src/components/ImageView/index.native.tsx

  1. We should clean up the native implementation as it will be a redudancy.
  2. We should ensure that the new lightbox implementation for touch devices, has the default values such as zoomRange with DEFAULT_ZOOM_RANGE from '@components/MultiGestureCanvas'; As well as other's as defined in the native lightbox implementation.
  3. We can remove the existing loading logic from. https://github.com/Expensify/App/blob/8bdb753045cf0eee6cf1d342adf8b76040cabac4/src/components/ImageView/index.tsx#L212 and rely in the Lightbox's loading logic
  4. We don't need a zoomScale on lightbox

What alternative solutions did you explore? (Optional)

jeremy-croff avatar Feb 25 '24 22:02 jeremy-croff

proposed implementation experience: Android: https://github.com/Expensify/App/assets/157416545/7f1d7a51-1dc8-4cfb-ac33-8fc24b648368 IOS:

https://github.com/Expensify/App/assets/157416545/7315f00a-0cc8-471e-a401-a873d32b9cdc

jeremy-croff avatar Feb 25 '24 22:02 jeremy-croff

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

Simulator Screenshot - iPhone 14 - 2024-02-26 at 21 00 30

andreasnw avatar Feb 26 '24 14:02 andreasnw

@jeremy-croff zoomScale is necessary to . The native implementation does not have this behaviour; either it's intentional, or if we want to add a scale factor, both experiencesges.githubusercontent.com/60419079/307819073-342ef517-062c-45d2-9bf3-56f89faffc37.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MDg5NTY3NDQsIm5iZiI6MTcwODk1NjQ0NCwicGF0aCI6Ii82MDQxOTA3OS8zMDc4MTkwNzMtMzQyZWY1MTctMDYyYy00NWQyLTliZjMtNTZmODlmYWZmYzM3LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAyMjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMjI2VDE0MDcyNFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWQ0Zjg2MGFlNWRkNDEwZmFkMGVmZDRlM2RlMzYyYTA1NmE5MzJiZWU3MGQ4YmQ1OGI2MGE1YTcxZmQwZjc5Y2MmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.Vg6CjitMNGlPA5VbSgW1FOM-FsdFT2ky1SElXEGUFSE)

Thanks for the example, So my basis is that it's not a requirement, the native implementation does not have this behaviour either it's intentional, or if we want to add a scale factor both experience will need it.

jeremy-croff avatar Feb 26 '24 14:02 jeremy-croff

@situchan some new proposals to review ^^

laurenreidexpensify avatar Feb 27 '24 06:02 laurenreidexpensify

@andreasnw @jeremy-croff do you mind sharing test branches?

situchan avatar Feb 28 '24 13:02 situchan

@andreasnw @jeremy-croff do you mind sharing test branches?

https://github.com/Expensify/App/pull/37424/files

jeremy-croff avatar Feb 28 '24 13:02 jeremy-croff

sure @situchan https://github.com/andreasnw/Expensify/tree/feat/web_app_gesture_handler

andreasnw avatar Feb 28 '24 13:02 andreasnw

@laurenreidexpensify @situchan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Feb 29 '24 15:02 melvin-bot[bot]

proposals are in review & testing

situchan avatar Feb 29 '24 15:02 situchan

📣 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] avatar Mar 02 '24 16:03 melvin-bot[bot]

@situchan any update on the review and testing yet? thanks

laurenreidexpensify avatar Mar 04 '24 05:03 laurenreidexpensify