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.

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

Extend style sanitizer to include CSS parser #930

Closed
westonruter opened this issue Feb 5, 2018 · 1 comment · Fixed by #1048
Closed

Extend style sanitizer to include CSS parser #930

westonruter opened this issue Feb 5, 2018 · 1 comment · Fixed by #1048
Assignees
Labels
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Feb 5, 2018

In order to be able to reliably excise !important from styles (#927), we need a proper CSS parser to be able to parse the rules so that higher-specificity selectors can be used instead of !important.

Additionally, a CSS parser used in conjunction with sanitization where the document has been output buffered and parsed into a DOM tree will allow us to strip out all CSS rules from the document that are not relevant to the current page, such as by checking to see if a rule references a class name that does not exist on the current page.

This will also address minification in #688. Class names could be minified in the CSS rules as well as in the document's elements that have the class attributes.

Current work can be seen in the add/css-parser branch. The idea is to use the same phpegjs library that is being used by Gutenberg to parse blocks. The PEG grammar needs to be written for CSS. The grammar currently in the branch needs to be simplified to reduce the granularity. We don't need each token separately parsed for a given selector, but rather just the entire selector string as a whole.

As part of this we should probably augment \AMP_Theme_Support::ensure_required_markup() to ensure the amp-boilerplate is present. Likewise, the way that AMP_WP_Styles waits to output styles should be removed in favor of just outputting them right away to then allow the sanitizer to collect them. The CSS sanitizer can then be responsible actually for adding the style[amp-custom] to the head.

This will supersede and incorporate #610 to switch from blacklist to whitelist. It will also need to improve the approach to scrubbing !important in #927. See #927 (comment).

@westonruter
Copy link
Member Author

In Twenty Fifteen, on a single page that is fully populated with all embeds, widgets, and has a comment in it, the style.css (97KB) coverage as reported by Chrome is:

image

So if we were able to delete all of the unused bytes, the the CSS would be below 50KB.

Additionally, if the CSS were also minified with all whitespace and comments stripped, then this would bring the initial size from 97KB down to 77KB.

Here's a list of the class names used by frequency: https://gist.github.com/westonruter/29c8e0284f39ace5c95cbe7d81546b6d

@westonruter westonruter added this to the v1.0 milestone Apr 6, 2018
@kevincoleman kevincoleman changed the title Extend style sanitizer to incude CSS parser Extend style sanitizer to include CSS parser Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants