Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add pjpg format only if image type of not png. #2400

Merged
merged 6 commits into from
May 14, 2020

Conversation

revanth0212
Copy link
Contributor

@revanth0212 revanth0212 commented May 13, 2020

Description

This PR adds logic in makeUrl.js file in venia-ui to handle png image requests.

Currently, the makeOptimizedUrl function does not consider the file type while adding URLSearchParams. It adds webp and pjpg in that respective order. This means if the server can not handle webp it will consider pjpg. webp is compatible with png, so when the user requests a png it will be returned with a transparent background.

image

If the server does not support webp since the current code does not regard png and applies pjpg as the format, instead of a transparent background, we will get the image with a black background.

image

But with the current change, instead of pjpg the UI will request a png instead.

image

All this happens only if the backend does not support webp and is served by a backend that does not handle pjpg properly. For instance, we were not able to reproduce this issue till we switched off fastly, because fastly handles pjpg well and does not return a black background. When we switched off fastly, the backend image optimization was handled by hastily which was retuning the black background.

I am not sure if the approach taken by fastly or hastily is correct but one thing is for sure, we have to send format as png for a png image request irrespective of the backend.

Troubleshooting Tips:

  1. Check if the backend service supports webp. If it does, everything should be fine.
  2. If it does not, check the file format requested in the URLSearchParams.

Related Issue

Closes #2340

Verification Stakeholders

@zetlen
@jimbo
@Jordaneisenburger

Verification Steps

To test use this, use this backend URL:
MAGENTO_BACKEND_URL=https://staging-5em2ouy-mfwmkrjfqvbjk.us-4.magentosite.cloud/

  1. Go to https://magento-venia-concept-7bnnn.local.pwadev:8914/simple13.html
  2. With fastly enabled/disabled everything should work fine. You should see a transparent background when you open the image in the network tab.

Screenshots / Screen Captures (if appropriate)

Checklist

None

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented May 13, 2020

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against f7c296c

@revanth0212 revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label May 13, 2020

const imageFileType = getFileType(baseURL);
if (imageFileType === 'png') {
params.set('format', 'png'); // use png if webp is not available
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Piggybacking on the comment @Jordaneisenburger made on the issue, what should we do to make this extensible?

Should we do something like this?

if (isValidImageFileType(imageFileType)) {
    params.set('format', imageFileType);
} else {
    params.set('format', 'pjpg');
}

@revanth0212 revanth0212 marked this pull request as ready for review May 13, 2020 21:33
Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the tests as well. I think this is the fix we're looking for. At the moment, I'm not sure how to open up these build processes to customization, but we'll have to spike on it.

@devops-pwa-codebuild
Copy link
Collaborator

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2400.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.66

@dpatil-magento dpatil-magento merged commit 96613a6 into develop May 14, 2020
@dpatil-magento dpatil-magento deleted the revanth/png-image-handling branch May 14, 2020 22:03
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pkg:venia-ui version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: transparent png's get a black background
5 participants