Validator rollup

extensions/amp-nested-menu/0.1/test/validator-amp-nested-menu-error.out

@@ -46,10 +46,10 @@ amp-nested-menu/0.1/test/validator-amp-nested-menu-error.html:36:4 The attribute
 amp-nested-menu/0.1/test/validator-amp-nested-menu-error.html:39:8 Mutually exclusive attributes encountered in tag 'h4' - pick one of ['amp-nested-submenu-close', 'amp-nested-submenu-open'].
 |          <div amp-nested-submenu amp-nested-submenu-close></div>
 >>         ^~~~~~~~~
-amp-nested-menu/0.1/test/validator-amp-nested-menu-error.html:40:8 Mutually exclusive attributes encountered in tag 'div' - pick one of ['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open'].
+amp-nested-menu/0.1/test/validator-amp-nested-menu-error.html:40:8 Mutually exclusive attributes encountered in tag 'div amp-nested-menu' - pick one of ['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open'].
 |          <div amp-nested-submenu amp-nested-submenu-open></div>
 >>         ^~~~~~~~~
-amp-nested-menu/0.1/test/validator-amp-nested-menu-error.html:41:8 Mutually exclusive attributes encountered in tag 'div' - pick one of ['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open'].
+amp-nested-menu/0.1/test/validator-amp-nested-menu-error.html:41:8 Mutually exclusive attributes encountered in tag 'div amp-nested-menu' - pick one of ['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open'].
 |        </div>
 |        <!-- Invalid: only divs may include the amp-nested-submenu attribute -->
 |        <div>
@@ -77,7 +77,7 @@ amp-nested-menu/0.1/test/validator-amp-nested-menu-error.html:51:8 The attribute
 |              <h4 amp-nested-submenu-open></h4>
 |              <div amp-nested-submenu></div>
 >>             ^~~~~~~~~
-amp-nested-menu/0.1/test/validator-amp-nested-menu-error.html:59:12 The attribute 'amp-nested-submenu' may not appear in tag 'div'.
+amp-nested-menu/0.1/test/validator-amp-nested-menu-error.html:59:12 The tag 'div amp-nested-menu' may not appear as a descendant of tag 'amp-accordion'.
 |            </div>
 |          </section>
 |        </amp-accordion>

extensions/amp-nested-menu/validator-amp-nested-menu.protoascii

@@ -45,19 +45,23 @@ tags: {  # <amp-nested-menu> <div amp-nested-submenu(-open/-close)>
   html_format: AMP
   tag_name: "DIV"
   spec_name: "div amp-nested-menu"
+  descriptive_name: "div amp-nested-menu"
   mandatory_ancestor: "AMP-NESTED-MENU"
   # submenus inside accordion do not display correctly due to styling of <amp-accordion>.
   disallowed_ancestor: "AMP-ACCORDION"
   attrs: {
     name: "amp-nested-submenu"
+    dispatch_key: NAME_VALUE_DISPATCH
     mandatory_oneof: "['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open']"
   }
   attrs: {
     name: "amp-nested-submenu-close"
+    dispatch_key: NAME_VALUE_DISPATCH
     mandatory_oneof: "['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open']"
   }
   attrs: {
     name: "amp-nested-submenu-open"
+    dispatch_key: NAME_VALUE_DISPATCH
     mandatory_oneof: "['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open']"
   }
 }

validator/js/engine/validator.js

@@ -855,6 +855,35 @@ class ParsedTagSpec {
     sortAndUniquify(this.mandatoryOneofs_);
     sortAndUniquify(this.mandatoryAnyofs_);
     sortAndUniquify(this.mandatoryAttrIds_);
+
+    /**
+     * @type {!Array<string>}
+     * @private
+     */
+    this.dispatchKeys_ = [];
+    for (const attrName of Object.keys(this.attrsByName_)) {
+      const attrId = this.attrsByName_[attrName];
+      if (attrId < 0)  // negative attr ids are simple attrs (only name set).
+        continue;
+      const parsedAttrSpec = parsedAttrSpecs.getByAttrSpecId(attrId);
+      const attrSpec = parsedAttrSpec.getSpec();
+      if (attrSpec.dispatchKey !== null) {
+        const mandatoryParent =
+            tagSpec.mandatoryParent !== null ? tagSpec.mandatoryParent : '';
+        if (attrSpec.value.length > 0) {
+          this.dispatchKeys_.push(makeDispatchKey(
+              attrSpec.dispatchKey, attrName, attrSpec.value[0].toLowerCase(),
+              mandatoryParent));
+        } else if (attrSpec.valueCasei.length > 0) {
+          this.dispatchKeys_.push(makeDispatchKey(
+              attrSpec.dispatchKey, attrName, attrSpec.valueCasei[0],
+              mandatoryParent));
+        } else {
+          this.dispatchKeys_.push(makeDispatchKey(
+              attrSpec.dispatchKey, attrName, '', mandatoryParent));
+        }
+      }
+    }
   }
 
   /**
@@ -892,7 +921,7 @@ class ParsedTagSpec {
       }
       if (spec.name === 'type' && spec.valueCasei.length > 0) {
         for (const v of spec.valueCasei) {
-          if ('application/json' === v) {
+          if (('application/json' === v) || ('application/ld+json' === v)) {
             this.isTypeJson_ = true;
             break;
           }
@@ -990,6 +1019,23 @@ class ParsedTagSpec {
         typeIdentifiers, this.spec_.enabledBy, this.spec_.disabledBy);
   }
 
+  /**
+   * Returns an array (typically empty) of all unique dispatch keys for this
+   * tagspec. A dispatch key is a combination of attribute name, attribute
+   * value, and / or tag parent. If multiple TagSpecs have the same dispatch
+   * key, then the Tagwith the first instance of that dispatch key is used.
+   * When an encounttag matches this dispatch key, it is validated first
+   * against that first TagSpec in order to improve validation performance and
+   * error message selection. Not all TagSpecs have a dispatch key. If the
+   * attribute value is used (either value or value_casei), uses the first
+   * value from the protoascii.
+   * @return {!Array<string>}
+   */
+  GetDispatchKeys() {
+    return this.dispatchKeys_;
+  }
+
+
   /**
    * A TagSpec may specify other tags to be required as well, when that
    * tag is used. This accessor returns the IDs for the tagspecs that
@@ -1995,9 +2041,10 @@ function isAtRuleValid(cssSpec, atRuleName) {
   for (const atRuleSpec of cssSpec.atRuleSpec) {
     // "-moz-document" is specified in the list of allowed rules with an
     // explicit vendor prefix. The idea here is that only this specific vendor
-    // prefix is allowed, not "-ms-document" or even "document". We first search
-    // the allowed list for the seen `at_rule_name` with stripped vendor prefix,
-    // then if not found, we search again without sripping the vendor prefix.
+    // prefix is allowed, not "-ms-document" or even "document". We first
+    // search the allowed list for the seen `at_rule_name` with stripped
+    // vendor prefix, then if not found, we search again without sripping the
+    // vendor prefix.
     if (atRuleSpec.name === parse_css.stripVendorPrefix(atRuleName) ||
         atRuleSpec.name === atRuleName) {
       return true;
@@ -2071,7 +2118,8 @@ class InvalidRuleVisitor extends parse_css.RuleVisitor {
         this.context.addError(
             generated.ValidationError.Code.CSS_SYNTAX_INVALID_PROPERTY_NOLIST,
             new LineCol(declaration.line, declaration.col),
-            /* params */[getTagDescriptiveName(this.tagSpec), declaration.name],
+            /* params */
+            [getTagDescriptiveName(this.tagSpec), declaration.name],
             /* url */ '', this.result);
 
       } else {
@@ -2757,9 +2805,9 @@ class ExtensionsContext {
 }
 
 // If any script in the page uses a specific release version, then all scripts
-// must use that specific release version. This is used to record the first seen
-// script tag and ensure all following script tags follow the convention set by
-// it.
+// must use that specific release version. This is used to record the first
+// seen script tag and ensure all following script tags follow the convention
+// set by it.
 /** @enum {string} */
 const ScriptReleaseVersion = {
   UNKNOWN: 'unknown',
@@ -5929,9 +5977,9 @@ class ParsedValidatorRules {
 
     // For every tagspec that contains an ExtensionSpec, we add several
     // TagSpec fields corresponding to the data found in the ExtensionSpec.
-    // The addition of module/nomodule extensions happens in validator_gen_js.py
-    // and are built as proper JavaScript classes. They will also be expanded
-    // by this method.
+    // The addition of module/nomodule extensions happens in
+    // validator_gen_js.py and are built as proper JavaScript classes. They
+    // will also be expanded by this method.
     this.expandExtensionSpec_ = function() {
       const numTags = this.rules_.tags.length;
       for (let tagSpecId = 0; tagSpecId < numTags; ++tagSpecId) {
@@ -6033,6 +6081,11 @@ class ParsedValidatorRules {
       for (const otherTag of tag.alsoRequiresTagWarning) {
         this.tagSpecIdsToTrack_[otherTag] = true;
       }
+      const parsed = new ParsedTagSpec(
+          this.parsedAttrSpecs_,
+          shouldRecordTagspecValidated(tag, tagSpecId, this.tagSpecIdsToTrack_),
+          tag, tagSpecId);
+      this.parsedTagSpecById_[tagSpecId] = parsed;
       if (tag.tagName !== '$REFERENCE_POINT') {
         if (!(tag.tagName in this.tagSpecByTagName_)) {
           this.tagSpecByTagName_[tag.tagName] = new TagSpecDispatch();
@@ -6048,11 +6101,12 @@ class ParsedValidatorRules {
               /** @type {string} */ (tag.extensionSpec.name), '');
           tagnameDispatch.registerDispatchKey(dispatchKey, tagSpecId);
         } else {
-          const dispatchKey = this.rules_.dispatchKeyByTagSpecId[tagSpecId];
-          if (dispatchKey === undefined) {
-            tagnameDispatch.registerTagSpec(tagSpecId);
+          const dispatchKeys = parsed.GetDispatchKeys();
+          if (dispatchKeys.length > 0) {
+            for (const dispatchKey of dispatchKeys)
+              tagnameDispatch.registerDispatchKey(dispatchKey, tagSpecId);
           } else {
-            tagnameDispatch.registerDispatchKey(dispatchKey, tagSpecId);
+            tagnameDispatch.registerTagSpec(tagSpecId);
           }
         }
       }
@@ -6255,8 +6309,8 @@ class ParsedValidatorRules {
         }
       }
     }
-    // If AMP Email format and not set to data-css-strict, then issue a warning
-    // that not having data-css-strict is deprecated. See b/179798751.
+    // If AMP Email format and not set to data-css-strict, then issue a
+    // warning that not having data-css-strict is deprecated. See b/179798751.
     if (hasEmailTypeIdentifier && !hasCssStrictTypeIdentifier) {
       context.addWarning(
           generated.ValidationError.Code.AMP_EMAIL_MISSING_STRICT_CSS_ATTR,
@@ -6265,7 +6319,8 @@ class ParsedValidatorRules {
           validationResult);
     }
     if (!hasMandatoryTypeIdentifier) {
-      // Missing mandatory type identifier (any AMP variant but "transformed").
+      // Missing mandatory type identifier (any AMP variant but
+      // "transformed").
       context.addError(
           generated.ValidationError.Code.MANDATORY_ATTR_MISSING,
           context.getLineCol(),
@@ -6674,18 +6729,7 @@ class ParsedValidatorRules {
    * @return {!ParsedTagSpec}
    */
   getByTagSpecId(id) {
-    let parsed = this.parsedTagSpecById_[id];
-    if (parsed !== undefined) {
-      return parsed;
-    }
-    const tag = this.rules_.tags[id];
-    asserts.assert(tag !== undefined);
-    parsed = new ParsedTagSpec(
-        this.parsedAttrSpecs_,
-        shouldRecordTagspecValidated(tag, id, this.tagSpecIdsToTrack_), tag,
-        id);
-    this.parsedTagSpecById_[id] = parsed;
-    return parsed;
+    return this.parsedTagSpecById_[id];
   }
 
   /**
@@ -6899,8 +6943,8 @@ const ValidationHandler =
   }
 
   /**
-   * Currently, the Javascript HTML parser considers Doctype to be another HTML
-   * tag, which is not technically accurate. We have special handling for
+   * Currently, the Javascript HTML parser considers Doctype to be another
+   * HTML tag, which is not technically accurate. We have special handling for
    * doctype in Javascript which applies to all AMP formats, as this is strict
    * handling for all HTML in general. Specifically "attributes" are not
    * allowed, even things like `data-foo`.
@@ -6914,10 +6958,10 @@ const ValidationHandler =
         encounteredTag.attrs()[0].name === 'html')
       return;
     // <!doctype html lang=...> OK
-    // This is technically invalid. The 'correct' way to do this is to emit the
-    // lang attribute on the `<html>` tag. However, we observe a number of
-    // websites incorrectly emitting `lang` as part of doctype, so this specific
-    // attribute is allowed to avoid breaking existing pages.
+    // This is technically invalid. The 'correct' way to do this is to emit
+    // the lang attribute on the `<html>` tag. However, we observe a number of
+    // websites incorrectly emitting `lang` as part of doctype, so this
+    // specific attribute is allowed to avoid breaking existing pages.
     if (encounteredTag.attrs().length === 2) {
       if (encounteredTag.attrs()[0].name === 'html' &&
           encounteredTag.attrs()[1].name === 'lang')
@@ -6952,9 +6996,9 @@ const ValidationHandler =
       this.validateDocType(
           encounteredTag, this.context_, this.validationResult_);
       // Even though validateDocType emits all necessary errors about the tag,
-      // we continue to process it further (validateTag and such) so that we can
-      // record the tag was present and record it as the root pseudo element for
-      // the document.
+      // we continue to process it further (validateTag and such) so that we
+      // can record the tag was present and record it as the root pseudo
+      // element for the document.
     }
     /** @type {?string} */
     const maybeDuplicateAttrName = encounteredTag.hasDuplicateAttrs();

validator/js/engine/validator_test.js

@@ -485,35 +485,33 @@ describe('Validator.ScriptLength', () => {
   const inlineScriptBlob = 'alert(\'\');';
   assertStrictEqual(10, inlineScriptBlob.length);
 
-  it('accepts 10000 bytes of inline style',
-     () => {
-       const inlineScript = Array(1001).join(inlineScriptBlob);
-       assertStrictEqual(10000, inlineScript.length);
-       const test = new ValidatorTestCase('feature_tests/inline_script_length.html');
-       test.inlineOutput = false;
-       test.ampHtmlFileContents =
-           test.ampHtmlFileContents
-               .replace('replace_inline_script', inlineScript);
-       test.expectedOutput = 'PASS';
-       test.run();
-     });
+  it('accepts 10000 bytes of inline style', () => {
+    const inlineScript = Array(1001).join(inlineScriptBlob);
+    assertStrictEqual(10000, inlineScript.length);
+    const test =
+        new ValidatorTestCase('feature_tests/inline_script_length.html');
+    test.inlineOutput = false;
+    test.ampHtmlFileContents =
+        test.ampHtmlFileContents.replace('replace_inline_script', inlineScript);
+    test.expectedOutput = 'PASS';
+    test.run();
+  });
 
-  it('will not accept 10010 bytes in inline script',
-     () => {
-       const inlineScript = Array(1001).join(inlineScriptBlob) + ' ';
-       assertStrictEqual(10001, inlineScript.length);
-       const test = new ValidatorTestCase('feature_tests/inline_script_length.html');
-       test.inlineOutput = false;
-       test.ampHtmlFileContents =
-           test.ampHtmlFileContents
-               .replace('replace_inline_script', inlineScript);
-       test.expectedOutputFile = null;
-       test.expectedOutput = 'FAIL\n' +
-           'feature_tests/inline_script_length.html:35:2 The inline script ' +
-           'is 10001 bytes, which exceeds the limit of 10000 bytes. ' +
-           '(see https://amp.dev/documentation/components/amp-script/#faq)';
-       test.run();
-     });
+  it('will not accept 10010 bytes in inline script', () => {
+    const inlineScript = Array(1001).join(inlineScriptBlob) + ' ';
+    assertStrictEqual(10001, inlineScript.length);
+    const test =
+        new ValidatorTestCase('feature_tests/inline_script_length.html');
+    test.inlineOutput = false;
+    test.ampHtmlFileContents =
+        test.ampHtmlFileContents.replace('replace_inline_script', inlineScript);
+    test.expectedOutputFile = null;
+    test.expectedOutput = 'FAIL\n' +
+        'feature_tests/inline_script_length.html:35:2 The inline script ' +
+        'is 10001 bytes, which exceeds the limit of 10000 bytes. ' +
+        '(see https://amp.dev/documentation/components/amp-script/#faq)';
+    test.run();
+  });
 });
 
 describe('Validator.CssLength', () => {
@@ -1340,8 +1338,8 @@ function attrRuleShouldMakeSense(attrSpec, tagSpec, rules) {
   // dispatch_key
   if (attrSpec.dispatchKey !== null && attrSpec.dispatchKey) {
     it('mandatory must be true when dispatch_key is true', () => {
-      expect(attrSpec.mandatory).toBeDefined();
-      expect(attrSpec.mandatory).toBe(true);
+      expect(attrSpec.mandatory === true || attrSpec.mandatoryOneof !== null)
+          .toBe(true);
     });
   }
   // Value property names must be unique.
@@ -1683,7 +1681,6 @@ describe('ValidatorRulesMakeSense', () => {
     }
 
     // attr_specs within tag.
-    let seenDispatchKey = false;
     const attrNameIsUnique = {};
     for (const attrSpecId of tagSpec.attrs) {
       if (attrSpecId < 0) {
@@ -1724,14 +1721,6 @@ describe('ValidatorRulesMakeSense', () => {
                  .toBe(true);
            });
       }
-      if (attrSpec.dispatchKey) {
-        it('tag_spec ' + tagSpecName +
-               ' can not have more than one dispatch_key',
-           () => {
-             expect(seenDispatchKey).toBe(false);
-             seenDispatchKey = true;
-           });
-      }
     }
 
     // generated.TagSpecs with an ExtensionSpec are extensions. We have a few
@@ -1863,6 +1852,7 @@ describe('ValidatorRulesMakeSense', () => {
         let hasTextPlain = false;
         let hasOctetStream = false;
         let hasCiphertext = false;
+        let hasAmpOnerror = false;
         for (const attrSpecId of tagSpec.attrs) {
           if (attrSpecId < 0) {
             continue;
@@ -1888,14 +1878,17 @@ describe('ValidatorRulesMakeSense', () => {
               }
             }
           }
+          if (attrSpec.name === 'amp-onerror') {
+            hasAmpOnerror = true;
+          }
         }
         it('script tags must have either a src attribute or type json, ' +
                'octet-stream (during SwG encryption), or text/plain',
            () => {
              expect(
                  hasSrc || hasJson || hasTextPlain ||
-                 (hasOctetStream && hasCiphertext))
-                 .toBe(true);
+                 (hasOctetStream && hasCiphertext) ||
+                 hasAmpOnerror).toBe(true);
            });
       }
       // cdata_regex and mandatory_cdata