if default color profile base and original style.css background color are the same, output is invalid CSS background:;
mikesmullin - April 6, 2007 - 20:56
| Project: | Drupal |
| Version: | 6.10 |
| Component: | color.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Description
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;).
#1
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.
#2
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.
#3
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.
#4
And for d6 head. Haven't tested this one but color.module wasn't changed much.
#5
mikesmullin or senpai: can you confirm that this patch fixes your problem? Thanks.
#6
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.
#7
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.
#8
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.
#9
@Dries: The patch does solve the problem.
#10
#11
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?
#12
Offending line is 374 in drupal-6.10/modules/color/color.module -- please remove!
#13
just spent a few hours figuring out this same issue for myself and can confirm that removing the line fixes the problem.