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

Allow <source> to use <img>'s sizes data as a fallback #6633

Open
jakearchibald opened this issue Apr 29, 2021 · 27 comments · May be fixed by #6695
Open

Allow <source> to use <img>'s sizes data as a fallback #6633

jakearchibald opened this issue Apr 29, 2021 · 27 comments · May be fixed by #6695
Labels
addition/proposal New features or enhancements topic: img

Comments

@jakearchibald
Copy link
Contributor

jakearchibald commented Apr 29, 2021

Within a <picture>, <source> and <img> can have a sizes attribute.

sizes defines the layout width of the image, in CSS pixels. It supports media conditions so you can provide different layout widths for different viewport widths.

Currently, this must be specified on every <source> and <img> that uses w units in its srcset. However, it feels like the sizes data will frequently be the same in every <source> and <img> within a given <picture>, since the image's width is dictated by the CSS that operates at a document level.

To avoid duplication, should <source> take its sizes data from the <img> if it doesn't have a sizes attribute of its own?

@zcorpan, I'm told you might have thoughts here.

@annevk annevk added topic: img addition/proposal New features or enhancements labels Apr 29, 2021
@jakearchibald
Copy link
Contributor Author

jakearchibald commented Apr 29, 2021

Here's a worked example:

<p>Currently, need to duplicate sizes:</p>
<picture>
  <source type="image/avif"
       media="(min-width: 800px)"
       srcset="f1-689.avif 689w,
               f1-1378.avif 1378w,
               f1-500.avif 500w,
               f1-1000.avif 1000w"
       sizes="(min-width: 1066px) 689px,
              calc(75vw - 137px)">
  <source media="(min-width: 800px)"
       srcset="f1-689.jpg 689w,
               f1-1378.jpg 1378w,
               f1-500.jpg 500w,
               f1-1000.jpg 1000w"
       sizes="(min-width: 1066px) 689px,
              calc(75vw - 137px)">
  <source type="image/avif"
       srcset="f1-focused-800.avif 800w,
               f1-focused-1406.avif 1406w"
       sizes="(min-width: 530px) calc(100vw - 96px),
              100vw">
  <img alt="F1 car"
       src="f1-cropped-800.jpg"
       srcset="f1-focused-800.jpg 800w,
               f1-focused-1406.jpg 1406w"
       sizes="(min-width: 530px) calc(100vw - 96px),
              100vw">
</picture>

In this case I've split the sizes data, so it only includes the sizes that would apply when that source is selected. But, I still need to duplicate that data between the avif and jpeg version.

However, it feels like the way I've split the sizes is bad for maintainability. sizes is "give me the layout width of this image element in advance", so it shouldn't be source specific.

With my proposal, it would be:

<p>No duplicate sizes!</p>
<picture>
  <source type="image/avif"
       media="(min-width: 800px)"
       srcset="f1-689.avif 689w,
               f1-1378.avif 1378w,
               f1-500.avif 500w,
               f1-1000.avif 1000w">
  <source media="(min-width: 800px)"
       srcset="f1-689.jpg 689w,
               f1-1378.jpg 1378w,
               f1-500.jpg 500w,
               f1-1000.jpg 1000w">
  <source type="image/avif"
       srcset="f1-focused-800.avif 800w,
               f1-focused-1406.avif 1406w">
  <img alt="F1 car"
       src="f1-cropped-800.jpg"
       srcset="f1-focused-800.jpg 800w,
               f1-focused-1406.jpg 1406w"
       sizes="(min-width: 1066px) 689px,
              (min-width: 800px) calc(75vw - 137px),
              (min-width: 530px) calc(100vw - 96px),
              100vw">
</picture>

Now there's no duplication, and all the layout information is in one place.

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Apr 29, 2021

Alternatively, rather than falling back to <img>'s sizes if no sizes is present on the source, it could get it from the next matching <source> that does have sizes, eventually falling back to <img>.

That means you can still split sizes across <source>s, if there are cases where that's easier to maintain:

<p>No duplicate sizes!</p>
<picture>
  <!-- this gets its sizes from the next <source>
  since the media also matches -->
  <source type="image/avif"
       media="(min-width: 800px)"
       srcset="f1-689.avif 689w,
               f1-1378.avif 1378w,
               f1-500.avif 500w,
               f1-1000.avif 1000w">
  <source media="(min-width: 800px)"
       srcset="f1-689.jpg 689w,
               f1-1378.jpg 1378w,
               f1-500.jpg 500w,
               f1-1000.jpg 1000w"
       sizes="(min-width: 1066px) 689px,
              calc(75vw - 137px)">
  <!-- this gets its sizes from the <img> -->
  <source type="image/avif"
       srcset="f1-focused-800.avif 800w,
               f1-focused-1406.avif 1406w">
  <img alt="F1 car"
       src="f1-cropped-800.jpg"
       srcset="f1-focused-800.jpg 800w,
               f1-focused-1406.jpg 1406w"
       sizes="(min-width: 530px) calc(100vw - 96px),
              100vw">
</picture>

@zcorpan
Copy link
Member

zcorpan commented Apr 29, 2021

It seems likely that it will be the same value for every source in the vast majority of cases. The only somewhat reasonable thing I can think of is having entirely different breakpoints between screen and print and wanting to have that be reflected in images. (But see #4482.)

The main crux I guess is that picture wasn't optimized for avoiding duplication, but instead to be flexible, easy to reason about, and avoid unnecessary implementation complexity. When combining different use cases it becomes almost ridiculously repetitive.

A very early design of srcset actually was designed to avoid repetition, through a URL template: https://junkyard.damowmow.com/506 -- but this didn't work because people need to be able to use arbitrary URLs for each possible image, IIRC.

That said, it seems not terrible to fall back to the sizes of the img. It would be similar to width and height, which can now also be specified on source (but will use the ones on img if not specified). The idea of using a sizes of a later source seems a bit too magic and unexpected though, imo.

@jakearchibald
Copy link
Contributor Author

Yeah, that seems fair to me

@Yay295
Copy link
Contributor

Yay295 commented Apr 30, 2021

Why not allow sizes on <picture> itself? A sizes attribute on a child would override this value for that child.

@zcorpan
Copy link
Member

zcorpan commented Apr 30, 2021

@Yay295 that would be inconsistent with all other attributes involved in picture/source/img. The img is what owns the resource selection, and all "fallback" attributes (src, width, height, alt...) are on the img.

@romainmenke
Copy link

What would the effect be on cases where sizes is missing by mistake on source elements?

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Apr 30, 2021

Right now, it'll take a default of 100vw, which means the browser may pick a suboptimal source. Changing the implementation will improve cases like that.

The change will cause compatibility problems with sites that have deliberately omitted the sizes attribute from a source, and are expecting it to result in the fallback value of 100wv. However, I can't think of real-world cases where you'd do that.

@romainmenke
Copy link

romainmenke commented Apr 30, 2021

What I am seeing in our sites ( tiny sample size, but still :) ) is that sizes is always missing on source elements.
With closer inspection there are two main cases :

  1. sizes attribute is just not needed as there is no srcset with multiple entries with w units.
  2. Fallback to 100vw gives the correct result (might be by design, or by accident).

There is a very small group of cases that are wrong and will need updating, but personally I am fine with that.
Overal this change will highlight incorrect usage and make it easier to fix, I think.


It does not help that there is no code snippet on MDN showing srcset + sizes on either picture or source

The docs do clearly state the purpose and necessity of the sizes attribute.
But a more elaborate example might help here.

@zcorpan
Copy link
Member

zcorpan commented Apr 30, 2021

@jakearchibald are you interested in investigating the web compat impact? An elaborate HTTP Archive query (or series of queries) could maybe work, or implementing a use counter that triggers when the selected <source> has a w-descriptor srcset and no sizes attribute, and the img has a sizes attribute, or something along those lines.

Even for cases that do match that, for it to be "breaking" (in terms of changed layout, not just which image was loaded from srcset), it also needs to have no dimensions specified with CSS or the width and height attributes. I would expect this to be very rare.

@jakearchibald
Copy link
Contributor Author

Yeah, I can take this on, and I agree with your analysis.

@jakearchibald
Copy link
Contributor Author

jakearchibald commented May 4, 2021

@zcorpan here's my plan, let me know if it sounds bad (I don't have a ton of HTTP Archive experience):

  1. Query latest/response_bodies_desktop where page = url and the body contains something that looks like a <source> that has a srcset with a w-descriptor, but no sizes. I guess this will need to use RegEx 😬
  2. Put those URLs through a puppeteer script that:
    1. Confirms there's a <source> in a <picture> that doesn't have sizes, it has a srcset with more than one entry using the w-descriptor, the <img> has sizes that isn't 100vw. Otherwise it skips this URL.
    2. For each <source>:
      1. Try to make the media apply (eg by resizing viewport)
      2. Copy the sizes from the <img> and see if the selected image changes, and if layout changes.

The selected image changing is ok, but it'd be interesting to know. The layout changing would be a worry.

@zcorpan
Copy link
Member

zcorpan commented May 4, 2021

I ran a query today to capture all <picture>...</picture> markup in latest.response_bodies_desktop, as it happens. See #6627 (comment)

(I tried sharing the resulting table with you, but got an error. Oh well.)

Confirms there's a <source> worth testing, and the related <img> has sizes (bails otherwise).

This could probably be checked in step 1 before running matches through the puppeteer script, to reduce the number of pages to run through step 2. (I figure if you're running step 2 on your own computer, you want a not-so-huge dataset so it doesn't take forever to complete.)

But yeah, I think that should find the interesting cases!

@jakearchibald
Copy link
Contributor Author

It still needs to be checked in puppeteer in case the source has changed, or the RegEx got it wrong.

@jakearchibald
Copy link
Contributor Author

jakearchibald commented May 13, 2021

@zcorpan here's the query I'm using to gather the URLs. Let me know if something looks amiss:

SELECT
  page
FROM (
  SELECT
    page,
    ARRAY_TO_STRING(REGEXP_EXTRACT_ALL(LOWER(body), r'<source[^>]*\ssrcset=[\'"][^\'"]*\s\d+w[^\'"]*[^>]*>'), '\n') AS markup
  FROM
    `httparchive.latest.response_bodies_desktop`
  WHERE
    page = url )
WHERE
  markup != ''
  AND NOT REGEXP_CONTAINS(markup, r'sizes=')

@zcorpan
Copy link
Member

zcorpan commented May 14, 2021

@jakearchibald it looks OK. There are some minor tweaks to make it a bit more correct (like allow spaces around the =), but it usually doesn't impact the numbers noticeably.

@jakearchibald
Copy link
Contributor Author

jakearchibald commented May 14, 2021

Phase 1: From 5,642,228 HTML pages, 3251 (0.06%) matched the regex in the above query.

Phase 2: Of that group, 258 were found to have a <source> within <picture> that had a srcset, but not a sizes, where the corresponding <img> did have a sizes that wasn't 100vw.

Phase 2 was done with node & JSDOM, so pages may have changed since the versions in the HTTP archive. 123 of the URLs from page 1 failed to fetch.

Numbers here seem so small, I don't think it's worth digging in further. WDYT @zcorpan?

One-liner to find impacted <picture>s on pages from the above list:

[...new Set($$('source[srcset]:not([sizes])').map(el => el.closest('picture')))].filter(el => { const img = el.querySelector('img'); return img && img.sizes && img.sizes !== '100vw';})

@jakearchibald
Copy link
Contributor Author

Picking a few at random:

https://www.harpercollinschristian.com/ - the picture elements here are really weird. The width descriptor is very different to the actual image widths.

https://waterlesshaircare.com/en-us - the sources only have one item in the srcset. I could filter for this.

https://www.dominatoryachts.com/en - the proposed change would improve src selection on this page.

https://www.yalehome.co.nz/en/ - this is an interesting one:

<picture>
  <source
    srcset="/presets/small-mobile/Yale/yalehomeCONZ/Banners/store-old%20mobile.jpg 480w, /presets/medium-mobile/Yale/yalehomeCONZ/Banners/store-old%20mobile.jpg 767w"
    media="(max-width: 767px)">
  <img
    src="/presets/large-desktop/Yale/yalehomeCONZ/Banners/store-old.jpg"
    srcset="/presets/small-desktop/Yale/yalehomeCONZ/Banners/store-old.jpg 768w, /presets/medium-desktop/Yale/yalehomeCONZ/Banners/store-old.jpg 1345w, /presets/large-desktop/Yale/yalehomeCONZ/Banners/store-old.jpg 1800w"
    sizes="(min-width: 767px) 100vw, 1800px">
</picture>

If I'm reading that right, the change proposed here would result in browsers selecting a higher resolution image than needed at smaller (<767px) widths.

https://springandvine.com/en-us - another one with a single item in each srcset. The sizes are just wrong here too.

https://www.allianz.co.uk/ - the sizes are pretty wrong here, but it looks like the change proposed here would improve image selection.

@romainmenke
Copy link

Hehe, we make up 2% of affected sites.
Does kinda confirm your test as it found the same sites as I did checking each manually.
For me this is a non issue with such a low number of issues globally.

@jakearchibald
Copy link
Contributor Author

@romainmenke hah, this gives me some confidence that the test is correct

@jakearchibald
Copy link
Contributor Author

PR #6695

@jakearchibald
Copy link
Contributor Author

@zcorpan do you know who works on this stuff at Safari & Firefox, to see if they're happy with this change?

@zcorpan
Copy link
Member

zcorpan commented May 19, 2021

258 out of 5,642,228 is ~0.0046%, and per #6633 (comment) those cases are mostly using sizes wrong. So yeah, I think users aren't going to notice anything breaking here.

cc @emilio @smfr @rwlbuis

@colinbendell
Copy link

Echoing the comments, the use of sizes is nearly mostly wrong / weird. In the 2019 almanac we found the use of sizes in the proper way was very limited. Specifically, the patterns we found boiled down to:

  • <img sizes="auto"> - a pattern used by lazysizes
  • <img sizes="(max-width: 300px) 100vw, 300px"> - this is the second most popular design pattern. It is the one auto generated by WordPress and likely a few other platforms. It appears auto generated based on the original image size (in this case 300px).
  • <img sizes="100vw">
  • <img sizes="200px">
  • <img sizes="(max-width: 767px) 89vw, (max-width: 1000px) 54vw, ...">

Of which the last style was pretty rare.

@jakearchibald
Copy link
Contributor Author

The last one is 'most correct' right? (although I personally use min-width, as that's how I write my media queries)

@colinbendell
Copy link

By which "most correct" I would qualify it that it appears that a human hand crafted an intention and was generally unique across sites. The example snippet above was a random selection for illustration. I'll see if I have the results of the original query still handy (see: 04-06.sql and 04-06b.sql ).

@strarsis
Copy link

strarsis commented Feb 3, 2022

@colinbendell: That's a nice wrap-up of sizes use cases. Also I didn't know that queries against a large corpus of HTML is possible using the HTTP archive.

The static width (px) default/fallback approach that also CMS like WordPress uses appears to have an additional caveat with regards to device pixel density, see #7580.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: img
Development

Successfully merging a pull request may close this issue.

7 participants