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

Update picture source markup and make sizes attribute configurable from shortcode. #34

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Update picture source markup and make sizes attribute configurable from shortcode. #34

wants to merge 5 commits into from

Conversation

meigo
Copy link
Contributor

@meigo meigo commented Oct 8, 2020

In general images plugin works quite nicely for setting up responsive sizes for the same image.
'Art direction' would be too complicated to handle for such a general tool.

Have some proposals though. Current picture markup looks like this

<picture>
	<source type="image/webp" data-srcset="/images/img-ejs-1000.webp" media="(min-width: 1000px)">
	<source type="image/webp" data-srcset="/images/img-ejs-500.webp">
	<source data-srcset="/images/img-ejs-1000.jpeg media="(min-width: 1000px)">
	<source data-srcset="/images/img-ejs-500.jpeg">
	<img ...>
</picture>

🔗respimagelint complains about source elements:

Images in different elements shouldn’t be the same
The element should only be used for art direction and format-based selection. For providing multiple resolutions of the same image use the srcset attribute instead.

So, with w descriptors and sizes attribute it should look like this instead ('data-' prefixes are added for 🔗vanilla-lazyload)

<picture>
	<source type="image/webp" data-srcset="/images/img-ejs-500.jpeg 500w, /images/img-ejs-1000.webp 1000w" data-sizes="100vw">
	<source data-srcset="/images/img-ejs-500.jpeg 500w, /images/img-ejs-1000.jpeg 1000w" data-sizes="100vw" >
	<img ...>
</picture>

According to 🔗css-tricks x descriptors (1x 2x etc) only account for a small percentage of responsive images usage while using srcset / w + sizes accounts for around 85% of responsive images usage on the web.


As sizes attribute is too layout specific, it could have default value "100vw" and be configurable from shortcode as

{{picture src='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/images/img.jpg' alt="img" sizes="(min-width: 1000px) 33vw, 96vw" /}}
{{picture src='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/images/img.jpg' alt="img" sizes="(max-width: 500px) calc(100vw - 2rem), (max-width: 700px) calc(100vw - 6rem), calc(100vw - 9rem - 200px)" /}}

As I understand, exact sizes are almost impossible to set by hand as each browser does its own thing:

So, given the same viewport size and screen density, Chrome, Safari, Firefox, and Edge may very well each pick a different resource out of a given srcset.


Also 🔗respimagelint tool gives sizes suggestions that can be copied to shortcode from there.
No idea how this works but for example using it's suggested sizes on above the fold image:

sizes="(min-width: 560px) 100vw, calc(9.58vw + 471px)"

effectively calmed down lighthouse complaints about images with inappropriate size.


Used resources

Update picture source markup and make sizes attribute configurable from shortcode.
Update picture source markup and make sizes attribute configurable from shortcode.
@nickreese
Copy link
Contributor

nickreese commented Oct 15, 2020

Hey man, great contribution. Sorry for the delay. In testing it locally here is what I'm seeing.

  • scaled images are causing issues with the output, easily fixed.
  • using the default sizes seems to cause Firefox and Safari to always load the largest image. Maybe we offer an override of that default in the plugin config instead of having to specify it in the shortcode?.. taking the idea further, maybe there are "named" types of photos so you could pass in type="thumbnail" and the corresponds to a smaller sizes calculation?
  • If we go this way maybe we should rip out scale support as I don't think it'll be needed? If not how should we handle it?

@meigo
Copy link
Contributor Author

meigo commented Oct 17, 2020

using the default sizes seems to cause Firefox and Safari to always load the largest image. Maybe we offer an override of that default in the plugin config instead of having to specify it in the shortcode?.. taking the idea further, maybe there are "named" types of photos so you could pass in type="thumbnail" and the corresponds to a smaller sizes calculation?

Hi Nick,
I'm definitely no expert, just testing out responsive images feature.

This test: https://thirsty-aryabhata-198022.netlify.app/ has upper image sizes set to default ('100vw') and lower one ('(min-width: 500px) 500px, 100vw'). So I assume it would hint to browser that first image would be always scaled to the full width of screen and second one would be scaled to display width only when display is narrower than 500px and after that would be locked to 500px.

There are images with widths 500, 100 and 1500px.
So the second image would only need to be loaded with width 500px.

Observations on first image

  • Loading page with screen width about 300px > both chrome and firefox load 500px version of image.
  • Scaling up the screen width > Firefox loads 1000px version at screen width 400px and chrome ~560px.
  • Scaling up > Firefox loads 1500px version at screen width 800px and chrome ~980px.

So chrome works as expected - loads all sizes for the first image depending on screen width and only 500px version for second one.
Firefox switches to higher resolution much earlier and loads also 1000px version for second image even if it's not needed.

Also chrome will stick to largest image loaded, firefox will keep loading images when screen width goes up or down.

It might be that reformulating media conditions for sizes would affect this behaviour for firefox, hard to say, probably needs further research.

Default sizes attribute could be specified in config but overridden also from shortcode as it's usually quite image and layout specific.

There are other things as well that could be made configurable, like webp options as currently the output is quite heavy. Related mainly to this hard coded option

nearLossless: true,

but should deal with these separately probably

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

Successfully merging this pull request may close these issues.

2 participants