[HOLD for payment 2023-05-22] [$1000] Loading spinner is cut off on long images when reopening the chat

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Sent a long image in a chat.
2. Closed the chat window.
3. Reopened the chat window containing the sent long image.
4. Observed the loading spinner that appears while the image is still loading.

Expected Result:

The loading spinner should be fully visible and centered on the screen while the long image is still loading after reopening the chat window. The thumbnail should be capped at a maximum height and minimum width so that the loading spinner is displayed properly.

Actual Result:

The loading spinner is cut off on both the left and right sides, with only a small portion of the spinner visible.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.5.4
Reproducible in staging?: Needs reproduction
Reproducible in production?: Needs reproduction
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
Notes/Photos/Videos: Any additional supporting documentation

2023-04-25.21-56-41.online-video-cutter.com.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Harshdeepjoshi
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682442117599109

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0105084e7856349f53
• Upwork Job ID: 1651749591472959488
• Last Price Increase: 2023-04-28

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[shawnborton]

Let's first discuss in Slack

[Harshdeepjoshi]

Proposal

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

Loading spinner is cut off on long images when reopening the chat

What is the root cause of that problem?

The problem arises due to the aspect ratio of long images, which is causing the thumbnailScreenWidth to become narrower than the width of the spinner.

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

Solution A

We can cap the max-height and limit the min-width of the thumbnail by updating this function

App/src/components/ThumbnailImage.js

Lines 55 to 76 in f8acfae

	calculateThumbnailImageSize(width, height) {
	if (!width || !height) {
	return {};
	}
	// Width of the thumbnail works better as a constant than it does
	// a percentage of the screen width since it is relative to each screen
	// Note: Clamp minimum width 40px to support touch device
	let thumbnailScreenWidth = lodashClamp(width, 40, 250);
	const imageHeight = height / (width / thumbnailScreenWidth);
	let thumbnailScreenHeight = lodashClamp(imageHeight, 40, this.props.windowHeight * 0.40);
	const aspectRatio = height / width;
	// If thumbnail height is greater than its width, then the image is portrait otherwise landscape.
	// For portrait images, we need to adjust the width of the image to keep the aspect ratio and vice-versa.
	if (thumbnailScreenHeight > thumbnailScreenWidth) {
	thumbnailScreenWidth = Math.round(thumbnailScreenHeight * (1 / aspectRatio));
	} else {
	thumbnailScreenHeight = Math.round(thumbnailScreenWidth * aspectRatio);
	}
	return {thumbnailWidth: thumbnailScreenWidth, thumbnailHeight: Math.max(40, thumbnailScreenHeight)};
	}

Which is responsible to calculate Thumbnail ImageSize ,thumbnailScreenWidth and thumbnailScreenHeight to this

    calculateThumbnailImageSize(width, height) {
        if (!width || !height) {
            return {};
        }
        // Width of the thumbnail works better as a constant than it does
        // a percentage of the screen width since it is relative to each screen
        // Note: Clamp minimum width 40px to support touch device
        let thumbnailScreenWidth = lodashClamp(width, 40, 250);
        // alert(thumbnailScreenWidth);
        let imageHeight = height / (width / thumbnailScreenWidth);
        let thumbnailScreenHeight = lodashClamp(imageHeight, 40, this.props.windowHeight * 0.40);
        let aspectRatio = height / width;

        // If thumbnail height is greater than its width, then the image is portrait otherwise landscape.
        // For portrait images, we need to adjust the width of the image to keep the aspect ratio and vice-versa.
        
        
        if (thumbnailScreenHeight > thumbnailScreenWidth) {
        //Checking if the aspect ratio of image does not cause the value of thumbnailScreenWidth to fall below the minimum width.
            if(Math.round(thumbnailScreenHeight * (1 / aspectRatio))<80){
                thumbnailScreenWidth = 80;
                thumbnailScreenHeight = 80*aspectRatio;
                // Capping the maximum height of the thumbnail 
                if (thumbnailScreenHeight>500){
                    thumbnailScreenHeight = 500;
                }
            }
            else{
            thumbnailScreenWidth = Math.round(thumbnailScreenHeight * (1 / aspectRatio));
           }
        } else {
            
            thumbnailScreenHeight = Math.round(thumbnailScreenWidth * aspectRatio);
        }
        return {thumbnailWidth: thumbnailScreenWidth, thumbnailHeight: Math.max(40, thumbnailScreenHeight)};
    }

and then we can change resize mode from contain to cover here

App/src/components/ImageWithSizeCalculation.js

Line 85 in 4fefc48

resizeMode={Image.resizeMode.contain}

 resizeMode={Image.resizeMode.cover}

Solution B

We can cap the max height here

App/src/components/ThumbnailImage.js

Line 90 in 6f6cf65

<View style={[this.props.style, styles.overflowHidden]}>

by adding a new style maxHeight like this

 <View style={[this.props.style, styles.overflowHidden , {maxHeight:400}]}> //we can add a new style in stylesheet for this 

and since the aspect ratio of long images causes the width of the thumbnail to fall below the width of spinner, we can simply check if thumbnailScreenWidth is less than the desired min with (or width of spinner) and if so we can change it in the function
calculateThumbnailImageSize to our desired min with

and then to make the image fill the entire area of the thumbnail and be centered, we can simply set image resize mode to cover. To do this we can change this line

App/src/components/ImageWithSizeCalculation.js

Line 85 in 6f6cf65

resizeMode={Image.resizeMode.contain}

to this

 resizeMode={Image.resizeMode.cover}

Result

Both solutions will give us same result

I think we should also check and set cap on min-height and Max width of the thumbnail when it is in landscape mode

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0105084e7856349f53

[MelvinBot]

Current assignee @NicMendonca is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

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

[hellohublot]

Proposal

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

Loading spinner is cut off on long images when reopening the chat

What is the root cause of that problem?

The image does not maintain a minimum zoom ratio

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

Solution A

• We can simply add thumbnailWidth: Math.max(40, thumbnailScreenWidth) in ThumbnailImage.calculateThumbnailImageSize.return, just like thumbnailHeight: Math.max(40, thumbnailScreenHeight)
• We can modify resizeMode to cover

Solution B

• I also suggest that we do refactor, just like ImageView.native.configureImageZoom

-    let thumbnailScreenWidth = lodashClamp(width, 40, 250);
-    const imageHeight = height / (width / thumbnailScreenWidth);
-    let thumbnailScreenHeight = lodashClamp(imageHeight, 40, this.props.windowHeight * 0.40);
-    const aspectRatio = height / width;
-
-    // If thumbnail height is greater than its width, then the image is portrait otherwise landscape.
-    // For portrait images, we need to adjust the width of the image to keep the aspect ratio and vice-versa.
-    if (thumbnailScreenHeight > thumbnailScreenWidth) {
-        thumbnailScreenWidth = Math.round(thumbnailScreenHeight * (1 / aspectRatio));
-    } else {
-        thumbnailScreenHeight = Math.round(thumbnailScreenWidth * aspectRatio);
-    }
-    return {thumbnailWidth: thumbnailScreenWidth, thumbnailHeight: Math.max(40, thumbnailScreenHeight)};
+   let zoomScale = Math.min(250 / width, this.props.windowHeight * 0.4 / height)
+   let thumbnailWidth = Math.max(40, width * zoomScale)
+   let thumbnailHeight = Math.max(40, height * zoomScale)
+   return {thumbnailWidth, thumbnailHeight)};

• We can modify resizeMode to cover

[Harshdeepjoshi]

Proposal

Updated proposal based on slack conversation

[wildan-m]

Proposal

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

The loading spinner is cut off on long images when reopening the chat

What is the root cause of that problem?

We are expecting all image content to be shown in the thumbnail while also being required to keep the aspect ratio.

App/src/components/ImageWithSizeCalculation.js

Line 85 in a786349

resizeMode={Image.resizeMode.contain}

This expectation will make the thumbnail image too narrow when the height is too long.

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

According to the slack discussion. We are ok to hide some image content and show the middle part of the image if the dimension is extremely long.

I'd propose removing this code part, since we are not required to keep the aspect ratio for our thumbnail:

App/src/components/ThumbnailImage.js

Lines 66 to 74 in a786349

	const aspectRatio = height / width;
	// If thumbnail height is greater than its width, then the image is portrait otherwise landscape.
	// For portrait images, we need to adjust the width of the image to keep the aspect ratio and vice-versa.
	if (thumbnailScreenHeight > thumbnailScreenWidth) {
	thumbnailScreenWidth = Math.round(thumbnailScreenHeight * (1 / aspectRatio));
	} else {
	thumbnailScreenHeight = Math.round(thumbnailScreenWidth * aspectRatio);
	}

And also extract resizeMode inside ImageWithSizeCalculation and use cover value for ThumbnailImage.

Before:

App/src/components/ThumbnailImage.js

Lines 98 to 102 in a786349

	<ImageWithSizeCalculation
	url={this.props.previewSourceURL}
	onMeasure={this.updateImageSize}
	isAuthTokenRequired={this.props.isAuthTokenRequired}
	/>

After:

                    <ImageWithSizeCalculation
                        url={this.props.previewSourceURL}
                        onMeasure={this.updateImageSize}
                        isAuthTokenRequired={this.props.isAuthTokenRequired}
                        resizeMode={Image.resizeMode.cover}
                    />

What alternative solutions did you explore? (Optional)

N/A

Result

[wildan-m]

@parasharrajat since you are the author of the code part I mentioned. With the change to the expected behavior, do you think my proposal approach is correct? or it will cause regression?

Here is how the thumbnail will look like using a sample image from your previously fixed issue.

[Beamanator]

Not overdue, proposals yet to be reviewed 👍

[davidcruzcs]

Proposal

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

Loading spinner is cut off on long images when reopening the chat.

What is the root cause of that problem?

When images are to slim after resizing them to fit in the ThumbnailImage component. The Loading Spinner no longer has enough room to display correctly.

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

As per the Slack bug thread feedback, I'm proposing a new solution where we use a maxHeight and minWidth on the ThumbnailImage. This values are defaultProps of the component, so they can be dynamically changed if needed.

/** Min Width of the thumbnail image */
minImageWidth: PropTypes.number,

/** Max Height of the thumbnail image */
maxImageHeight: PropTypes.number,

I propose to replace the contain resizeMode with the cover mode in ImageWithSizeCalculation. This guarantees that images that properly displayed before work exactly the same, But also that images with extreme h-w ratios, are now gracefully displayed to have a size that allows the spinner to show properly, and the image to be recognizable.

resizeMode={Image.resizeMode.cover}

As part of the proposal, two new StyleUtils are added: function getMaximumHeight(maxHeight) and function getMinimumWidth(minWidth). The codebase already had the opposite methods of the two above, so it is also a nice addition to complete all the minMax possibilities.

function getMaximumHeight(maxHeight)

function getMinimumWidth(minWidth)

What alternative solutions did you explore? (Optional)

I explored working with the resize of the image within calculateThumbnailImageSize. But as the Slack thread commented, it overcomplicates things, whilst accomplishing a very similar outcome.

Result

(minWidth set to 50 to minimize the footprint of the change, while still allowing the spinner to show properly and with a nice small margin.)

Screen.Recording.2023-05-01.at.10.25.53.AM.mov

[MelvinBot]

📣 @davidcruzcs! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[davidcruzcs]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~0149e06885366ff847

[MelvinBot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[sobitneupane]

Thanks for proposal everyone.

Solution A from @hellohublot's proposal looks good to me. I do not think maxHeight will be required. But if required we can clamp thumbnailScreenHeight in return statement of calculateThumbnailImageSize.

🎀👀🎀 C+ reviewed

cc: @Beamanator

[melvin-bot]

@Beamanator @hellohublot @NicMendonca @sobitneupane 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!

[NicMendonca]

Not overdue: #18485 (comment)

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.13-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix ThumbnailImage minWidth to display spinner #18485

If no regressions arise, payment will be issued on 2023-05-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
• [@sobitneupane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@sobitneupane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@sobitneupane] Determine if we should create a regression test for this bug.
• [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@slafortune ] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:https://github.com/Expensify/Expensify/issues/286120

[NicMendonca]

@Harshdeepjoshi @sobitneupane @hellohublot please apply to the job so we can issue payment: https://www.upwork.com/jobs/~0105084e7856349f53

[NicMendonca]

Going OOO until June 5th so assigning a buddy to this GH to issue payment once everyone has applied ^^

[melvin-bot]

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

[sobitneupane]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#8238

• [@sobitneupane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

#8238 (comment)

• [@sobitneupane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

https://expensify.slack.com/archives/C049HHMV9SM/p1684922451095849

[sobitneupane]

Regression Test Proposal

1. Go to any chat
2. Send a picture with very large height and comparatively small width.
3. Verify the loading spinner can display well and is not cut off.

Do we agree 👍 or 👎

[slafortune]

@Harshdeepjoshi @sobitneupane @hellohublot - offers sent 👍

[melvin-bot]

@Beamanator, @slafortune, @hellohublot, @sobitneupane Huh... This is 4 days overdue. Who can take care of this?

[slafortune]

@Harshdeepjoshi looks like two offers got sent and accepted - I paid one and close the other 👍