On Mon, 8 Feb 2021 11:37:35 GMT, Ambarish Rapte <ara...@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. This is taking me longer to review than I expected, because I ran into some anomalies while doing some additional testing. There is an unexpected change in behavior for nested panes with relative sizes. We need to understand this change before this is integrated. I added a modified version of the original test program to the JBS bug report that creates a scene graph like this, where the root and both parent nodes specify the font size in absolute pixels: Root // -fx-font-size: 96px P1 // -fx-font-size: 48px P2 // -fx-font-size: 36px L1 (unset), L2 (18px), L3 (0.5em) // All three had padding and bg radius at 0.5em The above scene graph works as expected with the fix, whereas before the fix label L3 had incorrect padding. I then added a button to cycle through 4 modes replacing first P2, then P1, then the Root with what would be "equivalent" relative font sizes if the definition of an "em" is its parent's font size. Root // -fx-font-size: 96px P1 // -fx-font-size: 48px P2 // -fx-font-size: 0.75em L1 (unset), L2 (18px), L3 (0.5em) // All three had padding and bg radius at 0.5em Root // -fx-font-size: 96px P1 // -fx-font-size: 0.5em P2 // -fx-font-size: 0.75em L1 (unset), L2 (18px), L3 (0.5em) // All three had padding and bg radius at 0.5em Root // -fx-font-size: 8.0em P1 // -fx-font-size: 0.5em P2 // -fx-font-size: 0.75em L1 (unset), L2 (18px), L3 (0.5em) // All three had padding and bg radius at 0.5em Things start getting unexpected when there is a parent with a relative font size, and the label had a relative font size (e.g., L3 when P2 is relative). When all nodes are relative (the last case), the padding size is completely wrong. Btw, I'm not currently worried about the calculation of the font size itself; this fix is intended to be a targeted fix that doesn't change the calculated font size (separately, we could look at that, but it would be much riskier and is out of scope for this bug fix). What I want to make sure is that in all cases, specifying the padding or other sizes in a node in ems will be relative to whatever the calculated font size is. ------------- Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/397
