Skip to content

8367602: Regression: TabPane with wrapped label calculates wrong initial size#1900

Closed
hjohn wants to merge 1 commit intoopenjdk:masterfrom
hjohn:feature/tabpane-regression
Closed

8367602: Regression: TabPane with wrapped label calculates wrong initial size#1900
hjohn wants to merge 1 commit intoopenjdk:masterfrom
hjohn:feature/tabpane-regression

Conversation

@hjohn
Copy link
Collaborator

@hjohn hjohn commented Sep 13, 2025

Fix for regression in TabPane size calculation caused by https://bugs.openjdk.org/browse/JDK-8350149 (#1723) between versions 25-ea+8 and 25-ea+10

In Region several height calculations functions were altered to match the behavior and options of their horizontal counterparts to solve discrepancies between the behavior of vertical biased layouts and horizontal biased layouts, which other than their axis, should behave identical.

The height calculations would take a width provided by the caller, but if unavailable (set to -1) and the child was biased, it would just automatically query the child's preferred width and use that as the dependent width.

In contrast, the horizontal functions would only do this if a height was provided by the caller (not -1), and would only override this value if the property fillHeight was false (in which case it would query the child's height directly).

With the change in JDK-8350149, the vertical calculations operate the same as the horizontal ones, as this is generally more flexible. However, the automatic behavior to query the child's size if the dependent size given was set to -1 is no longer there (as this behavior isn't how biased calculations should work).

The TabPaneSkin has chosen to directly call several width/height functions without obeying the content bias contract, which says that in case of bias, the dependent size must be calculated first and then passed to the size calculation one is interested in.

(As an aside, if TabPaneSkin had elected to put all tabs in a single StackPane, then StackPane would have correctly done these calculations, but digging into why it is done this way, with each individual Tab wrapped in its own Node (also a StackPane), is a bit out of scope for this fix).

As it is, TabPaneSkin should check the bias of the tab it is doing calculations for, and if biased, should first query the dependent size before triggering the size calculation it is interested in.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8367602: Regression: TabPane with wrapped label calculates wrong initial size (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1900/head:pull/1900
$ git checkout pull/1900

Update a local copy of the PR:
$ git checkout pull/1900
$ git pull https://git.openjdk.org/jfx.git pull/1900/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1900

View PR using the GUI difftool:
$ git pr show -t 1900

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1900.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 13, 2025

👋 Welcome back jhendrikx! 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
Copy link

openjdk bot commented Sep 13, 2025

@hjohn 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:

8367602: Regression: TabPane with wrapped label calculates wrong initial size

Reviewed-by: angorya, kcr

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 3 new commits pushed to the master branch:

  • c8cfc89: 8361644: Update ICU4C to 77.1
  • 15c50c2: 8361893: Update libxml2 to 2.14.5
  • 17e37c2: 8340378: [XWayland] FXCanvas apps and tests crash on Ubuntu 24.04

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 changed the title 8367602 Regression: TabPane with wrapped label calculates wrong initial size 8367602: Regression: TabPane with wrapped label calculates wrong initial size Sep 13, 2025
@openjdk openjdk bot added the rfr Ready for review label Sep 13, 2025
@mlbridge
Copy link

mlbridge bot commented Sep 13, 2025

Webrevs

@kevinrushforth
Copy link
Member

Reviewers: @arapte @andy-goryachev-oracle

/reviewers 2

@openjdk
Copy link

openjdk bot commented Sep 15, 2025

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle 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 (macOS 15.6.1 M1)

You could have just edited https://bugs.openjdk.org/browse/JDK-8365827 description instead of creating a whole new ticket. Save a virtual tree, I say! :-)

@andy-goryachev-oracle
Copy link
Contributor

Thank you for fixing the issue, @hjohn !

@hjohn
Copy link
Collaborator Author

hjohn commented Sep 15, 2025

The fix looks good (macOS 15.6.1 M1)

You could have just edited https://bugs.openjdk.org/browse/JDK-8365827 description instead of creating a whole new ticket. Save a virtual tree, I say! :-)

Sorry about that, I entered a whole new ticket before realizing that other one existed :)

@andy-goryachev-oracle
Copy link
Contributor

no harm done, and thanks for fixing it! and yours has more technical insight which is good.

@dlemmermann
Copy link

Can't wait for this one to get merged. It's the only thing stopping me from upgrading.

@andy-goryachev-oracle
Copy link
Contributor

we need one more review. join us, @dlemmermann ! :-)

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.

Looks great. Thanks for fixing this.

@openjdk openjdk bot added the ready Ready to be integrated label Sep 16, 2025
@hjohn
Copy link
Collaborator Author

hjohn commented Sep 17, 2025

/integrate

@openjdk
Copy link

openjdk bot commented Sep 17, 2025

Going to push as commit 7e692bd.
Since your change was applied there have been 5 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 17, 2025
@openjdk openjdk bot closed this Sep 17, 2025
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Sep 17, 2025
@openjdk
Copy link

openjdk bot commented Sep 17, 2025

@hjohn Pushed as commit 7e692bd.

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

@hjohn
Copy link
Collaborator Author

hjohn commented Sep 17, 2025

/backport jfx jfx25u

@openjdk
Copy link

openjdk bot commented Sep 17, 2025

@hjohn The target branch jfx25u does not exist
List of valid branches: master, jfx25, jfx24, jfx23, jfx22, jfx21, jfx20, jfx19, jfx18, jfx17...

@hjohn
Copy link
Collaborator Author

hjohn commented Sep 17, 2025

/backport jfx jfx25

@openjdk
Copy link

openjdk bot commented Sep 17, 2025

@hjohn the backport was successfully created on the branch backport-hjohn-7e692bd0-jfx25 in my personal fork of openjdk/jfx. To create a pull request with this backport targeting openjdk/jfx:jfx25, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 7e692bd0 from the openjdk/jfx repository.

The commit being backported was authored by John Hendrikx on 17 Sep 2025 and was reviewed by Andy Goryachev and Kevin Rushforth.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jfx:

$ git fetch https://github.com/openjdk-bots/jfx.git backport-hjohn-7e692bd0-jfx25:backport-hjohn-7e692bd0-jfx25
$ git checkout backport-hjohn-7e692bd0-jfx25
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jfx.git backport-hjohn-7e692bd0-jfx25

⚠️ @hjohn You are not yet a collaborator in my fork openjdk-bots/jfx. An invite will be sent out and you need to accept it before you can proceed.

@kevinrushforth
Copy link
Member

kevinrushforth commented Sep 17, 2025

/backport jfx jfx25u
...
/backport jfx jfx25

That should be: /backport jfx25u, since jfx25u is a repo, not a branch in the jfx repo.

@hjohn
Copy link
Collaborator Author

hjohn commented Sep 17, 2025

/backport jfx jfx25u
...
/backport jfx jfx25

That should be: /backport jfx25u, since jfx25u is a repo, not a branch in the jfx repo.

Ah, that explains it, thank you!

@hjohn
Copy link
Collaborator Author

hjohn commented Sep 17, 2025

/backport jfx25u

@openjdk
Copy link

openjdk bot commented Sep 17, 2025

@hjohn the backport was successfully created on the branch backport-hjohn-7e692bd0-master in my personal fork of openjdk/jfx25u. To create a pull request with this backport targeting openjdk/jfx25u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 7e692bd0 from the openjdk/jfx repository.

The commit being backported was authored by John Hendrikx on 17 Sep 2025 and was reviewed by Andy Goryachev and Kevin Rushforth.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jfx25u:

$ git fetch https://github.com/openjdk-bots/jfx25u.git backport-hjohn-7e692bd0-master:backport-hjohn-7e692bd0-master
$ git checkout backport-hjohn-7e692bd0-master
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jfx25u.git backport-hjohn-7e692bd0-master

⚠️ @hjohn You are not yet a collaborator in my fork openjdk-bots/jfx25u. An invite will be sent out and you need to accept it before you can proceed.

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