Web - Attachments - Unable to get to the top of images with long height and not able to zoom

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

---

Issue was reproduce when executing #6442

Action Performed:

1. Open newDot and login
2. Go to a chat with a really long attachment
3. Click on it to preview

Expected Result:

User is able to see whole image and scroll thorough all of it

Actual Result:

User can't reach very top of the image by either scrolling or zooming

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.1.17-0
Reproducible in staging?: Yes
Reproducible in production?: No
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
Long image

Issue was reproduce when executing #6442

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

View all open jobs on GitHub

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[MelvinBot]

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

[aswin-s]

Looks like this was introduced in #6437.

On line 107, top of the image is being set to a negative value when image height is greater than container height. This prevents the scroll from ever reaching the top of the image.

App/src/styles/StyleUtils.js

Lines 93 to 115 in 2b7d6d0

	function getZoomSizingStyle(isZoomed, imgWidth, imgHeight, zoomScale, containerHeight) {
	if (imgWidth === 0 || imgHeight === 0) {
	return {
	height: isZoomed ? '250%' : '100%',
	width: isZoomed ? '250%' : '100%',
	};
	}
	if (isZoomed) {
	return {
	height: `${(imgHeight * zoomScale)}px`,
	width: `${(imgWidth * zoomScale)}px`,
	};
	}
	const top = `${(containerHeight - imgHeight) / 2}px`;
	const left = `calc(50% - ${imgWidth / 2}px)`;
	return {
	height: `${imgHeight}px`,
	width: `${imgWidth}px`,
	top,
	left,
	};
	}

This can be fixed by to modifying the line to something like below, which will set top to 0 for extra long images.

    const top = `${Math.max((containerHeight - imgHeight) / 2, 0)}px`;

[parasharrajat]

@railway17 could you please fix the issue?
As @aswin-s pointed out that this is regression from your PR #6437

Thanks @aswin-s for investigation.

[railway17]

@parasharrajat, @aswin-s , @deetergp
I've created a new pull request for this regression
Thank you

[MelvinBot]

📣 @railway17 You have been assigned to this job by @johnmlee101!
Please apply to this job in Upwork 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 📖

[railway17]

📣 @railway17 You have been assigned to this job by @johnmlee101! Please apply to this job in Upwork 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 📖

Where is Upwork job link?

[parasharrajat]

That message is deceptive. This is a regression so there won't be any job for it.

[railway17]

That message is deceptive. This is a regression so there won't be any job for it.

Ah..
I was also confused.
Thank you for your letting me know.
And waiting for your feedback from another reviewers

[roryabraham]

Okay, this is no longer a deploy blocker, but it seems like this issue is reproducible in production on android. So @railway17 we'll need to get this fixed because it was a regression caused by #6442 that was not fixed by #6535

[roryabraham]

@railway17 Here's a draft PR which has some changes that should be included in the fix: #6599

[johnmlee101]

Where are we with this?

[railway17]

I've attached my video in #6326 below.
https://user-images.githubusercontent.com/29966461/145676609-ea158234-146c-4e2b-8ea9-d25a1517c8ed.mp4
Actually, it is deployed in both staging and production, you can also check if you want.
If you think it is fixed, please close this issue.
Thank you.

[johnmlee101]

@kavimuru could this be re-tested and closed if resolved?

[isagoico]

Issue is still reproducible

Screen_Recording_20211213-150141_New.Expensify.mp4

[railway17]

Issue is still reproducible

Screen_Recording_20211213-150141_New.Expensify.mp4

Ok, I got it.
I have just tested it in Web version because issue was marked with only Web.
Let me try to fix and create a new pull request for this.
Thank you.

[railway17]

Hi, @roryabraham , @johnmlee101
As I commented here, this issue is not related to my fix for #6326.

But I have also noticed that the Android version has an important bug regarding this.
While testing with earlier Android version (I've tested with Android 8 simulator and real device), it is crashing after displaying image modal.
Please look at my testing video here:

20211215004816703.mp4

I think these 2 mobile issues should be treated as another ticket.
Thank you
Han

[MelvinBot]

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

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[MelvinBot]

@johnmlee101, @railway17 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[johnmlee101]

Going to see if this remaining not reproducible for another week.

I think these 2 mobile issues should be treated as another ticket.

Can you propose this in the contributor Slack? That way we can go through the process to tackle this 😄

[railway17]

Going to see if this remaining not reproducible for another week.

I think these 2 mobile issues should be treated as another ticket.

Can you propose this in the contributor Slack? That way we can go through the process to tackle this 😄

I reported this issue in contributor Slack

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[MelvinBot]

@johnmlee101, @railway17 Huh... This is 4 days overdue. Who can take care of this?

[johnmlee101]

Can you link the Slack message? 😄

[mvtglobally]

Issue not reproducible during KI retests. (Third week) Should we close this @johnmlee101?

[johnmlee101]

Let's do it!