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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update allowed tags/attributes from spec in amphtml 2004142326360 #4548

Merged
merged 7 commits into from
Apr 21, 2020
Merged

Update allowed tags/attributes from spec in amphtml 2004142326360 #4548

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1e83fde
Update spec parser initially for changes in v2004041903580
westonruter Apr 8, 2020
fac2a76
Omit Jetpack from being activated during PHPUnit test runs
westonruter Apr 8, 2020
2c9c68e
Update handling of blacklisted_cdata_regex
westonruter Apr 8, 2020
b5b2978
Update spec to 2004142326360 to remove container layout from amp-list
westonruter Apr 18, 2020
48a8a55
Merge branch 'develop' of github.com:ampproject/amp-wp into update/am…
westonruter Apr 18, 2020
3b3bfb9
Exclude data-ampdevmode attribute exclusion rule
westonruter Apr 20, 2020
d156e34
Add test for disable-inline-width on amp-img
westonruter Apr 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions bin/amphtml-update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,10 @@ def GenValidatorProtoascii(validator_directory, out_dir):
"""
logging.info('entering ...')

protoascii_segments = [open(os.path.join(validator_directory, 'validator-main.protoascii')).read()]
protoascii_segments = [
open(os.path.join(validator_directory, 'validator-main.protoascii')).read(),
open(os.path.join(validator_directory, 'validator-css.protoascii')).read()
]
extensions = glob.glob(os.path.join(validator_directory, '../extensions/*/validator-*.protoascii'))
extensions.sort()
for extension in extensions:
Expand Down Expand Up @@ -484,7 +487,7 @@ def GetTagSpec(tag_spec, attr_lists):
if 'blacklisted_cdata_regex' in cdata_dict:
if 'error_message' not in cdata_dict['blacklisted_cdata_regex']:
raise Exception( 'Missing error_message for blacklisted_cdata_regex.' );
if cdata_dict['blacklisted_cdata_regex']['error_message'] not in ( 'CSS !important', 'contents', 'html comments' ):
if cdata_dict['blacklisted_cdata_regex']['error_message'] not in ( 'CSS !important', 'contents', 'html comments', 'CSS i-amphtml- name prefix' ):
raise Exception( 'Unexpected error_message "%s" for blacklisted_cdata_regex.' % cdata_dict['blacklisted_cdata_regex']['error_message'] );
tag_spec_dict['cdata'] = cdata_dict

Expand Down
29 changes: 25 additions & 4 deletions includes/sanitizers/class-amp-allowed-tags-generated.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
*/
class AMP_Allowed_Tags_Generated {

private static $spec_file_revision = 1012;
private static $spec_file_revision = 1027;
private static $minimum_validator_revision_required = 375;

private static $descendant_tag_lists = array(
Expand Down Expand Up @@ -566,6 +566,7 @@ class AMP_Allowed_Tags_Generated {
'bbmi',
'chrome',
'itms-services',
'facetime',
'fb-me',
'fb-messenger',
'feed',
Expand Down Expand Up @@ -4070,6 +4071,7 @@ class AMP_Allowed_Tags_Generated {
'allow_relative' => true,
'protocol' => array(
'https',
'amp-state',
),
),
),
Expand All @@ -4079,6 +4081,7 @@ class AMP_Allowed_Tags_Generated {
'tag_spec' => array(
'amp_layout' => array(
'supported_layouts' => array(
5,
Copy link
Member Author

Choose a reason for hiding this comment

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

@schlessera Here's where the container layout is being added for amp-list. I'm not sure if it would change #4541, but FYI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in b5b2978 after updating to updating to v2004142326360.

6,
2,
3,
Expand Down Expand Up @@ -5151,6 +5154,12 @@ class AMP_Allowed_Tags_Generated {
'amp-script' => array(
array(
'attr_spec_list' => array(
'data-ampdevmode' => array(
'blacklisted_value_regex' => 'false',
'value' => array(
'false',
),
),
'max-age' => array(
'value_regex' => '[0-9]+',
),
Expand Down Expand Up @@ -5784,6 +5793,9 @@ class AMP_Allowed_Tags_Generated {
'amp-story-grid-layer' => array(
array(
'attr_spec_list' => array(
'aspect-ratio' => array(
'value_regex' => '\\d+:\\d+',
),
'position' => array(
'value' => array(
'landscape-half-left',
Expand Down Expand Up @@ -8973,7 +8985,14 @@ class AMP_Allowed_Tags_Generated {
),
'html' => array(
array(
'attr_spec_list' => array(),
'attr_spec_list' => array(
'data-amp-autocomplete-opt-in' => array(
'blacklisted_value_regex' => 'false',
'value' => array(
'false',
pierlon marked this conversation as resolved.
Show resolved Hide resolved
),
),
),
'tag_spec' => array(
'mandatory' => true,
'mandatory_parent' => '!doctype',
Expand Down Expand Up @@ -15507,6 +15526,7 @@ class AMP_Allowed_Tags_Generated {
),
array(
'attr_spec_list' => array(
'nonce' => array(),
'type' => array(
'dispatch_key' => 3,
'mandatory' => true,
Expand Down Expand Up @@ -16606,8 +16626,8 @@ class AMP_Allowed_Tags_Generated {
),
'cdata' => array(
'blacklisted_cdata_regex' => array(
'error_message' => 'CSS !important',
'regex' => '!\\s*important',
Comment on lines -16609 to -16610
Copy link
Member Author

Choose a reason for hiding this comment

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

This has moved from the blacklisted_cdata_redex in the validator spec to an allowImportant read by the validator JS:

https://github.com/ampproject/amphtml/blob/f2963066145114b26774747455f1f6ea6caec884/validator/engine/validator.js#L2155-L2168

This was introduced in ampproject/amphtml#27412.

The allowImportant property is defined as being false by default for the CssSpec:

https://github.com/ampproject/amphtml/blob/f2963066145114b26774747455f1f6ea6caec884/validator/validator.proto#L263-L266

'error_message' => 'CSS i-amphtml- name prefix',
'regex' => '(^|\\W)i-amphtml-',
westonruter marked this conversation as resolved.
Show resolved Hide resolved
),
'css_spec' => array(
'allowed_at_rules' => array(
Expand Down Expand Up @@ -18238,6 +18258,7 @@ class AMP_Allowed_Tags_Generated {
private static $layout_allowed_attrs = array(
'data-amp-bind-height' => array(),
'data-amp-bind-width' => array(),
'disable-inline-width' => array(),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is new. Docs: https://amp.dev/documentation/guides-and-tutorials/learn/amp-html-layout/#disable-inline-width

I wonder, does this mean that the sizes attribute can cease to be removed in favor of setting this boolean attribute?

amp-wp/includes/sanitizers/class-amp-img-sanitizer.php

Lines 333 to 334 in 014bbbb

// Remove sizes attribute since it causes headaches in AMP and because AMP will generate it for us. See <https://github.com/ampproject/amphtml/issues/21371>.
unset( $new_attributes['sizes'] );

Copy link
Member Author

Choose a reason for hiding this comment

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

Asked here: ampproject/amphtml#27083 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Answer is yes! ampproject/amphtml#27083 (comment)

So in a subsequent PR we should eliminate the unset() here and instead add the disable-inline-width attribute. This would be done on the develop branch to give it some time to get tested.

Copy link
Contributor

@pierlon pierlon Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is this being tracked in a new issue?

Nevermind I see #4606 has been created.

'height' => array(),
'heights' => array(),
'layout' => array(),
Expand Down