Skip to content

8370498: Improve how Node detects whether a layout property change requires a new layout pass #1945

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

8370498: Improve how Node detects whether a layout property change requires a new layout pass #1945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19246b6
Change how Node detects whether a relayout is required
hjohn Oct 23, 2025
a22ce09
Fix ToolBarSkinTest
hjohn Oct 23, 2025
7929632
Add regression test
hjohn Oct 27, 2025
8d496e9
Rename test
hjohn Oct 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ToolBarSkinTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
4 changes: 2 additions & 2 deletions modules/javafx.graphics/src/main/java/javafx/scene/Node.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
41 changes: 25 additions & 16 deletions modules/javafx.graphics/src/main/java/javafx/scene/Parent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*
Expand All @@ -1280,19 +1282,27 @@ public final void layout() {
break;
}
performingLayout = true;
layoutChildren();
inLayoutChildren = true;

try {
layoutChildren();
}
finally {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor suggestion:
} 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;
}
Expand All @@ -1309,12 +1319,11 @@ public final void layout() {
protected void layoutChildren() {
for (int i=0, max=children.size(); i<max; i++) {
final Node node = children.get(i);
currentLayoutChild = node;

if (node.isResizable() && node.isManaged()) {
node.autosize();
}
}
currentLayoutChild = null;
}

/**
Expand Down
162 changes: 133 additions & 29 deletions modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 shouldOnlyDoSingleLayoutPass() {
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
Expand Down Expand Up @@ -2070,4 +2165,13 @@ public void treeVisibleFlagUpdatesChildrenTreeVisibleFlag() {
assertTrue(NodeHelper.isTreeVisible(n1));
assertTrue(NodeHelper.isTreeVisible(n2));
}

private static void assertLayoutFlags(Parent leaf, LayoutFlags... flags) {
List<Parent> 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);
}
}
}