RFR: JDK-8292276 : Add named colors from CSS Color Module Level 4 [v36]
Phil Race
prr at openjdk.org
Sat Oct 7 23:46:19 UTC 2023
On Thu, 17 Aug 2023 23:13:51 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?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 with a new target base due to a merge or a rebase. The pull request now contains 51 commits:
>
> - Merge scientificware-patch-003-CSS-add_4_8_digits_hex_coded_color
>
> # Conflicts:
> # src/java.desktop/share/classes/javax/swing/text/html/CSS.java
> - Merge master
> - Merge branch 'master'
>
> CSS.java :
> Removes assigning values inside the if-condition
> Extracts the assignments into separate line.
>
> Hex3468DigitsColor.java :
> Adds a message to result in case of failure only
> - Updates copyright date.
>
> Updates copyright date to 2023.
> - Updates copyright date.
>
> Updates copyright date to 2023.
> - Update imports
>
> Remove java.util.Pattern and add java.util.Map imports
> - Performance improvement
>
> Performance results came from my repository I mentioned in the header.
>
> The code before this PR ran in 230ms.
> Our previous codes ran in 1 200ms to 1800 ms with String + formatted + %n$s usage.
> They ran in 350ms to 380ms with String + formatted + %s usage.
> And in 100ms to 110ms if we replace String + format with a string concatenation.
> Now the code below gives the same results in 36ms and with all our expected behaviors. Since we control notation length we
> can bypass some controls,
> directly generate the color value,
> without generate a new string,
> and reject a wrong number format without generate any exception.
> - Corrects a value in a message.
>
> A message is added to the result in case of failure only. The updated code does not output the actual value. The tested color is #f12a instead of #f00a.
> - Simplifications of the test.
>
> Removes individual color tests and only compares the RGB value.
> - Renames an identifier.
>
> Suggested change, not to use `l` as an identifier because it could be confused with `1`.
> This part of code could change and be replaced by bits right rotation after performance tests.
> - ... and 41 more: https://git.openjdk.org/jdk/compare/a8ab3be3...6a6af622
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 https://github.com/openjdk/jdk/pull/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.
src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 1545:
> 1543: Map.entry("olive", new Color(128, 128, 0)),
> 1544: Map.entry("olivedrab", new Color(107, 142, 35)),
> 1545: Map.entry("orange", new Color(255, 165, 0)),
I see this differs from the previous definition of orange
color = hexToColor("#FF8000");
but it looks like the new value matches the CSS spec, so the existing value may have been based on some outdated CSS spec .. or a bug ..
test/jdk/javax/swing/text/html/CSS/MissingColorNames.java line 70:
> 68: {null, null},
> 69: // This color doesn't belong to the CSS-COLOR-4 specification
> 70: {"com.scientificware.color", null},
if you are going to test using a made up colour name, in this context, I think "openjdk.org" or even better "notacolor", might be more appropriate.
test/jdk/javax/swing/text/html/CSS/MissingColorNames.java line 116:
> 114: {"dimgray", "ff696969"},
> 115: {"dimgrey", "ff696969"},
> 116: {"dodgerblue", "ff1e90ff"},
Seriously ? That's in CSS ?
The committee must have been loaded with people from SoCal .. where's "giantsorange" ?
-------------
Changes requested by prr (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/9825#pullrequestreview-1636378814
PR Review Comment: https://git.openjdk.org/jdk/pull/9825#discussion_r1332099808
PR Review Comment: https://git.openjdk.org/jdk/pull/9825#discussion_r1332121449
PR Review Comment: https://git.openjdk.org/jdk/pull/9825#discussion_r1332124035
More information about the client-libs-dev
mailing list