-
Notifications
You must be signed in to change notification settings - Fork 542
8350149: VBox ignores bias of child controls when fillWidth is set to false #1723
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
Conversation
|
👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into |
|
@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: 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 19 new commits pushed to the
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 |
e36303f to
635e486
Compare
635e486 to
33500ba
Compare
Webrevs
|
|
you probably want to start off a new PR from the latest master... |
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a finished review, only a few initial comments - I need to do a bit more testing.
This change however looks exceptionally good.
| double baseline = child.getBaselineOffset(); | ||
| if (child.isResizable() && baseline == BASELINE_OFFSET_SAME_AS_HEIGHT) { | ||
| return top + snapSizeY(boundedSize(child.minHeight(alt), child.maxHeight(alt), Double.MAX_VALUE)) + bottom | ||
| return top + snapSizeY(boundedSize(child.minHeight(alt), max, Double.MAX_VALUE)) + bottom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and elsewhere: a wise man once said that the sum of snapped values may not come out snapped. Should we start snapping the result of any algebraic operation that involves snapped values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL, I think I may have said that :) However, perhaps the problem is not as bad as I made it out to be. It's definitely true that any operation on a floating point value may slightly nudge it away from a true snapped value (somewhere in the 10th decimal place) and that this problem gets worse the more operations you perform (ie. a HBox with 10.000 children will have a worse error than one with only a few children).
But there may be some middle ground possible here. As long as our layout containers are aware that values returned from computeMinWidth etc are only "near" snapped (meaning that if you round them to the nearest pixel, you'll always get the right result), it should be fine -- one must take care not to immediately call ceil or floor on these values. A thing we may want to look into is how this is rendered on screen by the rendering layer; does the rendering do additional work when values are near snapped, or do they perform some rounding already anyway. I certainly never noticed these small errors resulting in display artifacts.
In any case, this is probably better addressed in a separate effort, and we should probably write some guidelines first. I'm hoping to do some of this with the space distributor PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, all these calculations end up being used by the layoutChildren() which always (?) snap the values.
So the question is whether this small error gets accumulated enough to shift the result to a wrong value. Given a typical number of children (<1000) and screen sizes (<10000), we might conclude that it's unlikely, since
Math.ulp(1000 * 10_000.0) = 1.862645149230957E-9
which is much smaller than the distance between pixels even at 1000% scale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's absolutely true, however this can change quickly when large values are added or subtracted to/from small values, or when division or multiplication gets involved. So I'd say its relatively safe to do simple calculations with near snapped values, but one must be careful with functions like ceil and floor as they can amplify tiny floating-point errors due to their discontinuous nature. For example, if take a snapped spacing and add a snapped left and right margin, then call ceil on the result, it could go like this:
1.99999999999999 +
4.00000000000001 +
4.00000000000001 =
10.00000000000001 -> ceil -> 11 (instead of the expected 10)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
This is exactly the reason for the code in ScaledMath:71
return Math.ceil(d - Math.ulp(d)) / scale;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come to think of it, most the issues here are caused by using functions like floor and ceil. It might be an idea to change these functions to bias them slightly towards rounding to the closest value, instead of always doing a straight up floor or ceil.
For example, let's say I calculate a size as 3.0000001; ceiling this (with snapSize) to 4 is quite ridiculous; obviously 3 was intended, but the ceiling function won't care. But what if we subtracted a value (assuming we're dealing with pixels of course, which the snap functions are)? We could bias it slightly towards the correct value by using something ridiculously small like 1/10000th of a pixel. For example:
3.0000001 - 1/10000th of a pixel = 2.9999. Ceiling this value yields the intended 3. We could work much safer with near snapped values, as there is much less risk of a one ulp difference being dramatically amplified by floor/ceil functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
This is exactly the reason for the code in ScaledMath:71
return Math.ceil(d - Math.ulp(d)) / scale;
Yeah, but I think we may want to subtract more than just 1 ulp. A one ulp difference can be created after any operation (like add/subtract). Do two of these without resnapping, and the difference will be >1 ulp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are onto something here. It almost feels like we shouldn't be doing ceil/floor at all, rounding to the closest pixel instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a much larger change, though. Perhaps something to consider for a future discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinrushforth Yes, I wasn't intending to do anything about this in this change as it is unrelated. I understand why ceil is used; FX distinguishes between sizes and spaces, where sizes are part of some visual part of a control, and spaces are non-control areas. Spaces are generally ceil'd, so that even the thinnest control or border would show up as 1 pixel, while positions and spaces are just rounded. Still, a border that would end up being 0.0001 pixels wide probably is still best hidden completely.
| double left = margin != null ? snapSpaceX(margin.getLeft(), snap) : 0; | ||
| double right = margin != null ? snapSpaceX(margin.getRight(), snap) : 0; | ||
|
|
||
| return width - left - right; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hjohn You may have answered this question in the larger discussion above, but I wanted to double-check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry, this was addressed in the larger discussion. I've left this as-is to keep the PR focus'd on one thing.
The calculation here is using 3 snapped values, and one can reasonably assume the result is "nearly" snapped. If this value is used later with a ceiling function though, then it might ceil to the next higher value if the result is slightly too high due to floating point errors. This is why it might be a good idea to adjust how our ceiling functions work in all cases; instead of using a tiny epsilon (or ulp), use a much larger value but still tiny in terms of pixels (like 1/10000th of a pixel). Any "near" snapped values won't accidentally get rounded up to the next higher pixel then when ceil is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any "near" snapped values won't accidentally get rounded up to the next higher pixel
I like this idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. This sounds promising.
modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java
Outdated
Show resolved
Hide resolved
| * # bounded width/heights: | ||
| * | ||
| * The space allocated to a child, minus its margins, adjusted according to | ||
| * its constraints (min <= X <= max). These are never -1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always > 0.0 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 is probably still allowed, but it shouldn't be negative.
| * Ensure that no fillHeight/height combinations have effect on controls that are not vertically biased: | ||
| */ | ||
|
|
||
| assertEquals(12, RegionShim.computeChildMinAreaWidth(pane, c2, -1, new Insets(1), -1, false), 1e-100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might as well declare a static computeChildMinAreaWidth() which delegates to RegionShim.computeChildMinAreaWidth...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can streamline this a bit more.
|
FYI: To help with testing, I've added some containers to the monkey tester: (AnchorPane, BorderPane, FlowPane) and added more options to the HBox/VBox page and add child function and the childrens' context menus. |
|
I'll review this too. /reviewers 2 |
|
@kevinrushforth |
|
Mailing list message from Dirk Lemmermann on openjfx-dev: Thank you for detecting and fixing this issue guys. I have spent many days / weeks chasing issues related to this issue. It often came up with my custom controls and I was always assuming that it is me making a mistake. I never even considered that this might be a bug in the layout code. Dirk |
modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/shims/java/javafx/scene/layout/RegionShim.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderPaneTest.java
Show resolved
Hide resolved
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and I don't see any regressions.
|
@kevinrushforth Do you have time to take a look? It would be good to get this in, as there are more changes I'd like to do in this area (fixing the HBox/VBox rounding errors on high DPI displays). |
|
Yes, sorry for the delay. This one is next on my list. |
kevinrushforth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look correct to me as does my testing. I left a minor question or two, but nothing that needs to be addressed before integration.
| double baseline = child.getBaselineOffset(); | ||
| if (child.isResizable() && baseline == BASELINE_OFFSET_SAME_AS_HEIGHT) { | ||
| return top + snapSizeY(boundedSize(child.minHeight(alt), child.maxHeight(alt), Double.MAX_VALUE)) + bottom | ||
| return top + snapSizeY(boundedSize(child.minHeight(alt), max, Double.MAX_VALUE)) + bottom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a much larger change, though. Perhaps something to consider for a future discussion.
| double left = margin != null ? snapSpaceX(margin.getLeft(), snap) : 0; | ||
| double right = margin != null ? snapSpaceX(margin.getRight(), snap) : 0; | ||
|
|
||
| return width - left - right; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hjohn You may have answered this question in the larger discussion above, but I wanted to double-check.
|
I think all questions have been resolved; if so, this is ready to integrate. Let's file a follow-on bug to address possible changes to snapping. |
|
/integrate |
|
Going to push as commit a550e5e.
Your commit was automatically rebased without conflicts. |
|
Unfortunately, this commit caused our app to behave abnormally :( I know this is a problem with our app, I will try to fix it. But what I want to say is that this PR does break some real-world applications, and maybe a workaround is really needed to make them work. |
|
@Glavo I'd be interested in what problem you're running into (maybe before/after description?). Perhaps there is a bug in the fix. It's also possible you inadvertently have been relying on the behavior where content bias is sometimes ignored, which was a bug. All FX containers always respect content bias. In that case the only solution is to modify your layout a bit. Let me know if I can be of help. |
We encountered this problem. The content of this page (excluding the title bar) is a In OpenJFX 25+9 and earlier, the height of this It is not difficult for us to solve this problem. Wrapping this |
|
The BorderPane shares some of the calculations with VBox, so it's possible it is a bug. This small program can reproduce the issue: .main .button {
-fx-pref-width: 12em;
-fx-pref-height: 12em;
}package app;
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.FlowPane;
import javafx.stage.Stage;
public class LayoutBugDemo extends Application {
@Override
public void start(Stage stage) {
BorderPane borderPane = new BorderPane();
FlowPane flowPane = new FlowPane();
flowPane.getStyleClass().add("main");
borderPane.setTop(new Label("Instance Name"));
borderPane.setCenter(flowPane);
flowPane.getChildren().addAll(
new Button("Minecraft"),
new Button("Forge"),
new Button("NeoForge"),
new Button("OptiFine"),
new Button("Fabric"),
new Button("Fabric API"),
new Button("Quilt"),
new Button("SQL/QFAPI")
);
borderPane.setBottom(new Button("Install"));
Scene scene = new Scene(borderPane, 600, 400);
scene.getStylesheets().add("styles.css");
stage.setScene(scene);
stage.setTitle("JavaFX Layout Bug Demo");
stage.show();
}
public static void main(String[] args) {
launch(args);
}
} |
|
I looked a bit further, and I think that the The only troubling part of that fix however is that its counterpart ( I'll need to look more closely what is the correct fix here, as it seems to me that having vertical calculations use fill, but not horizontal calculations is also not what one would expect. Creating an example where the FlowPane is laid out vertically and seeing how it behaves then may give a clue if that breaks in the same way (allocating too much width to it when used as the center of a border pane). |
|
It looks like when using a vertical flow pane, that the left-to-right version of the layout is also quite poor: When I apply the same fix in Note how the flow pane now doesn't take an unreasonable amount of space anymore cutting the install button from view. |
|
It might be better to create a new JBS ticket if this is a new bug (or reference an existing one). |
|
Do you plan to fix this issue before the release of OpenJFX25? I'm wondering if I need to use a workaround to get around it. |
|
There first needs to be a new bug filed. Once there is, and a fix is available, we can see how risky the fix will be and decide whether to backport it to jfx25. |
|
@kevinrushforth @Glavo bug is filed here: https://bugs.openjdk.org/browse/JDK-8362873 |




Fixes the case where
VBoxwill ignore the (horizontal) bias of a child when its fill width property is set tofalse. This manifests itself with labels that have their wrap text property set totrue, and the container is not wide enough to hold the text on a single line (in other words, the label is potentially far wider, and fill width should have no effect). No reflow would occur before this fix.The solution is to ensure the
computeChildAreaMin/Pref/MaxHeightfunctions are provided with afillWidthparameter, to be used in the case a horizontally biased control is encountered (several of the parameters to these compute functions are only used for those special cases and ignored otherwise).With this additional information, the compute functions can decide between the preferred width of a control or the available width of the container. In the old implementation this decision was made before these functions were called, and the available width was reset to -1 to indicate the case that the preferred width should be used. However, there are three cases, and so setting a single parameter to -1 can't effectively communicate that:
Assuming a control that is horizontally biased, and is resizable there are three cases when calculating a height:
All in all, this PR removes several inconsistencies between horizontal and vertical computations. The end result is that the horizontal and vertical bias calculations now more closely mirror each other.
Note: some tests have had their values adjusted. This is because the asymmetries between the compute width/height functions have been removed. Most of these tests use the
MockBiasedtest control that simulates an area of pixels that always covers the same size (ie. it is 10x1000 or 100x100 or 1000x10 = 10000). One should be aware though that when correctly querying its minimum size, it will say something like 100x100 -- and that's correct, as that's whatMockBiasedwill tell the layout system, even though in a single dimension it can return a minimum size of 1 when the other dimension provided is set to say 10000.Due to a bug in how content bias was handled before in the height functions (content bias was taken into account when
-1was passed which is incorrect) some tests saw much smaller minimum sizes.HBoxTestis an example of this, but if you compare it to the equivalent code inVBoxTest, you can see that something is clearly off, as the tests don't agree even though they mirror each other. The original developer tried to rationalize these differences, instead of investigating where they came from.It may be worth to add tests for a different type of biased control.
MockBiasedis an example of a control that requires roughly the same area when resized (like a reflowing Label, or a FlowPane). Another example is a control that tries to maintain a fixed aspect ratio, like a scaling Image. The latter type is not interested in maintaining an area of roughly the same size, but instead it is interested in maintaining a fixed factor between its width and height.Note 2: Many layout containers will call min/pref/max width/height methods with a dependent size provided (instead of -1) even when the control has no content bias. Control implementations therefore need to be careful not to use this value when they have no bias or have the opposite bias.
MockBiasedhandles this correctly.BorderPaneTest
BorderPaneTest has had some tests adjusted as its height computations were affected by the bug in the compute height functions where a biased calculation was partially executed when it shouldn't have. The height values make much more sense now instead of being some weird fractional number when a biased control is involved. I've adjusted the tests accordingly, and tried adding some rationalization of how the values are reached. Note that
BorderPane(apparently) always adjusts whatever size it is given during layout to be greater than its minimum size; this is the first time I've seen containers do that, instead of going with whatever they've been given...Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1723/head:pull/1723$ git checkout pull/1723Update a local copy of the PR:
$ git checkout pull/1723$ git pull https://git.openjdk.org/jfx.git pull/1723/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1723View PR using the GUI difftool:
$ git pr show -t 1723Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1723.diff
Using Webrev
Link to Webrev Comment