From 19246b6349a328cc593be5a36e30c5e85156f09f Mon Sep 17 00:00:00 2001 From: John Hendrikx Date: Thu, 23 Oct 2025 15:27:59 +0200 Subject: [PATCH 1/4] Change how Node detects whether a relayout is required This new check is much more accurate to detect whether a parent is currently laying out its children. The previous code almost never worked, resulting in additional unnecessary layouts. --- .../src/main/java/javafx/scene/Node.java | 4 +- .../src/main/java/javafx/scene/Parent.java | 41 +++++++++++-------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/modules/javafx.graphics/src/main/java/javafx/scene/Node.java b/modules/javafx.graphics/src/main/java/javafx/scene/Node.java index c9f3ee20387..79fd9e5dc36 100644 --- a/modules/javafx.graphics/src/main/java/javafx/scene/Node.java +++ b/modules/javafx.graphics/src/main/java/javafx/scene/Node.java @@ -2850,7 +2850,7 @@ protected void invalidated() { final Parent p = getParent(); // Propagate layout if this change isn't triggered by its parent - if (p != null && !p.isCurrentLayoutChild(Node.this)) { + if (p != null && !p.inLayoutChildren()) { if (isManaged()) { // Force its parent to fix the layout since it is a managed child. p.requestLayout(true); @@ -2924,7 +2924,7 @@ protected void invalidated() { final Parent p = getParent(); // Propagate layout if this change isn't triggered by its parent - if (p != null && !p.isCurrentLayoutChild(Node.this)) { + if (p != null && !p.inLayoutChildren()) { if (isManaged()) { // Force its parent to fix the layout since it is a managed child. p.requestLayout(true); diff --git a/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java b/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java index 007d161d9dc..7c51e90fed4 100644 --- a/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java +++ b/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java @@ -984,6 +984,19 @@ boolean isPerformingLayout() { return performingLayout; } + private boolean inLayoutChildren; + + /** + * Returns whether this node is currently running its {@link #layoutChildren()} + * method. This is useful to detect if a change to layout properties of a + * direct child should be ignored, or should trigger a layout pass. + * + * @return {@code true} if this node is running {@link #layoutChildren()}, otherwise {@code false} + */ + boolean inLayoutChildren() { + return inLayoutChildren; + } + private boolean sizeCacheClear = true; private double prefWidthCache = -1; private double prefHeightCache = -1; @@ -1243,17 +1256,6 @@ protected double computeMinHeight(double width) { return super.getBaselineOffset(); } - /** - * It stores the reference to the current child being laid out by its parent. - * This reference is important to differentiate whether a layout is triggered - * by its parent or other events. - */ - private Node currentLayoutChild = null; - - boolean isCurrentLayoutChild(Node node) { - return node == currentLayoutChild; - } - /** * Executes a top-down layout pass on the scene graph under this parent. * @@ -1280,19 +1282,27 @@ public final void layout() { break; } performingLayout = true; - layoutChildren(); + inLayoutChildren = true; + + try { + layoutChildren(); + } + finally { + inLayoutChildren = false; + } + // Intended fall-through case DIRTY_BRANCH: for (int i = 0, max = children.size(); i < max; i++) { final Node child = children.get(i); - currentLayoutChild = child; + if (child instanceof Parent) { ((Parent)child).layout(); } else if (child instanceof SubScene) { ((SubScene)child).layoutPass(); } } - currentLayoutChild = null; + performingLayout = false; break; } @@ -1309,12 +1319,11 @@ public final void layout() { protected void layoutChildren() { for (int i=0, max=children.size(); i Date: Thu, 23 Oct 2025 18:52:19 +0200 Subject: [PATCH 2/4] Fix ToolBarSkinTest Reusing a toolbar as part of several scenes, in combination with the StubToolkit that doesn't handle pulses makes this test fail with the relayout detection fix. --- .../javafx/scene/control/skin/ToolBarSkinTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ToolBarSkinTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ToolBarSkinTest.java index f4da91c99c2..7be09baa0dc 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ToolBarSkinTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ToolBarSkinTest.java @@ -87,14 +87,14 @@ public void overflowMenuNotShowingWithDifferentRenderScales() { 2.25 }; - Rectangle rect = new Rectangle(100, 100); - ToolBar toolBar = new ToolBar(rect); - toolBar.setSkin(new ToolBarSkin(toolBar)); - for (var orientation : Orientation.values()) { - toolBar.setOrientation(orientation); - for (double scale : renderScales) { + Rectangle rect = new Rectangle(100, 100); + ToolBar toolBar = new ToolBar(rect); + toolBar.setSkin(new ToolBarSkin(toolBar)); + + toolBar.setOrientation(orientation); + Stage stage = new Stage(); stage.renderScaleXProperty().bind(DoubleConstant.valueOf(scale)); stage.renderScaleYProperty().bind(DoubleConstant.valueOf(scale)); From 792963257fb322c4f94d8850cff1c7a96ecc61bf Mon Sep 17 00:00:00 2001 From: John Hendrikx Date: Mon, 27 Oct 2025 15:28:07 +0100 Subject: [PATCH 3/4] Add regression test --- .../test/java/test/javafx/scene/NodeTest.java | 162 ++++++++++++++---- 1 file changed, 133 insertions(+), 29 deletions(-) diff --git a/modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java b/modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java index 57becf9a8e5..3892813df18 100644 --- a/modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java +++ b/modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java @@ -35,6 +35,7 @@ import com.sun.javafx.geom.transform.Translate2D; import test.com.sun.javafx.pgstub.StubToolkit; import com.sun.javafx.scene.DirtyBits; +import com.sun.javafx.scene.LayoutFlags; import com.sun.javafx.scene.NodeHelper; import com.sun.javafx.scene.input.PickResultChooser; import com.sun.javafx.scene.shape.RectangleHelper; @@ -63,18 +64,23 @@ import java.lang.ref.WeakReference; import java.lang.reflect.Method; import java.util.Comparator; +import java.util.List; +import java.util.Objects; import java.util.Set; +import java.util.stream.Stream; import javafx.scene.Group; import javafx.scene.Node; import javafx.scene.NodeShim; import javafx.scene.ParallelCamera; +import javafx.scene.Parent; import javafx.scene.ParentShim; import javafx.scene.PerspectiveCamera; import javafx.scene.Scene; import javafx.scene.SceneShim; import javafx.scene.SubScene; import javafx.scene.layout.AnchorPane; +import javafx.scene.layout.HBox; import javafx.scene.transform.Affine; import javafx.scene.transform.Scale; import javafx.scene.transform.Shear; @@ -1608,55 +1614,144 @@ public void testRootMirroringWithTranslate() { @Test - public void testLayoutXYTriggersParentSizeChange() { - final Group rootGroup = new Group(); - final Group subGroup = new Group(); - ParentShim.getChildren(rootGroup).add(subGroup); + public void testLayoutXYTriggersParentSizeChangeForManagedChild() { + Rectangle r1 = new Rectangle(50, 50); + Rectangle r2 = new Rectangle(1, 1); + Group group = new Group(r1, r2); + HBox root = new HBox(group); - Rectangle r = new Rectangle(50,50); - r.setManaged(false); - Rectangle staticR = new Rectangle(1,1); - ParentShim.getChildren(subGroup).addAll(r, staticR); + r1.setManaged(false); - assertEquals(50,subGroup.getLayoutBounds().getWidth(), 1e-10); - assertEquals(50,subGroup.getLayoutBounds().getHeight(), 1e-10); + // Assert expected initial state: + assertLayoutFlags(group, LayoutFlags.NEEDS_LAYOUT, LayoutFlags.NEEDS_LAYOUT); + assertEquals(50, group.getLayoutBounds().getWidth()); + assertEquals(50, group.getLayoutBounds().getHeight()); - r.setLayoutX(50); + root.layout(); + + // Assert everything is clean after layout: + assertLayoutFlags(group, LayoutFlags.CLEAN, LayoutFlags.CLEAN); + assertEquals(50, group.getLayoutBounds().getWidth()); + assertEquals(50, group.getLayoutBounds().getHeight()); + + // Test for Layout X change: + r1.setLayoutX(50); + + // Assert layout is required: + assertLayoutFlags(group, LayoutFlags.NEEDS_LAYOUT, LayoutFlags.CLEAN); - rootGroup.layout(); + root.layout(); - assertEquals(100,subGroup.getLayoutBounds().getWidth(), 1e-10); - assertEquals(50,subGroup.getLayoutBounds().getHeight(), 1e-10); + // Assert that all is clean, and the change has been applied: + assertLayoutFlags(group, LayoutFlags.CLEAN, LayoutFlags.CLEAN); + assertEquals(100, group.getLayoutBounds().getWidth()); + assertEquals(50, group.getLayoutBounds().getHeight()); + // Test for Layout Y change: + r1.setLayoutY(40); + + // Assert layout is required: + assertLayoutFlags(group, LayoutFlags.NEEDS_LAYOUT, LayoutFlags.CLEAN); + + root.layout(); + + // Assert that all is clean, and the change has been applied: + assertLayoutFlags(group, LayoutFlags.CLEAN, LayoutFlags.CLEAN); + assertEquals(100, group.getLayoutBounds().getWidth()); + assertEquals(90, group.getLayoutBounds().getHeight()); } @Test - public void testLayoutXYWontBreakLayout() { - final Group rootGroup = new Group(); - final AnchorPane pane = new AnchorPane(); - ParentShim.getChildren(rootGroup).add(pane); - - Rectangle r = new Rectangle(50,50); - ParentShim.getChildren(pane).add(r); + public void testLayoutXYWillRelayoutAndUndoChangesToManagedChild() { + Rectangle r = new Rectangle(50, 50); + AnchorPane pane = new AnchorPane(r); + Group root = new Group(pane); AnchorPane.setLeftAnchor(r, 10d); AnchorPane.setTopAnchor(r, 10d); - rootGroup.layout(); + // Assert expected initial state: + assertLayoutFlags(pane, LayoutFlags.NEEDS_LAYOUT, LayoutFlags.NEEDS_LAYOUT); + + root.layout(); + + // Assert everything is clean after layout: + assertLayoutFlags(pane, LayoutFlags.DIRTY_BRANCH, LayoutFlags.CLEAN); + assertEquals(10, r.getLayoutX()); + assertEquals(10, r.getLayoutY()); + + // Note: above we expected all to be clean, but code in width/height change listeners interferes + // and draws the wrong conclusions. Relayout and reassert should fix this harmless situation: - assertEquals(10, r.getLayoutX(), 1e-10); - assertEquals(10, r.getLayoutY(), 1e-10); + root.layout(); + // Assert everything is clean after layout: + assertLayoutFlags(pane, LayoutFlags.CLEAN, LayoutFlags.CLEAN); + assertEquals(10, r.getLayoutX()); + assertEquals(10, r.getLayoutY()); + + // Test for Layout X change: r.setLayoutX(50); - assertEquals(50, r.getLayoutX(), 1e-10); - assertEquals(10, r.getLayoutY(), 1e-10); + // Assert layout is required: + assertLayoutFlags(pane, LayoutFlags.NEEDS_LAYOUT, LayoutFlags.NEEDS_LAYOUT); + assertEquals(50, r.getLayoutX()); + assertEquals(10, r.getLayoutY()); + + root.layout(); + + // Assert that all is clean, and the change has been reverted: + assertLayoutFlags(pane, LayoutFlags.CLEAN, LayoutFlags.CLEAN); + assertEquals(10, r.getLayoutX()); + assertEquals(10, r.getLayoutY()); + + // Test for Layout Y change: + r.setLayoutY(40); - rootGroup.layout(); + // Assert layout is required: + assertLayoutFlags(pane, LayoutFlags.NEEDS_LAYOUT, LayoutFlags.NEEDS_LAYOUT); + assertEquals(10, r.getLayoutX()); + assertEquals(40, r.getLayoutY()); - assertEquals(10, r.getLayoutX(), 1e-10); - assertEquals(10, r.getLayoutY(), 1e-10); + root.layout(); + // Assert that all is clean, and the change has been reverted: + assertLayoutFlags(pane, LayoutFlags.CLEAN, LayoutFlags.CLEAN); + assertEquals(10, r.getLayoutX()); + assertEquals(10, r.getLayoutY()); + } + + @Test + public void onLayoutPositionChangeShouldTriggerNeedsLayoutIfInLayoutChildrenOfDirectChild() { + Rectangle r = new Rectangle(50, 50); + AnchorPane pane = new AnchorPane(r); + HBox root = new HBox(pane); + + AnchorPane.setLeftAnchor(r, 10d); + AnchorPane.setTopAnchor(r, 10d); + + // Assert expected initial state: + assertLayoutFlags(pane, LayoutFlags.NEEDS_LAYOUT, LayoutFlags.NEEDS_LAYOUT); + + root.layout(); + + /* + * Note: we really expect the layout graph to be "CLEAN" after this, however + * there is logic for the width/height fields that currently is a bit too + * eager (and in this situation doesn't set the flags correct either, but + * its relatively harmless). We assert it below, but if this ever changes, + * these asserts can be removed: + */ + + assertLayoutFlags(pane, LayoutFlags.DIRTY_BRANCH, LayoutFlags.CLEAN); + assertEquals(10, r.getLayoutX()); + assertEquals(10, r.getLayoutY()); + root.layout(); // this should "correct" the harmless intermediate state + + // Assert expected final state: + assertLayoutFlags(pane, LayoutFlags.CLEAN, LayoutFlags.CLEAN); + assertEquals(10, r.getLayoutX()); + assertEquals(10, r.getLayoutY()); } @Test @@ -2070,4 +2165,13 @@ public void treeVisibleFlagUpdatesChildrenTreeVisibleFlag() { assertTrue(NodeHelper.isTreeVisible(n1)); assertTrue(NodeHelper.isTreeVisible(n2)); } + + private static void assertLayoutFlags(Parent leaf, LayoutFlags... flags) { + List subtree = Stream.iterate(leaf, Objects::nonNull, Parent::getParent).toList().reversed(); + + for (int i = 0; i < subtree.size(); i++) { + Parent p = subtree.get(i); + assertEquals(flags[i], ParentShim.getLayoutFlag(p), "" + p); + } + } } From 8d496e9c93a0133093056e22ba8a0011ac17735a Mon Sep 17 00:00:00 2001 From: John Hendrikx Date: Mon, 27 Oct 2025 15:41:24 +0100 Subject: [PATCH 4/4] Rename test --- .../src/test/java/test/javafx/scene/NodeTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java b/modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java index 3892813df18..075eb3df47f 100644 --- a/modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java +++ b/modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java @@ -1722,7 +1722,7 @@ public void testLayoutXYWillRelayoutAndUndoChangesToManagedChild() { } @Test - public void onLayoutPositionChangeShouldTriggerNeedsLayoutIfInLayoutChildrenOfDirectChild() { + public void shouldOnlyDoSingleLayoutPass() { Rectangle r = new Rectangle(50, 50); AnchorPane pane = new AnchorPane(r); HBox root = new HBox(pane);