JDK-8292276 : Add named colors from CSS Color Module Level 4 #9825
Conversation
|
👋 Welcome back scientificware! A progress list of the required criteria for merging this PR into |
|
@scientificware The following label will be automatically applied to this pull request:
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. |
|
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. |
scientificware
changed the title
scientificware
changed the title
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) - Updates, Copyright year. - Adds relative imports. - Replaces, if ... then ... else statements with a Map called "colorNamed". Code is more readable and easy to maintain. After tests, TreeMap seems slower than Map. Results are available at #12 (comment). Warning : The Previous JDK CSS Orange Color is different from CSS Color Recommendation.
|
Hey there. The current implementation creates a new To get around this, you can do One thing I'd request is to have 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. |
|
@SWinxy Thanks for comments.
~~All color declarations are ready. ~~
I think, I misinterpreted your message ! But could you tell me witch method you prefer, I wrote 4 for tests at scientificware#12
|
This concern only named Colors. For RGB and Hex, the code is the same than I think there is no change.
|
Adds a jtreg test case that fails before JDK-8292276 patch and passes after. Test the Cyan color name. Cyan is the missing color name that originates the PR JDK8292276 : Missing Color NamesIn CSS. Cyan name, as most color names Colors defined in CSS Color Module Level 4, is not referenced in CSS.java. This test fails, if getAttribute doesn't return a cyan Color Object. When a color name is missing getAttribute returns a black Color Object.
|
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? |
|
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. |
|
@SWinxy All my apologies, I skipped testing CASE + RGB implementation. I posted it but not tested (previous results were about CASE + Hex.
All the stringToColor codes are available, feel free to make a test. |
|
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. |
|
@SWinxy Thanks for your reviews and confirmation tests. |
- by correcting orange definition,
- by adding 132 missing color names,
- including transparent keyword
- but excluding currentcolor keyword.
- by treating RGB and Hex notations in insensitive case mode.
This commit modifies stringToColor method which has no modifier, so only usages are limited to :
- its Class.
- The inner Class ColorValue also without modifier uses it through the method to return the Color Object
- its Package.
- stringToColor is also used by javax.swing.text.html.StyleSheet stringToColor method to publicly return the Color Object.
- conclusion : since Color Object is publicly exposed, one can't change method behavior. This preventing to return a constant Color excepted for the existing String.isEmpty argument.
Refactoring resolving color value and align it with 5.7. Resolving Values Values) will be treated in another PR. Extends JDK-8149631 rgb(...) CSS color values are not parsed properly.
Un caractère de retour à la ligne s'est immiscer dans le nom du dossier.
openjdk
bot
added
rfr
|
Swing supports CSS1 only, it defines color names for 16 colors:
Would the addition of color names from CSS4 give a wrong impression that Swing implements CSS4? |
|
@prrace and @aivanov-jdk #15262, #10317 and #9825 (this PR) are independant. That's why I planed to treat them separatly. It is also easier to test performance. Yes, all affect the result of the
In jshell, compare the following results : May be we can write it like that : |
Not sure I see that to be the case.
They are not just "comments", they are Java SE API specification.
But the one in Stylesheet is merely a wrapper around the one in CSS Basically at the very least you need to merge the two we asked you to merge. === I think it best to WITHDRAW that other PR and absorb it in this PR Also you should in the doc include the spec link. Initially, it looked as if handling these issues separately was a good idea. Now that we know that the specification of a public needs updating, which requires a CSR, it is better to have everything ready: named colors, hex-parsers, rgb() and rgba() parsers. The spec refers to all the cases. If one is missing, the method is not compliant to its specification. Amending the implementation one by one would require updating the spec and reviewing the CSR. Thus, having everything ready in one larger PR makes more sense ===== |
# Conflicts: # src/java.desktop/share/classes/javax/swing/text/html/CSS.java
|
|
|
/csr |
|
Sorry, slow in getting back to this. I ran our test suite against this change. |
There was a problem hiding this comment.
So there's a pre-existing mess here and I hope we can address it here
since it clearly needs addressing.
The existing spec. of StyleSheet.stringToColor() is clear that if what is
passed in doesn't match what it supports - it returns null.
/**
* Converts a color string such as "RED" or "#NNNNNN" to a Color.
* Note: This will only convert the HTML3.2 color strings
* or a string of length 7;
* otherwise, it will return null.
*
* @param string color string such as "RED" or "#NNNNNN"
* @return the color
*/
public Color stringToColor(String string) {
return CSS.stringToColor(string);
}
However since at least JDK 7 the CSS.stringToColor() method that is called says
/**
* Convert a color string such as "RED" or "#NNNNNN" or "rgb(r, g, b)"
* to a Color.
*/
And indeed it does.
But the HTML 3.2 spec says NOTHING about that last format.
So this has been out of compliance forever.
Here's a program run with JDK 8 that per spec should return null f
import javax.swing.text.html.StyleSheet;
public class StringToColor {
public static void main(String[] args) {
StyleSheet ss = new StyleSheet();
Color c = ss.stringToColor("rgb(50 50 50)");
System.out.println(c);
}
}
~/jdk8/Contents/Home/bin/java StringToColor
java.awt.Color[r=50,g=50,b=50]
Next in this fix https://bugs.openjdk.org/browse/JDK-8256019 for JDK 17
"rgba" support was added making it even further out of compliance.
The test and use case there did not make any direct use of StyleSheet.
it just passed HTML text to Swing.
So I expect this was overlooked (although there's a comment in the bug
report that this didn't appear to be specified which isn't true).
Perhaps the same is how rgb() got in too.
We need to rectify as much of this as we can.
I don't propose to go back to earlier releases and undo changes
to match the spec. and we can't go back and update the spec. either.
So its probably something we need to live with.
The update proposed here should go a very long way to addressing
this disconnect, bringing the spec. into alignment with existing
implementation.
But I think rather than just referencing the CSS spec, it should
be explicit in what it supports unless it implements EVERY SINGLE THING
that spec says. And clearly it does not !
Now we also have this PR #15262
which so far as I can tell is intended to bring the CSS support for rgb/rgba
into alignment with the CSS spec. So clearly that PR needs to be merged
into this one. I KNOW we already merged another PR into here but this
whole thing needs to be ONE PR.
We can't put spec implementation later and we can't put implementation
which (further!) contradicts the spec.
Then when we get all of these together I need to see that it really
implements what the spec. says and if it doesn't we'll need to
call out what is supported.
And either way the spec. would benefit from at least some examples
of each case, as to what are valid values for r,g,b and a.
|
PR JDK-8294090 #15262 is closed. Its content will be merged in this PR after applying requiered changes. |
- rename the list of string test. CSS.java : - correct a wrong index.
- Add a subset of RGB and RGBA tests.
- Rename the color which doesn't belong to CSS-COLOR-4 specification.
|
Hex3468DigitsColor.java, this test is no longer useful. All tests are now included in MissingColorNames.java. I'll remove it. |
| // Remove the next line after merging JDK-8294090 : Aligns the CSS <rgb()> and <rgba()> function behaviours #15262. | ||
| {"rgb(12 24 200)", "ff0c18c8"} | ||
| }; | ||
| } |
There was a problem hiding this comment.
Please keep an empty line at the end of the files.
- Catches the Color Illegal Argument Exception and appends it to the result summary. - Uncomments rgb tests.
- Update source code declaration in documentation comments and specifications. - Typo in an argument.
|
| * <li><a href="https://www.w3.org/TR/css-color-4/#rgb-functions">`rgb()` and `rgba()` | ||
| * functions</a></li> | ||
| * </ul> | ||
| * as specified by the <a href="https://www.w3.org/TR/css-color-4/">CSS Color Module Level 4</a>. |
There was a problem hiding this comment.
Since this PR : https://github.com/openjdk/jdk/pull/13360/files
We have the new {@SPEC .. } tag whereever we reference an external spec.
So you should add it here too.
There was a problem hiding this comment.
Also, I think that in the public class doc of CSS.java which talks about CSS1, you need to add a comment
"Additionally, the color strings from CSS 4 are supported" and again point to the spec.
| public static void main(String[] args) { | ||
| StringBuilder result = new StringBuilder(); | ||
| boolean passed = true; | ||
| StyleSheet styleSheet = new StyleSheet(); |
There was a problem hiding this comment.
The MissingColorNames test makes this one look very sparse.
can we please cover all the supported patterns and have negative tests too.
|
Hmm .. % java MissingColorNames |
|
The test fails because the content of JDK-8294090 #15262 has not been merged into this PR. |
|
Actually, JDK-8294090 #15262 successfully parses expressions like |
|
@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! |
|
@scientificware This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
This is referenced in Java Bug Database as
This is tracked in JBS as
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 : Add named colors from CSS Color Module Level 4
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/9825/head:pull/9825$ git checkout pull/9825Update a local copy of the PR:
$ git checkout pull/9825$ git pull https://git.openjdk.org/jdk.git pull/9825/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9825View PR using the GUI difftool:
$ git pr show -t 9825Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9825.diff
Webrev
Link to Webrev Comment