On Wed, 19 Aug 2026 00:17:46 GMT, Michael Strauß <[email protected]> wrote:
> BorderPane's measurement and layout calculations are not correct when > pixel-snapping is enabled. Here is what's wrong with BorderPane, and what > consequently needs to be fixed: > > 1. Insets are not snapped: > * all measurement methods use raw insets > * `layoutChildren()` also uses raw insets > 3. Content-biased children use the wrong dependent dimension: > * top and bottom children call `prefHeight(adjustedWidth)` before their > final width is established; left and right have the same problem with height > * measurement and layout can disagree because > - constrained measurement passes the pane's total width/height without > first removing its snapped insets > - `getAreaWidth` uses `fillHeight=false`, while left/right/center > layout uses `fillHeight=true` > 5. Arithmetic results are not re-snapped. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Looks good to me overall, left some minor comments and one suggestion that if I understand the snapping guide correctly, should be the better way to go. EDIT: Can you also enable in `SnappingTest` the `BorderPane` case? It succeeds with your changes and is a good candidate to at least test the padding. I may will rework the tests a bit at one point, since I have more insights of snapping now. modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line 433: > 431: > 432: double middleAreaMinHeight = Math.max(centerMinHeight, > Math.max(rightMinHeight, leftMinHeight)); > 433: Minor: Newline here can be removed modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line 490: > 488: > 489: double middleAreaPrefHeight = Math.max(centerPrefHeight, > Math.max(rightPrefHeight, leftPrefHeight)); > 490: minor: empty newline can be removed (to match with the other methods) modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line 502: > 500: final double minWidth = minWidth(-1); > 501: final double minHeight = minHeight(-1); > 502: width = snapSpaceX(Math.max(width, minWidth)); Is it really needed to `snapSpace` the result of `minWidth` or `minHeight` ? modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line 532: > 530: if (t != null && t.isManaged()) { > 531: Insets topMargin = getNodeMargin(t); > 532: double adjustedWidth = > snapSpaceX(adjustWidthByMargin(insideWidth, topMargin)); I wonder if we instead should `snapSpace` the result of `adjustWidthByMargin` and `adjustHeightByMargin` ? Then also the `HeaderBar` will receive the 'fix'. I just took a quick look, and everything in `Region` is all jumbled up: Some methods correctly snap their result, others do not. So probably up to you, I would like to see all `Region` methods eventually to snap their result, but thats just me. modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line 538: > 536: topHeight = result.y; > 537: t.resize(result.x, topHeight); > 538: topHeight = snapSpaceY(snapSpaceY(topMargin.getBottom()) + > topHeight + snapSpaceY(topMargin.getTop())); Shouldn't the entire result here simply be snapped using `snapSpace`, something like this: Suggestion: topHeight = snapSpaceY(topMargin.getBottom() + topHeight + topMargin.getTop()); Since the result is snapped and the `topHeight` is as well modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line 556: > 554: bottomHeight = result.y; > 555: b.resize(result.x, bottomHeight); > 556: bottomHeight = > snapSpaceY(snapSpaceY(bottomMargin.getBottom()) + bottomHeight + > snapSpaceY(bottomMargin.getTop())); Same as above: Suggestion: bottomHeight = snapSpaceY(bottomMargin.getBottom() + bottomHeight + bottomMargin.getTop()); modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line 558: > 556: bottomHeight = > snapSpaceY(snapSpaceY(bottomMargin.getBottom()) + bottomHeight + > snapSpaceY(bottomMargin.getTop())); > 557: Pos alignment = getAlignment(b); > 558: positionInArea(b, insideX, snapPositionY(insideY + > insideHeight - bottomHeight), Minor: Could we extract the result of `snapPositionY` here for better readability? modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line 577: > 575: leftWidth = result.x; > 576: l.resize(leftWidth, result.y); > 577: leftWidth = snapSpaceX(snapSpaceX(leftMargin.getLeft()) + > leftWidth + snapSpaceX(leftMargin.getRight())); same `snapSpace` question/suggestion as above modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line 596: > 594: rightWidth = result.x; > 595: r.resize(rightWidth, result.y); > 596: rightWidth = snapSpaceX(snapSpaceX(rightMargin.getLeft()) + > rightWidth + snapSpaceX(rightMargin.getRight())); same `snapSpace` question/suggestion as above ------------- PR Review: https://git.openjdk.org/jfx/pull/2262#pullrequestreview-5028145114 PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3860834824 PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3860864291 PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3860873045 PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3860907749 PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3860928057 PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3860935939 PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3860947209 PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3860951743 PR Review Comment: https://git.openjdk.org/jfx/pull/2262#discussion_r3860952640
