Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Currently, whenever a CSS file is added that uses a media query in the options array it creates a new aggregation group when processed. This introduces a frontend performance issue on sites that use many different media queries as it potentially raises the number of aggregates drastically. For example, if a media query dependent stylesheet is added in the middle of a group of CSS files the current behavior causes the creation of 3 aggregates instead of just one (pre-media-query-stylesheets / media-query-stylesheets / post-media-query-stylesheets).
Proposed resolution
Ignore the media query option of stylesheets that have 'preprocess' set to TRUE when CSS aggregation is enabled and write the media query into the CSS aggregate instead (wrap it around the string content of that file when building the cache).
Remaining tasks
Tests, maybe.
API changes
None. It doesn't have an effect on the weighting of stylesheets either.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 1488910-css-aggregation-media.patch | 5.46 KB | iamcarrico |
| #3 | css-aggregation-media.patch | 3.48 KB | fubhy |
| #2 | css-aggregation-media.patch | 3.5 KB | fubhy |
| css-aggregation-media.patch | 3.57 KB | fubhy | |
Comments
Comment #2
fubhy CreditAttribution: fubhy commentedComment #3
fubhy CreditAttribution: fubhy commentedSame patch but a easier to read.
Comment #4
nod_Good idea but current patch doesn't work. Don't think it takes much to fix it though.
Comment #5
JohnAlbinTagging.
Comment #6
Wim LeersNeeds reroll now that #352951: Make JS & CSS Preprocessing Pluggable has landed.
Definitely a task, not a feature: no APIs changed, just smarter CSS aggregation!
Comment #7
Wim LeersWe *must* do this before D8 release. This is at least a major performance problem. If we don't do it before release, then we must do it in a Drupal 8 point release.
Comment #8
sdboyer CreditAttribution: sdboyer commentedthis is being rolled in as part of #1762204: Introduce Assetic compatibility layer for core's internal handling of assets. we can mark it a dupe once that goes in.
(or just do it now, this is pretty simple and an immediate win)
Comment #9
iamcarrico CreditAttribution: iamcarrico commentedI just revisited this in the current look of things, and added this re-roll.
This patch also updates the current tests, and then adds a new test to make sure this is actually working... which it seems to be.
Comment #10
sdboyer CreditAttribution: sdboyer commentedlooks like a step in the right direction. i should probably review the tests in more detail to ensure they really test the output fully here, but before that...
this is the bit that's been raising some question marks for me. i think this isn't quite right.
it seems like this should be right IF the media type of the containing aggregate asset is either not set, or 'all'. however, if the aggregate is something else, this won't be correct.
and, it is currently possible that the containing asset would have a different media type, as the groups no longer differentiate based on media - which means the aggregate inherits the media property of whatever css asset happens to be the first to pass through it.
the solution, i think, is to either simply enforce that we ALWAYS apply 'all' as the media type on aggregates, or that we remove the conditional in this code block and just *always* attach a media wrapper. and this is where i get fuzzy, because i don't know which is best.
Comment #11
iamcarrico CreditAttribution: iamcarrico commentedRodger, I will check it out and fix.
Comment #12
iamcarrico CreditAttribution: iamcarrico commentedRodger, I will check it out and fix.
Comment #13
MustangGB CreditAttribution: MustangGB commentedsdboyer appears to be re-writing the world in #1488910: Incorporate media queries in aggregated CSS files, don't create a new group for each new media query, whist throwing in this plus with it, keep up the good work.
Comment #14
Wim LeersHe's changing everything in the internals of asset handling. It will NOT affect aggregate generation, besides a few minor changes (typehinted parameters). This patch can still go in. :)
Comment #15
catchThis doesn't seem tied to the release, could do it any time no?
Comment #16
iamcarrico CreditAttribution: iamcarrico commentedIt could--- also: there might be a use case to not doing this at all. I will ponder some use cases in the near future.
Comment #17
joginderpc CreditAttribution: joginderpc at Srijan | A Material+ Company commentedComment #18
Dane Powell CreditAttribution: Dane Powell at Acquia commentedPatch in #9 no longer applies and needs to be re-rolled.
Comment #19
NickDickinsonWildeA *similar* implementation is included in AdvAgg. The D8 version is in development and does include that.
Comment #22
catchThis is still valid and would make a big difference - i.e. just standard install logged in as admin, toolbar is set to 'screen' in the library definition, which results in two or three files that could have been one (it's usually three, because unless it's the first or last file in an aggregate, it ends up as a new aggregate sandwiched between two.
http://caniuse.com/#feat=css-mediaqueries shows that IE11 doesn't support nested media queries, so I don't think we can make this work without using a proper CSS preprocessor (that supports un-nesting media queries).
Comment #23
catchComment #32
catchComment #35
catchI had opened #2695871: Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration four years later back in 2016 without realising it was a duplicate, and that issue got committed to 10.0.x, so marking this one outdated.