Color history incorrectly displays colors entered as name or in RGB format

[Dumluregn]

Type of report

Bug

Provide detailed reproduction steps (if any)

1. Go to CDN.
2. Select some text.
3. Open color dialog.
4. Type red in the input and click Ok.
5. Open color panel and examine the first color in color history.

Expected result

There is a red color box.

Actual result

Color box is transparent - it's color is ED.

Other details

The same problem occurs if you type a color in the RGB format. The reason is color history assumes the first character in color name will be a # and removes it - no questions asked.

• Browser: each
• OS: macOS
• CKEditor version: starting with 4.15.0 and color history feature (Preserving custom colors feature #1795)
• Installed CKEditor plugins: colordialog

[sculpt0r]

I'm not sure if we should just take care of HEX and RGB format?

Maybe we should validate input for common standards (HEX, RGB, HSV, others?).
Then, in the case of 'plain text', we could try to define allowed color names (red, green, etc), and look into it?

If there is no match - there is no color change after OK button click or just set it to #000 or #FFF?

[f1ames]

I'm not sure if we should just take care of HEX and RGB format?

Maybe we should validate input for common standards (HEX, RGB, HSV, others?).
Then, in the case of 'plain text', we could try to define allowed color names (red, green, etc), and look into it?

If there is no match - there is no color change after OK button click or just set it to #000 or #FFF?

Well, there are at least two cases here - one related to the mechanism which assumes that all colors inserted in color dialog are in HEX format (thus starting with #). This assumption results in first character from color code (no matter the format and character itself) being removed. AFAIU it means it happens for any color format and the first character is simply removed. And this is what this issue is about. This means we should try to handle only this case here.

The other case is related to RGBA/HSLA handling and reported in #4351 (not sure if you had this one in mind), but it should be handled as separate fix.

Maybe we should validate input for common standards (HEX, RGB, HSV, others?).

I guess you may be looking into major branch and not master. We have recently added validation but the latest code is on master branch for now only. @sculpt0r if you are working on a bugfix, you should always branch and target to master, while only new features should branch and target major branch.

[sculpt0r]

Well, there are at least two cases here - one related to the mechanism which assumes that all colors inserted in color dialog are in HEX format (thus starting with #). This assumption results in first character from color code (no matter the format and character itself) being removed. AFAIU it means it happens for any color format and the first character is simply removed. And this is what this issue is about. This means we should try to handle only this case here.

Solving this as @Dumluregn expected require handling any type of color format (inserted by user). If we fix only 'cutting' first character problem (from user input) - it will provide proper results for data in HTML. But still - there will be transparent ColorBox.

ColorBox assumes only HEX format - so still we have to deal with conversion. So I think we can't provide proper expected behavior without solving 'higher' problem with color conversion. Like you said: #4351.

I guess you may be looking into major branch and not master. We have recently added validation but the latest code is on master branch for now only. @sculpt0r if you are working on a bugfix, you should always branch and target to master, while only new features should branch and target major branch.

That was definitely that problem - Thanks.

[sculpt0r]

We could create Color class which get 'string` at constructor. Inside, we could:

• verify if input string is correct (HEX, RGB, etc)
• create methods that provide any format - base on input data
• handle string colors (like red, green) - there is already some mapping inside core/tools

Benefits:

• we can pass Color object between functions, instead of 'magic string`
• in every use case, we use explicit expected format, like color.toHex() or color.toRGBA - so it is always known what type of color formating is used
• it could be used in an entire editor, so there should be no more problem with colors, also pretty big core/tools file could be shrinked
• each conversion inside one class should be quite comfortable testable with unit tests

It's similar to @jacekbogdanski idea at #4351

Maybe it's worth doing as a common solution for both issues?

[jacekbogdanski]

I don't mind cleaning this mess. @sculpt0r proposition about separate Color class sounds sensible as we still need to write RGBA -> HEX conversion. Just keep note that we can't simply:

it could be used in an entire editor, so there should be no more problem with colors, also pretty big core/tools file could be shrinked

Because we are in LTS support, so no breaking changes allowed (although we could deprecate some methods). Besides, we will still probably have to keep it in core/tools file due to limited module loading in CKE4 😄

The question is if we want to spend significant time on this refactoring for a fairly simple bug fix.

[jacekbogdanski]

Looks like I mixed up the different issue with this one - #4351 😄 So, yeah the proposition looks good, but:

Well, there are at least two cases here - one related to the mechanism which assumes that all colors inserted in color dialog are in HEX format (thus starting with #). This assumption results in first character from color code (no matter the format and character itself) being removed. AFAIU it means it happens for any color format and the first character is simply removed. And this is what this issue is about. This means we should try to handle only this case here.

I agree with that and your proposition should rather be implemented in #4351

The issue is probably caused by a hardcoded hash in

ckeditor4/plugins/colorbutton/plugin.js

Line 524 in 345756a

'<span class="cke_colorbox" style="background-color:#' + this.color + '"></span>' +

So, moving the correct color hash composition outside the template should be enough to fix it, right? In that case, it doesn't make sense going with such big refactoring, since it will be rather available via a major release. I would be for extracting it into a separate issue if we are able to fix this bug in a couple lines of code.

[jacekbogdanski]

Ok, I've mixed it up again, is it Monday or something? I'm still writing about #4351 where we have prefixed hash character, not removed when it doesn't exist. I suppose the issue happens somewhere during hex normalization, so it should be still easy to fix. It would be good to check if #4351 hash prefix for rgba/hsla is caused by invalid conversion or some magic with # and maybe also fix this part (with #, not alpha blending) here, as it may be the same piece of code.

[f1ames]

@jacekbogdanski was first with his #4333 (comment), but it seems we mostly agree 👍 (yeah, not after his two next comments 🙈)

Because we are in LTS support, so no breaking changes allowed (although we could deprecate some methods).

Yes, no breaking changes (unless we have a very good reason which is almost never 😄), but deprecating stuff is acceptable.

Besides, we will still probably have to keep it in core/tools file due to limited module loading in CKE4:smile:

Keeping it there will be the simplest option (especially that we can't move existing function to not cause breaking changes - but we can somehow proxy new ones to have backwards compatibility). But adding new file is not difficult, since it's enough to add it to loader class. So that's an option too and since everything is minified and concat I don't see a reason not to extract it.

---

So unifying/normalizing colors generally seems like a good idea to keep the code simpler (by extracting colors handling logic). After looking into colorbutton/colordialog plugins it seems we have in fact 3 4 issues here:

1. The one mentioned here - so removing first character from color name (because of HEX format assumption).
2. The other issue described in RGBA/HSLA colors are incorrectly encoded #4351 - so adding # to color name in dialog color input.
3. The fact that color dialog matches selected color (to focus it in the palet after dialog is opened) by HEX values, so any other format (even if the color is present in the palet) will not be highlighted (I see we have some kind of normalization there but not sure if it works for all formats). This is in fact related to RGBA/HSLA colors are incorrectly encoded #4351.
4. Color history assumes HEX only so if you add red then #F00 and then rgb(255,0,0) via dialog it will create 3 separate entries instead of one. And also ColorBox in color history will not be highlighted based on selection (because colors from content are normalized to HEX and there is no HEX red there).

So I'm for extracting colors logic, so we can cover above issues (including #4351) at once, but @sculpt0r please try the KISS approach to really focus on solving the above issues. And as we already have some conversion/normalization color functions in tools (_isValidColorFormat, convertRgbToHex, normalizeHex) maybe these can be reused somehow.

And it will be good to extract separate issue here for Introducing color normalization tools.

WDYT @jacekbogdanski @sculpt0r?

---

As a side not, the colors in the content itself (e.g. <span style="color:rgba(255,0,0,1);"> are not normalized which is kind of dirty 🤷‍♂️ but as one can paste or insert them other ways we still need to normalize them when getting from style attribute so that's another story 🙈

[f1ames]

After F2F talk with @jacekbogdanski and @sculpt0r we decide to go with "proper" solution, proposed initially by @sculpt0r and described also in #4333 (comment) - so introducing tool for color normalization. Such approach will help us to fix all issues mentioned above without making already convoluted color handling logic even more complex.

[Comandeer]

Ok, so basically it seems that trying to fix this issue equal de facto unifying the whole color handling flow, as any change to ColorBox breaks current behavior of color plugins in multiple ways (e.g. storing color as CKEDITOR.tools.color instance breaks applying color to the editor and requires additional workarounds).

[Comandeer]

Blocked on #4592 as trying to fix this issue requires a vast refactoring of how colors are passed between specific parts of the editor.

[f1ames]

We had started working on the PR to cover this issue but decided to postpone it due to #4718 (comment).