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