Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#35229 closed enhancement (fixed)

Stop generating `wp-admin.min.css`

Reported by: dd32's profile dd32 Owned by: dd32's profile dd32
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-unit-tests has-patch
Focuses: administration Cc:

Description (last modified by dd32)

Currently WordPress generates and ships relatively large 235K wp-admin.min.css and wp-admin-rtl.min.css files which are created from source files which we also ship.

I'd like to propose that we stop generating these files, and instead rely upon load-styles.php to combine them.

My main reason is that I'd love for commits such as [35896] not to cause 510KB of CSS to require be shipped to end users (That's the two 235K wp-admin files, plus dashboard.css/rtl). Instead, we'd only have to ship the 4 dashboard.css files which is around 72K.

Package size for regular releases won't be changed significantly, full package sizes would increase by 88K as we'd no longer get the benefit of the CSS minification process combining some CSS rules which are currently in separate files. (Currently wp-admin/css is 2,240KB and would be 2,328KB after this proposal)

We'd still need to generate wp-admin.min.css and friends as back-compat for those who include it directly, although script-loader.phps dependencies will take care of wp_enqueue_style( 'wp-admin' ).

Attached is a patch which does:

  • Alters the Gruntfile to just create minified versions of all wp-admin/css files
  • Creates wp-admin.min.css, wp-admin-rtl.css, and wp-admin-rtl.min.css manually (ie. not through cssmin) as we only need to do string replacements on the contents.
  • Adds all the individual wp-admin.css @imports to script-loader.php
  • Allows for load-styles.php to accept load[] like load-scripts.php to work around long-query-string limitations on some servers

The only real downside of doing this is that we're effectively shifting from pre-compute to on-demand-on-millions-of-sites concatenation, not a huge deal IMHO as we set cache headers onload-styles.php for 1 year already.
It also shows how much needless CSS we load on all admin pages.

Attachments (7)

35229.diff (10.3 KB) - added by dd32 8 years ago.
35229.2.diff (16.4 KB) - added by dd32 8 years ago.
35229.3.diff (7.0 KB) - added by azaozz 8 years ago.
35229.4.diff (17.8 KB) - added by dd32 8 years ago.
35229.5.diff (3.9 KB) - added by dd32 8 years ago.
35229.6.diff (4.5 KB) - added by ocean90 8 years ago.
35229.7.diff (5.0 KB) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (30)

@dd32
8 years ago

#1 @dd32
8 years ago

  • Description modified (diff)

@dd32
8 years ago

#2 @dd32
8 years ago

Package size for regular releases won't be changed significantly, full package sizes would increase by 88K as we'd no longer get the benefit of the CSS minification process combining some CSS rules which are currently in separate files. (Currently wp-admin/css is 2,240KB and would be 2,328KB after this proposal)

Turns out that the increase was partly due to login.css @importing forms.css and l10n.css.

35229.2.diff instead moves those into the script loader too, and ultimately causes wp-login.php to use wp-admin/load-styles.php.
I'm a little hesitant to introduce a reliance upon wp-admin php to wp-login.php, however we're already including wp-admin/css/* so the harm seems minimal (plus we already treat the login as "admin" in other ways).

35229.2.diff also added the "new" styles to $rtl_styles which was missed in the first iteration. After this new change including wp-login.php the size difference is down to 40K (current: 2,240K, after: 2,280K)

Last edited 8 years ago by dd32 (previous) (diff)

@azaozz
8 years ago

#3 @azaozz
8 years ago

I may be missing something but why move all styles to the bottom in wp-login.php. That can cause a flicker of the unstyled content on slower connections and is generally not recommended. Seems we can load all needed styles in the head there. We used to load four, now we are loading six.

35229.3.diff is almost identical to 35229.2.diff except it loads all styles in the head on wp-login.php and rearranges the dependencies to load the stylesheets in exactly the same order.

#4 @dd32
8 years ago

I may be missing something but why move all styles to the bottom in wp-login.php

That was mostly a bad implementation, I couldn't figure out why they were loading in the footer rather than header. I think I was missing an action I needed to add to cause the styles to print on the head.

I see that you've removed if ( ( ! is_admin() && ! did_action( 'login_init' ) ), which has caused the styles to go back to the header, but not via load-styles.php.

#5 @dd32
8 years ago

35229.4.diff works off 35229.3.diff's ordering and restores the usage of load-styles.php.

It adds the missing add_action( 'login_head', 'print_admin_styles' ); call which was needed to print the styles in the header rather than the footer.

Now a production site will load 2 CSS files, load-styles.php and open sans, both in the header. A development site will of course have multiple files loaded.

@dd32
8 years ago

#6 @azaozz
8 years ago

35229.4.diff works better. We still have to ensure that 'open-sans', 'dashicons' are the first and second dependencies so they are loaded before the rest of the wp-admin or wp-login stylesheets.

Also I'd move add_action( 'login_head', 'print_admin_styles', 9 ); before add_action( 'login_head', 'wp_print_head_scripts', 9 ); because of the general "stylesheets before scripts" rule. This action might eventually cause some stylesheets added by plugins and intended for wp-admin to be outputted on wp-login, but thinking the risk of breaking something there is almost non-existent.

Last edited 8 years ago by azaozz (previous) (diff)

#7 @dd32
8 years ago

because of the general "stylesheets before scripts" rule

Wasn't aware of that..

This action might eventually cause some stylesheets added by plugins and intended for wp-admin to be outputted on wp-login, but thinking the risk of breaking something there is almost non-existent.

I'm not really worried about that, is_admin() isn't true, admin_init hasn't fired, and the admin_head_* + admin enqueue actions haven't fired.

#8 @dd32
8 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

In 36341:

CSS: Stop using wp-admin.min.css and instead queue the individual stylesheets up through load-styles.php.
We still generate the wp-admin.* files for compabitility purposes, however they only include the @import() lines.

Fixes #35229

#9 @dd32
8 years ago

In 36342:

CSS: Reference the original location of the CSS rule being overridden.

See #35229

#10 @ocean90
8 years ago

For posterity: Last week I gave postcss-import + css-mqpacker + cssnano a try. These PostCSS processors are a replacement for grunt-contrib-cssmin.
Long story short: The size difference was very, very small, mainly caused by combining the media query rules.

#11 @ocean90
8 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

This seems to break plugins which are adding inline styles to the wp-admin handle like Polldaddy or Jetpack. Simple example:

<?php
add_action( 'admin_init', function() {
        wp_add_inline_style( 'wp-admin', 'a { color: red }' );
} );

@dd32
8 years ago

#12 @dd32
8 years ago

Part of me feels like that's a wontfix - don't add data to a script that's not your own, but i don't like breaking things either :)

35229.5.diff implements a fix by allowing inline css to be added to a srcless style. It may break other things though.

This ticket was mentioned in Slack in #meta by ocean90. View the logs.


8 years ago

@ocean90
8 years ago

#14 @ocean90
8 years ago

  • Keywords has-unit-tests added

Attached 35229.6.diff with two tests. test_wp_register_script_with_handle_without_source() is currently failing:

1) Tests_Dependencies_Scripts::test_wp_register_script_with_handle_without_source
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 '<script type='text/javascript' src='http://example.com?ver=1'></script>
 <script type='text/javascript' src='http://example.com?ver=2'></script>
+<script type='text/javascript' src='?ver=4.5-alpha-35776-src'></script>
 '

@ocean90
8 years ago

#15 @ocean90
8 years ago

  • Keywords has-patch added; needs-patch removed

35229.7.diff adds a ! $obj->src check to WP_Scripts->do_item() to fix the failing test.

#16 @ocean90
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 36550:

Script/Style Dependencies: Make sure that inline styles for handles without a source are printed.

This prevents breaking plugins which are adding inline styles to the wp-admin handle after [36341].

Props dd32, ocean90.
Fixes #35229.

#17 @azaozz
8 years ago

In 36551:

Styles:

  • Restore loading order for wp-admin: open-sans, dashicons, etc.
  • Remove couple of redundant dependencies.

See #35229.

#18 @ocean90
8 years ago

In 36590:

Styles: Bail if WP_Styles::_css_href() returns an empty value.

The style colors gets registered with true as the source value which gets handled later by wp_style_loader_src(), a callback for the style_loader_src filter in WP_Styles::_css_href(). wp_style_loader_src() may return false, for example for the default color scheme.

This was removed in [36550].

See #35229.

#19 @afercia
8 years ago

In 36869:

CSS: Rename the handle for deprecated-media.css after [36341].

The media handle is now used for media.css thus the stylesheet
for the old media UI needs a different handle name.

See #35229.

#20 follow-up: @dd32
8 years ago

@afercia @ocean90 After this change, any plugins which were registering a stylesheet which relied upon the media handle (deprecated-media.css), will now be getting the wrong stylesheet loaded.

Should we perhaps have named the new media handle something else instead?

#21 in reply to: ↑ 20 @afercia
8 years ago

Replying to dd32:

@afercia @ocean90 After this change, any plugins which were registering a stylesheet which relied upon the media handle (deprecated-media.css), will now be getting the wrong stylesheet loaded.

Should we perhaps have named the new media handle something else instead?

@dd32 yeah, we discussed a bit this on Slack and maybe wrongly assumed deprecated-media.css was always loaded?

#22 @dd32
8 years ago

we discussed a bit this on Slack and maybe wrongly assumed deprecated-media.css was always loaded?

In the media iframes it's always loaded, but that doesn't mean people didn't mark it as a dependency of their own stylesheets.

I'm not too concerned about breakage, as it really was 10 releases back..

#23 @ocean90
8 years ago

In 36884:

Script Loader: Enqueue the minified version of farbtastic if SCRIPT_DEBUG is false.

The files are available since [36341].

See #36083, #35229.

Note: See TracTickets for help on using tickets.