Problem/Motivation

Viewing the front page of a Drupal 10.1 install as user 1 shows the following CSS aggregates in the header.

As you can see, there are four aggregates, with the second aggregate including media="screen".

    <link rel="stylesheet" media="all" href="/sites/default/files/css/css_AI3tdfWguXKwGYQh77azpIWFB75vRNg_QczKMd7wJAo.css?delta=0&amp;language=en&amp;theme=olivero&amp;include=contextual/drupal.contextual-links%2Csystem/base%2Colivero/global-styling%2Ccore/drupal.active-link%2Colivero/powered-by-block%2Colivero/feed%2Cviews/views.module%2Colivero/navigation-secondary%2Colivero/search-wide%2Colivero/navigation-primary%2Colivero/search-narrow%2Ccore/modernizr%2Ccore/drupal.debounce%2Ctoolbar/toolbar%2Cuser/drupal.user.icons%2Ccore/shepherd%2Ctour/tour-styling%2Ctour/tour%2Ccore/drupal.tabbingmanager%2Ccontextual/drupal.contextual-toolbar%2Cshortcut/drupal.shortcut%2Ctoolbar/toolbar.escapeAdmin%2Cbig_pipe/big_pipe" />
<link rel="stylesheet" media="screen" href="/sites/default/files/css/css_5FA3K14RQkYOhjLi22sUkXa3rr9UmOmp2HuTM9zq2n4.css?delta=1&amp;language=en&amp;theme=olivero&amp;include=contextual/drupal.contextual-links%2Csystem/base%2Colivero/global-styling%2Ccore/drupal.active-link%2Colivero/powered-by-block%2Colivero/feed%2Cviews/views.module%2Colivero/navigation-secondary%2Colivero/search-wide%2Colivero/navigation-primary%2Colivero/search-narrow%2Ccore/modernizr%2Ccore/drupal.debounce%2Ctoolbar/toolbar%2Cuser/drupal.user.icons%2Ccore/shepherd%2Ctour/tour-styling%2Ctour/tour%2Ccore/drupal.tabbingmanager%2Ccontextual/drupal.contextual-toolbar%2Cshortcut/drupal.shortcut%2Ctoolbar/toolbar.escapeAdmin%2Cbig_pipe/big_pipe" />
<link rel="stylesheet" media="all" href="/sites/default/files/css/css_kuhPJIoUEPrw6lSo2T4L7_-4v37wAeKmhT8fPFwvE1E.css?delta=2&amp;language=en&amp;theme=olivero&amp;include=contextual/drupal.contextual-links%2Csystem/base%2Colivero/global-styling%2Ccore/drupal.active-link%2Colivero/powered-by-block%2Colivero/feed%2Cviews/views.module%2Colivero/navigation-secondary%2Colivero/search-wide%2Colivero/navigation-primary%2Colivero/search-narrow%2Ccore/modernizr%2Ccore/drupal.debounce%2Ctoolbar/toolbar%2Cuser/drupal.user.icons%2Ccore/shepherd%2Ctour/tour-styling%2Ctour/tour%2Ccore/drupal.tabbingmanager%2Ccontextual/drupal.contextual-toolbar%2Cshortcut/drupal.shortcut%2Ctoolbar/toolbar.escapeAdmin%2Cbig_pipe/big_pipe" />
<link rel="stylesheet" media="all" href="/sites/default/files/css/css_qKcdHo1cKu_xYxsR9LUksx-DrCUAFfMtkTdFg2jt9CM.css?delta=3&amp;language=en&amp;theme=olivero&amp;include=contextual/drupal.contextual-links%2Csystem/base%2Colivero/global-styling%2Ccore/drupal.active-link%2Colivero/powered-by-block%2Colivero/feed%2Cviews/views.module%2Colivero/navigation-secondary%2Colivero/search-wide%2Colivero/navigation-primary%2Colivero/search-narrow%2Ccore/modernizr%2Ccore/drupal.debounce%2Ctoolbar/toolbar%2Cuser/drupal.user.icons%2Ccore/shepherd%2Ctour/tour-styling%2Ctour/tour%2Ccore/drupal.tabbingmanager%2Ccontextual/drupal.contextual-toolbar%2Cshortcut/drupal.shortcut%2Ctoolbar/toolbar.escapeAdmin%2Cbig_pipe/big_pipe" />

The media="screen" contains only the CSS file for tour-styling, due to this:

tour-styling:
  version: VERSION
  css:
    component:
      css/tour.module.css: { media: screen }

I think when we added media support many years ago, we thought it'd add one extra file to support media="screen" (and that it would mostly be used for media="print"), however in fact it turns one aggregate into three which is a lot of extra http requests.

This is because we also deal with weights of assets, so if some assets in an aggregate need to go before tour, and some after, for it to be in the middle there have to be three files. And we also add weights based just on the order that files are added to the page, so that's pretty much all the time unless an asset with media screen happened to be the very last or very first one. Even if we remove weight support, unless we explicitly put screen assets first or last, they could still end up in the middle and require their own aggregate.

Proposed resolution

Inline media declarations during aggregation, for anything that is not media="print", this means that the common case of media="screen" will be included in the same aggregates. Since we're concerned about front end performance on screens, and not worried about a few extra CSS bytes when someone's printing a web page, there should not be a downside to doing it like this.

All browsers that Drupal supports, support nested media queries now that we've dropped support for IE11.

Here's how the CSS aggregates for exactly the same page look after applying the patch and clearing caches:

  <link rel="stylesheet" media="all" href="/sites/default/files/css/css_fp0O1ULW3vedV1lZnnaJEzM2rc-NGp8-8c7NCRZR1ng.css?delta=0&amp;language=en&amp;theme=olivero&amp;include=contextual/drupal.contextual-links%2Csystem/base%2Colivero/global-styling%2Ccore/drupal.active-link%2Colivero/powered-by-block%2Colivero/feed%2Cviews/views.module%2Colivero/navigation-secondary%2Colivero/search-wide%2Colivero/navigation-primary%2Colivero/search-narrow%2Ccore/modernizr%2Ccore/drupal.debounce%2Ctoolbar/toolbar%2Cuser/drupal.user.icons%2Ccore/shepherd%2Ctour/tour-styling%2Ctour/tour%2Ccore/drupal.tabbingmanager%2Ccontextual/drupal.contextual-toolbar%2Cshortcut/drupal.shortcut%2Ctoolbar/toolbar.escapeAdmin%2Cbig_pipe/big_pipe" />
<link rel="stylesheet" media="all" href="/sites/default/files/css/css_qKcdHo1cKu_xYxsR9LUksx-DrCUAFfMtkTdFg2jt9CM.css?delta=1&amp;language=en&amp;theme=olivero&amp;include=contextual/drupal.contextual-links%2Csystem/base%2Colivero/global-styling%2Ccore/drupal.active-link%2Colivero/powered-by-block%2Colivero/feed%2Cviews/views.module%2Colivero/navigation-secondary%2Colivero/search-wide%2Colivero/navigation-primary%2Colivero/search-narrow%2Ccore/modernizr%2Ccore/drupal.debounce%2Ctoolbar/toolbar%2Cuser/drupal.user.icons%2Ccore/shepherd%2Ctour/tour-styling%2Ctour/tour%2Ccore/drupal.tabbingmanager%2Ccontextual/drupal.contextual-toolbar%2Cshortcut/drupal.shortcut%2Ctoolbar/toolbar.escapeAdmin%2Cbig_pipe/big_pipe" />

As you can see, this reduces four aggregates down to two. The reason we still have two aggregates instead of one, is because core intentionally puts theme CSS into its own aggregate, that's unrelated to this change and by design.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#32 2695871-32.patch5.79 KBcatch
#31 2695871-30.patch5.79 KBcatch
#31 2695871-24-30-interdiff.txt1.29 KBcatch
#24 2695871-24.patch4.76 KBcatch
#23 2695871-23.patch4.76 KBcatch
#21 2695871.patch4.76 KBcatch
#4 2695871.patch4.71 KBcatch
#3 2695871.patch1.58 KBcatch

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2695871.patch1.58 KB

Here's a patch.

Keeps separate aggregates for media print, everything else gets inlined.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 2695871.patch4.71 KB

Wrong patch.

catch’s picture

catch
he/him
Primary language English
commented

This might be stuck on nested media query support, or at least it gets a lot more complicated if we try to take that into account:

http://caniuse.com/#feat=css-mediaqueries

The last submitted patch, 3: 2695871.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
commented

Couple of things from reading up on nested @media queries:

Every browser except IE supports them more or less, but even IE11 doesn't.

IE11 and other browsers apparently support nested media queries if the nesting happens via an @import.

- we currently pull stylesheets referenced with @import when generating aggregates, this could break if the @import is inside a media query.

- so a potentiall workaround for this might be to 1. check if the file has a @media statement 2. use @import if it does instead of actually aggregating 3. additionally don't aggregate files via @import if the imported file has a @media query.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

catch
he/him
Primary language English
commented
Version: 9.5.x-dev » 10.1.x-dev
Status: Needs review » Needs work

Now that core no longer supports IE11, we're not blocked on nested media query support any more, that means the patch might be viable more-or-less as-is.

However, this needs a re-roll for 10.1.x after #1014086: Stampedes and cold cache performance issues with css/js aggregation.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
StatusFileSize
new 2695871.patch4.76 KB

Actually this was unaffected by that change, but it was so old it needed a re-roll for array syntax. Nothing else appears to have changed.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 2695871-23.patch4.76 KB
catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 2695871-24.patch4.76 KB
catch’s picture

catch
he/him
Primary language English
commented
Title: Media support for style tags results in 'split' aggregates » Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

We are special casing print here, but there is also speech according to w3, so should we special case that as well, or do it the other way around? Or should we not worry about it (because before googling this, I didn't know this existed).
https://www.w3schools.com/css/css3_mediaqueries.asp

The code here looks great, so if we don't worry about the speech type, then this is rtbc as far as I'm concerned.

catch’s picture

catch
he/him
Primary language English
commented

The speech media type is deprecated now (i.e. it's allowed, but treated as if it was 'all'), so I think it's fine to only special-case print here.

So there's only 'all', 'screen', and 'print' in terms of what should be included, and we're ensuring that 'screen' and 'all' are bundled together, and that 'print' is bundled separately.

https://www.w3.org/TR/mediaqueries-4/#media-types

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community

Agreeing with @borisson_ in #26 and reading the explanation just above this comment: RTBC

catch’s picture

catch
he/him
Primary language English
commented
Version: 10.1.x-dev » 10.0.x-dev

This should be eligible for 10.0.x since no IE11 support and no API change, so moving back a version.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

This looks fine and a nice saving on http requests for the 99.9% use-case.

Unfortunately we need to fix the docs for \Drupal\Core\Asset\CssCollectionGrouper::group() which currently say:

  /**
   * {@inheritdoc}
   *
   * Puts multiple items into the same group if they are groupable and if they
   * are for the same 'media'. Items of the 'file' type are groupable if their
   * 'preprocess' flag is TRUE, and items of the 'external' type are never
   * groupable.
   *
   * Also ensures that the process of grouping items does not change their
   * relative order. This requirement may result in multiple groups for the same
   * type and media, if needed to accommodate other items in between.
   */
  public function group(array $css_assets) {
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2695871-24-30-interdiff.txt1.29 KB
new 2695871-30.patch5.79 KB

Expanded the docs a fair bit.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 2695871-32.patch5.79 KB

Fixing that typo s/seperate/separate.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed d587699e0a to 10.1.x and e91369d0b8 to 10.0.x. Thanks!

  • alexpott committed d587699 on 10.1.x 
    Issue #2695871 by catch, borisson_, alexpott: Aggregation creates two...

  • alexpott committed e91369d on 10.0.x 
    Issue #2695871 by catch, borisson_, alexpott: Aggregation creates two...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

catch credited fubhy.

catch’s picture

catch
he/him
Primary language English
commented
catch’s picture

catch
he/him
Primary language English
commented

Marked #1488910: Incorporate media queries in aggregated CSS files, don't create a new group for each new media query as duplicate and transferring credit.