8000 Provide recommended widths for 'srcset' images · Issue #62 · ausi/respimagelint · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Provide recommended widths for 'srcset' images #62

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.

Sign up for GitHub

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

Jump to bottom
Closed
danielbachhuber opened this issue Jan 12, 2022 · 18 comments
Closed

Provide recommended widths for 'srcset' images #62

danielbachhuber opened this issue Jan 12, 2022 · 18 comments
Assignees
@ausi
Labels
enhancement

Comments

@danielbachhuber
Copy link

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: GoogleChrome/lighthouse#11593 (comment)
@ausi
Copy link
Owner
ausi commented Jan 12, 2022

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 danielbachhuber mentioned this issue Jan 12, 2022
Open
@danielbachhuber
Copy link
Author

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: GoogleChrome/lighthouse#13563

@danielbachhuber
Copy link
Author

Here are some more links on the topic:

@danielbachhuber
Copy link
Author

Couple of questions I'm uncertain about:

  • Should one resize an image to a fixed set of potential widths (seemingly the Vue component's implementation), or "[calculate] breakpoints for every 0.1 megapixel increase in resolution"?
  • We can't just assume 100vw, so how difficult will it be to apply the constraints defined by sizes to the set of potential images?

@danielbachhuber
Copy link
Author

In re-reading the thread on GoogleChrome/lighthouse#11593 (comment), it seems like the best solution would be to:

  • Calculate breakpoints "that scale with the area of the image (aka the file size) rather than with a single dimension".
  • Optimize for the best, say, 6 breakpoints, but make the number of breakpoints configurable. The optimization would weight the breakpoints towards the higher end with 1920 a default upper bound (because we don't need to waste breakpoint budget on 3000 px wide images if the original image is 5000 px wide).
  • Have the image widths respect the sizes attribute too.
  • Ensure 2 and 3 DPI versions are accommodated for within the set of image sizes.

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

@ausi ausi self-assigned this Jan 14, 2022
@ausi
Copy link
Owner
ausi commented Jan 17, 2022

Added a recommentation in 35df131

Looks something like this now:

Bildschirmfoto 2022-01-17 um 17 29 14

@danielbachhuber
Copy link
Author

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
Copy link
Owner
ausi commented Jan 17, 2022

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

@danielbachhuber
Copy link
Author

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
Copy link
Author

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
Copy link
Owner
ausi commented Jan 20, 2022 •
edited
Loading

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
Copy link
Author

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 s 8000 hare 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
Copy link
Author

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
Copy link
Owner
ausi commented Jan 20, 2022

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
Copy link
Author

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>

This was referenced Jan 20, 2022
Open
Closed
@danielbachhuber
Copy link
Author

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 GoogleChrome/lighthouse#11593 and ultimately the suggestion of GoogleChrome/lighthouse#11593 (comment)

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
Copy link
Owner
ausi commented Jan 21, 2022

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
Copy link
Author

💯 Nice work on this!

@danielbachhuber danielbachhuber mentioned this issue Jan 31, 2022
Closed
@ausi ausi mentioned this issue Jun 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ausi ausi

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@danielbachhuber @ausi
0