Closed
Conversation
…_Allowed_Styles_Generated to validate CSS. Also replaced use of with a custom function which ignores delimiters in parentheses and quotation marks.
18 tasks
Member
|
Closing in favor of #1048. |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@mjangda, This PR is to address issue #604.
Summary:
Replaced
safecss_filter_attr()withsanitize_amp_custom_style()which usesAMP_Allowed_Styles_Generatedto validate CSS. Also replaced use ofexplode()with a custom functioncss_explode()which ignores delimiters in parentheses and quotation marks. Includedamp_wp_build_styles.pyto generateAMP_Allowed_Styles_Generatedclass.Detail:
The original issue was caused by the following:
The sanitizer used 'safecss_filter_attr()' from kses which would filter out any declaration with a
(in it. For example, this would incorrectly remove a declaration that usesbackground:url(...)safe_filter_attr()has been replaced with a new function:sanitize_amp_custom_style()which uses a new class calledAMP_Allowed_Styles_Generatedto validate theamp-customCSS. This new class is generated from the same protocol buffers that AMP uses to produce the official validator.The only rules in this version of the specification that apply to inline styles are:
A list of regular expressions that will flag blacklisted data in the amp-custom styles section. Note that
!importantis included in this blacklist, but there is no rule that disallows variants with whitespace between the!andimportant, thus tests that relate to strings such as! importanthave been removed.A set of rules that specify requirements of image URLs present in the styles.
Notably, the current AMP spec does not blacklist
overflowand it does not specify thatwidthshould be replaced bymax-width, thus these rules are no longer enforced.Note: Even though the AMP website mentions that
overflowmay not be styled asautoorscroll, there is nothing in the AMP validator specification that enforces this and the current version of the JS validator does not flag this use ofoverflowas an AMP error. (Perhaps the AMP web page needs to be updated??) The use ofoverflowseems to be handled by the AMP JS now rather than explicitly disallowing it in CSS.Additionally, the sanitizer was using
explode( ';', $string )to split apart CSS directives in a CSS directive block. This would incorrectly split something like:into three tokens instead of two:
The changes in this PR include a new function
css_explode()that works in a manner similar to php'sexplode(), but it ignores delimiters inside parentheses and quoted strings. This new function is used to break apart CSS directive blocks as well as split individual directives into their property/value pairs inprocess_style().