Posted by grendzy on January 20, 2011 at 5:43pm
7 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | frontend performance, markup |
Issue Summary
When using hook_css_alter, sometimes extra aggregates are produced. This is because drupal_group_css() expects the items to be sorted such that matching groups are adjacent in the list. However, drupal_sort_css_js ignores 'media' and 'browser' keys, so for example if a print stylesheet appears in between two media=all stylesheets then you get extra aggregates.
Steps to reproduce:
1) Drop this alter hook somewhere:
<?php
function mymodule_css_alter(&$css) {
foreach ($css as $name => $style) {
$css[$name]['group'] = CSS_DEFAULT;
$css[$name]['every_page'] = FALSE;
}
}
?>2) Enable aggregation
Expected behavior: only one media=all stylesheet will be output, since the alter hook has forced them into the same group.
Actual behavior: 3 media=all stylesheets are output (not counting conditional IE styles)
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal_sort_css_js.patch | 2.13 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 31,553 pass(es). | View details | Re-test |
Comments
#1
I don't think we want this. Ideally, we don't want to mess with CSS rule order. If a media="print" stylesheet is weighted between 2 media="all" stylesheets, then I think it's better to allow 3 aggregate files to be produced than to disrespect the weights. In #769226: Optimize JS/CSS aggregation for front-end performance and DX, we introduced a 'every_page' key that has greater priority than weight, but this was done because there was a compelling performance reason to do it, and it appeared to make some sense. However, even that has proven to confuse people: #977844: The impact of 'every_page' within drupal_add_(css|js|library)() is confusing.
#2
Tagging so that people watching the markup queue see this, since any proposed changes to CSS rule order affects them.
#3
Hm, I see what you mean about respecting weights. I dug deeper to see why core would assign such strange weights (such that print / IE styles would be sandwiched in between global styles), and it's because of this in drupal_add_css:
<?php// Always add a tiny value to the weight, to conserve the insertion order.
$options['weight'] += count($css) / 1000;
?>
Here are the groups that are produced by the default install (after applying the above css_alter). You can see that Bartik's print and IE stylesheets break the global styles into 3 groups, and the weights are essentially arbitrary:
----- new group -----themes/bartik/css/layout.css, weight: 0, media: all
themes/bartik/css/style.css, weight: 0.001, media: all
themes/bartik/css/colors.css, weight: 0.002, media: all
----- new group -----
themes/bartik/css/print.css, weight: 0.003, media: print
----- new group -----
misc/ui/jquery.ui.core.css, weight: 0.004, media: all
misc/ui/jquery.ui.theme.css, weight: 0.005, media: all
modules/overlay/overlay-parent.css, weight: 0.006, media: all
modules/system/system.base.css, weight: 0.007, media: all
modules/system/system.menus.css, weight: 0.008, media: all
modules/system/system.messages.css, weight: 0.009, media: all
modules/system/system.theme.css, weight: 0.01, media: all
modules/comment/comment.css, weight: 0.011, media: all
modules/field/theme/field.css, weight: 0.012, media: all
modules/node/node.css, weight: 0.013, media: all
modules/search/search.css, weight: 0.014, media: all
modules/user/user.css, weight: 0.015, media: all
modules/contextual/contextual.css, weight: 0.016, media: all
----- new group -----
themes/bartik/css/ie.css, weight: 0.017, media: all
----- new group -----
themes/bartik/css/ie6.css, weight: 0.018, media: all
----- new group -----
modules/shortcut/shortcut.css, weight: 0.019, media: all
modules/toolbar/toolbar.css, weight: 0.02, media: all
Preserving insertion order is quite handy, but I'm having a hard time imagining a developer wanting insertion order to have a higher precedence than media type or browser. What do you think about sorting by floor(weight), then browser/media, then weight again? That way explicit integer weights could still "break out" of a group, but implicit fractional weights could not.
#4
Here's an updated patch. The main change is sorting by integer weight first, which prevents implicit weights from splitting a group, but still allows explicit weights to break out of a group. Insertion order is still preserved within groups.
I also made the sort callback more compact by just using subtraction to compare numeric values - rather than separate conditions for less than / greater than.
#5
subscribe
#6
subscribe