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

Validator reports valid script types as invalid #4392

Closed
aaemnnosttv opened this issue Mar 16, 2020 · 3 comments
Closed

Validator reports valid script types as invalid #4392

aaemnnosttv opened this issue Mar 16, 2020 · 3 comments

Comments

@aaemnnosttv
Copy link
Contributor

Bug Description

When validating a URL, the validator reports that script types are invalid, in contrast to the valid types defined in the docs for AMP HTML.

According to the AMP HTML Specification, <script> tags are allowed with the following type attributes:

  • application/ld+json
  • application/json
  • text/plain

Of these, only application/ld+json is not flagged as invalid by the validator.

image

image

Expected Behaviour

All valid script types outlined by the AMP HTML Spec should not be marked invalid and removed by the AMP Validator.

Steps to reproduce

  1. Validate any page with a script tag on the page with either type of application/json or text/plain
  2. See validation error

Additional context

  • WordPress version: 5.3.2
  • Plugin version: 1.4.4
  • Gutenberg plugin version (if applicable):
  • AMP plugin template mode: Standard
  • PHP version: 7.2
  • OS: MacOS
  • Browser: Chrome
  • Device: Macbook Pro

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter
Copy link
Member

These script types are allowed only when they are used on the context of other AMP components. You can't add an application/json script anywhere. It had to be inside of an amp-analytics component, for example.

So this is working as expected.

In v1.5 the error message will be improved to explain why it is invalid.

@aaemnnosttv
Copy link
Contributor Author

Thanks for clarifying @westonruter! I should have tested this with https://validator.amp.dev first, which fails with the same types as well. Am I interpreting the spec incorrectly or do you think this could be clarified better since it doesn't mention that some types are only valid in a particular context?

image

@westonruter
Copy link
Member

westonruter commented Mar 16, 2020

What you have screenshotted are the docs, not the formal spec. (The docs could be made more clear here.) To see what the formal spec requires, you should look at the underlying protoascii.

For example, here is a definition for an application/json that is used for amp-ima-video:
https://github.com/ampproject/amphtml/blob/8be63bf7b769f7c903a7d57bf44f8cca19760743/validator/validator-main.protoascii#L5036-L5055

Notice this key line:

mandatory_parent: "AMP-IMA-VIDEO"

You can see that same by looking at https://github.com/ampproject/amp-wp/blob/develop/includes/sanitizers/class-amp-allowed-tags-generated.php

For every instance of application/json you'll see it has an associated tag_spec that has a mandatory_parent.

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