As some have noticed, the Seven theme (and I bet a lot of other themes out there) contains a pretty massive CSS reset, which sets most styles from system.css right back to zero.

One might ask, why not override system.css completely (by having a file with the same name inside the theme) instead of resetting everything?

I guess in most cases, the problem is that system.css contains a lot of 'functional' styles. Styles that are pretty much required in order to be able to use Drupal properly. Think of #autocomplete, body.drag, etc. These are all styles that themers probably don't want to touch.

However, not overriding system.css completely means you get some other, non-functional styles too, like the gray table backgrounds and lots and lots of margins and paddings. Themers don't seem to like these styles. They don't seem to want or need the default margins, paddings, borders and gray tables. (I'm also speaking for myself here btw :P)

Which results in a monster CSS reset, as happened with Seven.

The problem could be solved by seperating the 'functional' styles from the non-functional, 'stylish' styles. We have system.css and defaults.css already, so why not use system.css just for functional styles and defaults.css for the stylish default styles. This would make themers able to easily override defaults.css without having to override all the functional stuff too. Yay :)

I have attached a patch which simply moves some of the styles from system.css from defaults.css, no other changes. It's just a quickie, there might be some other styles that need to be moved around across these two files, but I'll await some feedback on this first.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jix_’s picture

Status: Active » Needs review
alanburke’s picture

Issue tags: +Needs themer review
Jacine’s picture

Status: Needs review » Reviewed & tested by the community

I support this. It's a small change, but a big improvement for the themer experience :)

alanburke’s picture

Good move.
A small step along the road to this.
http://groups.drupal.org/node/20109

quicksketch’s picture

I totally support this change also, a great collection of styles to move to defaults.css. All the moved styles do seem to be entirely display, keeping all the absolutely necessary styles in system.css. RTBC + 1.

jix_’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
23.38 KB

I created another patch, this one is a bit more extreme but I think it's needed because the original system.css has become a bit of a maze, especially after the seperation between functional and non-functional.

So the new patch is really not just about seperation anymore but serves as a cleanup too. I pretty much re-ordered everything, fixed a lot of whitespace and added comment blocks here and there. I haven't touched any actual properties or selectors, so the patch shouldn't have any impact on how Drupal looks.

I also added @file comment blocks:

defaults.css

/**
 * @file
 * Defines default styles for generic and common elements.
 */

system.css

/**
 * @file
 * Defines system-wide styles to support certain functionalities.
 */

Developers and themers new to Drupal will understand the difference between these files quicker this way.

I changed the comment block style from …

/*
** Comment block
*/

… to …

/**
[space]* Comment block
[space]*/

… which seems to be the standard in Drupal.

There seem to be some 'lost' styles in there btw, like .block ul. (That one should really be in block.css, right?) It's probably out of scope for this issue though.

jix_’s picture

Status: Reviewed & tested by the community » Needs review
jix_’s picture

Status: Reviewed & tested by the community » Needs work

Ah, I just noticed quicksketch’s comment in #575294: Add reset.css through drupal_add_css() instead of through the .info file about whitespace between styles. I was confused about CSS coding standards in Drupal, so I added whitespace in the previous patch that I probably shouldn't have added. I'll update the patch soon.

jix_’s picture

Status: Needs work » Needs review
FileSize
16.1 KB

Right. Forget the previous patch, it's embarrasing ;)

Here's the updated version.

jix_’s picture

FileSize
16.02 KB

Moved .item-list .icon back to system.css since that one can be considered 'functional', with the following styles still in defaults.css:

.item-list .title {
  font-weight: bold;
}
.item-list ul {
  margin: 0 0 0.75em 0;
  padding: 0;
}
.item-list ul li {
  margin: 0 0 0.25em 1.5em; /* LTR */
  padding: 0;
  list-style: disc;
}
alanburke’s picture

+++ modules/system/system.css	14 Oct 2009 12:09:22 -0000
@@ -376,33 +269,9 @@ html.js .resizable-textarea textarea {
-/*
-** Teaser splitter
-*/
+/**
+ * Teaser splitter
+ */
 .joined + .grippie {
   height: 5px;
   background-position: center 1px;
@@ -424,9 +293,9 @@ html.js .no-js {
@@ -424,9 +293,9 @@ html.js .no-js {
   display: none;
 }

Isn't the teaser splitter now gone?

This review is powered by Dreditor.

jix_’s picture

Yes it is, hehe. I guess nobody took the time to remove those styles yet. Should I update the patch for this or would it be better to do it in another issue (to prevent any possible confusion)?

alanburke’s picture

Generally a patch should do one thing at a time.
Probably best as a seperate issue?

jix_’s picture

Ok, we'll deal with that somewhere else.

How about the @file blocks btw? I wasn't sure if those are recommended or even allowed in stylesheets.

Status: Needs review » Needs work

The last submitted patch failed testing.

jix_’s picture

Status: Needs work » Needs review

Invalid PHP syntax in modules/system/defaults.css.

lol, what?

jix_’s picture

FileSize
17.56 KB

Excuse all the re-rolls, I think this will do.

Styles in defaults.css:

  • item lists
  • pagers
  • tables
  • forms
  • "more"/"help" links
  • messages, warnings, errors
  • breadcrumb
  • local tasks (primary + secondary tabs)
  • filter tips
  • .clearfix

Styles in system.css:

  • drag and drop
  • multiselect form
  • inline items (.container-inline div, .container-inline label)
  • autocomplete
  • collapsing fieldsets
  • resizable textareas
  • teaser splitter (obsolete, but we'll remove that in a seperate patch)
  • progress bar
  • formatting for welcome page
  • installation task list
  • installation clean URLs
  • styles for the system modules page
  • styles for the system themes page
  • password strength indicator
  • password confirmation checker
  • .nowrap, .element-hidden, .element-invisible, html.js .js-hide

Just to clarify: I got rid of the @file blocks for now. Should probably be a seperate patch anyway.

jix_’s picture

FileSize
18.22 KB

Okay seriously, someone ought to slap me.

I messed up the order of table drag styles in the previous patch.

jix_’s picture

Status: Needs review » Reviewed & tested by the community

Since the last patch got lots of positive reactions on Twitter (like "oh god, please let this go in" and "tons of thanks for this"), plus the three reviews above about the earlier patch which was basically the same thing, I'm just going to mark it as "reviewed by the community".

jix_’s picture

I might just do another patch to change the comment block "More"/"Help" links to just "More" links.

jix_’s picture

FileSize
18.21 KB

Here it is.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Looks like the latest patch still needs review.
Could you please hold off the RTBC status until you're actually done at at least one or two of the #d4d crew has given this a thumbs-up? ;)

jix_’s picture

Ok, my bad! :)

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for staying on this mverbaar! I just reviewed it again, and I think it's RTBC.

Dries’s picture

I know the original patch was already committed but I find "defaults.css vs system.css" still vague. I'm all for splitting it up, but I'd prefer to have file names that are more explanatory. Given a CSS snippet, based on the file names, I wouldn't be able to figure out which file to put it in without studying the contents of the files. The separation (see comment #17) feels random to me. It would be good to better explain the difference at the top of each file...

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

webchick’s picture

Testing bot running amok. But it sounds like this is needs work anyway?

jix_’s picture

I know the original patch was already committed but I find "defaults.css vs system.css" still vague. I'm all for splitting it up, but I'd prefer to have file names that are more explanatory. Given a CSS snippet, based on the file names, I wouldn't be able to figure out which file to put it in without studying the contents of the files. The separation (see comment #17) feels random to me. It would be good to better explain the difference at the top of each file...

So, you are suggesting we put @file back in the files, like in #6?

defaults.css: "Default styles for common elements."
system.css: "Styles to support system-wide behaviors like draggable table rows and collapsing fieldsets."

The file names are fine I think, although perhaps it might be something to rename system.css to something like behaviors.css?

The only things that feel random to me at the moment are the admin-related styles (e.g. the styles for the "themes" and "modules" pages), which probably shouldn't be in system.css OR defaults.css (but rather in admin.css). I'm not sure if this is something to do in this patch or separately?

I'm not sure what you mean by "the original patch was already committed" btw. HEAD still contains the "old" defaults/system.css, right?

webchick’s picture

I went on a tirade this morning clearing out the RTBC queue and I think Dries thought that I committed this patch already, but I haven't yet, since it still needed review at that time.

I think what's being asked for here is not @file attributes at the top of the CSS files, but rather renaming the files themselves so that it's clear to a core developer, "I just added a style for X. That should obviously go in Y.css" and also clear to a themer without Firebug (just humour me for a second ;)) "I'm looking to override the X styles. I'm sure that would be located in Y.css."

Right now, the distinction between system and defaults seems totally arbitrary. deafults.css is almost like a "page-elements.css" or something. But yet then there's stuff like drag and drop and progress bar in system.css. Hm.

Anyone have some thoughts on this?

jix_’s picture

I agree it's a bit arbitrary.

My primary goal with this is to just be able to override something like a defaults.css, hit command (ctrl) + A, backspace and basically just start with naked HTML, without losing any crucial system-related functionality. If I would do this with system.css, I wouldn't be able to drag table rows or collapse fieldsets anymore.

So I'm basically separating default styles (defaults.css), and Drupal-specific default styles (system.css).

I'm thinking of html-defaults.css + system-defaults.css now ... or maybe even drupal-defaults.css.

Or just ditch the word "defaults" and name them html-elements.css and drupal-elements.css.

quicksketch’s picture

How about "system-functionality.css" for functional CSS and "system-defaults.css" for stylistic defaults? Just a note that we should probably rename "defaults.css" to start with the name of the module anyway so it's less likely that themes will override "defaults.css" inadvertently. System module shouldn't be exempt from module name prefixing.

Jacine’s picture

I agree that some of the admin stuff is a little random, but only a small part of it. I thought we would address these things after this patch, but it seems some of this is causing confusion so here's the nit-picky version of my review...

1. The installation stuff doesn't belong anymore, and I would suggest moving it to maintenance.css. Installation typically happens once, and IMO is a "maintenance-ish" task. There is no point in loading this on every page forever.

/**
 * Installation task list
 */
ol.task-list li.active {
  font-weight: bold;
}

/**
 * Installation clean URLs
 */
#clean-url.install {
  display: none;
}

2. Aside from these being poorly coded, i.e. div.incompatible { font-weight: bold; } would be better at the very least, if these can't replaced with <strong> (more semantic markup). Since they are not, I would leave this in the functional category for now.

/**
 * Styles for the system modules page (admin/config/modules)
 */
#system-modules div.incompatible {
  font-weight: bold;
}

/**
 * Styles for the system themes page (admin/appearance)
 */
#system-themes-form div.incompatible {
  font-weight: bold;
}

3. I believe this stuff is no longer applicable, so it needs to go. There is no "welcome" page or teaser splitter anymore (as noted in #11). Since there is confusion, maybe it should go in this patch? Only one I'm not sure about here is html.js .no-js

/**
 * Formatting for welcome page
 */
#first-time strong {
  display: block;
  padding: 1.5em 0 .5em;
}

/**
 * Teaser splitter
 */
.joined + .grippie {
  height: 5px;
  background-position: center 1px;
  margin-bottom: -2px;
}
/* Keeps inner content contained in Opera 9. */
.teaser-checkbox {
  padding-top: 1px;
}
.teaser-checkbox div.form-item {
  float: right; /* LTR */
  margin: 0 5% 0 0; /* LTR */
  padding: 0;
}
textarea.teaser {
  display: none;
}
html.js .no-js {
  display: none;
}

Once you remove all that, everything else seems "system" appropriate and entirely "functional" to me. The progress bars, resizable textareas and autocomplete fields are used in admin and user facing forms, and in most cases cannot be removed without serious thought/pain from a themer standpoint because removing them breaks stuff and also modules build on top of them. I've been there, and it's a huge WTF. This patch as is it is now is a nice step in the right direction to solve that.

I think adding a @file description should happen, but I don't know that changing the names of the files themselves is something that should happen in this issue/patch. The separation of functional vs. presentational CSS is a growing problem with the increasing use of jQuery/Javascript so the file name discussion is an important one. We should give it a bit of thought with a larger audience and consider the impact that what is done here could have on coding standards in general. Personally, I hate the thought of more stylesheets, but I would vote for these styles being separated across the board, the same way we do x-rtl.css. Yes, this adds more dreaded CSS files, but at least it presents the opportunity to easily organize and differentiate what is going to break your site vs. what is "extra" (i.e. what a themer often needs to override/fix).

system.css (would contain the default presentational styles for system.module, which should just be assumed since it's a CSS file)
system-functional.css (would contain the functional styles for system.module)

Jacine’s picture

Also, just wanted to add re @webchick in #29:

I think what's being asked for here is not @file attributes at the top of the CSS files, but rather renaming the files themselves so that it's clear to a core developer, "I just added a style for X. That should obviously go in Y.css" and also clear to a themer without Firebug (just humour me for a second ;)) "I'm looking to override the X styles. I'm sure that would be located in Y.css."

Part of the problem here (as I see it) is that the theme functions are not logically split up into the files/location you would expect them to be in. For example: modules/system/defaults.css contains CSS for local tasks. The theme functions for these are:

theme_menu_local_task()
theme_menu_local_tasks()
theme_menu_local_action()

I would expect to find these functions in menu.module, or menu/menu.inc because of how they are named and because they relate to menus. Instead they are in includes/menu.inc (wtf). If it were just that, I'd think it would make sense to move them into modules/menu/menu.css, but there is no menu.css. Some of the menu-related styles are in modules/system/system-menus.css (wtf), and then others are in modules/system/defaults.css (wtf again)... And there are many more examples like this. This is part of what makes it near impossible to theme Drupal without Firebug, api.drupal.org and the attention span and will to take the time to figure it all out. LOL.

I'm sure there is a reason why these theme functions and their CSS are not located in their respective module directories, but I don't know what that reason is so it's hard to make sense out of how to clean it up (or if it can be cleaned up). Maybe someone can enlighten me ;) Are we still looking to clean this up in D7?

jix_’s picture

Good points about the naming convention. I agree it would be a good solution to come up with a Drupal-wide standard for style separation. Using {module name}.css for all presentational styles sounds good, now we just need a clever name for the other one. {module name}-functional.css as suggested above sounds good, although it might still be a bit blurry. (As in, what's "functional" about them.) Since the "functional" styles all support a certain behavior, perhaps {module name}-behavior.css?

It would come down to basically put everything in {module name.css}. As soon as you are adding styles that support certain behavior, behavior that should not be lost when overriding the module's default stylesheet, put those styles in a separate {module name}-behavior.css. Modules that don't add any behavior-related styles could just stick with the regular {module name}.css like they're used to.

For system.css there would still be some exceptions, like the current admin.css and system-menus.css. Dealing with those somehow should probably be a separate patch. I agree with #32 that moving some of the styles from system.css to admin.css is something to do in this patch.

webchick’s picture

The tricky thing with adding styles to something like menu.css is that Menu module can be disabled. This will remove the UI for moving menu items around, but doesn't remove the concept of a menu from Drupal. So styles/theme functions like this do end up in system.module.

jix_’s picture

Perhaps the menu module should at some point be split into menu.module and menu_ui.module then. (And make menu.module a required module.)

*Keeps on dreaming ...* ;)

jix_’s picture

FileSize
28.24 KB

Updated the patch.

  • Introduces system-behavior.css and system-behavior-rtl.css.
  • "Renames" defaults.css and defaults-rtl.css to system.css and system-rtl.css.
  • Moves some styles from system-behavior.css to maintenance.css.
  • Removes styles from the teaser splitter and welcome page from system-behavior.css.
webchick’s picture

Title: Seperate 'functional' and 'stylish' styles in system.css and defaults.css » Re-organize styles across stylesheets from system.module and separate presentational and behavior-supporting styles
Status: Needs work » Needs review

Could you provide a nice summary like you did back in #17 that explains where stuff is now?

jix_’s picture

Status: Needs work » Needs review

system.css:

  • item lists
  • pagers
  • tables
  • forms
  • "more" links
  • messages, warnings, errors
  • breadcrumb
  • local tasks (primary + secondary tabs)
  • filter tips
  • .clearfix

system-behavior.css:

  • drag and drop
  • autocomplete
  • collapsing fieldsets
  • resizable textareas
  • progress bar
  • multiselect form
  • password strength indicator
  • password confirmation checker
  • styles for the system modules page
  • styles for the system themes page
  • inline items (.container-inline div, .container-inline label)
  • .nowrap, .element-hidden, .element-invisible, html.js .js-hide
  • teaser splitter (removed)
  • formatting for welcome page (removed)

maintenance.css:

  • installation task list
  • installation clean URLs
jix_’s picture

Perhaps the primary and secondary tabs should move to system-menus.css while we're at it?

seutje’s picture

subscribe

yoroy’s picture

#40: Better to get this committed sooner than later. There's always room for improvements but the scope of changes is already pretty big no?

I'm saying this because there's issues being postponed on issues that are postponed on this one so it's becoming a bit of a bottleneck for further improvements. (#575294: Add reset.css through drupal_add_css() instead of through the .info file and #547068: D7UX: use Seven for installation / updates)

Great work so far!

deekayen requested that failed test be re-tested.

jix_’s picture

Better to get this committed sooner than later. There's always room for improvements but the scope of changes is already pretty big no?

I agree.

If anyone would like to take the time to review the last patch (#37) so we can mark this rtbc again ...

davidtrainer’s picture

Patch in #37 does not apply.

webchick’s picture

Status: Needs review » Needs work

Bleh. BAD testing bot, BAD.

jix_’s picture

FileSize
18.06 KB
jix_’s picture

Forgot to fakeadd the behavior stylesheets.

davidtrainer’s picture

I'm not too familiar with this patch, so I may be doing something dumb - but I applied it and cleared my cache, and it appears that system-behavior.css is not being loaded, which is causing some things to break.

jix_’s picture

I did a quick "find in project" with my editor for the strings "system.css" and "system-rtl.css". They should all be covered now.

I'm not sure about system-rtl.css btw. I found a reference to that in common.test but not in the actual module. Is that correct?

Status: Needs review » Needs work

The last submitted patch failed testing.

jix_’s picture

Ah I got a little sloppy in common.test I see. New patch coming up.

jix_’s picture

The last submitted patch failed testing.

jix_’s picture

Status: Needs work » Needs review
FileSize
31.4 KB
seutje’s picture

applying this patch left me with a defaults.css and defaults-rtl.css which contained nothing but this and an empty line:
/* $Id: defaults.css,v 1.6 2009/02/18 14:28:23 webchick Exp $ */

and these were not used anywhere (which is the intention I assume)

I took the liberty of deleting these 2 files, hope u don't mind :)

other than that, I didn't notice anything weird

jix_’s picture

Yes, that's what I meant to do. Didn't know I could actually delete them in a patch. :)

Re-test of from comment #2284710 was requested by @user.

Jacine’s picture

@webchick #35 - Thanks for the explanation. I figured there was a reason ;)

I went to test this again last night and it would not apply so I rerolled #56. As far as actual CSS styles and where they end up, mverbaar's summary in #39 is still where we are at.

Note: The patch is a couple hundred lines less because I didn't apply coding style cleanups (except in the new files: system-behavior.css and system-behavior-rtl.css), because they made the patch harder to read/reroll and I figured that doing them again would only further complicate this patch.

Hope it's not too late.

Jacine’s picture

Reroll.

jensimmons’s picture

I am all for this improvement too! Yes, please, let's get this in.

Sorry I can't test the patch though, I have a no-patch rule for myself on my D7 install to maintain as much sanity as I can while writing a D7 theme.

jix_’s picture

Got a small error when applying the patch in #62:

Hunk #7 FAILED at 277.
1 out of 8 hunks FAILED -- saving rejects to file modules/system/system.css.rej

So here's another reroll.

Jacine’s picture

Just tested #64 on a fresh install, and it looks good to me. Only tiny thing wrong with it is a few "\ No newline at end of file." This was probably my doing from one of the earlier patches, and it was just brought to my attention that it is a no-no.

This re-roll fixes that, and I think it's ready. If someone could just take a look, I think we can mark this RTBC :)

jix_’s picture

Status: Needs review » Reviewed & tested by the community

Perfect! :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Since the bulk of the work was done on this in mid-November, prior to markup freeze, and since I just can't bear to see grown themers cry, I committed this to HEAD. :) Great work, folks. This looks like a nice clean-up and improvement.

We'll need a note in the theme upgrade guide at http://drupal.org/update/theme/6/7 about the CSS filename change, and the general re-organization. Marking needs work for documentation.

Jacine’s picture

Status: Needs work » Needs review

Yay! :D

Ok, I added docs here: http://drupal.org/update/theme/6/7#system-stylesheets

Not sure whether to mark needs review, or fixed, so marking needs review.

yoroy’s picture

Status: Needs review » Fixed

Docs are to the point and reference back to this issue, marking fixed.

mcrittenden’s picture

Untagging.

Status: Fixed » Closed (fixed)

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