CSSParser Color.parse() for unexpected CSS properties
Kevin Rushforth
kevin.rushforth at oracle.com
Thu Apr 5 11:39:55 UTC 2018
OK, thanks. For the benefit of others who might be interested, this bug
is now visible here:
https://bugs.openjdk.java.net/browse/JDK-8201135
-- Kevin
Matthew Elliot wrote:
> 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
> <mailto: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 <mailto: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 <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
>
>
>
>
>
>
>
>
More information about the openjfx-dev
mailing list