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 noscript>img fallbacks for amp-img #1832

Closed
westonruter opened this issue Jan 18, 2019 · 2 comments
Closed

Add noscript>img fallbacks for amp-img #1832

westonruter opened this issue Jan 18, 2019 · 2 comments
Milestone

Comments

@westonruter
Copy link
Member

Search engines that index AMP pages that have <amp-img> will likely not know what the web component is, at a semantic level. Unless they have special logic to recognize what an amp-img is, they would have to fire up a headless browser to actually render the page with JavaScript in order to find out what the web component constructs in the DOM. Beyond search engines, users who have JavaScript disabled are also adversely affected by amp-img because it does not render given the lack of the AMP runtime.

However, as can be seen at https://ampbyexample.com/components/amp-img/ the amp-img element allows a regular img as fallback content if it inside of a noscript element:

<amp-img src="/img/amp.jpg"
  alt="AMP"
  width="475"
  height="268"
  layout="responsive">
  <noscript>
    <img src="/img/amp.jpg" width="475" height="268" alt="AMP"> 
  </noscript>
</amp-img>

So instead of replacing an img with an amp-img, we should be constructing an amp-img and then inserting the original img as a descendant of an added noscript element.

Related #1316: Add support for <picture> element, which also involves fallbacks.

@westonruter
Copy link
Member Author

Note that the script sanitizer would explicitly need to prevent unwrapping noscript elements under amp-img elements.

@westonruter
Copy link
Member Author

@hellofromtonya @kienstra I spent a bit of time on this in #1861. The remaining task is to finish updating the unit tests to account for the changes.

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

No branches or pull requests

2 participants
@westonruter and others