CSS Files are in major need of clean up!

You are absolutely right.

First things I'm noticing that definitely belong in admin.css:

• Styles for the system modules page (admin/modules) (now in system.css)
• Styles for the system themes page (admin/appearance) (now in system.css

I think, another thing that really needs cleaning up is the whitespace everywhere (although it has lower priority). Hopefully without starting all over with the CSS coding standards debate again. The current standard in core style sheets seems to be to not put a blank line between blocks. A blank line is only inserted when we want to group certain styles. In system-messages.css, for example, line 13, 22 and 31 should be removed in order to properly group errors, warnings and 'ok' message styles.

If I understood correctly you are already working on a patch; I'll post further comments/ideas/reviews/more patches/and so on once your patch arrives :)

Sweet!

Great job on all the comments. I noticed that you changed all "Formatting for [status report, theme overview, etc.]" comments to just "Status report", "Theme overview" etc. The appearance styles still says "Styles for the Appearance page", I think it would be good to remove the "Styles for the" there as well.

I remember a conversation about @file blocks at the top of style sheets; Dries or Webchick saying they shouldn't be necessary. The file name should tell people what should or shouldn't be in there. But since a lot of styles were put in system.css (where they didn't belong) again after that issue, I think we don't really have much of a choice but to add comments to make it extra clear.

I was thinking about system-menus.css (which contains ul.menu and ul.links) lately; .tabs and .pager are still in system.css. What is your opinion on creating a system-navigation.css which would contain styles for ul.menu, .links, .tabs and .pager all together (and remove system-menus.css)? Is that too radical?

Looks awesöme! (Spreading the Drüpalcon spirit ;)

And here's a placeholder follow-up issue: #892486: Put navigation-related styles from system module in system-navigation.css

+++ modules/filter/filter.css	23 Aug 2010 04:23:18 -0000
@@ -44,3 +44,10 @@
+.tips {
+  margin-top: 0;
+  margin-bottom: 0;
+  padding-top: 0;
+  padding-bottom: 0;
+  font-size: 0.9em;
+}

huh? hah! I actually believe that this can be dropped entirely. Did not even know that there's styling for .tips somewhere.

+++ modules/system/admin-rtl.css	23 Aug 2010 04:23:18 -0000
@@ -1,9 +1,11 @@
+/**
+ * Configuration listings
+ */

(and elsewhere) Like in phpDoc, also CSSDoc comments should form proper sentences (and end in a period [full-stop]).

+++ modules/system/admin-rtl.css	23 Aug 2010 04:23:18 -0000
--- modules/system/admin.css	28 Apr 2010 20:08:39 -0000	1.22
+++ modules/system/admin.css	23 Aug 2010 04:23:18 -0000

Separate issue (not to be changed in this issue): admin.css violates the module's namespace and prevents Admin module in contrib to use its own namespace. The filename should be system-admin.css.

+++ modules/system/admin.css	23 Aug 2010 04:23:18 -0000
@@ -1,26 +1,27 @@
+/**
+ * @file
+ * Contains styles which are specific to administration pages.
+ */

Can we shorten this to (and use as semi-standard):

"Styles specific to administration pages."

+++ modules/system/admin.css	23 Aug 2010 04:23:18 -0000
@@ -1,26 +1,27 @@
+ * Configuration listings

@@ -31,21 +32,17 @@ div.admin .right {
+ * Modules listing

@@ -115,26 +121,100 @@ table.screenshot {
+ * Theme Settings

s/listings/listing/
s/Modules/Module/
s/Settings/settings/

I think, no?

+++ modules/system/system-behavior.css	23 Aug 2010 04:23:18 -0000
@@ -262,6 +257,30 @@ div.password-confirm {
+/**
+ * To be used with displace.js
+ * ?? There is no such file. I think this is for the overlay module so why is

The .displace styles can be entirely removed. They were overlooked in the rollback of #787940: Generic approach for position:fixed elements like Toolbar, tableHeader

+++ modules/system/system-behavior.css	23 Aug 2010 04:23:18 -0000
@@ -274,6 +293,10 @@ div.password-confirm {
+/**
+ * Helper class to prevent text wrapping. Currently used in the locale and path
+ * modules.
+ */
 .nowrap {

CSSDoc also follows the concept of summary + description. The summary should be one short sentence, ideally not exceeding 80 chars.

Not sure whether the 2nd sentence is required/needed though.

That said, .nowrap seems to be defined in system-behavior.css, but is not related to JS behaviors, AFAIK. Should be moved into system.css.

+++ modules/system/system-menus-rtl.css	23 Aug 2010 04:23:18 -0000
@@ -14,3 +14,6 @@ li.collapsed,
+.block ul {
+  padding: 0 1em 0.25em 0;
+}

Somehow, this looks like Block module CSS to me that should belong into block.css...?

Note that Block module is optional in D7.

+++ modules/system/system-rtl.css	23 Aug 2010 04:23:18 -0000
@@ -5,13 +5,15 @@ th {
+ * Lists generated by theme_item_list()

@@ -22,9 +24,18 @@ thead th {
+ * More link
+ * @see theme_more_link()

(and elsewhere) It would be nice to semi-standardize on a comment for these -- how about:

"Markup generated by theme_FUNCTIONNAME()."

1) We don't have to try to describe what is being output, and just state "markup" (not "lists", "more link", "more help" (no link? :P), etc.)

2) We refer to the native theme function in the summary, skipping the need for a separate @see line.

If you have a better suggestion, use it. Let's not hold off this patch by these comments.

+++ modules/system/system.css	23 Aug 2010 04:23:18 -0000
@@ -35,37 +46,70 @@ tr.odd {
+/**
+ * Table Drag
+ * @see tabledrag.js
+ */
...
+/**
+ * Table Select
+ * Used with tableselect.js which allows all or multiple table rows to be
+ * selected at the same time for batch operations.
+ */

Separate issue:

1) I wonder why tableDrag styles aren't in a separate tabledrag.css that's only loaded together with (the giant) tabledrag.js.

2) The same applies to tableselect styles.

Both should be defined as separate libraries via system_library(), so CSS/JS can be loaded with one call.

Technically, both rather belong into admin.css, but before moving them there, I'd highly prefer to move them into corresponding library CSS files.

+++ modules/system/system.css	23 Aug 2010 04:23:18 -0000
@@ -35,37 +46,70 @@ tr.odd {
+ * Lists generated by theme_item_list()
+ */
 .item-list .icon {

This must be something else. theme_item_list() does not generate icons, at least to my knowledge.

Toolbar module, Seven's admin overview pages, or Forum module could be candidates.

+++ modules/system/system.css	23 Aug 2010 04:23:18 -0000
@@ -182,20 +220,20 @@ a.module-link-configure {
+/**
+ * Compact links
+ * @see theme_system_compact_link()
+ */
 .compact-link {

This definitely belongs into admin.css.

+++ modules/system/system.css	23 Aug 2010 04:23:18 -0000
@@ -247,158 +285,10 @@ ul.secondary a.active {
+/**
+ * Markup free clearing
+ * Details: http://perishablepress.com/press/2009/12/06/new-clearfix-hack
+ */

Minor:

1) There should be blank CSSDoc line between summary and description.

2) We can use @see here.

Powered by Dreditor.

Like in phpDoc, also CSSDoc comments should form proper sentences (and end in a period [full-stop]).

I don't think that would make sense here. Sentences such as "Styles for the Appearance section" were shortened to "Appearance section", and adding a period to that is weird. I think it's okay to make a difference between comments that are written as a title, and comments that are written as one or multiple sentences.

Although I rtbc'd it (a tad too quickly I suppose), I do agree with most other points in #7.

I think it's okay to make a difference between comments that are written as a title, and comments that are written as one or multiple sentences.

Sorry for being asinine, but no. We only want to know, learn, and teach one form for all code comments everywhere, which is to write proper sentences (starting with a capital letter, ending in a period), using proper grammar, and all other (common) details mentioned in the Doxygen formatting conventions. It's true that our CSS coding standards are still incomplete, but it's very clear that CSSDoc and PHPDoc specs do not really differ with regard to comment syntax and style, and that we don't want to make differences here. At least not when it comes to bare essential coding style issues.

In-patch corrections for comments. I've purposively added trailing white-space to comments that require further fixes.

1. Removed obsolete/orphan styles for .tips and .displace.
2. Moved .nowrap into system.css.
3. I've entirely removed the .block ul styles. They are overridden in core themes and shouldn't be contained in system's CSS anyway, I think, as applying a default style for lists in blocks is clearly a job of the theme.
4. Moved .compact-link into admin.css.

@todo in follow-ups:

• Rename admin.css into system.admin.css
• Move tabledrag and tableselect styles from system.css into corresponding dedicated CSS files in /misc.
• Figure out what .item-list .icon is. (Toolbar/Seven/Forum/...?)

1) Re-added .tips to filter.css.

2) .nowrap is technically not limited to a JS behavior, so at the very least, system-behavior.css is the wrong place. Other places don't really match either, so system.css is it.

- Removed .item-list .icon

Hm. Hah! So you want to sneak #777814: Allow modules to provide styles for themes (finally leverage structure/behavior split of CSS files) into D7? :)

Obviously, I'd do whatever is possible to do for D7. :) I can't imagine why this mere resembling of code lines should be bumped to D8.

- As of now, system-behavior.css really contains styles directly related to JavaScript behaviors only... well, almost only. #732914: Improve the markup/CSS for content and user filter forms removes some. And as you mentioned, .element-visible and .container-inline don't belong in there, too, right. However, that means that the purpose and contents of system-behavior.css is pretty clear already and only needs further minor adjustments.

- Renaming admin.css to system.admin.css should happen in a separate issue.

- We can try to extract theme styles into a new system.theme.inc in a separate follow-up issue.

tracking this baby, great work peeps, awesome.

Wow, this is great work folks. Subbing.

I am very glad you are doing this. And for anyone who's thinking "this is way too late!" — actually this is the perfect time to do it. After all the gazillion little patches that changed the css organization without looking at the big picture, taking out the wtfs and cleaning organizing the files.

+1

The password strength checker styles really belong into user.css, not system-behavior.css.

.breadcrumb belongs into system-menus.css.

"Tab navigation" styles all belong into system-menus.css.

This is how it would look like. Pretty much complete, even. Code is solely moved around - no other changes.

The patch at #24 includes some JS changes.
I'm not sure they were meant to be included in this patch?

No .js file changes. It only needs to change drupal_add_js() into drupal_add_library() for the Collapsible fieldsets, TableDrag, Resizable textareas and other core libraries accordingly. Note that this patch applies http://drupal.org/node/777814#comment-3374466 to System module's CSS and JS behaviors.

#24 patch is very good, however there are a couple of things I would question about it.

1) The file naming is quite inconsistent and potentially confusing, there are 3 different formats. Wouldn't it be better to have one format?

system-behavior.css
system-menus.css
system-messages.css

system.admin.css
system.base.css
system.theme.css

maintenance.css

2) system.base.css seems like the wrong name for this - a base css file would normally contain things like a reset - a base set of "styles" for the system or theme. I tend to agree that these could go in system-behavior.css as they appear to be all styles that do something (prevent wrapping, hide something, clear something).

For me system.theme.css is correctly added.

Hrm. :( Why, oh why, can't these initiatives happen during "code polish" phase where they belong? Sigh. I'll have to run this past Dries, as it impacts all D7 themes well after "code freeze" (oh how hilarious is that phrase).

Also, why the heck are there changes to how JS is added to the page if this is indeed merely about CSS clean-up, as the issue title suggests? These look like they should be split.

As the release manager, if I say "Yeah, ok, we'll let your really-nice-to-have feature in 12 months+ late" to one group of people and "Hell no, stay focused on critical bugs" to another, it creates an environment of perceived favouritism which breeds bad blood among contributors. I need to be able to justify bending the rules when I bend them, and I'd really rather not bend them at all since we're 12+ months after feature freeze, 9+ months after polish phase, and have been sitting around 20-30 critical issues for a good 3 months now. We need to get Drupal 7 out the door, which means focusing on fixing bugs, not making further refinements.

Can you speak more about the regressions that have happened since #592018: Re-organize styles across stylesheets from system.module and separate presentational and behavior-supporting styles went in (which was timed appropriately, during polish phase)? That sounds like something I can justify bending the rules about. The other two though... not so much. We can discuss over there, though.

Ok, Jacine and I synced up on IRC.

What she means by regressions is various commits have introduced inconsistencies again where they previously were cleaned up. For example, http://drupalcode.org/viewvc/drupal/drupal/modules/system/system.css?r1=.... IMO a big part of this is there is no documentation anywhere in the coding standards for this "file split" adherence. Methinks http://drupal.org/node/302199 needs some love.

Patch #17 is actually a reasonable clean-up that basically carries forward pre-existing work in HEAD and removes style regressions that have occurred since polish phase. I am actually totally fine committing that patch as clean-up. But then all of a sudden in #24 launches us smack into straight-up D8 territory. I've read http://jacine.net/post/978688556/drupal-css#fixing-css and think this is a great plan, but it is way too late for this kind of whole-sale change in D7, as beneficial to themers as it may be. For starters, it imposes upon module developers a year after API freeze that they need to start splitting up their CSS files (as opposed to this being merely a recommended best practice for what we hope will happen in D8).

Sounds like we need a follow-up to #17 that contains the further CSS refinements in #24 but without all the crazy file splitting stuff that still needs more discussion.

Hm. That's tough, I understand. Although I'm really not sure how many real D7 themes exist at all already, and even if there are more than a few, I'd really like to know whether not all of the authors wouldn't just simply love to do #24, regardless of whether it's late or not. It's unfortunate that we don't have some kind of a simple feedback/voting/veto system for early adopters.

Attached patch no longer splits CSS for each JS behavior in /misc into corresponding CSS files, although that was plain lovely awesomeness.

@Jeff, @Jacine: The point of system.base.css is that the contained styles are independent from any theme, modules have to be able to rely on these, you will never want to override them, and they are also not bound to any JavaScript behavior. Such styles need a home. We could also name that file system.defaults.css or something.

system.base.css makes sense.

Although system-menus.css looks good for this patch, I raise concerns for it as a foundation CSS file in the future. If the intent of the declarations is to style system menus, then the declarations should be limited to a scope that prevents them from cascading into a theme. This sheet mixes layout styling with presentations styling, for instance:

ul.primary li a {
  background-color: #ddd;
  border-color: #bbb;
  border-width: 1px;
  border-style: solid solid none solid;
  height: auto;
  margin-right: 0.5em; /* LTR */
  padding: 0 1em;
  text-decoration: none;
}

It is not unreasonable that a themer would want to alter the colors or border styles of these links and leave the margin/padding intact. By mixing layout and presentation, a themer only has two options: override the styles and introduce cruft or eliminate the file altogether from their theme and reimplement the layout styling. @Jacine's recommendations to separate these types of styles into .module and .theme CSS is the correct evolution of these sheets. Again, not something to address in this issue.

Another one (based on patch #45). Summary of my changes:

common.test, testAlter()
Comment said "add system.base.css" but the code was adding system.theme.css. Changed code so it adds system.base.css.

system.base-rtl.css
Removed .nowrap, .element-hidden, .element-invisible and .clearfix, as the exact same styles are already in system.base.css. (No need to repeat them in the RTL file.)

system.base.css, system.theme.css, system.menus.css
Moved styles for markup from theme_links() from system.base.css to system.menus.css. Also took the padding from system.theme.css and put it in system.menus.css as the other navigation-related presentational styles are currently in there.

system.maintenance.css
Corrected comment syntax for the first two style groups.

system.messages.css
Properly grouped styles for errors, warnings and status messages, and corrected a typo in the CVS Id tag.

I would really like to rename system.menus.css to system.navigation.css as it not only contains menu styles. I didn't in this patch yet, but is there a reason not to do so in a reroll?

Whoops, and another because my editor had put in some weird .project file. (Which is now removed from the patch.)

Only visible with this awesome patch:

Given how many styles for administrative pages like admin/modules and admin/appearance are incorrectly and needlessly loaded on regular pages currently, this is a straight bug fix.

I have:

- triple-checked all moved styles to make sure nothing got lost (most recent CSS changes were not contained).

- reverted all unnecessary additional clean-ups (i.e., order of styles and order of entire sections) to be able properly review this patch.

- fixed very few things, such as RTL styles for collapsible fieldsets still being in system.base-rtl.css instead of system.theme-rtl.css, and such.

This looks RTBC to me, and is a badly needed bug fix and fantastic improvement at the same time. Also attaching an interdiff, so you see my changes.

+++ modules/system/system.menus-rtl.css
@@ -0,0 +1,38 @@
+ul.menu {
+  text-align:right;
+}

Space after colon.

+++ modules/system/system.menus.css
@@ -0,0 +1,119 @@
+  text-align:left; /* LTR */

Space after colon.

+++ modules/system/system.theme.css
@@ -0,0 +1,240 @@
+.item-list .pager li {
+  background-image:none;
+  display:inline;
+  list-style-type:none;
+  padding: 0.5em;
+}
+.pager-current {
+  font-weight:bold;
+}

Space after colons.

Powered by Dreditor.

+++ modules/system/system.theme.css
@@ -0,0 +1,240 @@
+.item-list ul li {
+  margin: 0 0 0.25em 1.5em; /* LTR */
+  padding: 0;
+  list-style: disc;
+}

A nit pick and a personal grievance - in most places we have 0.25em, but in others we have .25em (for example), and in one or two instances I even saw 0em etc, should we clean this up to be consistent?

The personal grievance is with list-style: disc; I hate this because it cascades down the UL tree and applies list-style: disc; at every level - stripping the browsers normal bullet styles for nested lists. If I want them I have to do a lot of work in my theme to re-apply them - kind of annoying.

Powered by Dreditor.

OK, thanks Jacine, I will stop nit picking! RTBC!

What the? How did we go from #32 where we were just going to fix wrongly placed markup regressions to further file splittage? You folks are making me nuts. :\

I hope this patch will not be committed.

I hope this patch will not be committed.
—mertskeli

Enlighten us. This hope you speak of, what is it based on.

You folks are making me nuts. :\
—webchick

Okay, I take it you find the patch too extreme? I had taken this into account when reviewing it myself, and file-structure-wise, this is basically all that happens:

system-behavior.css » system.base.css
system.css » system.theme.css
system-menus.css » system.menus.css
admin.css » system.admin.css
maintenance.css » system.maintenance.css

… along with some style moves across these files so they'll all be in the file they belong.

It isn't actually that extreme and it's way more easier to understand what these files are for, so that when people write new patches for the system module they'll actually put their styles in the right files, so we won't have to do another issue like this in another 6 months. And, it's compatible with the ideas we're having for D8, so we won't have to keep shuffling with these files every time we want to clean something up.

I think there might have been some misunderstandings about what we can or can't do in this patch after #32. If you really think we should change some things before this patch could possibly go in, could you provide us with small list of those exact things so we can look into it?

I do understand the desire to make the current css clean and light. But it is too late. Such changes as renaming/rearranging of core ui files can be considered as the critical ui "api" changes. Further splitting/merging/whateverelsedoing will make things even worse.
Besides, it is not possible to make it more "clean". It is "by design".

If you really think we should change some things before this patch could possibly go in, could you provide us with small list of those exact things so we can look into it?

Here is one example.
Go to Management > People.
There is a set of filter selects followed by the user list.
Who decided to markup all the selects (which are besides wrapped as normal form item divs) as DL list items? What to do with the default html dl identation? You can not apply margin:0/padding:0 to it because it will break a lot of other DLs including the starting admin page? No problem, we will wrap this with another div with multiselect class, and will make a css dl.multiselect margin/padding 0. Very nice.
How can it be "cleaned-up" by css?

In fact there is a major need of html clean-up. It has deviated too far from the normal html default behavior.
But it is out of the this issue's scope. And out of D7 scope, for sure.

[removed double post, drupal.org was having issues.]

[removed double post, drupal.org was having issues.]

It's not about cleaning up Drupal like in the example mertskeli gave. It's about making CSS easier to maintain, making it easier to contribute to Drupal CSS without unintentionally messing everything up or having to dig through a couple hundred lines of code for hours just to be able to tell where to add your own. To make writing CSS for Drupal less painful, bit by bit.

If we don't take the first big step now, before D7 gets out, themers will have to deal with this CSS trash can called the system module (no offense) for another two years. Like Jacine pointed out in the first message of this thread, this is unacceptable for many people. Because it's not only about writing code for core (I guess it could wait for D8 if that was the case); it also affects themers who use Drupal in their commercial projects.

That's why we came with this patch, which we essentially did some months earlier when it wasn't too late but it got messed up again (which is why we came up with better file names in this one, the current ones are clearly failing). I hope that somehow we can make an exception to the "it's too late" rule.

mertskeli's example applies after we get this thing in, because once these things are properly organized, it'll be a heck of a lot easier for people to start looking at the bigger picture regarding Drupal markup and CSS, and start fixing things like the one he pointed out.

And again, if there are concrete things that we can do so the patch can go in, then please, share them with us so we can get this to happen.

Sorry, I'm sure I understand this issue pretty well.
And I do not understand how renaming and resorting core files can help to solve the problem. You suppose "system.base" is a better name than "system-behavior"? I do not. Imo both of them say nothing about their contents. But the case is that I have been living with system-behavior for a year already, and now I am in danger of being forced to rewrite everything again. Where is the famous "code freeze"?

Easier to maintain? What are we talking about? About 100 core css files? What about extra 100 css files in contribs? Who on earth is able to maintain them?
The only way to "maintain" them is to kill all of them and to write 3-4 new from scratch with simultaneous cleaning of the core divs and classes. That would be realy easier to maintain.
Core should output raw default html. If it does not look good it means the problem is in markup, and it should be solved before touching any css.
This patch does not solve this problem. And will not.

Right now I suggest not to rename anything. And not to move elements between files. But I'll accept any community decision, and will accomodate my code. Just I hope this patch will not be committed.

why am I not subscribed to this?

Easier to maintain? What are we talking about? About 100 core css files? What about extra 100 css files in contribs? Who on earth is able to maintain them?

This concerns the system module, the module containing some of the biggest, dirtiest CSS that one can currently find in core. Because a lot of patches for core affect the system module, the CSS in this module tends to get messier and messier, unless we do something to prevent that.

But like I said, that's not even the biggest concern right now; that could wait for D8. The biggest concern is about projects that use Drupal, which will be affected by this CSS until another major release comes around. (Two years.) People are making base themes to work around this, because it's a hell of a job to get the CSS clean for every project separately. It really shouldn't be necessary in the first place.

The only way to "maintain" them is to kill all of them and to write 3-4 new from scratch with simultaneous cleaning of the core divs and classes. That would be realy easier to maintain.

I agree with you on that. But I don't think something big like that is going to happen soon. So we're approaching it in smaller, separate steps.

mverbaar, I did not mean to neglect the importance of the syntax clean-up. It can be patched and be useful for D7. But I suggest not to rename/split/move anything until D8.

I think Jen's comment from earlier is a perfect explanation on why we should do this right now, btw:

I am very glad you are doing this. And for anyone who's thinking "this is way too late!" — actually this is the perfect time to do it. After all the gazillion little patches that changed the css organization without looking at the big picture, taking out the wtfs and cleaning organizing the files.

mertskeli,
It's the naming and moving (organizing) that makes this so important, not so much the syntax, whitespace etc.

That's what I'm afraid of :)

People are making base themes to work around this, because it's a hell of a job to get the CSS clean for every project separately....

HELL YES. I have a 900+ line reset to deal with Drupals crappy CSS. I have theme functions, template overrides and lord knows what else just to bash Drupal sensible so I can use it everyday to build clients themes.

mertskeli - this has nothing to do with code freeze - its a strait out bug that needs to be fixed. Please look at this is as a baby first step (despite looking like a big change) in the long road to improving core CSS (all help appreciated). You yourself are saying things need to be fixed - but here you are opposing that process? Simply opposing something because it doesn't do *everything* that you want it to is not how things get done (= scope creep) - we need to work incrementally - patch by patch. Its likely much of the hard-core progress will be made in D8, but this is at least a start and a big win for D7 themers. And BTW - if you have a problem with markup please post an issue - thats not what this issue is about.

@69 - there will be follow up issues and so on - you are advocating scope creep (I am very guilty of doing this myself...), please focus on the issue here.

@webchick: Your reaction is totally understandable. Let me try to explain why this is a major bug fix from our perspective:

Problem

• System CSS files contain admin-only styles, which are needlessly loaded for all regular pages.
• Styles, which are absolutely required for Drupal sites to work as intended, are not cleanly separated from visual theme styles.
• People do not understand what styles to put into which file.
• Missing or entirely outdated documentation in System CSS files.

Goals

• Fix CSS files so that we don't load administrative styles for regular site users and visitors.
• Make it easier for themers to just override the visual style, so as to not break the structural base styles that are required for Drupal behaviors.
• Prevent developers and novice contributors from doing the same mistakes again.
• Fix the documentation, so everyone finally understands which particular markup every style is targeting.

Details

• Currently, we are loading lots of totally needless styles on all pages for every single visitor of a site. Those styles are targeting specific administrative pages only (such as the Modules or Appearance pages). They should not have been added to any of the non-administrative stylesheet files in the first place. But given the current chaos and disorder in System's CSS files, it's understandable that contributors may simply resign with "WTF?" and just put their stuff somewhere, anywhere.
• The fact that those styles were able to get in means that Drupal core maintainers do not understand it either. Again, no pun intended! This is perfectly understandable, as I've lost orientation for the existing files, too.
• Base styles that are absolutely required to make JavaScript and other intended behaviors properly work are not cleanly separated from visual theme styles. It was the original intention of the system-behaviors.css file to only contain structural, required styles. The file is not very old yet. But in the meantime, it contains many style definitions that simply do not belong in there.
• Since no one seems to understand the meaning of the current files (neither by the file names, nor by the file contents), we have lots of additional maintenance work to fix and clean up wrongly placed styles. I.e., required styles for Drupal system behaviors were not added in system-behaviors.css, but in system.css. And vice versa; simply anywhere.

  Those bug fixes and clean-ups need to happen either way, but they have been incorporated into this patch already:

  • Remove the ambiguity by re-thinking and renaming the file names to reflect their purpose.
  • Check what others are doing, ideally something that both developers and themers may already be familiar with. And find jQuery, which cleanly and purposively splits into $plugin.base.css and $plugin.theme.css files. How much more simple can it get?
  • system-behaviors.css becomes system.base.css: As the name implies, here are the absolute base styles, required to make Drupal work. Without these styles, lots of markup and JavaScript behaviors entirely break. You see elements you should not see, unusable autocomplete, invisible tabledrag, unreadable sticky table headers, etc... However, when only loading this file, then you can properly use Drupal and get the intended visual output and behavior. But without any specific styling - it only works and works only. (Similar to the Stark theme)

    If you recall Drupal 5 times, Drupal shipped with a defaults.css, and apparently, it was immediately clear to everyone that you likely do not want to override this file or the contained styles. Actually, however, for the styles that were contained in defaults.css back then, that was not really true, which is probably the reason the file got nixed and merged into system.css at some point. With system.base.css, we are restoring the real, original, and badly required intention of defaults.css back from the old days: Absolutely required styles to make Drupal work. Stuff like .element-hidden, .element-invisible, .js-hide, etc. - all of that is required to work as expected in D7. Modules have to be able to rely on that functionality.

  • system.css becomes system.theme.css: As the name implies, it contains basic visual styles that make Drupal core's output look slightly fancier and should work for most themes. But in case you are writing your own custom theme, it's very likely that you simply want to override this entire file. And you easily can, and you even can leave it out entirely, without destroying any essential Drupal functionality.
  • admin.css becomes system.admin.css: This is basically the same file as the existing file, but it now contains a bit more styles that were improperly added to one of the other files. It is possible that people overlook this file, because it is not listed with the other files or something. Furthermore, this fixes the module namespace violation; the Admin module is currently not able to use a CSS filename it should be able to use.
• We actually discussed and think that the two additional files system.menus.css and system.messages.css are no longer needed with this patch, and pure overhead. They were introduced in #163785: Themes can't override core & module stylesheets and resp. #193482: Styling status messages in system.css with the following note:
  

  moves the menu-related styles in system.css, our worst offenders for high-specificity hard-to-override rules, into system-menus.css and system-menus-rtl.css

  Not seeing the big picture, those changes were probably right at that time. However, being themers for a long time who went through the entire learning curve like everyone else, we don't understand why you'd ever want to override messages or menus styles separately. Either you do your custom theme, or you do not. I.e., either you override the entire sytem.theme.css file anyway, or you just expand on it. But you'd never override those two parts only, that's just silly.

  Merging system.menus.css and system.messages.css into system.theme.css would

  • further aid in front-end performance: Two files less means two less HTTP requests.
  • aid in avoiding the IE 31 files limit: Two files less means there is room for two other files. (Actually four files less on RTL sites)
  • further aid in decreasing confusion, as that would leave us with simplicity: system.base.css, system.admin.css, system.theme.css.

  Although we'd really like to, we didn't merge the two files into system.theme.css, because we feared that it would get even more hard to get this patch in with that additional change. But if possible, then we'd merge those two needless extra files into system.theme.css.

• Lastly, we finally fixed the documentation for all the styles defined in those files. This even revealed some styles that we're needlessly carrying around since Drupal 4.7, for markup that no longer exists in Drupal core. Apparently, in order to properly review this patch, I reverted most of the additional clean-ups that Jacine did and listed in #2. We'll make sure to re-do those in a follow-up issue.

@mertskeli: Based on your replies, I doesn't sound like you applied this patch and looked at the actual results. It rather seems you want to blatantly hi-jack this issue by spreading FUD, which of course makes it much harder for us to convince webchick of this patch. We are totally happy to discuss actual and relevant issues you may have with this patch. But please refrain from posting totally unrelated problems you have with Drupal's CSS to this issue. It's not in the scope and also not the topic. Thanks!

Thanks for the detailed summary, sun. I'll need to consider it and confer with Dries.

Reading stuff like this from Jeff Burnz:

"Please look at this is as a baby first step ... in the long road to improving core CSS"

...is what is extremely alarming about this patch. We do not want to launch on a full-scale re-organization of core CSS at this point. Remember. Drupal 7 has been in development since February 2008, and included an 18 month code thaw + 3 month "polish" phase for patches exactly like this. We need to get Drupal 7 out the door, and the issues you're talking about here have plagued Drupal since forever.

So, if this patch goes in, when does this end? Do we stop here with system module, which is the most egregious offender? Or are we launching into 6+ weeks of totally unstable markup/CSS and get to tell themers "You might as well wait until 7.0 to start porting your themes?" The the theme and markup related bug queue is a non-trivial size, and adding more to it doesn't seem like a great idea at this point.

I'm not sure what what everyone else is thinking here but I assume apart from small issues such as #51 & #53 this is it, the rest is D8 material? I'm not sure if we really want to go further than this, it could create a deluge of extra work in our core themes (needs checking of course) when we're already struggling to fix the existing issues (right now, in Bartik queue I have a quite celebration if I get a patch review...). Personally I would be more than happy to compromise on this one patch and a cleanup for minor formatting issue.

Yes, after this patch landed, there will only be one or two minor follow-ups. I.e., to be precise, the removal of totally obsolete styles that Jacine happened to find and listed in #2, and for the sake of completeness, #732914: Improve the markup/CSS for content and user filter forms also touches System's CSS, and we'll happily re-roll that patch after this one landed.

@sun, I have neither fear nor doubts. And I'm not spreading anything. I wrote enough to explain why such late changes (and which of them) are undesirable.
And I have no problems with Drupal CSS except those which everybody else has. These problems can not be fixed by simple css juggling.
The issue title is "CSS Files are in major need of clean up". So all renaming/splitting/merging (as well as patching .inc files) are out of scope here.
I support most of the things of this issue but I want D7 to be released.

I don't see what could go wrong with this change. On the plus side, it's a beautiful example of how CSS should be structured in any module or theme, period. Another +1 from me.

Subscribe and +1. I haven't read the patch or comments here thoroughly, so can't speak to the risks (if any) of this landing, but at first glance, seems like a *great* theme DX improvement, which I think is a very important part of Drupal's evolution from D6 to D7: D6 convinced a lot of back-end developers that Drupal is the platform to jump on to. Hopefully, D7 will do the same for UI designers and front-end developers, so fixing major legacy CSS WTFs seems like a very smart move for the Drupal project. And loading less bloat for non-admin users improves front-end performance, and improving performance is one of the exceptions to code freeze, so I think this is within scope of what is still appropriate for D7, unless there's justified concern that it will unstabilize things too much, but from a skim of the issue comments, I'm seeing frustration that D7 is taking so long to release, but not a solid argument that this patch will cause it to take longer.

That said, I'm somewhat concerned that we have 100+ "major" issues in the queue in addition to the 22 "critical" ones. It might be good to scrub those to see if they're all really "major". At some point, we do need to stop perfecting, and get serious about releasing.

@Jacine: I'm very sorry if my comment offended you. I am EXTREMELY happy with and in awe of all the ways in which you've made D7TX soooooooo much better than it would have otherwise been without you. I completely agree that this issue deserves to be committed, so perhaps it wasn't the correct place in which to express my concern at the long "major" queue. You doing such a fantastic job at maintaining the markup issues puts you under no obligation to help on Bartik or any other issue that's not your itch to scratch. But I do know that Dries and webchick are growing weary of all the issues they're still reviewing, many, many months after all the freezes were supposed to take effect. I think the best thing we can do to help is become more disciplined ourselves about what constitutes "major" and be more selective about which "normal" issues we RTBC rather than bump to D8. It's possible you've already been appropriately disciplined and selective. I'm not sure that I have, so my comment was partially me venting my own guilt, thinking it might resonate with others. I think it only ended up in this issue, because that's what I happened to be reading, and the other comments expressing frustration at D7 taking so long to release triggered those feelings, even though I really like what's being done in this patch.

Re-rolled against HEAD by manually merging in recent changes into the new files. Will likely fail to apply for user.css.

Alright, git == quite a pain. Once again via CVS.

Removed stale .multiselect styles (replaced by the new exposed filters styles which broke this patch)

Would be nice to get this in before it breaks once again - quite a challenge to properly re-roll.

Subscribe and +1 - yes, please and thank you sun/Jacine.

Committed to CVS HEAD. Thanks.

im redirecting from @Jacine blog post.

I always have a question (I was thought it as code standard).. WHY some of drupal css rule have TAG before #id/.class, eg div.abc, ul#id
most of them are no reason to add it.... especially DIV.somthing

Although both having ok class, div.ok and tr.ok are two different items, and can be styled differently. So besides the common ok class, styling can be even more narrowed. I guess.