Unify color handling #4718

Comandeer · 2021-05-18T15:23:28Z

What is the purpose of this pull request?

Task

Does your PR contain necessary tests?

All patches that 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

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

Fixed Issues:

* [#4333](https://github.com/ckeditor/ckeditor4/issues/4333): Fixed: [Color Button](https://ckeditor.com/cke4/addon/colorbutton) history does not render correctly colors passed in non-hex format.

API Changes:

* [#4726](https://github.com/ckeditor/ckeditor4/issues/4726): Added `options` parameter to both [`CKEDITOR.tools.color#getHex()`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_tools_color.html#method-getHex) and [`CKEDITOR.tools.color#getHexWithAlpha()`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_tools_color.html#method-getHexWithAlpha) methods with `options.shorten` property to allow getting short hex from the given color.

What changes did you make?

I've used CKEDITOR.tools.color wherever it was possible. Some places still need some love and refactor, but I'm not sure if it's worth it.

I've also introduced the ability to get short hex from CKEDITOR.tools.color#getHex() and CKEDITOR.tools.color#getHexWithAlpha() via a new parameter, shorten. However I'm not sure if it wouldn't be better to introduce options parameter. It would be more flexible 🤔

I've also changed the way in which colors in Color History are rendered.

Which issues does your PR resolve?

Closes #4592.
Closes #4726.
Closes #4333.

sculpt0r

It looks like the root issue is fixed now (#4333) 👍 👍

However I'm not sure if it wouldn't be better to introduce options parameter. It would be more flexible

Well, it sounds like a good idea. When you look at some functions, that didn't use an object as an option. They end with lots of code just to provide compatibility later. For example:

ckeditor4/core/editor.js

Lines 1125 to 1135 in 8e5575f

    
           setData: function( data, options, internal ) { 
        
           	var fireSnapshot = true, 
        
           		// Backward compatibility. 
        
           		callback = options, 
        
           		eventData; 
        
           	if ( options && typeof options == 'object' ) { 
        
           		internal = options.internal; 
        
           		callback = options.callback; 
        
           		fireSnapshot = !options.noSnapshot; 
        
           	}

It looks for now that there won't be any more thing to add, but... you never know that - until you have to add another one :)

I've also changed the way in which colors in Color History are rendered.

I found a little inconsistency here. When you explicitly type lowercase 6-HEX - it's lowercase in history. If you type it uppercase - it's uppercased.

Also, If we are allowing 6-HEX in history, why we even want them to be shortened? It looks like that creates some additional code that could be skipped? I know, we are allowing 3-HEX-es, but for sake of unify we could decide on one format to be displayed. Therefore, even 3-HEX will be converted to 6-HEX.

Overall I see, that a few '#' and substr disappears 👍

plugins/colorbutton/plugin.js

sculpt0r · 2021-05-26T11:55:44Z

plugins/colorbutton/plugin.js

 				}

 				// Replace 3-character hexadecimal notation with a 6-character hexadecimal notation (#1008).
-				return CKEDITOR.tools.normalizeHex( '#' + CKEDITOR.tools.convertRgbToHex( color || '' ) ).replace( /#/g, '' );
+				// It also covers other cases, like colors with alpha channel (#4351).
+				return CKEDITOR.tools.normalizeHex( colorInstance.getHex() || '' );


This function is already marked as deprecated. We already have some ambiguous normalizations around. The one is here in ColorBox and another in tools. So I'm starting to wonder - what is the normalization process because it leads to very different results. It is related to parsing values from CSS attributes. So maybe we should separate the so-called normalization and extracting value from the CSS attribute?

The issue is that removing this normalization will basically nuke tests… CKEDITOR.tools.color returns uppercase hexes and whole tests assume they are lowercase. So I'm not sure if it's worth it to refactor all tests.

Or not, nothing happened 🤔 Strange, because it suggests that there's yet another normalization on the way…

Comandeer · 2021-05-31T10:31:36Z

Also, If we are allowing 6-HEX in history, why we even want them to be shortened? It looks like that creates some additional code that could be skipped? I know, we are allowing 3-HEX-es, but for sake of unify we could decide on one format to be displayed. Therefore, even 3-HEX will be converted to 6-HEX.

The issue is that user could want to use the original format of the color (see http://localhost:1030/tests/plugins/colorbutton/config#tests%2Fplugins%2Fcolorbutton%2Fconfig%20test%20config.colorButton\_foreStyle%20with%20color%20attribute), not the normalized one. And the current flow de facto disables the ability to force normalization for history entries (it would require creating separate ColorBox class for history).

And that's why I'm so unhappy with the whole refactor: the code is still messy. And with changes from this PR it becomes even messier…

sculpt0r

I added a few requests for color class extensions and few other thoughts about passing the entire value. Also, I've made some diagram which helps me with the code flow (https://miro.com/app/board/o9J_lApbiDQ=/)

If my comments would appear to be invalid due to back incompatibility, I think we will be finishing with this only with a few detailed comments I also left. 🤞 .

Indeed there is a lot going on. It takes a while before I catch up how it works.

The last thing: I think we should look at colordialog - there is also some magic with color validation and RegExps...

sculpt0r · 2021-06-01T12:20:09Z

plugins/colorbutton/plugin.js

@@ -308,7 +308,7 @@
 								}
 							}, null, colorData );
 						} else {
-							setColor( color && '#' + color, colorName, history );
+							setColor( color && color, colorName, history );


Suggested change

setColor( color && color, colorName, history );

setColor( color, colorName, history );

sculpt0r · 2021-06-01T12:44:22Z

plugins/colorbutton/plugin.js

@@ -269,9 +269,9 @@
 								finalColor = '';
 							}
 							if ( type == 'fore' ) {
-								colorData.automaticTextColor = '#' + ColorBox.normalizeColor( automaticColor );
+								colorData.automaticTextColor = ColorBox.normalizeColor( automaticColor );


Should we also adjust line with automaticColor = '#ffffff'; to be used with tools.color?

Then we can use explicit `panelBlock.element.findOne( '#' + colorBoxId ).setStyle( 'background-color', automaticColor.getHex() );

sculpt0r · 2021-06-01T12:50:21Z

plugins/colorbutton/plugin.js

-
-					return CKEDITOR.tools.normalizeHex( colorInstance.getHex() || '' ).replace( /#/g, '' );
+				var colorInstance = ( color instanceof CKEDITOR.tools.color ) ? color : new CKEDITOR.tools.color( color, '' ),
+					isTransparent = Number( colorInstance._.alpha ) === 0;


Don't we already store alpha as a number in the color instance? But, what is more important, I would rather add colorInstance.isFullyTransparent() method to the color class without taking out the inner implementation of alpha outside. We shouldn't reach for private members just to avoid problems if we decide to change something inside the color class.

sculpt0r · 2021-06-01T13:00:03Z

plugins/colorbutton/plugin.js

 			 * @returns {String} Returns color in hex format, but without the hash at the beginning, e.g. `ff0000` for red.
+			 * In case of transparent colors, it returns `'transparent'` string.
 			 */
 			normalizeColor: function( color ) {


I have a problem with naming here. Since it is private - nothing should break on the client's side if we rename it? As I wrote before normalization is a kind of a wide concept. In tools, we have normalizeHex which means to convert styleText into 6-HEX. Here, normalizeColor means: return transparent or 6-HEX or even default value which could be anything that comes with the color instance.

And we have some intention when we call this method. We want to receive some value that would be applicable as a CSS attribute. And we use it to compare two colors that come from various sources.

So maybe convertToStyleValue() or sth similar?

I'm not sure about changing name. It's not used only for getting color suitable for [style] attribute, e.g.

ckeditor4/plugins/colorbutton/plugin.js

Line 180 in 70d7394

color = ColorBox.normalizeColor( color );

In one case it's even used for the reversed operation – getting color from CSS →

ckeditor4/plugins/colorbutton/plugin.js

Line 651 in 70d7394

return color in list ? list[ color ].substr( 1 ) : ColorBox.normalizeColor( span.getComputedStyle( cssProperty ) ).toUpperCase();

sculpt0r · 2021-06-04T05:26:14Z

plugins/colorbutton/plugin.js


 			this.setHtml();
 		},

 		statics: {
+			getColorName: function( editor, color ) {
+				var names = ColorBox.colorNames( editor ),
+					hex = color.getHex().replace( /^#/g, '' ),


If we need HEX without # sign, maybe it's good time to add another option (e.g. named withHash or sth similar) to our getHex( optionObj ). The same goes for the below line. It should be easier to read that we want

getHex( { withHash: false } )

than decoding replace while we are reading this logic. I know, this is not so complicated, but still - requires at least a moment to find out what is the intention here.

sculpt0r · 2021-06-04T07:06:37Z

plugins/colorbutton/plugin.js

+				return colorInstance.getHex() || '';
+			},
+
+			getAppliableColor: function( color ) {


It is used only inside setHtml, why we need this to be static?

I thought that would be more coherent with existing normalizeColor. But there isn't any real need for that, it can be moved as a private function into setHtml.

sculpt0r · 2021-06-04T07:44:12Z

plugins/colorbutton/plugin.js

+			},
+
+			getAppliableColor: function( color ) {
+				if ( !color._.isValidColor ) {


We are reaching for a private members of the color instance - which is not good practice. I try to break color-code / color-obj flow (from the getApplicableColor point of view): https://miro.com/app/board/o9J_lApbiDQ=/

I'm not sure how much of this should be affected by backward compatibility, but it looks like we are spinning around with our own value transformations. The ideal way: we are take anything from colordialog and convert it to a color object. The same for input from config.

If color from colordialog is ivalid -> then we could have an empty default value (as default value returned by color class in case of invalid color). If we want to store original input provided by the user, we can always store it as data attribute using color.getInitialValue().

sculpt0r · 2021-06-04T07:46:48Z

plugins/colorbutton/plugin.js

+						color.getHex( { shorten: initialValue.length === 3 } ).toLowerCase().substr( 1 ),
+					isShortHexLike = isHexLike && initialValue.length === 3;
+
+				return isHexLike ? color.getHex( { shorten: isShortHexLike } ) : initialValue;


We probably could return to the user getInitialValue() without any checks. The User always gets back what was provided. And just in case someone misspells RGB value, it can be corrected instead of typing it again from scratch. But we probably should add some indicator to the colordialog in case of invalid value.

sculpt0r · 2021-06-04T07:52:20Z

plugins/colorbutton/plugin.js

@@ -532,16 +553,19 @@
 			},

 			setHtml: function() {
+				var appliableColor = ColorBox.getAppliableColor( this.color ),
+					colorStr = appliableColor ? this.color.getInitialValue() : '';


colorStr name seems to be low informative. Could you name it something like validatedOriginalColor or similar?

sculpt0r · 2021-06-04T07:57:52Z

plugins/colorbutton/plugin.js

-				} );
+				var normalizedColor = ColorBox.normalizeColor( colorCode ),
+					index = CKEDITOR.tools.getIndex( this.boxes, function( box ) {
+						return ColorBox.normalizeColor( box.color ) === normalizedColor;


Two instance creations of ColorBox have color property passed as an object. We should create compare method inside color class that tells whenever two colors are equals.

…istory.

Comandeer · 2021-06-04T10:22:06Z

Rebased onto latest major.

f1ames · 2021-06-09T11:12:53Z

After some talks with @Comandeer and @sculpt0r we have decided to freeze this PR for now. Any reasonable changes would require refactoring big part of the existing solution which would be really time consuming and due to other priorities we can't proceed with this ATM.

Comandeer self-assigned this May 20, 2021

Comandeer force-pushed the t/4592 branch from a80b8c6 to 8b18fc3 Compare May 24, 2021 15:15

Comandeer marked this pull request as ready for review May 25, 2021 15:57

Comandeer removed their assignment May 25, 2021

sculpt0r self-assigned this May 26, 2021

sculpt0r self-requested a review May 26, 2021 09:37

sculpt0r suggested changes May 26, 2021

View reviewed changes

CKEditorBot unassigned sculpt0r May 26, 2021

Comandeer self-assigned this May 31, 2021

Comandeer force-pushed the t/4592 branch from 72f1fc5 to ec2077b Compare May 31, 2021 10:32

Comandeer requested a review from sculpt0r May 31, 2021 10:34

CKEditorBot unassigned Comandeer May 31, 2021

sculpt0r self-assigned this Jun 1, 2021

sculpt0r suggested changes Jun 4, 2021

View reviewed changes

CKEditorBot unassigned sculpt0r Jun 4, 2021

Comandeer self-assigned this Jun 4, 2021

Comandeer added 13 commits June 4, 2021 12:21

Add manual test for colorbox rendering.

509bf4e

Use CKEDITOR.tools.color in Colorbox.

3262413

Update colors in manual test.

402c8e5

Check the initial value of color box color while building the color h…

06a4f00

…istory.

Add check for color box selection.

e906ec2

Simplify normalization to always return valid hex.

d9328c7

Introduce ColorBox.getColorName function.

4ad8178

Update tests to work with the new normalization method.

f9a17f8

Apply color as is, without adding #.

7216709

Introduce ColorBox.getAppliableColor method.

4271615

Search for name for both long and short hex.

f476f9b

Add unit tests for fetching color names for long hexes.

c73b002

Introduce the ability to shorten hexes.

19595b3

Comandeer added 16 commits June 4, 2021 12:21

Use built-in support for short hexes.

5ca73bc

Update ColorBox.getAppliableColor() to support 3-HEX values.

f61d977

Add tests for detecing automatic highlight.

f2b2b42

Update test for converting transparent value to hex color.

828112e

Update normalizing for transparent colors.

ce791c5

Add manual test for selecting automatic color.

4185b05

Ignore failing color conversion tests.

361aedf

Add tests for non-hex colors in color history.

cec9d68

Normalize colors in Color History.

094f0ee

Update failing tests.

5b40497

Do not render incorrect colors.

416b1ba

Update incorrect comment in the test [ci skip].

4037936

Update API docs for ColorBox.normalizeColor.

b75fb89

Use correct form of options parameter in color#getHex().

36e0e56

Remove CKEDITOR.tools.normalizeHex() from ColorBox.normalizeColor().

50880fc

Introduce options parameter to CKEDITOR.tools.color#getHex().

5d3dc76

Comandeer force-pushed the t/4592 branch from ec2077b to 5d3dc76 Compare June 4, 2021 10:21

f1ames closed this Jun 9, 2021

f1ames added the pr:frozen ❄ The pull request on which work was suspended due to various reasons. label Jun 9, 2021

This was referenced Jun 9, 2021

Unify color handling #4592

Open

Ability to get the short hex from CKEDITOR.tools.color instance #4726

Open

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify color handling #4718

Unify color handling #4718

Comandeer commented May 18, 2021 •

edited

Loading

sculpt0r left a comment

sculpt0r May 26, 2021

Comandeer May 31, 2021

Comandeer May 31, 2021

Comandeer commented May 31, 2021

sculpt0r left a comment

sculpt0r Jun 1, 2021

sculpt0r Jun 1, 2021

sculpt0r Jun 1, 2021

sculpt0r Jun 1, 2021

Comandeer Jun 4, 2021 •

edited

Loading

sculpt0r Jun 4, 2021

sculpt0r Jun 4, 2021

Comandeer Jun 4, 2021

sculpt0r Jun 4, 2021

sculpt0r Jun 4, 2021

sculpt0r Jun 4, 2021

sculpt0r Jun 4, 2021

Comandeer commented Jun 4, 2021

f1ames commented Jun 9, 2021

	setData: function( data, options, internal ) {
	var fireSnapshot = true,
	// Backward compatibility.
	callback = options,
	eventData;

	if ( options && typeof options == 'object' ) {
	internal = options.internal;
	callback = options.callback;
	fireSnapshot = !options.noSnapshot;
	}

	setColor( color && color, colorName, history );
	setColor( color, colorName, history );

Unify color handling #4718

Unify color handling #4718

Conversation

Comandeer commented May 18, 2021 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

Did you follow the CKEditor 4 code style guide?

What is the proposed changelog entry for this pull request?

What changes did you make?

Which issues does your PR resolve?

sculpt0r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer commented May 31, 2021

sculpt0r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer commented Jun 4, 2021

f1ames commented Jun 9, 2021

Comandeer commented May 18, 2021 •

edited

Loading

Comandeer Jun 4, 2021 •

edited

Loading