8187307: ListView, TableView, TreeView: receives editCancel event when edit is committed

Reviewed-by: mhanl, aghaisas

Author: Jeanette Winzenburg

modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java

@@ -386,8 +386,16 @@ public void changed(
     /** {@inheritDoc} */
     @Override public void commitEdit(T newValue) {
         if (! isEditing()) return;
-        ListView<T> list = getListView();
 
+        // inform parent classes of the commit, so that they can switch us
+        // out of the editing state.
+        // This MUST come before the updateItem call below, otherwise it will
+        // call cancelEdit(), resulting in both commit and cancel events being
+        // fired (as identified in RT-29650)
+        super.commitEdit(newValue);
+
+        ListView<T> list = getListView();
+        // JDK-8187307: fire the commit after updating cell's editing state
         if (list != null) {
             // Inform the ListView of the edit being ready to be committed.
             list.fireEvent(new ListView.EditEvent<T>(list,
@@ -396,13 +404,6 @@ public void changed(
                     list.getEditingIndex()));
         }
 
-        // inform parent classes of the commit, so that they can switch us
-        // out of the editing state.
-        // This MUST come before the updateItem call below, otherwise it will
-        // call cancelEdit(), resulting in both commit and cancel events being
-        // fired (as identified in RT-29650)
-        super.commitEdit(newValue);
-
         // update the item within this cell, so that it represents the new value
         updateItem(newValue, false);
 

modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java

@@ -353,26 +353,27 @@ TablePosition<S, T> getEditingCellAtStartEdit() {
     @Override public void commitEdit(T newValue) {
         if (!isEditing()) return;
 
+        // inform parent classes of the commit, so that they can switch us
+        // out of the editing state.
+        // This MUST come before the updateItem call below, otherwise it will
+        // call cancelEdit(), resulting in both commit and cancel events being
+        // fired (as identified in RT-29650)
+        super.commitEdit(newValue);
+
         final TableView<S> table = getTableView();
+        // JDK-8187307: fire the commit after updating cell's editing state
         if (getTableColumn() != null) {
             // Inform the TableColumn of the edit being ready to be committed.
             CellEditEvent<S, T> editEvent = new CellEditEvent<>(
-                table,
-                editingCellAtStartEdit,
-                TableColumn.editCommitEvent(),
-                newValue
-            );
+                    table,
+                    editingCellAtStartEdit,
+                    TableColumn.editCommitEvent(),
+                    newValue
+                    );
 
             Event.fireEvent(getTableColumn(), editEvent);
         }
 
-        // inform parent classes of the commit, so that they can switch us
-        // out of the editing state.
-        // This MUST come before the updateItem call below, otherwise it will
-        // call cancelEdit(), resulting in both commit and cancel events being
-        // fired (as identified in RT-29650)
-        super.commitEdit(newValue);
-
         // update the item within this cell, so that it represents the new value
         updateItem(newValue, false);
 

modules/javafx.controls/src/main/java/javafx/scene/control/TreeCell.java

@@ -394,8 +394,17 @@ public String getName() {
      /** {@inheritDoc} */
     @Override public void commitEdit(T newValue) {
         if (! isEditing()) return;
+
+        // inform parent classes of the commit, so that they can switch us
+        // out of the editing state.
+        // This MUST come before the updateItem call below, otherwise it will
+        // call cancelEdit(), resulting in both commit and cancel events being
+        // fired (as identified in RT-29650)
+        super.commitEdit(newValue);
+
         final TreeItem<T> treeItem = getTreeItem();
         final TreeView<T> tree = getTreeView();
+        // JDK-8187307: fire the commit after updating cell's editing state
         if (tree != null) {
             // Inform the TreeView of the edit being ready to be committed.
             tree.fireEvent(new TreeView.EditEvent<T>(tree,
@@ -405,13 +414,6 @@ public String getName() {
                     newValue));
         }
 
-        // inform parent classes of the commit, so that they can switch us
-        // out of the editing state.
-        // This MUST come before the updateItem call below, otherwise it will
-        // call cancelEdit(), resulting in both commit and cancel events being
-        // fired (as identified in RT-29650)
-        super.commitEdit(newValue);
-
         // update the item within this cell, so that it represents the new value
         if (treeItem != null) {
             treeItem.setValue(newValue);

modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableCell.java

@@ -370,26 +370,27 @@ TreeTablePosition<S, T> getEditingCellAtStartEdit() {
     @Override public void commitEdit(T newValue) {
         if (!isEditing()) return;
 
+        // inform parent classes of the commit, so that they can switch us
+        // out of the editing state.
+        // This MUST come before the updateItem call below, otherwise it will
+        // call cancelEdit(), resulting in both commit and cancel events being
+        // fired (as identified in RT-29650)
+        super.commitEdit(newValue);
+
         final TreeTableView<S> table = getTreeTableView();
+        // JDK-8187307: fire the commit after updating cell's editing state
         if (getTableColumn() != null) {
             // Inform the TreeTableColumn of the edit being ready to be committed.
             CellEditEvent<S,T> editEvent = new CellEditEvent<S,T>(
-                table,
-                editingCellAtStartEdit,
-                TreeTableColumn.<S,T>editCommitEvent(),
-                newValue
-            );
+                    table,
+                    editingCellAtStartEdit,
+                    TreeTableColumn.<S,T>editCommitEvent(),
+                    newValue
+                    );
 
             Event.fireEvent(getTableColumn(), editEvent);
         }
 
-        // inform parent classes of the commit, so that they can switch us
-        // out of the editing state.
-        // This MUST come before the updateItem call below, otherwise it will
-        // call cancelEdit(), resulting in both commit and cancel events being
-        // fired (as identified in RT-29650)
-        super.commitEdit(newValue);
-
         // update the item within this cell, so that it represents the new value
         updateItem(newValue, false);
 

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

@@ -874,6 +874,26 @@ public void testStartEditOffRangeMustNotUpdateEditingLocation() {
         assertEquals("list editing location must not be updated", -1, list.getEditingIndex());
     }
 
+    @Test
+    public void testCommitEditMustNotFireCancel() {
+        list.setEditable(true);
+        // JDK-8187307: handler that resets control's editing state
+        list.setOnEditCommit(e -> {
+            int index = e.getIndex();
+            list.getItems().set(index, e.getNewValue());
+            list.edit(-1);
+        });
+        cell.updateListView(list);
+        int editingIndex = 1;
+        cell.updateIndex(editingIndex);
+        list.edit(editingIndex);
+        List<EditEvent<String>> events = new ArrayList<>();
+        list.setOnEditCancel(events::add);
+        String value = "edited";
+        cell.commitEdit(value);
+        assertEquals("sanity: value committed", value, list.getItems().get(editingIndex));
+        assertEquals("commit must not have fired editCancel", 0, events.size());
+    }
 
     // When the list view item's change and affects a cell that is editing, then what?
     // When the list cell's index is changed while it is editing, then what?

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

@@ -600,7 +600,28 @@ public void testEditStartEventAfterStartOnTable() {
     }
 
 //------------- commitEdit
- // fix of JDK-8271474 changed the implementation of how the editing location is evaluated
+
+    @Test
+    public void testCommitEditMustNotFireCancel() {
+        setupForEditing();
+        // JDK-8187307: handler that resets control's editing state
+        editingColumn.setOnEditCommit(e -> {
+            table.getItems().set(e.getTablePosition().getRow(), e.getNewValue());
+            table.edit(-1, null);
+        });
+        int editingRow = 1;
+        cell.updateIndex(editingRow);
+        table.edit(editingRow, editingColumn);
+        List<CellEditEvent<?, ?>> events = new ArrayList<>();
+        editingColumn.setOnEditCancel(events::add);
+        String value = "edited";
+        cell.commitEdit(value);
+        assertEquals("sanity: value committed", value, table.getItems().get(editingRow));
+        assertEquals("commit must not have fired editCancel", 0, events.size());
+    }
+
+
+// fix of JDK-8271474 changed the implementation of how the editing location is evaluated
 
      @Test
      public void testEditCommitEvent() {

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

@@ -867,6 +867,27 @@ public void testStartEditOffRangeMustNotUpdateEditingLocation() {
         assertNull("tree editing location must not be updated", tree.getEditingItem());
     }
 
+    @Test
+    public void testCommitEditMustNotFireCancel() {
+        tree.setEditable(true);
+        int editingIndex = 1;
+        TreeItem<String> editingItem = tree.getTreeItem(editingIndex);
+        // JDK-8187307: handler that resets control's editing state
+        tree.setOnEditCommit(e -> {
+            editingItem.setValue(e.getNewValue());
+            tree.edit(null);
+        });
+        cell.updateTreeView(tree);
+        cell.updateIndex(editingIndex);
+        List<EditEvent<String>> events = new ArrayList<>();
+        tree.setOnEditCancel(events::add);
+        tree.edit(editingItem);
+        String value = "edited";
+        cell.commitEdit(value);
+        assertEquals("sanity: value committed", value, tree.getTreeItem(editingIndex).getValue());
+        assertEquals("commit must not have fired editCancel", 0, events.size());
+    }
+
 
     // When the tree view item's change and affects a cell that is editing, then what?
     // When the tree cell's index is changed while it is editing, then what?

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

@@ -912,7 +912,28 @@ public void testEditStartEventAfterStartOnTable() {
     }
 
  //------------- commitEdit
- // fix of JDK-8271474 changed the implementation of how the editing location is evaluated
+
+    @Test
+    public void testCommitEditMustNotFireCancel() {
+        setupForEditing();
+        // JDK-8187307: handler that resets control's editing state
+        editingColumn.setOnEditCommit(e -> {
+            TreeItem<String> treeItem = tree.getTreeItem(e.getTreeTablePosition().getRow());
+            treeItem.setValue(e.getNewValue());
+            tree.edit(-1, null);
+        });
+        int editingRow = 1;
+        cell.updateIndex(editingRow);
+        tree.edit(editingRow, editingColumn);
+        List<CellEditEvent<?, ?>> events = new ArrayList<>();
+        editingColumn.setOnEditCancel(events::add);
+        String value = "edited";
+        cell.commitEdit(value);
+        assertEquals("sanity: value committed", value, tree.getTreeItem(editingRow).getValue());
+        assertEquals("commit must not have fired editCancel", 0, events.size());
+    }
+
+// fix of JDK-8271474 changed the implementation of how the editing location is evaluated
 
      @Test
      public void testEditCommitEvent() {