Color.module does not support more than 5 colors and has hard-coded labels

Color.module has been such a long-neglected module because of these limitations. Simply expanding the number of colours allowed and the labelling would make it much more likely to be used by contrib themers.

There is this also module, which works fairly well, but hasn't really ended up in a stable state: http://drupal.org/project/color_soc08

Sub.

Yeah I've wanted to add color.module integration to a module I was writing and it's impossible with how much hard-coding is going on.

Here is an initial patch that makes color.module more flexible. You are now able to define which fields should be used (and which labels they have) in color.inc.
Next step is to make the gradient more flexible (allow more than one gradient, etc., ).

Happy testing.

This is amazing! I have a theme that sues the color module (dropshadow), and I always had been annoyed by the fact that the labels couldn't be changed. Thanks for your work here guys! Will test the patch and report back.

Here is another version that refactors the gradients.
You are now able to have multiple gradients in your theme (vertical and horizontal!).
Unfortunately I couldn't really test horizontal gradients because of I do not have a theme that makes use of it (though it should work; hopefully).

I confirm this worked well. I changed the labels defined in Garland's color.inc, and added a custom field for which I added a hex value in some of the color schemes. I didn't have any problems. I would love for this patch, after its tested by more people, to go in. The changes are nothing drastic and is something that would be so beneficial to themers who don't really get the color module, I could definitely see a lot more themes with nice color schemes come out if this went through.

I would love to use this version of the module when porting my dropshadow theme to Drupal 7, because one of the things that let me down when working on the 2.x branch is that I couldn't rename the labels without hacking core. That coupled with problems using the base color field because it was so heavily tied with Garland, that I couldn't get it to work very well (wouldn't save the exact colors I picked), led me to slow my work on that branch and never come up with a beta release.

This issue should probably be also tagged with: Bartik, and D7 theme.

I will try to quickly port the 2.x branch of Dropshadow to d7(with this patch) and see if I have the same problem I had with base color as I did in d6.

This is a screenshot of the patch in action:
http://img.skitch.com/20100124-rxia8u83j6etijux9ui3xk48g7.png

A little bit of documentation:
The "fields" array *must* contain the keys "base", "link" and "text". All other items are optional.

If you define a new scheme, you only have to add the color keys that are different from "default" because it is merged with the values of scheme "default".
A definition of the following code in color.inc

    'default' => array(
      'title' => t('Blue Lagoon (Default)'),
      'colors' => array(
        'base' => '#0072b9',
        'link' => '#027ac6',
        'top' => '#2385c2',
        'bottom' => '#5ab5ee',
        'text' => '#494949',
      ),
    ),
    'test' => array(
      'title' => t('Test scheme'),
      'colors' => array(
        'top' => '#2a2b2d',
        'bottom' => '#5d6779',
      ),
    ),

will result in

    'test' => array(
      'title' => t('Test scheme'),
      'colors' => array(
        'base' => '#0072b9',
        'link' => '#027ac6',
        'top' => '#2a2b2d',
        'bottom' => '#5d6779',
        'text' => '#494949',
      ),
    ),

for scheme "test".

Just noting that this feature supports #695292: Add new theme to core: Busy (and therefore I think an exception could me made in adding new features while in alpha)

This feature also supports #683026: Add a new theme to D7 core: Bartik

Please make an exception for this! Drupal 7 really deserves a more flexible color module!

Before I forget:

+    	// Horizontal gradient.

That line's using a tab instead of spaces.

+1 for inclusion in D7.

Tested the patch; while I don't have a theme to try it on, the existing features of color module all seem to work fine on Garland. In other words: this doesn't break existing stuff :D

This patch totally breaks for me with Bartik. When I go into Bartik settings I get an empty overlay as seen in the attached screenshot. It does work with Garland, though.

When I go back to the List overlay, I see in $messages:

* Notice: Undefined index: default in color_scheme_form() (line 151 of /Users/theresasumma/Repositories/d7/modules/color/color.module).
* Notice: Undefined index: default in color_scheme_form() (line 151 of /Users/theresasumma/Repositories/d7/modules/color/color.module).
* Notice: Undefined index: default in color_scheme_form() (line 151 of /Users/theresasumma/Repositories/d7/modules/color/color.module).
* Notice: Undefined index: default in color_scheme_form() (line 151 of /Users/theresasumma/Repositories/d7/modules/color/color.module).

Even if we didn't get flexible color names, I would love to see support for a link hover color, content section header color, block header color. Just those would make a huge difference!

@theresaanna It couldn't work with Bartik (or better: Bartik won't work with the patched version of color.module for now) since it uses a completely new (and different) syntax for defining schemes and gradients.

Therefore "old-style" color.inc implementations do not work with the patched color.module.

Here's a perl script that helps convert old-style colour schemes from color.inc to a new array with key names.

To use it, run it in your theme folder. It will print out the new format for the 'schemes' key, which you can then use in your color.inc file.

cool, subscribe!

I've updated this patch with some minor code style changes. I also tested the patch and it works fine with Garland (which is the only theme in core that supports the color module).

I seriously doubt this can get into D7.

(Adding the tag Bartik, so we can keep an eye on this in that tag list.)

@dmitrig01 I agree this is an API change. Although, it's a very minor one that only touches one core theme at this moment (Garland). It also removes some very ugly hard coded - Garland specific - stuff.

So it is for a very good reason, imho. The state of color module in D6 and currently in D7 is almost unusable for other themes like Bartik, with fixed labels etc.

If this won't change, the color module support for Bartik won't happen, I think.

IMO, color module is so flawed, before we release D7, we should:

1. remove it as a module and converted into a garland_form_system_theme_settings_alter form alter, or
2. fix the ridiculously garland-specific hard-coded things about the API (i.e. the label and the 5 color thing)

@John "fix the ridiculously garland-specific hard-coded things about the API (i.e. the label and the 5 color thing)"
This is done by the patch I posted.

You are now able to have more than 5 colors (at least "base", "link" and "text" are required) and you can have different gradients on the site (though I couldn't really test that feature).

Hey guys. I'd love to help. Been a while since I've been in the issues. :)

I'd be happy to help out anyway possible to tweak D7's color module for our new default theme(s).

Yeah, I understand it's a little late, but perhaps we can slip it in as a theme-centric type patch. Like reconciliation in senate :D

#4: 693504-color_module-custom_fields-4.patch queued for re-testing.

What I need to do in busy is create gradients with diagonal direction (this is fine with doing them by overlaying a semi-transparent png over the background color).
But here comes the trick: I need to do it in different colors, since in this one http://drupal.org/files/issues/busy-red-1.jpg The gradient for the footer must be created in blue, and the one for the sidebar on the right side in red.
If it is just too hard to do or impossible or too much effort I may back down to regular vertical gradients that can be done with the gradient tool. But not yet... For the PNGs allow for all kinds of crazy (one-color) gradients other themes can also make use of.

I wonder if this can be made possible.

If we apply normal rules, which we generally should, this patch gets bumped to Drupal 8 as a new feature.

However... I can plainly see by reading the patch that the current Color module was only intended to ever work with Garland, and that's making it hell for these new core themes to use. And even though Color module has been part of Drupal core since version 5, so the current situation cannot be classified as a regression, it wasn't until right now that we actually started trying to add functionality that took advantage of this API elsewhere in core.

I think I'm willing to grant an extremely limited exemption to this patch, assuming that Dries doesn't overrule (which would be perfectly sane to do so). And by that I mean keep it to only fixing the clear and obvious "bugs" in the API that make hard-coded assumptions about Garland and make it work for other themes, not going totally bananas and making Awesome Color Module With Kittens And Ponies And Yay!

Deal?

Deal!
Ok, lets sum up what we need to fix:
- fields that defines colors are hardcoded in color.module and color.js
- only one gradient can be set

This is all fixed with the patch.

Did I forgot something (or added to much)?

Oh, I'm liking this!

+++ modules/color/color.js	30 Jan 2010 19:38:25 -0000
@@ -24,11 +24,17 @@ Drupal.behaviors.color = {
+    var h = [];
+    var w = [];

Don't use one-character variable names.

+++ modules/color/color.js	30 Jan 2010 19:38:25 -0000
@@ -24,11 +24,17 @@ Drupal.behaviors.color = {
+      var max_val = (settings.gradients[i]['vertical'] ? h[i] : w[i]);
+      for (j = 0; j < max_val; ++j) {

You can combine this into one statement.

+++ modules/color/color.js	30 Jan 2010 19:38:25 -0000
@@ -41,11 +47,12 @@ Drupal.behaviors.color = {
+      var schemes = settings.color.schemes;
+      var colorscheme = this.options[this.selectedIndex].value;

var schemes = settings.color.schemes, colorscheme = this.options[this.selectedIndex].value;

Also, use camelCase for variables in JavaScript

+++ modules/color/color.module	30 Jan 2010 19:38:25 -0000
@@ -251,10 +268,12 @@ function color_scheme_form_submit($form,
-      $palette[$k] = array_shift($scheme);
+      if (isset($info['schemes'][$form_state['values']['scheme']]['colors'][$k])) {
+        $palette[$k] = $info['schemes'][$form_state['values']['scheme']]['colors'][$k];

Again, don't use single-character variable names.

+++ themes/garland/color/color.inc	30 Jan 2010 19:38:25 -0000
@@ -35,8 +172,14 @@ $info = array(
+      'vertical' => TRUE, // vertical or horizontal gradient.

I would rather use 'type' => 'vertical' or 'type' => 'horizontal'

+++ themes/garland/color/preview.css	30 Jan 2010 19:38:25 -0000
@@ -6,10 +6,10 @@
+#preview #gradient-0 {

What about all subsequent gradients? shouldn't it be .gradient or something?

Powered by Dreditor.

Ok, I've changed the coding according to Dmitris notes.
Except for "What about all subsequent gradients? shouldn't it be .gradient or something?". This is a theme specific selector that could be totally different in other themes (and is surely different if you are using a horizontal gradient).

+++ modules/color/color.js	7 Feb 2010 13:17:59 -0000
@@ -24,11 +24,16 @@ Drupal.behaviors.color = {
+    var height = [];
+    var width = [];
+    for (i in settings.gradients) {
+      $('#preview').once('color').append('<div id="gradient-' + i + '"></div>');
+      var gradient = $('#preview #gradient-' + i);
+      height.push(parseInt(gradient.css('height'), 10) / 10);
+      width.push(parseInt(gradient.css('width'), 10) / 10);
+      for (j = 0; j < (settings.gradients[i]['direction'] == 'vertical' ? height[i] : width[i]); ++j) {
+        gradient.append('<div class="gradient-line"></div>');
+      }
     }

A comment would be cool, what this code does.

Powered by Dreditor.

Here you are.

+++ modules/color/color.js	8 Feb 2010 18:41:37 -0000
@@ -24,11 +24,23 @@ Drupal.behaviors.color = {
+      // gradients) of 10px (because we devided the height/width by 10 above).

Divided.... :)

I'm on crack. Are you, too?

Ups. Corrected.

So, I'm trying to make the busy theme candidate color enabled, and these changes would be HUGE towards making that possible. I'll try out the patch tomorrow hopefully and reply back, but base on having dug around in color's guts some already, these changes look very sane.

Changing names for "top" and "bottom" and adding more colors works fine. I will do more tests later this day.

If I set base color for default color scheme in color.inc to be exactly the same as in style.css I'm getting "Undefined index: base in _color_rewrite_stylesheet()" error when switching schemes in admin panel. But if base color is slightly different than in style.css (e.g. #f0f8fe instead of #f0f8ff) everything works correctly.

I have attached minimal theme which demonstrates this issue.

What comes through is that color module needs to support any sort of CSS selectors to be recolored: say you want all h3, #content a and .node h2 elements to have the same color - this must be possible.

Else you get always tripped by two elements having by chance the same color in the default color scheme and you cannot seperate them out in a clean way. How I understand the module at the moment is it picks elements depending on the area on the site and apart from that by color name, regardless of semantic circumstances.

To keep it easy, it might be possible to just override color names, but set a selector of say #content h2 to have greater weight than that. Thus you can keep the handy ability that the module picks any color name from the css - if you do not adress the selectors more precisely.

@eigentor Alternative solution that comes to my mind would be using PHP variables in CSS files. E.g. you could define values for $page_background variable in color.inc for each color scheme and then use this variable throughout all stylesheets like this:

#page {
  background-color: $page_background;
}

But that would make an "awesome Color Module With Kittens And Ponies And Yay!".

When I thought about it some more, it came to me that my idea does not really work.

I guess Color Module does simple string replacements and thus can only replace color names.
For someone creating color styles there is a simple Trick: One could differentiate the colors by giving say #FF8800 to .block h2 and #FF8801 to .node h2. This way one always keeps theme seperate.

More important would be what the title of this issue implies: supporting more than 5 colors and giving custom names to them.

I just cannot get this to work. I am not sure what I am doing incorrectly. I have tried applying a couple different patches that are supposed to add the custom fields and and they just don't show up. I have tried this in Garland and in Pixture_Reloaded. Nothing in either.

Any thoughts on what I could be doing wrong? I mean, it is not that it won't work properly, but that the field just doesn't show up. Thanks

@jibninjas Are you sure that you have enabled color module in admin panel? It's disabled by default. Also note that you have to initialize it in template.php file (should be already done in Garland).

Yes, both of those things have been done. I have already been using the color module in the pixture_reloaded theme but wanted to add in a couple new fields. But no matter what I do only the 5 standard fields show up.

Also, looking when applying the patch there were numerous things that were not where they should be. The lines that it said to edit rarely matched up with the line it was really on. Also I really could not find a perfect match to

@@ -172,7 +189,10 @@ function color_scheme_form($complete_for
         $base . '/color.js',
         array(
           'data' => array(
-            'color' => array('reference' => color_get_palette($theme, TRUE)),
+            'color' => array(
+              'reference' => color_get_palette($theme, TRUE),
+              'schemes' => $schemes,
+            ),

Well, it looks like this is only for D7 and I was trying to get it to work with D6. Does anyone know if you can use the color.module from D7 in D6? Or if there is another patch for D6 that accomplishes the same thing?

> Or if there is another patch for D6 that accomplishes the same thing?

There isn't, and there probably won't be as it would break D6 themes that depend on color module.
What you could do instead is write an alternative color module as a D6 contrib module and let themes use that if they wish to. But that's outside the scope of this issue.

I would love to write a contributed module to accomplish this. But unfortunately I am just not that good. I know a little php and suck at javascript, so it would be pretty difficult for me.

@jibninjas: try Color.module TNG.

Colors in admin panel are sorted very chaotically, I can't find a pattern. It would make sense to preserve the same order as in color.inc file or at least sort them alphabetically.

After applying 693504-color_module-custom_fields-35.patch i get a blank page when selecting 'settings for bartik on appearance page. After re-selecting 'appearance'

# Notice: Undefined index: default in color_scheme_form() (line 151 of /Library/WebServer/Documents/testcms/modules/color/color.module).
# Notice: Undefined index: default in color_scheme_form() (line 151 of /Library/WebServer/Documents/testcms/modules/color/color.module).
# Notice: Undefined index: default in color_scheme_form() (line 151 of /Library/WebServer/Documents/testcms/modules/color/color.module).
# Notice: Undefined index: default in color_scheme_form() (line 151 of /Library/WebServer/Documents/testcms/modules/color/color.module).
# Notice: Undefined index: default in color_scheme_form() (line 151 of /Library/WebServer/Documents/testcms/modules/color/color.module). 7-alpha1 with bartik downloaded from git. Garland color selection works OK.

Oops. Sorry for duplicate. I saw #39 but not #13 so thought mine was different.

@dcrocks: I guess bartik doesn't builds upon the patched version of color.module so I wouldn't expect this to work.

It appears the array structure for 'schemes' and 'gradients' in color.inc has changed, I don't know if after the patch or before. I made corresponding changes from Garland's color.inc to bartik's color.inc and it appears to work. I assumed the array of colors in the current bartik file corresponded to (base,link,top,bottom,text). Is there a current demo ssite where I can check to see if I screwed up the layoit?

I am new to drupal and don't know how to build patches, never mind testing them. I am playing with several of the 'proposed' core themes for ver 7 and am trying to learn by seeing how they operate, so the color problem with bartik intrigued me.

At them moment there is no theme build upon the patched version ot color.module (except for garland, which is modified by this patch too).
So you have to manually edit color.inc of bartik to match the new structure.
Bartik and Busy will be using the changes made to color.module if they will be committed. Otherwise it has to wait until D8.

Since Bartik is being developed on Git, we could start a branch there in ancitipation of this patch getting in -- that would give us a second theme to play with.

I understand change has to end somewhere, but it doesn't look difficult for individuals to make their own changes if they like this. Still a lot I don't understand about this module yet. But the color picker is neat to play with. Am also trying to understand role of color.css file in bartik. Drupal has so many logical overlays and confusing documentation it would be nice if every file would carry a purpose and impact statement embedded.

I've found another two issues:
- colors other than "base", "link" and "text" are not shifted on preview page.
- overwriting theme_color_scheme_form($variables) does not work

Corolla theme now supports patched color module.

Subscribing.

I have ported my theme back to standard, unpatched version of color module and it has the same issues:
- "Notice: Undefined index: base" errors appear if base color value specified in color.inc is exactly the same as in stylesheets
- overwritting theme_color_scheme_form() does not work
- color preview does not show "Header top" and "Header bottom" colors

So this patch does not really introduce any new bugs, though the color module is so flawed that it would make more sense to remove it altogether and use subthemes or stylesheet switching instead.

What's the status of this?

dcrocks created a txt file with proposed changes to Bartik, to help it match the work going on here. #749276: Alter Bartik to run with the modified color module (from #693504)

dcrocks also said above:

Am also trying to understand role of color.css file in bartik.

This is why we made color.css:
When you use color module, it duplicates the stylesheet where the colors live, and takes the duplicate and hides it away in color module world. It also hijacks the theme, and points the theme to the new hidden copy of the stylesheet, disabling the original copy.

When the color styles are in styles.css, this hijacking means that the original styles.css file is totally disabled. And changes that anyone subsequently makes to styles.css don't work.

Since we know people like to take core themes, duplicate them and hack them into submission, letting color module disable styles.css is a recipe for frustration. By putting color module styles in a file be themselves, all color module hijacks is control of the color. All the other styles remain unaffected and available for changing.

Looks like this issue got derailed with unrelated stuff and no one really reviewed.

So here's my initial review. Note that I have tried to remain as ignorant as possible as to what's been going on in this issue, so that I can give it a totally fresh set of eyes. Apologies if any of this was already mentioned above. Also, I didn't actually test this patch, deferring that to others.

+++ modules/color/color.js	8 Feb 2010 21:20:28 -0000
@@ -24,11 +24,23 @@ Drupal.behaviors.color = {
+    for (i in settings.gradients) {

Missing var here.

+++ modules/color/color.js	8 Feb 2010 21:20:28 -0000
@@ -24,11 +24,23 @@ Drupal.behaviors.color = {
+      var gradient = $('#preview #gradient-' + i);

Double id selectors like this usually don't make sense. IDs are unique, by nature. :)

+++ modules/color/color.js	8 Feb 2010 21:20:28 -0000
@@ -24,11 +24,23 @@ Drupal.behaviors.color = {
+      for (j = 0; j < (settings.gradients[i]['direction'] == 'vertical' ? height[i] : width[i]); ++j) {

Missing var for j here.

+++ modules/color/color.js	8 Feb 2010 21:20:28 -0000
@@ -39,13 +51,14 @@ Drupal.behaviors.color = {
+        for (field_name in colors) {

@@ -56,29 +69,31 @@ Drupal.behaviors.color = {
+      for (i in settings.gradients) {
...
+          for (j in color_start) {
...
+            for (j in accum) {

Ditto (var).

+++ modules/color/color.js	8 Feb 2010 21:20:28 -0000
@@ -39,13 +51,14 @@ Drupal.behaviors.color = {
+          callback($('#edit-palette-' + field_name), colors[field_name], false, true);

@@ -132,7 +147,7 @@ Drupal.behaviors.color = {
+    function callback(input, color, propagate, colorScheme) {

@@ -140,8 +155,8 @@ Drupal.behaviors.color = {
+      if ($(input).val() && $(input).val() != color) {
+        $(input).val(color);

callback() (what a *ugly* function name! :P) gets a jQuery object in 'input' already. Therefore, input should be renamed to $input - which would have prevented the double-and-triple invocation of $() here.

+++ modules/color/color.js	8 Feb 2010 21:20:28 -0000
@@ -56,29 +69,31 @@ Drupal.behaviors.color = {
+            delta[j] = (color_end[j] - color_start[j]) / (settings.gradients[i]['vertical'] ? height[i] : width[i]);

I don't see height or width being defined in this function...?

+++ modules/color/color.module	8 Feb 2010 21:20:29 -0000
@@ -146,17 +142,38 @@ function color_scheme_form($complete_for
   // See if we're using a predefined scheme.
-  $current = implode(',', variable_get('color_' . $theme . '_palette', array()));
+  if (!variable_get('color_' . $theme . '_palette', FALSE)) {
+    $current = 'default';
+  }
+  else {
+    $current = implode(',', variable_get('color_' . $theme . '_palette', array()));
+  }

Would be good to fetch variable_get() into a local variable first (and once).

+++ modules/color/color.module	8 Feb 2010 21:20:29 -0000
@@ -146,17 +142,38 @@ function color_scheme_form($complete_for
+      }
+  	}
+  }

@@ -466,10 +485,23 @@ function _color_render_images($theme, &$
+    if (isset($gradient['direction']) && $gradient['direction'] == 'horizontal') {
+    	// Horizontal gradient.
+      for ($x = 0; $x < $gradient['dimension'][2]; $x++) {

Strange indentation here.

+++ themes/garland/color/preview.css	8 Feb 2010 21:20:32 -0000
@@ -6,10 +6,10 @@
 #preview, #preview #img {
...
-#preview #gradient {
+#preview #gradient-0 {

@@ -29,7 +29,7 @@
-#preview #gradient .gradient-line {
+#preview #gradient-0 .gradient-line {

Same as above mentioned for JS -- ids are unique.

Of course, to prevent name clashes, all of these ids should be namespaced with "color-" in reality, but that's a separate issue.

Powered by Dreditor.

> Double id selectors like this usually don't make sense. IDs are unique, by nature. :)

A selector of "#foo #bar" can make sense. It means "only select id bar when it happens to be inside id foo". In other words, only select #gradient-i when it is in a #preview.
That said, I've no idea if this is necessary -- if this jQuery is only being loaded when #gradient-i is in #preview anyway.

The patched version works quite well.

One thing is bugging me though: whatever pre-defined Color Set I choose, the dropdown always says "Custom" after saving.

Here is my complete color.inc file for debugging:

<?php
// $Id$
$info = array(
  // Available colors and color labels used in theme.
  'fields' => array(
    'base' => t('Header Sidebar'),
    'gradient-block' => t('Block with Gradient Background'),
    'header-bar' => t('Header Background Bar'),
    'text' => t('Copy Text'),
    'link' => t('Link'),
    'link-hover' => t('Link Hover'),
    'menu-active' => t('Menu Active'),
    'content-theader' => t('Header Content Area'),
    'block-featured-header' => t('Header Featured Block'),
    'block-featured-text' => t('Copy Text Featured Block'),
  ),
  // Pre-defined color schemes.
  'schemes' => array(
    'default' => array(
      'title' => t('Monoblue'),
      'colors' => array(
        'base' => '#266A9B',
        'gradient-block' => '#266A9B',
        'header-bar' => '#266A9B',
        'text' => '#6C9ACD',
        'link' => '#1D4C7E',
        'link-hover' => '#1D4C7E',
        'menu-active' => '#3D72CE',
        'content-header' => '#575757',
        'block-featured-header' => '#E3F1FF',
        'block-featured-text' => '#B6D7F2',
      ),
    ),
    'blackwhite' => array(
      'title' => t('Black and white'),
      'colors' => array(
        'base' => '#44444',
        'gradient-block' => '#444444',
        'header-bar' => '#444444',
        'text' => '#777777',
        'link' => '#0091e2',
        'link-hover' => '#1D4C7E',
        'menu-active' => '#0091e2',
        'content-header' => '#0091e2',
        'block-featured-header' => '#EEEEEE',
        'block-featured-text' => '#DDDDDD',
      ),
    ),
  ),
  // Images to copy over.
  'copy' => array(
    'logo.png',
    'screenshot.png',
  ),
  // CSS files (excluding @import) to rewrite with new color scheme.
  'css' => array(
    'css/style.css',
  ),
  //// Gradient definitions.
  'gradients' => array(
//    array(
//      // (x, y, width, height).
//      'dimension' => array(0, 0, 200, 20),
//      // Direction of gradient ('vertical' or 'horizontal').
//      'direction' => 'vertical',
//      // Keys of colors to use for the gradient.
//      'colors' => array('link', 'text'),
//    ),
  ),
  // Color areas to fill (x, y, width, height).
  'fill' => array(
    'gradient-block' => array(1201, 0, 10, 250),
    'base' => array(1201, 250, 10, 396),
  ),
  // Coordinates of all the theme slices (x, y, width, height)
  // with their filename as used in the stylesheet.
   'slices' => array(
  'images/navigation-background.png' => array(1201, 0, 10, 250),
  'images/bg-color-bar-low.jpg' => array(1201, 250, 10, 396),
  ),
  // Reference color used for blending. Matches the base.png's colors.
  'blend_target' => '#ffffff',
  // Preview files.
  'preview_image' => 'color/preview.png',
  'preview_css' => 'color/preview.css',
  // Base file for image generation.
  'base_image' => 'color/base.png',
);
?>

Attached patch fixes the problem eigentor noticed, and addresses sun's point in #61 about using variable_get just once. I didn't have time to get to the javascript, so I'm leaving this at "needs work."

@ksenzee: The patch you uploaded has 0 bytes.

Is there still a chance to get this in? It seems like it would be cool to make Color module more flexible. I didn't realize you were so limited in the number of colors you can pick.

If this issue can still make it into D7, is the current status that the patch from #35 needs to be updated to reflect sun's code review from #61, and eigentor's testing in #63?

#66 Yes, the patched color module is working fine (with minor issue pointed by eigentor), so after code style issues are resolved we could set status to RTBC.

It would be nice to see a follow-up patch that fixes (or completely removes) extremely garlandic color preview code.

Now! With more bytes! Wonder what happened to the upload.

With this patch, the current status is that we need to resolve sun's remaining concerns, and the ones I will find when I finish reviewing. :) I checked with webchick and it sounds like this can still go in. I'm setting to needs review for testbot.

ksenzee:

Thanks for the patch. I tested it in Garland, and all the default color schemes worked. I was also able to create a color scheme of my own.

Got to get on with the rest of my day, but hopefully, I'll be able to test with Bartik sometime this weekend (using patch from #749276: Alter Bartik to run with the modified color module (from #693504)).

Is there anything else I should test? I wish I could test what it actually fixes, but only the patch for Bartik actually is coded to support this.

Forgot to mention something important: when I first went to the Garland settings, it said I was in a Custom theme, even though it was the Default (Blue Lagoon). So there's some logic that needs to be fixed there.

I tested this patch with the patch I rolled on #749276: Alter Bartik to run with the modified color module (from #693504). Works great! (Can't remember if it had that same issue about starting out with Custom selected, though).

Just tested with Corolla: http://drupal.org/project/corolla & it works on there also. Too bad that Corolla probably won't make it into Drupal 7 core - it is a very clean-looking theme.

<offtopic>
@EvanDonovan if you know of any issues that would prevent Corolla from getting into core then please report them on project's issue queue. I will do my best to fix them as soon as possible.
</offtopic>

There are still problems -rtl stylesheets. I know this is discussed elsewhere but they seem to be inactive issues. The fix in the color module will only work for the garland theme. The fix says:

if there is a $stylesheet-rtl.css in the same directory as $stylesheet.css, then also write out a new $stylesheet-rtl.css file to have color mods applied.

That is, if I read the code right.

I believe this is ready now. Attaching an interdiff so it's easy to see changes from the last patch. I went through the JS and changed the absolute minimum of what I could stand to change - fixing variable declarations, basically.

Forgot to mention I also tested the HEAD-HEAD upgrade path and it's fine. I can't speak for the D6-D7 upgrade path, but if it worked before it still works. :)

Oops, forgot to name my interdiff such that the bot would leave it alone. Here's the same patch again for the bot's benefit.

I tried this out, and nothing exploded. That's always good.

I patched D7, and I patch Bartik with http://drupal.org/node/749276#comment-2800512.
Testing out changing colors in Bartik works just fine.

Patch applied, and worked correctly on Garland (i.e., no issues with the "Custom" text). It also worked correctly on Bartik with the patch I re-rolled on #749276: Alter Bartik to run with the modified color module (from #693504). Finally, it worked on Corolla, which already had been modified for it.

So, should be good to go!

webchick pointed out that there are no tests in this patch. After I forgave her for noticing, I whipped one up. Only difference between this and the last patch is the .test hunk.

Excellent. Great work on this, all!

Honestly, code-wise there's not really anything to complain about. A bunch of difficult to read code got comments and split from comma-separated lines of color to named arrays, etc. I did ask for the test because when doing this kind of refactoring you want to make sure you cover it. The new test is basically just a copy/paste of the other test so although there are some things I'd change about it (more comments, etc.) it would create inconsistency with the existing one.

Therefore........ Committed to HEAD! :D

Hope I come back from Vancouver with lots of lovely Color module-enabled core themes. ;)

Huge thanks to ksenzee, webchick and all who helped on this issue. Time to take myself over to #749276: Alter Bartik to run with the modified color module (from #693504) & related issues :)

Is there any docs about changes?

Hmmm.

+++ modules/color/color.js
@@ -24,11 +25,23 @@ Drupal.behaviors.color = {
+      for (j = 0; j < (settings.gradients[i]['direction'] == 'vertical' ? height[i] : width[i]); ++j) {

Missing var for 'j' ?

+++ modules/color/color.js
@@ -56,29 +70,31 @@ Drupal.behaviors.color = {
+      for (i in settings.gradients) {
...
+          for (j in color_start) {
...
+            for (j in accum) {

Missing var for i and j here (separate function).

+++ modules/color/color.js
@@ -56,29 +70,31 @@ Drupal.behaviors.color = {
+            delta[j] = (color_end[j] - color_start[j]) / (settings.gradients[i]['vertical'] ? height[i] : width[i]);

I still don't see height being defined in the preview() function.

+++ modules/color/color.js
@@ -132,7 +148,8 @@ Drupal.behaviors.color = {
+    function callback(input, color, propagate, colorScheme) {

@@ -140,20 +157,20 @@ Drupal.behaviors.color = {
+      if ($(input).val() && $(input).val() != color) {
+        $(input).val(color);

callback() still gets a jQuery object for input. input should therefore be renamed to $input, so as to prevent needless $() calls.

Powered by Dreditor.

As I want this feature in D6, I tried adapting the last patch. Garland's presets seem to work fine upon save, but the js is broken (the hex fields don't update and the preview doesn't work). I'm attaching the patch if anyone wants to have a look.

Sorry, didn't mean to change the status.

Got it working for D6 (although it doesn't take into account comments in #85). Thanks for your work ksenzee!

Suppose better to make color2 contrib module to backport this changes and provide a upgrade path if this is needed

EDIT: maybe integrate this changes with http://drupal.org/project/color_soc08

EDIT2: issue #761322: Need for this in D7 now & should this be refactored to reflect D7 version?

@whoviz @andypost:

Excellent idea. color_soc08 is kind of an awkward name for a project, though, in my opinion.

Can further discussion of that happen in a separate issue? Let's keep this one focused on any cleanup that needs to be done after the D7 patch having been committed.

I have filed #761322: Need for this in D7 now & should this be refactored to reflect D7 version? against color_soc08. Perhaps, whoviz, you could post your D6 patch in there, so we could keep this issue for the D7 followups.

I have just added another theme that uses patched color module.

This is not the right place to discuss general JS cleanup for color.module such as sun is suggesting in #85. JS does not have block scope, so it's incorrect to redefine in an inner function a variable such as j that has already been defined in the outer function. Ideally you wouldn't reuse such a counter variable in an inner function. Even more ideally, you wouldn't create unneeded closures, which this module does with abandon (as indeed does much of Drupal's JS, but color.module is particularly fond of them). But like I said, that's a separate issue, and one I would happily review.

That's true, to some extent. Wasn't aware of that messy code, sorry. But anyway, this patch introduced a global variable.

That j has been global since http://drupal.org/node/88202#comment-455697, which is egregious enough of an error that it's worth fixing here even if it is the wrong issue. Looking forward to that color.js cleanup patch, hint hint. :)

So looks like sun's patch fixes the scope of the j, which is good. ksenzee: I never saw the issue that put Garland in core before, so thanks for that. I hadn't even heard of Drupal at that time :)

Committed to CVS HEAD. Thanks.

The patches appear to have introduced a new bug: #769922: Color module never loads styles directly from theme directory
I guess this is not intended. Don't know if it is Sun's or ksenzees patch or if those simply don't work with D7 alpha 3.

Just posting any new color module issues here, as this place may serve as a meta issue: #772106: Color Module: Impossible to use a defined color twice in filling for background images
Not sure if this is an old bug that has always been there, or if it is new.

The D6 patch no longer works again 6.22, so this is the exact same patch re-factored to work against that, but including the changed from #85 (I really don't know what I'm doing re: $(input) part)

How do we get this committed to Drupal 6 HEAD? Having to patch it all the time to get things to stay working is a PITA. We use a number of fully color supported themes for ease of management.

I am still getting an error with farbtastic.js when swapping between presets in either Garland or my own theme. This seems to be new for 6.22 as it worked fine in 6.20.

rgb is undefined
[Break On This Error] var r = rgb[0], g = rgb[1], b = rgb[2];
farbtastic.js?T (line 285)

Perhaps I should open this up as a D6 color.module dedicated issue.

Fixing version/status.

Does this ever got into core? I am trying to use custom colors in D7 and no luck? Is it working for D6 out of the box, or not?

It's in Drupal 7, but not in 6.... Unsubscribe.

I've made a patch for Drupal 7 to accept and work with multiple colors.

You can have multiple colors already in Bartik and other supported themes in 7.x...Unsubscribe.

Maybe, but I was working with a Zen subtheme.

err .. this functionality has been in D7 for a while now. Currently trying to get those changed fixed up enough to be back ported into D6 which is what the patch in http://drupal.org/node/693504#comment-4970060 is for. I've got it most of the way except for the javascript being a little bung, and I do not know how to fix that.

There's no need to patch core to get this to work with a zen sub-theme. I have it working without just fine.

What exactly does your patch do as it seems to be screwing the 'base' colour? Perhaps you should open it as a separate issue as this is specifically dealing with the limitations of the hard coded colours in the color.module.

Returning this to 'needs work' since that's what it needs. Still no idea what the patch in #106 does except for breaking base colour vectors.

Any javascript gurus out there? Javascript debugging isn't even a suit I wear.

Maybe I wasn't very explicit in my problem. I had to make color module change just CSS colors. I didn't need overlays and so on. Just defining other colors, ~ 15, and have the ability to change them from the admin panel. With the default color module, it displayed only 5 colors, or in some cases, displayed them all, but didn't changed the css color only on the first 5. With my patch, I can do that without a problem.

Just to clarify what's happening in this thread: When creating Bartik for Drupal 7, I wanted to be able to implement more colors (with their own labels) using color module. In Drupal 6 (and 5) there's a limit of 5 colors, and the labels are hardcoded. We accomplished that goal — and Drupal 7 allows any theme to define colors that will be changeable with color module, as many as you want with whatever labels you want. It's done.

It looks like people have recently requested that this change to color module be back ported to Drupal 6. I highly doubt that will happen. In fact, I could perhaps say there is no way this is going to happen. Drupal 6 APIs are locked and have been for years. This kind of change is not allowed.

In fact, we were lucky to be allowed to change this for Drupal 7 almost two years ago, because even in early 2010, we were very late in the D7 development cycle. Steven Merrill (who did much of this work) had lots of ideas about how to seriously revise color module while he was in the code looking around, but we did not even try to implement those ideas for Drupal 7 because it was too late to make heavy alterations. Those kinds of ideas can be implemented in Drupal 8 (and not back ported to Drupal 7).

Any discussion about ideas of how to alter colors in a Drupal 6 theme should happen in other discussion threads. Perhaps you can make a contrib module that people can download and use with their Drupal 6 website. Or if you need this for D6, you'll have to just keep patching your copy of D6. The official version will be staying as is.