Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Color Module cannot save colors if uppercase hex codes used in theme's color.inc #6057

Closed
stpaultim opened this issue Apr 12, 2023 · 14 comments · Fixed by backdrop/backdrop#4431

Comments

@stpaultim
Copy link
Member

stpaultim commented Apr 12, 2023

Creating a separate issue for this comment raised here: #1905

Description of the bug

Color Module is intolerant of upper case hex values.

I used uppercase hex values in my css file, only to learn (after three hours of head-scratching and hair-pulling) that the Color Module is intolerant of upper case letters. It reads #45C690 as #000000, and proceeds from there. So you basically get kind of a dark mode thing going for all your color configurations. Once I replaced my biggie letters with smallies, everything worked great.

The issue shows up when you include upper case hex colors in your css file, and the Backdrop Color Module tries to process it (doing color substitutions). It croaks on upper case text.

This issue also discussed here: https://forum.backdropcms.org/forum/color-module-cant-handle-uppercase-hex

@stpaultim
Copy link
Member Author

@ericfoy If you are able to provide specific steps to reproduce this problem, that would be helpful.

@ericfoy
Copy link

ericfoy commented Apr 14, 2023

Through the process of debugging my issue, I put a comment header in my "colors.css" file (the only file I set to be processed by the color module) to help me discover what substitutions were being made by the color module code. Note that I simply repeated the colors, omitting the # in front of one column. My thinking was:

  • the color module would still process the colors, even though they're embedded in comments (I was right on that), and
  • the color module would simply ignore the six-digits of the ones missing the # (seemed obvious).
  • this would also make it very easy for me to iteratively discover the hex codes of the colors that look pretty when I later was going to want to load in my preset color schemes (it does!).

So here are the resulting outputs of two versions; one with upper case hex values, the other with lower case. These are snapshots of the system-generated colors.css file found in the files/color/[my_theme] folder:

/*      --default colors--
    Page Text           090909    #262431
    Link Text           093579    #044f40
    Block Borders       0a0000    #ebebe1
    Page Background     ffffff    #ebebe1
    Header Background   bcc8c8    #ebebe1
    Header Text         0f0f0e    #fffff5
    Footer Background   bcc8c9    #78aa82
    Footer Text         0f0e0f    #262e31
    Menu Background     c8d7d2    #78aa82
    Menu Text           0e0f0f    #0e0f0f
    Menu Highlight      d2e1dc    #73b45c
    Table Rows          e6e6e6    #bedcc5
    Active Sort Column  dcdcdc    #c4ecce
*/    

And here is the failure case:

/*      --default colors--
    Page Text           090909    #262431
    Link Text           093579    #044f40
    Block Borders       0A0000    #000000
    Page Background     FFFFFF    #000000
    Header Background   BCC8C8    #000000
    Header Text         0F0F0E    #000000
    Footer Background   BCC8C9    #000000
    Footer Text         0F0E0F    #000000
    Menu Background     C8D7D2    #000000
    Menu Text           0E0F0F    #000000
    Menu Highlight      D2E1DC    #000000
    Table Rows          E6E6E6    #000000
    Active Sort Column  DCDCDC    #000000
*/    

Note that these are both output files. In the original unprocessed css file both columns are the same (except for the missing # in the first column. This allows me to see what each color was substituted with after processing, making it easy for me to fill out the colors of my stock schemes in color.inc
Notice that everything seems to work fine on the first two colors, but then disaster strikes when it encounters the upper case A.

@klonos
Copy link
Member

klonos commented Apr 14, 2023

Thanks @ericfoy 🙏🏼 ...I could spend some time to whip up a minimal test custom theme that implements color support, and then start working with that, but it would greatly help if you reduced the theme you are currently working with down to basics and uploaded a copy of that here. That would save me (or anyone else willing to work on a fix for this bug) some time/trouble. So I will try to start working on it, but would be awesome if you beat me to it and uploaded a sample theme that I can test/troubleshoot with.

@klonos klonos self-assigned this Apr 14, 2023
@ericfoy
Copy link

ericfoy commented Apr 14, 2023

I think it's easier than that...

  1. Install the Modoc theme (probably any theme with color module support would work, but in case it's something peculiar to Modoc... might as well use it).
  2. Edit themes/modoc/css/colors.css, replacing some or all the letters in the color values with upper case.
  3. Empty the css cache.
  4. Navigate to Appearance | Modoc Settings, change some colors, hit Save.

Look now at the replacement colors.css file found in the files/color/modoc--- folder. In there, you can see the replacement values as described in my earlier comment.

@ericfoy
Copy link

ericfoy commented Apr 29, 2023

While perusing color.module, I ran across line 467
return preg_match('/^#([a-f0-9]{3}){1,2}$/iD', $color);

Seems like it should look more like this:
return preg_match('/^#([a-fA-F0-9]{3}){1,2}$/iD', $color);

When I get some time, I'll play around a bit with this...

@kiamlaluno
Copy link
Member

kiamlaluno commented Apr 29, 2023

The used pattern is case-insensitive. That is what the i at the end the pattern means ('/^#([a-f0-9]{3}){1,2}$/iD').

The complete list of modifiers is given on Pattern Modifiers.

@ericfoy
Copy link

ericfoy commented Apr 29, 2023

Ohhhhhh Oh well.
Still learning...

@ericfoy
Copy link

ericfoy commented May 4, 2023

Turns out, I had errantly diagnosed the actual problem. The color module handles upper case hex values in the CSS just fine. The problem arises when upper case is used in the color.inc file by the theme designer. I could give excuses for why I mis-reported this issue, but I won't waste anybody's time. Sorry about that.

In any event, I find that the following change seems (upon initial impression--more testing and verification is in order) to fix the problem...

image

I'll do a little more testing and verification, and then submit a PR...

@ericfoy
Copy link

ericfoy commented May 5, 2023

Here is a lengthy analysis:

Test Case 1: Everything lower case. Normal setup

In the color.inc file, we have the following excerpt, which defines the default color scheme (Note that all color hex values are in lower case only):

  // Pre-defined color schemes.
  'schemes' => array(
    'default' => array(
      'title' => t('Default'),
      'colors' => array(
        'text' => '#0f0f0f',
        'link' => '#093579',
        'border' => '#000000',
        'pagebg' => '#ffffff',
        'headerbg' => '#bcc8c8',
        'headertxt' => '#0f0f0e',
        'footerbg' => '#bcc8c9',
        'footertxt' => '#0f0e0f',
        'menubg' => '#c8d7d2',
        'menutxt' => '#0e0f0f',
        'menuhl' => '#d2e1dc',
        'tablerow' => '#e6e6e6',
        'sortcol' => '#dcdcdc',
      ),

The following excerpt is in the header of 'colors.css' (the only css file configured for the color module to scan and process.
It is designed (all inside a comment block) so that the first column will show the converted colors in their original form
even after processing, since they have no '#' in front, and are thus not considered colors to be processed. On the other hand,
the second column should be processed by the color module, and converted to the colors chosen in the saved scheme.

/*** colors.css ***/
 /*              -default color-  -substitute- 
 ------------------------------------------
    Page Text           0f0f0f    #0f0f0f
    Link Text           093579    #093579
    Block Borders       000000    #000000
    Page Background     ffffff    #ffffff
    Header Background   bcc8c8    #bcc8c8
    Header Text         0f0f0e    #0f0f0e
    Footer Background   bcc8c9    #bcc8c9
    Footer Text         0f0e0f    #0f0e0f
    Menu Background     c8d7d2    #c8d7d2
    Menu Text           0e0f0f    #0e0f0f
    Menu Highlight      d2e1dc    #d2e1dc
    Table Rows          e6e6e6    #e6e6e6
    Active Sort Column  dcdcdc    #dcdcdc
*/    

After selecting a different color scheme via the UI at admin/appearance/settings/modoc,
we find the following in the header of the alternate colors.css file located in

files/color/modoc-fce85949

/*** colors.css  ***/
/*              -default color-  -substitute- 
 ------------------------------------------
    Page Text           0f0f0f    #1d1b25
    Link Text           093579    #044f40
    Block Borders       000000    #375c37
    Page Background     ffffff    #f1f2ec
    Header Background   bcc8c8    #375c37
    Header Text         0f0f0e    #0f0f0e
    Footer Background   bcc8c9    #375c37
    Footer Text         0f0e0f    #0f0e0f
    Menu Background     c8d7d2    #8dbc99
    Menu Text           0e0f0f    #0e0f0f
    Menu Highlight      d2e1dc    #76f268
    Table Rows          e6e6e6    #d0ddd0
    Active Sort Column  dcdcdc    #cbe4d3
*/    

Note here that all of the original color codes remain in the first column, and that their replacement
values are respectively shown in the second column (having been replaced). This is a nice indication that color substitution
is being successfully carried out.

Test Case 2: Introduction of upper case hex values into the CSS file:

Original file:

/*** colors.css  ***/
/*              -default color-  -substitute-
 ------------------------------------------
    Page Text           0f0f0f    #0f0f0f
    Link Text           093579    #093579
    Block Borders       000000    #000000
    Page Background     ffffff    #ffffff
    Header Background   bcc8c8    #bcc8c8
    Header Text         0F0F0E    #0F0F0E
    Footer Background   BCC8C9    #BCC8C9
    Footer Text         0f0e0f    #0f0e0f
    Menu Background     C8D7D2    #C8D7D2
    Menu Text           0E0F0F    #0E0F0F
    Menu Highlight      D2E1DC    #D2E1DC
    Table Rows          e6e6e6    #e6e6e6
    Active Sort Column  dcdcdc    #dcdcdc
*/    

Output file:

/*** colors.css  ***/
/*              -default color-  -substitute-
 ------------------------------------------
    Page Text           0f0f0f    #1d1b25
    Link Text           093579    #044f40
    Block Borders       000000    #375c37
    Page Background     ffffff    #f1f2ec
    Header Background   bcc8c8    #375c37
    Header Text         0F0F0E    #0f0f0e
    Footer Background   BCC8C9    #375c37
    Footer Text         0f0e0f    #0f0e0f
    Menu Background     C8D7D2    #8dbc99
    Menu Text           0E0F0F    #0e0f0f
    Menu Highlight      D2E1DC    #76f268
    Table Rows          e6e6e6    #d0ddd0
    Active Sort Column  dcdcdc    #cbe4d3
*/    

Here we see that the color module has had no problems dealing with upper case or mixed-case hex color values, and that all the appropriate substitutions have been made.


Test case 3: Introduction of upper case hex values into color.inc

excerpt from color.inc

  // Pre-defined color schemes.
  'schemes' => array(
    'default' => array(
      'title' => t('Default'),
      'colors' => array(
        'text' => '#0f0f0f',
        'link' => '#093579',
        'border' => '#000000',
        'pagebg' => '#ffffff',
        'headerbg' => '#bcc8C8',
        'headertxt' => '#0F0F0E',
        'footerbg' => '#BCC8C9',
        'footertxt' => '#0f0e0f',
        'menubg' => '#C8D7D2',
        'menutxt' => '#0E0F0F',
        'menuhl' => '#D2E1DC',
        'tablerow' => '#e6e6e6',
        'sortcol' => '#dcdcdc',
     ),

Note the existence of upper case letters in the array definition above.

Now, we have Test sub-case 3a, in which the CSS file duplicates the upper case propensities of the color.inc array definition...

Original file:

/*** colors.css  ***/
/*              -default color-  -substitute-
 ------------------------------------------
    Page Text           0f0f0f    #0f0f0f
    Link Text           093579    #093579
    Block Borders       000000    #000000
    Page Background     ffffff    #ffffff
    Header Background   bcc8c8    #bcc8c8
    Header Text         0F0F0E    #0F0F0E
    Footer Background   BCC8C9    #BCC8C9
    Footer Text         0f0e0f    #0f0e0f
    Menu Background     C8D7D2    #C8D7D2
    Menu Text           0E0F0F    #0E0F0F
    Menu Highlight      D2E1DC    #D2E1DC
    Table Rows          e6e6e6    #e6e6e6
    Active Sort Column  dcdcdc    #dcdcdc
*/    

Output file:

/*** colors.css  ***/
/*              -default color-  -substitute-
 ------------------------------------------
    Page Text           0f0f0f    #1d1b25
    Link Text           093579    #044f40
    Block Borders       000000    #375c37
    Page Background     ffffff    #f1f2ec
    Header Background   bcc8c8    #000000
    Header Text         0F0F0E    #000000
    Footer Background   BCC8C9    #000000
    Footer Text         0f0e0f    #0f0e0f
    Menu Background     C8D7D2    #000000
    Menu Text           0E0F0F    #000000
    Menu Highlight      D2E1DC    #000000
    Table Rows          e6e6e6    #d0ddd0
    Active Sort Column  dcdcdc    #cbe4d3
*/    

Notice all the zeros. This is a failure case.

Likewise, for Test case 3b, in which the CSS file contains only lower case hex values, we get the same failure:

Original file:

/*** colors.css  ***/
/*              -default color-  -substitute-
 ------------------------------------------
    Page Text           0f0f0f    #0f0f0f
    Link Text           093579    #093579
    Block Borders       000000    #000000
    Page Background     ffffff    #ffffff
    Header Background   bcc8c8    #bcc8c8
    Header Text         0f0f0e    #0f0f0e
    Footer Background   bcc8c9    #bcc8c9
    Footer Text         0f0e0f    #0f0e0f
    Menu Background     c8d7d2    #c8d7d2
    Menu Text           0e0f0f    #0e0f0f
    Menu Highlight      d2e1dc    #d2e1dc
    Table Rows          e6e6e6    #e6e6e6
    Active Sort Column  dcdcdc    #dcdcdc
*/    

Output file:

/*** colors.css  ***/
/*              -default color-  -substitute-
 ------------------------------------------
    Page Text           0f0f0f    #1d1b25
    Link Text           093579    #044f40
    Block Borders       000000    #375c37
    Page Background     ffffff    #f1f2ec
    Header Background   bcc8c8    #000000
    Header Text         0f0f0e    #000000
    Footer Background   bcc8c9    #000000
    Footer Text         0f0e0f    #0f0e0f
    Menu Background     c8d7d2    #000000
    Menu Text           0e0f0f    #000000
    Menu Highlight      d2e1dc    #000000
    Table Rows          e6e6e6    #d0ddd0
    Active Sort Column  dcdcdc    #cbe4d3
*/    

So we conclude that the problem lies in the fact that the color.inc file (the array definition) contains upper case hex values.

I will introduce Test case 4 in the next comment: the case where the predefined schemes (other than the default) contain upper case... This will determine whether further modification of the code is required...

@ericfoy
Copy link

ericfoy commented May 5, 2023

Test case 4: Predefined schemes (other than the default) contain upper case.

It appears that this case causes no problems whether before or after my patch has been applied. Although I could spend untold amounts of time figuring out why this is the case, I think I'd rather simply be satisfied that everything seems to work.

—Eric

@klonos
Copy link
Member

klonos commented May 6, 2023

@ericfoy thank you for providing a PR and detailed explanation for this 🙏🏼

@backdrop/core-committers can one of you please approve the PR so that the tests can run? Thanks.

@klonos
Copy link
Member

klonos commented May 6, 2023

...I've left some comments in that PR.

@ericfoy
Copy link

ericfoy commented Jul 11, 2023

New PR...
backdrop/backdrop#4431

@quicksketch quicksketch changed the title Color Module is intolerant of upper case hex values Color Module cannot save colors if uppercase hex codes used in theme's color.inc Jul 14, 2023
@quicksketch
Copy link
Member

quicksketch commented Jul 14, 2023

Thanks @ericfoy! I tested this manually and confirmed it fixes the saving (and loading) of color values if a theme uses uppercase letters in it's color.inc file. Usually I would request automated test coverage for a fix like this, but as we have no test theme in core that supports color palettes (all testing is done on Basis and Bartik), adding tests seemed like a big ask. Thanks for your extensive investigation getting to the bottom of this problem!

Merged into 1.x and 1.25.x, thanks!

backdrop/backdrop@2efff2a by @ericfoy, @stpaultim, @kiamlaluno, and @klonos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants