App icon indicating copy to clipboard operation
App copied to clipboard

[$125] Web - Chat - PNG attachment's background colour turns to white

Open IuliiaHerets opened this issue 1 year ago • 6 comments

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


Version Number: v9.0.73-7 Reproducible in staging?: Yes Reproducible in production?: Yes Issue reported by: Applause Internal Team

Action Performed:

  1. Open https://staging.new.expensify.com
  2. Click on the plus icon in the compose box of any chat.
  3. Select a PNG image without a background and send it to the chat.

Expected Result:

PNG image file thumbnail should be shown as it is without added background colour.

Actual Result:

The image's background turns to white after some seconds.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6679038_1733763117600!koliandros (1) Bug6679038_1732796033852!Screenshot_2024-11-28_141042

https://github.com/user-attachments/assets/5ba1a1c7-afe6-4db7-8b24-b4556c512e0e

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021866612835147904206
  • Upwork Job ID: 1866612835147904206
  • Last Price Increase: 2024-12-10
Issue OwnerCurrent Issue Owner: @hungvu193

IuliiaHerets avatar Dec 10 '24 18:12 IuliiaHerets

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Dec 10 '24 18:12 melvin-bot[bot]

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

melvin-bot[bot] avatar Dec 10 '24 22:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 10 '24 22:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 10 '24 22:12 melvin-bot[bot]

Was able to reproduce. Unsure if this is a recent regression, was going to post in #expensify-open-source to check but it might not matter if we want to fix it either way. Set starting price at $125 cuz I'm hoping it's a quick fix.

mallenexpensify avatar Dec 10 '24 22:12 mallenexpensify

Edited by proposal-police: This proposal was edited at 2024-12-11 02:05:38 UTC.

Proposal

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

The image's background turns to white after some seconds.

What is the root cause of that problem?

The AddAttachment API returns an image URL with the .1024.jpg extension. And we render image here, as a result, the app renders the image as a JPEG instead of PNG, causing the transparency in the background to be lost "html": "<img src=\"https:\/\/www.expensify.com\/chat-attachments\/8887844905473165339\/w_fd9ef52869d8f5923663098fbd26f7c08f8e3fa1.png.1024.jpg\" data-expensify-source=\"https:\/\/www.expensify.com\/chat-attachments\/8887844905473165339\/w_fd9ef52869d8f5923663098fbd26f7c08f8e3fa1.png\" data-name=\"images__1_-removebg-preview.png\" data-expensify-height=\"221\" data-expensify-width=\"228\" \/>"

AddAttachment API Screenshot 2024-12-11 at 7 22 30 AM

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

We can replace .1024.jpg in the FE by using previewSource.replace(/\.png\.(1024|320)\.jpg$/, '.png'). That code only replaces if the previewSource format is .png.1024.jpg or .png.320.jpg.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

What alternative solutions did you explore? (Optional)

N/A

linhvovan29546 avatar Dec 11 '24 00:12 linhvovan29546

Updated my proposal https://github.com/Expensify/App/issues/53881#issuecomment-2533336193

linhvovan29546 avatar Dec 11 '24 02:12 linhvovan29546

Edited by proposal-police: This proposal was edited at 2024-12-11 07:04:55 UTC.

Proposal

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

PNG attachment's background colour turns to white

What is the root cause of that problem?

The RCA for this ticket is that previewSource is using htmlAttribs.src, which is the HTML image link

https://github.com/Expensify/App/blob/6207c96523a447817bcb84681603c3b2fa74c14a/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx#L60

example link: https://dev.new.expensify.com:8082/chat-attachments/4704811877564310284/w_c651b5ce012983297284c509ccdd4d10dea5e567.png.1024.jpg

  • Screenshot 2024-12-11 at 13 47 28

This URL, which includes 1024.jpg, still loads the image with a white background, as shown above, because JPG images do not support transparency.

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

Because previewSource is using htmlAttribs.src to get the previewSource link and then appending the 1024.jpg suffix, I think we should use htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE] instead of htmlAttribs.src, if htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE] is available. This is similar to the logic used to get the source. previewSource and source should use the same logic to get the URL consistently, like this:

https://github.com/Expensify/App/blob/f8f12c2dcb6d3b62663fc4edea50cd543e44e74d/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx#L60

Change to

    const previewSource = tryResolveUrlFromApiRoot(isAttachmentOrReceipt ? attachmentSourceAttribute : htmlAttribs.src);
POC

https://github.com/user-attachments/assets/0b0a1b71-df5d-443d-958e-e4dbb9fc6253

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

huult avatar Dec 11 '24 06:12 huult

@hungvu193 👀 on @huult 's proposal plz. Thx

mallenexpensify avatar Dec 12 '24 21:12 mallenexpensify

Surre

hungvu193 avatar Dec 13 '24 00:12 hungvu193

@hungvu193, @mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Dec 16 '24 09:12 melvin-bot[bot]

I'll review today!

hungvu193 avatar Dec 16 '24 09:12 hungvu193

I can reproduce this issue. Reviewing shortly!

hungvu193 avatar Dec 17 '24 11:12 hungvu193

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

melvin-bot[bot] avatar Dec 17 '24 16:12 melvin-bot[bot]

Still under reviewing

hungvu193 avatar Dec 17 '24 16:12 hungvu193

Thanks for the proposals, everyone.

Based on what we have here: https://github.com/Expensify/App/blob/b6ebedb611a549bb755aee934ca7f7707ce13798/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx#L39-L47

htmlAttribs.src refers to the thumbnail, so it's better to keep it as the preview image instead of using attachmentSourceAttribute (aka htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]).

Removing .1024.jpg from previewSource feels like a workaround to me. I think this issue should be handled on the BE, and it's best to ask the Internal Engineer about the .1024.jpg extension of the preview source.

🎀 👀 🎀

hungvu193 avatar Dec 18 '24 08:12 hungvu193

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

melvin-bot[bot] avatar Dec 18 '24 08:12 melvin-bot[bot]

@hungvu19 Can you review my proposal? My proposal does not require any backend changes or path removal. Thank you

huult avatar Dec 18 '24 09:12 huult

htmlAttribs.src refers to the thumbnail, so it's better to keep it as the preview image instead of using attachmentSourceAttribute (aka htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]).

Hey, I commented here, I don't think using the full-sized image for thumbnail image is a good idea.

hungvu193 avatar Dec 18 '24 09:12 hungvu193

@francoisl, @hungvu193, @mallenexpensify Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] avatar Dec 23 '24 09:12 melvin-bot[bot]

@francoisl @hungvu193 @mallenexpensify 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!

melvin-bot[bot] avatar Dec 24 '24 09:12 melvin-bot[bot]

Not overdue.

hungvu193 avatar Dec 26 '24 03:12 hungvu193

Upwork job price has been updated to $250

melvin-bot[bot] avatar Dec 30 '24 22:12 melvin-bot[bot]

Not overdue, @francoisl when you're back can you take a look at my comment above. Thanks

hungvu193 avatar Dec 31 '24 03:12 hungvu193

@francoisl, @hungvu193, @mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Jan 03 '25 09:01 melvin-bot[bot]

Not overdue

hungvu193 avatar Jan 06 '25 01:01 hungvu193

@francoisl @hungvu193 @mallenexpensify this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] avatar Jan 07 '25 09:01 melvin-bot[bot]

Anytime a receipt or chat attachment is uploaded, Expensify generates additional .1024.jpg and .320.jpg thumbnail versions of it too. I don't know why it's hardcoded to .jpg instead of using the original image's format, that code is over 14 years old, and people who made the original decision no longer work here.

It would be fairly simple to create a .1024.png thumbnail for PNG images and preserve the transparency, however we have a bunch of places in the code that expect .1024.jpg/.320.jpg thumbnails, and more importantly we'd risk breaking thumbnail links for all existing PNG images and receipts.

As much as I agree that removing .1024.jpg from the source image sounds like a workaround, it sounds like the safer option. We could also do a small adjustment and just remove .png.1024.jpg, so that we keep using the thumbnail for other image formats than png.

francoisl avatar Jan 07 '25 23:01 francoisl

Updated proposal to only remove .png.1024.jpg or .png.320.jpg

linhvovan29546 avatar Jan 07 '25 23:01 linhvovan29546

Cool. Let's go with @linhvovan29546 's proposal 🎀 👀 🎀

hungvu193 avatar Jan 08 '25 02:01 hungvu193