Add preserving custom colors feature

[Dumluregn]

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

• Unit tests
• Manual tests

What is the proposed changelog entry for this pull request?

* [#1795](https://github.com/ckeditor/ckeditor4/issues/1795): Colors picked from [Color Dialog](https://ckeditor.com/cke4/addon/colordialog) are now stored in [Color Button](https://ckeditor.com/cke4/addon/colorbutton) palette and can be easily reused.
* [#3783](https://github.com/ckeditor/ckeditor4/issues/3783): Colors used in the document are now displayed as a part of the [Color Button](https://ckeditor.com/cke4/addon/colorbutton) palette.

What changes did you make?

• Features

Implementation is mainly based on conclusions listed here, but during development I made one significant change - in palette there are two separate sections for Content Colors (colors found in the editor content - checked on every colorbutton panel open) and Custom Colors (colors picked from color dialog). Without this division the palette was changing unintuitive when e.g. some parts of coloured text were deleted.
Thanks to that it was also possible to implement both features independently, so theoretically this PR could be divided into two if it's easier to work with for reviewer.

• Unit tests

1. As we are in the middle of discussion about place of issue references in unit tests (using bender-tags or not), for now I just added references for the whole files as it was quicker way. If we determine that adding tags for each test is better, of course I'll change it.
2. Features are implemented for both Text Color and Background Color buttons and the only difference between them is that they should track colors independently, so only tests checking setup are doubled - the core features are tested only for Text Color. If this is insufficient, of course I'll change it.
3. Code for preserving color style is entirely in colorbutton plugin so I also put tests there, but in fact without colordialog it doesn't do a thing, so I wonder if it would be better to place them in the colordialog folder.

• Manual tests

1. I'm guess despite manual tests are split into 2 files they may still be to complex, if reviewer will share this point of view then of course I'll change it.
2. The same as point 3. for unit tests - maybe tests for custom colors should be in colordialog folder?

• PS

1. There will be probably two new phrases to translate - right now they are "Content Colors" and "Custom Color". Yet I didn't update lang files, as they can still change - let's create new entries just before merge.
2. In our test base I saw e.g. such code snippet:

<h2 style="color:FF0000">Hello</h2>

and in this format the color will not be displayed in palette (which makes sense with because default ACF settings don't allow this construction).
As for now this feature only finds colors expressed in hex code attached to spans through style attribute, like here:

<p><span style="color:334455">Hello</span></p>

But I just realised that the below snippet is also fine for ACF:

<p><span style="color:red">instance</span></p>

so probably it should work too, and now it doesn't... So I'll fix that after the first review :)

Which issues your PR resolves?

Closes #1795.
Closes #3783.

[jacekbogdanski]

I took first look at this PR, but before we go with a full review, let's resolve issues on how the feature should work, as I'm not sure if specification fully covered use cases.

I have some doubts about the current color matching algorithm, but I may have a bit different view of how this feature could work. Overall, it seems nicely match colors from editor content + colors matched from the color dialog. However, it's a bit hard to find out from UX how the whole feature works and I'm not sure if we didn't slightly miss the point of the feature usability.

The main doubt is about what's more important for a user: match available colors in current editor content or show the latest chosen color in the quick access panel.

It's somehow more convenient from my perspective that this feature:

1. Finds out all available content colors on the first color button opening to fill empty quick access panel as at this point I didn't choose anything manually. Sort them in some smart order e.g. from the start of the content.

Content colors:
red | blue | yellow | green

2. If I have chosen color manually it's moved at the beginning of the quick access color row. The last color is removed if there is no space left. It doesn't matter if color is chosen from a color dialog or just predefined panel color - at this point, a quick access row is treated as the color history. I know specs tell that we don't want duplicate colors, but if we don't, it breaks feature flow and makes it less user-friendly. You shouldn't really find out how things works, it should just 'click' on the first peek.

Chosen black:
black | red | blue | yellow

3. If I choose the same color twice, it's moved at the beginning, but the rest of the colors stays at their position (-1 because the color from the end has been moved, but nothing removed).

Chosen yellow
yellow | black | red | blue

Of course, currently, we have 2 separate rows, one for document content colors and the second for colors chosen from the color dialog. However, after some clicking, I'm not sure about UX and if it's worth to separate both features 🤔 Maybe additional config option for more color rows could cover cases where someone would like to have a "bigger picture".

Also, please take a look at what's going on with CI.

[f1ames]

I have some additional feedback regarding @jacekbogdanski #3784 (review).

The initial specs was about having one custom colors row, you have decide to split it to two - which I'm fine with as long as it works intuitively.

Content colors row

Content colors - seems to be showing colors used in the content. The problem here is that, both custom colors from the content appears there (so pasted from Word or added manually via source mode) and colors picked from color panel/dialog too 🤔 So these are just all colors used but sorted in order based on their appearance in the content.

So from my perspective, it should only show colors which were not picked manually from color panel or dialog - only these which appeared there other ways (paste, etc).

And as mentioned in #1795 as we treat them as content colors these should be sorted by number of appearances:

Colors found in content should be ordered by number of occurrences, counted in elements.

and if the number is the same I would be for sorting them in order of appearance in the content (we didn't covered this in #1795).

Custom colors row

I know those are only colors picked from dialog, however it seems unintuitive for me as picking color from dialog or panel has the same effect - the dialog is just an extension of the panel with more colors so these should be treated the same way.

Also since colors added from dialog also appear in the content, they are also shown in the Content colors row which might be confusing 🤔

---

Getting back to @jacekbogdanski #3784 (review) it seems to be in tact with what we drafted as sepcs some time ago here - #1795 (comment) and so:

1. Finds out all available content colors on the first color button opening to fill empty quick access panel as at this point I didn't choose anything manually. Sort them in some smart order e.g. from the start of the content.

Content colors:
red | blue | yellow | green

Agree. The only thing I'm not sure and is different in the initial specs is:

Document colors should be updated on each panel show. This way we are able to catch custom colors in content pasted from Word etc.

The idea was here that you can open color panel and apply some color (or even just open/close), then paste some colorful content and then open color panel again - shouldn't we fill empty color slots with the colors that appeared in the content?

2. If I have chosen color manually it's moved at the beginning of the quick access color row. The last color is removed if there is no space left. It doesn't matter if color is chosen from a color dialog or just predefined panel color - at this point, a quick access row is treated as the color history. I know specs tell that we don't want duplicate colors, but if we don't, it breaks feature flow and makes it less user-friendly. You shouldn't really find out how things works, it should just 'click' on the first peek.

Chosen black:
black | red | blue | yellow

Color should be moved to the beginning when picked (from panel, dialog or even or custom colors row) as mentioned.

What about duplicating colors? Can't we just move picked color to the beginning and left the empty slot on the end (or fill with white or next content color available)?

3. If I choose the same color twice, it's moved at the beginning, but the rest of the colors stays at their position (-1 because the color from the end has been moved, but nothing removed).

Chosen yellow
yellow | black | red | blue

Not sure what's the difference between this point and previous one 🤔

@jacekbogdanski Consider your scenario wit a little different set of colors:

1. Initial colors:

Content colors:
red | blue | yellow | green

2. Picked yellow from color panel (moving to beginning with duplication)

Content colors:
yellow | red | blue | yellow

3. Picked yellow again (so picked twice - moving to the beginning):

Content colors:
yellow | red | blue | ? / content color

wouldn't it be simpler with just two steps:

1. Initial colors:

Set 1
red | blue | yellow | green

Set 2
red | blue | yellow | empty

Set 3
red | blue | black | green

2. Picked yellow from color panel (moving to beginning, no duplication)

Set 1
yellow | red | blue | green

Set 2
yellow | red | blue | empty

Set 3
yellow | red | blue | black

[f1ames]

@Dumluregn can you add a shot summary what we agreed on during F2F talk?

[Dumluregn]

Sure:

1. We prefer to keep both content colors and custom colors unified in one row, which will be used as a color history.
2. Only at the first panel open (so onBlock) colors from content will be read and rendered in the history row. When a new color is applied (no matter if it comes from default palette or from color dialog), it is prepended to the history row (excessive colors from the end are removed). If this color was present in the row before, it is just relocated to the beginning (it doesn't matter if it's color from palette or from content).
3. Colors from the content should be sorted according to their frequency of occurrence.
4. Colors from the content that appeared in the editor after the first panel open (e.g. pasted from external source) are ignored for now. Probably handling them will be treated as a follow-up issue and suspended for now.
5. If a color from default palette is applied, it will be duplicated and appear in both places.
6. We would like to expose API for finding colors in the editor contents.
7. As for the config options, right now we only implement the possibility to set number of custom rows. Later on we may think about adding different color finding algorithm (e.g. to search not only by span elements) or separating content and custom colors, but as follow-up issues.

[f1ames]

7. For now we will not include any config options. Later on we may think about adding multiple rows, different color finding algorithm (e.g. to search not only by span elements) or separating content and custom colors, but as follow-up issues.

I think we agreed on config option for number of custom colors rows? @jacekbogdanski?

The rest of the summary looks good 👍

[Dumluregn]

Hm you may be right. I edited the summary.

[jacekbogdanski]

I think we agreed on config option for number of custom colors rows?

Yes, could be useful to preserve more colors from content if needed.

[Dumluregn]

One more thing I think we agreed to do was to ask some non-tech or UI people to test the solution.

[jacekbogdanski]

Yeah, but let's create the solution first 😄

[Dumluregn]

Failing CI can be fixed by rebasing after #3744.

[jacekbogdanski]

Rebased into the latest upstream.

[jacekbogdanski]

Smaller issues

1. Failing test on Chrome:
   
2. There are multiple errors when running unit tests in dev console:
   
3. There is no need to overuse config.colorButton_colorsPerRow configuration option in manual tests. We just need one single manual test dedicated to this option to make sure that history row is correctly updated depending on the available colors.

Proposed architecture

I see that you tried to update current implementation with a more OOP approach, so 👍 It seems like we could even more clean up this code by introducing more component-based implementation. Overall, we should try to go with a similar class structure to:

in its a basic form of public/internal API. I created a very simple structure using mostly pseudo code here: t/1795...t/1795b - please, take a look, maybe it will give you some clues about how we could improve the code. Mostly, it's just about moving proposed functions into these 3 classes and hiding logic inside smaller components. We will be able to save DOM elements references in that case, which should improve performance, readability and hide function signatures with too many arguments.

The current implementation of the colorbutton should also reuse ColorBox and ColorRow to generate panel HTML.

Of course, if you find out it too time-consuming, tell so we can find some golden mean.

[Dumluregn]

@jacekbogdanski do you want me to rebase this PR onto latest major?

[jacekbogdanski]

There are some merge conflicts, so yes please @Dumluregn

[Dumluregn]

Rebased onto latest major.

[Dumluregn]

CI failed because some code was lost during rebase, so please pull this branch again.

[jacekbogdanski]

Good job @Dumluregn 🎉 We are ready to merge 🔥