8276671: Iterative layout algorithm

[mstr2]

This PR fixes a long-standing issue with baseline layout computation in JavaFX as mentioned in JDK-8090261 and makes baseline layouts more consistent and dependable. There may be other filed bugs or enhancement requests that relate to the same fundamental issue.

How the baseline offset is computed

Generally, the value returned from Node.getBaselineOffset() is one of the following:

1. the actual baseline offset
2. the special constant BASELINE_OFFSET_SAME_AS_HEIGHT

The reason for this is that there is a circular dependency between the height of a resizable control and its baseline: in order to know the baseline offset, one must know the size of the control; and in order to know the size of the control, one must know the baseline offset. The special constant BASELINE_OFFSET_SAME_AS_HEIGHT basically means: we don't know the baseline offset at this time, but when it comes to layouting the node, we just use whatever the height of the resizable node turns out to be.

However, some resizable controls (Labeled being the most prominent) must report an actual baseline value in order to work meaningfully, but this value can't be computed consistently for the reasons mentioned (this is the cause of JDK-809261).

Step 1: solving Labeled

Since we can't know the actual baseline offset of a resizable Labeled before the control is laid out, we schedule another layout pass when the layoutY property of the Text node was changed during layout. Repeatedly layouting and measuring the resulting baseline offset will quickly converge to the true baseline offset of the Labeled control. This is the code sample taken from JDK-8090261 before and after the change:

Step 2: making baseline layouts more consistent

Even with Labeled solved, the more general issue is that baseline layouts often yield inconsistent results depending on which layout containers are used to compose the scene graph. In order to fix this, we need to ensure that the baseline offset of any given layout container meaningfully reflects the baseline of the nodes placed within it.

This is achieved by adding Node.isTextBaseline(), which returns true if the node or any of its children contain text (and thus should be the primary source when determining a baseline). Currently, Labeled, Text and TextInputControl return true by default.

Additionally, a new boolean property Node.prefBaselineProperty() is introduced. This flag can be used to override the baseline source selection.

The default behavior of Parent.getBaselineOffset() is changed such that the baseline is derived from the first managed child for which

1. Node.isPrefBaseline() returns true, or if there is no such child,
2. Node.isTextBaseline() returns true, or if there is no such child,
3. Parent.getBaselineOffset() returns a value other than BASELINE_OFFSET_SAME_AS_HEIGHT.

The difference can be seen when comparing the old layout behavior to the new layout behavior:

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires a CSR request matching fixVersion (No fixVersion in .jcheck/conf) to be approved (needs to be created)
• Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)

Issue

• JDK-8276671: Iterative layout algorithm

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 433

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

Using diff file

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

[bridgekeeper]

👋 Welcome back mstr2! 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.

[kevinrushforth]

Since this enhancement represents a behavioral change, and adds new API, it will need to be discussed on the openjfx-dev mailing list prior to review. A Draft PR can be helpful to illustrate what you are proposing, but the focus should be first on the problem, and then on the API changes and behavior changes you propose to address that problem.

If as a result of that discussion, there is general agreement about the problem and the approach, then next step would be to focus on the API and specification changes, which will need a CSR.

/reviewers 3
/csr

[openjdk]

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

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@mstr2 this pull request must refer to an issue in JBS to be able to link it to a CSR request. To refer this pull request to an issue in JBS, please use the /issue command in a comment in this pull request.

[openjdk]

@mstr2 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout feature/baselinelayout
git fetch https://git.openjdk.java.net/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[mstr2]

I've created a small playground app that shows a few scenarios in which a baseline-aligned layout now works as expected.

Here's a screenshot of the current behavior on the left side, and the new behavior on the right side.

[bridgekeeper]

@mstr2 This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@mstr2 This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[kevinrushforth]

As mentioned in PR #1879 we should consider resurrecting this enhancement.

[Maran23]

I agree, also given the current regression that is discussed by @hjohn and @johanvos. As discussed, we should consider having a good documentation and spec about the algorithm.

[hjohn]

I agree, also given the current regression that is discussed by @hjohn and @johanvos. As discussed, we should consider having a good documentation and spec about the algorithm.

This should already be available somewhere at Oracle. Anything we write up would therefore not only be repeating existing work, but be based on best guesses and reverse engineering. Also this would be mostly for developers, not specification, as the specification should not be going into details of the algorithm.

Also, this has not caused a regression in my opinion. It's a fix for a bug that stopped controls from updating completely because the layout system got into a bad state (controls needing updating, but this not being propagated up the scene graph).

This has however exposed a new bug, which was always there, but slightly harder to trigger (as the system would go into a bad state before it could happen, ie, a bug hiding another bug). That 2nd bug is that code in Node does not correctly detect that a change made to layout positions was triggered by layout code, and thus was expected, and should therefore not trigger a new layout. The fix for that is here: #1945

I already knew of that bug (as the isCurrentLayoutChild check Node is doing simply can't work at all), however at the time I felt that it would be quite tricky to fix (requiring an update to all layout containers, and setting the current layout child before each position update -- a ridiculous situation, and one that wouldn't even fix the problem discovered by Johan). As it has been working like that since forever, I thought it would not expose new problems, but unfortunately it did.

[Maran23]

This should already be available somewhere at Oracle. Anything we write up would therefore not only be repeating existing work, but be based on best guesses and reverse engineering. Also this would be mostly for developers, not specification, as the specification should not be going into details of the algorithm.

True. I think every kind of documentation (but especially for us developers) would be good to have somewhere (not buried) for such a core functionality that is the inner workings of the layout.

Also, this has not caused a regression in my opinion. It's a fix for a bug that stopped controls from updating completely because the layout system got into a bad state (controls needing updating, but this not being propagated up the scene graph).

I agree 100%! I just wanted to say here that we may need to rethink the current design, part of which is included in this PR.
To have a more stable base and more knowledge about it (although it might be much harder than that).

I already knew of that bug (as the isCurrentLayoutChild check Node is doing simply can't work at all), however at the time I felt that it would be quite tricky to fix (requiring an update to all layout containers, and setting the current layout child before each position update -- a ridiculous situation, and one that wouldn't even fix the problem discovered by Johan). As it has been working like that since forever, I thought it would not expose new problems, but unfortunately it did.

Also agree, the code was questionable. IMO it is very good that this was found and we have a good fix and understanding (I already checked the PR), which should also improve performance while making the layout (expectations) more stable - a very good thing! :)