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 script sanitizer to convert the most common scripts to AMP #1032

Closed
westonruter opened this issue Mar 21, 2018 · 6 comments
Closed

Add script sanitizer to convert the most common scripts to AMP #1032

westonruter opened this issue Mar 21, 2018 · 6 comments
Assignees

Comments

@westonruter
Copy link
Member

WordPress users very commonly add script tags to Custom HTML widgets and directly to theme templates to show ads, add analytics, etc. Assuming that the vast majority of these scripts are very templated (e.g. including via Google Tag Manager), then it should be possible to reliably identify the each script and extract the relevant information to convert to AMP.

See also #1031 which could serve as a way to build up a database of scripts for prioritization.

@amedina amedina self-assigned this Apr 2, 2018
@westonruter
Copy link
Member Author

As part of the script sanitizer this we could potentially look at de-duplicating validation errors that are currently coming when you enqueue some JS. For example, when enqueueing twentyseventeen-navigation there are two validation errors reported:

  • enqueued_script
  • invalid_element

The first is coming from when the script is first enqueued and the second comes from the whitelist sanitizer when the script is actually removed. This is due to the fact that the enqueued_script validation error is triggered prior to sanitization happening in the first place: it gets triggered during output buffering. When coming across an invalid_element error we could potentially look at AMP_Validation_Utils::$enqueued_script_sources to see if it exists there (by comparing the src with the enqueued script src URLs) and then skip reporting the validation error. But in this case should it also apply to any scripts that are printed due to being dependencies of the originally enqueued script?

@westonruter
Copy link
Member Author

This is being added in 811d133#diff-7d234989187d87a372e2aa6aa1299a71R672 via #1093.

@westonruter
Copy link
Member Author

It will be important to also make sure any noscript elements are removed if they have corresponding scripts that are recognized and converted to AMP.

@westonruter
Copy link
Member Author

@amedina Here's another bit of code that should be moved into the new script sanitizer:

https://github.com/Automattic/amp-wp/blob/a68c93fd62df93c6c79cf99d4917bb912e11b684/includes/class-amp-theme-support.php#L1064-L1069

@westonruter
Copy link
Member Author

Adding support for noscript contents replacing adjacent script elements could be a better first step for this (see #1213).

@westonruter
Copy link
Member Author

Closing per resolution in #1127 (comment)

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

Successfully merging a pull request may close this issue.

2 participants