CSSParser Color.parse() for unexpected CSS properties

[Matthew Elliot]

Hi Kevin,

Priyanka from Oracle beat me to it and this small example hit the nail on
the head immediately. The below will throw and swallow and
IllegalArgumentException in CSSParser in the following method.

private ParsedValueImpl<Color,Color> colorValueOfString(String str) {



import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;


public class CssParserTest extends Application  {


    @Override
    public void start(Stage primaryStage) throws Exception {
        primaryStage.setTitle("HBox Experiment 1");

        Button button1 = new Button("Button Number 1");
        Button button2 = new Button("Button Number 2");

        VBox vbox = new VBox(button1, button2);
        vbox.setStyle("-fx-alignment: top-center;");

        Scene scene = new Scene(vbox, 200, 100);

        primaryStage.setScene(scene);
        primaryStage.show();
    }

    public static void main(String[] args) {
        Application.launch(args);
    }
}

There are at least 3 properties I have seen doing this and depending on the
complexity of the CSS, in our case quite extensive it leads to a lot of
throw/catch/ignore. :)

M.
​

On 4 April 2018 at 18:06, Kevin Rushforth <kevin.rushforth at oracle.com>
wrote:

> Hi Matt,
>
> Thank you for filing this bug.
>
> Can you provide a standalone test case that reproduces this (a .java and a
> .css file), so we can attach it to the bug? Our WebBugs triage engineer
> will ask for this, and it will save time if you can provide it now.
> Otherwise the bug report looks fine.
>
> -- Kevin
>
>
>
> Matthew Elliot wrote:
>
>> 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
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>