CSSParser Color.parse() for unexpected CSS properties

[Matthew Elliot]

Hey David, thanks.
I have filed a bug via the Oracle website.

internal review ID : 9053225

Hopefully this was correct as it was also my first time.
Matt


On 4 April 2018 at 17:21, David Grieve <david.grieve at oracle.com> wrote:

> On 4/4/18 10:44 AM, Matthew Elliot wrote:
>
> Hi David, thanks for the quick response, the parser does seem to have
> knowledge of the property and values as in the method ...
>
> ParsedValueImpl valueFor(String property, Term root, CSSLexer lexer) throws ParseException {}
>
> it looks for particular properties for parsing... e.g.
>
> } else if ("-fx-background-color".equals(prop)) {
>     return parsePaintLayers(root);
> } else if ("-fx-background-image".equals(prop)) {
>     return parseURILayers(root);
> } else if ("-fx-background-insets".equals(prop)) {
>      return parseInsetsLayers(root);
>
> ... but fx-alignment and fx-shape for example aren't listed here and fall
> into this strange catch all place I noted in my previous email.
>
> My follow up questions would be:
>
> 1. Why does it hit this for standard css properties as defined for JavaFX
> components(fx-alignment, fx-shape, etc) I.e. https://docs.oracle.com/
> javafx/2/api/javafx/scene/doc-files/cssref.html (hbox, vbox have
> -fx-alignment)
> 2. Even if it is wanted to be extensible, isn't by default attempting to
> parse a color where the property is not known and therefore triggering
> exception throw / catch on the thread critical to UI perf a less than
> optimal solution? Could it be changed to handle this more gracefully than
> catch / ignore exceptions?
>
> Is it worth raising a ticket for such a topic, would it ever be considered
> for improvement.
>
> I think it is worth raising a ticket.
>
>
> Thanks again,
> Matt
>
>
> On 4 April 2018 at 16:20, David Grieve <david.grieve at oracle.com> wrote:
>
>> The parser doesn't have any concept of what the property is or value it
>> might have. This allows the addition of new properties (such as an user
>> might add for their own CSS styles) without having to modify the parser to
>> handle them.
>>
>>
>>
>> On 4/4/18 10:03 AM, Matthew Elliot wrote:
>>
>>> Hi all, (first post).
>>>
>>> I was profiling our PROD JavaFX application recently I discovered
>>> something
>>> rather peculiar in the CSSParser. (jdk1.8.0_151)
>>>
>>> I noticed several hundred IllegalArgumentExceptions on the
>>> JavaApplicationThread where for various unrelated css properties the
>>> CSSParser is trying to parse a color. While the exception is subsequently
>>> caught and swallowed silently doing this hundred of times on this thread
>>> is
>>> rather ugly and caused *minor* delays in the application thread.
>>>
>>> This happened for alignment, shape, and a few other properties where
>>> no-lookup case was found and it ended on approx. line 900 of the
>>> CSSParser
>>> in
>>>
>>> colorValueOfString()
>>>
>>> with a value like 'center'; clearly no color.
>>>
>>> // if the property value is another property, then it needs to be looked
>>> up.
>>> boolean needsLookup = isIdent && properties.containsKey(text);
>>> if (needsLookup || ((value = colorValueOfString(str)) == null )) {
>>>      // If the value is a lookup, make sure to use the lower-case text
>>> so it matches the property
>>>      // in the Declaration. If the value is not a lookup, then use str
>>> since the value might
>>>      // be a string which could have some case sensitive meaning
>>>      //
>>>      // TODO: isIdent is needed here because of RT-38345. This
>>> effectively undoes RT-38201
>>>      value = new ParsedValueImpl<String,String>(needsLookup ? text :
>>> str, null, isIdent || needsLookup);
>>> }
>>>
>>> I had a look in the bug tracker https://bugs.openjdk.java.net/ but
>>> didn't
>>> find much in this regard so thought I would post in case it has come up
>>> before.
>>>
>>> I saw some of the css properties are from our application and some from
>>> e(fx)clipse which I can raise to Tom Schindl separately if it is a
>>> stylesheet issue, however it would appear that for example -fx-alignment
>>> in
>>> a layout VBOX/HBOX component should be valid according to JavaFX docs.
>>>
>>> More generally, is it expected that a property such as -fx-alignment
>>> should
>>> fall into this else {} catch all case, and why does JavaFX try to parse a
>>> Color by default?
>>>
>>> -fx-alignment: center;
>>>
>>> Any input much appreciated.
>>>
>>> Regards,
>>> Matt
>>>
>>
>>
>
>