-
Notifications
You must be signed in to change notification settings - Fork 382
Update allowed tags/attributes from spec in amphtml 2004142326360 #4548
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.
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
Conversation
| 'tag_spec' => array( | ||
| 'amp_layout' => array( | ||
| 'supported_layouts' => array( | ||
| 5, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 'error_message' => 'CSS !important', | ||
| 'regex' => '!\\s*important', |
There was a problem hiding this comment.
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:
This was introduced in ampproject/amphtml#27412.
The allowImportant property is defined as being false by default for the CssSpec:
* Add support for blacklisted_cdata_regex being plural. * Remove obsolete INVALID_CDATA_CSS_IMPORTANT error code. * Add new INVALID_CDATA_CSS_I_AMPHTML_NAME error code. Fixes #4319
| private static $layout_allowed_attrs = array( | ||
| 'data-amp-bind-height' => array(), | ||
| 'data-amp-bind-width' => array(), | ||
| 'disable-inline-width' => array(), |
There was a problem hiding this comment.
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'] ); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…phtml-2004041903580 * 'develop' of github.com:ampproject/amp-wp: (48 commits) Bump https-proxy-agent from 2.2.2 to 2.2.4 (#4596) Update dependency babel-jest to v25.3.0 (#4550) Update dependency core-js to v3.6.5 (#4558) For the 'Preview AMP' button, use a title instead of a tooltip (#4601) Bump stable tag to 1.5.3 Fix handling of Mustache templates (#4583) Stub request based on test scenario (#4588) Return early instead of storing eventual return value in variable Improve phpdoc and logic conditions Update links in pull request template Update contributing.md with link to wiki Remove engineering.md now that it is on the wiki Remove project-management.md since only applicable to Stories Add conditions for comment feed, trackback, robots, and favicon Fix typo in global phpdoc Update tests after block-library/style.css changes in Gutenberg 7.9 (#4579) Remove special conditions for Reader mode; remove need for $exit condition in redirects Fix translators comment Add comment explaining short-circuit behavior when query var is present Update version for _doing_it_wrong() from 1.5.3 to 1.6.0 ...
pierlon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
| private static $layout_allowed_attrs = array( | ||
| 'data-amp-bind-height' => array(), | ||
| 'data-amp-bind-width' => array(), | ||
| 'disable-inline-width' => array(), |
There was a problem hiding this comment.
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.
Cherry-picks commit from #4548
Cherry-picks commit from #4548
Previously #4390.
./bin/amphtml-update.sh(lando ssh -c 'bash ./bin/amphtml-update.sh vendor/amphtml')Changelog
validator-css.protoascii.blacklisted_cdata_regexas a list instead of a single value, ensuring that for CSS theCSS i-amphtml- name prefixconstraint (with new error codeINVALID_CDATA_CSS_I_AMPHTML_NAME) is applied in addition tohtml commentsfor CDATA. Fixes Multiple denylist patterns are not applied to CDATA validation #4319. Relates to Validation errors are not reported for “i-amphtml” appearing in CSS #771 where such class names need to be removed from elementclassattributes as well.CSS !importantconstraint which the validator now internalizes (removingINVALID_CDATA_CSS_IMPORTANTerror code), and which the AMP plugin also prevents from occurring via the style sanitizer's\AMP_Style_Sanitizer::transform_important_qualifiers()method.facetimeto the list of allowed link protocols.amp-stateas a URL protocol foramp-list.data-ampdevmodeattribute exclusionary rule which prevented the attribute from being passed through the sanitizers, which the AMP plugin intentionally ignores to suppress validation errors.aspect-ratioattribute toamp-story-grid-layer.data-amp-autocomplete-opt-inattribute to be invalid on thehtmlelement. (This only applies to AMP4Email. See Enable amp-autocomplete in AMP4Email with doc level opt-in amphtml#27174.)disable-inline-widthlayout attribute. This will allow us to discontinue removing thesizesattribute onamp-img. See Update allowed tags/attributes from spec in amphtml 2004142326360 #4548 (comment). Follow-up issue: Discontinue removing sizes attribute when converting img to amp-img #4606.Details
( PREV_VERSION=2003031842100; THIS_VERSION=2004142326360; git checkout $THIS_VERSION; git diff $PREV_VERSION...$THIS_VERSION -w -- $( git ls-files | grep '.protoascii' ); git checkout - > /dev/null )2003031842100...2004142326360