fix issue 6326: Chat - Magnifying glass displayed anywhere

[railway17]

Details

• Because cursor: zoom-in style was assigned to the ancient of the Image component, this cursor was displayed out of the image area.
  To resolve it, I reassign this style to image (Actually, I assigned it to Pressable component because the Image component is wrapped by it).
• While reassigning this style, I had to keep the center alignment of the image in the modal.
  For center alignment, I calculated the offset based on modal size and image size.
• When the image is expanded, it was using scaled width and height. I have kept this code like below

   if (isZoomed) {
        return {
            height: `${(imgHeight * zoomScale)}px`,
            width: `${(imgWidth * zoomScale)}px`,
        };
    }

Fixed Issues

$ GH_LINK

Tests

1. Send the image to someone.
2. Click the image thumbnail in the chatbox.
3. Modal is open to display the image.
4. Image is center aligned.
5. Check the cursor while hovering around the image.
6. Check the zoom in and zoom out functionality.

QA Steps

1. Send the image to someone.
2. Click the image thumbnail in the chatbox.
3. Check the center alignment of an image in the modal.
4. Check the cursor while hovering around the image.
5. Check the zoom in and zoom out functionality.

Tested On

This issue is just a hovering issue, don't need to check for mobile platforms.

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

web fix video

Desktop

desktop fix video

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[railway17]

I have read the CLA Document and I hereby sign the CLA

[deetergp]

The code looks good and tests well when the image is zoomed out, but once the image is zoomed in, the magnifying glass goes back to appearing everywhere. Can you update the zoomed in behavior to match the zoomed out?

2021-11-23_15-38-08.mp4

[railway17]

As I mentioned in 3) of the pull request detail, I kept the width and height when it is zoomed ((it is original code) because I thought you want this size.
But I got it you need the same feature even though the image is zoomed.
I will fix it and update you shortly

[deetergp]

Fair, though it didn't really make sense till I saw it in action — after seeing it, I think it makes more sense to have it behave the same.

I will fix it and update you shortly

Thanks!

[railway17]

@deetergp
Please review new commits.
I calculated the zoomScale again by using Math.min instead of Math.max based on modal size.
I am attaching my testing video again.

- new web fix
- new desktop fix

[deetergp]

Looks good and tests well. Thanks!

[OSBotify]

@railway17, Great job getting your first Expensify/App pull request over the finish line! 🎉

I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:

1. Now that your first PR has been merged, you can be hired for another issue. Once you've completed a few issues, you may be eligible to work on more than one job at a time.
2. Once your PR is deployed to our staging servers, it will undergo quality assurance (QA) testing. If we find that it doesn't work as expected or causes a regression, you'll be responsible for fixing it. Typically, we would revert this PR and give you another chance to create a similar PR without causing a regression.
3. Once your PR is deployed to production, we start a 7-day timer ⏰. After it has been on production for 7 days without causing any regressions, then we pay out the Upwork job. 💰

So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[OSBotify]

🚀 Deployed to staging by @deetergp in version: 1.1.16-11 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[ogumen]

The accessibility issue found in this PR:

1. No keyboard alternative is provided for zoom-in / zoom-out functionality - failure of WCAG SC 2.1.1

No.keyboard.alternative.provided.to.zoom-in.functionality.mp4

[railway17]

The accessibility issue found in this PR:

1. No keyboard alternative is provided for zoom-in / zoom-out functionality - failure of WCAG SC 2.1.1

No.keyboard.alternative.provided.to.zoom-in.functionality.mp4

Was there any keyboard functionality before I've changed before?
Because I am the first contributor and didn't report for keyboard function, I didn't consider it.

[ogumen]

The accessibility issue found in this PR:

1. No keyboard alternative is provided for zoom-in / zoom-out functionality - failure of WCAG SC 2.1.1

No.keyboard.alternative.provided.to.zoom-in.functionality.mp4

Was there any keyboard functionality before I've changed before? Because I am the first contributor and didn't report for keyboard function, I didn't consider it.

@railway17 no, there was no keyboard alternative provided for zoom-in functionality before the latest changes. Tagging @roryabraham for an input on accessibility issues, but I think they are not deploy blockers for any PR.

[railway17]

The accessibility issue found in this PR:

1. No keyboard alternative is provided for zoom-in / zoom-out functionality - failure of WCAG SC 2.1.1

No.keyboard.alternative.provided.to.zoom-in.functionality.mp4

Sorry, but I didn't understand your issue.
What does mean keyboard alternative?
What are the current results and the expected result?

[roryabraham]

@railway17 We're piloting an in-sprint accessibility testing program to improve the UI/UX of our app as it is consumed by users with various forms of impairments. This program is still in its infancy, and we don't need to address accessibility issues at this time. @ogumen (and their team) test PRs and post issues found to gain familiarity with our process and application, and to provide educational opportunities for us as engineers to be able to identify and fix these accessibility issues.

Thanks @ogumen! I'll create a follow-up issue to address this shortcoming. Quick question before I do: our zoom-in functionality zooms into different areas on the image depending on where you click on it (i.e cursor position). How do you recommend we implement that sort of functionality with a keyboard alternative?

[railway17]

@railway17 We're piloting an in-sprint accessibility testing program to improve the UI/UX of our app as it is consumed by users with various forms of impairments. This program is still in its infancy, and we don't need to address accessibility issues at this time. @ogumen (and their team) test PRs and post issues found to gain familiarity with our process and application, and to provide educational opportunities for us as engineers to be able to identify and fix these accessibility issues.

Thanks @ogumen! I'll create a follow-up issue to address this shortcoming. Quick question before I do: our zoom-in functionality zooms into different areas on the image depending on where you click on it (i.e cursor position). How do you recommend we implement that sort of functionality with a keyboard alternative?

Ok, I got it.

@ogumen
Please take a look issue report for this pull request if you like to see it.
Maybe, it will help you to notice what you have to test at this moment.

[ogumen]

@railway17 We're piloting an in-sprint accessibility testing program to improve the UI/UX of our app as it is consumed by users with various forms of impairments. This program is still in its infancy, and we don't need to address accessibility issues at this time. @ogumen (and their team) test PRs and post issues found to gain familiarity with our process and application, and to provide educational opportunities for us as engineers to be able to identify and fix these accessibility issues.

Thanks @ogumen! I'll create a follow-up issue to address this shortcoming. Quick question before I do: our zoom-in functionality zooms into different areas on the image depending on where you click on it (i.e cursor position). How do you recommend we implement that sort of functionality with a keyboard alternative?

Thank you @roryabraham. As of now after hovering over and clicking on the image in the Attachment dialog, a portion of the image is zoomed in, and user can move over the other parts of the image using arrow keys (this is great!). My recommendation is to provide a toggle button which will zoom in / zoom out the image once activated (the same functionality as the magnifying glass icon does on click).
As a side note, the left and up edge of the enlarged image attachment are cut off (it is a functional issue, just thought it might be useful).

[OSBotify]

🚀 Deployed to production by @roryabraham in version: 1.1.17-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅