8277122: SplitPane divider drag can hang the layout

Reviewed-by: fastegal, aghaisas

Author: Marius Hanl

modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -36,7 +36,6 @@
 import javafx.geometry.VPos;
 import javafx.scene.Cursor;
 import javafx.scene.Node;
-import javafx.scene.control.Accordion;
 import javafx.scene.control.Control;
 import javafx.scene.control.SkinBase;
 import javafx.scene.control.SplitPane;
@@ -65,6 +64,13 @@ public class SplitPaneSkin extends SkinBase<SplitPane> {
     private ObservableList<Content> contentRegions;
     private ObservableList<ContentDivider> contentDividers;
     private boolean horizontal;
+    /**
+     * Flag which is used to determine whether we need to request layout when a divider position changed or not.
+     * E.g. We don't want to request layout when we are changing the divider position in
+     * {@link #layoutChildren(double, double, double, double)} since we are currently doing the layout.
+     * See also: JDK-8277122
+     */
+    private boolean duringLayout;
 
 
 
@@ -216,8 +222,8 @@ public SplitPaneSkin(final SplitPane control) {
             previousSize = horizontal ? sw : sh;
         }
 
-        // If the window is less than the min size we want to resize
-        // proportionally
+        duringLayout = true;
+        // If the window is less than the min size we want to resize proportionally
         double minSize = totalMinSize();
         if (minSize > (horizontal ? w : h)) {
             double percentage = 0;
@@ -235,6 +241,7 @@ public SplitPaneSkin(final SplitPane control) {
             setupContentAndDividerForLayout();
             layoutDividersAndContent(w, h);
             resize = false;
+            duringLayout = false;
             return;
         }
 
@@ -411,6 +418,7 @@ public SplitPaneSkin(final SplitPane control) {
         }
 
         layoutDividersAndContent(w, h);
+        duringLayout = false;
         resize = false;
     }
 
@@ -617,7 +625,7 @@ private void addDivider(SplitPane.Divider d) {
         ChangeListener<Number> posPropertyListener = new PosPropertyListener(c);
         c.setPosPropertyListener(posPropertyListener);
         d.positionProperty().addListener(posPropertyListener);
-        initializeDivderEventHandlers(c);
+        initializeDividerEventHandlers(c);
         contentDividers.add(c);
         getChildren().add(c);
     }
@@ -633,7 +641,7 @@ private void removeAllDividers() {
         lastDividerUpdate = 0;
     }
 
-    private void initializeDivderEventHandlers(final ContentDivider divider) {
+    private void initializeDividerEventHandlers(final ContentDivider divider) {
         // TODO: do we need to consume all mouse events?
         // they only bubble to the skin which consumes them by default
         divider.addEventHandler(MouseEvent.ANY, event -> {
@@ -954,7 +962,9 @@ public PosPropertyListener(ContentDivider divider) {
                 // When checking is enforced, we know that the position was set explicitly
                 divider.posExplicit = true;
             }
-            getSkinnable().requestLayout();
+            if (!duringLayout) {
+                getSkinnable().requestLayout();
+            }
         }
     }
 

modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,6 +25,11 @@
 
 package test.javafx.scene.control;
 
+import javafx.collections.FXCollections;
+import javafx.scene.control.ComboBox;
+import javafx.scene.control.Tab;
+import javafx.scene.control.TabPane;
+import org.junit.After;
 import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
 import javafx.css.CssMetaData;
 import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*;
@@ -53,6 +58,8 @@
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.concurrent.atomic.AtomicInteger;
+
 /**
  *
  * @author srikalyc
@@ -65,6 +72,7 @@ public class SplitPaneTest {
     private Scene scene;
     private Stage stage;
     private StackPane root;
+    private StageLoader stageLoader;
 
     @Before public void setup() {
         tk = (StubToolkit)Toolkit.getToolkit();//This step is not needed (Just to make sure StubToolkit is loaded into VM)
@@ -79,6 +87,11 @@ public class SplitPaneTest {
         stage.setScene(scene);
     }
 
+    @After
+    public void cleanup() {
+        if (stageLoader != null) stageLoader.dispose();
+    }
+
     /*********************************************************************
      * Helper methods (NOTE TESTS)                                       *
      ********************************************************************/
@@ -1329,4 +1342,56 @@ private double convertDividerPostionToAbsolutePostion(double pos, double edge) {
 
         sl.dispose();
     }
+
+    /**
+     * Verifies that a divider position change of the {@link SplitPane} does not hang the layout.
+     * Previously, this may happen when the divider position changed to a large number (>1),
+     * which can hang the layout as it resulted in multiple layout requests (through SplitPaneSkin.layoutChildren).
+     * See also: JDK-8277122
+     */
+    @Test
+    public void testDividerOverOneDoesNotHangLayout() {
+        testSetDividerPositionDoesNotHangLayout(10);
+    }
+
+    /**
+     * Verifies that a divider position change of the {@link SplitPane} does not hang the layout.
+     * Previously, this may happen when the divider position changed to a negative number (<1),
+     * which can hang the layout as it resulted in multiple layout requests (through SplitPaneSkin.layoutChildren).
+     * See also: JDK-8277122
+     */
+    @Test
+    public void testDividerUnderZeroDoesNotHangLayout() {
+        testSetDividerPositionDoesNotHangLayout(-1);
+    }
+
+    private void testSetDividerPositionDoesNotHangLayout(double dividerPosition) {
+        AtomicInteger layoutCounter = new AtomicInteger();
+        ComboBox<String> cbx = new ComboBox<>(FXCollections.observableArrayList("1", "2", "3")) {
+            @Override
+            protected void layoutChildren() {
+                layoutCounter.incrementAndGet();
+                super.layoutChildren();
+            }
+        };
+        SplitPane pane = new SplitPane(new Label("AAAAA"), new TabPane(new Tab("Test", cbx)));
+        StackPane root = new StackPane(pane);
+
+        stageLoader = new StageLoader(root);
+
+        Toolkit.getToolkit().firePulse();
+
+        pane.setDividerPosition(0, dividerPosition);
+
+        Toolkit.getToolkit().firePulse();
+
+        // Reset layout counter
+        layoutCounter.set(0);
+
+        cbx.getSelectionModel().select(0);
+        Toolkit.getToolkit().firePulse();
+
+        assertTrue(layoutCounter.get() > 0);
+    }
+
 }