Color.module changes CSS IDs when switching color scheme

#bac is a legal css hex string. The color.module substitutes any #xxx string if xxx is a hex string it thinks it need to substitute. I have to make all my css id strings not begin with hex string to stop this. for example, if use #_background will solve the problem.

Definitely a bug - I just wasted some time on this too. Color.module's regexp needs to look for a # and three or six characters followed by a space.

Flagging for 7 - anyone know if this is still an issue?

Something like this?

// Find all colors in the stylesheet and the chunks in between.
$style = preg_split('/(#[0-9a-f]{6}|#[0-9a-f]{3})/i', $style, -1, PREG_SPLIT_DELIM_CAPTURE);

TO

// Find all colors in the stylesheet and the chunks in between.
$style = preg_split('/(#([0-9a-f]{6}|[0-9a-f]{3})[^0-9a-zA-Z])/i', $style, -1, PREG_SPLIT_DELIM_CAPTURE);

@aspilicious: the new regex should also take into account that hyphens and underscores are valid in CSS identifiers. So the regex you propose, while an improvement, could still rename "#bac-foo" to something else.

Moving.

I agree with @seanr's in #3. The regex needs to only match against # and three or six characters followed by a space.

It may not alway be a space (i.e. #fff;). What about this?

/(#[0-9a-f]{6}|#[0-9a-f]{3})\b/i

\b being word boundary. That still doesn't get us around the hyphen issue though.

Working with this.

Updated the issue summary to match the current state of the issue.

@iflista please feel free to re-assign yourself if you'd like.

I created a patch based on #10. Not perfect as mentioned, but it's definitely an improvement.

Patch looks good. We need to manually test that this fixes the issue though.

I've tweaked it a little bit.

With the patch from #14, the css id's with a dash or a direct colon after it are still captured while they shouldn't be:
http://rubular.com/r/QASUuvVh8m

I've extended the regex to check for one or more spaces and 1 semicolon.
My result: http://rubular.com/r/X4QupGFn4Z

As a result of adding the semicolon to the regex it (the semicolon) is not spitted out as a chunk anymore.
Therefor I've added the

$chunk .= ';';

on line 443.

To review it manually:
Add this to the top of bartik's style.css:

#background: #bacbac;
#bac-ground: #bac;
#bac: #bac;
#bac: #bac ;
#BACKGROUND : #BACBBC;
#BAC-ground : #BACcbc ;

Add some styles to the different schemes of color.inc:

...
  // Pre-defined color schemes.
  'schemes' => array(
    'default' => array(
      'title' => t('Blue Lagoon (default)'),
      'colors' => array(
        'test' => '#bacbac',
        'test2' => '#bac',
        'top' => '#0779bf',
...
    'firehouse' => array(
      'title' => t('Firehouse'),
      'colors' => array(
        'test' => '#123456',
        'test2' => '#123',
        'top' => '#cd2d2d',

Start switching presets and check /sites/default/files/color/bartik-foobar/color.css.

The one downside that I can think of about the approach in #16 is that a semicolon is not strictly required in CSS if it's the last property in a rule. For example, this is valid CSS:

#something {
  font-weight: bold;
  color: #000000
}

There's no semicolon after #000000 but it's still valid since it's the last rule, and the patch in #16 wouldn't catch that since it requires a semicolon be present. I'm not sure if this is really something we should worry about though, since I almost never see this in practice.

Thoughts?

@mcrittenden: You're right.
I came up with:
/(#[0-9a-f]{6}\b|#[0-9a-f]{3}\b)\s*;*$/im
basically checking on an end-of-line.

but that fails on
#bac
{
bac-ground: #bac;
}

http://rubular.com/r/T7NeUVveZw

Patch attached, not sure if my regex-fu goes any further than this :)

+++ b/core/modules/color/color.module
@@ -423,7 +423,7 @@ function _color_rewrite_stylesheet($theme, &$info, &$paths, $palette, $style) {
   // Find all colors in the stylesheet and the chunks in between.
-  $style = preg_split('/(#[0-9a-f]{6}|#[0-9a-f]{3})/i', $style, -1, PREG_SPLIT_DELIM_CAPTURE);
+  $style = preg_split('/(#[0-9a-f]{6}\b|#[0-9a-f]{3}\b)\s*;{1}/i', $style, -1, PREG_SPLIT_DELIM_CAPTURE);

I may not be getting the context of the function right now, but are we splitting these into "chunks" for any particular reason? Or are we just splitting them into chunks to try and find all the the hex values in the style sheet? It seems to me that this is just the wrong regex/function to begin with. Why iterate over each chunk when you can find them all at once?

Could we just not change this to preg_match_all and just find the hex values? Maybe something like:

$matches = array();
preg_match_all('/{[^#]*(#[0-9a-f]{6}|#[0-9a-f]{3})[^\n;\s]*/ims', $style, $matches, PREG_SET_ORDER);

Just a thought.

edit: note this regex only matches hexes inside curly brackets (to completely avoid #ids).

Oops, forgot to upload a screenshot of my regex matches (note this has the "ims" pattern modifiers enabled):

The stylesheet is splitted into chunks because the css identifiers are also analyzed (to see if it is a link, text, or something else).
Link and text are special properties, and when they are detected in the chunk that is not a color, their value gets colorshifted.
see _color_shift().

My last attempt:
/\:\s*(#[0-9a-f]{6}\b|#[0-9a-f]{3}\b)/i
checking for a colon before the start of the color.
Worked pretty good for me!
http://rubular.com/r/p4E771NmFb

As the colon is now part of the capture, it needs to be added to the color chunk.

Fair enough, this fixes the immediate issue. I still don't agree that this is really the right approach overall (for figuring out the right colors), but that is part of a bigger refactoring issue anyway. I'll just have to live with how bad color is until 9.x.

@thijsvdanker, could you update the issue summary with your steps to reproduce. Someone someone else, please manually test. Provide screenshots and upload the modified bartik source and the rewritten color files. Thanks!

The previous patch stripped of the space between the colon and the color, which is expected in the test.
This patch should make the test pass again.
I'll start updating the issue summary.

This patch can be used to modify Bartik to have some css that would fail before the patch.
Can be used to manually test the patch.

thijsvdanker: Changed the issue summary up to comment #11

Updated the issue summary.

The patch in #25 is just to be used for manual testing.
The real patch in #24 passes the test.

I manually tested this. Everything looks good, but noticed that the gradients didn't get changed. I'm still convinced we should just try a different approach (like the one I suggested in #19) instead of trying to work inside of "chunks". Perhaps we should preg_match_all/preg_replace the color shifted things like links/text that match certain criteria first? Idk, just seems that we're trying to fix a broken system.

I'm going to try and step through this to see how this is actually re-writing things.

Here is an 8.x version using preg_replace_callback and closures (which is far more powerful and easier to understand). This still does "chunks" but it is geared more around each selector group (between {}). @thijsvdanker, if you want we could still use the simpler regex for 7.x (if you can figure out the gradients).

Also attached is the before/after bartik CSS stylesheets.

Updated issue summary to reflect state and manual testing.

Reroll attached.

Per #2361823: Remove usage of drupal_strtolower() changed drupal_strtolower to Unicode::strtolower

Re-uploading for the bots.

Color has been removed from core, #3270899: Remove Color module from core.