Problem/Motivation

This issue was originally opened because an asset library depended upon the CSS files provided by the Seven theme. But it could not declare a dependency on those since it wasn't a library yet.

However, the problem is broader than that: there's a strong disconnect between how theme(r)s attach assets and how modules/developers attach assets. Everything is using asset libraries… except themes. But themes already support asset libraries! (There is a libraries property in themes that allows you to declare which libraries should always be loaded for this theme.)

The advantages:

  1. dependencies, hence a step towards getting rid of weights
  2. one uniformly applied system for modules and themes alike
  3. themes are also forced to think about categorisation in their CSS
  4. libraries can contain CSS and JS as logically associated units, and can declare dependencies on other libraries
  5. themes are encouraged to create a library per component/topic/logical unit and hence are encouraged to not load all their CSS on all pages anymore, but to attach it when appropriate in a preprocess hook and/or only on the relevant pages via hook_page_attachments_alter()

The downsides:

  1. a layer of indirection in a theme's *.info.yml file, hence perceived as more complex

Proposed resolution

Remove the stylesheets property from theme *.info.yml files, in favor of the libraries property (which already exists).

This does not affect stylesheets-override and stylesheets-remove, which continue to function as-is.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because themes not using libraries provides an inconsistent DX/TX, prevents dependencies from being declared on theme CSS assets.
Prioritized changes The main goal of this issue is performance and removing previously deprecated code.
Disruption Disruptive for contributed and custom themes because it will require a BC break. The benefit is a consistent DX/TX, and the unblocking of #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests, which brings better front-end performance.

Remaining tasks

Get green & review.

User interface changes

None.

API changes

Removed the stylesheets property from theme *.info.yml files, in favor of the libraries property (which already exists)

Original report by @alexpott

Follow up from #2376147: Installer is missing all of the global Seven theme stylesheets

The maintenance-page library in seven depends on all the stylesheets in seven.info.yml - we should make a base library.

Files: 
CommentFileSizeAuthor
#77 themes_stylesheets_to_libraries-2377397-77.patch23.64 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,755 pass(es). View

Comments

alexpott’s picture

Status: Active » Needs review
Issue tags: -D8MI, -language-base, -sprint
FileSize
7.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,217 pass(es). View
sqndr’s picture

Status: Needs review » Reviewed & tested by the community

Looks good; the same thing has been done for Bartik as well:

maintenance_page:
  version: VERSION
  css:
    theme:
      css/maintenance-page.css: {}
  dependencies:
    - system/maintenance
    - bartik/base

This patch does the same thing for Seven. Manually tested and all the Seven stylesheets all included on the maintenance page.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/seven/seven.libraries.yml
@@ -1,3 +1,39 @@
+  css:
+    theme:
+      css/base/elements.css: { media: screen }
+      css/base/typography.css: { media: screen }
+      css/components/admin-list.css: { media: screen }
+      css/components/admin-options.css: { media: screen }
+      css/components/admin-panel.css: { media: screen }
+      css/components/block-recent-content.css: { media: screen }
+      css/components/content-header.css: { media: screen }
+      css/components/breadcrumb.css: { media: screen }
+      css/components/buttons.css: { media: screen }
+      css/components/buttons.theme.css: { media: screen }
+      css/components/comments.css: { media: screen }
+      css/components/messages.css: { media: screen }
+      css/components/dropbutton.component.css: { media: screen }
+      css/components/entity-meta.css: { media: screen }
+      css/components/field-ui.css: { media: screen }
+      css/components/form.css: { media: screen }
+      css/components/help.css: { media: screen }
+      css/components/menus-and-lists.css: { media: screen }
+      css/components/modules-page.css: { media: screen }
+      css/components/node.css: { media: screen }
+      css/components/page-title.css: { media: screen }
+      css/components/pager.css: { media: screen }
+      css/components/skip-link.css: { media: screen }
+      css/components/tables.css: { media: screen }
+      css/components/tabs.css: { media: screen }
+      css/components/tour.theme.css: { media: screen }
+      css/components/update-status.css: { media: screen }
+      css/components/views-ui.css: { media: screen }
+      css/layout/layout.css: { media: screen }
+      css/layout/node-add.css: { media: screen }
+      css/theme/appearance-page.css: { media: screen }

One important difference is that Bartik only has SMACSS "theme" CSS files. But Seven AFAICT has SMACSS "base", "component" and "layout" CSS files as well.

I think this might need to be changed to:

  css:
    base:
      css/base/elements.css: {}
      css/base/typography.css: {}
    component:
      css/components/admin-list.css: {}
      css/components/admin-options.css: {}
      css/components/admin-panel.css: {}
      css/components/block-recent-content.css: {}
      css/components/content-header.css: {}
      css/components/breadcrumb.css: {}
      css/components/buttons.css: {}
      css/components/buttons.theme.css: {}
      css/components/comments.css: {}
      css/components/messages.css: {}
      css/components/dropbutton.component.css: {}
      css/components/entity-meta.css: {}
      css/components/field-ui.css: {}
      css/components/form.css: {}
      css/components/help.css: {}
      css/components/menus-and-lists.css: {}
      css/components/modules-page.css: {}
      css/components/node.css: {}
      css/components/page-title.css: {}
      css/components/pager.css: {}
      css/components/skip-link.css: {}
      css/components/tables.css: {}
      css/components/tabs.css: {}
      css/components/tour.theme.css: {}
      css/components/update-status.css: {}
      css/components/views-ui.css: {}
    layout:
      css/layout/layout.css: {}
      css/layout/node-add.css: {}
    theme:
      css/theme/appearance-page.css: {}

… but somebody with more SMACSS experience should confirm that.

joelpittet’s picture

Assigned: Unassigned » LewisNyman

@LewisNyman may need to weigh in on this, assigning him to have a peak. Though anybody who was involved in the whole base/theme/layout/component can weigh in here.

I'd assume that structuring the library to swap out the different levels could be handy in some cases though. Like lightweight pages or HTML email templates, etc.

Wim Leers’s picture

#4: Yes, that's another possibility :) (One library per SMACSS level.)

sqndr’s picture

#3++

Actually looks like a better idea to me. This way, we could only include seven/base, instead of the css for all components. :)

LewisNyman’s picture

All I want is for someone to explain to me if this is now the recommend best practice for all Drupal 8 themes? Because people will look at how core themes implement certain things for guidance. I think this method of adding CSS globally is more complex than the stylesheets property.

joelpittet’s picture

Assigned: LewisNyman » Unassigned

@Wim Leers you are pretty good and splaining why libraries are the future:) *putting people on the spot:P*

joelpittet’s picture

The only thing I see that is better about 'libraries' is that they are named, and by naming them you get to use them as dependencies, and dependencies are much better than weights. IMO

Wim Leers’s picture

#7:

All I want is for someone to explain to me if this is now the recommend best practice for all Drupal 8 themes?

Yes.

Because people will look at how core themes implement certain things for guidance.

Exactly. And this is why Bartik was already converted.

I think this method of adding CSS globally is more complex than the stylesheets property.

It's slightly more complex, but it's also consistent with everything else in Drupal core. If you want assets (CSS/JS) to be loaded, use asset libraries. Period. In modules, in themes, in alter hooks, in preprocess functions, everywhere. So it's learning one pattern, that is applied consistently everywhere, rather than learning a mishmash of patterns:

  1. asset libraries (ALWAYS *.libraries.yml PLUS $build['#attached']['libraries'] in render arrays/preprocess functions OR the libraries property in a theme's *.info.yml)
  2. $build['#attached]['css'] + $build['#attached]['js']
  3. the stylesheets property in a theme's *.info.yml

A long, long time ago, in #1813186: Integrate scripts/stylesheets from info/layout files with libraries, we chose to remove the scripts property from a theme's *.info.yml, in favor of that libraries property. Many themes ship with some JS. Hence for a theme to load that JS, you already need to use a library anyway.

On July 27, 2014, mdrummond also wrote this at #2228745-12: Color module doesn't look for CSS files declared in a library (where we converted Bartik's stylesheets property into libraries):

Any progress on this, sqndr? It would be nice to get this in so we can get stylesheets out of info.yml and just have them in libraries.yml.

When it got committed, he even said:

+1 zillion to this.

Theme sanity, yay!

So I was always under the impression that front-end people are on board with this general approach, and that the conversion was only a matter of time. Perhaps that's wrong?


#9:
The advantages:

  1. dependencies (as you say), hence a step towards getting rid of weights
  2. one uniformly applied system for modules and themes alike
  3. themes are also forced to think about SMACSS in their CSS
  4. libraries can contain CSS and JS as logically associated units, and can declare dependencies on other libraries
  5. themes are encouraged to create a library per component/topic/logical unit and hence are encouraged to not load all their CSS on all pages anymore, but to attach it when appropriate in a preprocess hook and/or only on the relevant pages via hook_page_attachments_alter()

The downsides:

  1. a layer of indirection in a theme's *.info.yml file, hence perceived as more complex
sqndr’s picture

@Wim Leers:

A long, long time ago, in #1813186: Integrate scripts/stylesheets from info/layout files with libraries, we chose to remove the scripts property from a theme's *.info.yml, in favor of that libraries property.

Does this mean we're also going to remove the stylesheets property from *.info.yml? And what about the second method? Keep several ways in to do this will most likely bring confusing. It means my own theming guide + the theming guide on Drupal 8 needs an update regarding adding stylesheets: https://www.drupal.org/node/2349827

Wim Leers’s picture

Does this mean we're also going to remove the stylesheets property from *.info.yml?

That's the only sensible way forward, IMHO.

catch’s picture

Priority: Normal » Major

I'd personally like to see us move to using libraries everywhere, it helps to unblock issues such as #1014086: Race conditions, stampedes and cold cache performance issues with css/js aggregation.

stylesheets-remove and stylesheets-override should still be OK though that's more of a shortcut to alter hooks which is different anyway.

sqndr’s picture

@Wim Leers; sounds fair to me.

So to conclude: all themes and modules should define there css files as a library and depend on the library in the *.info.yml file. This is the only right way to include stylesheets inside themes/modules. This has been changed in all modules. It has also been changed in Bartik, but not yet in the Seven theme. That's what this issue is about. When defining the libraries stylesheets, the SMACSS methodology should be used if the theme/module's css folder has been organised in such as way. And they should, because it's part of the coding standards. The means there's 5 extra keys available: theme/base/component/layout/states:

theme-or-module
  css:
    theme:
      theme.css
    base:
      base.css
    components:
      component.css
    layout:
      layout.css
    states:
      states.css

The stylesheets property for *.info.yml should be removed because of this. The stylesheets-remove and stylesheets-override can stay because that's more of a shortcut to alter hooks which is different anyway.

$build['#attached]['css'] + $build['#attached]['js']

What about that?

LewisNyman’s picture

Category: Bug report » Task
Issue tags: +TX (Themer Experience), +CSS, +frontend

That's the only sensible way forward, IMHO.

Ok this answers my original question. Let's have a real discussion about this, in this issue or a meta, because I think we need to have a focused discussion about the advantages and the harm to themer experience. We're adding another Drupalism and another barrier to frontend developers. We're adding another Drupalism and another barrier to frontend developers. CSS isn't directly covered by #2298551: Documentation updates adding/adjusting inline/external JS in themes

The libraries system was designed to deal with a complex use case (dependancies) but it feels like we're applying to a separate use case. CSS does not have dependancies on other CSS like Javascript does. The maintenance-page.css does not depend on any other CSS to function.

Wim Leers’s picture

Title: Seven should have a base library » Themes should use libraries, not individual stylesheets
Component: Seven theme » theme system
Category: Task » Bug report
Issue summary: View changes
Issue tags: +DX (Developer Experience)
Parent issue: » #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests
Related issues: +#2378095: Convert all remaining attached individual CSS/JS assets to attached asset libraries

#14: "exactly!" to everything you said :)

About attaching individual CSS and JS files: that's also going away, first step at #2378095: Convert all remaining attached individual CSS/JS assets to attached asset libraries, an issue to remove that is coming up…


As per #13, I'm expanding the scope of this issue.

Not only does this block #1014086: Race conditions, stampedes and cold cache performance issues with css/js aggregation as catch said in #13, it also blocks #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests, which is critical.

Issue summary updated.

Wim Leers’s picture

#15: We cross-posted. Here are my thoughts.

We're adding another Drupalism and another barrier to frontend developers.

Yes, we add one layer of indirection. But for the following front-end related reasons:

  1. enforced SMACSS categorization
  2. encouraged to create groups (libraries) of assets that logically form a unit; e.g. CSS to style articles, CSS to style menus, CSS for the overall layout, CSS + JS for a megamenu, etc — all of these should be separate libraries. Most themes don't create these logical separations, but they'd benefit from it.
  3. because of the previous point, an encouragement of attaching assets only when necessary (using $variables['#attached'] in hook_preprocess()), rather than loading all CSS on all pages

I'm convinced this helps to futureproof Drupal from a front-end perspective, which will increasingly be about reusable, reactive, responsive widgets/components/snippets/blocks. Asset libraries then map perfectly to that use case, and will become feasible once Shadow DOM and Element Queries become available.


I could see a few additions now or in the future to improve the TX further:

  1. a libraries-extend property in theme *.info.yml files:
    libraries-extend:
      contextual/drupal.contextual-links:
        css:
          theme:
            mytheme-fancy-contextual-links.css
        js:
          mytheme-fancy-contextual-links.js
    

    … to attach additional CSS/JS to existing libraries (if those existing libraries are present)

  2. a attach-library-to-hook property in theme *.info.yml files:
    attach-library-to-hook:
      menu_local_action:
        - core/modernizr
      block___menu:
        - mytheme/fancymenublock
    

    … to automatically attach a library instead of having to implement hook_preprocess_HOOK() for each.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
22.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

This patch:

  1. removes the stylesheets property from theme *.info.yml files (as in: removes support for it and removes it from all core themes)
  2. also incorporates the changes of the earlier patch
  3. but now only loads node-add.css when it's actually necessary.

It does not yet:

  1. split up Seven's library in one-library-per-SMACSS-level
  2. load other CSS files only when necessary (e.g. views-ui.css)

(because I'm not sure yet everybody wants that).

Status: Needs review » Needs work

The last submitted patch, 18: themes_stylesheets_to_libraries-2377397-18.patch, failed testing.

Wim Leers’s picture

Actually, there were only 3 failures. Testbot is currently failing due to #2376039: Undefined property ContainerAwareEventDispatcherTest::results in run-tests.sh. Fixing those…

sqndr’s picture

It does not yet: split up Seven's library in one-library-per-SMACSS-level

As you mentioned (@ Wim Leers), by using the libraries we enforced SMACSS categorization. In my opinion this should be fixed as well. It was the main reason why you changed the ticket as well in #3. I hope others will agree.

It does not yet: load other CSS files only when necessary

This would however be great as well. You don't need all the css files on every page. We should be able to include small parts of the css, only where we need them.

@Wim Leers: Great work! ;)

Wim Leers’s picture

In my opinion this should be fixed as well. It was the main reason why you changed the ticket as well in #3.

Actually, those are two different things. :)

I did apply SMACCS level. I did not create one library per SMACCS level, which was proposed by Joël Pittet in #4:

I'd assume that structuring the library to swap out the different levels could be handy in some cases though. Like lightweight pages or HTML email templates, etc.

So I think you slightly misunderstood me? :)

From the patch:

+++ b/core/themes/seven/seven.libraries.yml
@@ -1,3 +1,49 @@
+  css:
+    base:
+      css/base/elements.css: {}
+      css/base/typography.css: {}
+    component:
+      css/components/admin-list.css: {}
…
+      css/components/views-ui.css: {}
+    layout:
+      css/layout/layout.css: {}
+    theme:
+      css/theme/appearance-page.css: {}

SMACSS metadata: check!

Fabianx’s picture

There is another reason, which makes this boundary-critical for Performance reasons:

We want to implement AJAX / ESI, etc. requests that only load the necessary libraries. (e.g. https://docs.google.com/a/tag1consulting.com/presentation/d/1Q44eWLI2qvZ...)

However what you normally would do is you would overwrite the CSS of e.g. a module in your theme.

module-a.css:

/** FOO of Module */
.foo {
  color: green;
}

theme/css/mytheme.css

/** FOO of Theme */
.foo {
  color: red;
}

So the last one wins.

Now lets assume the module.css is loaded _after_ the theme css, then you suddenly have:

/** FOO of Theme */
.foo {
  color: red;
}

AJAX REQUEST:

/** FOO of Module */
.foo {
  color: green;
}

Huh, what happened just now?

We now have the wrong CSS order.

That means CSS has dependencies (or at least an order), too. - in a way.

The ways Wim suggested will solve this, because you can:

- extend the library, so your CSS is guaranteed to come after the module libraries CSS.
- overwrite module-a.css directly via stylesheets-remove or stylesheets-overwrite (that works already now)

So by using libraries everywhere, we can much more easily define ordering, too - which is important for CSS.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
22.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,322 pass(es). View
421 bytes
joelpittet’s picture

One thing I always do in D7, always remove stylesheets and scripts (stylesheets more so). It's part of my workflow: if CSS interferes at all with my design or it's large and inefficient remove, copy and rewrite into a sass partial.

Essentially what magic module or omega4 provides.

I'm also a big fan of dependencies vs weights, they are easy to understand and get at the root of the problem, IMO. Whether 'libraries' is the best approach is up for debate as you may be including just one js file or one css file which seems small for a 'library' with 1 book for take out:P

I know WordPress does work like libraries and dependencies though they are doing it more along the lines of drupal_add_js()/drupal_add_css() was, just each one is given an identifier (library name) and an array of dependencies.
(@Wim Leers I was mentioning that briefly in Amsterdam) Just because that is the way it seems to be leaning towards it may be interesting to see how others have tackled the problem:
@see https://developer.wordpress.org/reference/functions/wp_enqueue_style/
@see https://developer.wordpress.org/reference/functions/wp_enqueue_script/

Wim Leers’s picture

#25: Thanks, interesting information! Especially that WordPress apparently was already requiring dependencies. They are ahead of us in that respect, then.
Having libraries declared in declarative YML files (Drupal 8 ) rather than in code (WordPress & Drupal 7) is better though: it ensures you have a predictable (declarative) system, rather than one where PHP logic can dynamically create a different set of libraries on every request. It allows for cacheability and predictability, and hence less frustration when debugging asset-related problems :)

LewisNyman’s picture

Issue summary: View changes

Thanks for outlining everything in the issue summary! Excellent work. I'll comment on a few but it would be great to get the a wider perspective of the frontend community, especially ones who aren't familiar with D8 core.

1. dependencies, hence a step towards getting rid of weights

Do themes need weighting for their CSS? I have never had this problem.

Now lets assume the module.css is loaded _after_ the theme css

I thought our aggregation group prevent this from happening? Is it possible for module CSS to load after the theme? (Unless you intentionally modify the group.)

2. One uniformly applied system for modules and themes alike

Consistency does sound like a good idea, from the perspective of a core developer who has an overview of everything. But most themers are not module developers (I think). I'm worried about applying this solution to a use case that does not warrant it, just for the sake of consistency. Hopefully we get more feedback from others here.

3. Themes are also forced to think about SMACSS in their CSS

This is not a good thing. We shouldn't force a particular architecture on frontend developers who use Drupal. What happens if they prefer another system and what if something better comes along? The best thing Drupal can do is be agnostic.

I don't think this is what you mean though, because it's not forcing SMACSS categorisation, it's just forcing some kind of categorisation, which is fine. I've updated the summary to remove mentions of SMACSS.

4. Libraries can contain CSS and JS as logically associated units, and can declare dependencies on other libraries

This is fine, but here we are talking about generic styling. CSS does not have a dependancy on other CSS. It feels like a we are applying the solution to another problem.

Themes are encouraged to create a library per component/topic/logical unit and hence are encouraged to not load all their CSS on all pages anymore, but to attach it when appropriate in a preprocess hook and/or only on the relevant pages via hook_page_attachments_alter()

This is a good goal, but a total opposite from how all/most theme developers use CSS right now, which is why I think this is a big issue.

Let's look at the most popular base themes:

Zen, Omega, Aurora, and Fusion all ship with one referenced CSS file their starterkit info file. AT and Tao include a base file and generic 'style.css'.

Zen, Omega, Aurora, architect their styling using Sass partials, which then compile into one CSS file. This means you can split up your styling into whatever logically makes sense to you and Drupal doesn't care. I think bypassing Drupal's asset handling is a draw here, because it's confusing and annoying. It's much easier to remove all stylesheets and manage/compress them yourself.

I'm not against this idea, and would like to see it taken further in Seven so we aren't loading all the component stylesheets globally. I just think it needs some more discussion.

Wim Leers’s picture

This is getting long, so replying per subject.


Weights vs dependencies

I thought our aggregation group prevent this from happening? Is it possible for module CSS to load after the theme? (Unless you intentionally modify the group.)

Fabianx was specifically talking about the case of ESI/AJAX/Bigpipe ("AJAXing in" additional content, with associated CSS/JS). In that case, the basic page with the main content would be rendered as-is, but blocks would be loaded later, after the initial page had loaded. In that case, the theme's "every page" CSS would already be loaded as part of the basic page with the main content.
However, when e.g. a block FOO is loaded, which comes with its own CSS, it would then be overriding the theme's CSS for FOO. Because the theme didn't specify a separate library for FOO, and hence it was loaded as part of the theme's "every page" CSS.

It is very likely that this is going to be the technique to make Drupal 8 blazingly fast for authenticated users. I think it's even quite possible that in a Drupal 8 point release (8.1, 8.2…), Drupal will ship with this by default.


The why: only consistency?

I'm worried about applying this solution to a use case that does not warrant it, just for the sake of consistency. Hopefully we get more feedback from others here.

I understand the concern. And I initially was concerned about it too! But having talked to several front-end people, this doesn't seem to be a problem, really. I also want to see more feedback though.
Note that this is not only for the sake of consistency. It's also to enforce front-end people to think about categorization, to encourage smaller components, and hence to load less CSS (plus referenced images) and improve front-end performance. Plus, it blocks #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests, which is also important for front-end performance.

I've thought about turning a theme's stylesheets property into a library automagically internally, but that has several flaws:

  1. unable to specify SMACSS categories
  2. overlap with the existing support for libraries
  3. still encourages the poor practice of loading rarely needed CSS on all pages… just because that's easier
  4. you'd still need to use additional libraries in a theme to attach assets in preprocess hooks, so if you're ever going to do that, you'll need to learn it anyway

Themes only have generic styling and don't need dependencies

This is fine, but here we are talking about generic styling. CSS does not have a dependency on other CSS. It feels like a we are applying the solution to another problem.

Dependency implies order. If A depends on B, then B must be loaded before A. The order in which CSS is loaded is of great importance… I'm sure you agree with that?

Next, I strongly disagree that this is "generic styling". If it really was generic styling, and all themes only did generic styling, there wouldn't be a problem. But even a core theme like Seven violates this. Its stylesheets property lists the following CSS files, which are hardly generic (I could argue this for even more, but these are the clearest offenders):

  1. css/components/field-ui.css
  2. css/components/modules-page.css
  3. css/components/tour.theme.css
  4. css/components/update-status.css
  5. css/components/views-ui.css
  6. css/layout/node-add.css
  7. css/theme/appearance-page.css

On top of that, I disagree with the general statement here and what you wrote in #15 (The maintenance-page.css does not depend on any other CSS to function.). This is not true; without the general Seven stylesheets, seven/maintenance-page does not achieve the intended effect: maintenance-page.css builds on top of the "every page" Seven CSS files.
In exactly the same way, install-page.css builds on top of the "every page" Seven CSS files AND maintenance-page.css, this is why seven/install-page declares a dependency on seven/maintenance-page.


Asset libraries imply componentization in Drupal, themers do componentization in precompiled CSS

This is a good goal, but a total opposite from how all/most theme developers use CSS right now, which is why I think this is a big issue.

Absolutely.

Zen, Omega, Aurora, and Fusion all ship with one referenced CSS file their starterkit info file. AT and Tao include a base file and generic 'style.css'.

Omega's ships with 4, but they're still not per-component for sure.

So if I understand you correctly, your biggest concerns are that A) Drupal would encourage themes to create a library per component/logical unit, whereas they're used to just load all CSS on all pages, B) Drupal would encourage to handle the "componentization" in Drupal itself, whereas many themes use SASS or other CSS preprocessors, split it up into files per component and then compile it into a single CSS file and hence encourage "componentization" in their precompiled CSS.

Please confirm or correct.

If that's the case, then I have four thoughts:

  1. Those themes are disregarding front-end performance (unless their users write so compact and generic CSS that a single aggregate for the entire site is all the user ever needs to load)
  2. I don't believe it'd be so difficult to set up several compiled CSS targets rather than a single one. That'd even simplify maintenance, code review, maintainability, and frustration with CSS compilers (been there, done that: an update for SASS -> ALL of your compiled CSS is changed… facepalm).
  3. What I proposed at the bottom of #17 could be of help here: libraries-extend and/or attach-library-to-hook properties (naming TBD).
  4. This (a single compiled CSS file, disable all "default" Drupal CSS) is actually more or less the technique I use on my personal site (http://wimleers.com) as well… to make it faster. My personal site is so small and simple, that it doesn't make sense to only load attached CSS, it's more efficient to load all the CSS always. (It's 30 KB, of which >20 KB is for a font.) What if there was a singlecss D8 contrib module that allowed you to use this workflow if you wanted to? It can't be the default because we can't automagically split up huge monolithic CSS files, but we can automagically combine many small ones… and we can cater for a "compile it all into a single CSS file" approach.
cosmicdreams’s picture

It's also worth noting that HTTP/2 will bring new asset optimization strategies and largely deprecate the compile & compress strategy that many systems currently deploy:

Prioritization will be key. See:
https://plus.google.com/+IlyaGrigorik/posts/47FNmFEQpjN
https://nghttp2.org/blog/2014/04/27/how-dependency-based-prioritization-...

I wonder if we're building system-level enforcement of a development methodology that will be better enforced by web standards. Should we leave asset delivery optimization to HTTP/2?

Wim Leers’s picture

It's also worth noting that HTTP/2 will bring new asset optimization strategies and largely deprecate the compile & compress strategy that many systems currently deploy:

Yes! :)

I wonder if we're building system-level enforcement of a development methodology that will be better enforced by web standards. Should we leave asset delivery optimization to HTTP/2?

This will only prioritize which assets are loaded first (those in the critical path to do a first paint etc.). For example, this would ensure that CSS/JS are loaded before any images are loaded.

It does not help with not loading unnecessary assets. It also does not change the order in which CSS or JS files are loaded (if it would, it'd break the web/violate specs).

Besides HTTP/2 Prioritization, there will also be HTTP/2 Server Push and HTTP/2 Server Hint… and for both of those, we will need dependency information to figure out which assets to push to the client and which assets to hint to the client.

See https://www.igvita.com/2013/06/12/innovating-with-http-2.0-server-push/, http://www.chromium.org/spdy/spdy-whitepaper

davidhernandez’s picture

I like all of the advantages posted in the summary, especially 2 and 4.

The disadvantage I don't think is too big a deal. It does make it slightly more complicated to add your CSS file, but I don't think there is anything intuitive about the way we do it now. Every new themer I talk to will ask me which template file to edit so they can add their CSS to <head>. They don't know anything about the "way" to do it until they read the instructions.

We shouldn't force a particular architecture on frontend developers who use Drupal. What happens if they prefer another system and what if something better comes along? The best thing Drupal can do is be agnostic.

I completely agree with this statement from Lewis, enough to emphasis it again. We don't want to force a particular workflow or organization on others.

I'm trying to read through everything, and understand everything being proposed, but getting a little lost with so much to read :), so forgive me if this a stupid question. This doesn't prevent a themer from putting all their CSS in one file, correct? They would just put one file under theme or base?

I know it is isn't performant or smart or professional or whatever, but it is something a significant percentage of users are going to do. People who run small sites, or are just getting started, are not going to have complex development methodologies. We have to support simple as well as sophisticated.

Wim Leers’s picture

This doesn't prevent a themer from putting all their CSS in one file, correct? They would just put one file under theme or base?

Correct. Nothing prevents you from having a library with a single CSS file if you choose to do so.

We have to support simple as well as sophisticated.

Absolutely!

leahtard’s picture

Themes are encouraged to create a library per component/topic/logical unit and hence are encouraged to not load all their CSS on all pages anymore, but to attach it when appropriate in a preprocess hook and/or only on the relevant pages via hook_page_attachments_alter()

As the front-end community (not just the front-end Drupal community) moves towards Modular CSS implementations (SMACSS, OOCSS, etc) and online style guides, I see this as a hindrance or irrelevant to this production workflow. Here is an example:

1. A front-end developer creates reusable class that styles certain grouping of content.
2. We add these styles to the style guide for other developers to reference.
3. A back-end developer or a junior developer comes along (maybe with limited front-end skills) and they create a new grouping of content. After referencing the style guide, they see this type of content has already been styled and a class is available.

If that class was only loaded on "relevant pages" this workflow would not be possible. I feel our CSS should be agnostic to the page an element is sitting on. Yes, as themer, I would still have the ability to simply load all of my css on all pages. I just question if this would be useful. Interested in other opinions!

Cheers, Leah

markabur’s picture

Themes are encouraged to create a library per component/topic/logical unit and hence are encouraged to not load all their CSS on all pages anymore, but to attach it when appropriate in a preprocess hook and/or only on the relevant pages via hook_page_attachments_alter()

I do this sometimes now, in D7. E.g. if there is an image gallery content type and it has a significant amount of unique CSS that isn't used anywhere else in the site, I'll load it conditionally, as a separate stylesheet. But for me this is really rare, and I can't think of more than a couple of examples where this sort of componentization is worthwhile or even possible.

My current workflow uses a lot of SASS partials, and they all depend on _base.scss, which is where I define colors, font stacks, etc. that are used sitewide, and which need to be available to the other partials when compiling to CSS. On the surface, many of my partials look like good candidates for becoming separate libraries (_slideshow.scss, _navigation.scss, etc.), but in reality they depend on that _base.css file and would be a real hassle to maintain separately (less DRY).

joelpittet’s picture

Themes are encouraged to create a library per component/topic/logical unit and hence are encouraged to not load all their CSS on all pages anymore, but to attach it when appropriate in a preprocess hook and/or only on the relevant pages via hook_page_attachments_alter()

Seems that this problem has been taken care of mostly with precompilers (SASS/LESS/etc). I do exactly what is mentioned in #34 as well and because it's compiled into a maybe 3 CSS files for my theme with an ungoddly amount of partials grouped in a SMACSS like structure. This helps me keep things DRY, orgranized and maintainable AND when caching is turned off (during developement) I'm only loading one or 2 CSS files and it can be compressed (less work for for Drupal to do) and faster load time during development.

And I'll echo @LewisNyman and @davidhernandez comment, SMACSS/OOCSS are nice and we should use them, but I strongly suggest we don't enforce/force that structure on anybody as it may not fit their use-case or a better strategy may come along shortly.

joelpittet’s picture

@Wim Leers look at how we discribe to others how to build a theme for some guidance:

http://www.appnovation.com/blog/creating-drupal-8-theme-sasssingularity-...
https://www.drupal.org/node/2165673
https://www.acquia.com/blog/ultimate-guide-drupal-8-episode-5-front-end-...
http://drupal.stackexchange.com/questions/104091/how-to-add-css-javasrip...

With the promise that if we add this to our fluffiness.info.yml, we will see those styles applied:

# fluffiness.info.yml
# Stylesheets
stylesheets:
  all:
    - css/fluffiness.css

Though I (and @dawhener at BADCamp) were struggling with libraries because of the extra steps to accomplish this.

This would become (please god correct me if I'm wrong @Wim Leers because I litterally threw my hands up in the air and gave up trying to do it with libraries, though to be fair it could have been a typo and not enough coffee).

# fluffiness.libraries.yml
# Theme Styles
style:
  css:
    theme:
      css/fluffiness: {}

Followed by:

// fluffiness.theme
function fluffiness_preprocess_page(&$variables) {
  $variables['#attached'] = [
    'library' => [ 'fluffiness/style'],
  ];
}
joelpittet’s picture

I guess the addition would be in the info file...
so

# fluffiness.info.yml
libraries:
  - fluffiness/style

Instead of the fluffiness_preprocess_page

davidhernandez’s picture

#37 I think that is right, if you want to always include the library. If you want to discriminate when the library was attached you would still need to use the preprocess, correct?

Dragan Eror’s picture

@LewisNyman

#27

This is fine, but here we are talking about generic styling. CSS does not have a dependancy on other CSS. It feels like a we are applying the solution to another problem.

CSS does not have dependancy on other CSS, you are right, but does dependency on other PHP, JS or Library.
I agree with this idea.

Few examples:

if there is no jQuery on the page I do not need styles for some crazy menu that depends on jQuery.

I can also add my personal styles to my theme to make panel pages look better, which I do not need if there is no panels module and library which comes with panels module.

I want to style comments. I do not need to load those styles if there are no comments on page.

Fabianx’s picture

#36, #37: The assumptions are correct.

Please also note that you have to learn this workflow for adding some javascript anyway - so the learning curve will be needed one way or another anyway.

I think this is a great chance to provide both workflows, which is worth its own discussion.

Compotenized and dependent CSS is _already_ a reality and the examples of SASS and LESS show clearly that CSS _has_ dependencies and order.

Because both in SASS and LESS you include other partials, which are your dependencies per se.

So if you wanted to create a mytheme/slideshow library, you would have that thing depend on mytheme/base.

So dependencies are very similar to _partials in SASS.

But it means that not the whole 'style.css' is loaded over the wire, but only those CSS files that are actually needed.

And while that clearly is a paradigm change it is:

a) very mobile friendly and HTTP 2.0 proof (where aggregation is a moot point)
b) matches the current SASS workflow really nicely

Overall it might even be possible to write a SASS plugin that creates drupal libraries in addition to the CSS files that are no partials ...

jwilson3’s picture

So if you wanted to create a mytheme/slideshow library, you would have that thing depend on mytheme/base.

Ajax example was used above as one of the advantages for this dependency structure... so, if you have a dependency on base and you load the slideshow with ajax, how would it know not to pull in the base again (presumably its already loaded on the page).

I would think rather the mytheme/slideshow has some dependency on a stylesheet (or something else?) from views_slideshow, not from my theme's base.

Fabianx’s picture

#41:

We would use something similar to https://docs.google.com/a/tag1consulting.com/presentation/d/1Q44eWLI2qvZ....

Incremental loads and negative information providing. So we would always know that theme/base is already loaded and not load it again.

The idea is that after AJAX requests there is one single call like:

/js?+slideshow-mytheme/base

to get aggregated styles (for HTTP 1.1).

---

In the ideal case your slideshow library would _extend_ the views slideshow library, so your styles are always added when the slideshow library is added (and always added after the slideshow library CSS, so no ordering issues).

If however you have a custom component and you theme it completely in your theme (with no CSS output), and you need some base styles always, then your mytheme/slideshow might indeed rely on certain base styles.

This makes it e.g. possible to use the same slideshow styles on an admin page and makes preview easier. (in the case you add it dynamically via #attached library from a module for example.)

#17 / @Wim:

I think your libraries-extend is even more useful if you can add dynamic dependencies, too:

libraries-extend:
  core/contextual-links:
    dependencies:
       mytheme/base
    css:
       foo.css

Edit: Just thinking loud, please disregard for this issue, but what if we allowed to instead of merely extending to provide core/contextual-links yourself and just extend from the "aliased" version.

mytheme/contextual-links:
  dependencies:
     bar
   extends:
     core/contextual-links
   css:
     foo.css
   js:
     foo.js

and then

library-provides:
core/contextual-links: mytheme/contextual-links

which does the necessary hook_library_alter(). This would be more OOP like ... As I said just thinking out loud, currently still like the library-extends key better ...

mortendk’s picture

So looking at it from the different persective's of "Themer-that-dont-care-how-drupal-works-just-want-to-add-my-sass-generated-css-file": It dosn't really matter in the theme, its gonna be one file, if that takes up 4 lines of codes instead of 1, yes its gonna be a little bit annoying the first time, but its not gonna be a day to day operation to do that.

I think were going into the we wanna do this for 100%, lets step back and remember the 80% rule for the frontend.

So from my persepctive i think were good with adding in css files this way, if its a win for consistency, developers & generally happiness im fine with it.

The one thing thats gonne be weird is now removing the existing css files, and not using stylesheet-remove in the info file, so working with css files thats gonna be a bit strange, but we can totally fix that with documentation.

davidhernandez’s picture

The one thing thats gonne be weird is now removing the existing css files, and not using stylesheet-remove in the info file...

This doesn't change. It will still be in the info file.

If this goes in, it will need an extremely well publicized change record and we will need to update documentation everywhere.

Also, because of the significance of the change, should this only be in a beta release?

mdrummond’s picture

1. Yay, consistency and ease of explanation. Woot.
2. Yay that people don't have to provide a few dozen CSS files for each component if they don't want to do so.

In most cases, I'm using Sass to aggregate base + component + layout partials into one or maybe a few CSS files because with the prevalence of HTTP 1.1 right now, saving server round trips is by far the most important performance optimization.

Yes, HTTP 2 may well change that strategy, so having the option to split things up is a good thing. I'm not necessarily convinced that themers are going to work out matching up one component CSS file with whatever set of conditions are needed to create that component (because it's likely not going to be merely the existence of the use of a module on a particular page) so that it will truly work like a dependency, but hey, that's fine to have that possibility out there.

I guess with ESI caching maybe having a different aggregated CSS file per page, so that the CSS is as small as possible, is a good thing, again as an option, not a requirement. I need to read more about HTTP 1.1 vs 2 to better understand the impact of that on server roundtrips, as latency is often a much bigger factor for performance than file size. But anyhow... as long as there's flexibility, then great.

Also, just to be clear, this isn't saying that the only way CSS can be added to a site is through a libraries.yml file, right? Because sometimes you need to do processing before you can add something in: using Typekit, and you need to dynamically load in your kit ID, for example. Or you need to add in some inline CSS in head for a specific purpose in certain situations.

Anyhow, as long as front enders do have flexibility in how they work with this (except for adding in via the .info.yml file, which I agree is an unnecessary duplication), I'm +1.

joelpittet’s picture

Issue tags: -Needs change record

themes are also forced to think about categorization in their CSS

themes are encouraged to create a library per component/topic/logical unit and hence are encouraged to not load all their CSS on all pages anymore, but to attach it when appropriate in a preprocess hook and/or only on the relevant pages via hook_page_attachments_alter()

Not a big fan of these two clauses, any way I can avoid categorization while declaring a CSS file?

I'm thinking more like:

Encourages logical groupings of assets to aid in correct order of dependencies.

And for consistency's sake if I'm not wrong currently I can't attach a library globally in an module's info file. Can we add that?

# MODULE.info.yml
libraries:
  - MODULE/library.name

I stubbed out a change record, feel free to update https://www.drupal.org/node/2379475

Wim Leers’s picture

#33: Thanks for taking the time to write up your thoughts! :)

I'm a bit confused though. First you say the front-end community is moving towards SMACSS. Then you say that asset libraries/componentization is counter to that.

IMHO it's actually the contrary.

Also note that it's entirely okay to have one or more libraries loaded on all pages! Things like typography, basic layout, and so on are of course necessary on every page.

It's only things like styling Views (unless almost every page is a View) or a specific View or a specific page that we should take care to only load the library when necessary.

If that class was only loaded on "relevant pages" this workflow would not be possible. I feel our CSS should be agnostic to the page it is sitting on. Yes, as themer, I would still have the ability to simply load all of my css on all pages. I just question if this would be useful. Interested in other opinions!

It's not clear to me what your concern is, but it's either:

  1. that the developer does not extend the existing reusable, modular CSS; or
  2. that the developer creates a new piece of CSS, instead of not doing anything, because it already exists; or.
  3. that we'd be encouraging to write non-reusable CSS?

Can you clarify that, so that I only have to answer your actual question rather than all three cases? :) I can answer all cases, and for every one, the answer is that your workflow is still supported.


#34: You wouldn't have to main _slideshow.css separately, they'd continue to depend on that _base.css file. You'd just put (the compiled) _base.css in a mytheme/base library and (the compiled) _slideshow.css in a mytheme/slideshow library and declare a dependency:

base:
  css:
    theme:
      css/base.css: {}

slideshow:
  css:
    theme:
      css/slideshow.css: {}
  dependencies:
    - mytheme/base

#35 + #36 + #37: Indeed, #37 is the correct solution. Please see bartik.info.yml and bartik.libraries.yml; they're the example that's in core. This issue is about making all themes use the same approach as Bartik.


#38:

So dependencies are very similar to _partials in SASS.

Indeed!

Overall it might even be possible to write a SASS plugin that creates drupal libraries in addition to the CSS files that are no partials ...

Yes! :)


#42: I like that idea of that extends property!


#43: Yep, you can still do everything exactly the same CSS-wise: if you don't want to create multiple libraries, then you don't have to. You can still have all your stylesheets in a single library. In fact, this is what Bartik does!

As the issue summary and #44 already say, you still continue to have stylesheets-remove and stylesheets-overrid; the only thing that's removed from a theme's *.info.yml file is the stylesheets property, in favor of the already existing libraries property. For consistency, and because stylesheets is merely a subset of what libraries can do anyway.


you need to dynamically load in your kit ID

We'll continue to have JS settings for this kind of dynamic settings. For the advanced things, there's also still hook_library_info_alter() + hook_library_alter().


#45: RE not being a fan of SMACSS: I can understand that, but that's nothing new. It's not being introduced here. We've had that for at least a year already, see https://www.drupal.org/node/1887922 and https://www.drupal.org/node/1887918#separate-concerns. That being said, it's totally possible to not use SMACSS; in that case just always use the theme category.

if I'm not wrong currently I can't attach a library globally in an module's info file

This is wrong; Drupal has never had the ability for modules to add CSS to every page. Because a module is inherently, well… modular and only affects certain pages or even only parts of certain pages. It's highly exceptional for a module needing to add assets to all pages. For themes, it's the other way around. Hence the libraries property is only supported in THEME.info.yml, not in MODULE.info.yml.

Thanks for the change record, I've refined it further — hope you like it. I've added sections with optional things: multiple libraries (e.g. one for node styling), libraries that use dependencies, SMACCS categoriziation, and even library-per-SMACSS-category (handy for subthemes). They're all optional. They're all interesting possibilities for experimentation that we get if we do this.
See https://www.drupal.org/node/2379475.

(There are some massive code block formatting problems, but that's due to the code filter on d.o that's doing crazy shit :()


Overall

  1. One of the main concerns seems to be an inability to continue to have a single CSS file (or several) that are loaded on every page. That's false; you continue to be able to do that, and Bartik does exactly this already today.
  2. Another concern seems to be SMACSS; but that was decided a long, long time ago. Besides, you can choose to not comply with SMACSS by just putting all your CSS files in the SMACSS theme category. I was under the impression that this was widely discussed and agreed upon, but apparently that's not true. I'm now guessing we only adopted it for Drupal core, but we don't want to enforce it on contrib. That works too, of course.
    • SASS' "_partials" is similar to Drupal's separate libraries plus dependencies between libraries. But SASS's approach is less flexible than Drupal's, because of the way that most people seem to use it: lots of partials (good!), but all compiled into a single CSS file (bad!). This is bad because it forces all the CSS to be loaded always.
    • This is fine for simple sites (I do this for my own simple blog also), but bad for more complex sites (i.e. sites with lots of CSS).
    • Aggregation should be up to the CMS, because the CMS knows what's necessary on the current page. Aggregation shouldn't be up to SASS (especially in a HTTP/2 world, which is quickly appearing).
    • I think a hybrid approach like what Fabianx said, to essentially expose SASS partials as Drupal asset libraries, would be a great intermediate. But no worries: this will never be enforced (we couldn't even if we wanted to!), it's just a practice that will have to be evolved over D8's release cycle.
Fabianx’s picture

I think the concern of #33 with a modular CSS approach is that something like http://warpspire.com/kss/styleguides/ is no longer possible.

However:

If you use SASS, then you can have e.g. a partial for _views.scss and a file + library views.scss (which just includes this partial) with library theme/views -> views.css.

But you can then still have a style-guide.scss that includes all the partials to automatically create your styleguide as usual.

Just because you have a catch-all style.css now does not mean we need to continue that :).

And nice base themes will be able to be created :).

davidhernandez’s picture

That change record is looking really nice. Thank you!

Can I get more explanation for the last section? "Optional: library-per-SMACCS-category" I don't quite understand the significance of the example. Is it saying that naming the library the same as the category lets a subtheme override it, but a custom library name cannot be overridden?

Also, it sounds like the categories are rigid, correct? (base, component, theme...) A themer can't decide their own to use? I also agree with Joel that it would be nice not to force categories at all if someone didn't want to use them.

Wim Leers’s picture

#49: I added that example because it was mentioned by sqndr in #6. It's up to the themer to decide how we wants to organize his CSS files in libraries. It can be 1 library, but it can also be 20. It's only saying that one way of organizing a theme's CSS more granularly is to comply with SMACSS and then create one library per SMACCS category. Again, you can do that, but you don't have to. I was under the impression that multiple people liked that idea a lot, but perhaps that impression was wrong.

It's not saying Is it saying that naming the library the same as the category lets a subtheme override it, but a custom library name cannot be overridden?. It only says that some base themes might like the idea of having one library per SMACSS category, because then a subtheme would e.g. only override the themename/layout library that contains all SMACSS "layout" CSS files. That's conceptually very easy to understand. And hence that could be one way of doing things.

It is just one example of how a theme can be creative with libraries to organize files better, and particularly how base themes can use a certain strategy for libraries to make it easier to implement subthemes.

I also agree with Joel that it would be nice not to force categories at all if someone didn't want to use them.

This is why the change record uses the theme SMACSS category in case you don't care about SMACSS categorization. Again, from #47:

#45: RE not being a fan of SMACSS: I can understand that, but that's nothing new. It's not being introduced here. We've had that for at least a year already, see https://www.drupal.org/node/1887922 and https://www.drupal.org/node/1887918#separate-concerns. That being said, it's totally possible to not use SMACSS; in that case just always use the theme category.

Dragan Eror’s picture

Guys, stop trolling, everything was already clear 20 comments ago. Any further discussion is just waisting of time (for writing and reading).
Defining CSS in info file is anyway "Drupal way of doing things" and the only one who might not like this idea are "old school Drupal people" who do not want to change their habits.

@alexpot, @Wim Leers thanks for this, one more thing we need here, just well written, easy understandable documentation (with several examples).

davidhernandez’s picture

@Dragon Eror, no one is trolling. People are trying to understand what this change means before forming an opinion of it, which is an important thing to do with such significant changes. All your trolling remark does is tell people who read this issue and haven't yet participated that their comments and questions are unwelcome.

catch’s picture

This is wrong; Drupal has never had the ability for modules to add CSS to every page. Because a module is inherently, well… modular and only affects certain pages or even only parts of certain pages. It's highly exceptional for a module needing to add assets to all pages. For themes, it's the other way around. Hence the libraries property is only supported in THEME.info.yml, not in MODULE.info.yml.

You could do this in Drupal 7, but it was committed without profiling, and too late to remove from Drupal 7 when I profiled it - the change required loading the entire .info contents on every request for every module. So it was one of the earlier Drupal 8 patch to rip that support out again.

Wim Leers’s picture

#53: oh, HAH! Nice! That must be one very unknown gem :P

Dragan Eror’s picture

@davidhernandez No, I was referring to people who submit comment without reading all previous comments and repeating same questions. With that starts to be hard to follow important parts of discussion. Of course comments are welcome, but please, read everything from beginning slowly before you ask something what is already explained in earlier comments.

@Wim Leers I forgot to tell I'm willing to help with documentations...

davidhernandez’s picture

I was referring to people who submit comment without reading all previous comments and repeating same questions

Fair enough, but lets be patient. Wim's responses are very long, and very thorough, and we're not all as smart as him. :) We can work on getting some of this explanation in the change record. I think that will be really helpful for other people. That's of course assuming we're willing to make a decision? I'm comfortable stating my opinion, but leave "voting" to Joel and Fabian and others who are the actual theme system maintainers.

lauriii’s picture

I think this issue is important and should be properly discussed so its good to have comments here.

#53 Lol!

I like the path we are moving on this issue because of the consistency with modules. I didn't read everything from the discussion so sorry if this is duplicate but it could be nice if you could easily remove whole libraries in a *.info.yml file. Then when you have a base theme that has a lot of styles but you don't want to have anything else than the base level on your own theme you could just choose to remove everything else.

Jeff Burnz’s picture

To be frank this is how I am working in D8 already so I am pretty down with how libraries work and how to use them. Its all good and I love dependancies, just magic and eased my pain a lot.

@27/28 one reason many base/starter themes load one big sheet is that it's a "reasonable compromise", in the grand scheme of all things to consider when building generic base themes, not that its the best way by any means. Users have it ingrained that one stylesheet is best for performance, therefor best for SEO, there is massive pressure to reduce the number of stylesheets loaded to that magic number: 1.

+1 based on the issue summary points 1, 2 and 4.

3, 5 I am not fussed on, we can encourage people this way or that - I think thats OK, for me this does not matter, I do it my way and libraries lets me do that, so thats just great.

Jeff Burnz’s picture

@57 yes but "remove whole libraries in info" is probably not for modules as well? But at the same time we should move towards consistency, not away from it, for example you can surely do this in hook_page_attachements_alter() and just blow them away or replace with your own. At least thats how I see it, move towards using the same tools for the same jobs, I use those tools and have for years, I am not a module dev, I just build themes, but I dislike working in info files, they seem like voodoo magic to often.

markabur’s picture

#34: You wouldn't have to main _slideshow.css separately, they'd continue to depend on that _base.css file. You'd just put (the compiled) _base.css in a mytheme/base library and (the compiled) _slideshow.css in a mytheme/slideshow library and declare a dependency:

Quick clarification: by definition, a SASS partial such as _base.scss won't compile into a standalone CSS file. So _base.css, _slideshow.css, etc. are not possible in a SASS workflow. And generally speaking, the _base.scss file won't have any CSS in it, so compiling it as a standalone file wouldn't make sense even if it was possible.

mdrummond’s picture

Quick note on the change notice at https://www.drupal.org/node/2379475:

<strong>Before</strong>
example.info.yml:

name: Example
type: theme
core: 8.x
stylesheets:
  all:
    - css/style.css
  print:
    - css/print.css

<strong>After</strong>
example.libraries.yml:

libraryname:
  css:
    theme:
      css/style.css: {}
      css/print.css: { media:print }

These aren't quite the same as the media query declaration of all is missing in this example. all and only can be important trigger words if you want to exclude older browsers from processing the CSS. Not something you want to do all the time, but just want to make sure that is still a possibility.

One other really important thing to note here. In looking at the documentation for how libraries work for JS (https://www.drupal.org/theme-guide/8/adding-javascript) and CSS (https://www.drupal.org/theme-guide/8/adding-stylesheets), it's noted that not only does the CSS and JS need to be added to the libraries.yml, and the library needs to be added in .info.yml, but also sitewide libraries need to be added via a HOOK_page_attachments_alter function.

It would be really nice if libraries.yml had an all category to put sitewide assets into in order to avoid this. Attaching CSS in an info file was nice and simple: now we need entries in the .info.yml file, libraries.yml files and the .theme file just to add a sitewide CSS file. Maybe I'm misunderstanding this, but it seems like somewhat clunky TX.

Also, while the issue summary talks about consistency as the goal of this issue, which is fine and good, it looks like from the timeline that https://www.drupal.org/node/2368797 was what triggered this discussion. Reducing what goes into the first TCP packet is also important, but it seems important to note that this is one of the key reasons we're creating a more complicated workflow for themers, and from what I can understand, making libraries the only way to add CSS for a theme.

I'll also quote what I said in https://www.drupal.org/node/2298551#comment-9366295:

The end result of this is that our performance optimization is based on trying to only load the relevant CSS/JS for a particular page, thus reducing the file size of any aggregated files. However, this also greatly reduces the ability for browsers to cache these assets as somebody navigates throughout the site. Rather than loading the JS and CSS up front when first visiting the site, and then using cached versions of those files as somebody goes from page to page, instead the browser will need to load new JS/CSS aggregate files on pages with different requirements. Overall, that means both larger total file sizes being downloaded and an increase in the number of times the browser hits the server, taking a latency penalty each time. If page A has a 12k script aggregate file due its dependencies and page B has a 14k script aggregate file due to its dependencies, but if dependencies were ignored there would have been a 20k script aggregate file, then somebody is downloading 26k of scripts vs 20k, which isn't a savings.

In an HTTP 2 world, without asset aggregation, this presumably matters less.

As has been pointed out, moving everything to libraries.yml doesn't prevent people from having Sass aggregate partials to one CSS file and attaching that as a library. In a lot of cases, I think a single aggregate CSS file is probably going to better for performance when working with HTTP 1.1, but your mileage may vary.

Jeff Burnz’s picture

@ #61, I have been adding a library in the info file and it does not need to be added in hook_page_attachments_alter() as well, or in preprocess, if you add your theme library in the info.yml file it gets loaded everywhere that theme is used, however I am not clear on what you mean by "site wide".

I can clarify my comments earlier to show how I am using libraries.

1. I have a lot of components, maybe 30 or so, it will depend on the theme.
2. I use libraries to group the components and load them conditionally.
3. Some libraries have dependancies, such as JS, which in turn have dependancies, e.g. - core/modernizr etc.

It's those conditions that are the real crucial bit and this is where I start getting on the fence a bit, is it better going forward to load, for example, my "forum library" only for forum pages, or every page. From what I can tell you need to have a pretty big CSS file to offset the cost of the http request. AFAICT drupal will create a new aggregate if I use a path/state strategy in my conditions, so this will force browser to fetch a new stylesheet, which will largely contain what they already had + the additional "forum library". Correct?

At the moment I am going with a strategy of module exists sort of thing, e.g. taxonomy, forum, etc and just load the library for every page. All loading is done in preprocess.

As you can see I am not using SASS to aggregate a set of partials, in fact there are almost no partials. Everything is categorised but the actual grouping is done in the library.

In effect I am decoupling components and loading, which gives me a lot of flexibility which is what a base theme needs to have.

The dependancy chain is a big win here, I can simply load one library and ensure everything I need is loaded in the right order at the right time, it's simply fantastic to be able to do this so easily.

Fabianx’s picture

#60: Correct, what I meant is, you have e.g. a libraries/base-styles.scss file, which includes _base-styles.scss partial (if it had base styles) and a slideshow.scss file (which includes the _slideshow partial).

Out of the box this is a probably too complicated workflow, but in the future more might be possible.

#62:

Thanks for sharing and I agree, especially using module exists, which is very similar to extending an existing library (as proposed by Wim), to attaching libraries is a good way to build a base theme in 2014, especially, because you can depend on the base library, so things are consistent.

In general libraries everywhere will make a lot of things in the caching / render layer simpler and with that futureproof it for performance.

In D7 this is kinda hell to get the order right and ensure the right things are attached for advanced caching, so if we do this issue to only give me a favor I am fine too :-D.

Cottser’s picture

I haven't had a chance to read all the comments here but I am in favour of what is proposed in the issue summary and the draft change record.

I think we should remove the pros and cons section from the change record, that information can be gleaned from the issue summary if folks are interested. And on that subject, very very minor but shouldn't there be a space here:

media:print

should be (?):

media: print

I made one other minor edit but didn't want to make that edit because I'm not sure about it.

joelpittet’s picture

@Wim Leers you are a machine, you like responded to everybody! Kudos and @Wim Leers++

joelpittet’s picture

@Cottser I agree and I've removed the pros and cons section. Good call.

joelpittet’s picture

@Wim Leers regarding #47

This is wrong; Drupal has never had the ability for modules to add CSS to every page. Because a module is inherently, well… modular and only affects certain pages or even only parts of certain pages. It's highly exceptional for a module needing to add assets to all pages. For themes, it's the other way around. Hence the libraries property is only supported in THEME.info.yml, not in MODULE.info.yml.

I'm going to disagree with this being an exception, because IMHO many modules should provide global CSS and I believe it would improve performance because it would get aggregated into the same CSS files and thus letting the browser cache it instead of the browser having to cache the specific page that it's attached to.

Now I'm not saying that should be the case in all circumstances, and I do consede it's rare, but I do think the use-case if not currently previlent should be more so for that reason. And the modules I'm going to pick on are ones like flag, and all the 3rd party modules like twitter, instagram, campaign monitor, mailchimp and chosen.

They could all and most do provide their own CSS that should be applied to all pages, and as a themer I'll likely (as previously mentioned) rip them right out and move them into a partial to clean up my precieved inacciquecies in their implementations. Having a default that looks decent out of the box for a demo is great.

All of the above can still be done with #attached in a preprocess or build render array. The intent is to propose for consistency modules should have library declarations too. Maybe I should follow-up this request or do you think it could be included here?

Fabianx’s picture

#67: Follow-up please, the reason this was ripped out is performance, because else you need to load all .info files to memory during a request. And while it can be cached, it is still a lot of scanning.

joelpittet’s picture

@Fabianx ah that makes a bunch of sense, thanks for the details. And somehow I missed #53 which said the same thing with different words:) thanks @catch also!

Wim Leers’s picture

Issue tags: -Needs Documentation

#58 + #59 + #62: Thanks for explaining how libraries help you a lot as a themer :)

#60: Noted; but I'm sure you can quite easily have SASS generate multiple CSS files.

#61: the all media query is the default. Regarding a theme attaching a library on all pages (unconditionally): that can be done simpler, using the libraries key in a theme's *.info.yml file. I updated the documentation page now: https://www.drupal.org/node/2216195/revisions/view/7857605/7865503 + https://www.drupal.org/node/2216195/revisions/view/7865503/7865513. https://www.drupal.org/theme-guide/8/adding-javascript is very much complete. And once this goes in, https://www.drupal.org/theme-guide/8/adding-stylesheets is very much unnecessary, since JS and CSS assets are handled in exactly the same way; if this issue lands, I'll update https://www.drupal.org/theme-guide/8/adding-javascript to cover both CSS and JS.

#67: That's an eternal discussion that's finally going to become moot once HTTP/2 becomes widely deployed :) But today, in a HTTP/1 world, it depends on the site's complexity, it depends on the "average user", and it depends on so much more. If Drupal is given global (non-modular) CSS assets, then it doesn't have the ability to attach them only when necessary. If it's given modular CSS assets, then it can still choose to attach them globally. Historically, this has been nigh impossible. BUT! One huge advantage that I forgot to mention in this issue is the following: if all modules and themes provide modular/componentized CSS assets, and declare them as libraries… then Drupal 8 has a complete overview of all the available CSS assets on the site, which means we can write an allcss module that simply attaches all CSS on every page! In Drupal 7 and before, it was impossible for Drupal to get a complete overview of all assets that could ever be loaded. Thanks to declaring all assets in libraries, Drupal has complete insight, and that allows us to make such interesting use case-dependent optimizations.
I vouch to write such a module, if anybody wants it. (And I think I'll personally want it for my own site, which is a use case where that'd be more optimal.)


It seems like most people are now on board with this change? If so, is there anything left to be done before this is RTBC?

sqndr’s picture

+++ b/core/themes/seven/seven.info.yml
@@ -7,39 +7,8 @@ tags: 'multi-column, fluid, responsive, sans-serif, accessible'
+libraries:
+ - seven/styling

+++ b/core/themes/stark/stark.info.yml
@@ -4,8 +4,7 @@ description: 'An intentionally plain theme with almost no styling to demonstrate
+libraries:
+  - stark/styling

Wouldn't it be nice if we changed it in Bartik as well? Bartik still has a base as library name for the css. Would be nice if it was styling too. No?

davidhernandez’s picture

...then Drupal 8 has a complete overview of all the available CSS assets on the site...

Stop, I can't handle this much win! :)

If so, is there anything left to be done before this is RTBC?

Yes. Has anyone reviewed the patch?

LewisNyman’s picture

Wouldn't it be nice if we changed it in Bartik as well? Bartik still has a base as library name for the css. Would be nice if it was styling too. No?

IMO styling also seems like a bad name. Every CSS file is styling. Maybe something like 'global'?

Wim Leers’s picture

#71: agreed!

#73: global doesn't imply it's global styling. What about globalstyling then?

LewisNyman’s picture

Issue summary: View changes

Bug because themes not using libraries provides an inconsistent DX/TX, violates the coding standards (no SMACSS), prevents dependencies from being declared on theme CSS assets.

I've modified this part of the beta evaluation because you don't need to loading the CSS over libraries to be SMACSS compliant. Seven is currently the most SMACSS compliant part of core and it isn't using libraries.

LewisNyman’s picture

Wim and I quickly discussed this on IRC and I think we agree globalstyling is the most descriptive name for this library.

Wim Leers’s picture

FileSize
23.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,755 pass(es). View
5.86 KB

#75: fair point!

Per #73 + #74 + #76: standardizing on global-styling (everywhere else we split up multiple words by using dashes, so doing the same here).

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Given all feedback has been addressed and it was only asked for a review, I did a review and found nothing outstanding.

Therefore: RTBC

catch’s picture

Status: Reviewed & tested by the community » Fixed

Given libraries can be declared in libraries.info.yml by themes, the consistency here outweighs the potential updates that contrib/custom themes may need to do.

If we were only making the change for this reason, then I'd be iffy on beta suitability, but given it helps a lot to unblock both #1014086: Race conditions, stampedes and cold cache performance issues with css/js aggregation and #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests (both longstanding performance issues in core, AjaxPageState makes me sad every time I view source on any Drupal site), that tips it well over the edge.

Committed/pushed to 8.0.x, thanks!

  • catch committed 0bfb32b on 8.0.x
    git commit -m 'Issue #2377397 by Wim Leers, alexpott: Themes should use...
nod_’s picture

Glad to see that in. Especially glad to hear about things like #62 (despite a relative lack of communication/explanation about the topic), that's exactly why library use was generalized and as catch said, it's much needed for JS stuff down the line.

catch’s picture

Also just a note on #62:

AFAICT drupal will create a new aggregate if I use a path/state strategy in my conditions, so this will force browser to fetch a new stylesheet, which will largely contain what they already had + the additional "forum library". Correct?

There's one aggregate for 'every page' assets, and one aggregate for 'per page' assets - this counts for both JavaScript and CSS.

This means the only duplication happens when:

- something is added with the per-page flag but is actually conditional.
- there are lots and lots of different per-page assets on different pages

Otherwise there should be a single site-wide (or at least theme-wide) every page aggregate, and a handful of per-page with different items in.

joachim’s picture

Status: Fixed » Active

Documentation needs to be updated:

- https://www.drupal.org/theme-guide/8/adding-stylesheets
- https://www.drupal.org/node/2349827

and possibly other pages too.

Wim Leers’s picture

Status: Active » Fixed

I've now updated both.

Status: Fixed » Closed (fixed)

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