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

[Kevin Rushforth]

On Fri, 5 Mar 2021 17:42:25 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> Issue is that the size of properties that are relatively(`em`) sized is not computed correctly when the reference `-fx-font-size` is also specified relatively and is nested.
>> 
>> Fix is a slight variation of an earlier suggestion in [the PR](https://github.com/javafxports/openjdk-jfx/pull/94).
>> 
>> Fix is very specific to this scenario and did not show any side effect nor any test failure.
>> 
>> There are 12 new unit tests added along with fix:
>> - Two tests fail before and pass after this fix. These test verify the reported failing scenario.
>> sameRelativeFontSizeNestedParentTest
>> relativeFontSizeDeepNestedParentControlTest
>> - Two other tests fail both before and after this fix. They are not related to the fix. These two tests are ignored. I shall file new JBS issues to track these cases and update PR with the IDs added to @Ignore.
>> propertySizesRelativeToFontSizeOfControlTest
>> propertySizesRelativeToFontSizeOfParentTest
>> - Other 8 tests are sanity tests which pass both before and after this fix.
>
> Ambarish Rapte has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - update to address review
>  - Merge branch 'master' into 8204568_css_relative_size
>  - update to recalculate properties when font size changes
>  - fix and test

Thanks for the response to the performance question. For the stress test you added, the performance hit seems reasonable to achieve correct rendering behavior. Especially since it is only applied for those cases where needed and only the first time, or when the font size changes.

I left a few wording suggestions inline.

modules/javafx.controls/src/test/java/test/javafx/css/PropertySizeTest.java line 220:

> 218: 
> 219:         // When a Parent and its Parent's font size is relative then that Parent's
> 220:         // font size is computed in relative to it's grand parent's font size.

Very mInor: grandparent is one word (I only mention in case there are other changes you are making at the same time).

modules/javafx.controls/src/test/java/test/javafx/css/PropertySizeTest.java line 256:

> 254:         // This should have been the behavior of font size calculation with nested
> 255:         // set of parents and controls, that a Parent's font size is calculated with reference
> 256:         // to its Parent and not grand parent. We are not changing current behaviour to avoid

Very minor: grandparent.
Also very minor: you use both behavior and behaviour in this paragraph. Since this isn't public docs, I don't care which one you use, but you might want to pick one and stick with it.

modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 574:

> 572:     }
> 573: 
> 574:     // The font size property of the Controls that are inherited from class Labeled is a shared a property,

Extra "a" here: "is a shared a property" --> "is a shared property"

modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 577:

> 575:     // it is shared with a child LabeledText. The Control's(inherited from Labeled) css properties are
> 576:     // first computed relative to the font size that was computed for the Control. And when the font size
> 577:     // is computed for LabeledText, it computes a different font size(which is not same as what was computed for Control).

Minor: add a space before the `(` here in the line above.

modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 584:

> 582:     // is changed by LabeledText.
> 583:     // This method is a reduced version of transitionToState() method, it is added as a fix for JDK-8204568.
> 584:     // Any modifications to the method transitionToStates() should be relatively applied here if needed.

I might drop the word "relatively" since it could be confusing (with relative font size).

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

Marked as reviewed by kcr (Lead).

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