Implemented color normalizer

[sculpt0r]

What is the purpose of this pull request?

New feature: Implemented color normalizer. Refactor color operations in CKEDITOR.tools

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?

* [#4358](https://github.com/ckeditor/ckeditor4/issues/4358): Implemented color normalizer.

What changes did you make?

Added new class Color in core in CKEDITOR.tools.style namespace.

Added Color as a dependency for tools. Also Color depends on tools.

Move color operations, RegExps, and Names Colors into Colors. tools preserved old methods (for backward compatibility). Now old methods and properties are using Color object inside.

Color constructor has compatibility flag as second parameter. It can be skipped, so Color class should behave as expected - so only valid color codes can be received (with default color: #000000). If compatibility flag is passed, then if there is no match to Named Colors, HEX, RGB or HSL input string (formerly first constructor argument) is returned.

Due to circular dependency (Color needs CKEDITOR.tools.createClass and tools needs Color for backward compatibility):

• Color class wrap class creation inside function. Until first Color object is created - there is no need for tools
• Color and tools are checking if a namespace is already declared.
• tools namespace has now a more extensible approach, instead of 'declaration'. Before, Color was declared in CKEDITOR.tools.style namespace and after tools loaded - namespace was overwritten. A new namespace declaration was done for almost every object in tools so it could be a nice step to separate other classes from this swollen file.

Which issues does your PR resolve?

Closes #4358

[f1ames]

I have rebased and retargeted this PR on latest major. Strangely there were some commits which shouldn't be there at all so looks like some rebase issue 🤔

But fixed this also 👍

[sculpt0r]

Thank you for rebasing.

I have some issues with solving tools.js diffs.

I may be wrong - but after fix commit there are two zip functions.

One in the proper namespace CKEDITOR.tools.array:

ckeditor4/core/tools.js

Line 2096 in bbc6ca5

zip: function( array1, array2 ) {

The second one is without any namespace:

ckeditor4/core/tools.js

Line 2240 in bbc6ca5

zip: function( array1, array2 ) {

The only diff is in @returns section. A two-dimensional array of object pairs. - Letter A added for correctness.

[f1ames]

The code

I had to rethink the entire approach especially that we have an issue with circular deps here. It is caused by the fact that Color class tries to use createClass function from tools and tools refers to some method/props from Color. From what I see createClass doesn't give us many benefits (almost none I suppose) and the complications caused by using it are quite significant... So I would get rid of creating Color class this way and go with another standard way we use - see for example

ckeditor4/core/dom/element.js

Lines 11 to 34 in c01d652

	/**
	* Represents a DOM element.
	*
	* // Create a new <span> element.
	* var element = new CKEDITOR.dom.element( 'span' );
	*
	* // Create an element based on a native DOM element.
	* var element = new CKEDITOR.dom.element( document.getElementById( 'myId' ) );
	*
	* @class
	* @extends CKEDITOR.dom.node
	* @constructor Creates an element class instance.
	* @param {Object/String} element A native DOM element or the element name for
	* new elements.
	* @param {CKEDITOR.dom.document} [ownerDocument] The document that will contain
	* the element in case of element creation.
	*/
	CKEDITOR.dom.element = function( element, ownerDocument ) {
	if ( typeof element == 'string' )
	element = ( ownerDocument ? ownerDocument.$ : document ).createElement( element );
	// Call the base constructor (we must not call CKEDITOR.dom.node).
	CKEDITOR.dom.domObject.call( this, element );
	};

This would mean we can get rid of all the magic from https://github.com/ckeditor/ckeditor4/pull/4366/files#diff-82fb93ef0501268a8be9290add301a266e430ea74b7f96dd0d437b10e6183de9R444 and simplify Color class.

Also I'm not sure about changes in tools.js. Assuming we would go with the above proposal (so getting rid of circular dependency), tools itself would not require flattening entire CKEDITOR.tools object and could exists in the current form. It would only need to refer to some of Color props/methods (liked namedColors) but since there will be direct, one-way dependency it will no longer cause any issues. (The Color class should inject those as mentioned in #4366 (comment)).

Other things:

1. I would be for moving colors.js to core/tools/color.js.
2. Something strange is going on since I see colornormalizer.js and color.js which has the same (partially) code 🤔 Some rebase issue probably? I assume the color.js one is correct so will add my comments there. And as you also pointed out in Implemented color normalizer #4366 (comment) that zip function got duplicated during my rebase... 🤔 Could I ask you to do a clean up here?
3. Please see also review comments.

I hadn't dive deeper into the code since it will be good to first clean up a little and then we can polish the whole thing.

[f1ames]

I feel like some clarifications are needed here 🤔💡

The current approach assumes that color util are part of the tools but should be moved to a separate file to not bulk the tools.js file further and be easier to maintain. This implicates close coupling of those two and since we would like to move some parts it has kind of dependency issue. It could be solved in a way that color depends on tools (and so tools are loaded first) and then color class/util when loaded injects itself into tools namespace.

I had some doubts whether color should really depend on tools but since it extends it, it makes sense:bulb: I will rephrase some of the review comments which are not in line with the above to be consistent with this comment.

[sculpt0r]

Realize that there is no API-doc in my PR. But maybe it will be good if I wait till all code will be accepted?

[f1ames]

Ok, starting to look good 👍

Apart from review comments, I would also suggest cleaning-up color.js a little:

Realize that there is no API-doc in my PR. But maybe it will be good if I wait till all code will be accepted?

I think it will be good opportunity to clean up color.js class and make it more readable (and also easier to review 😉 ) so I think it's good idea to work on docs now. So:

• Please add API docs to color.js file. Please remember about descriptive docs and marking private methods with @private etc.
• Also I would suggest slight code restructuring - getters should go firts then setters and then all the related helpers.
• If you think any method should not be accessible in any way through API you can move it out from class scope as a local function (e.g. clamp function, percentage normalization, etc).

[sculpt0r]

I'm not sure about

ckeditor4/core/tools.js

Lines 1078 to 1084 in 88adc86

	* @since 4.15.1
	* @private
	* @param {String} colorCode String to be validated.
	* @returns {Boolean} Whether the input string contains only allowed characters.
	* @deprecated Use CKEDITOR.tools.style.color
	*/
	_isValidColorFormat: function( colorCode ) {

Marked it as deprecated before, but now I'm not sure if I should move it to CKEDITOR.tools.color?

[sculpt0r]

I made all the requested changes.
Please look at #4366 (comment) which explain why I decided to make very strict access level for most methods in color.

[f1ames]

As for the main part of the code (color.js) we agreed with @sculpt0r for a pair refactoring session to polish the solution together. Apart form that I'm leaving few comments related to docs and tests (which we will cover too).

Documentation

There are some invalid API docs properties:

Tests

See review comments.

[f1ames]

I took this occasion to polish the code a little. Extracting and operating on RGBA color channels simplifies code visibly, but still there were some unnecessary conversions between formats in parseInput() method. Since we need color channels only, it makes more sense to extract them without converting all formats to HEX first (still we will need to have color channels conversion from HSLA since it uses different representation). And so my proposal is to just introduce extraction methods - I already prepared one which handles HEX - see 6f52e75. RGBA and HSLA still needs to be covered.

I tried to split refactoring into separate logical parts (commits) so you can see what and why specific changes were applied. And so:

1. 6f52e75 - as already mentioned switching from unnecessary conversions to RGBA channels extraction.
2. 3a51038 - simplified HEX getters. You have introduced higher order function (method to be precise) getColorValue() only to handle simple logic in getters. This significantly increases code complexity and doesn't really bring any benefit (apart from few lines of code less).
3. cc13329 - just paying attention to details in API docs and minor rewordings. Please also keep in mind that all types in API docs type annotations should start with uppercase like {String} or {Function}.
4. fec25d5 - moved blendAlphaColor() method outside of class scope. I'm not sure, on the second thought it seems all color related functions/methods should be color class private members 🤔
5. f5e360e - simplified HEX3 to HEX6 conversion. If you have a method like this, which does one thing only and the input is always the same (sting with fixed length here) there is no reason to complicate things. Also since it's private method, you can see based on it's usage that it will always get valid input.
6. 40b0748 - since we are going to extract color channels from initial formats, there is no need for those conversion methods.
7. 79e630f - again attention to details, fixed camelCase for static properties and added proper spacing between API comments there.
8. 413997d - at this point I build CKEditor 4 docs locally to check how it looks, there was one missing @private annotation.

There is also some stuff mentioned in previous comments not yet covered:

• Implemented color normalizer #4366 (comment)
• Implemented color normalizer #4366 (comment)

• I would be for moving colors.js to core/tools/color.js. (see Implemented color normalizer #4366 (review))

As for now, unit tests related to RGBA and HSLA handling are failing. This is due missing extraction methods for those formats. I would like to ask you @sculpt0r to add these methods, cleanup unnecessary stuff and polish API docs if necessary.

[sculpt0r]

I made corrects according to #4366 (comment)

Methods order.

• Mainly covered in 33992fa
• Also in 527297b, where methods were moved into the class private scope

Failing tests on IEs.

• Partially fixed for IE > 8 in 380f32a and 8253d2d
  I used global parseFloat instead of Number.parseFloat as those methods are equivalent. Also used global isNaN instead of Number.isNaN.
• For IE8 fix is a71fe56. Use trim from CKEDITOR.tools instead of native which is not supported at IE8.
• I run tests for IE 8-11 after the entire refactor and they still pass.

I have added 3 tests which should pass but are failing alpha channel in resulting RGBA ranges from 0 to 255 while it should be between 0 and 1 AFAIU. And also alpha channel in HEX format instead of FF is converted to 01 (see 3b06417).

• Failing test fixed in 8fea2d9 which was finally refactored by @f1ames in 8107b83 Alpha wasn't normalized from HEX representation parsing.

• Other case fixed in 1f12a6e
  Alpha for HEX representation needs to be re-ranged into 0-255 and was 0-1.

#4366 (comment)

• Floating consts moved to class scope in aaa6a4d

Adding API docs to new methods (I skipped some and left TODO there).

• New comments added and add small corrects in:
• while moving const to class scope: aaa6a4d
• f072e1d
• 2178922
• d4dcb01
• 216dc93

Moving colors related methods (so blendAlpha and format*String) as private color class methods.

• Methods moved in 527297b

Also, I thought that range inclusiveness (while checking if a variable is within range) is more obvious in the new form d17b51b

Found missed cases with invalid arguments number according to color code prefix (rgb & hsl). So I add two failing tests for it 943dd73 and then fixed it in e2b23c1

There was a mismatch in test naming: sometimes rgb or hsl was lowercased and sometimes uppercased. So I made everything uppercase - cause it seems to be more readable in those long tests names: f46b988

Finally, I rebased on the newest major. As before - I had a problem with core/tools.js, but finally, only one fix was needed: 59b65e3

Now it's ready for another review.

[f1ames]

LGTM 👍 🎉 🍰

Code

👍

Docs

Did some minor polishing only 👍

As for CKEDITOR.tools.convertRgbToHex() function it cannot be entirely replaced by color class since it operates on styles string, for example:

background-color: rgb(255,255,255); text-align: center;

so it does a little more. I have removed deprecated annotation and rephrase Note there.

Tests

• IE8 - IE11
• Chrome
• Firefox
• Safari

---

Apart from the above, this PR does not introduce any feature but extends public API. In general, such changes should be targeted to minor release (master branch). However, since we deprecate some API here too it means major release (major branch) 👍