RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value [v6]

Marius Hanl mhanl at openjdk.org
Sun Sep 11 22:34:52 UTC 2022


On Fri, 8 Jul 2022 23:12:50 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> The children of HBox/VBox don't always pixel-snap to the same value as the container itself when a render scale other than 1 is used. This can lead to a visual glitch where the content bounds don't line up with the container bounds. In this case, the container will extend beyond its content, or vice versa.
>> 
>> The following program shows the problem for HBox:
>> 
>> Region r1 = new Region();
>> Region r2 = new Region();
>> Region r3 = new Region();
>> Region r4 = new Region();
>> Region r5 = new Region();
>> Region r6 = new Region();
>> r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
>> r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, null)));
>> r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
>> r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
>> r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, null)));
>> r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
>> r1.setPrefWidth(25.3);
>> r2.setPrefWidth(25.3);
>> r3.setPrefWidth(25.4);
>> r4.setPrefWidth(25.3);
>> r5.setPrefWidth(25.3);
>> r6.setPrefWidth(25.4);
>> r1.setMaxHeight(30);
>> r2.setMaxHeight(30);
>> r3.setMaxHeight(30);
>> r4.setMaxHeight(30);
>> r5.setMaxHeight(30);
>> r6.setMaxHeight(30);
>> HBox hbox1 = new HBox(r1, r2, r3, r4, r5, r6);
>> hbox1.setBackground(new Background(new BackgroundFill(Color.RED, null, null)));
>> hbox1.setPrefHeight(40);
>> 
>> r1 = new Region();
>> r2 = new Region();
>> r3 = new Region();
>> r4 = new Region();
>> r5 = new Region();
>> r6 = new Region();
>> r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
>> r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, null)));
>> r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
>> r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
>> r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, null)));
>> r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
>> r1.setPrefWidth(25.3);
>> r2.setPrefWidth(25.3);
>> r3.setPrefWidth(25.4);
>> r4.setPrefWidth(25.3);
>> r5.setPrefWidth(25.3);
>> r6.setPrefWidth(25.4);
>> r1.setMaxHeight(30);
>> r2.setMaxHeight(30);
>> r3.setMaxHeight(30);
>> r4.setMaxHeight(30);
>> r5.setMaxHeight(30);
>> r6.setMaxHeight(30);
>> HBox hbox2 = new HBox(r1, r2, r3, r4, r5, r6);
>> hbox2.setBackground(new Background(new BackgroundFill(Color.RED, null, null)));
>> hbox2.setPrefHeight(40);
>> hbox2.setPrefWidth(152);
>> 
>> VBox root = new VBox(new HBox(hbox1), new HBox(hbox2));
>> root.setSpacing(20);
>> Scene scene = new Scene(root, 500, 250);
>> 
>> primaryStage.setScene(scene);
>> primaryStage.show();
>> 
>> 
>> Here's a before-and-after comparison of the program above after applying the fix. Note that 'before', the backgrounds of the container (red) and its content (black) don't align perfectly. The render scale is 175% in this example.
>> ![pixel-glitch](https://user-images.githubusercontent.com/43553916/112891996-146e2d80-90d9-11eb-9d83-97754ae38dc1.png)
>> 
>> I've filed a bug report and will change the title of the GitHub issue as soon as it's posted.
>
> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into fixes/box-snap-to-pixel
>  - revert snappedSum
>  - don't call snappedSum in hot loop
>  - Improved code documentation
>  - Merge branch 'master' into fixes/box-snap-to-pixel
>  - changed some method names, make test config a local class
>  - added documentation, improved method names
>  - Merge branch 'master' into fixes/box-snap-to-pixel
>  - Fix pixel-snapping glitches in VBox/HBox
>  - Failing test

Tested the changes with some easy and more complex layout and it looks good

modules/javafx.graphics/src/test/java/test/javafx/scene/layout/VBoxTest.java line 804:

> 802:      */
> 803:     @Test public void testPixelSnappedContentHeightIsSameAsBoxHeight() {
> 804:         class testPixelSnapConfig {

should be `TestPixelSnapConfig`. 
And I think for the sake of testing, this can be a standalone class or inner class as well

modules/javafx.graphics/src/test/java/test/javafx/scene/layout/VBoxTest.java line 819:

> 817:             // For these tests, VBox.prefHeight is specified, so we expect the final height to be exactly that.
> 818:             // Child heights will be adjusted appropriately such that the sum of child widths corresponds to VBox.prefHeight.
> 819:             new testPixelSnapConfig(76.0, 1.0, true),

I would prefer the initialization of the array before looping so the code looks more clean

-------------

PR: https://git.openjdk.org/jfx/pull/445


More information about the openjfx-dev mailing list