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

Introduce stylesheet prioritization when determining which to concatenate #2346

Merged
merged 14 commits into from May 23, 2019

Conversation

Projects
None yet
6 participants
@westonruter
Copy link
Member

commented May 18, 2019

Fixes #2322.

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:

  1. Parent theme (non-print) stylesheets
  2. Child theme (non-print) stylesheets
  3. Core stylesheets used by themes (for blocks)
  4. Plugin (non-print) stylesheets
  5. Stylesheets from wp-includes
  6. Additional CSS from Customizer
  7. Styles added by blocks and widgets
  8. Dashicons
  9. Print stylesheets
  10. 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:

image

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.

@westonruter westonruter added the CSS label May 18, 2019

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

@googlebot googlebot added the cla: yes label May 18, 2019

@westonruter westonruter force-pushed the add/css-prioritization branch from ab7a382 to 72b5a57 May 18, 2019

@postphotos

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

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

@westonruter westonruter force-pushed the add/css-prioritization branch from c4c1b56 to 60e4e42 May 19, 2019

@westonruter westonruter marked this pull request as ready for review May 19, 2019

@westonruter westonruter requested review from swissspidy and amedina May 19, 2019

@westonruter

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

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:

image

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:

image

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

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

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

@swissspidy

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

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

This comment has been minimized.

Copy link
Member

commented May 21, 2019

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

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

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 westonruter force-pushed the add/css-prioritization branch from 52131c9 to 1c5a639 May 21, 2019

@westonruter

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

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

westonruter added some commits May 15, 2019

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

@amedina

This comment has been minimized.

Copy link
Member

commented May 22, 2019

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.

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

This comment has been minimized.

Copy link
@swissspidy

swissspidy May 22, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@westonruter

westonruter May 23, 2019

Author Member

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 added some commits May 23, 2019

@westonruter

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

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

@swissspidy

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

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

What about the remaining todos in the code?

@westonruter

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

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
Copy link
Collaborator

left a comment

Let's get this out for some testing 💥

@westonruter westonruter merged commit e49afb7 into develop May 23, 2019

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@swissspidy swissspidy deleted the add/css-prioritization branch May 23, 2019

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.