On Thu, 4 Mar 2021 04:01:03 GMT, Ambarish Rapte <[email protected]> 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 incrementally with one additional > commit since the last revision: > > update to recalculate properties when font size changes The fix looks good to me, as do the new unit tests. I tested it with the test program I wrote as well as by running your new unit test with / without the fix. As with any CSS fix, the two things we need to ensure are that there are no functional regressions and that there is no significant performance hit. I have a a couple questions: 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? 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 also left some inline suggestions and questions. 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). 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. 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. 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. 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). 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. 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. ------------- PR: https://git.openjdk.java.net/jfx/pull/397
