Skip to content

Commit a5b64c5

Browse files
honeybadgerdontcarehoneybadgerdontcare
authored
Validator roll up (ampproject#25224)
* cl/275942712 Fix validator issue with error'ing tags satisfying unique requirements. * cl/276319310 Restore a stub of amp.validator.categorizeError in javascript.
1 parent ef2f279 commit a5b64c5

File tree

6 files changed

+570
-337
lines changed

6 files changed

+570
-337
lines changed

‎validator/engine/validator.js‎

Lines changed: 77 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ goog.require('amp.validator.AttrSpec');
3535
goog.require('amp.validator.CdataSpec');
3636
goog.require('amp.validator.CssDeclaration');
3737
goog.require('amp.validator.CssSpec');
38+
goog.require('amp.validator.ErrorCategory');
3839
goog.require('amp.validator.ExtensionSpec');
3940
goog.require('amp.validator.PropertySpecList');
4041
goog.require('amp.validator.ReferencePoint');
@@ -535,6 +536,42 @@ class ParsedAttrSpecs {
535536
}
536537
}
537538

539+
/** @enum {string} */
540+
const RecordValidated = {
541+
ALWAYS: 'ALWAYS',
542+
NEVER: 'NEVER',
543+
IF_PASSING: 'IF_PASSING'
544+
};
545+
546+
/**
547+
* We only track (that is, add them to Context.RecordTagspecValidated) validated
548+
* tagspecs as necessary. That is, if it's needed for document scope validation:
549+
* - Mandatory tags
550+
* - Unique tags
551+
* - Tags (identified by their TagSpecName() that are required by other tags.
552+
* @param {!amp.validator.TagSpec} tag
553+
* @param {number} tagSpecId
554+
* @param {!Array<boolean>} tagSpecIdsToTrack
555+
* @return {!RecordValidated}
556+
*/
557+
function shouldRecordTagspecValidated(tag, tagSpecId, tagSpecIdsToTrack) {
558+
// Always update from TagSpec if the tag is passing. If it's failing we
559+
// typically want to update from the best match as it can satisfy
560+
// requirements which otherwise can confuse the user later. The exception is
561+
// tagspecs which introduce requirements but satisfy none, such as unique.
562+
// https://github.com/ampproject/amphtml/issues/24359
563+
564+
// Mandatory and tagSpecIdsToTrack only satisfy requirements, making the
565+
// output less verbose even if the tag is failing.
566+
if (tag.mandatory || tagSpecIdsToTrack.hasOwnProperty(tagSpecId))
567+
return RecordValidated.ALWAYS;
568+
// Unique and similar can introduce requirements, ie: there cannot be another
569+
// such tag. We don't want to introduce requirements for failing tags.
570+
if (tag.unique || tag.requires.length > 0 || tag.uniqueWarning)
571+
return RecordValidated.IF_PASSING;
572+
return RecordValidated.NEVER;
573+
}
574+
538575
/**
539576
* This wrapper class provides access to a TagSpec and a tag id
540577
* which is unique within its context, the ParsedValidatorRules.
@@ -543,7 +580,7 @@ class ParsedAttrSpecs {
543580
class ParsedTagSpec {
544581
/**
545582
* @param {!ParsedAttrSpecs} parsedAttrSpecs
546-
* @param {boolean} shouldRecordTagspecValidated
583+
* @param {!RecordValidated} shouldRecordTagspecValidated
547584
* @param {!amp.validator.TagSpec} tagSpec
548585
* @param {number} id
549586
*/
@@ -574,7 +611,7 @@ class ParsedTagSpec {
574611
*/
575612
this.isTypeJson_ = false;
576613
/**
577-
* @type {boolean}
614+
* @type {!RecordValidated}
578615
* @private
579616
*/
580617
this.shouldRecordTagspecValidated_ = shouldRecordTagspecValidated;
@@ -816,7 +853,7 @@ class ParsedTagSpec {
816853
* Context.recordTagspecValidated_ if it was validated
817854
* successfullly. For performance, this is only done for tags that are
818855
* mandatory, unique, or possibly required by some other tag.
819-
* @return {boolean}
856+
* @return {!RecordValidated}
820857
*/
821858
shouldRecordTagspecValidated() {
822859
return this.shouldRecordTagspecValidated_;
@@ -2515,6 +2552,9 @@ class Context {
25152552
updateFromTagResult_(result) {
25162553
if (result.bestMatchTagSpec === null) {return;}
25172554
const parsedTagSpec = result.bestMatchTagSpec;
2555+
const isPassing =
2556+
(result.validationResult.status ===
2557+
amp.validator.ValidationResult.Status.PASS);
25182558

25192559
this.extensions_.updateFromTagResult(result);
25202560
// If this requires an extension and we are still in the document head,
@@ -2530,7 +2570,7 @@ class Context {
25302570
// the document.
25312571
this.satisfyConditionsFromTagSpec_(parsedTagSpec);
25322572
this.satisfyMandatoryAlternativesFromTagSpec_(parsedTagSpec);
2533-
this.recordValidatedFromTagSpec_(parsedTagSpec);
2573+
this.recordValidatedFromTagSpec_(isPassing, parsedTagSpec);
25342574

25352575
const {validationResult} = result;
25362576
for (const provision of validationResult.valueSetProvisions)
@@ -2546,8 +2586,7 @@ class Context {
25462586
errors.push(requirement.errorIfUnsatisfied);
25472587
}
25482588

2549-
if (result.validationResult.status ===
2550-
amp.validator.ValidationResult.Status.PASS) {
2589+
if (isPassing) {
25512590
// If the tag spec didn't match, we don't know that the tag actually
25522591
// contained a URL, so no need to complain about it.
25532592
this.markUrlSeenFromMatchingTagSpec_(parsedTagSpec);
@@ -2665,12 +2704,17 @@ class Context {
26652704

26662705
/**
26672706
* Records that this document contains a tag matching a particular tag spec.
2707+
* @param {boolean} isPassing
26682708
* @param {!ParsedTagSpec} parsedTagSpec
26692709
* @private
26702710
*/
2671-
recordValidatedFromTagSpec_(parsedTagSpec) {
2672-
if (parsedTagSpec.shouldRecordTagspecValidated())
2711+
recordValidatedFromTagSpec_(isPassing, parsedTagSpec) {
2712+
const recordValidated = parsedTagSpec.shouldRecordTagspecValidated();
2713+
if (recordValidated == RecordValidated.ALWAYS) {
2714+
this.tagspecsValidated_[parsedTagSpec.id()] = true;
2715+
} else if (isPassing && (recordValidated == RecordValidated.IF_PASSING)) {
26732716
this.tagspecsValidated_[parsedTagSpec.id()] = true;
2717+
}
26742718
}
26752719

26762720
/**
@@ -3478,21 +3522,6 @@ function CalculateLayout(inputLayout, width, height, sizesAttr, heightsAttr) {
34783522
}
34793523
}
34803524

3481-
/**
3482-
* We only track (that is, add them to Context.RecordTagspecValidated) validated
3483-
* tagspecs as necessary. That is, if it's needed for document scope validation:
3484-
* - Mandatory tags
3485-
* - Unique tags
3486-
* - Tags (identified by their TagSpecName() that are required by other tags.
3487-
* @param {!amp.validator.TagSpec} tag
3488-
* @param {number} tagSpecId
3489-
* @param {!Array<boolean>} tagSpecIdsToTrack
3490-
* @return {boolean}
3491-
*/
3492-
function shouldRecordTagspecValidated(tag, tagSpecId, tagSpecIdsToTrack) {
3493-
return tag.mandatory || tag.unique || tag.requires.length > 0 ||
3494-
tagSpecIdsToTrack.hasOwnProperty(tagSpecId) || tag.uniqueWarning;
3495-
}
34963525

34973526
/**
34983527
* DispatchKey represents a tuple of either 1-3 strings:
@@ -6238,3 +6267,28 @@ amp.validator.renderValidationResult = function(validationResult, filename) {
62386267
function isAuthorStylesheet(param) {
62396268
return goog.string./*OK*/ startsWith(param, 'style amp-custom');
62406269
}
6270+
6271+
/**
6272+
* This function was removed in October 2019. Older versions of the nodejs
6273+
* amphtml-validator library still call this function, so this stub is left
6274+
* in place for now so as not to break them. TODO(#25188): Delete this function
6275+
* after most usage had moved to a newer version of the amphtml-validator lib.
6276+
* @param {!amp.validator.ValidationError} error
6277+
* @return {!amp.validator.ErrorCategory.Code}
6278+
* @export
6279+
*/
6280+
amp.validator.categorizeError = function(error) {
6281+
return amp.validator.ErrorCategory.Code.UNKNOWN;
6282+
};
6283+
6284+
/**
6285+
* Convenience function which calls |CategorizeError| for each error
6286+
* in |result| and sets its category field accordingly.
6287+
* @param {!amp.validator.ValidationResult} result
6288+
* @export
6289+
*/
6290+
amp.validator.annotateWithErrorCategories = function(result) {
6291+
for (const error of result.errors) {
6292+
error.category = amp.validator.categorizeError(error);
6293+
}
6294+
};

‎validator/testdata/feature_tests/error_and_unique.html‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
<!--
2+
Copyright 2019 The AMP HTML Authors. All Rights Reserved.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS-IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the license.
15+
-->
16+
<!--
17+
Test Description:
18+
This test includes two <style> tags. The first produces an error because
19+
it is missing the amp-custom attribute. The second has the amp-custom but
20+
should not produce an error due to duplicate style amp-custom tags.
21+
See https://github.com/ampproject/amphtml/issues/24359 for history.
22+
-->
23+
<!doctype html>
24+
<html ⚡>
25+
<head>
26+
<meta charset="utf-8">
27+
<link rel="canonical" href="./regular-html-version.html">
28+
<meta name="viewport" content="width=device-width">
29+
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
30+
<script async src="https://cdn.ampproject.org/v0.js"></script>
31+
32+
<style>.bar {}</style>
33+
<style amp-custom>.foo {}</style>
34+
35+
</head>
36+
<body>
37+
Hello, world.
38+
</body>
39+
</html>

‎validator/testdata/feature_tests/error_and_unique.out‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
FAIL
2+
| <!--
3+
| Copyright 2019 The AMP HTML Authors. All Rights Reserved.
4+
|
5+
| Licensed under the Apache License, Version 2.0 (the "License");
6+
| you may not use this file except in compliance with the License.
7+
| You may obtain a copy of the License at
8+
|
9+
| http://www.apache.org/licenses/LICENSE-2.0
10+
|
11+
| Unless required by applicable law or agreed to in writing, software
12+
| distributed under the License is distributed on an "AS-IS" BASIS,
13+
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
| See the License for the specific language governing permissions and
15+
| limitations under the license.
16+
| -->
17+
| <!--
18+
| Test Description:
19+
| This test includes two <style> tags. The first produces an error because
20+
| it is missing the amp-custom attribute. The second has the amp-custom but
21+
| should not produce an error due to duplicate style amp-custom tags.
22+
| See https://github.com/ampproject/amphtml/issues/24359 for history.
23+
| -->
24+
| <!doctype html>
25+
| <html ⚡>
26+
| <head>
27+
| <meta charset="utf-8">
28+
| <link rel="canonical" href="./regular-html-version.html">
29+
| <meta name="viewport" content="width=device-width">
30+
| <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
31+
| <script async src="https://cdn.ampproject.org/v0.js"></script>
32+
|
33+
| <style>.bar {}</style>
34+
>> ^~~~~~~~~
35+
feature_tests/error_and_unique.html:32:2 The mandatory attribute 'amp-custom' is missing in tag 'style amp-custom'. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
36+
| <style amp-custom>.foo {}</style>
37+
|
38+
| </head>
39+
| <body>
40+
| Hello, world.
41+
| </body>
42+
| </html>

‎validator/testdata/feature_tests/regexps.out‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ feature_tests/regexps.html:111:2 The attribute 'name' in tag 'meta name= and con
147147
| -->
148148
| <style amp-custom>
149149
>> ^~~~~~~~~
150-
feature_tests/regexps.html:117:2 The tag 'style amp-custom' appears more than once in the document. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
150+
feature_tests/regexps.html:117:2 The text inside tag 'style amp-custom' contains 'CSS i-amphtml- name prefix', which is disallowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets)
151151
| .comic-amp-font-loaded .comic-amp {
152152
| font-family: 'Comic AMP', serif, sans-serif;
153153
| }

0 commit comments

Comments
 (0)