Add runtime CSS minification, !important replacement, and tree shaking by westonruter · Pull Request #1048 · ampproject/amp-wp

westonruter · 2018-03-30T07:18:13Z

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.

* 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.

…oser install in Travis build

…tion instructions

…uring end of body

…c selectors (following replace-important package) Eliminate illegal_css_important_qualifier validation error now that transformed

…are processed

…alidation

… not forbidden

* 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).

…lid class references

westonruter · 2018-04-01T15:55:45Z

includes/sanitizers/class-amp-style-sanitizer.php

+				$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 ) ) ) {


Better to use array_diff here instead maybe.

…ent false matches

…umed) file URLs

kienstra

Great Work

Hi @westonruter,
This PR is a big step forward for the plugin. It opens a lot of possibilities, including Gutenberg support.

As you know, the Gutenberg build/style.css file was too big for the 50 KB limit before.

It looks like there's still some discussion here. I added a few minor points, but I'm happy to approve this when it's ready.

kienstra · 2018-04-02T20:53:26Z

amp.php

+ *
+ * @param array $query_vars Query vars.
+ * @return array Query vars.
+ */


Nice improvements here with the DocBlocks and @since tags.

kienstra · 2018-04-02T21:36:57Z

includes/sanitizers/class-amp-allowed-tags-generated.php

 						'error_message' => 'CSS !important',
 						'regex' => '!important',
 					),
+					'css_spec' => array(


This corresponds well to the css_spec in the amphtml repo's validator-main.protoascii.

kienstra · 2018-04-02T23:04:13Z

includes/class-amp-response-headers.php

+/**
+ * Class AMP_Response_Headers
+ */
+class AMP_Response_Headers {


It's nice you you put this in a separate class 👍

kienstra · 2018-04-03T01:47:43Z

tests/test-class-amp-response-headers.php

@@ -0,0 +1,116 @@
+<?php
+/**
+ * Tests for Theme Support.


This DocBlock should probably be Tests for AMP_Response_Headers, and the same below.

kienstra · 2018-04-03T02:47:01Z

includes/sanitizers/class-amp-style-sanitizer.php

+			}
 		}
+
 		return $stylesheet;


Would it help to call the validation_error_callback here if $stylesheet is empty and it's from a <link>? This would account for when a stylesheet has no rules that pertain to DOM classes. Like if a plugin enqueues a stylesheet for its shortcode on every request.

if ( empty( $stylesheet ) && ( 'link' === $node->nodeName ) && ! empty( $this->args['validation_error_callback'] ) ) { // Call validation callback

We'd probably have to deprecate amp-default.css to do this.

amedina · 2018-04-03T19:20:18Z

@westonruter this packages were developed by @camelburrito (from the UNCSS doc with @pbakaus):

westonruter · 2018-04-03T20:24:20Z

@amedina yes, I referred to replace-important for the implementation here. I think it is improved on here in a couple ways:

It uses _ instead of FK_ID as the dummy ID for the :not() selector. This is important because the :not() has to repeated to achieve the required specificity.
It also accounts for adding specificity to rules like html { color:red !important; } in that it amends :not(#_) to the html root selector, as opposed to adding an additional ancestor selector (which shouldn't be valid). So here this becomes html:not(#_) { color:red; } instead of :root:not(#_) html { color:red; }.

westonruter · 2018-04-05T03:06:27Z

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?

…dd/css-tree-shaking

pbakaus · 2018-04-05T17:25:27Z

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

camelburrito · 2018-04-06T00:44:35Z

@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 · 2018-04-06T07:05:24Z

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)

kienstra

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.

kienstra · 2018-04-06T17:19:51Z

includes/sanitizers/class-amp-style-sanitizer.php

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


Nice DocBlock for this.

kienstra · 2018-04-06T19:22:59Z

tests/test-amp-style-sanitizer.php

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


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

kienstra · 2018-04-06T19:29:31Z

tests/test-amp-style-sanitizer.php

+	 * @param array  $expected_errors      Expected error codes.
 	 */
-	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() ) {


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.

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.

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

westonruter self-assigned this Mar 30, 2018

westonruter added 10 commits March 30, 2018 19:03

Add sabberworm/php-css-parser composer dependency

48a538b

Add initial Server Timing response headers

0e994a9

See #990

Use sabberworm/php-css-parser to sanitize CSS

8271fac

* 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.

Delete illegal properties (behavior and -moz-binding)

a96b39b

Include sabberworm/php-css-parser from composer in build; always comp…

06fca39

…oser install in Travis build

Add check for composer install; update contributing with dev installa…

c27e9a2

…tion instructions

Add tests for AMP_Response_Headers

8ef82f7

Add missing phpdoc for functions in amp.php

e16aba2

Improve handling of style[amp-keyframes] by merging multiples and ens…

036c631

…uring end of body

Transform !important property qualifiers into rules with more specifi…

fd04b20

…c selectors (following replace-important package) Eliminate illegal_css_important_qualifier validation error now that transformed

westonruter force-pushed the add/css-tree-shaking branch from 73ad61a to fd04b20 Compare March 31, 2018 02:04

westonruter added 4 commits March 30, 2018 23:01

Remove illegal @-rules and ensure properties inside @media/@supports …

5da10f3

…are processed

Include css_spec from validator in AMP_Allowed_Tags_Generated

523d5ee

Capture full cdata spec for amp-keyframes and amp-custom to use for v…

1889f70

…alidation

Discontinue deleting scroll/auto values for overflow properties since…

9187710

… not forbidden

This was referenced Mar 31, 2018

Better CSS Sanitization #610

Closed

Remove AMP-illegal overflow values (auto, scroll) xwp/ampnews#82

Closed

westonruter added 5 commits March 31, 2018 16:27

Add parsing and validation of keyframes

3d18b8a

* 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).

Ensure stylesheets are output in legacy post templates

f245a8f

Bump required PHP to 5.3.2 for sake of php-css-parser

6c49807

Add class-based selector tree shaking of CSS rules

3e3266b

Further reduce CSS size by deleting individual selectors that lack va…

6b5f97e

…lid class references

westonruter commented Apr 1, 2018

View reviewed changes

westonruter added 5 commits April 1, 2018 14:05

Prevent :not() pseudo-selectors from tree shaking classes erroneously

ae68eee

Remove attribute selectors prior to searching for class names to prev…

6d4778e

…ent false matches

Simplify accounting for used class names when tree shaking

2693f7e

Make font-face source urls non-relative; replace data: URLs with (ass…

faef934

…umed) file URLs

Use expiring transients when no external object cache present

cb3cc2c

kienstra reviewed Apr 3, 2018

View reviewed changes

amedina requested a review from pbakaus April 3, 2018 19:05

westonruter mentioned this pull request Apr 3, 2018

Add caching of post processor output #959

Closed

westonruter force-pushed the add/css-tree-shaking branch from b278bc2 to 9a6cec0 Compare April 5, 2018 06:08

Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…

313f0b9

…dd/css-tree-shaking

westonruter force-pushed the add/css-tree-shaking branch from 9a6cec0 to 313f0b9 Compare April 5, 2018 06:29

Prevent filtering selectors which reference dynamic elements

bede9de

westonruter added 4 commits April 5, 2018 20:24

Skip removing style rules when the total CSS does not exceed max_bytes

adb1fb1

Verify existence of font file source for data: URL

a8fddf2

Add tests for validation errors raised during style sanitization

3cd9fdd

Add testing for remove_unused_rules=sometimes and excessive_css

efd1965

westonruter assigned kienstra and unassigned westonruter Apr 6, 2018

kienstra approved these changes Apr 6, 2018

View reviewed changes

kienstra assigned westonruter and unassigned kienstra Apr 6, 2018

westonruter merged commit 40d1945 into develop Apr 6, 2018

westonruter deleted the add/css-tree-shaking branch April 6, 2018 23:05

This was referenced Apr 6, 2018

[WIP] Add dynamic handling of validation errors #1063

Closed

@charset isn't being stripped from CSS; causes AMP validation error #1068

Closed

westonruter mentioned this pull request Apr 15, 2018

Bundling Front-end Assets WordPress/gutenberg#5445

Closed

westonruter mentioned this pull request May 3, 2018

Prevent CSS tree shaking from removing selectors with classes mentioned in [class] amp-bind attributes #1111

Merged

kevincoleman added v1.0-beta1 Release labels May 31, 2018

Conversation

westonruter commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amedina commented Apr 3, 2018

Uh oh!

westonruter commented Apr 3, 2018

Uh oh!

westonruter commented Apr 5, 2018

Uh oh!

pbakaus commented Apr 5, 2018

Uh oh!

camelburrito commented Apr 6, 2018

Uh oh!

westonruter commented Apr 6, 2018

Uh oh!

kienstra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

westonruter commented Mar 30, 2018 •

edited

Loading

kienstra left a comment •

edited

Loading

kienstra Apr 2, 2018 •

edited

Loading

kienstra left a comment •

edited

Loading

kienstra Apr 6, 2018 •

edited

Loading