Skip to content

Commit

Permalink
🐛Fix the bug that parseLayout ignores case and underscore (#32890)
Browse files Browse the repository at this point in the history 
* Fix the bug that parseLayout ignores case and '_'

* Add case sensitvie tests for layout value
  • Loading branch information
@caoboxiao
caoboxiao committed Feb 25, 2021
1 parent 72ba9c4 commit 2ef25d7
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 18 deletions.
5 changes: 3 additions & 2 deletions validator/js/engine/validator.js
View file
Expand Up @@ -3826,8 +3826,9 @@ function parseLayout(layout) {
if (layout === undefined) {
return generated.AmpLayout.Layout.UNKNOWN;
}
const normLayout = layout.toUpperCase().replace('-', '_');
const idx = generated.AmpLayout.Layout_NamesByIndex.indexOf(normLayout);
const idx = generated.AmpLayout.Layout_NamesByIndex.findIndex((name) => {
return name.toLowerCase().replace('_', '-') === layout;
});
if (idx === -1) {
return generated.AmpLayout.Layout.UNKNOWN;
}
Expand Down
6 changes: 6 additions & 0 deletions validator/testdata/feature_tests/amp_layouts.html
View file
Expand Up @@ -43,6 +43,12 @@
<!-- valid: layout=fixed-height -->
<amp-img layout="fixed-height" height="200" src="itshappening.gif"></amp-img>

<!-- invalid: layout value must be lower case -->
<amp-img layout="FIXED-HEIGHT" height="200" src="itshappening.gif"></amp-img>

<!-- invalid: layout value must use hyphen rather than underscore -->
<amp-img layout="fixed_height" height="200" src="itshappening.gif"></amp-img>

<!-- valid: layout=fixed-height, with width=auto -->
<amp-img layout="fixed-height" height="200" width="auto"
src="itshappening.gif"></amp-img>
Expand Down
42 changes: 26 additions & 16 deletions validator/testdata/feature_tests/amp_layouts.out
View file
Expand Up @@ -46,14 +46,24 @@ feature_tests/amp_layouts.html:41:2 The mandatory attribute 'height' is missing
| <!-- valid: layout=fixed-height -->
| <amp-img layout="fixed-height" height="200" src="itshappening.gif"></amp-img>
|
| <!-- invalid: layout value must be lower case -->
| <amp-img layout="FIXED-HEIGHT" height="200" src="itshappening.gif"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:47:2 The attribute 'layout' in tag 'amp-img' is set to the invalid value 'FIXED-HEIGHT'. (see https://amp.dev/documentation/components/amp-img/)
|
| <!-- invalid: layout value must use hyphen rather than underscore -->
| <amp-img layout="fixed_height" height="200" src="itshappening.gif"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:50:2 The attribute 'layout' in tag 'amp-img' is set to the invalid value 'fixed_height'. (see https://amp.dev/documentation/components/amp-img/)
|
| <!-- valid: layout=fixed-height, with width=auto -->
| <amp-img layout="fixed-height" height="200" width="auto"
| src="itshappening.gif"></amp-img>
|
| <!-- invalid: layout=fixed-height, prohibit width!=auto -->
| <amp-img layout="fixed-height" height="200" width="300"
>> ^~~~~~~~~
feature_tests/amp_layouts.html:51:2 Invalid value '300' for attribute 'width' in tag 'amp-img' - for layout 'FIXED_HEIGHT', set the attribute 'width' to value 'auto'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:57:2 Invalid value '300' for attribute 'width' in tag 'amp-img' - for layout 'FIXED_HEIGHT', set the attribute 'width' to value 'auto'. (see https://amp.dev/documentation/components/amp-img/)
| src="itshappening.gif"></amp-img>
|
| <!-- valid: layout=fixed-height - default with height -->
Expand All @@ -65,7 +75,7 @@ feature_tests/amp_layouts.html:51:2 Invalid value '300' for attribute 'width' in
| <!-- invalid: layout=fixed-height - requires height -->
| <amp-img layout="fixed-height" src="itshappening.gif"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:61:2 The mandatory attribute 'height' is missing in tag 'amp-img'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:67:2 The mandatory attribute 'height' is missing in tag 'amp-img'. (see https://amp.dev/documentation/components/amp-img/)
|
| <!-- valid: layout=responsive -->
| <amp-img layout="responsive" width="100" height="200"
Expand All @@ -82,7 +92,7 @@ feature_tests/amp_layouts.html:61:2 The mandatory attribute 'height' is missing
| for flex-item -->
| <amp-img layout="fill" width="50" height="auto"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:76:2 The attribute 'height' in tag 'amp-img' is set to the invalid value 'auto'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:82:2 The attribute 'height' in tag 'amp-img' is set to the invalid value 'auto'. (see https://amp.dev/documentation/components/amp-img/)
|
| <!-- valid: layout=flex-item -->
| <amp-img layout="flex-item" src="itshappening.gif"></amp-img>
Expand All @@ -104,12 +114,12 @@ feature_tests/amp_layouts.html:76:2 The attribute 'height' in tag 'amp-img' is s
| -->
| <amp-img layout="container" src="itshappening.gif"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:96:2 The specified layout 'CONTAINER' is not supported by tag 'amp-img'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:102:2 The specified layout 'CONTAINER' is not supported by tag 'amp-img'. (see https://amp.dev/documentation/components/amp-img/)
|
| <!-- invalid: layout=unknown -->
| <amp-img layout="unknown" src="itshappening.gif"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:99:2 The attribute 'layout' in tag 'amp-img' is set to the invalid value 'unknown'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:105:2 The attribute 'layout' in tag 'amp-img' is set to the invalid value 'unknown'. (see https://amp.dev/documentation/components/amp-img/)
|
| <!-- valid: should configure natural dimensions; default layout -->
| <amp-pixel src="https://www.example.com/make-my-visit-count"></amp-pixel>
Expand Down Expand Up @@ -141,19 +151,19 @@ feature_tests/amp_layouts.html:99:2 The attribute 'layout' in tag 'amp-img' is s
| support fixed-height layout. -->
| <amp-pixel src="https://www.example.com/make-my-visit-count"
>> ^~~~~~~~~
feature_tests/amp_layouts.html:129:2 The implied layout 'FIXED_HEIGHT' is not supported by tag 'amp-pixel'. (see https://amp.dev/documentation/components/amp-pixel/)
feature_tests/amp_layouts.html:135:2 The implied layout 'FIXED_HEIGHT' is not supported by tag 'amp-pixel'. (see https://amp.dev/documentation/components/amp-pixel/)
| width="auto" height="1px"></amp-pixel>
|
| <!-- invalid: width=X. -->
| <amp-pixel src="https://www.example.com/make-my-visit-count"
>> ^~~~~~~~~
feature_tests/amp_layouts.html:133:2 The attribute 'width' in tag 'amp-pixel' is set to the invalid value 'X'. (see https://amp.dev/documentation/components/amp-pixel/)
feature_tests/amp_layouts.html:139:2 The attribute 'width' in tag 'amp-pixel' is set to the invalid value 'X'. (see https://amp.dev/documentation/components/amp-pixel/)
| width="X" height="1px"></amp-pixel>
|
| <!-- invalid: height=X. -->
| <amp-pixel src="https://www.example.com/make-my-visit-count"
>> ^~~~~~~~~
feature_tests/amp_layouts.html:137:2 The attribute 'height' in tag 'amp-pixel' is set to the invalid value 'X'. (see https://amp.dev/documentation/components/amp-pixel/)
feature_tests/amp_layouts.html:143:2 The attribute 'height' in tag 'amp-pixel' is set to the invalid value 'X'. (see https://amp.dev/documentation/components/amp-pixel/)
| height="X" width="1px"></amp-pixel>
|
| <!-- valid responsive layout with heights -->
Expand All @@ -167,33 +177,33 @@ feature_tests/amp_layouts.html:137:2 The attribute 'height' in tag 'amp-pixel' i
| <!-- invalid: can't have heights with specified layout fixed -->
| <amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" width=400 height=300 layout="fixed"
>> ^~~~~~~~~
feature_tests/amp_layouts.html:149:2 The attribute 'heights' in tag 'amp-img' is disallowed by specified layout 'FIXED'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:155:2 The attribute 'heights' in tag 'amp-img' is disallowed by specified layout 'FIXED'. (see https://amp.dev/documentation/components/amp-img/)
| heights="(min-width:760px) 33%, (min-width:500px) 75%, 125%" ></amp-img>
|
| <!-- invalid: can't have heights with implied layout fixed_height -->
| <amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" height=300
>> ^~~~~~~~~
feature_tests/amp_layouts.html:153:2 The attribute 'heights' in tag 'amp-img' is disallowed by implied layout 'FIXED_HEIGHT'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:159:2 The attribute 'heights' in tag 'amp-img' is disallowed by implied layout 'FIXED_HEIGHT'. (see https://amp.dev/documentation/components/amp-img/)
| heights="(min-width:760px) 33%, (min-width:500px) 75%, 125%" ></amp-img>
| <!-- valid: intrinsic has height and width set with same units -->
| <amp-img src="img.png" height="30px" width="300px" layout="intrinsic"></amp-img>
| <!-- invalid: intrinsic has height and width set with different units -->
| <amp-img src="img.png" height="30px" width="300em" layout="intrinsic"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:158:2 Inconsistent units for width and height in tag 'amp-img' - width is specified in 'em' whereas height is specified in 'px'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:164:2 Inconsistent units for width and height in tag 'amp-img' - width is specified in 'em' whereas height is specified in 'px'. (see https://amp.dev/documentation/components/amp-img/)
| <!-- invalid: intrinsic height and width can not be auto -->
| <amp-img src="img.png" height="auto" width="300px" layout="intrinsic"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:160:2 The attribute 'height' in tag 'amp-img' is set to the invalid value 'auto'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:166:2 The attribute 'height' in tag 'amp-img' is set to the invalid value 'auto'. (see https://amp.dev/documentation/components/amp-img/)
| <amp-img src="img.png" height="30px" width="auto" layout="intrinsic"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:161:2 The attribute 'width' in tag 'amp-img' is set to the invalid value 'auto'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:167:2 The attribute 'width' in tag 'amp-img' is set to the invalid value 'auto'. (see https://amp.dev/documentation/components/amp-img/)
| <!-- invalid: intrinsic height and width must be set -->
| <amp-img src="img.png" height="30px" layout="intrinsic"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:163:2 The mandatory attribute 'width' is missing in tag 'amp-img'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:169:2 The mandatory attribute 'width' is missing in tag 'amp-img'. (see https://amp.dev/documentation/components/amp-img/)
| <amp-img src="img.png" width="300px" layout="intrinsic"></amp-img>
>> ^~~~~~~~~
feature_tests/amp_layouts.html:164:2 The mandatory attribute 'height' is missing in tag 'amp-img'. (see https://amp.dev/documentation/components/amp-img/)
feature_tests/amp_layouts.html:170:2 The mandatory attribute 'height' is missing in tag 'amp-img'. (see https://amp.dev/documentation/components/amp-img/)
| </body>
| </html>
| </html>

0 comments on commit 2ef25d7

Please sign in to comment.