ausi / respimagelint

Linter for Responsive Images - https://ausi.github.io/respimagelint/
MIT License
763 stars 29 forks source link

Provide recommended widths for 'srcset' images #62

Closed danielbachhuber closed 2 years ago

danielbachhuber commented 2 years ago

Thanks for the awesome bookmarket! It's very good 😊

Most of my work is with WordPress, which offers some standard but generally incorrect sizes attributes out of the box, so this bookmarklet is very helpful for figuring out what the expected sizes attribute should be.

WordPress also generates a variety of image sizes out of the box, but they may or may not be sufficient for srcset. An awesome enhancement to this bookmarklet would be to provide recommended widths for srcset images.

This is probably the preferred approach: https://github.com/GoogleChrome/lighthouse/issues/11593#issuecomment-716598218

ausi commented 2 years ago

Do you know a best practise threshold for these kind of calculations? Something like 250.000 square-pixels?
Example:

  1. 100x100
  2. 510x510
  3. 715x715
  4. 870x870
  5. 1004x1004
  6. 1122x1122

Or maybe something larger like 500.000 square-pixels?

danielbachhuber commented 2 years ago

Do you know a best practise threshold for these kind of calculations? Something like 250.000 square-pixels?

I don't — trying to figure that out now 😊

Cloudinary has a tool for it but, annoyingly, they put their algorithm on the server instead of client-side. I suppose someone could reverse-engineer it if they thought about it long enough.

I created an issue in Google Lighthouse too, so maybe they'll have some opinion/perspective: https://github.com/GoogleChrome/lighthouse/issues/13563

danielbachhuber commented 2 years ago

Here are some more links on the topic:

danielbachhuber commented 2 years ago

Couple of questions I'm uncertain about:

danielbachhuber commented 2 years ago

In re-reading the thread on https://github.com/GoogleChrome/lighthouse/issues/11593#issuecomment-716598218, it seems like the best solution would be to:

A function like generateSrcSetWidths( origImageWidth, origImageHeight, sizesAttribute, breakpoints = 6, upperBound = 1920 ) would return an array of six image widths.

ausi commented 2 years ago

Added a recommentation in 35df131c64a7b21fe83c46a92a1ac9a478e09f4b

Looks something like this now:

Bildschirmfoto 2022-01-17 um 17 29 14
danielbachhuber commented 2 years ago

It looks like WordPress uses 2048 as the upper bound for an image used in srcset: https://github.com/WordPress/wordpress-develop/blob/a38ecee8c9554dfd3b6152fc0b1950cd69838fc4/src/wp-includes/media.php#L1206

I had previously mentioned 1920 as a default upper bound but it probably makes sense to simply match WordPress.

The default recommended widths probably should be between 150 and 2048. What do you think?

ausi commented 2 years ago

I set the upper bound to 2048 and the lower bound to 256 in 63170421f19f1dba15bf5184592ce324799a3627 This should be sensible for most projects I think.

danielbachhuber commented 2 years ago

Another thing I noticed: when an image doesn't have sizes or srcset, only the srcset is recommended.

image

Should sizes be recommended too?

Additionally, that particular image is hidden for 768px and higher (md:hidden). Is that a known issue?

danielbachhuber commented 2 years ago

Not sure if it's the same problem but here's another buggy recommendation:

image

Maybe the lower bound should be based on sizes, instead of being fixed?

ausi commented 2 years ago

The lower bound is now more “flexible” meaning the recommended size can get lower if the max size is lower or if it detected a fixed (non-fluid) size that is smaller. @danielbachhuber Can you please retry your avatar example?

danielbachhuber commented 2 years ago

Great work! The results for the avatar example seem more sensible now:

image

When I add 120w and 240w images to the srcset, the check passes:

image

The live URL is https://www.foodbloggerpro.com/ if you want to try it out.

Here's another edge case I found that we could potentially track as a separate issue:

image

I'm not sure the 268w recommendation makes sense for that image, given it won't display until 768px width (768 * .465 = 357.12). Any ideas what's going on there?

Also, what do you think about rounding the suggestions to the closest 10 (e.g. 268 -> 270)? I feel like it might be a little more human-friendly.

Lastly, for the sake of the next person to come across this thread, could you share a high-level summary of how the code...

  1. Identifies all of the potential image widths?
  2. Chooses which image widths to offer as suggestions?
danielbachhuber commented 2 years ago

Found another one 🙃

On our membership page, Lighthouse is flagging an image that the bookmarklet identifies as passed:

image image

The image's markup is:

<img
    alt="a collage of two girls shooting a recipe video, an analytics icon, and a course tracker icon" data-pin-nopin="nopin"
    src="https://www.foodbloggerpro.com/wp-content/assets/images/membership/introducing-collage-520x492.png"
    srcset="https://www.foodbloggerpro.com/wp-content/assets/images/membership/introducing-collage-520x492.png 520w, https://www.foodbloggerpro.com/wp-content/assets/images/membership/introducing-collage-1040x982x.png 1040w, https://www.foodbloggerpro.com/wp-content/assets/images/membership/introducing-collage-1560x1474.png 1560w"
    sizes="(min-width: 1180px) 520px, (min-width: 780px) calc(48.95vw - 48px), (min-width: 600px) 520px, 90vw"
    loading="lazy"
    width="520"
    height="492">

There's also an odd recommendation with this one:

image

400w and 800w already exist in the srcset, and the recommendation is for an image width that already exists.

ausi commented 2 years ago

Any ideas what's going on there?

The first <source> in your example misses the media attribute and therefore gets always selected by the browser. The media queries in the sizes attribute are only used for the size part, not for selecting the right <source>

The other errors are due to the fact that the check-algorithm doesn’t use the same threshold (in megapixels) as the algorithm that calculates the recommendation. I have to update the check-algorithm and then these errors should go away.

danielbachhuber commented 2 years ago

The first <source> in your example misses the media attribute and therefore gets always selected by the browser. The media queries in the sizes attribute are only used for the size part, not for selecting the right <source>

Ah, good catch. It doesn't seem like <picture> is even necessary there, so I've changed it to <img>

danielbachhuber commented 2 years ago

To recap some of our conversation on Twitter (which I think will be helpful for historical context):


D: Still getting marked as passed


image

D: Does it show up as passed for you? foodbloggerpro.com/membership/

M: the current setting of the linter is set to allow a distance of 0.75 megapixels that seems to be too large for google...

D: Here’s the source code for the Google check https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/audits/byte-efficiency/uses-responsive-images.js D: The threshold is 4096 bytes? https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/audits/byte-efficiency/uses-responsive-images.js#L34 D: I believe it’s solely analyzing at the Moto G4 device width too.

M: yes, it calculates the diffrence in bytes which we cannot do :/

D: I think this is where we could be running into https://github.com/GoogleChrome/lighthouse/issues/11593 and ultimately the suggestion of https://github.com/GoogleChrome/lighthouse/issues/11593#issuecomment-716598218

M: I’d have to lower the threshold to 0.35MP for the image #2 to fail, so I think google is too strict there indeed

D: A couple of options I see:

  1. Modify the bookmarklet’s algorithm to ensure Google Lighthouse’s current inspection criteria are always met (e.g. make sure the set of image widths always include those necessary for Moto G4).
  2. Develop an opinion on what the optimal algorithm should be, apply that algorithm to the bookmarklet, and work to get the change applied in Google Lighthouse.

If you didn’t feel like 1 was too much of a hack, we could always start with 1 and then do 2.

M: hard coding image sizes to a specific device would not be a long term solution as I would guess that google updates such things regularly... M: an optimal algorithm depends strongly on the project the images the format and many many more factors. M: I think the current version which allows a distance of 0.75 megapixels is a pretty good tradeoff for most use-cases If I run your example image through http://tinypng.com it gets way way smaller, maybe that is enought to make google happy?

D: Well, I think that Google would change it to an algorithm that isn’t restricted to device size, and when it’s applied it will be noted on that issue. The current restriction hasn’t change for two years.

image

D: Ultimately, the challenge is that most people using the bookmarklet will want the bookmarklet to solve for the warning in Google Lighthouse. It may be a scenario where the “optimal for the real world” solution is different than the “technically optimal” solution. D: Another option, if it wasn’t too complex: the bookmarklet produces two sets of recommendations, the first based on the optimal algorithm, and the second based on what will solve the Google Lighthouse error. The user can choose which they apply. When Google Lighthouse updates, the second set of recommendations can be removed.

M: I agree that it should make google lighthouse happy for most cases. But in your case I think the problem is that the PNG images are just too large. If you would use WEBP or an optimized PNG instead the issue might be solved I think.

D: Oh, good point. We use Cloudflare to dynamically serve PNG as WebP, but it only kicks in after the image is in cache (i.e. not very reliably).

M: lighthouse also suggested using webp for your website, but I cannot reproduce that now. the results are always slightly different... :/

D: Yeah, once Cloudflare kicks in, the WebP suggestion goes away.


The specific example is now fixed!

For this particular issue, I think it'd still be helpful to:

Lastly, for the sake of the next person to come across this thread, could you share a high-level summary of how the code...

  1. Identifies all of the potential image widths?
  2. Chooses which image widths to offer as suggestions?

The srcset suggestions seem to be working well now though.

ausi commented 2 years ago

Lastly, for the sake of the next person to come across this thread, could you share a high-level summary of how the code...

  1. Identifies all of the potential image widths?
  2. Chooses which image widths to offer as suggestions?

Sure!

When the linter runs, it resizes the viewport (browser window) to many different dimensions and checks how large each image is for each viewport.

The calculateSuggestedDimenions algorithm then takes this data and searches for image widths that appear often. These sizes are most likely statically sized images (non-fluid) and their exact widths are added to the recommendation list as @​1x and @​2x versions.

Next, it adds the lowest and largest measured size of the image to the recommendation list too.

From this list it then removes sizes that are less than 0.2 megapixels apart. Gaps in that list of sizes that are larger than 0.75 megapixels get divided into equal parts of less than 0.75 megapixels.

At the end of all of this we get a list of image widths where every gap between them is from 0.2 to 0.75 megapixel large. And for the non-fluid sized parts of an image we have an exact match for @​1x and @​2x screen resolutions.

danielbachhuber commented 2 years ago

💯 Nice work on this!