if $default[$base] (default color profile base color) and $chunk (e.g. original style.css background color) are the same, the style.css in /files/color contains invalid CSS (e.g. background:;).

Steps to replicate:

  • Add background:#ff0000; to the original style.css
  • Set the default profile base color to #ff0000 in color/color.ini
  • Set a base color to anything (e.g. #0000ff) in admin/build/themes/settings/yourtheme and save (to regenerate /files/color)
  • Check the style.css in /files/color... see the background;: in the /files/color/*/styles.css?

Obviously, we need valid CSS. This has to be a bug... after some investigation I determined the cause is line 275 of color.module which states:

unset($conversion['base']);

Anyone remember the purpose of this line? It's not commented...

This sets it up for the failure on lines 301-303 where the 'base' key is not matched and therefore there is no new color to output.

Temporary Fix:

By commenting-out line 275 I was able to get the desired behavior--which is the output of the exact color specified in admin/build/themes/settings/yourtheme (e.g. background:#0000ff;).

CommentFileSizeAuthor
#29 drupal-6_color_base-D6.patch596 bytesdboulet
#27 drupal-6_color_base_3.patch627 bytesmarkabur
#24 drupal-6_color_base_2.patch444 bytesmarkabur
#20 drupal-6_color_base.patch597 bytesdboulet
#16 drupal-HEAD_color_base.patch611 bytesdboulet
#4 base_color-head.patch585 bytesdvessel
#3 base_color-5.x.patch597 bytesdvessel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dvessel’s picture

dvessel CreditAttribution: dvessel commented

I ran into the same behavior just now. Spent 3 hours trying to figure it out. :\

All I did was change the base color slightly making the difference almost imperceptible.

Senpai’s picture

Senpai CreditAttribution: Senpai commented

I ran into this also, and spent a fair bit of time experimenting, only to find out by trial and error that you need to delete some rules in the original style.css if you wanna go messing about with the generated style.css file and have any amount of success.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Active » Needs review
FileSize
base_color-5.x.patch597 bytes

Okay, tried deleting the line mentioned by mikesmullin. Tested on my theme and Garland. Both seem to work fine without unsetting the base.

Here's a patch to remove it.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Version: 5.1 » 6.x-dev
FileSize
base_color-head.patch585 bytes

And for d6 head. Haven't tested this one but color.module wasn't changed much.

Dries’s picture

Dries CreditAttribution: Dries commented

mikesmullin or senpai: can you confirm that this patch fixes your problem? Thanks.

Steven’s picture

Steven CreditAttribution: Steven commented

IIRC, the reason the base color is unset in $conversion is to handle the case where another color field in the palette is the same as the base color. In Garland, this worked, because the base color is just a global tint, and only derivatives of it appear in the style.css. The array_search() breaks this behaviour though, since it searches $default, not $conversion. It probably got undone in later development tweaks.

If people can live with the fact that each color in the palette must be unique (but they can differ by only +1/-1), that line can be removed.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Needs review » Postponed (maintainer needs more info)

So what should be done here? Blanking out the background color is definitely a bug but what would be the other behavior if the base color is the same as one of the other colors _without_ the base color being unset?

It's hard for me to tell since it's not 100% clear to me how colors are targeted in the style sheet.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Postponed (maintainer needs more info) » Active

Just wanted to note that if you can use the shorthand colors in the style sheet to match the base it won't get cleared.

ms2011’s picture

ms2011 CreditAttribution: ms2011 commented

@Dries: The patch does solve the problem.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Active » Needs review
zany’s picture

zany CreditAttribution: zany commented

I doesn't look like the original behaviour has any effect. $conversion is only read from ($chunk = $conversion[$key]). It won't do any good to delete 'base' from that array -- no matter if it is the same color as any other color or not.

If somehow the lookup of 'base' as a color should be prevented it needs to be deleted from $default.

The patch still works (given some fuzziness on the line numbers). Can this be rolled into the next release?

zany’s picture

zany CreditAttribution: zany commented
Version: 6.x-dev » 6.10

Offending line is 374 in drupal-6.10/modules/color/color.module -- please remove!

markabur’s picture

markabur CreditAttribution: markabur commented

just spent a few hours figuring out this same issue for myself and can confirm that removing the line fixes the problem.

freelock’s picture

freelock
Location Seattle
CreditAttribution: freelock commented

After 3 years, this bug hasn't been addressed?

Came to the same conclusion as the others in this thread, that line 374 needs to be removed. This is plainly a bug -- if the base color is in the underlying stylesheet at all, the result is invalid CSS. I don't understand why you would not want to rewrite the base color in the stylesheet -- it seems like the whole point of the color module. But if leaving the base color unchanged is the goal, then surely the replacement array should not have the base color in it, so that at least you don't end up with broken CSS!

Status: Needs review » Needs work

The last submitted patch, base_color-head.patch, failed testing.

dboulet’s picture

dboulet CreditAttribution: dboulet commented
Version: 6.10 » 7.x-dev
Status: Needs work » Needs review
FileSize
drupal-HEAD_color_base.patch611 bytes

I actually started using the color module for the first time yesterday, and ran into this bug. Looks like this bug still exists in Drupal 7, here's a patch.

markabur’s picture

markabur CreditAttribution: markabur commented

This change works for me in d6. It lets color module affect the base color in my custom theme. Could be RTBC, but I haven't tested it in d7 or with a core color-enabled theme such as Garland.

markabur’s picture

markabur CreditAttribution: markabur commented
Status: Needs review » Reviewed & tested by the community

Ok, checked Garland in d7 HEAD and it still changes colors just fine with the patch applied. I don't have a custom color-enabled theme in d7 to test with, but I can confirm that this specific change is needed in d6 in order to let color module affect the base color of my theme. I have been working with a color-enabled zen-2.x subtheme.

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 7.x-dev » 6.x-dev

As far as I can tell, this is harmless bugfix. I've committed it to CVS HEAD, and I'm moving it to D6.

dboulet’s picture

dboulet CreditAttribution: dboulet commented
FileSize
drupal-6_color_base.patch597 bytes

Thanks Dries, here's a patch for Drupal 6.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-6_color_base.patch, failed testing.

markabur’s picture

markabur CreditAttribution: markabur commented
Status: Needs work » Needs review

#20: drupal-6_color_base.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-6_color_base.patch, failed testing.

markabur’s picture

markabur CreditAttribution: markabur commented
FileSize
drupal-6_color_base_2.patch444 bytes

Here's a reroll.

markabur’s picture

markabur CreditAttribution: markabur commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-6_color_base_2.patch, failed testing.

markabur’s picture

markabur CreditAttribution: markabur commented
Status: Needs work » Needs review
FileSize
drupal-6_color_base_3.patch627 bytes

Ok, let's try it this way.

Status: Needs review » Needs work

The last submitted patch, drupal-6_color_base_3.patch, failed testing.

dboulet’s picture

dboulet CreditAttribution: dboulet commented
Status: Needs work » Needs review
FileSize
drupal-6_color_base-D6.patch596 bytes

Same patch, but renamed to be ignored by testbot.

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.