Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 Validator: support decimals for intrisic sizer dimensions #27544

Merged
merged 1 commit into from Apr 2, 2020
Merged

馃悰 Validator: support decimals for intrisic sizer dimensions #27544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions validator/testdata/transformed_feature_tests/server_side_rendering.html
View file
Open in desktop
Expand Up @@ -40,8 +40,12 @@
<i-amphtml-sizer class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;charset=utf-8,<svg height=&quot;100&quot; width=&quot;300&quot; xmlns=&quot;http://www.w3.org/2000/svg&quot; version=&quot;1.1&quot;/>"></i-amphtml-sizer>
</amp-img>
<!-- Valid -->
<amp-img src="image.png" height="100" width="300" layout="intrinsic" class="i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" i-amphtml-layout="intrinsic">
<i-amphtml-sizer class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;charset=utf-8,<svg height='100' width='300' xmlns='http://www.w3.org/2000/svg' version='1.1'/>"></i-amphtml-sizer>
<amp-img src="image.png" height="200" width="400" layout="intrinsic" class="i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" i-amphtml-layout="intrinsic">
<i-amphtml-sizer class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;charset=utf-8,<svg height='200' width='400' xmlns='http://www.w3.org/2000/svg' version='1.1'/>"></i-amphtml-sizer>
</amp-img>
<!-- Valid -->
<amp-img src="image.png" height="200.1" width="400.15" layout="intrinsic" class="i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" i-amphtml-layout="intrinsic">
<i-amphtml-sizer class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;charset=utf-8,<svg height='200.1' width='400.15' xmlns='http://www.w3.org/2000/svg' version='1.1'/>"></i-amphtml-sizer>
</amp-img>
<!-- Invalid i-amphtml-sizer > img does not specify an svg -->
<amp-img src="image.png" height="100" width="300" layout="intrinsic" class="i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" i-amphtml-layout="intrinsic">
Expand Down
24 changes: 14 additions & 10 deletions validator/testdata/transformed_feature_tests/server_side_rendering.out
View file
Open in desktop
Expand Up @@ -41,52 +41,56 @@ FAIL
| <i-amphtml-sizer class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;charset=utf-8,<svg height=&quot;100&quot; width=&quot;300&quot; xmlns=&quot;http://www.w3.org/2000/svg&quot; version=&quot;1.1&quot;/>"></i-amphtml-sizer>
| </amp-img>
| <!-- Valid -->
| <amp-img src="image.png" height="100" width="300" layout="intrinsic" class="i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" i-amphtml-layout="intrinsic">
| <i-amphtml-sizer class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;charset=utf-8,<svg height='100' width='300' xmlns='http://www.w3.org/2000/svg' version='1.1'/>"></i-amphtml-sizer>
| <amp-img src="image.png" height="200" width="400" layout="intrinsic" class="i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" i-amphtml-layout="intrinsic">
| <i-amphtml-sizer class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;charset=utf-8,<svg height='200' width='400' xmlns='http://www.w3.org/2000/svg' version='1.1'/>"></i-amphtml-sizer>
| </amp-img>
| <!-- Valid -->
| <amp-img src="image.png" height="200.1" width="400.15" layout="intrinsic" class="i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" i-amphtml-layout="intrinsic">
| <i-amphtml-sizer class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;charset=utf-8,<svg height='200.1' width='400.15' xmlns='http://www.w3.org/2000/svg' version='1.1'/>"></i-amphtml-sizer>
| </amp-img>
| <!-- Invalid i-amphtml-sizer > img does not specify an svg -->
| <amp-img src="image.png" height="100" width="300" layout="intrinsic" class="i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" i-amphtml-layout="intrinsic">
| <i-amphtml-sizer class="i-amphtml-sizer"><img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="image.png"></i-amphtml-sizer>
>> ^~~~~~~~~
transformed_feature_tests/server_side_rendering.html:48:45 The attribute 'src' in tag 'IMG-I-AMPHTML-INTRINSIC-SIZER' is set to the invalid value 'image.png'.
transformed_feature_tests/server_side_rendering.html:52:45 The attribute 'src' in tag 'IMG-I-AMPHTML-INTRINSIC-SIZER' is set to the invalid value 'image.png'.
| </amp-img>
| <!-- Invalid intrinsic i-amphtml-sizer inside responsive sizer -->
| <amp-img src="image.png" height="100" width="300" layout="intrinsic" class="i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" i-amphtml-layout="intrinsic">
| <i-amphtml-sizer style=display:block;padding-top:171.4370%;>
| <img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;charset=utf-8,<svg height='100' width='300' xmlns='http://www.w3.org/2000/svg' version='1.1'/>">
>> ^~~~~~~~~
transformed_feature_tests/server_side_rendering.html:53:4 The parent tag of tag 'IMG-I-AMPHTML-INTRINSIC-SIZER' is 'i-amphtml-sizer', but it can only be 'i-amphtml-sizer-intrinsic'.
transformed_feature_tests/server_side_rendering.html:57:4 The parent tag of tag 'IMG-I-AMPHTML-INTRINSIC-SIZER' is 'i-amphtml-sizer', but it can only be 'i-amphtml-sizer-intrinsic'.
| </i-amphtml-sizer>
| </amp-img>
| <!-- Valid -->
| <amp-social-share class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" i-amphtml-layout=fixed style=width:60px;height:44px; type=test></amp-social-share>
| <!-- Invalid i-amphtml-layout attribute value does not match layout value -->
| <amp-img class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" height=2911 i-amphtml-layout=nodisplay layout=responsive src=https://example-com.cdn.ampproject.org/i/s/example.com/lemur-narrow.jpg srcset="https://example-com.cdn.ampproject.org/i/s/example.com/lemur-wide.jpg 640w, https://example-com.cdn.ampproject.org/i/s/example.com/lemur-narrow.jpg 320w" width=1698>
>> ^~~~~~~~~
transformed_feature_tests/server_side_rendering.html:59:2 Invalid value 'nodisplay' for attribute 'i-amphtml-layout' in tag 'amp-img' - for layout 'RESPONSIVE', set the attribute 'i-amphtml-layout' to value 'responsive'. (see https://amp.dev/documentation/components/amp-img)
transformed_feature_tests/server_side_rendering.html:63:2 Invalid value 'nodisplay' for attribute 'i-amphtml-layout' in tag 'amp-img' - for layout 'RESPONSIVE', set the attribute 'i-amphtml-layout' to value 'responsive'. (see https://amp.dev/documentation/components/amp-img)
| <i-amphtml-sizer style=display:block;padding-top:171.4370%;></i-amphtml-sizer>
| </amp-img>
| <!-- Invalid class attribute value due to not matching layout value -->
| <amp-img class="i-amphtml-layout-nodisplay i-amphtml-layout-size-defined" height=2911 i-amphtml-layout=responsive layout=responsive src=https://example-com.cdn.ampproject.org/i/s/example.com/lemur-narrow.jpg srcset="https://example-com.cdn.ampproject.org/i/s/example.com/lemur-wide.jpg 640w, https://example-com.cdn.ampproject.org/i/s/example.com/lemur-narrow.jpg 320w" width=1698>
>> ^~~~~~~~~
transformed_feature_tests/server_side_rendering.html:63:2 The attribute 'class' in tag 'amp-img' is set to the invalid value 'i-amphtml-layout-nodisplay i-amphtml-layout-size-defined'. (see https://amp.dev/documentation/components/amp-img)
transformed_feature_tests/server_side_rendering.html:67:2 The attribute 'class' in tag 'amp-img' is set to the invalid value 'i-amphtml-layout-nodisplay i-amphtml-layout-size-defined'. (see https://amp.dev/documentation/components/amp-img)
| <i-amphtml-sizer style=display:block;padding-top:171.4370%;></i-amphtml-sizer>
| </amp-img>
| <!-- Invalid class attribute value due to layout not being size defined (spaces) -->
| <amp-img class="i-amphtml-layout-nodisplay i-amphtml-layout-size-defined" i-amphtml-layout=nodisplay layout=nodisplay></amp-img>
>> ^~~~~~~~~
transformed_feature_tests/server_side_rendering.html:67:2 The attribute 'class' in tag 'amp-img' is set to the invalid value 'i-amphtml-layout-nodisplay i-amphtml-layout-size-defined'. (see https://amp.dev/documentation/components/amp-img)
transformed_feature_tests/server_side_rendering.html:71:2 The attribute 'class' in tag 'amp-img' is set to the invalid value 'i-amphtml-layout-nodisplay i-amphtml-layout-size-defined'. (see https://amp.dev/documentation/components/amp-img)
| <!-- Invalid class attribute value due to layout not being size defined (tabs) -->
| <amp-img class="i-amphtml-layout-nodisplay i-amphtml-layout-size-defined" i-amphtml-layout=nodisplay layout=nodisplay></amp-img>
>> ^~~~~~~~~
transformed_feature_tests/server_side_rendering.html:69:2 The attribute 'class' in tag 'amp-img' is set to the invalid value 'i-amphtml-layout-nodisplay i-amphtml-layout-size-defined'. (see https://amp.dev/documentation/components/amp-img)
transformed_feature_tests/server_side_rendering.html:73:2 The attribute 'class' in tag 'amp-img' is set to the invalid value 'i-amphtml-layout-nodisplay i-amphtml-layout-size-defined'. (see https://amp.dev/documentation/components/amp-img)
| <!-- Invalid i-amphtml-sizer due to css declarations -->
| <amp-img class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" height=2911 i-amphtml-layout=responsive layout=responsive src=https://example-com.cdn.ampproject.org/i/s/example.com/lemur-narrow.jpg srcset="https://example-com.cdn.ampproject.org/i/s/example.com/lemur-wide.jpg 640w, https://example-com.cdn.ampproject.org/i/s/example.com/lemur-narrow.jpg 320w" width=1698>
| <i-amphtml-sizer style=display:none;padding-bottom:171.4370%;></i-amphtml-sizer>
>> ^~~~~~~~~
transformed_feature_tests/server_side_rendering.html:72:4 CSS syntax error in tag 'I-AMPHTML-SIZER-RESPONSIVE' - the property 'display' is set to the disallowed value 'none'. (see https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/style_pages)
transformed_feature_tests/server_side_rendering.html:76:4 CSS syntax error in tag 'I-AMPHTML-SIZER-RESPONSIVE' - the property 'display' is set to the disallowed value 'none'. (see https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/style_pages)
>> ^~~~~~~~~
transformed_feature_tests/server_side_rendering.html:72:4 The property 'padding-bottom' in attribute 'style' in tag 'I-AMPHTML-SIZER-RESPONSIVE' is disallowed. (see https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/style_pages)
transformed_feature_tests/server_side_rendering.html:76:4 The property 'padding-bottom' in attribute 'style' in tag 'I-AMPHTML-SIZER-RESPONSIVE' is disallowed. (see https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/style_pages)
| </amp-img>
|
| </body>
Expand Down
2 changes: 1 addition & 1 deletion validator/validator-main.protoascii
View file
Open in desktop
Expand Up @@ -4876,7 +4876,7 @@ tags: {
}
attrs: {
name: "src"
value_regex: "data:image\\/svg\\+xml;charset=utf-8,<svg height=\"\\d+\" width=\"\\d+\" xmlns=\"http:\\/\\/www\\.w3\\.org\\/2000\\/svg\" version=\"1\\.1\"\\/>|data:image\\/svg\\+xml;charset=utf-8,<svg height='100' width='300' xmlns='http:\\/\\/www\\.w3\\.org\\/2000\\/svg' version='1\\.1'\\/>"
value_regex: "data:image\\/svg\\+xml;charset=utf-8,<svg height=\"\\d+(\\.\\d+)?\" width=\"\\d+(\\.\\d+)?\" xmlns=\"http:\\/\\/www\\.w3\\.org\\/2000\\/svg\" version=\"1\\.1\"\\/>|data:image\\/svg\\+xml;charset=utf-8,<svg height='\\d+(\\.\\d+)?\' width='\\d+(\\.\\d+)?\' xmlns='http:\\/\\/www\\.w3\\.org\\/2000\\/svg' version='1\\.1'\\/>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized a way to make this more resilient to make this even more resilient to HTML mutations, perhaps the pattern should be modified like so:

value_regex: "data:image\\/svg\\+xml;charset=utf-8,<svg(\\s+(height=[\"']\\d+(\\.\\d+)?[\"']|width=[\"']\\d+(\\.\\d+)?[\"']|xmlns=[\"']http:\\/\\/www\\.w3\\.org\\/2000\\/svg[\"']|version=[\"']1\\.1[\"']))+\\s*(\\/>|></svg>)"

That would allow the order of the attributes to be modified freely. The whitespace would also be more forgiving. (It also combines the two patterns to use ['"] to represent the attributes. Granted this would allow something like width='1" causing a XML syntax error.) It also allows for a closing </svg> tag which actually should solve the problem of HTML minifiers from stripping self-closing slashes.

Regex without escaping:

data:image\/svg\+xml;charset=utf-8,<svg(\s+(height=["']\d+(\.\d+)?["']|width=["']\d+(\.\d+)?["']|xmlns=["']http:\/\/www\.w3\.org\/2000\/svg["']|version=["']1\.1["']))+\s*(\/>|></svg>)

mandatory: true
}
}
Expand Down