RFR: JDK-8292276 : Add named colors from CSS Color Module Level 4 [v28]

[Alexey Ivanov]

On Sun, 12 Feb 2023 08:59:42 GMT, ScientificWare <duke at openjdk.org> wrote:

>> This is referenced in Java Bug Database as
>> - [JDK-8292276 : Add named colors from CSS Color Module Level 4](https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8292276)
>> 
>> This is tracked in JBS as 
>> - [JDK-8292276 : Add named colors from CSS Color Module Level 4](https://bugs.openjdk.java.net/browse/JDK-8292276)
>> 
>> Adds missing color names, defined by CSS Level 4, in CSS.java :
>> CSS Color Module Level 4
>> W3C Candidate Recommendation Snapshot, 5 July 2022
>> [7.1 Named Colors](https://www.w3.org/TR/css-color-4/#named-color)
>> 
>> Designed from : [ScientificWare JDK-8292276 : Add named colors from CSS Color Module Level 4](https://github.com/scientificware/jdk/issues/12)
>
> ScientificWare has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Delete configure
>   
>   Delete configure from the pull request.

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 1416:

> 1414:             }
> 1415:             // sometimes get specified without leading #
> 1416:             return hexToColor(str);

I can't see the definitions of `parseRGB`, `parseRGBA`, `hexToColor`.

src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 3780:

> 3778: 
> 3779:     static int baseFontSizeIndex = 3;
> 3780: }

Please preserve the new line char in the end of files.

src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java line 1019:

> 1017:      * Converts a color string such as "RED",  "rgb(r g b)",
> 1018:      * "rgba(r g b a)" or "#NNN", "#NNNN", "#NNNNNN",
> 1019:      * "#NNNNNNNN" to a Color.

[CSS Color Level 4, The RGB functions](https://www.w3.org/TR/css-color-4/#rgb-functions) specifies that both `rgb` and `rgba` have the same syntax and both accept `<alpha-value>`.

src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java line 1025:

> 1023:      * and rgb or rgba HTML3.2 strings.
> 1024:      * Otherwise, it will return null.
> 1025:      * This method is case-insensitive.

Suggestion:

     * Note: This will only convert strings which use any of the following:
     * <ul>
     *   <li><a href="https://www.w3.org/TR/css-color-4/#named-colors">named colors</a>,
     *   <li>hex color notation starting with {@code #} followed by 3, 4, 6, or 8 hexadecimal digits,
     *   <li>`rgb()` and `rgba()` functions
     * </ul>
     * as specified by the <a href="https://www.w3.org/TR/css-color-4/">CSS Color Module Level 4</a>.
     * Otherwise, it will return null.
     * <p>
     * This method is case-insensitive.

src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java line 1027:

> 1025:      * This method is case-insensitive.
> 1026:      * <p>
> 1027:      * The following code defines instances of the same color :

Suggestion:

     * The following code defines instances of the same color:

There should be no space before the colon.

src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java line 1030:

> 1028:      * {@snippet lang="java" :
> 1029:      *   import java.awt.Color;
> 1030:      *   import javax.swing.text.html.StyleSheet;

Probably, the imports could be omitted for the sake of brevity.

test/jdk/javax/swing/text/html/CSS/MissingColorNames.java line 62:

> 60:             passed = false;
> 61:             result.append(" [<name-color> keyword(s) missing]");
> 62:         }

Does it make sense to add a couple of named colors, especially from a _more_ extended set?

test/jdk/javax/swing/text/html/CSS/MissingColorNames.java line 70:

> 68:             passed = false;
> 69:             result.append(" [<rgb()> or <rgba()> values not case insensitive]");
> 70:         }

I'd rather see the test demonstrates all the supported syntax for defining color using `rgb()` and `rgba()` functions as well as for `#N+` notations so that we're sure they work as specified by the CSS specification.

It could be split into several test cases which use `StyleSheet.stringToColor` method for parsing.

I know this would look like a unit test than a regression test. Yet you still have to write one to verify your code works as expected, and the unit test has an intrinsic value: if the code is modified for whatever reason, it ensures all the cases still produce the expected result.

-------------

PR: https://git.openjdk.org/jdk/pull/9825