Validator Roll Up (#27412)

* cl/302030563 Revision bump for #27280 * cl/302033641 Remove trailing whitespace from protoascii * cl/302035592 Revision bump for #27159 * cl/302061262 github commit msg missing or malformed * revert * revert * revert * revert * revert * trying to kickstart travis Co-authored-by: Greg Grothaus <greggrothaus@google.com>
ampproject · Mar 25, 2020 · b9ddc1d · b9ddc1d
1 parent 6f90ee3
commit b9ddc1d
Show file tree

Hide file tree

Showing 17 changed files with 963 additions and 822 deletions.
diff --git a/extensions/amp-subscriptions/0.1/test/validator-amp-subscriptions-errors.out b/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>
diff --git a/extensions/amp-subscriptions/validator-amp-subscriptions.protoascii b/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

diff --git a/validator/build.py b/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.

diff --git a/validator/engine/parse-css.js b/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

diff --git a/validator/engine/parse-css_test.js b/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', () => {

diff --git a/validator/engine/validator.js b/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) {

diff --git a/validator/testdata/feature_tests/empty.out b/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)
diff --git a/validator/testdata/feature_tests/incorrect_custom_style.html b/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 }
 

diff --git a/validator/testdata/feature_tests/incorrect_custom_style.out b/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>

diff --git a/validator/testdata/feature_tests/multiple_body_tags_3.out b/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)
diff --git a/validator/testdata/feature_tests/not_amp.out b/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)
diff --git a/validator/testdata/feature_tests/unprintable_chars.out b/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)
diff --git a/validator/testdata/transformed_feature_tests/transformed_but_not_identified_transformed.out b/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)