Validator Roll Up

extensions/amp-subscriptions/0.1/test/validator-amp-subscriptions-errors.out

@@ -71,4 +71,4 @@ PASS
 |      Dialog for non logged in users
 |    </template>
 |  </body>
-|  </html>
+|  </html>

extensions/amp-subscriptions/validator-amp-subscriptions.protoascii

@@ -31,7 +31,7 @@ tags: {  # amp-subscriptions (json)
   spec_name: "amp-subscriptions extension .json script"
   unique: true
   mandatory_parent: "HEAD"
-  requires_extension: "amp-subscriptions"  
+  requires_extension: "amp-subscriptions"
   attrs: {
     name: "id"
     mandatory: true

validator/build.py

@@ -173,6 +173,7 @@ def GenValidatorProtoascii(out_dir):
   assert re.match(r'^[a-zA-Z_\-0-9]+$', out_dir), 'bad out_dir: %s' % out_dir
 
   protoascii_segments = [open('validator-main.protoascii').read()]
+  protoascii_segments.append(open('validator-css.protoascii').read())
   extensions = glob.glob('extensions/*/validator-*.protoascii')
   # In the Github project, the extensions are located in a sibling directory
   # to the validator rather than a child directory.

validator/engine/parse-css.js

@@ -31,6 +31,7 @@ goog.provide('parse_css.Stylesheet');
 goog.provide('parse_css.TokenStream');
 goog.provide('parse_css.extractAFunction');
 goog.provide('parse_css.extractASimpleBlock');
+goog.provide('parse_css.extractImportantDeclarations');
 goog.provide('parse_css.extractUrls');
 goog.provide('parse_css.parseAStylesheet');
 goog.provide('parse_css.parseInlineStyle');
@@ -346,6 +347,10 @@ parse_css.Declaration = class extends parse_css.Rule {
     this.value = [];
     /** @type {boolean} */
     this.important = false;
+    /** @type {number} */
+    this.important_line = -1;
+    /** @type {number} */
+    this.important_col = -1;
     /** @type {parse_css.TokenType} */
     this.tokenType = parse_css.TokenType.DECLARATION;
   }
@@ -711,7 +716,8 @@ class Canonicalizer {
     decl.value.push(tokenStream.next().copyPosTo(new parse_css.EOFToken()));
 
     let foundImportant = false;
-    for (let i = decl.value.length - 1; i >= 0; i--) {
+    // The last token is always EOF, so start at the 2nd to last token.
+    for (let i = decl.value.length - 2; i >= 0; i--) {
       if (decl.value[i].tokenType === parse_css.TokenType.WHITESPACE) {
         continue;
       } else if (
@@ -723,8 +729,11 @@ class Canonicalizer {
         foundImportant &&
           decl.value[i].tokenType === parse_css.TokenType.DELIM &&
         /** @type {parse_css.DelimToken} */ (decl.value[i]).value === '!') {
-        decl.value.splice(i, decl.value.length);
         decl.important = true;
+        decl.important_line = decl.value[i].line;
+        decl.important_col = decl.value[i].col;
+        // Delete !important and later, but not the EOF token
+        decl.value.splice(i, decl.value.length - i - 1);
         break;
       } else {
         break;
@@ -1067,6 +1076,29 @@ class UrlFunctionVisitor extends parse_css.RuleVisitor {
   }
 }
 
+/**
+ * Helper class for implementing parse_css::ExtractImportantProperties.
+ * Iterates over all declarations and returns pointers to declarations
+ * that were marked with `!important`.
+ * @private
+ */
+class ImportantPropertyVisitor extends parse_css.RuleVisitor {
+  /**
+   * @param {!Array<!parse_css.Declaration>} important
+   */
+  constructor(important) {
+    super();
+
+    /** @type {!Array<!parse_css.Declaration>} */
+    this.important = important;
+  }
+
+  /** @inheritDoc */
+  visitDeclaration(declaration) {
+    if (declaration.important) this.important.push(declaration);
+  }
+}
+
 /**
  * Extracts the URLs within the provided stylesheet, emitting them into
  * parsedUrls and errors into errors.
@@ -1085,6 +1117,17 @@ parse_css.extractUrls = function(stylesheet, parsedUrls, errors) {
   }
 };
 
+/**
+ * Extracts the declarations marked `!important` within within the provided
+ * stylesheet, emitting them into `important`.
+ * @param {!parse_css.Stylesheet} stylesheet
+ * @param {!Array<!parse_css.Declaration>} important
+ */
+parse_css.extractImportantDeclarations = function(stylesheet, important) {
+  const visitor = new ImportantPropertyVisitor(important);
+  stylesheet.accept(visitor);
+};
+
 /**
  * Helper class for implementing parse_css.parseMediaQueries.
  * @private

validator/engine/parse-css_test.js

@@ -970,6 +970,53 @@ describe('extractUrls', () => {
         parsedUrls);
   });
 
+  // This test verifies that a declaration ending in `!important` is treated by
+  // parsing out the `!important` marker and setting that as a special value in
+  // the declaration. In particular, we are looking for:
+  // 1) rules[0].declarations[0].important is true.
+  // 2) rules[0].declarations[0].value does not contain the tokens corresponding
+  //    to `!important`.
+  it('detects important', () => {
+    const css = 'b { color: red !important }';
+    const errors = [];
+    const tokenList = parse_css.tokenize(css, 1, 0, errors);
+    const sheet = parse_css.parseAStylesheet(
+        tokenList, ampAtRuleParsingSpec, parse_css.BlockType.PARSE_AS_IGNORE,
+        errors);
+    assertJSONEquals([], errors);
+    assertJSONEquals(
+        {
+          'line': 1,
+          'col': 0,
+          'tokenType': 'STYLESHEET',
+          'rules': [{
+            'line': 1,
+            'col': 0,
+            'tokenType': 'QUALIFIED_RULE',
+            'prelude': [
+              {'line': 1, 'col': 0, 'tokenType': 'IDENT', 'value': 'b'},
+              {'line': 1, 'col': 1, 'tokenType': 'WHITESPACE'},
+              {'line': 1, 'col': 2, 'tokenType': 'EOF_TOKEN'}
+            ],
+            'declarations': [{
+              'line': 1,
+              'col': 4,
+              'tokenType': 'DECLARATION',
+              'name': 'color',
+              'value': [
+                {'line': 1, 'col': 10, 'tokenType': 'WHITESPACE'},
+                {'line': 1, 'col': 11, 'tokenType': 'IDENT', 'value': 'red'},
+                {'line': 1, 'col': 14, 'tokenType': 'WHITESPACE'},
+                {'line': 1, 'col': 26, 'tokenType': 'EOF_TOKEN'}
+              ],
+              'important': true
+            }]
+          }],
+          'eof': {'line': 1, 'col': 27, 'tokenType': 'EOF_TOKEN'}
+        },
+        sheet);
+  });
+
   // This example contains both image urls, other urls (fonts) and
   // segments in between.
   it('handles longer example', () => {

validator/engine/validator.js

@@ -55,9 +55,11 @@ goog.require('goog.asserts');
 goog.require('goog.string');
 goog.require('goog.uri.utils');
 goog.require('parse_css.BlockType');
+goog.require('parse_css.Declaration');
 goog.require('parse_css.ErrorToken');
 goog.require('parse_css.ParsedCssUrl');
 goog.require('parse_css.RuleVisitor');
+goog.require('parse_css.extractImportantDeclarations');
 goog.require('parse_css.extractUrls');
 goog.require('parse_css.parseAStylesheet');
 goog.require('parse_css.parseInlineStyle');
@@ -2192,6 +2194,21 @@ class CdataMatcher {
           /* url */ '', validationResult);
     }
 
+    // If `!important` is not allowed, record instances as errors.
+    if (!cssSpec.allowImportant) {
+      /** @type {!Array<!parse_css.Declaration>} */
+      let important = [];
+      parse_css.extractImportantDeclarations(stylesheet, important);
+      for (const decl of important) {
+        context.addError(
+            amp.validator.ValidationError.Code.CDATA_VIOLATES_BLACKLIST,
+            new LineCol(decl.important_line, decl.important_col),
+            /* params */
+            [getTagSpecName(this.tagSpec_), 'CSS !important'],
+            getTagSpecUrl(this.tagSpec_), validationResult);
+      }
+    }
+
     const parsedFontUrlSpec = new ParsedUrlSpec(cssSpec.fontUrlSpec);
     const parsedImageUrlSpec = new ParsedUrlSpec(cssSpec.imageUrlSpec);
     for (const url of parsedUrls) {

validator/testdata/feature_tests/empty.out

@@ -13,12 +13,12 @@ feature_tests/empty.html:1:0 The mandatory tag 'meta charset=utf-8' is missing o
 >> ^~~~~~~~~
 feature_tests/empty.html:1:0 The mandatory tag 'meta name=viewport' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
 >> ^~~~~~~~~
-feature_tests/empty.html:1:0 The mandatory tag 'head > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
->> ^~~~~~~~~
-feature_tests/empty.html:1:0 The mandatory tag 'noscript > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
->> ^~~~~~~~~
 feature_tests/empty.html:1:0 The mandatory tag 'body' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
 >> ^~~~~~~~~
 feature_tests/empty.html:1:0 The mandatory tag 'noscript enclosure for boilerplate' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
 >> ^~~~~~~~~
+feature_tests/empty.html:1:0 The mandatory tag 'head > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
+>> ^~~~~~~~~
+feature_tests/empty.html:1:0 The mandatory tag 'noscript > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
+>> ^~~~~~~~~
 feature_tests/empty.html:1:0 The mandatory tag 'amphtml engine v0.js script' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)

validator/testdata/feature_tests/incorrect_custom_style.html

@@ -31,6 +31,8 @@
     foo.amp-class {}
     foo { b: red !important; }
     foo { b: red ! important; }
+    /* This is ok though, since it's a comment: { b: red !important } */
+
     @viewport (mumble mumble) { }
     @media (whatever) { @notallowednested }
 

validator/testdata/feature_tests/incorrect_custom_style.out

@@ -26,28 +26,32 @@ FAIL
 |    <link rel="canonical" href="./regular-html-version.html">
 |    <meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1,maximum-scale=1,user-scalable=no,minimal-ui">
 |    <style amp-custom>
->>   ^~~~~~~~~
-feature_tests/incorrect_custom_style.html:27:2 The text inside tag 'style amp-custom' contains 'CSS !important', which is disallowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
 |      /* These CSS Rules violate essential CSS validation */
 |      @import url("http://somewhere/on/the/internet.css");
 >>     ^~~~~~~~~
 feature_tests/incorrect_custom_style.html:29:4 CSS syntax error in tag 'style amp-custom' - saw invalid at rule '@import'.
 |      foo.i-amp-class {}
 |      foo.amp-class {}
 |      foo { b: red !important; }
+>>                  ^~~~~~~~~
+feature_tests/incorrect_custom_style.html:32:17 The text inside tag 'style amp-custom' contains 'CSS !important', which is disallowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
 |      foo { b: red ! important; }
+>>                  ^~~~~~~~~
+feature_tests/incorrect_custom_style.html:33:17 The text inside tag 'style amp-custom' contains 'CSS !important', which is disallowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
+|      /* This is ok though, since it's a comment: { b: red !important } */
+|
 |      @viewport (mumble mumble) { }
 >>     ^~~~~~~~~
-feature_tests/incorrect_custom_style.html:34:4 CSS syntax error in tag 'style amp-custom' - saw invalid at rule '@viewport'.
+feature_tests/incorrect_custom_style.html:36:4 CSS syntax error in tag 'style amp-custom' - saw invalid at rule '@viewport'.
 |      @media (whatever) { @notallowednested }
 >>                         ^~~~~~~~~
-feature_tests/incorrect_custom_style.html:35:24 CSS syntax error in tag 'style amp-custom' - saw invalid at rule '@notallowednested'.
+feature_tests/incorrect_custom_style.html:37:24 CSS syntax error in tag 'style amp-custom' - saw invalid at rule '@notallowednested'.
 |
 |      /* some tests for url verification - images */
 |      foo { background-image: url('') }  /* allowed for now */
 |      foo { background-image: url('invalid://invalid.com/1.jpg') }
 >>                             ^~~~~~~~~
-feature_tests/incorrect_custom_style.html:39:28 CSS syntax error in tag 'style amp-custom' - invalid url protocol 'invalid:'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
+feature_tests/incorrect_custom_style.html:41:28 CSS syntax error in tag 'style amp-custom' - invalid url protocol 'invalid:'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
 |      foo { background-image: url('https://valid.com/1.jpg') }
 |      foo { background-image: url('http://valid.com/1.jpg') }
 |      foo { background-image: url('absolute://disallow.com/soon.jpg') }
@@ -58,12 +62,12 @@ feature_tests/incorrect_custom_style.html:39:28 CSS syntax error in tag 'style a
 |      @font-face { src: url(''); } /* allowed for now */
 |      @font-face { src: url('invalid://invalid.com/1.ttf') }
 >>                       ^~~~~~~~~
-feature_tests/incorrect_custom_style.html:48:22 CSS syntax error in tag 'style amp-custom' - invalid url protocol 'invalid:'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
+feature_tests/incorrect_custom_style.html:50:22 CSS syntax error in tag 'style amp-custom' - invalid url protocol 'invalid:'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
 |      @font-face { src: url('https://valid.com/1.ttf') }
 |      @font-face { src: url('http://valid.com/1.ttf') }
 |      @font-face { src: url('absolute://invalid.com/1.ttf') }
 >>                       ^~~~~~~~~
-feature_tests/incorrect_custom_style.html:51:22 CSS syntax error in tag 'style amp-custom' - invalid url protocol 'absolute:'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
+feature_tests/incorrect_custom_style.html:53:22 CSS syntax error in tag 'style amp-custom' - invalid url protocol 'absolute:'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
 |      @font-face { src: url('://valid.ttf') }
 |      @font-face { src: url('valid.ttf') }
 |    </style>

validator/testdata/feature_tests/multiple_body_tags_3.out

@@ -51,6 +51,6 @@ feature_tests/multiple_body_tags_3.html:34:2 The parent tag of tag 'amphtml engi
 |  </body>
 |  </html>
 >>       ^~~~~~~~~
-feature_tests/multiple_body_tags_3.html:41:6 The mandatory tag 'head > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
->>       ^~~~~~~~~
 feature_tests/multiple_body_tags_3.html:41:6 The mandatory tag 'noscript enclosure for boilerplate' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
+>>       ^~~~~~~~~
+feature_tests/multiple_body_tags_3.html:41:6 The mandatory tag 'head > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)

validator/testdata/feature_tests/not_amp.out

@@ -34,10 +34,10 @@ feature_tests/not_amp.html:28:6 The mandatory tag 'link rel=canonical' is missin
 >>       ^~~~~~~~~
 feature_tests/not_amp.html:28:6 The mandatory tag 'meta name=viewport' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
 >>       ^~~~~~~~~
+feature_tests/not_amp.html:28:6 The mandatory tag 'noscript enclosure for boilerplate' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
+>>       ^~~~~~~~~
 feature_tests/not_amp.html:28:6 The mandatory tag 'head > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
 >>       ^~~~~~~~~
 feature_tests/not_amp.html:28:6 The mandatory tag 'noscript > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
 >>       ^~~~~~~~~
-feature_tests/not_amp.html:28:6 The mandatory tag 'noscript enclosure for boilerplate' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
->>       ^~~~~~~~~
 feature_tests/not_amp.html:28:6 The mandatory tag 'amphtml engine v0.js script' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)

validator/testdata/feature_tests/unprintable_chars.out

@@ -63,6 +63,6 @@ feature_tests/unprintable_chars.html:36:2 The parent tag of tag 'amphtml engine
 >>       ^~~~~~~~~
 feature_tests/unprintable_chars.html:41:6 The mandatory tag 'html doctype' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
 >>       ^~~~~~~~~
-feature_tests/unprintable_chars.html:41:6 The mandatory tag 'head > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
->>       ^~~~~~~~~
 feature_tests/unprintable_chars.html:41:6 The mandatory tag 'noscript enclosure for boilerplate' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
+>>       ^~~~~~~~~
+feature_tests/unprintable_chars.html:41:6 The mandatory tag 'head > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)

validator/testdata/transformed_feature_tests/transformed_but_not_identified_transformed.out

@@ -49,8 +49,8 @@ transformed_feature_tests/transformed_but_not_identified_transformed.html:32:4 T
 |  </body>
 |  </html>
 >>       ^~~~~~~~~
+transformed_feature_tests/transformed_but_not_identified_transformed.html:35:6 The mandatory tag 'noscript enclosure for boilerplate' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
+>>       ^~~~~~~~~
 transformed_feature_tests/transformed_but_not_identified_transformed.html:35:6 The mandatory tag 'head > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
 >>       ^~~~~~~~~
 transformed_feature_tests/transformed_but_not_identified_transformed.html:35:6 The mandatory tag 'noscript > style[amp-boilerplate]' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)
->>       ^~~~~~~~~
-transformed_feature_tests/transformed_but_not_identified_transformed.html:35:6 The mandatory tag 'noscript enclosure for boilerplate' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate?format=websites)