Validator Roll-up (#2903)

* Rename AMP_MUSTACHE_TEMPLATE_PROBLEM -> AMP_HTML_TEMPLATE_PROBLEM. * Allow filtering by error category. * Relax validation of mustache template layout attributes. * Update `<amp-sidebar>` to rename `direction` attr to `side` and require `nodisplay` layout.
ampproject · Apr 14, 2016 · 68e287b · 68e287b
1 parent 3181cbb
commit 68e287b
Show file tree

Hide file tree

Showing 9 changed files with 106 additions and 59 deletions.
diff --git a/extensions/amp-mustache/0.1/test/validator-amp-mustache.html b/extensions/amp-mustache/0.1/test/validator-amp-mustache.html
@@ -108,5 +108,14 @@
 <template type="amp-mustache">
 <amp-audio src="https://example.com/audio" layout="fixed" autoplay="{{valid}}">
 </template>
+
+<!-- Since layout calculations follow a different code path, test that layouts
+     validate. -->
+<!-- See https://github.com/ampproject/amphtml/issues/2670 -->
+<template type="amp-mustache">
+<amp-img src="{{image.url}}" width={{image.width}} height={{image.height}}></amp-img>
+</template>
+
+
 </body>
 </html>
diff --git a/extensions/amp-mustache/0.1/test/validator-amp-mustache.out b/extensions/amp-mustache/0.1/test/validator-amp-mustache.out
diff --git a/extensions/amp-sidebar/0.1/test/validator-amp-sidebar.html b/extensions/amp-sidebar/0.1/test/validator-amp-sidebar.html
@@ -32,7 +32,7 @@
 </head>
 <body>
   <!-- Example of a valid amp-sidebar. -->
-  <amp-sidebar direction="left">
+  <amp-sidebar side="left" layout="nodisplay">
     <amp-accordion>
       <section>
         <h2>Section 1</h2>
@@ -47,10 +47,10 @@ <h2>Section 2</h2>
     <amp-img layout="fill" src="img"></amp-img>
   </amp-sidebar>
   <!-- Invalid example: more than one amp-sidebar and invalid children. -->
-  <amp-sidebar>
+  <amp-sidebar layout="nodisplay">
     <amp-ad layout="fill" src="https://ad" type="ad"></amp-ad>
   </amp-sidebar>
-  <!-- Invalid example with invalid direction value. -->
-  <amp-sidebar direction="center"></amp-sidebar>
+  <!-- Invalid example with invalid side value. -->
+  <amp-sidebar side="center" layout="nodisplay"></amp-sidebar>
 </body>
 </html>
diff --git a/extensions/amp-sidebar/0.1/test/validator-amp-sidebar.out b/extensions/amp-sidebar/0.1/test/validator-amp-sidebar.out
@@ -1,4 +1,4 @@
 FAIL
-amp-sidebar/0.1/test/validator-amp-sidebar.html:50:2 The tag 'amp-sidebar' appears more than once in the document. [MANDATORY_AMP_TAG_MISSING_OR_INCORRECT]
+amp-sidebar/0.1/test/validator-amp-sidebar.html:50:2 The tag 'amp-sidebar' appears more than once in the document. (see https://github.com/ampproject/amphtml/blob/master/extensions/amp-sidebar/amp-sidebar.md) [MANDATORY_AMP_TAG_MISSING_OR_INCORRECT]
 amp-sidebar/0.1/test/validator-amp-sidebar.html:51:4 The tag 'amp-ad' may not appear as a descendant of tag 'amp-sidebar'. (see https://www.ampproject.org/docs/reference/amp-ad.html) [AMP_TAG_PROBLEM]
-amp-sidebar/0.1/test/validator-amp-sidebar.html:54:2 The attribute 'direction' in tag 'amp-sidebar' is set to the invalid value 'center'. [AMP_TAG_PROBLEM]
+amp-sidebar/0.1/test/validator-amp-sidebar.html:54:2 The attribute 'side' in tag 'amp-sidebar' is set to the invalid value 'center'. (see https://github.com/ampproject/amphtml/blob/master/extensions/amp-sidebar/amp-sidebar.md) [AMP_TAG_PROBLEM]
diff --git a/extensions/amp-sidebar/0.1/validator-amp-sidebar.protoascii b/extensions/amp-sidebar/0.1/validator-amp-sidebar.protoascii
@@ -44,8 +44,7 @@ tags: {  # amp-sidebar
       error_message: "contents"
     }
   }
-  # Uncomment once reference is available.
-  # spec_url: "https://www.ampproject.org/docs/reference/extended/amp-sidebar.html"
+  spec_url: "https://www.ampproject.org/docs/reference/extended/amp-sidebar.html"
 }
 tags: {  # <amp-sidebar>
   tag_name: "amp-sidebar"
@@ -55,13 +54,12 @@ tags: {  # <amp-sidebar>
   disallowed_ancestor: "amp-sidebar"
   also_requires: "amp-sidebar extension .js script"
   attrs: {
-    name: "direction"
+    name: "side"
     value_regex: "(left|right)"
   }
   attr_lists: "extended-amp-global"
-  # Uncomment once reference is available.
-  # spec_url: "https://github.com/ampproject/amphtml/blob/master/extensions/amp-sidebar/amp-sidebar.md"
+  spec_url: "https://github.com/ampproject/amphtml/blob/master/extensions/amp-sidebar/amp-sidebar.md"
   amp_layout: {
-    supported_layouts: CONTAINER
+    supported_layouts: NODISPLAY
   }
 }
diff --git a/validator/validator-in-browser.js b/validator/validator-in-browser.js
@@ -129,9 +129,11 @@ amp.validator.validateInBrowser = function(doc) {
  * and https://github.com/ampproject/amphtml/blob/master/src/validator-integration.js
  * @param {string} url
  * @param {!Document=} opt_doc
+ * @param {!string=} opt_errorCategoryFilter
  * @export
  */
-amp.validator.validateUrlAndLog = function(url, opt_doc) {
+amp.validator.validateUrlAndLog = function(
+    url, opt_doc, opt_errorCategoryFilter) {
   getUrl(url).then(
     function(html) {  // Success
       const validationResult = amp.validator.validateString(html);

diff --git a/validator/validator-main.protoascii b/validator/validator-main.protoascii
@@ -25,7 +25,7 @@ min_validator_revision_required: 112
 # 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: 207
+spec_file_revision: 208
 
 # Validator extensions.
 # =====================

diff --git a/validator/validator.js b/validator/validator.js
@@ -331,21 +331,46 @@ amp.validator.Terminal = class {
  *   errors.
  * @param {string} url
  * @param {!amp.validator.Terminal=} opt_terminal
+ * @param {!string=} opt_errorCategoryFilter
  */
 amp.validator.ValidationResult.prototype.outputToTerminal =
-    function(url, opt_terminal) {
+    function(url, opt_terminal, opt_errorCategoryFilter) {
 
   const terminal = opt_terminal || new amp.validator.Terminal();
+  const errorCategoryFilter = opt_errorCategoryFilter || null;
+
   const status = this.status;
   if (status === amp.validator.ValidationResult.Status.PASS) {
     terminal.info('AMP validation successful.');
-  } else if (status === amp.validator.ValidationResult.Status.FAIL) {
-    terminal.error('AMP validation had errors:');
-  } else {
+    return;
+  }
+  if (status !== amp.validator.ValidationResult.Status.FAIL) {
     terminal.error(
         'AMP validation had unknown results. This should not happen.');
+    return;
+  }
+  let errors;
+  if (errorCategoryFilter === null) {
+    terminal.error('AMP validation had errors:');
+    errors = this.errors;
+  } else {
+    errors = [];
+    for (const error of this.errors) {
+      if (amp.validator.categorizeError(error) === errorCategoryFilter) {
+        errors.push(error);
+      }
+    }
+    if (errors.length === 0) {
+      terminal.error('AMP validation - no errors matching ' +
+          'error_category_filter=' + errorCategoryFilter + ' found. ' +
+          'To see all errors, visit ' + url + '#development=1');
+    } else {
+      terminal.error('AMP validation - displaying errors matching ' +
+          'error_category_filter=' + errorCategoryFilter + '. ' +
+          'To see all errors, visit ' + url + '#development=1');
+    }
   }
-  for (const error of this.errors) {
+  for (const error of errors) {
     if (error.severity ===
         amp.validator.ValidationError.Severity.ERROR) {
       terminal.error(errorLine(url, error));
@@ -2056,6 +2081,17 @@ class ParsedTagSpec {
     const sizesAttr = attrsByKey.get('sizes');
     const heightsAttr = attrsByKey.get('heights');
 
+    // We disable validating layout for tags where one of the layout attributes
+    // contains mustache syntax.
+    const hasTemplateAncestor = context.getTagStack().hasAncestor('template');
+    if (hasTemplateAncestor &&
+        (ParsedTagSpec.valueHasTemplateSyntax(layoutAttr) ||
+         ParsedTagSpec.valueHasTemplateSyntax(widthAttr) ||
+         ParsedTagSpec.valueHasTemplateSyntax(heightAttr) ||
+         ParsedTagSpec.valueHasTemplateSyntax(sizesAttr) ||
+         ParsedTagSpec.valueHasTemplateSyntax(heightsAttr)))
+      return;
+
     // Parse the input layout attributes which we found for this tag.
     const inputLayout = parseLayout(layoutAttr);
     if (layoutAttr !== undefined &&
@@ -3347,7 +3383,7 @@ amp.validator.categorizeError = function(error) {
       TEMPLATE_PARTIAL_IN_ATTR_VALUE ||
       error.code === amp.validator.ValidationError.Code.
       TEMPLATE_IN_ATTR_NAME) {
-    return amp.validator.ErrorCategory.Code.AMP_MUSTACHE_TEMPLATE_PROBLEM;
+    return amp.validator.ErrorCategory.Code.AMP_HTML_TEMPLATE_PROBLEM;
   }
   // E.g. "The tag 'amp-ad' may not appear as a descendant of tag 'amp-sidebar'.
   if (error.code === amp.validator.ValidationError.Code
@@ -3358,7 +3394,7 @@ amp.validator.categorizeError = function(error) {
   if (error.code === amp.validator.ValidationError.Code
       .DISALLOWED_TAG_ANCESTOR &&
       (error.params[1] === "template")) {
-    return amp.validator.ErrorCategory.Code.AMP_MUSTACHE_TEMPLATE_PROBLEM;
+    return amp.validator.ErrorCategory.Code.AMP_HTML_TEMPLATE_PROBLEM;
   }
   // E.g. "Missing URL for attribute 'href' in tag 'a'."
   // E.g. "Invalid URL protocol 'http:' for attribute 'src' in tag

diff --git a/validator/validator.proto b/validator/validator.proto
@@ -351,9 +351,11 @@ message ErrorCategory {
     // a given AMP element, and responsiveness.
     // https://github.com/ampproject/amphtml/blob/master/spec/amp-html-layout.md
     AMP_LAYOUT_PROBLEM = 8;
-    // AMP supports Mustache templates; the validator generates errors
-    // for misplaced template syntax.
-    AMP_MUSTACHE_TEMPLATE_PROBLEM = 9;
+    // AMP supports the HTML template tag (e.g. within amp-list) and interprets
+    // Mustache syntax within such templates. To help with debugging, the AMP
+    // Validator generates errors for misplaced Mustache template syntax.
+    // https://github.com/ampproject/amphtml/blob/master/spec/amp-html-templates.md
+    AMP_HTML_TEMPLATE_PROBLEM = 9;
     // This is currently still allowed but may soon be disallowed. Usually
     // a recommendation for a replacement is rendered as part of the error.
     DEPRECATION = 10;