feat(Color) Regex and color code cleanup and new color format support #8916

asturur · 2023-05-13T12:04:33Z

Motivation

Improve the regexes and the documentation around them.

So the part with strings and composition was definitely the most readable.
But took 400 bytes of extra code where usually we fit a small feature or a fix or 2.
Now we don't read regexes as a flow, we want to be able to know what they do and i wrote extensive docs for them

Changes

Following

https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb
and
https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/hsl

improve the regex by adding the ability to parse the alpha channel from the format 80%
add the ability to recognize the format with spaces and not commas and the / for the alpha channel
add comments, lot of them

Gist

In Action

github-actions · 2023-05-13T12:06:18Z

Build Stats

file / KB (diff)	bundled	minified
fabric	919.369 (+4.151)	303.564 (-0.512)

github-actions · 2023-05-13T12:08:26Z

Coverage after merging regex-color-cleanup into master will be

83.65%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	47.83%	100%	25%	60%	17, 20, 23, 40, 43, 46
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	10
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.23%	85.29%	84.62%	89.36%	144–145, 170–171, 39–40, 48, 57, 91, 99
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
src/Pattern
Pattern.ts	92.21%	91.89%	90%	93.33%	116, 127, 136, 29, 92
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	107, 107, 107, 109, 111, 113–115, 117–120, 127–128, 135, 137, 22–23, 31–35, 39–43, 50–53, 61–65, 67, 75, 75, 75, 75, 75–76, 78, 78, 78–81, 83, 91–92, 94, 96–98
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.01%	82.35%	100%	93.75%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	106, 106, 106, 106, 106–107, 109–110, 117–118, 120, 122–126, 135, 139–140, 140, 148, 148, 148–151, 153–156, 16, 160–161, 163, 165–168, 17, 171, 178–179, 181, 183–184, 186, 19, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 21, 21, 211, 22–23, 27, 36, 43, 50, 57, 64, 83–85, 93–95, 97–98
src/canvas
Canvas.ts	78.87%	77.54%	81.67%	79.41%	1001–1002, 1002, 1002–1004, 1006–1007, 1007, 1007, 1009, 1017, 1017, 1017–1019, 1019, 1019, 1025–1026, 1034–1035, 1035, 1035–1036, 1041, 1043, 1074–1076, 1079–1080, 1084–1085, 1198–1200, 1203–1204, 1277, 1396, 1519, 1589, 162, 187, 297–298, 301–305, 310, 333–334, 339–344, 364, 364, 364–365, 365, 365–366, 37, 374, 379–380, 380, 380–381, 383, 392, 398–399, 399, 399, 41, 442, 450, 454, 454, 454–455, 457, 539–540, 540, 540–541, 547, 547, 547–549, 569, 571, 571, 571–572, 572, 572, 575, 575, 575–576, 579, 588–589, 591–592, 594, 594–595, 597–598, 610–611, 611, 611–612, 614–619, 625, 632, 669, 669, 669, 671, 673–678, 684, 690, 690, 690–691, 693, 696, 701, 714, 742, 742, 800–801, 801, 801–802, 804, 807–808, 808, 808–809, 811–812, 815, 815–817, 820–821, 891, 903, 910, 931, 963, 984–985
SelectableCanvas.ts	94.39%	91.16%	94.64%	96.62%	1119, 1119–1120, 1123, 1143, 1143, 1201, 1254–1255, 1276, 1284, 1409, 1411, 1413–1414, 518, 698–699, 701–702, 702, 702, 751–752, 813–814, 867–869, 901, 906–907, 934–935
StaticCanvas.ts	96.86%	92.91%	100%	98.61%	1102–1103, 1103, 1103–1104, 1224, 1234, 1288–1289, 1292, 1327–1328, 1405, 1414, 1414, 1418, 1418, 1465–1466, 310–311, 328, 759, 771–772
TextEditingManager.ts	100%	100%	100%	100%
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	93.90%	88.89%	90.91%	97.73%	232, 319, 319, 354
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	5.97%	0%	0%	11.11%	100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
rotate.ts	19.57%	12.50%	50%	21.43%	41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
scale.ts	93.57%	92.94%	100%	93.67%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	130–131, 162–163, 170, 176, 178
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env
browser.ts	84.21%	77.78%	50%	100%	14, 17
index.ts	100%	100%	100%	100%
node.ts	74.07%	33.33%	66.67%	88.89%	27, 31–32, 32, 32, 37
src/filters
BaseFilter.ts	21.62%	23.21%	32%	18.27%	100, 100, 100–101, 108–111, 111, 111–112, 118, 118, 118–121, 139, 157, 171–176,

asturur · 2023-05-13T14:22:06Z

src/color/Color.ts

@@ -279,7 +279,7 @@ export class Color {
   * @return {TRGBAColorSource | undefined} source
   */
  static sourceFromRgb(color: string): TRGBAColorSource | undefined {
-    const match = color.match(reRGBa);
+    const match = color.match(new RegExp(reRGBa, 'i'));


the truth is that the best code is the literal regex inside the function here.
The constants outside the file are an organization thing that we pay with having those in the scope of the library, always initialized.

asturur · 2023-05-13T15:23:49Z

changed my mind.
I ll overcome the readibility with chatgpt aided comments

asturur · 2023-05-13T20:45:56Z

src/color/Color.ts

-      s = parseFloat(match[2]) / (/%$/.test(match[2]) ? 100 : 1),
-      l = parseFloat(match[3]) / (/%$/.test(match[3]) ? 100 : 1);
+      s = parseFloat(match[2]) / 100,
+      l = parseFloat(match[3]) / 100;


s and l are always percentages only.
Non percentages aren't ammitted.

asturur · 2023-05-13T20:53:35Z

At this point this needs some extra test for the new format covered

…nto regex-color-cleanup

asturur · 2023-05-14T12:05:00Z

src/color/Color.ts

   * @return {TRGBColorSource} Hsl color
   */
-  _rgbToHsl(r: number, g: number, b: number): TRGBColorSource {
+  _rgbToHsl(r: number, g: number, b: number, a: number): TRGBAColorSource {


this should probably be an utility like hsltorgb

asturur · 2023-05-14T14:41:30Z

ok i can stop here with this.
Ready to review if some code choice isn't great

asturur · 2023-05-14T18:03:48Z

@Lazauya since you had previously worked with regexes, have a look for suggestions

ShaMan123

This PR is a GREAT cleanup!
Some recommended changes

ShaMan123 · 2023-05-15T07:38:02Z

src/color/Color.ts

-    const [r, g, b] = this.getSource();
-    return `${hexify(r)}${hexify(g)}${hexify(b)}`;
+    const fullHex = this.toHexa();
+    return fullHex.slice(0, 6);


why isn't it prefixed with #?

was like this before, trying to not change behaviour since it doesn't seems important.
I don't know the reasons, we should dig into git history

ShaMan123 · 2023-05-15T07:39:10Z

src/color/Color.ts

-    this.setSource([average, average, average, currentAlpha]);
+    const [r, g, b, a] = this.getSource(),
+      average = Math.round(r * 0.3 + g * 0.59 + b * 0.11);
+    this.setSource([average, average, average, a]);
    return this;


Not very related, but since I am reviewing
Do we want to return a new instance for the toXXX methods?

this would be a possible change, and we can do that outside the cleanup/refactor scope.
Something that also should be done maybe that is part of cleanup is consolidathing this average code in a function util

ShaMan123 · 2023-05-15T07:42:59Z

src/color/Color.ts

@@ -310,14 +251,14 @@ export class Color {
   * @see http://http://www.w3.org/TR/css3-color/#hsl-color
   */
  static sourceFromHsl(color: string): TRGBAColorSource | undefined {
-    const match = color.match(reHSLa);
+    const match = color.match(reHSLa());
    if (!match) {
      return;
    }

    const h = (((parseFloat(match[1]) % 360) + 360) % 360) / 360,


Suggested change

const h = (((parseFloat(match[1]) % 360) + 360) % 360) / 360,

const h = ((parseFloat(match[1]) + 360) % 360) / 360,

i wonder... this seems correct and i noticed was off.
I m trying to think of some non obvious value that could break.
But i prefer to discover in this case and in case fix later because this seems wrong

Ahhh I see.
What if the value is < -360. Now it fails, before it was safe.
Please revert this.
I didn't think of that.

Yeah that is fine, we discovered.

src/color/Color.ts

ShaMan123 · 2023-05-15T08:00:24Z

src/color/constants.ts

This is great!

in my head there was the fun experiment that 150 colors can be represented by 2 digits in base64 encoding. And so that should be possible to reduce this map to have custom hashed keys to which color names resolve univocally. But i m not sure is easy or hard, but for sure would be fun.

So desu, ne?!

ShaMan123 · 2023-05-15T08:01:31Z

src/color/constants.ts

-export const reRGBa =
-  /^rgba?\(\s*(\d{1,3}(?:\.\d+)?%?)\s*,\s*(\d{1,3}(?:\.\d+)?%?)\s*,\s*(\d{1,3}(?:\.\d+)?%?)\s*(?:\s*,\s*((?:\d*\.?\d+)?)\s*)?\)$/i;
+export const reRGBa = () =>
+  /^rgba?\(\s*(\d{0,3}(?:\.\d+)?%?)\s*[\s|,]\s*(\d{0,3}(?:\.\d+)?%?)\s*[\s|,]\s*(\d{0,3}(?:\.\d+)?%?)\s*(?:\s*[,/]\s*(\d{0,3}(?:\.\d+)?%?)\s*)?\)$/i;


does it over complicate reusing some string e.g. \(\s*(\d{0,3}(?:\.\d+)?%?)\s*[\s|,]\s* in a string template?

the issue with that is that i loose the ability to use the literal regex and i have to go with newRegexp.
I started with that if you look at the first commits. I got 400 extra bytes of code and i was pissed off.
I should try again now that i made the regex slightly different.

src/color/util.ts

ShaMan123 · 2023-05-15T08:04:20Z

src/color/util.ts

-  return hexValue.length === 1 ? `0${hexValue}` : hexValue;
-}
+export const hexify = (value: number) =>
+  Math.min(value, 255).toString(16).toUpperCase().padStart(2, '0');


padStart, who knew. 👍

This is exactly where we should verify rounding is not an issue.

ShaMan123 · 2023-05-15T08:05:58Z

test/unit/color.js

+      var oColor = fabric.Color.fromRgba(stringToParse);
+      assert.ok(oColor);
+      assert.ok(oColor instanceof fabric.Color);
+      assert.deepEqual(oColor.getSource(), expectedSource);
+      var oColorUppercase = fabric.Color.fromRgb(stringToParse.toUpperCase());
+      assert.ok(oColorUppercase);
+      assert.ok(oColorUppercase instanceof fabric.Color);
+      assert.deepEqual(oColorUppercase.getSource(), expectedSource);


seems this function can be reused in the hsl loop as well

could but i didn't becuase fromRgb vs fromHsl and didn't want to overcomplicate it. Still better than all different tests.

thought of

function runColorTest({ name, stringToParse, expectedSource }, from, fromAlpha) { var oColor = fromAlpha(stringToParse); assert.ok(oColor); assert.ok(oColor instanceof fabric.Color); assert.deepEqual(oColor.getSource(), expectedSource); var oColorUppercase = from(stringToParse.toUpperCase()); // fromAlpha?? assert.ok(oColorUppercase); assert.ok(oColorUppercase instanceof fabric.Color); assert.deepEqual(oColorUppercase.getSource(), expectedSource); }

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

asturur · 2023-05-15T10:26:13Z

action items:

average function
Revert and test the negative hsl < -360
~~see if i can fit the template in the rgba regex ( hsla not worth )~~ i ll stick to docs
verify output is always trimmed correctly if alpha isn't rounded during parsing

…nto regex-color-cleanup

ShaMan123 · 2023-05-16T23:00:23Z

src/color/util.ts

+ * Calculate the grey average value for rgb and pass through alpha
+ */
+export const greyAverage = (
+  r: number,
+  g: number,
+  b: number,
+  a = 1
+): [average: number, alpha: number] => [
+  Math.round(r * 0.3 + g * 0.59 + b * 0.11),
+  a,


seems alpha doesn't belong here
but I understand you want it the usage to be simple to use (less lines of code)
I suggest then returning a source value.
[greyAvg, greyAvg, greyAvg, alpha]

It is a bit weird like this, but not a big deal.

yes maybe we can return the source format, is an internal util, it can do whatever it needs to do to make the code efficient

we can make it from source to source.
Easier.

asturur · 2023-05-18T22:35:58Z

ok toHex and toHexa had a decimal bug, but now is forced as rounded

asturur · 2023-05-18T22:50:52Z

I think i addressed all comments

asturur added 2 commits May 13, 2023 14:01

not sure is a cleanup

c26f998

not sure is a cleanup

9792c44

asturur commented May 13, 2023

View reviewed changes

asturur added 4 commits May 13, 2023 17:55

lots of comments

8db4286

lots of comments and extended regexes

048eb28

removed extra console log

453248f

color code cleanup

a962ce1

asturur commented May 13, 2023

View reviewed changes

asturur added 4 commits May 13, 2023 22:47

color code cleanup

b3bca21

even better

3eb2bf7

added changelog

b445f9c

Merge branch 'master' into regex-color-cleanup

f17c216

asturur changed the title ~~chore() Regex color cleanup~~ feat(Color) Regex color cleanup and new color format support May 14, 2023

asturur added 6 commits May 14, 2023 10:44

Merge branch 'master' into regex-color-cleanup

979bfb3

in the process of changing tests, pause needed

29a5598

Merge branch 'regex-color-cleanup' of github.com:fabricjs/fabric.js i…

5a5c134

…nto regex-color-cleanup

ok seen those

4d15b1c

more test and fixed floats

89ccb5f

hopefully better code

f3eadd8

asturur changed the title ~~feat(Color) Regex color cleanup and new color format support~~ feat(Color) Regex and color code cleanup and new color format support May 14, 2023

hopefully better code

ed671f0

asturur commented May 14, 2023

View reviewed changes

converted tests

77d2dbb

asturur force-pushed the regex-color-cleanup branch from 1ad6d00 to 77d2dbb Compare May 14, 2023 14:26

asturur added 2 commits May 14, 2023 16:38

moved method to util

d4e98a5

missed lint

649ba3c

asturur requested review from ShaMan123 and melchiar May 14, 2023 14:41

ShaMan123 reviewed May 15, 2023

View reviewed changes

asturur and others added 6 commits May 15, 2023 11:56

Update src/color/Color.ts

02f0c5a

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Update src/color/constants.ts

2406fa3

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Update src/color/constants.ts

e3bfa4c

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Update src/color/constants.ts

b648cb4

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Update src/color/util.ts

c78c568

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Update src/color/constants.ts

43a2d1c

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

asturur added 3 commits May 16, 2023 10:33

save those

210a24a

Merge branch 'regex-color-cleanup' of github.com:fabricjs/fabric.js i…

fd75870

…nto regex-color-cleanup

test with negative and revert

4b3693f

asturur force-pushed the regex-color-cleanup branch from 575a3c5 to 4b3693f Compare May 16, 2023 09:56

ShaMan123 previously approved these changes May 16, 2023

View reviewed changes

Merge branch 'master' into regex-color-cleanup

ed121bc

asturur dismissed ShaMan123’s stale review via ed121bc May 18, 2023 22:20

asturur added 3 commits May 19, 2023 00:25

grey average redone

d90d07a

ensure hexify is rounder

713a425

added test

362879f

asturur merged commit 23999dc into master May 18, 2023
18 checks passed

asturur deleted the regex-color-cleanup branch May 18, 2023 22:51

asturur mentioned this pull request May 19, 2023

Version 6.0.0-beta6 #8935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Color) Regex and color code cleanup and new color format support #8916

feat(Color) Regex and color code cleanup and new color format support #8916

asturur commented May 13, 2023 •

edited

github-actions bot commented May 13, 2023 •

edited

github-actions bot commented May 13, 2023 •

edited

asturur May 13, 2023

asturur commented May 13, 2023

asturur May 13, 2023

asturur commented May 13, 2023

asturur May 14, 2023

asturur commented May 14, 2023

asturur commented May 14, 2023

ShaMan123 left a comment •

edited

ShaMan123 May 15, 2023

asturur May 15, 2023

ShaMan123 May 15, 2023 •

edited

asturur May 15, 2023

ShaMan123 May 15, 2023

asturur May 15, 2023

ShaMan123 May 15, 2023 •

edited

asturur May 15, 2023

ShaMan123 May 15, 2023

asturur May 15, 2023 •

edited

ShaMan123 May 15, 2023

ShaMan123 May 15, 2023

asturur May 15, 2023

ShaMan123 May 15, 2023

asturur May 15, 2023

ShaMan123 May 15, 2023

asturur May 15, 2023

ShaMan123 May 15, 2023 •

edited

asturur commented May 15, 2023 •

edited

ShaMan123 May 16, 2023

asturur May 17, 2023

asturur May 17, 2023

asturur commented May 18, 2023

asturur commented May 18, 2023

	const h = (((parseFloat(match[1]) % 360) + 360) % 360) / 360,
	const h = ((parseFloat(match[1]) + 360) % 360) / 360,

feat(Color) Regex and color code cleanup and new color format support #8916

feat(Color) Regex and color code cleanup and new color format support #8916

Conversation

asturur commented May 13, 2023 • edited

Motivation

Changes

Gist

In Action

github-actions bot commented May 13, 2023 • edited

github-actions bot commented May 13, 2023 • edited

Choose a reason for hiding this comment

asturur commented May 13, 2023

Choose a reason for hiding this comment

asturur commented May 13, 2023

Choose a reason for hiding this comment

asturur commented May 14, 2023

asturur commented May 14, 2023

ShaMan123 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 May 15, 2023 • edited

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

ShaMan123 May 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur May 15, 2023 • edited

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

ShaMan123 May 15, 2023 • edited

Choose a reason for hiding this comment

asturur commented May 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented May 18, 2023

asturur commented May 18, 2023

asturur commented May 13, 2023 •

edited

github-actions bot commented May 13, 2023 •

edited

github-actions bot commented May 13, 2023 •

edited

ShaMan123 left a comment •

edited

ShaMan123 May 15, 2023 •

edited

ShaMan123 May 15, 2023 •

edited

asturur May 15, 2023 •

edited

ShaMan123 May 15, 2023 •

edited

asturur commented May 15, 2023 •

edited