Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8204568: Relative CSS-Attributes don't work all time #397

Closed
wants to merge 6 commits into from

Conversation

arapte
Copy link
Member

@arapte arapte commented Feb 8, 2021

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.

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.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8204568: Relative CSS-Attributes don't work all time

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/397/head:pull/397
$ git checkout pull/397

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 8, 2021

👋 Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Feb 8, 2021
@mlbridge
Copy link

mlbridge bot commented Feb 8, 2021

Webrevs

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Feb 8, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth self-requested a review February 8, 2021 13:46
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@arapte
Copy link
Member Author

arapte commented Mar 4, 2021

The cause of wrong behaviour in the scenario that Kevin has mentioned in previous comment :
The font size property of the Controls that are inherited from class Labeled is a shared a property. It is shared with a child LabeledText control.
The Label's(inherited from Labeled) css properties are first computed relative to the font size property that was computed for Label. And when the font size is computed for LabeledText, it computes a different font size(which is not same as what was computed for Label).
This results in the behaviour that Label's css properties do not remain relative to its font size.

-fx-font-size is a very primary property and changing its behaviour may regress lot of applications. So we decided not to change the above behaviour.
Instead the other option to fix is: recompute the relative sized css properties of Labeled when the font size is changed for LabeledText.

Changes in commit:

  1. A method to recalculate relative sized properties when font size changes.
  2. Added more tests to verify different combinations of css among parent and child.

Tests are written only using Label control. The issue and its fix can also be observed using other controls inherited from Labeled class.(RadioButton, CheckBox, Button,..)

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@arapte
Copy link
Member Author

arapte commented Mar 5, 2021

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.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@arapte
Copy link
Member Author

arapte commented Mar 6, 2021

I left a few wording suggestions inline.

Thanks for the review comments. Please take a look at new commit. I have rephrased comment statements and made corrections as per review comments.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two additional comments, and otherwise looks good to me.

@arapte
Copy link
Member Author

arapte commented Mar 8, 2021

Two additional comments, and otherwise looks good to me.

Thanks for the review, both the comments are addressed in next commit. Please take a look.

@openjdk
Copy link

openjdk bot commented Mar 8, 2021

@arapte This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8204568: Relative CSS-Attributes don't work all time

Reviewed-by: dgrieve, kcr, aghaisas

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

  • e394b0a: 8262802: Wrong context origin coordinates when using EGL and HiDPI

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Mar 8, 2021
@arapte arapte requested a review from dsgrieve March 8, 2021 12:51
@arapte
Copy link
Member Author

arapte commented Mar 8, 2021

/integrate

@openjdk openjdk bot closed this Mar 8, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Mar 8, 2021
@openjdk
Copy link

openjdk bot commented Mar 8, 2021

@arapte Since your change was applied there have been 2 commits pushed to the master branch:

  • 3374f47: 8089913: CSS pseudo classes missing by default for some controls
  • e394b0a: 8262802: Wrong context origin coordinates when using EGL and HiDPI

Your commit was automatically rebased without conflicts.

Pushed as commit 9c7cf17.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@arapte arapte deleted the 8204568_css_relative_size branch May 10, 2022 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants