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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hussainweb created an issue. See original summary.

hussainweb’s picture

Version: 8.x-2.3 » 8.x-2.x-dev
Status: Active » Needs review
FileSize
575 bytes

Attaching the simplest patch here. This change is enough for this to work.

hussainweb’s picture

While 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.

hussainweb’s picture

FileSize
1.5 KB

Sorry, wrong patch. Here's the patch I meant in #3.

ant1’s picture

Status: Needs review » Reviewed & tested by the community

Thanks 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.

NickDickinsonWilde’s picture

Thanks 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

The last submitted patch, 6: 3156520-color-boxes-casing-6--test-only.patch, failed testing. View results

  • e0448ac committed on 8.x-2.x
    Issue #3156520 by hussainweb, NickDickinsonWilde: Color Boxes - can't...
NickDickinsonWilde’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.