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

Capture script tag contents in AMP validator errors #1031

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

Capture script tag contents in AMP validator errors #1031

westonruter opened this issue Mar 21, 2018 · 3 comments
Assignees
Labels
Milestone

Comments

@westonruter
Copy link
Member

The AMP validator utilities should capture script tag contents when it is less than 1KB (or some threshold) so that we can gather data about the scripts which are removed, so we can begin to identify the most common scripts that should get their own dedicated post-processor to convert to AMP (e.g. Google Analytics, AdSense, etc).

@amedina amedina self-assigned this Apr 2, 2018
westonruter added a commit that referenced this issue Apr 26, 2018
* Eliminate enqueued_script validation error in favor of merging sources into invalid_element errors
* Add source information for inline scripts attached to enqueued scripts.
* Account for enqueueing script bundles (where src is false).
* Capture inline script contents in validation errors. Fixes #1031.
@westonruter westonruter added this to the v1.0 milestone Apr 26, 2018
@westonruter
Copy link
Member Author

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

westonruter added a commit that referenced this issue Apr 26, 2018
* Eliminate enqueued_script validation error in favor of merging sources into invalid_element errors
* Add source information for inline scripts attached to enqueued scripts.
* Account for enqueueing script bundles (where src is false).
* Capture inline script contents in validation errors. Fixes #1031.
westonruter added a commit that referenced this issue Apr 26, 2018
* Eliminate enqueued_script validation error in favor of merging sources into invalid_element errors
* Add source information for inline scripts attached to enqueued scripts.
* Account for enqueueing script bundles (where src is false).
* Capture inline script contents in validation errors. Fixes #1031.
westonruter added a commit that referenced this issue May 2, 2018
* Eliminate enqueued_script validation error in favor of merging sources into invalid_element errors
* Add source information for inline scripts attached to enqueued scripts.
* Account for enqueueing script bundles (where src is false).
* Capture inline script contents in validation errors. Fixes #1031.
@westonruter westonruter self-assigned this May 3, 2018
@kienstra
Copy link
Contributor

kienstra commented Jun 5, 2018

Steps To Test

Hi @csossi, Could you please test this?

  1. Go to this staging site
  2. Create a new post
  3. Add this in the post content (using the "Text" tab, not the "Visual" one):
<script>doSomething(); alsoDoThis();</script>
  1. Publish the post
  2. Observe that a notice appears:

There is content which fails AMP validation. Non-accepted validation errors prevent AMP from being served. Review issues

  1. Click "Review issues"
  2. Expected: The content of that script appears in the text field of a validation error: doSomething(); andDoThis();

@csossi
Copy link

csossi commented Jun 7, 2018

verified in QA

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

No branches or pull requests

5 participants