CSSParser Color.parse() for unexpected CSS properties

[David Grieve]

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 
> <mailto: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
>
>
>