The color module is not reusable. It does not load the CSS files from $vars['css'] array and uses a hardcoded style.css only. This isn't the way Drupal Core is doing for CSS aggregation and compression and therefor it is not reusable for themes with more complex CSS Themes like Zen and YAML that adds additional CSS files in the theme with drupal_add_css(). This additional added css files need to be rewritten (or needs to be rewritten after a CSS aggregation), too.

Please make color module follow the way drupal_build_css_cache() is using. There is a bugfix for CNR in http://drupal.org/node/113607 that will fix some other important bugs and is the way how color module should do, too.

CommentFileSizeAuthor
#28 color_rtl_bug.patch1.61 KBhass
#24 color_rtl_bugfix.patch1.41 KBhass
#19 color_rtl_175121.patch2.58 KBhass

Comments

chx’s picture

chx commented
Version: 6.x-dev » 7.x-dev
Category: bug » feature
chx’s picture

chx commented
Priority: Critical » Normal
hass’s picture

hass commented
Version: 7.x-dev » 6.x-dev
Category: feature » bug

chx: You haven't commented why you moved this to D7. I think this is a bug in D6 and i'd like to use color module in my themes. In D6 the theme .info file allows to define a custom CSS list. This list is not used in color module - therefor moving back as a D6 bug, what this is.

hass’s picture

hass commented

Additional, the CSS RTL support is broken.

hass’s picture

hass commented
Title: _color_rewrite_stylesheet does not load $vars['css'] array for rewrite » _color_rewrite_stylesheet does not load $vars['css'] array for rewrite / RTL support looks broken
yhager’s picture

yhager commented

I can confirm the color module breaks an RTL core theme (Garland) after a new color is chosen.

hass’s picture

hass commented
Title: _color_rewrite_stylesheet does not load $vars['css'] array for rewrite / RTL support looks broken » _color_rewrite_stylesheet does not load $vars['css'] array for rewrite / RTL support is broken

thx for verification

yhager’s picture

yhager commented
Title: _color_rewrite_stylesheet does not load $vars['css'] array for rewrite / RTL support is broken » color.module hardcodes 'style.css' thus breaking support for RTL and stylesheets[] definition
Priority: Normal » Critical

Actually I think this bug is not only about RTL support only.

By hardcoding 'style.css', color.module ignores the stylesheets[] definition in the theme info file (see http://drupal.org/node/137062).

I am changing to critical, since color.module must be adapted to the new theme system before the release IMHO.

hass’s picture

hass commented

I really hope we are not going to solve this in a dirty way like switching from style.css and style-rtl.css. If we solve this problem in general i'm able to reuse the module and we are able to solve therefore two things together :-).

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

I agree that this should be fixed in the release, if possible. I heard more and more people would like to depend on color module. So someone can come up with patches or patch ideas. :)

verb3k’s picture

verb3k commented

This bug is critical....see the Following images:
before changing color
http://i192.photobucket.com/albums/z123/verb3k/problem1.jpg
After changing color
http://i192.photobucket.com/albums/z123/verb3k/problem2.jpg

If this bug makes it way to the final release, I will not be able to use Drupal for my site. Seriously, I would just have to wait another 6 months for the next release.
Please fix it before Drupal6 final.

Tresler’s picture

Tresler commented

Am looking into this. The underlying issue is that drupal_get_styles returns the array of stylesheets in the order in which they should be called. However, color.module by it's nature needs to trump that.

Meaning that generally we ALWAYS want the theme to trump any module-specific CSS. Except for in the case of color.module. Looking into the functions that do all this now. Patch ideas welcome.

hass’s picture

hass commented

@Tresler: please take a look at http://drupal.org/node/113607 and drupal_build_css_cache(). This should be the future way... i think we only need to remove the aggregate and compress part of css compression and then we are fine. the order of css file must be kept as is for css overwrites.

Tresler’s picture

Tresler commented

yup was starting to look over that this mornign when my battery died, will try to look this evening.

hass’s picture

hass commented

maybe the above makes only sense if we'd like to merge all files into one bigger... on the other side - maybe better to copy all css files per theme.

yhager’s picture

yhager commented

It is important to understand that there are actually two bugs here:
1) color.module ignores implicit stylesheets, like 'style-rtl.css'
2) color.module ignores explicit stylesheets defined in theme.info file.

With regard to bug (1), we can assume there are no color changes in the '-rtl.css' version of the file. Therefore, the only problem is that the style.css with the adapted colors is included *after* the '-rtl.css' file, which breaks css cascading. See this snippet:

function _color_page_alter(&$vars) {
  global $theme_key;

  // Override stylesheet
  $path = variable_get('color_'. $theme_key .'_stylesheet', NULL);
  if ($path) {
    $vars['css']['all']['theme'][$path] = TRUE;
    $vars['styles'] = drupal_get_css($vars['css']);
  }

I am not a PHP guru that I can tell how to put the new, color.module-modified style.css instead of the previous one (in term of location in the array), but that would solve this problem.

hass’s picture

hass commented

@yhager: no, that's not the problem. This style.css only represents a compressed and colorized style.css located in the files/color/themename-[md5]/ folder.

I have analyzed color module code and i think i got the idea behind hardcoded "style.css" and understood that it looks impossible to load the $vars['css'] while we are at theme settings page :-(((.

It might work sometimes, but it will not work if someone set the "Use administration theme for content editing" in "admin/settings/admin". In this case we only have the css array of the current admin theme and not the theme we'd like to alter color and we need the css file list from.

Additional we cannot read the [theme].info file manually and 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(). To solve this we must do a theme_init, but i remember some people removed this theme_init's on theme settings page for D6 what hurt's now back :-(((.

This all in mind - i have no clue how we can solve this without loading/init the real theme - if color module is used. Only quick solution for RTL issue seems like adding the additional style-rtl.css and keep all this $vars['css'] bugs as long as someone get's a very good idea to solve this.

I hoped to say something different, while i'd like to depend on color module and no i cannot... This suxxx.

hass’s picture

hass commented
Assigned: Unassigned »
Status: Active » Needs review

With regard to bug (1), we can assume there are no color changes in the '-rtl.css' version of the file.

RTL files contain colored border's and background colors and so on. We cannot say for sure this will not happen.

hass’s picture

hass commented
StatusFileSize
new color_rtl_175121.patch2.58 KB

Here is a patch that solved this RTL bug. For testing you need to apply the patch in http://drupal.org/node/113607 and afterwards this patch.

yhager’s picture

yhager commented

This patch requires additional parameter for stylesheet_rtl in the theme.info file.
How do you see it co-existing with drupal_add_css to select RTL CSS files when necessary and RTLized CSS files?

hass’s picture

hass commented

@yhager: as i tried to explain above we cannot look into theme.info files. This is the major problem... i don't know how to solve.

We must be sure there is a style.css file and then we simply use it. I thought to put some file_exist checks for RTL around, but decided not to do so, while current color module code doesn't too. If you'd like to use color module with a theme - this theme MUST have an style.css and now style-rtl.css, too. If not - there comes up a "file not found" error in theme setting page after save. This seems to be an expected behavior - i learned from the existing code.

hass’s picture

hass commented

For deeper understanding of the theme settings page and the problems you may read http://drupal.org/node/57676. The general problem is discussed there and has ended with an extra theme-settings.php file as the only solution.

verb3k’s picture

verb3k commented

@hass: what I want to know is will Drupal6 final have this bug fixed? I really need to know.

hass’s picture

hass commented
StatusFileSize
new color_rtl_bugfix.patch1.41 KB

I reduced this patch and now style-rtl.css is no more mandatory. I think this patch is much smarter then the patch before.

hass’s picture

hass commented

@verb3k: if you test this patch and approve it is working well, this patch can go in. So please test...

verb3k’s picture

verb3k commented

I've applied those 2 patches (hopefully correctly...first time to patch) and I got this error:

Fatal error: Cannot redeclare _drupal_build_css_cache() (previously declared in /var/www/drupal6/includes/common.inc:1670) in /var/www/drupal6/includes/common.inc on line 1753

I followed the "Applying Patches" tutorial but I don't really know if this error stems from my faulty patching or from the patches themselves.
I would appreciate it if you could give a brief howto on these 2 patches.
Thanks in advance,
verb3k

verb3k’s picture

verb3k commented

@hass: I can confirm that the patch works as intended (sorry about my previous post)
RTL themes renders correctly when using color.module. Thanks hass :)

hass’s picture

hass commented
StatusFileSize
new color_rtl_bug.patch1.61 KB

Thank you for your testing.

I created a new base patch at http://drupal.org/node/113607#comment-314734 that solved the general problem i described above and will allow to add whatever files the theme have... this patch is the follow up and add the RTL support only.

hass’s picture

hass commented

PS: to apply this patch you should move the file to modules/color directory and execute patch < color_rtl_bug.patch

hass’s picture

hass commented
Status: Needs review » Closed (duplicate)

This follow up patch has been integrated into http://drupal.org/node/113607