8360940: Layout stops updating when using Parent#setNeedsLayout(true) due to incorrect state management

modules/javafx.graphics/src/main/java/javafx/scene/Parent.java

@@ -1021,7 +1021,7 @@ private void markDirtyLayout(boolean local, boolean forceParentLayout) {
      * rendered. This is batched up asynchronously to happen once per
      * "pulse", or frame of animation.
      * <p>
-     * If this parent is either a layout root or unmanaged, then it will be
+     * If this parent is either a scene root or unmanaged, then it will be
      * added directly to the scene's dirty layout list, otherwise requestParentLayout
      * will be invoked.
      * @since JavaFX 8.0
@@ -1066,7 +1066,15 @@ void requestParentLayout(boolean forceParentLayout) {
         if (!layoutRoot) {
             final Parent p = getParent();
             if (p != null && (!p.performingLayout || forceParentLayout)) {
-                p.requestLayout();
+
+                /*
+                 * The forceParentLayout flag must be propagated to mark all ancestors
+                 * as needing layout. Failure to do so while performingLayout is true
+                 * would stop the propagation mid-tree. This leaves some nodes as needing
+                 * layout, while its ancestors are clean, which is an inconsistent state.
+                 */
+
+                p.requestLayout(forceParentLayout);
             }
         }
     }

modules/javafx.graphics/src/shims/java/javafx/scene/ParentShim.java

@@ -25,6 +25,8 @@
 
 package javafx.scene;
 
+import com.sun.javafx.scene.LayoutFlags;
+
 import java.util.List;
 import javafx.collections.ObservableList;
 
@@ -49,6 +51,10 @@ public static void setNeedsLayout(Parent p, boolean value) {
         p.setNeedsLayout(value);
     }
 
+    public static LayoutFlags getLayoutFlag(Parent p) {
+        return p.layoutFlag;
+    }
+
     public static List<Node> test_getRemoved(Parent p) {
         return p.test_getRemoved();
     }

modules/javafx.graphics/src/test/java/test/javafx/scene/ParentTest.java

@@ -25,6 +25,7 @@
 
 package test.javafx.scene;
 
+import com.sun.javafx.scene.LayoutFlags;
 import com.sun.javafx.scene.NodeHelper;
 import test.com.sun.javafx.pgstub.StubToolkit;
 import com.sun.javafx.sg.prism.NGGroup;
@@ -43,6 +44,8 @@
 import javafx.scene.Parent;
 import javafx.scene.ParentShim;
 import javafx.scene.Scene;
+import javafx.scene.layout.HBox;
+import javafx.scene.layout.VBox;
 import javafx.scene.shape.Rectangle;
 import javafx.stage.Stage;
 
@@ -556,7 +559,91 @@ public void assertAndClear(boolean b) {
         public void clear() {
             layoutCalled = false;
         }
+    }
+
+    /**
+     * Checks if layout flags are always consistent, even when a 2nd layout
+     * pass is requested due to a modification while layout was running.
+     *
+     * This test needs at least a layout tree of 4 levels deep due to
+     * how the layout flags are propagated:
+     * - Node will force another layout on the PARENT of sibling
+     * - Parent code will then ask for another layout on its parent
+     * - If forceParentLayout flag is not propagated, then this does
+     *   not continue up to the root, leaving the root clean.
+     */
+    @Test
+    public void layoutPositionModificationDuringLayoutPassShouldNotLeaveLayoutFlagsInInconsistentState() {
+        AtomicBoolean modifySiblingDuringLayout = new AtomicBoolean();
+        HBox sibling = new HBox();
+        HBox leaf = new HBox() {
+            @Override
+            protected void layoutChildren() {
+                super.layoutChildren();
+
+                /*
+                 * Sometimes layout code modifies a sibling's position,
+                 * in which case Node will force its parent to do another
+                 * layout in a next pass (see layoutX and layoutY properties).
+                 *
+                 * The layout flags should not become inconsistent
+                 * when it does so.
+                 */
+
+                if (modifySiblingDuringLayout.get()) {
+                    sibling.setLayoutX(100);
+                }
+            }
+        };
+        VBox level2 = new VBox(leaf, sibling);
+        HBox level1 = new HBox(level2);
+        VBox root = new VBox(level1);
+
+        // Assert default state after controls are created:
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(root));
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(level1));
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(level2));
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(leaf));
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(sibling));
+
+        root.layout();
+
+        // Assert that all is clean after a layout pass:
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(root));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(level1));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(level2));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(leaf));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(sibling));
+
+        leaf.requestLayout();
+
+        // Assert that all nodes between leaf and root are marked as needing layout:
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(root));
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(level1));
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(level2));
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(leaf));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(sibling));
+
+        // Trigger a layout that does a modification that needs a 2nd pass:
+        modifySiblingDuringLayout.set(true);
+        root.layout();
+
+        // Assert that the parent of the sibling, all the way to the root are marked as needing another layout pass:
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(root));
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(level1));
+        assertEquals(LayoutFlags.NEEDS_LAYOUT, ParentShim.getLayoutFlag(level2));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(leaf));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(sibling));
+
+        root.layout();
 
+        // Assert that after another layout pass all are clean again:
+        // Note: we still modify the sibling, but since its layoutX is unchanged now, no further pass is triggered
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(root));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(level1));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(level2));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(leaf));
+        assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(sibling));
     }
 
     @Test