[FEATURE] Convert legacy color notation to modern css 4 notation

[8ctopus]

Hello guys, happy holidays :) Please don't review the PR until after the festivities, I'm just not that type of person.

This PR draft aims at converting the legacy color notation: rgba(0,128,0,.7) into the modern variant rgba(0 128 0/.7).

I'm no css specialist so your feedback is going to be welcome.

There are no specific tests yet, and there are improvements that can be made, I just want to get your approval before I polish things up.

Question:

• should we also convert the rgba notation to rgb? my understanding is that rgba is deprecated. I also noticed that the current code does the opposite:

a {
   color: rgb(0 0 0/50%);
}

into this:

a {
   color: rgba(0 0 0/50%);
}

[oliverklee]

So I think this should be controllable via an OutputFormat option similar to setRGBHashNotation(), with the default being to use the legacy syntax (so there are no breaking changes). For the next major release we can change the default.

Let's do this.

[coveralls]

coverage: 70.316% (+1.1%) from 69.191% when pulling 44fbf83d168b45538636bf75e3e67999e7db030c on 8ctopus:improve-color into 0ae1fde22bc2f66363ebdd77095e6f1d35118bc8 on MyIntervals:main.

[8ctopus]

Here's the latest update following your feedback. I didn't implement the tests specific to the new OutputFormat property yet, as I prefer to wait for the concept approval first.

[oliverklee]

@8ctopus Thanks!

[8ctopus]

I updated to the best of my knowledge, but it's still not there yet. I will need your input for colors that include variables such as:

a {
  color: rgb(var(--r), 119, 0);
}

should it be converted to:

a {
  color: rgb(var(--r) 119 0);
}

because it's handled as function, not as a color.

[JakeQZ]

Apologies for the delayed reply; I only just found this when looking through open issues.

I will need your input for colors that include variables such as:

a {
  color: rgb(var(--r), 119, 0);
}

should it be converted to:

a {
  color: rgb(var(--r) 119 0);
}

That could be converted as such. But vars are messy. They are effectively a direct substitution in the property value without any context.

You could have a var that provides the red and blue values, but not the green, e.g.

a {
  --rg: 100 50;
  color: rgb(var(--rg) 200);
}

or

a {
  --rg: 100, 50;
  color: rgb(var(--rg), 200);
}

These both render in a nice shade of purple.

But if we do

a {
  --rg: 100, 50;
  color: rgb(var(--rg) 200);
}

or

a {
  --rg: 100 50;
  color: rgb(var(--rg), 200);
}

it does not work, because the component separators become mixed, and that's not valid.

The phrase "can of worms" springs to mind.

I think the syntax used will have to be recorded at parsing time, and if there's a var() covering more than one component, then only that syntax can be used for render(). It's quite probable that the library as it is doesn't get it right in all of the cases above. Then there's the case of what happens when a user changes the component values via the API.

I think this is quite a 'biggie' to get properly solved. Use of var() in other functions may also have issues.

We appreciate you opening this can of worms, as it highlights other issues that probably need addressing :)

That said, it may still be possible to proceed with this PR, and fix the other issues (whatever they are) separately.

[JakeQZ]

I think the syntax used will have to be recorded at parsing time, and if there's a var() covering more than one component, then only that syntax can be used for render(). It's quite probable that the library as it is doesn't get it right in all of the cases above. Then there's the case of what happens when a user changes the component values via the API.

I've looked at the code now, and see that if a colour function (like rgb()) includes a var() expansion, it will be represented as a generic CSSFunction, which will be rendered comma-separated. If there are no var() expansions, it will be represented as a Color, which can be rendered in the modern syntax if desired.

That said, it may still be possible to proceed with this PR, and fix the other issues (whatever they are) separately.

On the basis of the above, I don't think we need to be concerned about var() for Color::render(), because Color will not contain var()s. So the tests can just focus on whether the modern or legacy syntax is selected in the OutputFormat, and we can move forward with this in isolation.

Apologies that my previous comment made some assumptions which were not true (it seems we are handling var()s reasonably well by defaulting to a generic CSSFunction when they are encountered).