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

Automatically promote contents of noscript elements as acceptable replacement for adjoining script tag #1213

Closed
westonruter opened this issue Jun 14, 2018 · 3 comments

Comments

@westonruter
Copy link
Member

If a theme or plugin outputs something like:

Will this script run? <script>document.write('YES!');</script><noscript>No!</noscript>

It would make sense to me if the AMP plugin would automatically take the child elements of the noscript element to take the replace of the script element that is sanitized from the document. This would ensure that any fallback behavior would run as intended for browsers that have JS disabled.

For example, Google Tag Manager advises using the following HTML:

<!-- Google Tag Manager -->
<script>(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start':
new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0],
j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
'https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);
})(window,document,'script','dataLayer','GTM-XXXX');</script>
<!-- End Google Tag Manager -->

<!-- Google Tag Manager (noscript) -->
<noscript><iframe src="https://www.googletagmanager.com/ns.html?id=GTM-XXXX"
height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>
<!-- End Google Tag Manager (noscript) -->

When this gets served as AMP, then this could get all replaced with the contents of noscript, namely:

<iframe src="https://www.googletagmanager.com/ns.html?id=GTM-XXXX"
height="0" width="0" style="display:none;visibility:hidden"></iframe>

This would make sense as part of the AMP_Script_Sanitizer as proposed in #1032. When the script sanitizer does this replacement, it could result in bypassing the pass-through of the script element to the whitelist sanitizer and thus avoid raising a blocking validation error. And again, the fallback behavior would be preserved.

@postphotos
Copy link
Contributor

Thanks @westonruter! Agreed here, thanks for opening this ticket.

Calling out what should be obvious (but hasn't been stated): <noscript> is indeed, permitted in AMP, and has been available for quite a while.

@westonruter
Copy link
Member Author

Note that most of the work in this issue has been done already via #1226. What remains is determining a way to link a noscript with an adjoining script for the purposes of bypassing a validation error. This may not be feasible.

@westonruter westonruter modified the milestones: v1.0, v1.1 Sep 24, 2018
@postphotos postphotos added this to Definition in v1.1 Sep 25, 2018
@westonruter westonruter removed this from the v1.1 milestone Mar 6, 2019
@westonruter westonruter removed this from Definition in v1.1 May 14, 2019
@westonruter
Copy link
Member Author

I don't think this is going to be feasible, or at least not reliable. Often a noscript for a script may not be adjacent, and who knows whether a noscript is for the script before or after.

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

2 participants