Validator rollup (#32548)

* cl/355665213 Revision bump for #32371 * cl/355686523 Revision bump for #32315 * cl/355888443 Allow any declaration for inline style except in AMP4EMAIL with data-css-strict * cl/355944766 Allow module/nomodule in validator Co-authored-by: honeybadgerdontcare <sedano@google.com>
ampproject · Feb 9, 2021 · 7d026fe · 7d026fe
1 parent d86f7a8
commit 7d026fe
Show file tree

Hide file tree

Showing 38 changed files with 3,967 additions and 2,866 deletions.
diff --git a/validator/js/engine/validator.js b/validator/js/engine/validator.js
@@ -2563,7 +2563,7 @@ class CdataMatcher {
 
     // Validate the allowed CSS declarations (eg: `background-color`)
     if (maybeDocCssSpec !== null &&
-        !maybeDocCssSpec.spec().allowAllDeclarationInStyleTag) {
+        !maybeDocCssSpec.spec().allowAllDeclarationInStyle) {
       const invalidDeclVisitor = new InvalidDeclVisitor(
           maybeDocCssSpec, context, getTagDescriptiveName(this.tagSpec_),
           validationResult);
@@ -4903,30 +4903,30 @@ function validateAttrCss(
   }
 
   /** @type {?ParsedDocCssSpec} */
-  const maybeSpec = context.matchingDocCssSpec();
-  if (maybeSpec) {
+  const maybeDocCssSpec = context.matchingDocCssSpec();
+  if (maybeDocCssSpec !== null) {
     // Determine if we've exceeded the maximum bytes per inline style
     // requirements.
-    if (maybeSpec.spec().maxBytesPerInlineStyle >= 0 &&
-        attrByteLen > maybeSpec.spec().maxBytesPerInlineStyle) {
-      if (maybeSpec.spec().maxBytesIsWarning) {
+    if (maybeDocCssSpec.spec().maxBytesPerInlineStyle >= 0 &&
+        attrByteLen > maybeDocCssSpec.spec().maxBytesPerInlineStyle) {
+      if (maybeDocCssSpec.spec().maxBytesIsWarning) {
         context.addWarning(
             generated.ValidationError.Code.INLINE_STYLE_TOO_LONG,
             context.getLineCol(), /* params */
             [
               getTagDescriptiveName(tagSpec), attrByteLen.toString(),
-              maybeSpec.spec().maxBytesPerInlineStyle.toString()
+              maybeDocCssSpec.spec().maxBytesPerInlineStyle.toString()
             ],
-            maybeSpec.spec().maxBytesSpecUrl, result.validationResult);
+            maybeDocCssSpec.spec().maxBytesSpecUrl, result.validationResult);
       } else {
         context.addError(
             generated.ValidationError.Code.INLINE_STYLE_TOO_LONG,
             context.getLineCol(), /* params */
             [
               getTagDescriptiveName(tagSpec), attrByteLen.toString(),
-              maybeSpec.spec().maxBytesPerInlineStyle.toString()
+              maybeDocCssSpec.spec().maxBytesPerInlineStyle.toString()
             ],
-            maybeSpec.spec().maxBytesSpecUrl, result.validationResult);
+            maybeDocCssSpec.spec().maxBytesSpecUrl, result.validationResult);
       }
     }
 
@@ -4935,51 +4935,64 @@ function validateAttrCss(
     // relevant.
     for (const declaration of declarations) {
       const firstIdent = declaration.firstIdent();
-      // Allowed declarations vary by context. SVG has its own set of CSS
-      // declarations not supported generally in HTML.
-      const cssDeclaration = parsedAttrSpec.getSpec().valueDocSvgCss === true ?
-          maybeSpec.cssDeclarationSvgByName(declaration.name) :
-          maybeSpec.cssDeclarationByName(declaration.name);
-      // If there is no matching declaration in the rules, then this
-      // declaration is not allowed.
-      if (cssDeclaration === null) {
+      // Validate declarations only when they are not all allowed.
+      if (!maybeDocCssSpec.spec().allowAllDeclarationInStyle) {
+        // Allowed declarations vary by context. SVG has its own set of CSS
+        // declarations not supported generally in HTML.
+        const cssDeclaration =
+            parsedAttrSpec.getSpec().valueDocSvgCss === true ?
+            maybeDocCssSpec.cssDeclarationSvgByName(declaration.name) :
+            maybeDocCssSpec.cssDeclarationByName(declaration.name);
+        // If there is no matching declaration in the rules, then this
+        // declaration is not allowed.
+        if (cssDeclaration === null) {
+          context.addError(
+              generated.ValidationError.Code.DISALLOWED_PROPERTY_IN_ATTR_VALUE,
+              context.getLineCol(), /* params */
+              [declaration.name, attrName, getTagDescriptiveName(tagSpec)],
+              context.getRules().getStylesSpecUrl(), result.validationResult);
+          // Don't emit additional errors for this declaration.
+          continue;
+        } else if (cssDeclaration.valueCasei.length > 0) {
+          let hasValidValue = false;
+          for (const value of cssDeclaration.valueCasei) {
+            if (firstIdent.toLowerCase() == value) {
+              hasValidValue = true;
+              break;
+            }
+          }
+          if (!hasValidValue) {
+            // Declaration value not allowed.
+            context.addError(
+                generated.ValidationError.Code
+                    .CSS_SYNTAX_DISALLOWED_PROPERTY_VALUE,
+                context.getLineCol(), /* params */
+                [getTagDescriptiveName(tagSpec), declaration.name, firstIdent],
+                context.getRules().getStylesSpecUrl(), result.validationResult);
+          }
+        } else if (cssDeclaration.valueRegexCasei != null) {
+          const valueRegex = context.getRules().getFullMatchCaseiRegex(
+              /** @type {number} */ (cssDeclaration.valueRegexCasei));
+          if (!valueRegex.test(firstIdent)) {
+            context.addError(
+                generated.ValidationError.Code
+                    .CSS_SYNTAX_DISALLOWED_PROPERTY_VALUE,
+                context.getLineCol(), /* params */
+                [getTagDescriptiveName(tagSpec), declaration.name, firstIdent],
+                context.getRules().getStylesSpecUrl(), result.validationResult);
+          }
+        }
+      }
+      if (declaration.name.indexOf('i-amphtml-') > -1) {
         context.addError(
             generated.ValidationError.Code.DISALLOWED_PROPERTY_IN_ATTR_VALUE,
             context.getLineCol(), /* params */
             [declaration.name, attrName, getTagDescriptiveName(tagSpec)],
             context.getRules().getStylesSpecUrl(), result.validationResult);
         // Don't emit additional errors for this declaration.
         continue;
-      } else if (cssDeclaration.valueCasei.length > 0) {
-        let hasValidValue = false;
-        for (const value of cssDeclaration.valueCasei) {
-          if (firstIdent.toLowerCase() == value) {
-            hasValidValue = true;
-            break;
-          }
-        }
-        if (!hasValidValue) {
-          // Declaration value not allowed.
-          context.addError(
-              generated.ValidationError.Code
-                  .CSS_SYNTAX_DISALLOWED_PROPERTY_VALUE,
-              context.getLineCol(), /* params */
-              [getTagDescriptiveName(tagSpec), declaration.name, firstIdent],
-              context.getRules().getStylesSpecUrl(), result.validationResult);
-        }
-      } else if (cssDeclaration.valueRegexCasei != null) {
-        const valueRegex = context.getRules().getFullMatchCaseiRegex(
-            /** @type {number} */ (cssDeclaration.valueRegexCasei));
-        if (!valueRegex.test(firstIdent)) {
-          context.addError(
-              generated.ValidationError.Code
-                  .CSS_SYNTAX_DISALLOWED_PROPERTY_VALUE,
-              context.getLineCol(), /* params */
-              [getTagDescriptiveName(tagSpec), declaration.name, firstIdent],
-              context.getRules().getStylesSpecUrl(), result.validationResult);
-        }
       }
-      if (!maybeSpec.spec().allowImportant) {
+      if (!maybeDocCssSpec.spec().allowImportant) {
         if (declaration.important)
           context.addError(
               generated.ValidationError.Code.CSS_SYNTAX_DISALLOWED_IMPORTANT,
@@ -5005,16 +5018,16 @@ function validateAttrCss(
         // Validate that the URL itself matches the spec.
         // Only image specs apply to inline styles. Fonts are only defined in
         // @font-face rules which we require a full stylesheet to define.
-        if (maybeSpec.spec().imageUrlSpec !== null) {
+        if (maybeDocCssSpec.spec().imageUrlSpec !== null) {
           const adapter = new UrlErrorInStylesheetAdapter(
               context.getLineCol().getLine(), context.getLineCol().getCol());
           validateUrlAndProtocol(
-              maybeSpec.imageUrlSpec(), adapter, context, url.utf8Url, tagSpec,
-              result.validationResult);
+              maybeDocCssSpec.imageUrlSpec(), adapter, context, url.utf8Url,
+              tagSpec, result.validationResult);
         }
         // Subtract off URL lengths from doc-level inline style bytes, if
         // specified by the DocCssSpec.
-        if (!maybeSpec.spec().urlBytesIncluded && !isDataUrl(url.utf8Url))
+        if (!maybeDocCssSpec.spec().urlBytesIncluded && !isDataUrl(url.utf8Url))
           result.inlineStyleCssBytes -= htmlparser.byteLength(url.utf8Url);
       }
     }

diff --git a/validator/testdata/amp4ads_feature_tests/script_release_versions.html b/validator/testdata/amp4ads_feature_tests/script_release_versions.html
@@ -17,10 +17,6 @@
   Test Description:
   This test demonstrates that module/nomodule, modolue/nomodule LTS, and module
   SXG script tags are not valid for AMP4ADS.
-
-  TODO(b/173803451) some of the test comments below may be incorrect due
-  to module/nomodule not being allowed yet. They will be correct once
-  module/nomodule is allowed.
 -->
 <!doctype html>
 <html ⚡4ads transformed="google;v=1">

diff --git a/validator/testdata/amp4ads_feature_tests/script_release_versions.out b/validator/testdata/amp4ads_feature_tests/script_release_versions.out
@@ -18,15 +18,11 @@ FAIL
 |    Test Description:
 |    This test demonstrates that module/nomodule, modolue/nomodule LTS, and module
 |    SXG script tags are not valid for AMP4ADS.
-|
-|    TODO(b/173803451) some of the test comments below may be incorrect due
-|    to module/nomodule not being allowed yet. They will be correct once
-|    module/nomodule is allowed.
 |  -->
 |  <!doctype html>
 |  <html ⚡4ads transformed="google;v=1">
 >> ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:26:0 The attribute 'transformed' may not appear in tag 'html'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
+amp4ads_feature_tests/script_release_versions.html:22:0 The attribute 'transformed' may not appear in tag 'html'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
 |  <head>
 |    <meta charset=utf-8>
 |    <meta name="viewport" content="width=device-width,minimum-scale=1">
@@ -35,38 +31,38 @@ amp4ads_feature_tests/script_release_versions.html:26:0 The attribute 'transform
 |    <!-- Invalid: AMP4ADS module-nomodule pair for AMP runtime script -->
 |    <script async crossorigin=anonymous src=https://cdn.ampproject.org/v0.mjs type=module></script>
 >>   ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:33:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/validation-workflow/validation_errors/#custom-javascript-is-not-allowed)
+amp4ads_feature_tests/script_release_versions.html:29:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/validation-workflow/validation_errors/#custom-javascript-is-not-allowed)
 |    <script async nomodule src=https://cdn.ampproject.org/v0.js></script>
 >>   ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:34:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/validation-workflow/validation_errors/#custom-javascript-is-not-allowed)
+amp4ads_feature_tests/script_release_versions.html:30:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/validation-workflow/validation_errors/#custom-javascript-is-not-allowed)
 |
 |    <!-- Invalid: AMP4ADS module/nomodule pair for AMP4ADS runtime script -->
 |    <script async crossorigin=anonymous src=https://cdn.ampproject.org/amp4ads-v0.mjs type=module></script>
 >>   ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:37:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/validation-workflow/validation_errors/#custom-javascript-is-not-allowed)
+amp4ads_feature_tests/script_release_versions.html:33:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/validation-workflow/validation_errors/#custom-javascript-is-not-allowed)
 |    <script async nomodule src=https://cdn.ampproject.org/amp4ads-v0.js></script>
 >>   ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:38:2 The attribute 'nomodule' may not appear in tag 'amphtml engine script'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml/#required-markup)
+amp4ads_feature_tests/script_release_versions.html:34:2 The attribute 'nomodule' may not appear in tag 'amphtml engine script'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml/#required-markup)
 |
 |    <!-- Invalid: AMP4ADS module/nomodule pair for AMP extension script -->
 |    <script async crossorigin=anonymous custom-element=amp-analytics src=https://cdn.ampproject.org/v0/amp-analytics-0.1.mjs type=module></script>
 >>   ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:41:2 The attribute 'type' in tag 'amp-analytics extension script' is set to the invalid value 'module'. (see https://amp.dev/documentation/components/amp-analytics)
+amp4ads_feature_tests/script_release_versions.html:37:2 The attribute 'type' in tag 'amp-analytics extension script' is set to the invalid value 'module'. (see https://amp.dev/documentation/components/amp-analytics)
 |    <script async custom-element=amp-analytics nomodule src=https://cdn.ampproject.org/v0/amp-analytics-0.1.js></script>
 >>   ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:42:2 The attribute 'nomodule' may not appear in tag 'amp-analytics extension script'. (see https://amp.dev/documentation/components/amp-analytics)
+amp4ads_feature_tests/script_release_versions.html:38:2 The attribute 'nomodule' may not appear in tag 'amp-analytics extension script'. (see https://amp.dev/documentation/components/amp-analytics)
 |
 |    <!-- Invalid: AMP4ADS module/nomodule LTS pair for AMP extension script -->
 |    <script async crossorigin=anonymous custom-element=amp-audio src=https://cdn.ampproject.org/lts/v0/amp-audio-0.1.mjs type=module></script>
 >>   ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:45:2 The script version for 'amp-audio' is a module/nomodule LTS version which mismatches with the first script on the page using the module/nomodule version. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
+amp4ads_feature_tests/script_release_versions.html:41:2 The script version for 'amp-audio' is a module/nomodule LTS version which mismatches with the first script on the page using the module/nomodule version. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
 >>   ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:45:2 The attribute 'type' in tag 'amp-audio extension script' is set to the invalid value 'module'. (see https://amp.dev/documentation/components/amp-audio)
+amp4ads_feature_tests/script_release_versions.html:41:2 The attribute 'type' in tag 'amp-audio extension script' is set to the invalid value 'module'. (see https://amp.dev/documentation/components/amp-audio)
 |    <script async custom-element=amp-audio nomodule src=https://cdn.ampproject.org/lts/v0/amp-audio-0.1.js></script>
 >>   ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:46:2 The attribute 'nomodule' may not appear in tag 'amp-audio extension script'. (see https://amp.dev/documentation/components/amp-audio)
+amp4ads_feature_tests/script_release_versions.html:42:2 The attribute 'nomodule' may not appear in tag 'amp-audio extension script'. (see https://amp.dev/documentation/components/amp-audio)
 >>   ^~~~~~~~~
-amp4ads_feature_tests/script_release_versions.html:46:2 The script version for 'amp-audio' is a module/nomodule LTS version which mismatches with the first script on the page using the module/nomodule version. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
+amp4ads_feature_tests/script_release_versions.html:42:2 The script version for 'amp-audio' is a module/nomodule LTS version which mismatches with the first script on the page using the module/nomodule version. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
 |  </head>
 |  <body>
 |    Hello, world.