Validator updates (#3179)

* Produce line/col numbers for on=tap errors. * Allow JSON-LD in the document `<body>` as well as the `<head>`. * Add preload attr to amp-video allowing the values specified at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video. * Add protocol example with whitespace to unittests. * spec file version bump due to amp-social-share update
ampproject · May 10, 2016 · 2b734ea · 2b734ea
1 parent 745dee0
commit 2b734ea
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 64 deletions.
diff --git a/validator/testdata/feature_tests/aria.out b/validator/testdata/feature_tests/aria.out
@@ -1 +1,5 @@
-PASS
+FAIL
+feature_tests/aria.html:37:2 The attribute 'role' in tag 'amp-img' is missing or incorrect, but required by attribute 'on'. (see https://www.ampproject.org/docs/reference/amp-img.html) [DISALLOWED_HTML]
+feature_tests/aria.html:39:2 The attribute 'tabindex' in tag 'amp-img' is missing or incorrect, but required by attribute 'on'. (see https://www.ampproject.org/docs/reference/amp-img.html) [DISALLOWED_HTML]
+feature_tests/aria.html:41:2 The attribute 'role' in tag 'amp-img' is missing or incorrect, but required by attribute 'on'. (see https://www.ampproject.org/docs/reference/amp-img.html) [DISALLOWED_HTML]
+feature_tests/aria.html:41:2 The attribute 'tabindex' in tag 'amp-img' is missing or incorrect, but required by attribute 'on'. (see https://www.ampproject.org/docs/reference/amp-img.html) [DISALLOWED_HTML]
diff --git a/validator/testdata/feature_tests/urls.html b/validator/testdata/feature_tests/urls.html
@@ -81,5 +81,6 @@
   <amp-iframe src="http://amp-iframe" width="42" height="42"></amp-iframe>
   <amp-pixel src="http://amp-pixel" layout="fixed" width="42"></amp-pixel>
   <amp-video src="http://amp-video" width="42" height="42"></amp-video>
+  <a href=" j a v a s c r i p t :alert(1)">Invalid protocol</a>
 </body>
 </html>
diff --git a/validator/testdata/feature_tests/urls.out b/validator/testdata/feature_tests/urls.out
@@ -25,4 +25,5 @@ feature_tests/urls.html:80:2 Invalid URL protocol 'http:' for attribute 'src' in
 feature_tests/urls.html:81:2 Invalid URL protocol 'http:' for attribute 'src' in tag 'amp-iframe'. (see https://www.ampproject.org/docs/reference/extended/amp-iframe.html) [AMP_TAG_PROBLEM]
 feature_tests/urls.html:82:2 Invalid URL protocol 'http:' for attribute 'src' in tag 'amp-pixel'. (see https://www.ampproject.org/docs/reference/amp-pixel.html) [AMP_TAG_PROBLEM]
 feature_tests/urls.html:83:2 Invalid URL protocol 'http:' for attribute 'src' in tag 'amp-video'. (see https://www.ampproject.org/docs/reference/amp-video.html) [AMP_TAG_PROBLEM]
-feature_tests/urls.html:85:7 The mandatory tag 'link rel=canonical' is missing or incorrect. (see https://www.ampproject.org/docs/reference/spec.html#required-markup) [MANDATORY_AMP_TAG_MISSING_OR_INCORRECT]
+feature_tests/urls.html:84:2 Invalid URL protocol 'j a v a s c r i p t :' for attribute 'href' in tag 'a'. [DISALLOWED_HTML]
+feature_tests/urls.html:86:7 The mandatory tag 'link rel=canonical' is missing or incorrect. (see https://www.ampproject.org/docs/reference/spec.html#required-markup) [MANDATORY_AMP_TAG_MISSING_OR_INCORRECT]
diff --git a/validator/validator-in-browser.js b/validator/validator-in-browser.js
@@ -14,13 +14,11 @@
  * See the License for the specific language governing permissions and
  * limitations under the license.
  */
-goog.require('amp.validator.ValidationError');
 goog.require('amp.validator.ValidationResult');
 goog.require('amp.validator.validateString');
 goog.require('goog.Promise');
 
 goog.provide('amp.validator.validateInBrowser');
-goog.provide('amp.validator.validateTapActionsA11y');
 goog.provide('amp.validator.validateUrlAndLog');
 
 /**
@@ -46,61 +44,6 @@ function getUrl(url) {
   });
 }
 
-/**
- * Prints an element reference for error messages.
- * @param {!HTMLElement} element
- * @return {!string}
- */
-function printElementRef(element) {
-  // TODO(dvoytenko,johannes): Propagate the element object itself
-  // into the error object and log that to the browser's console.
-  if (element.hasAttribute('id')) {
-    return 'Element ' + element.tagName + '#' + element.getAttribute('id');
-  }
-  return 'Element ' + element.tagName;
-}
-
-/**
- * This looks for elements with an on attribute with missing role / tabindex
- * attr. Taken from https://github.com/ampproject/amphtml/pull/358.
- * @param {!Document} doc
- * @param {!amp.validator.ValidationResult} validationResult
- */
-amp.validator.validateTapActionsA11y = function(doc, validationResult) {
-  // TODO(johannes): Unfortunately this doesn't come with a way to
-  // generate line / column numbers like we do for all of our errors,
-  // and also it could work just fine with SAX. Port this to validator.js +
-  // validator.protoascii.
-
-  const elements = doc.querySelectorAll('[on*="tap:"]');
-  for (let i = 0; i < elements.length; i++) {
-    const element = goog.asserts.assertInstanceof(elements[i], HTMLElement);
-    if (element.tagName === 'A' || element.tagName === 'BUTTON') {
-      continue;
-    }
-    if (!element.hasAttribute('role')) {
-      const error = new amp.validator.ValidationError();
-      error.severity = amp.validator.ValidationError.Severity.ERROR;
-      error.code = amp.validator.ValidationError.Code.MANDATORY_ATTR_MISSING;
-      error.detail =
-          'A11Y: ' + printElementRef(element) + ' must have "role" attribute ' +
-          'due to "tap" action, e.g. role="button".';
-      validationResult.errors.push(error);
-      validationResult.status = amp.validator.ValidationResult.Status.FAIL;
-    }
-    if (!element.hasAttribute('tabindex')) {
-      const error = new amp.validator.ValidationError();
-      error.severity = amp.validator.ValidationError.Severity.ERROR;
-      error.code = amp.validator.ValidationError.Code.MANDATORY_ATTR_MISSING;
-      error.detail =
-          'A11Y: ' + printElementRef(element) + ' must have "tabindex" ' +
-          'attribute due to "tap" action.';
-      validationResult.errors.push(error);
-      validationResult.status = amp.validator.ValidationResult.Status.FAIL;
-    }
-  }
-};
-
 /**
  * Validates doc in the browser by inspecting elements, attributes, etc. in
  * the DOM. This method is exported so it can be unittested.
@@ -110,10 +53,8 @@ amp.validator.validateTapActionsA11y = function(doc, validationResult) {
 amp.validator.validateInBrowser = function(doc) {
   const result = new amp.validator.ValidationResult();
   result.status = amp.validator.ValidationResult.Status.UNKNOWN;
-  amp.validator.validateTapActionsA11y(doc, result);
 
-  // If adding more in-browser validation functions, please add them here,
-  // much like validateTapActionsA11y.
+  // If adding in-browser validation functions, please add them here.
   // Note that result.status is set to UNKNOWN by default. If a routine
   // finds an error, then it should be set to 'FAIL'. Otherwise, it
   // should be left alone - that is, even for warnings it should be left

diff --git a/validator/validator-main.protoascii b/validator/validator-main.protoascii
@@ -25,7 +25,7 @@ min_validator_revision_required: 133
 # newer versions of the spec file. This is currently a Google internal
 # mechanism, validator.js does not use this facility. However, any
 # change to this file (validator-main.js) requires updating this revision id.
-spec_file_revision: 212
+spec_file_revision: 216
 
 # Validator extensions.
 # =====================
@@ -2002,6 +2002,7 @@ tags: {
   }
   spec_url: "https://www.ampproject.org/docs/reference/spec.html#required-markup"
 }
+# JSON Linked Data
 tags: {
   tag_name: "script"
   spec_name: "script type=application/ld+json"
@@ -2019,6 +2020,25 @@ tags: {
     }
   }
 }
+# Variant of the above, allowed in the body. See Github #2966.
+# TODO(gregable): Support repeated mandatory_parent perhaps.
+tags: {
+  tag_name: "script"
+  spec_name: "body > script type=application/ld+json"
+  mandatory_parent: "body"
+  attrs: {
+    name: "type"
+    mandatory: true
+    value: "application/ld+json"
+    dispatch_key: true
+  }
+  cdata: {
+    blacklisted_cdata_regex: {
+      regex: "<!--"
+      error_message: "html comments"
+    }
+  }
+}
 # Specific script tags for custom elements and runtime imports.
 # 4.11.2 The noscript element
 tags: {
@@ -2233,6 +2253,7 @@ tags: {  # <amp-video>
   attrs: { name: "muted"    value: "" }
   attrs: { name: "placeholder" }
   attrs: { name: "poster" }
+  attrs: { name: "preload" value_regex: "(none|metadata|auto|)" }
   attrs: {
     name: "src"
     value_url: {
@@ -2320,7 +2341,14 @@ attr_lists: {
   attrs: { name: "aria-valuemin" }
   attrs: { name: "aria-valuenow" }
   attrs: { name: "aria-valuetext" }
-  attrs: { name: "on" }
+  attrs: {
+    name: "on"
+    trigger: {
+      if_value_regex: "tap:.*"
+      also_requires_attr: "role"
+      also_requires_attr: "tabindex"
+    }
+  }
   attrs: { name: "role" }
   # For placeholders inside amp-ad and other tags, we allow the
   # placeholder empty attribute. See e.g.