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

Support of imagesrcset and imagesizes attributes for preloading images #29302

Closed
CraigBusfield opened this issue Jul 16, 2020 · 7 comments
Closed

Comments

@CraigBusfield
Copy link

Describe the new feature or change to an existing feature you'd like to see

Support of imagesrcset and imagesizes attributes for <link rel="preload"> based on the HTML standard here: https://html.spec.whatwg.org/multipage/semantics.html#attr-link-imagesrcset

Describe alternatives you've considered

We've tried simply using the media attribute which works in some situations, but causes overly complex HTML when more control is needed, and in other cases I don't think it's capable of replicating the missing attributes as per the examples below.

In our first example, we have 4 breakpoints, each with their own art direction and the ability to serve 2x and 3x resolution variants. This is possible to do with the media attribute but it does mean 12 link tags need to be printed out in the page, rather than 4 if imagesrcset was available, and we'd like to avoid unnecessary bloat/complexity if possible. Here's the example of the code that works, but is invalid in AMP scenarios

<link
  rel="preload"
  as="image"
  imagesrcset="xsmall-1x.jpg 1x, xsmall-2x.jpg 2x, xsmall-3x.jpg 3x"
  media="(max-width:480px)"
/>
<link
  rel="preload"
  as="image"
  imagesrcset="small-1x.jpg 1x, small-2x.jpg 2x, small-3x.jpg 3x"
  media="(min-width:481px) and (max-width:767px)"
/>
<link
  rel="preload"
  as="image"
  imagesrcset="medium-1x.jpg 1x, medium-2x.jpg 2x, medium-3x.jpg 3x"
  media="(min-width:768px) and (max-width:1024px)"
/>
<link
  rel="preload"
  as="image"
  imagesrcset="large-1x.jpg 1x, large-2x.jpg 2x, large-3x.jpg 3x"
  media="(min-width: 1025px)"
/>

In our second example, which includes an image that displays at different widths of the screen depending upon the breakpoint and relies on the sizes attribute to calculate this, I don't know of an alternative for preloading efficiently. Take the following code as an example

<link rel="preload" as="image"
      imagesrcset="wolf_400px.jpg 400w, wolf_800px.jpg 800w, wolf_1600px.jpg 1600w"
      imagesizes="(max-width:768px) 100vw, 50vw">

this would then be used for the following img tag:

<img src="wolf.jpg" alt="A rad wolf"
     srcset="wolf_400px.jpg 400w, wolf_800px.jpg 800w, wolf_1600px.jpg 1600w"
     sizes="(max-width:768px) 100vw, 50vw">

N.B. I'm aware that the sizes attribute is not needed in this example if using amp-img due initially to #20968

Additional context

Looking through the validator files, it seems that these attributes would just need adding to an allowlist somewhere around

but I'm not sure if they're not included already for a particular reason

@jridgewell
Copy link
Contributor

/cc @ampproject/wg-caching

@twifkak
Copy link
Member

twifkak commented Jul 16, 2020

At first glance, this seems fine. Probably just not included because these attributes are newer than the creation of that rule. I think before allowing this we need to figure out what (if any) changes to cache-side transforms are necessary.

FYI @amaltas who has done some work in this space.

@twifkak
Copy link
Member

twifkak commented Jul 20, 2020

Discussed at wg-caching standup.

@dvoytenko @nainar Does this seem like something we should do? (We're guessing "yes" for AMP First, but wanted confirmation.)

Assigning P3 conservatively, but open to change.

Other details:

  • allow attributes only for image preloads (e.g. not 'v0')
  • transforms should either delete these links or rewrite URLs
  • check for other transformers that do link stuff

@dvoytenko
Copy link
Contributor

I have no reservations about supporting this. However, we need to correctly rewrite these links on cache. So we can't validate these tags until the cache processes them safely.

@dvoytenko
Copy link
Contributor

It looks like cache processes it correctly, so we should be all good supporting it in the validator.

@twifkak
Copy link
Member

twifkak commented Jul 21, 2020

Bumping priority per discussion with @dvoytenko.

@jridgewell jridgewell self-assigned this Jul 29, 2020
twifkak pushed a commit to ampproject/amppackager that referenced this issue Aug 13, 2020
Adds validator support for the two new `<link rel=preload>` attributes, and adds srcset URL rewriting for imagesrcset.

Fixes ampproject/amphtml#29302

PiperOrigin-RevId: 325359140
twifkak pushed a commit to ampproject/amppackager that referenced this issue Aug 13, 2020
Adds validator support for the two new `<link rel=preload>` attributes, and adds srcset URL rewriting for imagesrcset.

Fixes ampproject/amphtml#29302

PiperOrigin-RevId: 325359140
@twifkak
Copy link
Member

twifkak commented Sep 16, 2020

actually this was fixed in #29830

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

No branches or pull requests

6 participants