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

Add support for the <picture> element, converting to <amp-img> #1316

Closed
westonruter opened this issue Aug 4, 2018 · 7 comments
Closed

Add support for the <picture> element, converting to <amp-img> #1316

westonruter opened this issue Aug 4, 2018 · 7 comments
Labels
Blocked Enhancement New feature or improvement of an existing one P2 Low priority Sanitizers WS:Core Work stream for Plugin core

Comments

@westonruter
Copy link
Member

westonruter commented Aug 4, 2018

The AMP_Img_Sanitizer needs to be extended to add support for the <picture> element. For example, on https://fivethirtyeight.com/features/the-eternal-question-how-much-do-these-apricots-weigh/ there is:

<picture class="featured-picture">
	<source media="(min-width: 768px)" srcset="https://fivethirtyeight.com/wp-content/uploads/2016/07/riddler_4x3_default.gif?strip=info&amp;ssl=1">
	<source srcset="https://fivethirtyeight.com/wp-content/uploads/2016/07/riddler_4x3_default.gif?strip=info&amp;ssl=1">
	<img alt="riddler_4x3_default" src="https://fivethirtyeight.com/wp-content/uploads/2016/07/riddler_4x3_default.gif?strip=info&#038;ssl=1" />
</picture>

I'm not sure in this case why there are multiple source elements that have the same URL. But normally a type would be defined on the source so that the user agent can skip a source that is not supported. See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture

This should be converted into the appropriate amp-img. Note that amp-img allows for fallbacks to be defined as children. So that is how amp-img can be used to implement multiple sources: https://www.ampproject.org/docs/reference/components/amp-img#example:-specifying-a-fallback-image

The picture element allows for an img to be nested inside of it as a fallback, so it's not critical that the plugin recognize a picture to work.

@westonruter westonruter added Enhancement New feature or improvement of an existing one Sanitizers labels Aug 4, 2018
@westonruter westonruter added this to the v1.1 milestone Nov 12, 2018
@westonruter westonruter removed this from the v1.1 milestone Mar 6, 2019
@schlessera
Copy link
Collaborator

schlessera commented Aug 22, 2019

According to the docs, the <amp-img> element does directly support a srcset attribute, so I assume it is not needed to nest <amp-img> elements to simulate the <picture> element.

EDIT: That doesn't cover the type attribute of the <picture> element, though...

@schlessera
Copy link
Collaborator

So far I assume the expected output should be as follows...
1.)

<picture><img></picture>
=> <amp-img><noscript><img></noscript></amp-img>

2.)

<picture><source srcset /><img></picture>
=> <amp-img srcset><noscript><img></noscript></amp-img>

3.)

<picture><source srcset /><source srcset /><img></picture>
=> <amp-img srcset><amp-img srcset><noscript><img></noscript></amp-img></amp-img>

@schlessera
Copy link
Collaborator

The AMP_Img_Sanitizer needs to be extended to add support for the <picture> element.

As the AMP_Img_Sanitizer responds to the img tag only (and sanitizers seem to be built around singular tags, I opted to create a new AMP_Picture_Sanitizer instead of adapting the existing AMP_Img_Sanitizer.

I initially thought about literally extending AMP_Img_Sanitizer as a parent class, but the benefits of that don't really warrant the added coupling. If we need to reuse code across these, we can extract it out as needed.

@westonruter
Copy link
Member Author

EDIT: That doesn't cover the type attribute of the <picture> element, though...

Is this not implicit by nesting an amp-img inside another amp-img? As in the serve different image formats example:

<amp-img alt="Mountains"
  width="550"
  height="368"
  layout="responsive"
  src="/static/inline-examples/images/mountains.webp">
  <amp-img alt="Mountains"
    fallback
    width="550"
    height="368"
    layout="responsive"
    src="/static/inline-examples/images/mountains.jpg"></amp-img>
</amp-img>

@westonruter
Copy link
Member Author

Per conversation on #3082 (comment) there's not likely going to be short-term support for a picture-like amp-img. So it doesn't make sense to push for this now. We can revisit later.

@westonruter
Copy link
Member Author

This came up again: https://wordpress.org/support/topic/webp-image-not-loading/

Since the AMP plugin doesn't support converting picture and its sources into amp-img, perhaps what should be done is explicitly replacing the picture element with its fallback img child, and then allow the image sanitizer to convert the img into amp-img. This would prevent picture elements from being flagged internally as validation errors.

This should only be done if the underlying img fallback is a suitable replacement for the picture. Otherwise, the validation errors should still get flagged since we aren't able to convert it automatically.

@westonruter
Copy link
Member Author

With amp-img being deprecated in ampproject/amphtml#30442, I'm pretty sure this means that picture will eventually become allowed, since img will be allowed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Enhancement New feature or improvement of an existing one P2 Low priority Sanitizers WS:Core Work stream for Plugin core
Projects
None yet
Development

No branches or pull requests

3 participants