RFR: 8204568: Relative CSS-Attributes don't work all time [v2]

[Ambarish Rapte]

On Thu, 4 Mar 2021 21:56:49 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>     1. The new method is only needed for Labeled because of the way it constructs the child graph, right? Is there anything that would preclude some other control from using this in the future, if a similar situation would arise?

Yes, new method is needed only because the font size property is shared between parent(Labeled) and child(LabeledText) control. Given that method only recalculates relatively sized properties, I can't think of anything that would preclude it from reuse.

>     2. Have you done any performance testing to ensure that there are no regressions? I wouldn't think there would be, given that this only applies the fix when the size of a Labeled control changes.

I did a performance test today using 10,000 Label controls. The program is attached to JBS.
The readings show that performance does reduce by a some amount.
Following are the 10 time(in ms) readings(each is an average of 25 readings) of time taken for onShown listener of a Stage to be executed with 10,000 Labels with relatively sized properties.
Time taken to show stage without this change is around 960 ms and with change it is around 1090 ms.
Looks like an average increase by 130 ms for 10,000 Labels.

Time in ms:
With this change:
1098.04
1114.08
1081.88
1099.76
1081.52
1104.80
1088.48
1079.44
1097.84
1083.40

Without  change:
967.96
951.04
964.76
1040.92
932.64
985.68
948.96
960.64
961.64
952.88


Also a point to note is that, this scenario occurs only when the sizes are provided such that Label's and LabeledText's font size does not match. otherwise the recalculation of properties does not happen and performance remains same.

> modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 630:
> 
>> 628:                 ParsedValue resolved = resolveLookups(node, cssValue, styleMap, transitionStates[0], whence, new HashSet<>());
>> 629:                 boolean isRelative = ParsedValueImpl.containsFontRelativeSize(resolved, false);
>> 630:                 if (!isRelative) {
> 
> Is it possible to have a property that is absolute with sub-properties that might be relative? If so, then I think you need to add a check for `numSubProperties == 0` so you don't skip this case.

I referred the [css ref](https://openjfx.io/javadoc/15/javafx.graphics/javafx/scene/doc-files/cssref.html) document which records the Available CSS Properties tables for different controls. I could find that only two css properties of region have sub properties `-fx-region-background` and `-fx-region-border`. No other control has a css property with sub properties. None of these two region css properties can be directly specified in css style but they are created using any of their sub properties if specified. Because these two properties cannot be set from css the code flow does not enter the `if (style != null)` block.
So it seems safe to me to proceed without sub properties check in the if block.
I also verified by printing the css properties and only two of these properties shown to have sub properties.

> modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 707:
> 
>> 705:     }
>> 706: 
>> 707:     private boolean transitionStateInProgress = false;
> 
> Minor: I recommend a blank line here.

Corrected.

> modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 664:
> 
>> 662: 
>> 663:                 if (originOfCalculatedValue == null) {
>> 664:                     assert false : styleableProperty.toString();
> 
> We don't generally use `assert` in our code (although I see this part is copied from the original method, so I understand if you prefer to leave it).

Removed assert.

> modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 603:
> 
>> 601:         for (int n = 0; n < numStyleables; n++) {
>> 602: 
>> 603:             final CssMetaData<Styleable,Object> cssMetaData =
> 
> This may need the ` @SuppressWarnings("unchecked")` annotation, as done in the original method.

Added back the annotation.

> modules/javafx.controls/src/test/java/test/javafx/css/PropertySizeTest.java line 337:
> 
>> 335:         System.err.println("p2FontSize: " + p2FontSize);
>> 336:         System.err.println("p3FontSize: " + p3FontSize);
>> 337:         System.err.println("p4FontSize: " + p4FontSize);
> 
> Are you planning to leave these print statements in? It seems like noise now that you are done debugging the test.

They are removed now, I overlooked when cleaning up.

> modules/javafx.controls/src/test/java/test/javafx/css/PropertySizeTest.java line 224:
> 
>> 222:         double p2FontSize = rootFontSize * 0.7;
>> 223:         double p3FontSize = p1FontSize * 0.6;
>> 224:         double p4FontSize = p2FontSize * 0.5;
> 
> Is it worth a note that the expected font size is relative to the node's grandparent if both the node and its parent are relative? This is one of the oddities of font size that we are (intentionally) preserving, so it seems worth documenting it. Especially given your comment on the next test (the one that is `@Ignore`d).

Added a comment to explain the behavior.

> modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 578:
> 
>> 576:     // Currently this method is executed only by Labeled.fontProperty().set(), when it's font size
>> 577:     // is changed by LabeledText.
>> 578:     void recalculateRelativeSizeProperties(final Node node, Font fontForRelativeSizes) {
> 
> Given the tricky nature of this fix, I would like to see a little more description in the comment explaining why it is needed and how it works. Specifically, it should say why the originally calculated value is incorrect, and indicate that a second pass is needed for certain cases.

Updated the comment with more details.

-------------

PR: https://git.openjdk.java.net/jfx/pull/397