Drupal 6 gives designers the ability to create "CSS-only" themes using just a .info file and a collection of .css files. It's a HUGE boost for Drupal 6's friendliness to the design community. However, there's currently no way for an .info file to specify that core css files (like system.css and defaults.css, two that must often be mapped out for heavy-duty skinning work) be ignored.

This relatively tiny patch recognizes 'remove-stylesheets' as a new key in our theme .info files. What does this mean?

stylesheets[all][] = style.css
remove-stylesheets[all][] = modules/system/system.css

The two lines above will include the theme's own style.css file, and remove the system.css file from the list of stylesheets sent to the user's browser or included in the aggregate stylesheet when that feature is turned on.

It may be too late in the cycle for this patch, but if we want people to be able to create css-only themes, and I think we do given the boost it would give to designers, this is a must-fix problem.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

subscribe...

Crell’s picture

I'm all for making life easier for themers so that they stop asking me how to do things. :-) However, IMO having to specify the full actual path to the module now in a template.php when editing $css is a bug, not a feature. If a theme wants to remove user.css, it shouldn't care where user.module is. While core modules typically don't move (unless you're overriding them, which occasionally you have to do), many popular contrib modules provide CSS code that you may want to get rid of, too. There's four places those could live.

If we're going to add a way to remove them, let's do it in a path-agnostic way. Unfortunately I think that may be a somewhat bigger change.

Hm, random thought just now:

remove-stylesheets[all][system][] = system.css

(Read: remove from @media all the system.css stylesheet that resides in drupal_get_path('module', 'system');)

We can already override parent theme stylesheets, so we only need to handle modules here.

eaton’s picture

However, IMO having to specify the full actual path to the module now in a template.php when editing $css is a bug, not a feature.

Apply the patch and try it. That works. ;)

Technically, removing 'css' works to eliminate all external CSS files entirely, but that's probably undesirable. ;)

It's doing string matching at the moment... Though I think your approach might work well, too. Not sure which is more efficient...

Dries’s picture

Ah, just like I predicted in the Info File Issue From Hell; we're going to start using the .info file for more than just simple properties. ;-)

Personally, I'd much rather turn it around: make it mandatory to include all CSS files, and make it possible to overwrite that list rather than extend it. It seems simpler than combining add/remove logic. It's more explicit and transparent at least.

Crell’s picture

Ah, just like I predicted in the Info File Issue From Hell; we're going to start using the .info file for more than just simple properties. ;-)

That's what happens when you give Drupal developers anything that even resembles an associative array. :-)

I'm unclear what you mean though. Are you suggesting that instead of "remove user.css", we say "here's the alternate user.css to use instead?" I actually proposed an automated, simple version of that back in Drupal 5, but was superseded by the (long-term much better) drupal_add_css() changes that begot the CSS aggregator. I would be fine with override-instead-of-remove, provided that we had a good syntax for it. I fear we'd have to change the structure of the $css array, though, in order to do that.

Ideally, we'd have modules able to define their resource files (CSS and JS) abstractly rather than by passing an absolute path (another hook?), and then the theme could override that abstract definition, still without dealing with absolute paths. If we assume all modules will put their CSS file in the same directory as the .module (and I don't think that's an onerous requirement), then all we need is the module name as a key. I think that's definitely getting into Drupal 7 territory, though.

eaton’s picture

Personally, I'd much rather turn it around: make it mandatory to include all CSS files, and make it possible to overwrite that list rather than extend it. It seems simpler than combining add/remove logic. It's more explicit and transparent at least.

I'm not sure that really gets us anything in terms of explicitness, other than the assumption that people MUST have a matching .css file in their theme for every one a module defines. the 'remove' syntax allows designers to maintain a consolidated style.css file, for example, while removing css files that would conflict with it (like the notorious system.css).

The only reason to take that route ('overrides' versus 'removes') would be to force designers to completely separate their system.css override styles from their main style.css. Designers, for the most part, don't necessarily approach css styling that way -- it's something that comes from our modular PHP architecture, and those kinds of splits tends to make CSS work a lot *harder* when you're working on a final theme.

I think that's definitely getting into Drupal 7 territory, though.

Exactly. Right now, any more complex solutions will require large scale changes to the CSS handling mechanism and requirements on modules. The benefit of this approach is that it breaks nothing, changes nothing, save allowing one new key in the .info file to block out the standard .css files.

OK. I'll stop lobbying. ;) I hope that we can fix this problem in SOME way before D6 ships. But if this is a bad solution, I'm willing to scrap it.

Dries’s picture

I'm not feeling like rushing this patch just to get it into D6. So, let's keep discussing this -- we might come up with some ideas for alternative ideas.

eaton’s picture

I'm not feeling like rushing this patch just to get it into D6.

Fair enough. :-) A couple points to keep in mind, though:

1) we would still need a custom 'override-stylesheets' line item in the .info files to avoid mismatched naming conventions (stylesheets[all] = foo vs. stylesheets[all][modulename] = foo). That means any simplicity benefit from "overlaying" versus "removing" is negligible.
2) If we don't allow CSS theme designers some way of saying, "Don't include all those goofy default Drupal rules," they will be forced to implement inordinately complex style rules to hide some of the very, very specific CSS selectors implemented in system.css and defaults.css.

If we come up with some sort of approach that feels cleaner, would it be considered for inclusion in 6?

eaton’s picture

Status: Needs review » Needs work

Spent some time chatting with Steven about this, and he convinced me that this solution is probably the wrong way to go. Many of our core CSS files include the rules needed to implement critical usability features like collapsing fieldsets. Getting rid of those makes as much work for themers as requiring detailed overrides.

The biggest problem lies in a handful of very high-specificity rules we keep in our core css files. Background colors, borders, and assorted list styles used in both theme_table() and our menu/navigation links are often deeply nested and very specific, making it really difficult to override.

Quoth UnConeD: "Identifying high-specificity rules, especially those with ids, and seeing whether they can be made less specific and easier to override, is a useful thing to do."

After consideration I agree; that would be a much better use of time and far less controversial than adding tweaky bits to the info files.

eaton’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs work » Postponed
dvessel’s picture

It'd be nice to have it work like templates in 6. By moving .tpl.php files from module folders into the theme forces it to read the themes version. This would make it very straight forward for themers to work with styles that are already there.

It's not 100% fool proof (would break if more than one style sheet shared the same name) but I did this for Drupal 5 with a custom function sitting inside the theme.

http://drupal.org/node/155400

eaton’s picture

Title: CSS-only themes can't remove core/module stylesheets » Themes can't override core/module stylesheets
Version: 7.x-dev » 6.0-beta1
Status: Postponed » Needs review
FileSize
8.32 KB

After talking to a couple of people at the conference, and re-reading Dries' suggestion, I thought about this and decided it could be done. The attached patch changes no APIs and adds no new keys to .info files. Instead, it:

1) Checks to see if a css file added by a theme shares a file *name* with an already-added css file.
2) If it does, it swaps the NEW path for the OLD path, preserving the 'preprocess' flag of the old addition
3) 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.

What does this mean? If I add the following line to my info file:

stylesheets[all] = system-menus.css

drupal_add_css() will look for any other added paths that end with the filename 'system-menus.css', and replace them with the path pointing to my theme's version of that file. It still works with RTL language support, and it respects the preprocess flag settings that the original module used.

What do we gain?

First, as Dries requested earlier, it makes it easy for a theme designer to do with css what we now do with template files: copy a css file from a module or core, add it to the theme, and hack it to taste. No funky _variables overrides in template.php necessary. Second, it splits our high-specificity menu-related CSS rules out and keeps them in a separate stylesheet. That means a person designing a theme can easily nuke the default menu rules to implement their crazy, funky CSS rollover menu tricks or what not. This has been a persistent complaint from designers working on Drupal themes.

I think it's a big win, and it affects NO current themes, NO APIs, and none of the actual CSS that we output by default. It also avoids cluttering the info file with new keys.

sun’s picture

subscribing

Great approach Eaton!

eaton’s picture

Version: 6.0-beta1 » 6.x-dev

A note regarding performance: The styles array that we maintain via drupal_add_css() can become dozens of entries long on sites with lots of module-added styles and theme overrides. The approach this patch takes adds the expense of an looping over that array, and splitting a handful of strings, each time the function is called.

Given that 1) drupal_add_css() is nowhere near a performance bottleneck, and 2) the things it's doing are not actually expensive, I'm not very worried about that aspect of it. If there are serious concerns I can try to set up some benchmarking, though.

jjeff’s picture

Oooh! Separate CSS file for menus. Very nice!

mfer’s picture

While I think designers would prefer the remove-stylesheet method, I think this is a good start.

If this is the path for 6 lets file an issue to add remove-stylesheet to the .info files for 7.

dvessel’s picture

The only potential problem with this is if there are more than one style sheets with the same name. You never know with modules and it may end up swapping out the wrong style.

dvessel’s picture

Oh, and I had a similar approach and ended up comparing the parent module folder to see if it matched. If it did, it would use the original name. If not, it would factor in the parent folder.

example:
cck/content.css would be "cck-content.css"
node/node.css would be "node.css"

of for styles running in sub-directories:
foo/bar/styles.css would be "foo-bar-styles.css"

Not straight forward but the name check can factor for your patch eaton by simply adding in the paths.

stylesheets[all][modules/system/system-menus.css] = system-menus.css

sun’s picture

Actually, if we require to pass in the original core/module stylesheet path and filename, a themer was able to name the replacement stylesheet like she wants. Example:

stylesheets[all][modules/system/system-menus.css] = my-menus.css

I like that even more, because a themer could simply copy'n'paste a filepath of a core/module stylesheet from the HTML header.

eaton’s picture

I like that even more, because a themer could simply copy'n'paste a filepath of a core/module stylesheet from the HTML header.

I'm against that because it requires us to implement very different parsing for the special case. It would require us to change not only the API interface for drupal_add_css(), but also the .info file parsing code. That means more complexity, more documentation, and more bugs late in the cycle.

I think in Drupal 7 we can definitely look at revisiting how we add and manage CSS and JS assets. A simple solution that reuires *no* API changes, and makes the behavior simple and easy to explain, is currently the best IMO.

eaton’s picture

Also, because modules can be stored in modules/foo, profiles/foo/modules/bar, sites/all/modules/foo, sites/mysite.com/modules/foo, and so on, there's no good way to do full-path matching in the .info file. Matching on the very last element of the path (ie, the css filename itself) is the only reliable approach without refactoring the entire drupal_add_css() interface. I worked on a version of the patch that attempted to allow such syntax, and it gets very ugly very quickly.

I want to reiterate that this is an attempt to solve a bug in the current beta of Drupal 6, not an attempt to refactor CSS handling. Upthread in this issue, Dries said that he preferred an approach that allowed themers to simply supply a 'replacement file' for core CSS files. This patch allows that ith minimal impact.

I'm completely in favor of reopening the discussion for 7, and figuring out a more flexible approach. I believe, though, that this is the right path for 6.

Crell’s picture

A little known fact of the theme system is that it will not add a file that does not exist. That results in the side-effect for theme-based CSS files that you can override a parent theme's CSS without replacing it by specifying

stylesheets[all][] = parent.css

in the info file without actually providing a new version of it. That effectively removes it. I suspect the same quirk would apply here and allow themes to flat out remove a module's CSS file the same way. Neato. :-)

Will try to test this at some point soon when I am able.

Senpai’s picture

Title: Themes can't override core/module stylesheets » Themes can't override core & module stylesheets

[Removing the slash in the title to make it look less like an internal drupal path.]

How hard would it be to let each module have a checkbox in the Theme API so that by visiting that module's admin page, you could turn off the built-in styles for said module.

Better yet, why not have a single consolidated location for all modules to register their stylesheet names. That'd make it super easy to overview what's being aggregated by the css output engine, spot CSS file naming conflicts, and control what gets sent to the user's browser without having to override even more stuff.

eaton’s picture

How hard would it be to let each module have a checkbox in the Theme API so that by visiting that module's admin page, you could turn off the built-in styles for said module.

That would require style sheets to be registered using a central registry not unlike the menu or theme template system. It's definitely outside the scope of a D6 post-beta bug fix patch. :)

Better yet, why not have a single consolidated location for all modules to register their stylesheet names. That'd make it super easy to overview what's being aggregated by the css output engine, spot CSS file naming conflicts, and control what gets sent to the user's browser without having to override even more stuff.

That's what drupal_add_css() is, to some extent, though it has limitations. Expanding it to include these kinds of capabilities requires collecting more information, which requires changing its API and breaking all modules that use it -- definitely out of the question in D6.

That's why the approach above was taken. It solves the problem, in the way that Dries requested, without breaking any APIs or requiring new and funky hooks in .info files. :-)

eaton’s picture

Also, I have promised to compose an original limerick for each person posts a patch review with specific criticisms or positive test results. ;)

quicksketch’s picture

Some initial code style thoughts:
- Use the basename() function rather than array_pop(explode($path)) kind of method.
- Make sure our variable words are properly separated by underscores ($oldfilename => $old_filename).
- The adding of the new css file location just has a bit of a confusing line in it:

$css[$media][$oldtype][$path] = $oldpreprocess;

Could we simply change it to:

$css[$media]['theme'][$path] = $preprocess;

This would mean that over-ridden css files would be added in the 'theme' category, rather than 'module'. If the CSS file is added by the theme, it should probably be in that array, no? That and changing the variable name just to $preprocess make the line less confusing to me in my initial read-through.

All in all, I really like the concept of this patch. The menu styles are definitely a trouble area for anyone wanting to make their menu items structured in a different way that Drupal's default tab-like output. It would also make over-ridding CSS identical to over-ridding .tpl.php files! From the themer's standpoint, this is a very logical extension the way themes work in D6. Copy the file you want to over-ride to theme, then change as necessary. Brilliant!

We'll need to add an item to the upgrade path notes saying that all css files added by modules need to be prefixed with the module name. Although it seems contrib developers have generally figured this out now, when .css files first started showing up in modules it was a common practice to just name them style.css. Of course if you had 5 modules installed that all used style.css that didn't show up because the theme has style.css, you'd have one messed up site. All of the core modules set a nice precedent already for how css files should be named for individual module css.

eaton’s picture

FileSize
8.08 KB

Thanks for the review! I've rolled a new version of the patch that includes three changes:

  1. Uses the much cleaner basepath() function. Thanks! I hadn't realized that EXISTED, and it's much nicer.
  2. Uses clearer underscores rather than jammed together words in the variable names.
  3. Place the overriding css file in the themes section, not the modules section

The other issue -- that we should save use 'preprocess' rather than 'preprocess_old' -- is tricky. If CSS caching is turned on, and a module adds a non-preprocessed CSS file, we want to be sure that it's spotted. Although... Actually, that just occurred to me -- if a module only occasionally adds its CSS file, the theme will *always* add the CSS file. There's no way for a theme to say, 'only use my file IF the old one is there.' That would, in fact, require changing the format of .info files. Is that worth it?

Also:

Nate Haug is a patching tornado
whose code prowess should lead to bravado
His script's fit for kings
and his theming, it sings!
If you're looking he's in Colorado

quicksketch’s picture

Ah, I hadn't realized the $preprocess variable namespace had already been used in this function. $old_preprocess is totally acceptable then in this situation.

Couple more things:

unset($css[$media][$oldtype][$rtl_old_path]);

needs to be expanded to:

unset($css[$media][$old_type][$rtl_old_path]);

I had thought that copying a file to the theme's directory would be the only step necessary. I didn't realize that the themer still needs to add the following line to their template.php:

drupal_add_css(drupal_get_path('theme', 'garland') .'/system-menus.css', 'theme');

This is a point that is confusing for me. This function's last argument is $preprocess. But it looks from the code that this will be completely ignored, since the original module css file's $preprocess flag will be used instead.

I feel like the recognizing of new css files should be automatic, as they are with .tpl.php files. This would remove the need to drupal_add_css() and include a template.php file in a pure css/markup theme. What do you think?

quicksketch’s picture

Oh dear I got so caught up in my review that I didn't mention how much I enjoyed the limerick! It's very nice. I'd like to mention I'm single for any takers :)

eaton’s picture

FileSize
8.08 KB

Doh. Thanks for catching that, I didn't spot it hiding there in the RTL code. I've attached a fixed version.

I had thought that copying a file to the theme's directory would be the only step necessary. I didn't realize that the themer still needs to add the following line to their template.php:

Nope, this isn't necessary actually. They just need to add a line to the theme's .info file:

stylesheets[all] = 'my-override.css'

it's the same syntax used for adding any other stylesheets in an .info file, and doesn't require the presence of a template.php file. I don't think there's any good way to manage auto-detection of matching CSS files without that, though. We can only do it with tpl.php files because we scan directories recursively and precache all the files we find. Doing that, I think, would be a pretty major refactoring and I'm not sure that I'm up to it in the D6 timeframe...

eaton’s picture

Also, ladies, Quicksketch is an artist who is currently backpacking through Europe.

merlinofchaos’s picture

Doing it automatically would be:

1) cool
2) a new feature and therefore
3) not acceptable post freeze.

dvessel’s picture

FileSize
9.12 KB

Here's an alternate approach. It includes the menu style changes from Eaton.

All you have to do is set an override directory then drag & drop your style sheets.

Inside your .info file:

style override path = styles

"styles" can be anything you want it to be. Once that's set, you can drag any module styles and it'll detect it. It will also override base theme styles the same way.

There's more to this and I'll explain a bit later.

dvessel’s picture

Merlin, yeah it's a new feature but I think this would be easier to work with so I put it up as a possible alternative.

If the override isn't set, then it affects nothing.

There's a cascade for the paths it check. On potential problem is duplicate style names. This patch over comes that by letting you drop module styles with folder names associated with the original style.

The following assumes "styles" is the override path set in the .info file.

For example, a module of "sites/all/modules/foo/bar.css" will check these directories:
- styles/modules/foo/bar.css
- styles/foo/bar.css
- styles/bar.css

The same applies to base themes. So lets say it's located in "sites/all/themes/my_theme/colors.css"..
- styles/themes/my_theme/colors.css
- styles/my_theme/colors.css
- styles/colors.css

So, duplicate styles can still collide when placed directly in the override folder but you can add more paths to get around that.

And the point about style sheets behaving like how template files do now, I don't agree with completely. Template detection is handled automatically. Explicitly setting each override can be confusing and the duel purpose of the "stylesheets" key to "set" and "override" I think makes it a little worse. Trying to explain it in the theming handbooks was a bit of a mind twister. You have to keep track of how everything cascades with multiple sub-themes with this toggle feature of "stylesheets".

But It's late, I know..

If this is too much then I'm perfectly fine with going with Eaton's patch.

dvessel’s picture

@Eaton, setting the override for the module drops it down as the last style sheet.

Crell’s picture

Status: Needs review » Needs work

re #30:

Eaton, what's wrong with this picture? :-)

if ($type = 'theme') {

Once that was fixed, everything seemed to work correctly. I am a bit concerned about all the looping happening in drupal_add_css(), but I'm not certain where else we could put it that wouldn't have the same efficiency.

I do prefer eaton's approach to dvessel's, I think. I prefer specifying files explicitly to setting up override subdirectories. (And training my themers to use subdirectories differently than they do now would be a PITA. :-) ).

Also, eaton, please note that the syntax you're quoting is wrong. You need to use the [] push syntax, eg:

stylesheets[all][] = node.css

I'm tempted to say that the menu-style-split should be a separate patch. Not because I dislike it, but because it's important enough that it *really* needs to go in, even if the extra info file handling doesn't. It could still be overridden by the current PHP-based method if the rest doesn't make it in.

I need sleep now.

Senpai’s picture

Patch tested, and working as promised. One small problem I noticed was that once I duped Garland into /sites/all/themes, renamed it garlandMods, renamed the .info file as well, and then created a "style override path = styleOverrides" line inside the .info. Created the /sites/all/themes/garlandMods/styleOverrides/overrides.css path as well.

Once you switch themes to, say, Marvin, and then back to garlandMods, the color.module stops workin. Switch the color wheel to Shiny Tomato, and the only color that changes is the background of the site logo.png

That's the only drawback I could find. I love the ease of theming this patch offers, and I hope it makes it into the final release. All of us who work on Drupal for small-time clientele really need some easy functionality like this. +++ for dvessel's patch concept.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
8.08 KB

Ah, I see. I haven't looked enough into adding CSS files in D6 to know that you could register them in the .info file. I agree then that setting them in the .info file is a much better approach, I just didn't want someone to have to do what I posted in their template.php file. As for subdirectories, I'm not sure they'll be more useful than confusing. Like I mentioned above, we just need to put a note in the module upgrade path that css files should be prefixed with their module name. We can include it as part of coder if really necessary.

I agree with Crell and think the Eaton approach to adding/over-ridding CSS files is best. It doesn't change any of our existing syntax and is really easy to understand.

Here's a patch with the equality operator fixed as noted by Crell in #36.

dvessel’s picture

Crell, quicksketch, It really isn't confusing. I just put up a confusing example. Dropping the styles into the base level of the override folder is all you have to do. So, how is that confusing? What's there to train?

The extra sub-directories are optional and used for higher specificity based on the path of the originating style and it's a lot more inline with how CSS works in general. I only put it there for situations where complex overrides are being done and to prevent collisions.

But come on.. it being too much of a change is one thing but it's not more complicated.

eaton’s picture

But come on.. it being too much of a change is one thing but it's not more complicated.

If there's one thing that I've learned from working on Drupal, it's that any time the discussion turns to people discussing subjective opinions about how complicated one aproach is versus another -- for a completely DIFFERENT set of people -- there's not much more to be gained. ;)

The benefit of the first approach (using the info file) is that it is very, very simple to use and to explain. The mechanism by which it overrides styles is straightforward.

The benefit of the second approach is greater specificity, but it comes at the expense more complex rules to explain. Whether these rules are simple or not is kind of irrelevant: on a purely subjective level, I don't like requiring that themes have to have massively nested subdirectories to override things. That feels like the domain of the .info file.

But that, too, is a subjective consideration that should not dominate the discussion. My weight is behind the patch in #38. If there are concerns about its flexibility, it's worth considering dvessel's solution.

dvessel’s picture

Fair enough Eaton. :)

I'll test out your patch further. This is definitely important so I'll try to push it make sure we get this done right. :)

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

I applied and tested #38 and verified it works as described. Any other testers may note that visiting admin/build/themes resets the theme registry and the used css files. Interestingly enough, over-ridding the system-menu.css file with an empty css file doesn't have any visible effect on the Garland theme, since it actually over-rides every style in the entire system-menu.css file in style.css.

This patch presents some really interesting options for contrib, as themes can now provide a sort of 'optional' support for various contrib modules by including over-ridding .css files for each contrib module. Enabling (or disabling) the module-specific css could just be (un)commenting a single line in the info file. Perhaps in Drupal 7 we could see some more interesting options for disabling/enabling css files via an admin interface.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Reading what Dries said here already, I'd defer to Dries for judgement. What I noticed in the patch though:

- We capitalize CSS, HTML and so on in comments
- We add CVS ID tags to CSS files

eaton’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.1 KB

Added CVS ID tags to the two new CSS files and capitalized all uses of 'CSS' in the comments for drupal_add_css. It sounds like it's waiting for a thumbs-up/thumbs-down from Dries now.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
  1. I like the fact that it is a drop-in solution that does not require any changes to .info files.
  2. I don't like the name 'system-menus.css'. If this is related to the menu module, it should be 'menu.css' and be stored in the menu module's directory. If it can't be stored in the menu directory, I'd still recommend to rename the file to 'menu.css'.
  3. When I look at the patch it looks like we iterate over the entire list of CSS files, everything we call drupal_add_css(). This probably generates a couple dozen of file_exists() for every page view. The current code doesn't look particularly efficient so some good benchmarking is in order.
eaton’s picture

Status: Needs work » Needs review

I like the fact that it is a drop-in solution that does not require any changes to .info files.

It does require a change to info files in that the stylesheets must be LISTED in them, but it doesn't require learning any additional new syntax... Just to be clear. :-) The reason I want to clarify this is that earlier discussion hinted at 'auto-discovery' of CSS files without being explicitly listed in the .info file, and that approach WOULD definitely trigger the performance concerns you mention below.

# I don't like the name 'system-menus.css'. If this is related to the menu module, it should be 'menu.css' and be stored in the menu module's directory. If it can't be stored in the menu directory, I'd still recommend to rename the file to 'menu.css'.

We can't put it in with menu module because that module is optional, while these styles are used to build things as basic as the navigation menu. This is an issue with Drupal's scattered menu-handling code rather than its CSS.

At a higher level, this is a problem we're going to face any time we break down our styles into more granular chunks. The purpose of the stylesheet (menus) prefixed with the name of the module it's defined by (system) is the approach that I think remains consistent AND allows theme developers to copy-and-modify without jumping through excessive hoops. If we name it menu.css, and another module defines menu related CSS styles, should it ALSO name its file menu.css? At that point, the themer can only override one of them... If it's really a show-stopper, I can rename this patch's file to menu.css, but I really feel that it would get us into trouble as soon as it got into "the wild."

# When I look at the patch it looks like we iterate over the entire list of CSS files, everything we call drupal_add_css(). This probably generates a couple dozen of file_exists() for every page view. The current code doesn't look particularly efficient so some good benchmarking is in order.

It doesn't -- calling file_exists() was something suggested in an earlier discussion, not something that's actually tone in this patch. The only additional work that this version of the function does is iterating over the list of CSS files and doing a string comparison. We never run a file_exists() in this code unless RTL mode is turned on, and that was being done in the existing code already. That said, however, here's some benchmarking. I turned on several modules, including poll and aggregator, that add their own CSS files. Then I added an css override to Garland, replacing the menus CSS with an exact copy. That should be somewhat representative.

Before patch
Concurrency Level: 1
Time taken for tests: 119.511 seconds
Complete requests: 500
Failed requests: 0
Broken pipe errors: 0
Total transferred: 2811500 bytes
HTML transferred: 2570000 bytes
Requests per second: 4.18 [#/sec] (mean)
Time per request: 239.02 [ms] (mean)
Time per request: 239.02 [ms] (mean, across all concurrent requests)
Transfer rate: 23.53 [Kbytes/sec] received

Concurrency Level: 1
Time taken for tests: 1172.856 seconds
Complete requests: 5000
Failed requests: 0
Broken pipe errors: 0
Total transferred: 28585000 bytes
HTML transferred: 26170000 bytes
Requests per second: 4.26 [#/sec] (mean)
Time per request: 234.57 [ms] (mean)
Time per request: 234.57 [ms] (mean, across all concurrent requests)
Transfer rate: 24.37 [Kbytes/sec] received

After patch
Concurrency Level: 1
Time taken for tests: 119.064 seconds
Complete requests: 500
Failed requests: 0
Broken pipe errors: 0
Total transferred: 2858500 bytes
HTML transferred: 2617000 bytes
Requests per second: 4.20 [#/sec] (mean)
Time per request: 238.13 [ms] (mean)
Time per request: 238.13 [ms] (mean, across all concurrent requests)
Transfer rate: 24.01 [Kbytes/sec] received

Concurrency Level: 1
Time taken for tests: 1168.820 seconds
Complete requests: 5000
Failed requests: 0
Broken pipe errors: 0
Total transferred: 28585000 bytes
HTML transferred: 26170000 bytes
Requests per second: 4.28 [#/sec] (mean)
Time per request: 233.76 [ms] (mean)
Time per request: 233.76 [ms] (mean, across all concurrent requests)
Transfer rate: 24.46 [Kbytes/sec] received

Conclusions
While I'm loathe to suggest that performance actually INCREASED with this patch, testing with both 500 and 5000 requests was faster after the patch than before. I'd say that the change in this patch has no detrimental impact.

With the performance question resolved, I think we can focus just on the naming of the CSS file. While I'm willing to rename it if there's no other way to get this fix in, I think simply calling it 'menu.css' would be a mistake. Are there any other thoughts? I'm marking it CNR, but it's essentially RTBC pending an official thumbs-up thumbs-down on the naming of that one file from Dries.

dvessel’s picture

Status: Needs review » Needs work

I agree that "system-menus.css" is the best name for it for your exact reasons.

At first I thought it was a bug but overriding module styles will drop them out of their original order. Instead they get grouped with the "theme" group ordered by how it's listed inside the .info file. I think this is a nice touch. Modules shouldn't care about what order it's being listed, that's up to the themer.

There is a bug in this patch. In the RTL block your unsetting the old style and replacing it back with the exact same.

Also, the looping can also be reduced. There's only two types. It's etiher "module" or "theme". You don't have to loop for the type since we know we're trying to change module styles.

something like this..

foreach ($css[$media]['module'] as $module_path => $module_preprocess) {
  ...
}

not..

foreach ($css[$media] as $old_type => $paths) {
  foreach ($paths as $old_path => $old_preprocess) {
    ...
  }
}

As for the auto-detection.. I'll try and persue that in D7. Maybe we can have a registry for styles too to and make things more automated.

dvessel’s picture

Correction, It's setting the old type "module" for the rtl style while the main style is setting "theme".

They should share the same type.

// If the current language is RTL, add the CSS file with RTL overrides.
if (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL) {
  $rtl_path = str_replace('.css', '-rtl.css', $path);
  $rtl_old_path = str_replace('.css', '-rtl.css', $old_path);
  if (file_exists($rtl_path)) {
    unset($css[$media][$old_type][$rtl_old_path]);
    $css[$media][$old_type][$rtl_path] = $old_preprocess;
  }
}
dvessel’s picture

Status: Needs work » Needs review
FileSize
7.17 KB

Here's an update simplified a bit with the notes above. It will now unset the old styles in the override block and "pass through" the changes.

dvessel’s picture

FileSize
7.21 KB

Updated notes to better reflect what it's actually doing.

eaton’s picture

Status: Needs review » Reviewed & tested by the community

dvessel, thanks for the catch on the RTL stuff. I just ran it through again, and it's substituting things properly.

My only concern at this point is that removing the loop for core/module/theme makes it impossible for a theme to override a stylesheet added by its parent theme. Am I mistaken? Overriding theme stylesheets isn't, I suppose, part of the documented behavior of this patch so unless there's a pressing need for it right now, we can treat that as a separate issue.

After some additional testing, I'm going to set this back to RTBC. Dries, Goba, it appears the outstanding issue now is the naming of the system-menus.css file. I still believe that it needs to be modulename-prefixed to avoid collisions, but I'm willing to change it if there's no compromising on the issue. :-)

dvessel’s picture

Overriding the parent theme's styles was implemented already and it'd done before drupal_add_css even sees it.
http://drupal.org/node/141725

And there's only "module" and "theme".. "core" type never got in when m3avrck was implementing it for D5.

btw, the docs in drupal_get_css mentions core and should be removed.

dvessel’s picture

Oh, and here's a section on inheritance for sub-themes.. I'll need to clear that up.

http://drupal.org/node/171209

eaton’s picture

Well, awesome! Thanks for the clarification on those points, dvessel. :D

dvessel’s picture

No, thank you for bring this issue back to life. :)

It's great how this came about after the Lullabot podcast on the new Australian themer that had issues with this. Get some more newbs on the podcast. :p

Jacine’s picture

I'm new to the Drupal community. I've mainly been working with themes. I contributed my first one the other day. The theme I contributed, which should have taken me 2-3 days, took a full week and the CSS file is probably 4 times the size of what it should be, because of all of the overrides I had to include to get around the default, system and module styles. I also listened to the Lullabot podcast, and was not surprised at how the Australian team struggled getting the design together.

I can't tell you how happy I am to see this post, and that it's "ready to be committed" in D6. I can't thank you guys enough for doing something about it.

I love Drupal, and can't wait until this issue is resolved. I know that once it is, I'll start working on another theme to contribute, and I believe that other designers who don't necessarily have to time or patience to mess with all the necessary CSS overrides to achieve their desired result will come out of the woodwork as well.

eaton’s picture

Just a bump to note that the patch still applies against HEAD with the recent batch of changes.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

OK, I have a few concerns here:

- what about first checking whether there is a CSS in the array for the old path, and only then going to file_exists()? that would avoid most file_exists() calls on RTL sites
- I don't understand what effect that continue; has there... it seems to continue the foreach loop, which would continue anyway. The comment says it exits the search loop, which it should not do, because the remaining CSS files are not yet handled, right?

Also it seems to be a dangerous idea to let people override based on the file basename. It is just an informal "standard" to use modulename.css so some contribs could easily ship with style sheet names which overlap. Is this fear sanding without real grounds?

merlinofchaos’s picture

Goba: I believe your concern can be alleviated with documentation in drupal_add_css (which would need to be in this patch) to point out that modules must respect namespaces here. I don't think that's a very big issue, and it will be trivial for modules to ensure their .css files are properly named.

You are correct, the continue is pointless.

eaton’s picture

- what about first checking whether there is a CSS in the array for the old path, and only then going to file_exists()? that would avoid most file_exists() calls on RTL sites

It's a tossup between walking through the array a second time -- which would lead to uglier code -- and a single file_exists() call once we've found a matching style. Given the earlier performance testing, I think that the difference in speed is negligible and the payoff in simpler code (rather than multiple nested loops through the css array).

I don't understand what effect that continue; has there... it seems to continue the foreach loop, which would continue anyway. The comment says it exits the search loop, which it should not do, because the remaining CSS files are not yet handled, right?

You're right, this should be a break. The loop in question is only meant to search for a matching CSS file to override. Once we've found it -- and thus encounter the block of code in question -- we don't want to waste any time looping through the rest of the style array. I'll roll a version with that issue fixed.

Also it seems to be a dangerous idea to let people override based on the file basename. It is just an informal "standard" to use modulename.css so some contribs could easily ship with style sheet names which overlap. Is this fear sanding without real grounds?

It is, indeed, possible for modules to ship with style.css, or something along those lines. And this will cause it to collide with themes that add similarly-named stylesheets. There is, as best as I can tell, no way around this without ripping out our CSS management system and implementing it in a more robust manner. If the danger of stylesheet collisions -- and warning module developers to name their files accordingly -- is unacceptable, we should set this issue to Won't Fix right now and move on to other issues.

eaton’s picture

(That last bit sounded snappier than I intended -- I just mean that if a warning about CSS file naming is an unacceptable solution, I don't think we should waste time trying to come up with 'creative' ways around the lack of useful index/keys in our CSS storage arrays...) Patch coming shortly. :)

eaton’s picture

Status: Needs work » Needs review
FileSize
7.77 KB

I've fixed the bogus 'continue' call and replaced it with the appropriate 'break'.

I've also added comments for drupal_add_css() that explain the ned to module-prefix css filenames.

Regarding the file_exists() call for RTL themes, here's why we're calling it instead of just looping back through. If a module defines modulename.css, and modulename-rtl.css, the latter is automatically added. If a theme then overrides modulename.css, but NOT modulename-rtl.css, we don't want it to rip out both css files blindly. Thus, we check to see if the theme itself *provides* a replacement RTL file, then remove the module-defined one if it exists. The behavior seemed the safest approach, and the single file_exists() call doesn't slow us down in any measurable way.

Any reviews or feedback on this would be great; hopefully we can get it back to RTBC quickly.

eaton’s picture

OK, that's it. the patch is 7.77K -- if that's not a sign of perfection, what is? ;)

Gábor Hojtsy’s picture

Indeed, our logic about the RTL feature is a bit different. This implementation first looks if the theme has an RTL override file (file_exists()) and then removes the original one if there was an original one. What I suggested is that you look first if there was an original one (so there is a reason to file_exists() on an override which would mandate the removal). Now with the approach taken in the patch, all CSS overrides will result in a file_exists() if an RTL language is used, even if there was no RTL CSS to override, so there was no reason to remove anything.

You do:

if (file_exists(new file)) { remove old file from array }

I say:

if (in_array(old file) and file_exists(new file)) { remove old file from array }

This would save the file exists calls in cases where there is no RTL override present, if I see it right.

eaton’s picture

FileSize
7.89 KB

You're right! After picking through it and re-reading what you said, we can indeed preserve the same behavior while avoiding the file_exists call.

I've added that to the patch and beefed up the comment to ensure everything is understandable.

dvessel’s picture

Status: Needs review » Needs work

This is tricky, the rtl styles associated with the parent style sheet needs to come after. A theme providing only the base style (and not the -rtl.css) will make it useless due to cascading order.

For example, a theme overriding only node.css will drop that way down in the cascade below node-rtl.css.

And Eaton, the $old_rtl_path variable switches to $rtl_old_path.

eaton’s picture

Status: Needs work » Needs review
FileSize
7.89 KB

dvessel, thanks for catching that copy and paste error. I've corrected it.

I'm not sure there's any really good way to account for the trickiness of RTL handling, other than requiring that themes provide versions of both files. Is that something that we can simply document?

dvessel’s picture

FileSize
7.74 KB

Yeah, I agree. It'll get real ugly if we have to account for that. Since it'll be useless without themes providing rtl styles how about just removing them without a file_exists check? This also removes the need for a single str_replace().

eaton’s picture

Status: Needs review » Reviewed & tested by the community

That's a fair solution that sidesteps the icky problems, IMO. I dare to 'RTBC' this again...

eaton’s picture

For the record, from IRC:

[07:56am] NikLP: eaton: that "remove css" thread... is that pretty much going in, at some level? I didn't follow all of what was being said... :p
[07:56am] eaton: NikLP: good question. It's RTBC at this point, I think the biggest outstanding question now is Dries__' concern about the naming of system-menus.css versus menus.css.
[07:57am] Dries__: eaton: if you think that is the best name, than it is rtbc

system-menus.css is probably not the BEST name, but it's the only one that is safe for overriding without also defining a lot of metadata in drupal_add_css(), like 'what module added this file' and so on.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

OK, I only have documentation issues remaining. I would say that the theme related comments should be moved to the phpdoc for higher visibility (this function is not that big that this comment would be too far from the code) AND the new RTL pairing requirement should be documented there too. Otherwise I agree that this looks good now, so we are very close.

eaton’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.62 KB

I've moved the inline documentation from the function itself to the PHPDoc, explaining both the theme-overrides and the nature of the automatic '-rtl' inclusion. Here's hoping!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

The phpdoc coding style requires a one line, one sentence intro, so keep it that way. If you think it is too simple now, expand on it but keep it short please. Explanation can start in a paragraph under the one line summary.

eaton’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.64 KB

Took a closer look and realized that the RTL commentary was unnecessary. I've updated the PHPDoc again -- if there are any additional changes, specific direction would be helpful. :)

dvessel’s picture

Looks great! Time to mark as 'fixed'. :)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Great, committed. Thanks for the insistence and hard work on this, themers will be rejoicing around the world.

Gábor Hojtsy’s picture

Obviously this needs docs and possibly even coder module firing warnings when there are CSS files not named with a module name prefix. So get going on those!

Stefan Nagtegaal’s picture

Category: bug » task
Priority: Critical » Normal
Status: Fixed » Postponed (maintainer needs more info)

http://drupal.org/node/137062 and http://drupal.org/node/132442 definatly needs some information about this!

Gábor Hojtsy’s picture

Status: Postponed (maintainer needs more info) » Needs work

This is when we use (code needs work). (Not nicely, but this is what's available).

Bevan’s picture

JohnAlbin’s picture

Status: Needs work » Fixed

I thought this issue would be easy to doc, but the Writing .info files for themes was nearly empty of detail. I've filled out the details on this page, but I would appreciate others taking a look to make sure I didn't miss anything.

Also, I've added docs about overriding a module’s stylesheets to these pages:
Writing .info files for themes (Drupal 6.x)
.info files for themes (duplicate of above)
Converting 5.x themes to 6.x

Anonymous’s picture

Status: Fixed » Closed (fixed)
jjalocha’s picture

Hi, I'm not sure, if I'm allowed to post a question to a closed issue. Please correct me, if I'm doing wrong.

I'm trying to ignore 'system-menus.css' completely. According to the documentation (http://drupal.org/node/171205), I've added the following line to .info, providing NO replacement CSS file:

stylesheets[all][] = system-menus.css

But the whole <ul class="menu"> menu structure still renders with Drupal's default formatting, probably due to the following line in the HTML output:

<link type="text/css" rel="stylesheet" media="all" href="/html/modules/system/system-menus.css" />

I've asked for help at the Themes forum (http://drupal.org/node/196879) and #drupal-themes and #drupal channels, but I was told that D6 is still too new to expect much community support.

Crell’s picture

This issue is long-dead. What you're describing sounds like a bug, so please open a new issue for it. We try not to beat dead issues. :-) Thanks.

jjalocha’s picture

OK, sorry.
Bug reported: http://drupal.org/node/197124
Thanks.

iwasfirst’s picture

Hi!

I believe that the creation of this patch is one of the greatest contributions ever made to Drupal theming!

However, what I can't figure out is where to apply it, and exactly what to keep in mind when constructing the stylesheets i want to append. I' quite good with CSS, though a bit comfused as to where the stylesheets should be placed within the Drupal directory.

A short tutorial targeting the implementation and workflow for this patch would be greatly appreciated!!

//André (A newbie with visions)

dvessel’s picture

André, bookmark the theming guide. http://drupal.org/node/171179

Specific info on style sheets can be found here: http://drupal.org/node/171209

As usual, you may be able to find tutorials through Google.

This is not the place to look for support. Thanks.