Chat - Magnifying glass displayed anywhere on the modal page for small images

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Launch the app and login
2. Add a small size image as attachment and send the image to any chat
3. Click on the image thumbnail, it should open modal with the image
4. Verify you see a magnifying glass when hovering over the image

Expected Result:

Magnifying glass displayed only when hovering the image

Actual Result:

Magnifying glass displayed on all the places

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.1.15.1
Reproducible in staging?: Yes
Reproducible in production?: Yes
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Bug5330197_Recording__1115.mp4

Expensify/Expensify Issue URL:
Issue reported by: Applause
Slack conversation:

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @timszot (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

Triggered auto assignment to @mallenexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MelvinBot]

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

[mallenexpensify]

Job posted
https://www.upwork.com/jobs/~014d495c62816efa38

[CamilaRivera]

Proposal

We can calculate the pressable size by calculating the final size of the image. The final size of the image can be calculated using the size of the container (extracted with onLayout) and the original size of the image (this.state.imgHeight and this.state.imgWidth).

We will need to add the following functions to src/components/ImageView/index.js:

    storeImageContainerSize(event) {
        if (this.state.imageContainer) {
            // just do it first time to avoid potentially falling in a loop of update image size -> layout changes size -> update image size -> etc.
            return;
        }
        this.setState({
            imageContainer: {
                height: event.nativeEvent.layout.height,
                width: event.nativeEvent.layout.width,
                ratio: event.nativeEvent.layout.height / event.nativeEvent.layout.width,
            }
        });
    }

    shouldUseImageSize() {
        // We can use the calculated image size if we have all required sizes (original image size + container size) and we are not zoomed in
        if (!this.state.imageContainer || !this.state.imgHeight || !this.state.imgWidth || this.state.isZoomed) {
            return false;
        }
        return true;
    }

    getFinalImageWidth() {
        if (this.state.imageContainer.width >= this.state.imgWidth && this.state.imageContainer.height >= this.state.imgHeight) {
            // Image fits completely
            return this.state.imgWidth;
        }
        const imageAspect = this.state.imgWidth / this.state.imgHeight;
        const imageContainterAspect = this.state.imageContainer.width / this.state.imageContainer.height;
        if (imageAspect > imageContainterAspect) {
            // Image is wider than container
            return '100%';
        }

        // Image is taller than container
        return this.state.imageContainer.height * imageAspect;
    }

    getFinalImageHeight() {
        if (this.state.imageContainer.width >= this.state.imgWidth && this.state.imageContainer.height >= this.state.imgHeight) {
            // Image fits completely
            return this.state.imgHeight;
        }
        const imageAspect = this.state.imgWidth / this.state.imgHeight;
        const imageContainterAspect = this.state.imageContainer.width / this.state.imageContainer.height;
        if (imageAspect > imageContainterAspect) {
            // Image is wider than container
            return this.state.imageContainer.width / imageAspect;
        }

        // Image is taller than container
        return '100%';
    }

We will adjust the current Pressable height and width:
https://github.com/Expensify/App/blob/main/src/components/ImageView/index.js#L171-L177

like this:

                <Pressable
                    style={[
                        this.shouldUseImageSize() ? {height: this.getFinalImageHeight()} : styles.h100,
                        this.shouldUseImageSize() ? {width: this.getFinalImageWidth()} : styles.w100,
                        getZoomCursorStyle(this.state.isZoomed, this.state.isDragging),
                    ]}

And the store the layout container size from Image:
https://github.com/Expensify/App/blob/main/src/components/ImageView/index.js#L213-L217

like this:

                    <Image
                        onLayout={e => this.storeImageContainerSize(e)}
                        source={{uri: this.props.url}}
                        style={getZoomSizingStyle(this.state.isZoomed, this.state.imgWidth, this.state.imgHeight, this.state.zoomScale)}
                        resizeMode={this.state.isZoomed ? 'contain' : 'center'}
                    />

[railway17]

1. Replication
I also was able to reproduce this issue in my web, desktop environment.
Here are my replication videos
- web issue
- desktop issue

2. Problems
Problem is that cursor: zoom-in style was set to the parent component, but the parent size is bigger than the image itself when the modal is open.

3. Solution
We need to set the style cursor: zoom-in to the image area.

• Because the current image is clickable and wrapped by Pressable component, we need to set this style to the Pressable component.
• While processing this issue, we need to keep the placement of the small image in the center of the modal.
  To resolve this issue, we might use the margin style, but it will not resolve the hover area correctly. It means the zoom icon will be displayed on the margin area. So we need to use position: absolute, left: XXpx, top: YYpx property.
• While calculating the top value, we need to know the modal height. It can be get by onLayout event of the parent view.
• By doing this, we will show the image at the center of the modal dialog with its original size and zoom-in cursor will be displayed only on the image area.

This is my solution to resolve this issue.

[mallenexpensify]

@deetergp , can you please review @CamilaRivera and @railway17 's proposals above?

[deetergp]

Thank you both for your proposals @CamilaRivera & @railway17's. After reviewing them both, I'm inclined to go with @railway17 proposal is it's appears to get the job done with fewer changes.

[MelvinBot]

📣 @railway17 You have been assigned to this job by @deetergp!
Please apply to this job in Upwork and let us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[mallenexpensify]

Hired @railway17 / MingYun H. in Upwork for the job.

[railway17]

@mallenexpensify , @deetergp
Nice to meet you.
I have a coincidence issue with my pc.
Actually, I reinstalled my OS to Big Sur after submitting my proposal on Sunday (my old OS version was Catalina).
As you saw in my proposal, I was able to replicate and test on the desktop app.
- desktop issue.
But I can't start the desktop app (web is working) on my side.
So, I can't upload a working video screenshot in the pull request. (this function was tested in my old system when I submit the proposal).
My current system info like below

macOS Big Sur
version 11.5.2(20G95)

I tried with node 12.x, 14.x and 17.x by using nvm.
But not working in all of the cases.
Error is like below:

Even though it was tested before, I would like to keep the rule to write the pull request description with attachments.
Do you know how to run the desktop app in this environment?

[railway17]

I created a pull request with 2 videos (web fix was taken from my pc but desktop-fix video was taken from another pc).
I will check this issue while you review it or might be later. And will submit here why it would not work.

[mallenexpensify]

Hi @railway17 , this is likely a better question for @deetergp who will be the one reviewing the PR. Scott, can you take a look above plz

[railway17]

Hi, @deetergp , @mallenexpensify
I resolved this error by upgrading the dev package versions to latest one in package.json like below:

   ...
    "electron": "^16.0.1",
    "electron-builder": "^22.14.5",
    "electron-notarize": "^1.1.1",
    "electron-reloader": "^1.2.1",
   ....

[deetergp]

Hey @railway17. Here are the versions I'm running on my Big Sur machine:

➜  App git:(han-fix-magnifying-glass) ✗ node --version
v14.17.4
➜  App git:(han-fix-magnifying-glass) ✗ npm --version
6.14.14
➜  App git:(han-fix-magnifying-glass) ✗ nvm --version
0.38.0
➜  App git:(han-fix-magnifying-glass) ✗

Generally, when I run into these kinds of runtime issues, I find that just deleting the node_modules folder and re-running npm i takes care of them. FWIW, I was able to run your branch on the desktop app without any problems.

[parasharrajat]

We are tracking are a regression from this issue's PR. #6518. It must be fixed before this issue can be paid out.

Also, this is @railway17 's first issue/PR in our repo so I think further assignment should be restrained until the regression is fixed,

[roryabraham]

Just to reiterate and confirm what @parasharrajat said above, this issue should not be considered complete until #6518 is resolved as well.

[railway17]

Hi, @roryabraham , @parasharrajat
Please check the below video.
I have just tried to test again but still not sure what you are referring to about #6518.
As you can see, It is fully scrollable from top to bottom.
Actually, I created pull request after enough tests.
If you think we have to fix it further, please let me know.
Thank you

20211211201522018.mp4

[railway17]

Hi, @roryabraham , @parasharrajat
Could you please check my video and let me know if you have any feedback?
If not, we can close this issue with the Upwork job because it is already deployed in production.
Thank you

[roryabraham]

@railway17 have you tried testing this on a touch-screen device?

[railway17]

@railway17 have you tried testing this on a touch-screen device?

Touch screen means mobile devices?
Web browser in mobile devices?

Because #6518 is tagged with Web platform, I have just tried in Desktop browser and desktop app.
Testing video was attached to #6535

[railway17]

@roryabraham
I was able to reproduce this issue in the Android 11 simulator. (Usually, I use Android 8 device so didn't notice it earlier).
But also noticed that this issue was caused App/src/components/ImageView/index.native.js.
Actually, the #6326 issue was to fix the zoom-icon issue in the Web/Desktop app while hovering, so I fixed it by using App/src/components/ImageView/index.js and App/src/styles/StyleUtils.js (Actually I made my CSS change on styles/styles.js at first, but was moved to later here).

But for now, mobile apps never use my changes.
So, #6518 is never related to my changes.
Even though, do you still think I have to fix #6518 in this issue?

[iwiznia]

Not 100% sure, but it's possible that this PR added this regression

[railway17]

Hi, @roryabraham
Have you got my comment?

[railway17]

Not 100% sure, but it's possible that this PR added this regression

Those are not related, @iwiznia
Please check this issue too.

[parasharrajat]

PR fixing the regression found earlier creates another regression #6714.

cc: @railway17 @roryabraham @mallenexpensify

[parasharrajat]

List of active regressions

1. [Reporting bonus paid] Attachment view shows image as left aligned. It should be horizontally centered - Reported by: @Prashant Mangukiya #6714
2. Web - Attachments - Unable to get to the top of images with long height and not able to zoom #6518.

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[mallenexpensify]

Looks like we're holding on this before payment is issued.... right?

[railway17]

Looks like we're holding on this before payment is issued.... right?

I think there is no reason why we should hold this issue now.

[mallenexpensify]

@parasharrajat @deetergp can you help here? With the regressions, I'm unsure when I should pay

[parasharrajat]

Aha, this issue is all spread out. @mvtglobally Could you please retest it once and let us know if this is fixed? Thanks.

[mvtglobally]

This specific Issue not reproducible during KI retests. (Third week). We can close it.
Other regressions are tracked separately

[railway17]

This specific Issue not reproducible during KI retests. (Third week). We can close it. Other regressions are tracked separately

Thank you, @mvtglobally .
So, can we also finish Upwork job now?
Or need to wait for the next deployment?

[parasharrajat]

Ok, Looks like both regressions are fixed now. @mallenexpensify.

[mallenexpensify]

Thanks @parasharrajat for the update. Thanks @railway17 for the help, paid in Upwork, $250.

[railway17]

Thanks @parasharrajat for the update. Thanks @railway17 for the help, paid in Upwork, $250.

Thank you, @mallenexpensify