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

Add runtime CSS minification, !important replacement, and tree shaking #1048

Merged
merged 32 commits into from Apr 6, 2018

Conversation

@westonruter
Copy link
Member

westonruter commented Mar 30, 2018

  • Add support for Server Timing to reveal the time required for the plugin to render a response. Move header-sending logic to AMP_Response_Headers class. See #990.
  • Minify CSS to remove whitespace and comments. See #688.
  • Sanitize CSS by means of a CSS parser instead of relying on regular expressions.
  • Remove illegal CSS properties (-moz-binding and behavior).
  • Reduce length of CSS class names generated for style attributes.
  • If amp-keyframes appears anywhere but end of body, just move it to the end of the body.
  • Implement the replace-important approach to eliminating !important qualifiers.
  • Delete illegal CSS @-rules.
  • Make sure css_spec gets parsed and included in AMP_Allowed_Tags_Generated.
  • Limit @keyframes to opacity, transform and -vendorPrefix-transform.
  • Make sure legacy post template still renders properly with stylesheet changes.
  • Delete CSS rules that don't apply to the current page based on the class names used, if the total CSS size would otherwise be greater than 50KB. (CSS tree shaking.)
  • Delete selectors that reference class names that don't exist.
  • Handle case of false negatives when a selector contains :not(.classname). Also strip strings.
  • Figure out how to gracefully deal with Genericons and Dashicons.
  • Use expiring transient instead of indefinite cache when external cache is not available.
  • Improve handling of specificity, including giving higher selector specificity to rules from inline styles. Reduce length of fake ID.
  • Address relative URLs, such as for background images.

Fixes #990.
Fixes #930.
Fixes #688.
Closes #610.

Ideas for later:

  • Look at other tree-shaking methods beyond class names.
  • Add tree-shaking based on IDs.

@westonruter westonruter added this to the v1.0 milestone Mar 30, 2018

@westonruter westonruter self-assigned this Mar 30, 2018

westonruter added some commits Mar 26, 2018

Use sabberworm/php-css-parser to sanitize CSS
* Compress CSS output with compact format, removing whitespace and comments.
* Reduce length of generated class names for inline styles.
* Unify logic for handling inline style attributes with handling stylesheets in style/link.
Transform !important property qualifiers into rules with more specifi…
…c selectors (following replace-important package)

Eliminate illegal_css_important_qualifier validation error now that transformed

@westonruter westonruter force-pushed the add/css-tree-shaking branch from 73ad61a to fd04b20 Mar 31, 2018

westonruter added some commits Mar 31, 2018

Add parsing and validation of keyframes
* Handle CSS parse errors.
* Let spec inform which @-rules are allowed.
* Improve handling of vendor-prefixed properties in whitelist/blacklists.
* Inform when !important qualifier is removed from rules with selectors (that aren't keyframes).
$selectors = array();
if ( $should_tree_shake ) {
foreach ( $selectors_parsed as $selector => $class_names ) {
if ( count( $class_names ) === count( array_intersect( $class_names, $this->used_class_names ) ) ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 1, 2018

Author Member

Better to use array_diff here instead maybe.

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Apr 5, 2018

the point you make in the doc about amp-live-list is a key one: content could be added to the page later that depends on a rule that is filtered

Just occurred to me that we could help guard against this from happening by adding logic to skip removing selectors that reference amp-live-list or amp-list elements. Also, we could skip selectors that reference div[submit-success] and div[submit-error].

Nevertheless, there would be no way to statically account for class names assigned via amp-bind, so that's a danger, unless we adopt some some naming convention for amp-bind-supplied class names used in selectors.

@pbakaus Any other key dynamic content cases to have in mind?

@westonruter westonruter force-pushed the add/css-tree-shaking branch from b278bc2 to 9a6cec0 Apr 5, 2018

@westonruter westonruter force-pushed the add/css-tree-shaking branch from 9a6cec0 to 313f0b9 Apr 5, 2018

@pbakaus

This comment has been minimized.

Copy link
Collaborator

pbakaus commented Apr 5, 2018

@westonruter sounds sensible! Thats the only ones I have in mind right now.

@camelburrito

This comment has been minimized.

Copy link

camelburrito commented Apr 6, 2018

@westonruter usually the ones associated with amp-bind can be searchable in the dom (string search), even that would be a little risky because there could be string concat etc happening.
Naming conventions seems to be a sane idea. I guess these could be optional flag that we can enable (like list of prefixes that should not be deleted?)

And on the replace-important improvements feel free to edit the code here
https://github.com/ampproject/ampstart/tree/master/tools/replace-important
I can help you push it live. If that would help you keep it more modular.

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Apr 6, 2018

I guess these could be optional flag that we can enable (like list of prefixes that should not be deleted?)

@camelburrito I added initial support for this via the dynamic_element_selectors argument in
bede9de. By default it only includes the known dynamic content ancestors, but a given theme could amend it with additional dynamic selectors to exclude from removal.

@kienstra This is ready for review to merge so we can get this testing. I want to follow up later with some more improvements including the added ability to knowingly bypass removal of elements, attributes, and styles that are invalid but which a given site cannot afford to be removed. I'm thinking we'd want CSS over the 50KB limit to default to not be removed, even if that means it is invalid AMP. In terms of implementation here, I think this can be implemented by allowing the validation_error_callback to return false as a way to prevent removal from happening. That would allow a theme/plugin to decide on a case-by-case basis which things should get removed. This is in regards to #1048 (review) and #1048 (comment)

@westonruter westonruter assigned kienstra and unassigned westonruter Apr 6, 2018

@kienstra
Copy link
Collaborator

kienstra left a comment

Approved

Hi @westonruter,
This PR looks good, and it's approved. There's a small point about test_body_style_attribute_sanitizer(), but it's not a blocker.

Also:

allowing the validation_error_callback to return false as a way to prevent removal from happening

That sounds good, finalize_stylesheet_set() could check the return value of the validation_error_callback before removing the stylesheet.

* @type callable $validation_error_callback Function to call when a validation error is encountered.
* }
*/
protected $args;

This comment has been minimized.

Copy link
@kienstra

kienstra Apr 6, 2018

Collaborator

Nice DocBlock for this.

array(),
),
'styles_with_dynamic_elements' => array(
implode( '', array(

This comment has been minimized.

Copy link
@kienstra

kienstra Apr 6, 2018

Collaborator

Great idea to use implode() here for the long document.

*/
public function test_body_style_attribute_sanitizer( $source, $expected_content, $expected_stylesheets ) {
public function test_body_style_attribute_sanitizer( $source, $expected_content, $expected_stylesheets, $expected_errors = array() ) {

This comment has been minimized.

Copy link
@kienstra

kienstra Apr 6, 2018

Collaborator

Maybe the 3rd $expected_errors parameter isn't necessary, given that the dataProvider get_link_and_style_test_data always has a 3rd array item: array(). But this isn't a blocker to approving.

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 6, 2018

Author Member

Yeah, you're right. But I suppose it aligns with the other tests and opens the door to testing for validation errors in the future.

@kienstra kienstra assigned westonruter and unassigned kienstra Apr 6, 2018

@westonruter westonruter merged commit 40d1945 into develop Apr 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/css-tree-shaking branch Apr 6, 2018

@postphotos postphotos added this to Definition in v1.0 May 22, 2018

@westonruter westonruter moved this from Definition to Ready for QA in v1.0 May 22, 2018

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Jun 6, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.