[$500] Chat - Loading wheel for large image is not completely visible

[lanitochka17]

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

---

Version Number: 1.4.55-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4441489 Email or phone of affected tester (no customers): vdargentotest+ios032024@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

Pre-requisite: the user must be logged in

1. Go to any chat
2. Upload the attached large image
3. When the image finally uploads, verify that when it loads, the loading wheel is not completely visible

Expected Result:

The loading wheel should be fully visible

Actual Result:

The loading wheel is almost completely hidden

Workaround:

Unknown

Platforms:

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

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [x] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/4969a3bc-2716-4eaa-bb05-17019585ff4f

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010fa06a3d83b43db6
• Upwork Job ID: 1770603734538096640
• Last Price Increase: 2024-03-28
• Automatic offers:
• dukenv0307 | Reviewer | 0
• tienifr | Contributor | 0

[melvin-bot[bot]]

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

[lanitochka17]

@johncschuster FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[abzokhattab]

sounds like an edge case, no?

[johncschuster]

Yeah, this feels like a pretty extreme case to me. I think we can probably close this. (Discussing here)

[johncschuster]

Welp! After discussing, we feel it's worth fixing. I've added it to #vip-vsb

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~010fa06a3d83b43db6

[melvin-bot[bot]]

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

[Krishna2323]

Proposal

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

Chat - Loading wheel for large image is not completely visible

What is the root cause of that problem?

We don't have min width on the View containing ImageWithSizeCalculation. https://github.com/Expensify/App/blob/1a8b66a25f3c43ee0557c02cc65f7119148f50af/src/components/ThumbnailImage.tsx#L99-L100

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

Add minWidth & minHeight so there is enough width for the loading indicator. The values can be decided by the design team.

Also need to add here: https://github.com/Expensify/App/blob/1a8b66a25f3c43ee0557c02cc65f7119148f50af/src/components/ThumbnailImage.tsx#L84-L85

[!NOTE]
I have tested the my solution and it does work perfectly without any flickering and on all device sizes, we just need to update the minWidth & minHeight size on small width devices.

Result

Slack:

https://github.com/Expensify/App/assets/85894871/a1b341a8-a8c6-4198-a56c-7339cc8ff976

Fix:

https://github.com/Expensify/App/assets/85894871/949fa895-8f99-484b-a224-af3d45912480

Alternatively

We can pass ...sizeStyles to ImageWithSizeCalculation using style prop and inside ImageWithSizeCalculation we will set the minWidth until isLoading && !isImageCached is true.

[Krishna2323]

Proposal updated

[tienifr]

Proposal

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

The loading wheel is almost completely hidden

What is the root cause of that problem?

In here, we have the logic to set the image size, if the height is larger than width like in this case, the height will be fixed, and the width will be calculated based on the aspectRatio of the image.

The problem here is this image is much larger in height than in width, causing the width to become very small it's almost invisible. The loading indicator will also not be visible.

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

In the image size calculation logic, we need to cap the minimum width and minimum height of the image as well, by capping the aspectRatio, so the if the image width according to aspectRatio is less than minimum, it will take the minimum size due to the capped aspectRatio

To do this, we can update here

const aspectRatio = (height && Math.max(width / height, 0.3)) || 1;

(Do the same for height as well, it can be as easy as lodashClamp(width / height, 0.3, 1 / 0.3))

This will make sure that the image width displayed will be at least 30% of image height, and vice versa (we can adjust the percentage as we see fit). Do note that the fixed dimension of the image is different for small screen and large screen here, so a fixed minWidth/minHeight will not work since it produces different & inconsistent "capped aspect ratio" on different screen sizes. Meanwhile, the aspectRatio will cap by percentage, ensuring that the capped ratio will be the same regardless of screens.

This will make sure the image looks well not only in loading but also after loaded. Otherwise it will cause a flicker in size after the image is loaded, and also without this, the image after loaded is barely visible.

This is the same UX that Slack has, try sending the long image to Slack and you'll see the same (the minimum height is capped both before and after the image is loaded)

What alternative solutions did you explore? (Optional)

The issue mentioned here is a different bug and out of the issue's scope. We should fix it separately (probably by still rendering the image container that has the minimum width, height, if the image's actual width & height are small than that containers's we should show the image at its intrinsic width, height, but contained in the center of the outer container). This is an edge case also, I don't think regular users would send such small image, so I'm not sure it's worth fixing.

[Krishna2323]

Proposal Updated

Added slack UX and fix result.

[tienifr]

Proposal updated to add example code change

[Krishna2323]

Proposal Updated.

Added a note and we also need to set the maxHeight.

[dukenv0307]

Thanks for all your proposals

@Krishna2323 Your solution can fix the issue but I don't like using fixed min-width or min-height it can cause the inconsistency aspect-ratio on large device and small device, let's see the image below when I set min-width: 200px, they look so different

@jsdev2547 Your solution cause the flicker between loading state and loaded state

@tienifr's solution seems fine to me. We should adjust the minimum to lower than 0.3, maybe 0.15 is good.

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[Krishna2323]

@dukenv0307, I haven't suggested to use 200px :), just like slack we will use something like 52 or 72. Just to make enough space for loader. I tested my solution on small width devices and it works fine.

Also I think the selected proposal won't work with wide images with short height.

[dukenv0307]

I haven't suggested to use 200px :)

I mean your solution can cause the inconsistency, I want to use 200px to make it clearer. The reason behind that is here:

https://github.com/Expensify/App/blob/0aa0ffa8301d4d8bdb43f4a6985098b437428f47/src/hooks/useThumbnailDimensions.ts#L17

fixedDimension depends on screen size

Also I think the selected proposal won't work with wide images with short height.

Can you give me an example?

[tienifr]

Also I think the selected proposal won't work with wide images with short height.

@Krishna2323 It will work just fine. Perhaps you didn't see the below part of my proposal.

(Do the same for height as well, it can be as easy as lodashClamp(width / height, 0.3, 1 / 0.3))

[Krishna2323]

@dukenv0307 @tienifr I tried but it didn't worked, pls try with the image below.

[tienifr]

@Krishna2323 Works fine for me in the sample image in OP (rotated) and also the image you shared

@dukenv0307 can retest and confirm

[tienifr]

Please check for images with lesser width than the width of the loader. The aspect ratio would stretch the image should the minimum be lesser than the width of the loader. The image rendered is blurred far from the actual image.

@jsdev2547 This is a different bug, you can see it's already happening even without my change. Try sending a few pixels width x few pixels height image, (without my change) you'll see it's blurred like below

cc @dukenv0307

[dukenv0307]

Please provide some video or screenshot so I can understand the issue.

@jsdev2547 Pls check 2 images below. The first one is loading state and the second one is loaded state. The width changes after the image is loaded -> flicker

[dukenv0307]

Please check for images with lesser width than the width of the loader. The aspect ratio would stretch the image should the minimum be lesser than the width of the loader. The image rendered is blurred far from the actual image. I tested using,

Great find, I also can reproduce this bug even on main so I don't think we should fix this issue here. Wdyt @youssef-lr

[Krishna2323]

@dukenv0307, what do you think about just setting min width on the container only.

https://github.com/Expensify/App/assets/85894871/36983b2e-d100-403a-b3d1-e659482e97ad

Slack also does the same:

[dukenv0307]

Slack also does the same:

@Krishna2323 You sure? I see the different image you uploaded to Slack right?

[dukenv0307]

If it's the same, it should be displayed full 64px on Slack as the video you attached here

[dukenv0307]

@jsdev2547 I tested your case on both Slack and EPSF. It's blurred on EPSF with the so small image, but looks good on Slack

Slack:

EPSF:

But if you tried with the large attachment in the OP, they are quite similar. That why I think it's the different bug

[tienifr]

@dukenv0307 I think we should focus on this issue's scope. As far as I can see there was no issue/downside with my solution, I think we can move forward here and minimize unnecessary discussions.

[Krishna2323]

@dukenv0307, can you pls try with this image on Slack


[tienifr]

@dukenv0307 I am not getting blurred image on main branch though, the image might be thin but not blurred. The blur is caused by using,

@jsdev2547 As I already explained here, this is existing bug on main. Try uploading the following small image, the same blurring issue will happen. So it's not due to my change, it's a separate issue.

[dukenv0307]

what do you think about just setting min width on the container only.

I tried this solution with the image above, it didn't work like Slack

As I mentioned here. I still think it's other bug. Wdyt @youssef-lr ?

[dukenv0307]

@youssef-lr Can you help check my decision? About the bug here, I still think it's not related to this issue, since it still happens even on main.

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[johncschuster]

@youssef-lr, can you take a look at @dukenv0307's recent comment above?

[johncschuster]

Bump!

[melvin-bot[bot]]

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[youssef-lr]

I agree with your decision @dukenv0307

[melvin-bot[bot]]

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

[youssef-lr]

Sorry @johncschuster I was OOO sick, I'm still recovering.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @johncschuster, @youssef-lr, @tienifr, @dukenv0307 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot[bot]]

@johncschuster, @youssef-lr, @tienifr, @dukenv0307, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

[dukenv0307]

I got the delay from @youssef-lr. The PR is still in review, can you please open the issue @johncschuster

[youssef-lr]

Replied in the thread @dukenv0307, I'm still facing issues locally that deal with attachments, hoping we can resolve them today

[tienifr]

We want to close the PR based on this discussion.

[tienifr]

@johncschuster Can you please process payment for me (C) and @dukenv0307 (C+)

[johncschuster]

Sure thing, @tienifr!

I reviewed the issue with @youssef-lr, and it looks like the PR was closed and the fix was not implemented (discussion here). However, we feel you both deserve payment for putting in the work. I'm going to issue 50% of the payment for the work you've done here.

[johncschuster]

Payment Summary:

Contributor: @tienifr paid $250 via Upwork - PAID! Contributor+: @dukenv0307 owed ~$250~ $500 via Upwork - PAID!

[dukenv0307]

@johncschuster According to the thread, we're closing this because of changing priority (not prioritizing the bug any more), while all the work from contributors were already done.

From this thread to discuss SO on this type of case, the job should be paid out in full.

We should pay in full because we hired them for the job and they did the work they were supposed to.

What do you think?