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

AMP validation error due to to empty LD+JSON script #4430

Closed
westonruter opened this issue Mar 24, 2020 · 0 comments · Fixed by #4431
Closed

AMP validation error due to to empty LD+JSON script #4430

westonruter opened this issue Mar 24, 2020 · 0 comments · Fixed by #4431
Labels
Bug Something isn't working
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Mar 24, 2020

Bug Description

When testing in AMP plugin v1.5.0-RC1, I'm getting a validation error with code JSON_ERROR_EMPTY for a script emitted by Site Kit:

<script type="application/ld+json" id="__gaOptOutExtension"></script>

See google/site-kit-wp#1251 and introduced in google/site-kit-wp#1252.

Nevertheless, that HTML is not invalid AMP:

image

It appears the JSON_ERROR_EMPTY is not applicable to script elements with of type application/ld+json. The validator spec for LD+JSON is:

# JSON Linked Data
tags: {
  html_format: AMP
  html_format: AMP4ADS
  html_format: AMP4EMAIL
  html_format: ACTIONS
  tag_name: "SCRIPT"
  spec_name: "script type=application/ld+json"
  attr_lists: "nonce-attr"
  attrs: {
    name: "type"
    mandatory: true
    value_casei: "application/ld+json"
    dispatch_key: NAME_VALUE_DISPATCH
  }
  cdata: {
    blacklisted_cdata_regex: {
      regex: "<!--"
      error_message: "html comments"
    }
  }
}

No error is omitted for <amp-ima-video> either:

image

Its spec is:

# AMP IMA VIDEO
tags: {
  html_format: AMP
  html_format: ACTIONS
  tag_name: "SCRIPT"
  spec_name: "amp-ima-video > script[type=application/json]"
  mandatory_parent: "AMP-IMA-VIDEO"
  attrs: {
    name: "type"
    mandatory: true
    value_casei: "application/json"
    dispatch_key: NAME_VALUE_PARENT_DISPATCH
  }
  cdata: {
    blacklisted_cdata_regex: {
      regex: "<!--"
      error_message: "html comments"
    }
  }
}

Here it has the same CDATA spec, but one emits an warning and the other emits nothing.

It's the same for amp-analytics:

image

So the quick fix appears to be to eliminate the JSON_ERROR_EMPTY error code. It was introduced in #4340 (for #4320). The validator emits this as a warning:

            this.context_.addWarning(
                amp.validator.ValidationError.Code.INVALID_JSON_CDATA,
                this.context_.getLineCol(), /* params */[], '',
                this.validationResult_);

So since addWarning is being used as opposed to addError, the JSON_ERROR_EMPTY error should be removed. It turns out that application/ld+json scripts are not validated at all.

Steps to reproduce

  1. Enable AMP plugin with Standard or Transitional mode.
  2. Activate Site Kit with analytics.
  3. Validate an AMP page.
  4. See validation error.

Screenshots

image

Additional Context

  • Plugin Version: 1.5.1

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 westonruter added the Bug Something isn't working label Mar 24, 2020
@westonruter westonruter added this to the v1.5 milestone Mar 24, 2020
@westonruter westonruter changed the title AMP validation error due to to empty JSON+LD AMP validation error due to to empty JSON+LD script Mar 24, 2020
@westonruter westonruter changed the title AMP validation error due to to empty JSON+LD script AMP validation error due to to empty LD+JSON script Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant