8289357: (Tree)TableView is null in (Tree)TableRowSkin during autosize

8292009: Wrong text artifacts in table header

Reviewed-by: kcr, angorya

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

@@ -671,11 +671,12 @@ private <T,S> void resizeColumnToFitContent(TableView<T> tv, TableColumn<T, S> t
             if ((cell.getText() != null && !cell.getText().isEmpty()) || cell.getGraphic() != null) {
                 tableRow.applyCss();
                 maxWidth = Math.max(maxWidth, cell.prefWidth(-1));
-                tableSkin.getChildren().remove(cell);
             }
         }
+        tableSkin.getChildren().remove(tableRow);
 
-        // dispose of the cell to prevent it retaining listeners (see RT-31015)
+        // dispose of the row and cell to prevent it retaining listeners (see RT-31015)
+        tableRow.updateIndex(-1);
         cell.updateIndex(-1);
 
         // RT-36855 - take into account the column header text / graphic widths.
@@ -712,6 +713,8 @@ private <T,S> void resizeColumnToFitContent(TableView<T> tv, TableColumn<T, S> t
     private <T> TableRow<T> createMeasureRow(TableView<T> tv, TableViewSkinBase tableSkin,
             Callback<TableView<T>, TableRow<T>> rowFactory) {
         TableRow<T> tableRow = rowFactory != null ? rowFactory.call(tv) : new TableRow<>();
+        tableRow.updateTableView(tv);
+
         tableSkin.getChildren().add(tableRow);
         tableRow.applyCss();
         if (!(tableRow.getSkin() instanceof SkinBase<?>)) {
@@ -766,11 +769,12 @@ private <T,S> void resizeColumnToFitContent(TreeTableView<T> ttv, TreeTableColum
                 double w = cell.prefWidth(-1);
 
                 maxWidth = Math.max(maxWidth, w);
-                tableSkin.getChildren().remove(cell);
             }
         }
+        tableSkin.getChildren().remove(treeTableRow);
 
-        // dispose of the cell to prevent it retaining listeners (see RT-31015)
+        // dispose of the row and cell to prevent it retaining listeners (see RT-31015)
+        treeTableRow.updateIndex(-1);
         cell.updateIndex(-1);
 
         // RT-36855 - take into account the column header text / graphic widths.
@@ -808,6 +812,8 @@ private <T,S> void resizeColumnToFitContent(TreeTableView<T> ttv, TreeTableColum
     private <T> TreeTableRow<T> createMeasureRow(TreeTableView<T> ttv, TableViewSkinBase tableSkin,
             Callback<TreeTableView<T>, TreeTableRow<T>> rowFactory) {
         TreeTableRow<T> treeTableRow = rowFactory != null ? rowFactory.call(ttv) : new TreeTableRow<>();
+        treeTableRow.updateTreeTableView(ttv);
+
         tableSkin.getChildren().add(treeTableRow);
         treeTableRow.applyCss();
         if (!(treeTableRow.getSkin() instanceof SkinBase<?>)) {

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

@@ -117,7 +117,10 @@ private void setupTreeTableViewListeners() {
                 // When in fixed cell size mode, we must listen to the width of the virtual flow, so
                 // that when it changes, we can appropriately add / remove cells that may or may not
                 // be required (because we remove all cells that are not visible).
-                registerChangeListener(getVirtualFlow().widthProperty(), e -> tableView.requestLayout());
+                VirtualFlow<TableRow<T>> virtualFlow = getVirtualFlow();
+                if (virtualFlow != null) {
+                    registerChangeListener(virtualFlow.widthProperty(), e -> tableView.requestLayout());
+                }
             }
         }
     }

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

@@ -145,9 +145,10 @@ private void setupTreeTableViewListeners() {
                 // When in fixed cell size mode, we must listen to the width of the virtual flow, so
                 // that when it changes, we can appropriately add / remove cells that may or may not
                 // be required (because we remove all cells that are not visible).
-                lh.addChangeListener(getVirtualFlow().widthProperty(), (ev) -> {
-                    treeTableView.requestLayout();
-                });
+                VirtualFlow<TreeTableRow<T>> virtualFlow = getVirtualFlow();
+                if (virtualFlow != null) {
+                    lh.addChangeListener(getVirtualFlow().widthProperty(), (ev) -> treeTableView.requestLayout());
+                }
             }
         }
     }

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

@@ -32,7 +32,6 @@
 import javafx.beans.property.SimpleStringProperty;
 import org.junit.After;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 
 import com.sun.javafx.tk.Toolkit;
@@ -368,7 +367,6 @@ protected void updateItem(String item, boolean empty) {
      * The item of the {@link TableRow} should not be null, when the {@link TableCell} is not empty.
      * See also: JDK-8251483
      */
-    @Ignore("Fails currently but will be enabled again in JDK-8289357")
     @Test
     public void testRowItemIsNotNullForNonEmptyCell() {
         TableColumn<String, String> tableColumn = new TableColumn<>();

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

@@ -708,7 +708,6 @@ protected void updateItem(String item, boolean empty) {
      * The item of the {@link TreeTableRow} should not be null, when the {@link TreeTableCell} is not empty.
      * See also: JDK-8251483
      */
-    @Ignore("Fails currently but will be enabled again in JDK-8289357")
     @Test
     public void testRowItemIsNotNullForNonEmptyCell() {
         TreeTableColumn<String, String> treeTableColumn = new TreeTableColumn<>();

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

@@ -2589,14 +2589,14 @@ public void updateItem(String item, boolean empty) {
 
         StageLoader sl = new StageLoader(treeTableView);
 
-        assertEquals(21, rt_31200_count);
+        assertEquals(22, rt_31200_count);
 
         // resize the stage
         sl.getStage().setHeight(250);
         Toolkit.getToolkit().firePulse();
         sl.getStage().setHeight(50);
         Toolkit.getToolkit().firePulse();
-        assertEquals(21, rt_31200_count);
+        assertEquals(22, rt_31200_count);
 
         sl.dispose();
     }

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java

@@ -30,10 +30,14 @@
 import javafx.collections.ObservableList;
 import javafx.geometry.Insets;
 import javafx.scene.control.IndexedCell;
+import javafx.scene.control.Skin;
 import javafx.scene.control.TableColumn;
 import javafx.scene.control.TableRow;
 import javafx.scene.control.TableView;
 import javafx.scene.control.cell.PropertyValueFactory;
+import javafx.scene.control.skin.TableColumnHeader;
+import javafx.scene.control.skin.TableColumnHeaderShim;
+import javafx.scene.control.skin.TableRowSkin;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -42,11 +46,14 @@
 import test.com.sun.javafx.scene.control.test.Person;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 
 public class TableRowSkinTest {
 
     private TableView<Person> tableView;
     private StageLoader stageLoader;
+    private TableColumnHeader firstColumnHeader;
 
     @Before
     public void before() {
@@ -73,6 +80,32 @@ public void before() {
         tableView.setItems(items);
 
         stageLoader = new StageLoader(tableView);
+        firstColumnHeader = VirtualFlowTestUtils.getTableColumnHeader(tableView, firstNameCol);
+    }
+
+    /**
+     * The {@link TableView} should never be null inside the {@link TableRowSkin} during auto sizing.
+     * See also: JDK-8289357
+     */
+    @Test
+    public void testTableViewInRowSkinIsNotNullWhenAutoSizing() {
+        tableView.setRowFactory(tv -> new TableRow<>() {
+            @Override
+            protected Skin<?> createDefaultSkin() {
+                return new ThrowingTableRowSkin<>(this);
+            }
+        });
+        TableColumnHeaderShim.resizeColumnToFitContent(firstColumnHeader, -1);
+    }
+
+    /**
+     * The {@link TableView} should not have any {@link TableRow} as children.
+     * {@link TableRow}s are added temporary as part of the auto sizing, but should never remain after.
+     * See also: JDK-8289357 and JDK-8292009
+     */
+    @Test
+    public void testTableViewChildrenCount() {
+        assertTrue(tableView.getChildrenUnmodifiable().stream().noneMatch(node -> node instanceof TableRow));
     }
 
     @Test
@@ -261,4 +294,11 @@ private void removedColumnsShouldRemoveCorrespondingCellsInRowImpl() {
                 VirtualFlowTestUtils.getCell(tableView, 0).getChildrenUnmodifiable().size());
     }
 
+    private static class ThrowingTableRowSkin<T> extends TableRowSkin<T> {
+        public ThrowingTableRowSkin(TableRow<T> tableRow) {
+            super(tableRow);
+            assertNotNull(tableRow.getTableView());
+        }
+    }
+
 }

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java

@@ -30,24 +30,35 @@
 import javafx.collections.ObservableList;
 import javafx.geometry.Insets;
 import javafx.scene.control.IndexedCell;
+import javafx.collections.FXCollections;
+import javafx.collections.ObservableList;
+import javafx.scene.control.Skin;
+import javafx.scene.control.TableRow;
+import javafx.scene.control.TableView;
 import javafx.scene.control.TreeItem;
 import javafx.scene.control.TreeTableColumn;
 import javafx.scene.control.TreeTableRow;
 import javafx.scene.control.TreeTableView;
 import javafx.scene.control.cell.TreeItemPropertyValueFactory;
+import javafx.scene.control.skin.TableColumnHeader;
+import javafx.scene.control.skin.TableColumnHeaderShim;
+import javafx.scene.control.skin.TreeTableRowSkin;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
 import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils;
 import test.com.sun.javafx.scene.control.test.Person;
 
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 public class TreeTableRowSkinTest {
 
     private TreeTableView<Person> treeTableView;
     private StageLoader stageLoader;
+    private TableColumnHeader firstColumnHeader;
 
     @BeforeEach
     public void before() {
@@ -77,6 +88,32 @@ public void before() {
         treeTableView.setShowRoot(false);
 
         stageLoader = new StageLoader(treeTableView);
+        firstColumnHeader = VirtualFlowTestUtils.getTableColumnHeader(treeTableView, firstNameCol);
+    }
+
+    /**
+     * The {@link TreeTableView} should never be null inside the {@link TreeTableRowSkin} during auto sizing.
+     * See also: JDK-8289357
+     */
+    @Test
+    public void testTreeTableViewInRowSkinIsNotNullWhenAutoSizing() {
+        treeTableView.setRowFactory(tv -> new TreeTableRow<>() {
+            @Override
+            protected Skin<?> createDefaultSkin() {
+                return new ThrowingTreeTableRowSkin<>(this);
+            }
+        });
+        TableColumnHeaderShim.resizeColumnToFitContent(firstColumnHeader, -1);
+    }
+
+    /**
+     * The {@link TreeTableView} should not have any {@link TreeTableRow} as children.
+     * {@link TreeTableRow}s are added temporary as part of the auto sizing, but should never remain after.
+     * See also: JDK-8289357 and JDK-8292009
+     */
+    @Test
+    public void testTreeTableViewChildrenCount() {
+        assertTrue(treeTableView.getChildrenUnmodifiable().stream().noneMatch(node -> node instanceof TreeTableRow));
     }
 
     @Test
@@ -267,4 +304,11 @@ private void removedColumnsShouldRemoveCorrespondingCellsInRowImpl() {
                 VirtualFlowTestUtils.getCell(treeTableView, 0).getChildrenUnmodifiable().size() - 1);
     }
 
+    private static class ThrowingTreeTableRowSkin<T> extends TreeTableRowSkin<T> {
+        public ThrowingTreeTableRowSkin(TreeTableRow<T> treeTableRow) {
+            super(treeTableRow);
+            assertNotNull(treeTableRow.getTreeTableView());
+        }
+    }
+
 }