Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

misc: add usesPixelArtScaling flag to images #10481

Merged
merged 3 commits into from
Mar 19, 2020

Conversation

sk-
Copy link
Contributor

@sk- sk- commented Mar 19, 2020

Two new flags are added to ImageElements.

The first, usesPixelArtScaling will be used by the ImageSizeResponsive audit (#10460), as those images are intendedly displayed in a pixelated form, so the audit should not flag them.

Note that image-rendering: -moz-crisp-edges; is also an accepted value in Firefox, however in such case, the computed style will return crips-edges.

See https://developer.mozilla.org/en-US/docs/Games/Techniques/Crisp_pixel_art_look for more information.

The second is usesSrcSetDensityDescriptor. That flag will also be used by the ImageSizeResponsive and is required, as by the spec, the natural size of the image, is the actual size of the image divided by its density descriptor. Note that we cannot access the actual value of the density descriptor, so in the audit the best option will be to just skip those images.

An alternative would be to recompute the size of those images, but that may break some other assumptions about naturalWidth, naturalHeight.

This new flag will be used by the ImageSizeResponsive audit (GoogleChrome#10245), as those images are intendedly displayed in a pixelated form, so the audit should not flag them.

Note that `image-rendering: -mo-crisp-edges;` is also an accepted value in Firefox, however in such case, the computed style will return `crips-edges`.
@sk- sk- requested a review from a team as a code owner March 19, 2020 15:46
@sk- sk- changed the title feature: add usesPixelArtScaling flag to images misc: add usesPixelArtScaling flag to images Mar 19, 2020
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks @sk-! we could probably combine the two properties into the one PR since these are so small.

* @return {boolean}
*/
function usesPixelArtScaling(style) {
return ['pixelated', 'crisp-edges'].includes(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we might just want to do this inline, these get executed in browser which means we have to jump through some hoops like /* istanbul ignore next */, stringifying the function into global scope, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip. I had to inline it as otherwise the code would not run.

The only problem now is that there's duplicated code.

Apparently the code is executed in another process, which means, that functions mus be inlined, otherwise the code needs to go through some extra hoops.
@sk-
Copy link
Contributor Author

sk- commented Mar 19, 2020

@patrickhulce Thanks for the review, I added the other flag.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@patrickhulce patrickhulce merged commit 7904efb into GoogleChrome:master Mar 19, 2020
@patrickhulce
Copy link
Collaborator

@sk- not sure if your plan was to add this to the existing open PR or file another one once it is merged but some smoke tests for these would be great :)

@sk-
Copy link
Contributor Author

sk- commented Mar 19, 2020

@patrickhulce Thanks for merging.

Regarding smoke tests, I could do it in the open PR. Will update it accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants