Introduce stylesheet prioritization when determining which to concatenate by westonruter · Pull Request #2346 · ampproject/amp-wp

westonruter · 2019-05-18T06:18:45Z

A theme's stylesheets should never be excluded in favor of the admin bar CSS. Similarly, print stylesheets should never be included at the expense of plugin stylesheets. This PR introduces prioritization when determining which stylesheets to include and which to exclude when the tree shaker is not able to reduce the total CSS to the 50KB limit. The changes here should result in a drastic reduction in the frequency of a theme appearing completely broken when viewing in Native/Transitional modes.

The stylesheet priorities are:

Parent theme (non-print) stylesheets
Child theme (non-print) stylesheets
Core stylesheets used by themes (for blocks)
Plugin (non-print) stylesheets
Stylesheets from wp-includes
Additional CSS from Customizer
Styles added by blocks and widgets
Dashicons
Print stylesheets
Admin bar CSS

If it so happens that the Admin Bar CSS is excluded, then the HTML element for the admin bar as well as its associated CSS are both excluded.

Since the admin bar is automatically excluded, there is now no need for the option and it is removed:

TODO

Add additional tests for prioritization.
Remove admin bar when either admin-bar-css or admin-bar-inline-css is excluded.

Other items for follow-up

Add indication that admin bar is hidden?
How to indicate that CSS is truncated when admin bar is hidden?
Ensure admin bar print style is added as inline.
Include any CSS stylesheet which has admin-bar as dependency?
Or skip sanitizing the admin bar at all? Exempt processing of any stylesheet that targets admin bar selectors.
Hide admin bar when doing Validation request.
What to do about dashicons.
Tree Shaker must exempt admin-bar.css
Omit dashicons from tree Shaker and processing if there are no dashicon classes used in the document?
Remove admin bar before sanitizers are run, and the restore after all run. Add dirty styles rule for admin bar CSS when authenticated.

postphotos · 2019-05-18T19:51:54Z

Awesome! Excited to see this. It'll help a lot when working with excessive CSS issues.

westonruter · 2019-05-19T05:07:06Z

I'm not sure what to do about these two points:

Add indication that admin bar is hidden?

How to indicate that CSS is truncated when admin bar is hidden?

There is an indication now but it is just an HTML comment, so it could still be disorienting to a user who expects the admin bar to be present. Otherwise, the way that a user could discover that the admin bar is being excluded by re-validating the URL via the compatibility tool:

On Native mode in particular, they could decide to reject the validation error to cause the admin bar to appear, though with the warning as expected:

This would not be advisable in Transitional mode because the rejected state would then cause the amphtml link to be hidden. Perhaps this is all getting beyond the scope of this issue and should be addressed in #1921, #2326, and/or #1801.

westonruter · 2019-05-19T05:22:50Z

Build for testing: amp.zip (v1.2-alpha-20190519T052057Z-60e4e427)

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

swissspidy · 2019-05-21T21:45:25Z

This is looking sharp! I am not too familiar with the sanitizers yet, especially the one for stylesheets. That's why I cannot give detailed feedback on the general approach and the todos you added. But after going through the code and testing it with a few themes, this seems to work as expected.

Happy to give it a more thorough spin tomorrow though. Alternatively, I imagine that @kienstra might know this area better than I and could also provide some more valuable feedback? Correct me if I'm wrong 🙂

Perhaps this is all getting beyond the scope of this issue and should be addressed in #1921, #2326, and/or #1801.

Agreed, let's take this step by step.

amedina · 2019-05-21T21:49:39Z

Regarding:

Add indication that admin bar is hidden?
How to indicate that CSS is truncated when admin bar is hidden?

Could we show a "watered down" admin bar? Something along the lines of an empty Admin bar with a placeholder text indicating the truncation.

westonruter · 2019-05-21T22:11:16Z

Could we show a "watered down" admin bar? Something along the lines of an empty Admin bar with a placeholder text indicating the truncation.

If we did this we'd still need to include the CSS for the admin bar, at least a subset of it. And the excessive_css is the problem in the first place. For the best user experience, I think the admin bar should be “invisible” for purposes of validation (as noted in #1921) since the admin bar would not appear for any visitors. It doesn't make sense to raise an excessive_css validation error for it.

I was giving it some more thought last night about what do do and I'm thinking this: if the user is authenticated, we should skip #admin-bar-css and #admin-bar-inline-css from being included in the total CSS count. When we do this, we must still keep track of the fact that we skipped this, as we should probably make a comment in the AMP admin bar explaining the validation is only due to the admin bar and it would not be seen by Google. Also, the total byte size of the admin bar CSS could should be added to the total_bytes for the custom CSS so that the final validating sanitizer doesn't strip them out.

I guess this is just re-hashing #1921.

westonruter · 2019-05-21T22:37:12Z

Rebased with changes from develop. Make sure you git fetch && git reset --merge origin/add/css-prioritization.

…luded

amedina · 2019-05-22T01:22:36Z

If we did this we'd still need to include the CSS for the admin bar, at least a subset of it. And the excessive_css is the problem in the first place.

Comment was related to "how to provide an indicator" that the admin bar is hidden.

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

includes/class-amp-theme-support.php

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

swissspidy · 2019-05-22T15:11:02Z

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

+				$priority = 70;
+			}
+
+			if ( 'print' === $node->getAttribute( 'media' ) ) {


The media attribute in <link> element can be either a single media type, a white-space-separated list of media types, or a complex media query.

Source: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#Attributes

So this simple equality check might not be enough. Although the more complex media declarations might not be very common, so perhaps it's not a big deal.

Good point. In the case of a non-scalar media then the stylesheet would just be handled as a screen priority, which isn't the worst case. Since a simple media value is going to be the vast majority of cases, I don't think we need to worry about it here. But it's good to keep in mind.

westonruter · 2019-05-23T05:00:25Z

@swissspidy The remaining todos in the description are not for tackling in this PR.

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

swissspidy · 2019-05-23T11:30:36Z

The remaining todos in the description are not for tackling in this PR.

What about the remaining todos in the code?

westonruter · 2019-05-23T13:49:57Z

What about the remaining todos in the code?

There are three and they are largely for future reference, so they don't need (or can't be) done yet.

swissspidy

Let's get this out for some testing 💥

westonruter added the CSS label May 18, 2019

westonruter added this to the v1.2 milestone May 18, 2019

googlebot added the cla: yes Signed the Google CLA label May 18, 2019

westonruter force-pushed the add/css-prioritization branch from ab7a382 to 72b5a57 Compare May 18, 2019 06:26

westonruter force-pushed the add/css-prioritization branch from c4c1b56 to 60e4e42 Compare May 19, 2019 04:28

westonruter marked this pull request as ready for review May 19, 2019 04:56

westonruter requested review from amedina and swissspidy May 19, 2019 04:56

swissspidy reviewed May 21, 2019

View reviewed changes

includes/sanitizers/class-amp-style-sanitizer.php Outdated Show resolved Hide resolved

swissspidy reviewed May 21, 2019

View reviewed changes

includes/sanitizers/class-amp-style-sanitizer.php Outdated Show resolved Hide resolved

westonruter force-pushed the add/css-prioritization branch from 52131c9 to 1c5a639 Compare May 21, 2019 22:36

westonruter added 9 commits May 21, 2019 16:19

Introduce stylesheet priorities to ensure important styles aren't exc…

ef52c80

…luded

Eliminate IE hacks for admin-bar bump CSS

5a52afb

Remove admin bar element and associated styles if excessive CSS

7c26db0

Remove obsolete AMP_Style_Sanitizer::$stylesheets var

449b9dc

Eliminate the option to disable the admin bar

121c677

Exclude both admin-bar stylesheets if either one is excluded

cfeada6

Add test for stylesheet prioritization

d8ead1c

Prevent element ID and class from being duplicated in CSS manifest

ae3c140

Extract admin bar selector from translated string; remove todo

ee9fbb5

westonruter force-pushed the add/css-prioritization branch from 1c5a639 to ee9fbb5 Compare May 21, 2019 23:23

westonruter commented May 22, 2019

View reviewed changes

includes/sanitizers/class-amp-style-sanitizer.php Show resolved Hide resolved

kienstra reviewed May 22, 2019

View reviewed changes

includes/class-amp-theme-support.php Show resolved Hide resolved

kienstra reviewed May 22, 2019

View reviewed changes

includes/sanitizers/class-amp-style-sanitizer.php Show resolved Hide resolved

swissspidy reviewed May 22, 2019

View reviewed changes

westonruter added 4 commits May 22, 2019 17:08

Improve error messages for CSS @import failures

fd23dc0

Fix bug with replacing @import with contents

ab40437

Fix bug with non-rejected @import statements being dropped

f7f02ac

Add tests for fixes to @import handling

9946158

swissspidy reviewed May 23, 2019

View reviewed changes

includes/sanitizers/class-amp-style-sanitizer.php Outdated Show resolved Hide resolved

Remove unused closure import

a473956

swissspidy approved these changes May 23, 2019

View reviewed changes

westonruter merged commit e49afb7 into develop May 23, 2019

swissspidy deleted the add/css-prioritization branch May 23, 2019 14:22

westonruter mentioned this pull request May 23, 2019

Remove admin-bar class name from body when admin bar element removed due to excessive CSS #2405

Merged

This was referenced Jun 4, 2019

Ignore admin bar for AMP validation purposes #1921

Closed

Suppress admin bar from being shown during validation requests #2500

Merged

westonruter mentioned this pull request Sep 5, 2019

Integrate admin bar with AMP dev mode #3187

Merged

17 tasks

westonruter mentioned this pull request Sep 12, 2019

Style sanitizer contains a piece of logic that doesn't seem to have any effect #3232

Closed

schlessera mentioned this pull request Sep 13, 2019

Remove unused piece of code in Style Sanitizer #3235

Merged

schlessera mentioned this pull request Oct 8, 2019

Remove validation handling options with the checkbox to disable auto-accepting sanitization #3350

Closed

westonruter mentioned this pull request Dec 11, 2019

Allow CSS larger than 50k if 90% used ampproject/amphtml#4555

Closed

Conversation

westonruter commented May 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Other items for follow-up

Uh oh!

postphotos commented May 18, 2019

Uh oh!

westonruter commented May 19, 2019

Uh oh!

westonruter commented May 19, 2019

Uh oh!

Uh oh!

Uh oh!

swissspidy commented May 21, 2019

Uh oh!

amedina commented May 21, 2019

Uh oh!

westonruter commented May 21, 2019

Uh oh!

westonruter commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amedina commented May 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swissspidy May 22, 2019 • 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!

westonruter May 23, 2019

Choose a reason for hiding this comment

Uh oh!

westonruter commented May 23, 2019

Uh oh!

Uh oh!

swissspidy commented May 23, 2019

Uh oh!

westonruter commented May 23, 2019

Uh oh!

swissspidy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

westonruter commented May 18, 2019 •

edited

Loading

westonruter commented May 21, 2019 •

edited

Loading

swissspidy May 22, 2019 •

edited

Loading