Problem/Motivation
Cloning from #3152380: Color Boxes - can't change colors as I don't believe that was committed and closed accidentally. I can't reopen that. Anyway, I don't think that patch would help here as the problem is elsewhere.
When I use the widget "Color boxes", I can't edit the default colors. It works as long as I don't edit the default colors set by the module. But most of my colors don't work.
For example, I tried this list of colors: "#056DAB,#957000,#13224A,#774C60". Only #957000 would be displayed in the summary and on the form. There would be no error.
The problem is in how the validate method modifies the field.
preg_match_all("/#[0-9a-fA-F]{6}/", $default_colors, $default_colors, PREG_SET_ORDER);
foreach ($default_colors as $color) {
if (!empty($colors)) {
$colors .= ',';
}
$colors .= strtolower($color[0]);
}
The above block of code modifies the colors to lowercase. Then, in the summary and formElement methods, we have something like:
preg_match_all("/#[0-9A-F]{6}/", $default_colors, $default_colors, PREG_SET_ORDER);
This requires that the colors be in uppercase, but that is never possible because of the validate method. The only colors that work are those which don't have A-F hex codes in them. This is why only #957000 worked in my list.
Proposed resolution
Enforce that the colors get converted to uppercase instead of lowercase as that's the expectation elsewhere in code.
Remaining tasks
* Patch
Comment | File | Size | Author |
---|---|---|---|
#6 | 3156520-color-boxes-casing-6.patch | 3.83 KB | NickDickinsonWilde |
| |||
#6 | 3156520-color-boxes-casing-6--test-only.patch | 1.58 KB | NickDickinsonWilde |
#4 | 3156520-4.patch | 1.5 KB | hussainweb |
| |||
#2 | 3156520-2.patch | 575 bytes | hussainweb |
|
Comments
Comment #2
hussainwebAttaching the simplest patch here. This change is enough for this to work.
Comment #3
hussainwebWhile the patch above works, we could go a bit further and relax restrictions in summary and formElement methods to accept lowercase hex codes if the config happens to be there from before. Attaching a patch for that as well.
Comment #4
hussainwebSorry, wrong patch. Here's the patch I meant in #3.
Comment #5
ant1Thanks for the patch.
Works in my setup and all.
It is kind of weird that this became an issue in the first place.
I remembered it always working until the last version.
Comment #6
NickDickinsonWildeThanks Hussain - and sorry it took me a while to actually look at this.
So the CSS standard is actually lowercase for color hex values. Hence we should be using that. But those regexes were a problem. I've added a patch to ensure that both formats work and switched the default values to be lowercase.
Expanding the existing test case too to ensure that it stays fixed
Comment #9
NickDickinsonWildeThanks, committed