Skip to content

Prevent adding noscript fallbacks for AMP components inside AMP Stories#2107

Merged
westonruter merged 4 commits intoamp-stories-reduxfrom
fix/amp-story-noscript-fallbacks
Apr 11, 2019
Merged

Prevent adding noscript fallbacks for AMP components inside AMP Stories#2107
westonruter merged 4 commits intoamp-stories-reduxfrom
fix/amp-story-noscript-fallbacks

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented Apr 11, 2019

This PR adds a add_noscript_fallback argument to the sanitizers for img, video, audio, and iframe. By default this add_noscript_fallback is true, but for AMP Stories it gets set to false.

Builds on #1861 and #1913.


It turns out that the amp-img > noscript > img and amp-video > noscript > video (and others perhaps) are not allowed in AMP Stories. T

  • Prevent adding noscript > img fallback for amp-img.
  • Prevent adding noscript > video fallback for amp-video
  • Check if noscript fallbacks are needed for other elements (e.g. amp-audio)
  • Determine why adding a Video block still fails (see below).

[RESOLVED] Outstanding Issue: Adding a video block

Adding a video block is causing a validation error:

image

image

image

@googlebot googlebot added the cla: yes Signed the Google CLA label Apr 11, 2019
@westonruter westonruter marked this pull request as ready for review April 11, 2019 16:57
@westonruter westonruter requested a review from amedina April 11, 2019 16:57
Copy link
Copy Markdown
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and the fix works as described 👍

@westonruter westonruter merged commit 783ba27 into amp-stories-redux Apr 11, 2019
@westonruter westonruter deleted the fix/amp-story-noscript-fallbacks branch April 11, 2019 17:40
@westonruter westonruter added this to the v1.2 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Signed the Google CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants