jdk JDK-8292276 : Missing color names in CSS.

trafficstars

This is referenced in Java Bug Database as

JDK-8292276 : Missing color names in CSS

This is tracked in JBS as

JDK-8292276 : Missing color names in CSS

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

Designed from : ScientificWare JDK-8292276 : Missing color names in CSS

Progress

[ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue

Issue

JDK-8292276: Missing color names in CSS.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9825/head:pull/9825
$ git checkout pull/9825

Update a local copy of the PR:
$ git checkout pull/9825
$ git pull https://git.openjdk.org/jdk pull/9825/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9825

View PR using the GUI difftool:
$ git pr show -t 9825

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9825.diff

Aug 10 '22 20:08 scientificware

:wave: Welcome back scientificware! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Aug 11 '22 04:08 bridgekeeper[bot]

@scientificware The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Aug 11 '22 05:08 openjdk[bot]

First and foremost, you need to add JBS id 8292276 into the PR title. Also, please add a jtreg testcase which should fail before your fix and pass after.I guess JBS has a reproducer which can be made into a jtreg testcase, of which you can see several examples in jdk/test/javax/swing directory.

Aug 12 '22 07:08 prsadhuk

Webrevs

Aug 12 '22 08:08 mlbridge[bot]

Hey there. The current implementation creates a new Color object for each invocation of stringToColor (with the exception of "" because we want to keep developers on their toes). Using a Map will not create new Color objects for each invocation, which may explain why your results show Map as the most performant. This is technically a change in behavior, and technically not wanted.

To get around this, you can do new Color(c.getRed(), c.getGreen(), c.getBlue()) from the map, or--what I would prefer--to use an enhanced switch statement to create the colors.

One thing I'd request is to have stringToColor return in the branches, rather than setting the placeholder Color color; object. Things like this irk me. As in (using the map):

static Color stringToColor(String str) {
        if (str == null) {
            return null;
        } else if (str.length() == 0) {
            return Color.black;
        } else if (str.startsWith("rgb(")) {
            return parseRGB(str);
        } else if (str.startsWith("rgba(")) {
            return parseRGBA(str);
        } else if (str.charAt(0) == '#') {
            return hexToColor(str);
        } else if (colorNamed.containsKey(str.toLowerCase(Locale.ROOT))) {
            return colorNamed.get(str.toLowerCase(Locale.ROOT));
        } else {
            return hexToColor(str);
        }
    }

In general, good PR. I'd be interested to know the perf results when the behavior is unchanged, and if the enhanced switch would win out.

Aug 16 '22 00:08 SWinxy

@SWinxy Thanks for comments. ~~After reading them, I realize the Big Mistake !~~ ~~I Shouldn't create a new object but rather return an existing one.~~

~~That was in my intention. All the code is already available here, I wrote it because I expected to move this logic to Color.java and return an Color instance like the Color.black in the existing code.~~

~~All color declarations are ready. ~~

~~The Map will be something like :~~

static TreeMap<String, Color> colorNamed = new TreeMap<String, Color>(
        Map.ofEntries(
            Map.entry("aliceblue", ALICE_BLUE),
            ...
            Map.entry("yellowgreen", YELLOW_GREEN)
        );

~~Do you validate this approach ?~~

I think, I misinterpreted your message !

But could you tell me witch method you prefer, I wrote 4 for tests at https://github.com/scientificware/jdk/issues/12

One classic with if ... then ... else
Another with switch ... case
One with TreeMap.
And le last with Map.

Aug 16 '22 00:08 scientificware

@SWinxy

I'd be interested to know the perf results when the behavior is unchanged, and if the enhanced switch would win out. The results are also in my repository, the current implementation is

five times slower than the worst of other implementations
and eight times slower than the switch case implementation.

This concern only named Colors. For RGB and Hex, the code is the same than I think there is no change. What I test :

1 000 repetitions of 1 000 fixed random list of color names.
All tested implementations give the same result for only one RGB, RGBA and Hex because their logics are unchanged.

Aug 16 '22 02:08 scientificware

That's a bit sus for the enhanced switch statement to be significantly slower (7.5x more time than the map). Your methodology still shows that you aren't creating new objects for each invocation and returning the same objects each time. Is that accurate to your performance table?

Aug 16 '22 23:08 SWinxy

Yes @SWinxy, you're right. Map and TreeMap implementations return the same object. Therefore comparisons with the current implemention or switch case solution are not relevant. I going to add a test result with Map + your workaround.

Presently, the switch case solution is the best in respect of the current behaviors you want to preserve.

I integrated all your comments but yesterday, I was busy with the test case and other propositions that could content you. I take time to found the best solution because there are other places in swing html package where such optimisation would be interresting, especially in view factories.

Aug 17 '22 06:08 scientificware

@SWinxy All my apologies, I skipped testing CASE + RGB implementation. I posted it but not tested (previous results were about CASE + Hex. I updated my table including your workaround suggestion.

The results of CASE + RGB are still under Map.
But Map + your workaround gives good results.

All the stringToColor codes are available, feel free to make a test.

Aug 17 '22 11:08 scientificware

I mean I think I would prefer the switch over a map (it looks nicer to me). My crude tests showed that the switch is indeed slower, breaking my conception that switch statements are the peak of performance. Other than this request, I have no further comments.

Aug 22 '22 00:08 SWinxy

@SWinxy Thanks for your reviews and confirmation tests. Do you notice that the switch ... case is also two times slower than my "handmade" binary tree using if ... else ! A problem in the implementation of switch ... case ?

Aug 22 '22 21:08 scientificware

Swing supports CSS1 only, it defines color names for 16 colors:

The suggested list of keyword color names is: aqua, black, blue, fuchsia, gray, green, lime, maroon, navy, olive, purple, red, silver, teal, white, and yellow. These 16 colors are taken from the Windows VGA palette, and their RGB values are not defined in this specification.

Would the addition of color names from CSS4 give a wrong impression that Swing implements CSS4?

Sep 08 '22 14:09 aivanov-jdk

@aivanov-jdk I complete my first answer to try to release your objection :

there is no CSS Level 4. CSS is no more a monolithic specification. It's a set of modules with different Levels at their own pace.
implementation of recommendations could be partial and different from an application to another one. So the documentation can detail more precisely what is supported.
cherry-picking some of these recommandations respects this philosophy.
I'am aware that such PR was refused few years ago for Color.java. CSS Color Level 4 doesn't encourage usage of these names. They are only defined because they are widely used. Sometimes, it's more confortable for users and avoid basic preoccupations. Technically, it's also avoid that application wastes time to search a unimplemented keyword.
For the same reasons, I wish submit a PR about 3, 4, 6 or 8 digits hex coded colors (following CSS Color Level 4 recommendations).
and having this one in project for another part of this package (https://bugs.openjdk.org/browse/JDK-8289722).

Sep 09 '22 02:09 scientificware

I've submitted a test job with this fix.

Should it be an enhancement?

Sep 28 '22 16:09 aivanov-jdk

Should it be an enhancement?

This PR
- corrects or changes Orange value in (bug ?)
- adds new values + improve performance (enhancements) So I think it's rather an enhancement. Why is it important to qualifying this ?

Could we consider once more times @SWinxy 's Aug 16 comment about color instance ?

...
        String color = colorNames.get(strlc);
        if (color != null) {
            return hexToColor(color);
        }
        // sometimes get specified without leading #
        return hexToColor(str);
    }
}

private static final Map<String, String> colorNames =
    Map.ofEntries(
        Map.entry("aliceblue", "#f0f8ff")),
        Map.entry("antiquewhite", "#faebd7"),
        Map.entry("aqua", "#00ffff"),
...

Another way to proceed would be that Map returns the int value of the color, then we can return a new Color(value).

Sep 28 '22 21:09 scientificware

Could we consider once more times @SWinxy 's Aug 16 comment about color instance ?

Are you talking about keeping the old behavior? If so, your proposed solution looks to work (also reminder that .contains(Object) exists).

Sep 28 '22 22:09 SWinxy

Are you talking about https://github.com/openjdk/jdk/pull/9825#issuecomment-1216011827?

@SWinxy Yes, only because the result is publicly exposed : stringToColor is also used by javax.swing.text.html.StyleSheet stringToColor method to publicly return the Color Object. It seems unique, I didn't see any other usages.

also reminder that .contains...(Object) exists

I saw these methods but in our case we need a result anyway. Using containsKey(Object) implies a second request to get the value ?

Sep 28 '22 22:09 scientificware

Are you talking about #9825 (comment)?

@SWinxy Yes, only because the result is publicly exposed : stringToColor is also used by javax.swing.text.html.StyleSheet stringToColor method to publicly return the Color Object. It seems unique, I didn't see any other usages.

also reminder that .contains...(Object) exists

I saw these methods but in our case we need a result anyway. Using containsKey(Object) implies a second request to get the value ?

I thought we had closed this discussion:

A Color object is immutable, it is safe to return it as it's stored in the internal map. The fact that the previous implementation returned a new instance each time stringToColor was called is an implementation detail, no one should have relied on it.

@scientificware is right, we need the result anyway, so the code in the PR is more efficient: https://github.com/openjdk/jdk/pull/9825/files#diff-e2c9b23b1844fa877fb1c4f048a8a8b85cd21d146f963837e039ab20b9560fe7R1409-R1412

It looks up a color in the map once whereas

        if (colorNamed.containsKey(str.toLowerCase(Locale.ROOT))) {
            return colorNamed.get(str.toLowerCase(Locale.ROOT));
        }

performs the look up twice: for containsKey and for get.

Sep 29 '22 10:09 aivanov-jdk

Should it be an enhancement?

corrects or changes Orange value in (bug ?)

It's probably a bug. 'Orange' shouldn't have been there. Both HTML 3.2 and CSS 1 declare only 16 colors from Windows VGA palette.

adds new values + improve performance (enhancements)

So I think it's rather an enhancement.
Why is it important to qualifying this?

I've changed the type of the issue from bug to enhancement. This PR changes the specified behaviour of a public method StyleSheet.stringToColor. It is explicitly specified, “This will only convert the HTML3.2 color strings or a string of length 7; otherwise, it will return null.” Thus, the current behaviour is not a bug. What you propose is an enhancement.

I've submitted a test job with this fix.

And one JCK (Java Compatibility Kit) test fails, the one which verifies the statement that I quoted above.

You need to update the specification of StyleSheet.stringToColor. Once we agree on the text in this PR, I'll submit a CSR for you. This PR can be integrated only after the CSR is approved.

Shall I edit the subject of the issue to be more specific? “Add named colors from CSS Color Module Level 4”?

Sep 29 '22 10:09 aivanov-jdk

/csr

Sep 29 '22 10:09 aivanov-jdk

@aivanov-jdk has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@scientificware please create a CSR request for issue JDK-8292276 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Sep 29 '22 10:09 openjdk[bot]

Shall I edit the subject of the issue to be more specific? “Add named colors from CSS Color Module Level 4”?

Yes, you may. Thanks.

... please create a CSR request for issue JDK-8292276 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Do I need an access to JBS ? I don't have a JBS user account.

Sep 29 '22 14:09 scientificware

... please create a CSR request for issue JDK-8292276 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Do I need an access to JBS ? I don't have a JBS user account.

Yes, you do.

I said, I would submit a CSR for you after we agree on the updated javadoc for StyleSheet.stringToColor.

Sep 29 '22 14:09 aivanov-jdk

I said, I would submit a CSR for you after we agree on the updated javadoc for StyleSheet.stringToColor.

Sorry, I didn't realize the message was from the bot.

"JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color" #10317 could affect this specifications too.

Sep 29 '22 16:09 scientificware

"JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color" #10317 could affect this specifications too.

That's right. So you should take it into account. The updated javadoc should reference the color notation in CSS Color Module Level 4.

Sep 30 '22 17:09 aivanov-jdk

Shall I edit the subject of the issue to be more specific? “Add named colors from CSS Color Module Level 4”?

Yes, you may. Thanks.

I have updated the subject in JBS. Please edit the title of this PR accordingly.

Sep 30 '22 17:09 aivanov-jdk

@aivanov-jdk A proposition to update the specification of StyleSheet.stringToColor.

  /**
   * Converts a color string such as "RED",  "rgb(r g b)",
   * "rgb(r g b / a)" or "#NNN", "#NNNN", "#NNNNNN",
   * "#NNNNNNNN" N digits in radix-16 to a Color.
   * <p>
   * Note : This will only convert string colors using names, rgb function or 3, 4, 6, 8
   * digit hexadecimal notations as they are listed and described in
   * the CSS Color Module Level 4;
   * otherwise, it will return null.
   * This method is case-insensitive.
   * <p>
   * The following code defines instances of the same color :
   * {@snippet lang="java" :
   *   import java.awt.Color;
   *   import javax.swing.text.html.StyleSheet;
   *   StyleSheet styleSheet = new StyleSheet();
   *   // An opaque lightseagreen
   *   Color color0 = styleSheet.stringToColor("Lightseagreen");
   *   Color color1 = styleSheet.stringToColor("LIGHTSEAGREEN");
   *   Color color2 = styleSheet.stringToColor("rgb(32 178 170)");
   *   Color color3 = styleSheet.stringToColor("rgb(12.5% 69.8% 66.7%)");
   *   Color color4 = styleSheet.stringToColor("#20b2aa");
   *   // A slightly-transparent lightseagreen
   *   Color color5 = styleSheet.stringToColor("rgb(32 178 170 / 0.8)");
   *   Color color6 = styleSheet.stringToColor("rgb(32 178 170 / 80%)");
   *   Color color7 = styleSheet.stringToColor("rgb(12.5% 69.8% 66.7% / 0.8)");
   *   Color color8 = styleSheet.stringToColor("rgb(12.5% 69.8% 66.7% / 80%)");
   *   Color color9 = styleSheet.stringToColor("#20b2aacc");
   * }
   * <p>
   * @param string color, string such as "RED" or "rgb(r g b)",  "rgb(r g b / a)"  or "#NNN",
   * "#NNNN", "#NNNNNN",
   * #NNNNNNNN". N digits in radix-16. r, g, b decimal numbers between 0 and 255 or
   * percents of 255. a decimal number in range 0 to 1 or a percent.
   * @return the color
   */

Oct 05 '22 14:10 scientificware

@scientificware This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Nov 29 '22 19:11 bridgekeeper[bot]

The updated javadoc depends on https://bugs.openjdk.org/browse/JDK-8292276 implementation. This work is in progress.

Nov 30 '22 18:11 scientificware

jdk jdk copied to clipboard

JDK-8292276 : Missing color names in CSS.

Progress

Issue

Reviewing

Webrevs

jdk
jdk copied to clipboard