[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%);
}

[JakeQZ]

Hello guys, happy holidays :)

Thanks 🎄

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

Some users may want the rendered output to stay in the legacy notation for compatibility or other reasons. (For example the version of markdown used by GitHub only recognizes the legacy syntax for showing the actual colour alongside the value in backticks.)

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.

there are improvements that can be made

The simplest way of implementing this is for Color::shouldRenderInModernSyntax() to immediately return true if the modern syntax option is enabled in the OutputFormat.

• should we also convert the rgba notation to rgb? my understanding is that rgba is deprecated.

It's not currently deprecated; the two are synonymous, but it is now "recommended" to use rgb().

I think if the modern syntax option is enabled then rgb() should always be used, but otherwise the current behaviour should be retained (previously rgba() was required when there is an alpha value).

I also noticed that the current code does the opposite:

That's not quite the case. It uses rgb() if there's no alpha value, but rgba() if there is one.

There are no specific tests yet

Note that this change will (should) similarly affect hsl(), so some tests would also be needed for those colour functions.

@oliverklee, WDYT?

[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 44fbf83 on 8ctopus:improve-color
into 0ae1fde 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!

[JakeQZ]

I think if usesModernColorSyntax is enabled, the modern syntax should be used in all cases (i.e. also for hsl()).

Otherwise looks fine pending the addition of tests.

We'd also want an entry in CHANGELOG.md.

[JakeQZ]

I think this should be the first test. The colorFunctionMayHaveMixedValueTypes() determines if the modern syntax might be needed (as some combinations of values can't be represented in the legacy syntax). But if the user has selected the modern syntax, it should always be used (the modern syntax can represent all combinations of values).

As this would be a quicker test than hasNoneAsComponentValue(), it would be more optimal having it first. It can be combined into the same if statement.

[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).