Color module limited to style.css, broken with Minnelli

Can hardly be a breaker for beta 3, as that's been out for a little while.

Guys, there is no need to mark anything a breaker. If there is code ready, it will be in, otherwise the beta will be out without it. We are not delaying releases on issues without much of an interest.

Now that the patch this one depends on is committed, catch on work here :) This patch will also need to fix the IMHO bugos phpdoc on drupal_load_stylesheet():

This optimization will not apply for color.module enabled themes with CSS aggregation turned off.

If the aggregation is turned off, the optimization does not apply to any CSS anyway. So this is bugos.

Still applies although it has windows linebreaks, marking to needs work for the phpdoc.

However - without this patch we don't have color module support for RTL and a crippled color module that is totally non-reusable and buggy and only works for Garland.

About all bugs fixed with this patch, please read the referenced issue (http://drupal.org/node/113607#comment-500010). Everything has already been written.

Well, well. I tried to understand what issues are exactly fixed by *this* patch, but the link you gave discusses the fixes which are already in Drupal for @import handling. Also you point towards another issue for RTL stuff. Now that we have color module fixes here, maybe you can do a writeup on what is happening here. Please specify what bugs you found and how you solve them here. It would help a lot of we would not need to cherry-pick information from two other issues which also solved other issues. Thanks.

Or to put it another way: it is good to know if we can find more attractive things to market this patch for review here. Like Minnelli is not recolorable in Drupal 6 head, because it does not use style.css, it uses minnelli.css. That's a pretty important problem to solve. Let's have a list of bugs found and approaches taken in this issue!

Subscribing and testing..

Let me ask, is this really important? Rewriting rtl styles as it applies to color is really needed? rtl style sheets from what I've seen only apply to positioning and padding, not the actual color.

Did I miss something here? Please enlighten me.

The only thing I found useful is specifying the stylesheet. style.css isn't mandatory anymore and being specific about that would be nice. This would help the issue with Minnelli as Gabor mentioned.

I tested without this patch and rtl seems to work fine with color changes in Garland.

However - without this patch we don't have color module support for RTL and a crippled color module that is totally non-reusable and buggy and only works for Garland.

And if you can mention specific issues about this.. What are the issues outside of Garland. Thanks.

BUG: Add's RTL CSS re-coloring support. Current color module hardcodes "style.css" and does not import RTL files where border and background colors could be inside.

I agree, need to be changed.

BUG: Cannot add more then one file per theme. I'm adding ~10-20 in template.php with drupal_add_css(). Solved with the configuration array in color.inc

Was never supported so it's not a bug but I think this can solve another problem. See the end.

BUG: Color module does not import all type of @import we should support. For e.g. @import url("example.css") isn't possible. With the patch i'm reusing the @import code logic from #113607 and are not made to care about two different logics that make people confused.

Agreed, keep it consistent.

BUG: Dynamically added CSS files support is missing. The configuration option in color.inc is the solution for this issue. For e.g. you have two different menus in CSS. With theme-settings i'm able to provide a select box for switching them. In template.php i'm switching the menu style, but i cannot extend/write the style.css with @imports commands dynamically. With the configuration option i'm able to list all possible files that could be used - doesn't matter if really used now or only partial or not at all - but they are re-colored and i'm able to add the navigations dynamically if settings are changing. GREAT stuff...

Not a bug, your adding a new feature.

BUG: Current code add's the original style.css and the re-colored style.css to the page head. This doubles the CSS size and loading time and has been reported in #145369. After the refactoring this was only a one line fix in _color_page_alter and i decided to include this performance hit. This becomes a real performance issue if you add ~20 CSS files like i do and then you have 40 files with first byte download, 20 more connections to the webserver, etc.

That's only true when CSS aggregation is disabled and a big point of aggregation is minimizing requests. If you can colorize your theme, then you can turn on aggregation and solves that problem. I didn't study how your patch deals with this directly but if it fixes that, great. Not critical.

Feature _color_rewrite_stylesheet becomes re-usable. As themer i'm able to call the function whenever i'd like/must. I'm pushing content to the function and get content back. This content is then not saved to a predefined filename. Workaround this problem is really complex and i struggled hardly about this limitation.

Sounds like an interesting feature.

We cannot read the [theme].info file to get the css list from there... this might work often, but not every time, while some people like me dynamically adding css files via theme in theme_preprocess_page() with drupal_add_css(). ..[snip]

...see above.

Most of your points are leaning towards advanced features so I don't think it's helping this issue.

Note about the second point:

Instead of trying to automatically re-write the rtl styles (which I see as complicated and duplicates code from drupal_add_css()), why not just give the option to list the main style + the rtl inside color.inc. Looks like your patch is already set to accept multiple styles.

Not exactly related but there were complications with trying to let themes override module rtl styles automatically. To simplify, why not let the theme set it explicitly? Ya know, carry over that same requirement of being explicit with rtl files. If you know how to work with color.module, you surely can handle the extra line adding the extra styles.

About Minnelli.. it's broke and some minor tweaking should fix it. Should not a separate issue since that's a genuine bug.

In short, I think the only real issue is getting Minnelli working and tweaking the @import handling so it's more inline with the previous import patch.

RTL handling is not critical IMO but would be nice to fix. Even if it wasn't, there would be workarounds with setting a rtl class and overriding that through the main style sheet. Not ideal but than again, how often would a theme depend on what color a side border is in?

Hass, there's nothing wrong with rolling the Minnelli patch together. It is a color issue and it's doesn't complicate it further.

The other points I'll leave up to others to judge but code wise, there is an error where it does the split.

from "_color_rewrite_stylesheet()":

  // Split off the "Don't touch" section of the stylesheet.
  list($style, $fixed) = explode("Color Module: Don't touch", $style);

That function runs multiple times and the "Don't touch" string isn't always present so it will spit out a notice.

Checking for the string works. Here it is, fixed the notice and the help docs changed removing references to style.css. Everything else is the same.

I was going to make a simpler patch partly based on your code Hass, but I'll wait for what Gabor thinks.

[edit: and this does include the fix to Minnelli.]

Reviewed the latest patch:

- the code has weak comments... I find it quite hard to understand some of them like "// Add the non-colored stylesheet as we cannot use if/else in following loop.". So why cannot you use if/else? This sounds like a weak technical reason, but it might not be. I am confused.

- pulling out original styles looks like a good idea

- With the variable changes I wonder what happens on update... this is our first release, where color module needs to be upgraded... Because styles changed, color settings will need to be updated as well, right? This is not actually related, but people should be aware of this. What is related, is that the stylesheet variable is dropped, so as soon as it is dropped, the styles will not show up anyway. This should be on the upgrade path too (ie. do not leave unused variable in the database).

- Rewrite theme stylesheets has the exact same long code block copied twice. Why not make up a one or two element array and loop over that. Now whenever you modify something, you need to do it twice. This looks ugly.

- Hass, you use case about the CSS property in color.inc does not seem common. Why not use the built-in .info file based CSS discovery, which also handles inherited CSS files? The css property in color.inc seems like a superfluous extra place to specify the same as you did already in your theme .info file (or which was discovered by Drupal based on your .info file) for most themes.

1. Understandable, refine that comment please.

2. I mean if people upgrade their core Drupal 5 site to core Drupal 6 (no contrib module or theme, but a recolored Garland), because Garland's CSS was modified, they need to somehow run the recoloring process, right? So as part of the update, we would need to affect the settings array and possibly run recoloring on all themes, which were colored before. This later issue might need to go to another bug report, I would not like to overload this issue with more tasks, just realized that we have a problem here.

3. The long block of code seems to be the same so an array(original style, rtl style) or array(original style) if there was no rtl style and a foreach on the array with the same code block seems logical to me.

4. Ah. I am not buying all this stuff about the .info files. You can load the theme .info file whenever you wish. But I am buying the reasoning that you only need a subset of your styles recolored, so you can specify those. For *this* reason, a separate css array seems to be workable.

So tasks to do:

- fix the code comments (1)
- provide upgrade path for variable (2)
- clean up repeated code (3)

Well, this looks much better now. I hope you can take some time with testing this more, and dvessel or someone else can also look into it soon. I only see some code comments not looking nice, but those I can always fix myself, instead of playing this back and forth. Now concentrate on testing.

Retitled to be more to the point on what are we fixing, it is marginally RTL related.

It works, so marking RTBC.

I went through the code comments, and edited them for better English usage (as far as I know :). Also made the Don't touch string a variable as it is used multiple times, so now it is more evident. Committed.

forgive my ignorance

but I think there are MANY drupal users out there -- just like me -- who love D6, like the Garland theme, but just want to be able to change the color combinations.

so you got a patch here

great!

NOW......

How is it applied?

Why can't a file be created, so that when I upload into my site, it will override the existing file with the bug, and we all get a nice sleep?

Can anyone explaine how us -- not php savvy -- can apply this fix?

do we have to write some code? or can we just upload a file (patch)?

if a patch is just a file or files with coding entered or edited -- why can't the corrected file -- AS A WHOLE -- be provided?

please help

jprieto: This is included in Drupal 6, no need to apply anything yourself.

im really not into coding, except for html, but I just installed the latest drupal 6 and when i tried to change the colorset in the minelli AND garland theme to, lets say, teal top, all i get is a theme without any css. plain html, like you can see on blog.fefe.de for example (not my site, but it looks like it after using the color switcher in drupal.) so if you think this is patched. tell me whats wrong. and please tell me if you want a screenshot. hard to find info on this if you are a fresh drupal starter btw, i hope i didnt do anything wrong with posting this. but as this is "fixed" but doesnt work, i didnt know better.

greetings moortrommler