diff --git a/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TableCellBehaviorBase.java b/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TableCellBehaviorBase.java --- a/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TableCellBehaviorBase.java +++ b/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TableCellBehaviorBase.java @@ -316,8 +316,7 @@ getFocusModel().focus(row, (TC) (sm.isCellSelectionEnabled() ? column : null)); isAlreadySelected = false; } else { - // we check if cell selection is enabled to fix RT-33897 - sm.clearAndSelect(row, sm.isCellSelectionEnabled() ? column : null); + sm.clearAndSelect(row, column); } // handle editing, which only occurs with the primary mouse button diff --git a/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TreeTableCellBehavior.java b/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TreeTableCellBehavior.java --- a/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TreeTableCellBehavior.java +++ b/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TreeTableCellBehavior.java @@ -140,8 +140,7 @@ sm.clearSelection(index, column); isAlreadySelected = false; } else { - // we check if cell selection is enabled to fix RT-33897 - sm.clearAndSelect(index, sm.isCellSelectionEnabled() ? column : null); + sm.clearAndSelect(index, column); } // handle editing, which only occurs with the primary mouse button diff --git a/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/MouseEventFirer.java b/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/MouseEventFirer.java --- a/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/MouseEventFirer.java +++ b/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/MouseEventFirer.java @@ -35,11 +35,11 @@ import javafx.scene.input.MouseButton; import javafx.scene.input.MouseEvent; import javafx.scene.input.PickResult; -import javafx.stage.Stage; +import javafx.stage.Window; + import java.util.Arrays; import java.util.List; - public final class MouseEventFirer { private final EventTarget target; @@ -52,9 +52,17 @@ // Force the target node onto a stage so that it is accessible if (target instanceof Node) { Node n = (Node)target; - new StageLoader(n); - scene = n.getScene(); - targetBounds = n.getLayoutBounds(); + Scene s = n.getScene(); + Window w = s == null ? null : s.getWindow(); + + if (w == null || w.getScene() == null) { + new StageLoader(n); + scene = n.getScene(); + targetBounds = n.getLayoutBounds(); + } else { + scene = w.getScene(); + targetBounds = n.getLayoutBounds(); + } } else if (target instanceof Scene) { scene = (Scene)target; new StageLoader(scene); @@ -70,8 +78,12 @@ } public void fireMousePressAndRelease(int clickCount, KeyModifier... modifiers) { - fireMouseEvent(MouseEvent.MOUSE_PRESSED, MouseButton.PRIMARY, clickCount, 0, 0, modifiers); - fireMouseEvent(MouseEvent.MOUSE_RELEASED, MouseButton.PRIMARY, clickCount, 0, 0, modifiers); + fireMousePressAndRelease(clickCount, 0, 0, modifiers); + } + + public void fireMousePressAndRelease(int clickCount, double deltaX, double deltaY, KeyModifier... modifiers) { + fireMouseEvent(MouseEvent.MOUSE_PRESSED, MouseButton.PRIMARY, clickCount, deltaX, deltaY, modifiers); + fireMouseEvent(MouseEvent.MOUSE_RELEASED, MouseButton.PRIMARY, clickCount, deltaX, deltaY, modifiers); } public void fireMouseClicked() { @@ -140,7 +152,7 @@ private void fireMouseEvent(EventType evtType, MouseButton button, int clickCount, double deltaX, double deltaY, KeyModifier... modifiers) { // calculate bounds - final Stage stage = (Stage) scene.getWindow(); + final Window window = scene.getWindow(); // width / height of target node final double w = targetBounds.getWidth(); @@ -153,8 +165,8 @@ final double sceneX = x + scene.getX() + deltaX; final double sceneY = y + scene.getY() + deltaY; - final double screenX = sceneX + stage.getX(); - final double screenY = sceneY + stage.getY(); + final double screenX = sceneX + window.getX(); + final double screenY = sceneY + window.getY(); final List ml = Arrays.asList(modifiers); diff --git a/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/StageLoader.java b/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/StageLoader.java --- a/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/StageLoader.java +++ b/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/StageLoader.java @@ -26,16 +26,20 @@ import javafx.scene.Group; import javafx.scene.Node; +import javafx.scene.Parent; import javafx.scene.Scene; import javafx.stage.Stage; public class StageLoader { - private Group group; private Scene scene; private Stage stage; public StageLoader(Node... content) { + if (content == null || content.length == 0) { + throw new IllegalArgumentException("Null / empty content not allowed"); + } + group = new Group(); group.getChildren().setAll(content); scene = new Scene(group); @@ -56,6 +60,7 @@ public void dispose() { stage.hide(); + group.getChildren().clear(); group = null; scene = null; stage = null; diff --git a/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/VirtualFlowTestUtils.java b/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/VirtualFlowTestUtils.java --- a/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/VirtualFlowTestUtils.java +++ b/modules/controls/src/test/java/com/sun/javafx/scene/control/infrastructure/VirtualFlowTestUtils.java @@ -197,7 +197,14 @@ @Override public Void call(IndexedCell indexedCell) { if (indexedCell.getIndex() != row) return null; - IndexedCell cell = (IndexedCell) indexedCell.getChildrenUnmodifiable().get(column); + int _column = column; + + // we need to account for TreeTableView having LabeledText node in the TreeTableRow + if (indexedCell instanceof TreeTableRow) { + _column++; + } + + IndexedCell cell = (IndexedCell) indexedCell.getChildrenUnmodifiable().get(_column); assertEquals(expected, cell.getText()); return null; } @@ -321,15 +328,15 @@ } } + public static boolean BLOCK_STAGE_LOADER_DISPOSE = false; + public static VirtualFlow getVirtualFlow(Control control) { - Group group = new Group(); - Scene scene = new Scene(group); - - Stage stage = new Stage(); - stage.setScene(scene); - - group.getChildren().setAll(control); - stage.show(); + StageLoader sl = null; + boolean stageLoaderCreated = false; + if (control.getScene() == null) { + sl = new StageLoader(control); + stageLoaderCreated = true; + } VirtualFlow flow; if (control instanceof ComboBox) { @@ -339,7 +346,10 @@ } flow = (VirtualFlow)control.lookup("#virtual-flow"); - stage.close(); + + if (stageLoaderCreated && sl != null && ! BLOCK_STAGE_LOADER_DISPOSE) { + sl.dispose(); + } return flow; } diff --git a/modules/controls/src/test/java/javafx/scene/control/ListViewMouseInputTest.java b/modules/controls/src/test/java/javafx/scene/control/ListViewMouseInputTest.java --- a/modules/controls/src/test/java/javafx/scene/control/ListViewMouseInputTest.java +++ b/modules/controls/src/test/java/javafx/scene/control/ListViewMouseInputTest.java @@ -51,21 +51,12 @@ private MultipleSelectionModel sm; private FocusModel fm; - private KeyEventFirer keyboard; - - private StageLoader stageLoader; - @Before public void setup() { listView = new ListView(); sm = listView.getSelectionModel(); fm = listView.getFocusModel(); sm.setSelectionMode(SelectionMode.MULTIPLE); - - keyboard = new KeyEventFirer(listView); - - stageLoader = new StageLoader(listView); - stageLoader.getStage().show(); listView.getItems().setAll("1", "2", "3", "4", "5", "6", "7", "8", "9", "10"); sm.clearAndSelect(0); @@ -73,7 +64,6 @@ @After public void tearDown() { listView.getSkin().dispose(); - stageLoader.dispose(); } diff --git a/modules/controls/src/test/java/javafx/scene/control/TableViewMouseInputTest.java b/modules/controls/src/test/java/javafx/scene/control/TableViewMouseInputTest.java --- a/modules/controls/src/test/java/javafx/scene/control/TableViewMouseInputTest.java +++ b/modules/controls/src/test/java/javafx/scene/control/TableViewMouseInputTest.java @@ -51,11 +51,7 @@ private TableView tableView; private TableView.TableViewSelectionModel sm; private TableView.TableViewFocusModel fm; - - private KeyEventFirer keyboard; - - private StageLoader stageLoader; - + private final TableColumn col0 = new TableColumn("col0"); private final TableColumn col1 = new TableColumn("col1"); private final TableColumn col2 = new TableColumn("col2"); @@ -74,16 +70,10 @@ tableView.getColumns().setAll(col0, col1, col2, col3, col4); sm.clearAndSelect(0); - - keyboard = new KeyEventFirer(tableView); - - stageLoader = new StageLoader(tableView); - stageLoader.getStage().show(); } @After public void tearDown() { tableView.getSkin().dispose(); - stageLoader.dispose(); } /*************************************************************************** @@ -364,6 +354,8 @@ tableView.getItems().add("Row " + i); } + new StageLoader(tableView); + final MultipleSelectionModel sm = tableView.getSelectionModel(); sm.setSelectionMode(SelectionMode.MULTIPLE); sm.clearAndSelect(0); @@ -383,6 +375,7 @@ } private int rt_30626_count = 0; + @Ignore("This is now broken due to backing out RT-33897 (as it introduced RT-34685), so ignoring for now") @Test public void test_rt_30626() { final int items = 8; tableView.getItems().clear(); @@ -411,6 +404,7 @@ assertEquals(1, rt_30626_count); } + @Ignore("This is now broken due to backing out RT-33897 (as it introduced RT-34685), so ignoring for now") @Test public void test_rt_33897_rowSelection() { final int items = 8; tableView.getItems().clear(); diff --git a/modules/controls/src/test/java/javafx/scene/control/TableViewTest.java b/modules/controls/src/test/java/javafx/scene/control/TableViewTest.java --- a/modules/controls/src/test/java/javafx/scene/control/TableViewTest.java +++ b/modules/controls/src/test/java/javafx/scene/control/TableViewTest.java @@ -33,6 +33,7 @@ import java.util.*; import com.sun.javafx.scene.control.infrastructure.KeyEventFirer; +import com.sun.javafx.scene.control.infrastructure.MouseEventFirer; import com.sun.javafx.scene.control.infrastructure.StageLoader; import com.sun.javafx.scene.control.skin.*; import javafx.beans.property.ObjectProperty; @@ -1964,4 +1965,94 @@ // other value (based on the width of the content in that column) assertEquals(400, last.getWidth(), 0.0); } + + @Test public void test_rt_34685_directEditCall_cellSelectionMode() { + test_rt_34685_commitCount = 0; + test_rt_34685(false, true); + } + + @Test public void test_rt_34685_directEditCall_rowSelectionMode() { + test_rt_34685_commitCount = 0; + test_rt_34685(false, false); + } + + @Test public void test_rt_34685_mouseDoubleClick_cellSelectionMode() { + test_rt_34685_commitCount = 0; + test_rt_34685(true, true); + } + + @Test public void test_rt_34685_mouseDoubleClick_rowSelectionMode() { + test_rt_34685_commitCount = 0; + test_rt_34685(true, false); + } + + private int test_rt_34685_commitCount = 0; + private void test_rt_34685(boolean useMouseToInitiateEdit, boolean cellSelectionModeEnabled) { + assertEquals(0, test_rt_34685_commitCount); + + TableView table = new TableView<>(); + table.getSelectionModel().setCellSelectionEnabled(cellSelectionModeEnabled); + table.getSelectionModel().setSelectionMode(SelectionMode.SINGLE); + table.setEditable(true); + + Person person1; + table.setItems(FXCollections.observableArrayList( + person1 = new Person("John", "Smith", "john.smith@example.com"), + new Person("Jacob", "Michaels", "jacob.michaels@example.com"), + new Person("Jim", "Bob", "jim.bob@example.com") + )); + + TableColumn first = new TableColumn("first"); + first.setCellValueFactory(new PropertyValueFactory("firstName")); + first.setCellFactory(TextFieldTableCell.forTableColumn()); // note that only the first name col is editable + + EventHandler> onEditCommit = first.getOnEditCommit(); + first.setOnEditCommit(new EventHandler>() { + @Override public void handle(TableColumn.CellEditEvent event) { + test_rt_34685_commitCount++; + onEditCommit.handle(event); + } + }); + + table.getColumns().addAll(first); + + // get the cell at (0,0) + VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE = true; + TableCell cell = (TableCell) VirtualFlowTestUtils.getCell(table, 0, 0); + VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE = false; + assertTrue(cell.getSkin() instanceof TableCellSkin); + assertNull(cell.getGraphic()); + assertEquals("John", cell.getText()); + assertEquals("John", person1.getFirstName()); + + // set the table to be editing the first cell at 0,0 + if (useMouseToInitiateEdit) { + MouseEventFirer mouse = new MouseEventFirer(cell); + mouse.fireMousePressAndRelease(2, 10, 10); // click 10 pixels in and 10 pixels down + } else { + table.edit(0,first); + } + + Toolkit.getToolkit().firePulse(); + assertNotNull(cell.getGraphic()); + assertTrue(cell.getGraphic() instanceof TextField); + + TextField textField = (TextField) cell.getGraphic(); + assertEquals("John", textField.getText()); + + textField.setText("Andrew"); + textField.requestFocus(); + Toolkit.getToolkit().firePulse(); + + KeyEventFirer keyboard = new KeyEventFirer(textField); + keyboard.doKeyPress(KeyCode.ENTER); + + VirtualFlowTestUtils.getVirtualFlow(table).requestLayout(); + Toolkit.getToolkit().firePulse(); + + VirtualFlowTestUtils.assertTableCellTextEquals(table, 0, 0, "Andrew"); + assertEquals("Andrew", cell.getText()); + assertEquals("Andrew", person1.getFirstName()); + assertEquals(1, test_rt_34685_commitCount); + } } diff --git a/modules/controls/src/test/java/javafx/scene/control/TreeTableViewMouseInputTest.java b/modules/controls/src/test/java/javafx/scene/control/TreeTableViewMouseInputTest.java --- a/modules/controls/src/test/java/javafx/scene/control/TreeTableViewMouseInputTest.java +++ b/modules/controls/src/test/java/javafx/scene/control/TreeTableViewMouseInputTest.java @@ -61,8 +61,6 @@ private TreeTableView.TreeTableViewSelectionModel sm; private TreeTableView.TreeTableViewFocusModel fm; - private StageLoader stageLoader; - private final TreeTableColumn col0 = new TreeTableColumn("col0"); private final TreeTableColumn col1 = new TreeTableColumn("col1"); private final TreeTableColumn col2 = new TreeTableColumn("col2"); @@ -135,14 +133,10 @@ tableView.setRoot(root); tableView.getColumns().setAll(col0, col1, col2, col3, col4); - - stageLoader = new StageLoader(tableView); - stageLoader.getStage().show(); } @After public void tearDown() { tableView.getSkin().dispose(); - stageLoader.dispose(); sm = null; } @@ -433,6 +427,8 @@ root.getChildren().add(new TreeItem<>("Row " + i)); } + new StageLoader(tableView); + tableView.setShowRoot(true); final MultipleSelectionModel sm = tableView.getSelectionModel(); sm.setSelectionMode(SelectionMode.MULTIPLE); @@ -521,6 +517,7 @@ } private int rt_30626_count = 0; + @Ignore("This is now broken due to backing out RT-33897 (as it introduced RT-34685), so ignoring for now") @Test public void test_rt_30626() { final int items = 8; root.getChildren().clear(); @@ -551,6 +548,7 @@ assertEquals(1, rt_30626_count); } + @Ignore("This is now broken due to backing out RT-33897 (as it introduced RT-34685), so ignoring for now") @Test public void test_rt_33897_rowSelection() { final int items = 8; root.getChildren().clear(); diff --git a/modules/controls/src/test/java/javafx/scene/control/TreeTableViewTest.java b/modules/controls/src/test/java/javafx/scene/control/TreeTableViewTest.java --- a/modules/controls/src/test/java/javafx/scene/control/TreeTableViewTest.java +++ b/modules/controls/src/test/java/javafx/scene/control/TreeTableViewTest.java @@ -36,6 +36,8 @@ import java.util.Random; import com.sun.javafx.scene.control.infrastructure.KeyEventFirer; +import com.sun.javafx.scene.control.infrastructure.MouseEventFirer; +import com.sun.javafx.scene.control.skin.TreeTableCellSkin; import com.sun.javafx.scene.control.test.Data; import javafx.beans.InvalidationListener; @@ -2941,4 +2943,98 @@ assertTrue(table.getSelectionModel().isSelected(0)); assertTrue(table.getFocusModel().isFocused(0)); } + + @Test public void test_rt_34685_directEditCall_cellSelectionMode() { + test_rt_34685_commitCount = 0; + test_rt_34685(false, true); + } + + @Test public void test_rt_34685_directEditCall_rowSelectionMode() { + test_rt_34685_commitCount = 0; + test_rt_34685(false, false); + } + + @Test public void test_rt_34685_mouseDoubleClick_cellSelectionMode() { + test_rt_34685_commitCount = 0; + test_rt_34685(true, true); + } + + @Test public void test_rt_34685_mouseDoubleClick_rowSelectionMode() { + test_rt_34685_commitCount = 0; + test_rt_34685(true, false); + } + + private int test_rt_34685_commitCount = 0; + private void test_rt_34685(boolean useMouseToInitiateEdit, boolean cellSelectionModeEnabled) { + assertEquals(0, test_rt_34685_commitCount); + + Person person1; + ObservableList> persons = FXCollections.observableArrayList( + new TreeItem<>(person1 = new Person("John", "Smith", "john.smith@example.com")) + ); + + TreeTableView table = new TreeTableView<>(); + table.getSelectionModel().setCellSelectionEnabled(cellSelectionModeEnabled); + table.getSelectionModel().setSelectionMode(SelectionMode.SINGLE); + table.setEditable(true); + + TreeItem root = new TreeItem(new Person("Root", null, null)); + root.setExpanded(true); + table.setRoot(root); + table.setShowRoot(false); + root.getChildren().setAll(persons); + + TreeTableColumn first = new TreeTableColumn("First Name"); + first.setCellValueFactory(new TreeItemPropertyValueFactory("firstName")); + first.setCellFactory(TextFieldTreeTableCell.forTreeTableColumn()); + + EventHandler> onEditCommit = first.getOnEditCommit(); + first.setOnEditCommit(new EventHandler>() { + @Override public void handle(TreeTableColumn.CellEditEvent event) { + test_rt_34685_commitCount++; + onEditCommit.handle(event); + } + }); + + table.getColumns().addAll(first); + + // get the cell at (0,0) - we're hiding the root row + VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE = true; + TreeTableCell cell = (TreeTableCell) VirtualFlowTestUtils.getCell(table, 0, 0); + VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE = false; + assertTrue(cell.getSkin() instanceof TreeTableCellSkin); + assertNull(cell.getGraphic()); + assertEquals("John", cell.getText()); + assertEquals("John", person1.getFirstName()); + + // set the table to be editing the first cell at 0,0 + if (useMouseToInitiateEdit) { + MouseEventFirer mouse = new MouseEventFirer(cell); + mouse.fireMousePressAndRelease(2, 10, 10); // click 10 pixels in and 10 pixels down + } else { + table.edit(0,first); + } + + Toolkit.getToolkit().firePulse(); + assertNotNull(cell.getGraphic()); + assertTrue(cell.getGraphic() instanceof TextField); + + TextField textField = (TextField) cell.getGraphic(); + assertEquals("John", textField.getText()); + + textField.setText("Andrew"); + textField.requestFocus(); + Toolkit.getToolkit().firePulse(); + + KeyEventFirer keyboard = new KeyEventFirer(textField); + keyboard.doKeyPress(KeyCode.ENTER); + + VirtualFlowTestUtils.getVirtualFlow(table).requestLayout(); + Toolkit.getToolkit().firePulse(); + + VirtualFlowTestUtils.assertTableCellTextEquals(table, 0, 0, "Andrew"); + assertEquals("Andrew", cell.getText()); + assertEquals("Andrew", person1.getFirstName()); + assertEquals(1, test_rt_34685_commitCount); + } } diff --git a/modules/controls/src/test/java/javafx/scene/control/TreeViewMouseInputTest.java b/modules/controls/src/test/java/javafx/scene/control/TreeViewMouseInputTest.java --- a/modules/controls/src/test/java/javafx/scene/control/TreeViewMouseInputTest.java +++ b/modules/controls/src/test/java/javafx/scene/control/TreeViewMouseInputTest.java @@ -57,10 +57,7 @@ private TreeView treeView; private MultipleSelectionModel> sm; private FocusModel> fm; - - private KeyEventFirer keyboard; - private StageLoader stageLoader; - + private TreeItem root; // 0 private TreeItem child1; // 1 private TreeItem child2; // 2 @@ -124,18 +121,10 @@ sm = treeView.getSelectionModel(); sm.setSelectionMode(SelectionMode.MULTIPLE); fm = treeView.getFocusModel(); - - // set up keyboard event firer - keyboard = new KeyEventFirer(treeView); - - // create a simple UI that will be shown (to send the keyboard events to) - stageLoader = new StageLoader(treeView); - stageLoader.getStage().show(); } @After public void tearDown() { treeView.getSkin().dispose(); - stageLoader.dispose(); }