Fix validation in non-buffered Grid editor (#12281)

Handle possible race condition by disabling the editor's widget while awaiting validation from the server.

Fixes #12270

9 files changed, 314 insertions, 8 deletions

diff --git a/client/src/main/java/com/vaadin/client/connectors/grid/EditorConnector.java b/client/src/main/java/com/vaadin/client/connectors/grid/EditorConnector.java
index 515bb17915..297a360f8b 100644
--- a/client/src/main/java/com/vaadin/client/connectors/grid/EditorConnector.java
+++ b/client/src/main/java/com/vaadin/client/connectors/grid/EditorConnector.java
@@ -29,7 +29,6 @@ import com.vaadin.client.extensions.AbstractExtensionConnector;
 import com.vaadin.client.widget.grid.EditorHandler;
 import com.vaadin.client.widgets.Grid;
 import com.vaadin.client.widgets.Grid.Column;
-import com.vaadin.shared.Range;
 import com.vaadin.shared.data.DataCommunicatorConstants;
 import com.vaadin.shared.ui.Connect;
 import com.vaadin.shared.ui.grid.editor.EditorClientRpc;
@@ -104,6 +103,12 @@ public class EditorConnector extends AbstractExtensionConnector {
                     getParent().getWidget().getEditor()
                             .setEditorError(errorMessage, errorColumns);
                 }
+
+                @Override
+                public void confirmValidity(boolean isValid) {
+                    getParent().getWidget().getEditor().getEventHandler()
+                            .confirmValidity(isValid);
+                }
             });
         }
 
@@ -131,6 +136,11 @@ public class EditorConnector extends AbstractExtensionConnector {
         }
 
         @Override
+        public void checkValidity() {
+            rpc.checkValidity();
+        }
+
+        @Override
         public Widget getWidget(Column<?, JsonObject> column) {
             String connId = getState().columnFields
                     .get(getParent().getColumnId(column));
diff --git a/client/src/main/java/com/vaadin/client/widget/grid/DefaultEditorEventHandler.java b/client/src/main/java/com/vaadin/client/widget/grid/DefaultEditorEventHandler.java
index 4580f16105..a76760af6e 100644
--- a/client/src/main/java/com/vaadin/client/widget/grid/DefaultEditorEventHandler.java
+++ b/client/src/main/java/com/vaadin/client/widget/grid/DefaultEditorEventHandler.java
@@ -15,6 +15,10 @@
  */
 package com.vaadin.client.widget.grid;
 
+import java.util.List;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
 import com.google.gwt.core.client.Duration;
 import com.google.gwt.dom.client.BrowserEvents;
 import com.google.gwt.dom.client.Element;
@@ -30,8 +34,6 @@ import com.vaadin.client.widgets.Grid;
 import com.vaadin.client.widgets.Grid.Editor;
 import com.vaadin.client.widgets.Grid.EditorDomEvent;
 
-import java.util.List;
-
 /**
  * The default handler for Grid editor events. Offers several overridable
  * protected methods for easier customization.
@@ -51,6 +53,7 @@ public class DefaultEditorEventHandler<T> implements Editor.EventHandler<T> {
     private int lastTouchEventX = -1;
     private int lastTouchEventY = -1;
     private int lastTouchEventRow = -1;
+    private PendingEdit pendingEdit;
 
     /**
      * Returns whether the given event is a touch event that should open the
@@ -223,7 +226,16 @@ public class DefaultEditorEventHandler<T> implements Editor.EventHandler<T> {
                     }
                 }
 
-                editRow(event, rowIndex + delta.rowDelta, colIndex);
+                int newRowIndex = rowIndex + delta.rowDelta;
+                if (newRowIndex != event.getRowIndex()) {
+                    triggerValueChangeEvent(event);
+                    // disable until validity check is done
+                    setWidgetEnabled(event.getEditorWidget(), false);
+                    event.getEditor().getHandler().checkValidity();
+                    pendingEdit = new PendingEdit(event, newRowIndex, colIndex);
+                } else {
+                    editRow(event, newRowIndex, colIndex);
+                }
             }
 
             return changed;
@@ -401,10 +413,6 @@ public class DefaultEditorEventHandler<T> implements Editor.EventHandler<T> {
         // Limit colIndex between 0 and colCount - 1
         colIndex = Math.max(0, Math.min(colCount - 1, colIndex));
 
-        if (rowIndex != event.getRowIndex()) {
-            triggerValueChangeEvent(event);
-        }
-
         event.getEditor().editRow(rowIndex, colIndex);
     }
 
@@ -451,4 +459,53 @@ public class DefaultEditorEventHandler<T> implements Editor.EventHandler<T> {
 
         return handled || swallowEvent;
     }
+
+    @Override
+    public void confirmValidity(boolean isValid) {
+        if (pendingEdit == null) {
+            getLogger().log(Level.SEVERE,
+                    "An editor's validation confirmation was received, but"
+                            + " no pending edit object was found ");
+            return;
+        }
+        setWidgetEnabled(pendingEdit.pendingEvent.getEditorWidget(), true);
+        if (isValid) {
+            editRow(pendingEdit.pendingEvent, pendingEdit.pendingRowIndex,
+                    pendingEdit.pendingColIndex);
+        } else {
+            pendingEdit.pendingEvent.getEditorWidget().getElement().focus();
+        }
+
+        pendingEdit = null;
+    }
+
+    private void setWidgetEnabled(Widget widget, boolean widgetEnabled) {
+        final ComponentConnector connector = Util.findConnectorFor(widget);
+        // only enable widget if it hasn't been disabled programmatically
+        if (connector.getState().enabled) {
+            connector.setWidgetEnabled(widgetEnabled);
+        }
+    }
+
+    private static final Logger getLogger() {
+        return Logger.getLogger(DefaultEditorEventHandler.class.getName());
+    }
+
+    private final class PendingEdit {
+        private EditorDomEvent<T> pendingEvent;
+        private int pendingRowIndex;
+        private int pendingColIndex;
+
+        private PendingEdit(EditorDomEvent<T> pendingEvent, int pendingRowIndex,
+                int pendingColIndex) {
+            if (pendingEvent == null) {
+                throw new IllegalArgumentException(
+                        "The pending event cannot be null");
+            }
+            this.pendingEvent = pendingEvent;
+            this.pendingRowIndex = pendingRowIndex;
+            this.pendingColIndex = pendingColIndex;
+        }
+
+    }
 }
diff --git a/client/src/main/java/com/vaadin/client/widget/grid/EditorHandler.java b/client/src/main/java/com/vaadin/client/widget/grid/EditorHandler.java
index f603ebb5ea..958af5061b 100644
--- a/client/src/main/java/com/vaadin/client/widget/grid/EditorHandler.java
+++ b/client/src/main/java/com/vaadin/client/widget/grid/EditorHandler.java
@@ -189,4 +189,11 @@ public interface EditorHandler<T> {
      *         editable
      */
     public Widget getWidget(Grid.Column<?, T> column);
+
+    /**
+     * Called by the editor's event handler when editing is shifting to a new
+     * row in order to check the validity of the binder's value.
+     *
+     */
+    public void checkValidity();
 }
diff --git a/client/src/main/java/com/vaadin/client/widgets/Grid.java b/client/src/main/java/com/vaadin/client/widgets/Grid.java
index c5fe79d260..6e0b9aaf59 100755
--- a/client/src/main/java/com/vaadin/client/widgets/Grid.java
+++ b/client/src/main/java/com/vaadin/client/widgets/Grid.java
@@ -1416,6 +1416,15 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
              *         done, false otherwise
              */
             boolean handleEvent(EditorDomEvent<T> event);
+
+            /**
+             * Confirms the valid status of the binder so as to determine
+             * whether to allow pending navigation action.
+             *
+             * @param isValid
+             *            {@code true} if the binder value is valid
+             */
+            void confirmValidity(boolean isValid);
         }
 
         protected enum State {
diff --git a/server/src/main/java/com/vaadin/ui/components/grid/EditorImpl.java b/server/src/main/java/com/vaadin/ui/components/grid/EditorImpl.java
index f7943c8f9c..32f3753ac7 100644
--- a/server/src/main/java/com/vaadin/ui/components/grid/EditorImpl.java
+++ b/server/src/main/java/com/vaadin/ui/components/grid/EditorImpl.java
@@ -151,6 +151,11 @@ public class EditorImpl<T> extends AbstractGridExtension<T>
                 rpc.confirmBind(true);
                 doEdit(getData(key));
             }
+
+            @Override
+            public void checkValidity() {
+                rpc.confirmValidity(getBinder().validate().isOk());
+            }
         });
 
         setBinder(Binder.withPropertySet(propertySet));
diff --git a/shared/src/main/java/com/vaadin/shared/ui/grid/editor/EditorClientRpc.java b/shared/src/main/java/com/vaadin/shared/ui/grid/editor/EditorClientRpc.java
index ce0499e0f6..356ac1a949 100644
--- a/shared/src/main/java/com/vaadin/shared/ui/grid/editor/EditorClientRpc.java
+++ b/shared/src/main/java/com/vaadin/shared/ui/grid/editor/EditorClientRpc.java
@@ -70,4 +70,13 @@ public interface EditorClientRpc extends ClientRpc {
      *
      */
     void setErrorMessage(String errorMessage, List<String> errorColumnsIds);
+
+    /**
+     * Confirms whether the binder's validation has passed so as to determine
+     * whether to allow the pending navigation action.
+     *
+     * @param isValid
+     *            {@code true} if the binder value is valid
+     */
+    void confirmValidity(boolean isValid);
 }
diff --git a/shared/src/main/java/com/vaadin/shared/ui/grid/editor/EditorServerRpc.java b/shared/src/main/java/com/vaadin/shared/ui/grid/editor/EditorServerRpc.java
index e88e365eb1..4de49f25ba 100644
--- a/shared/src/main/java/com/vaadin/shared/ui/grid/editor/EditorServerRpc.java
+++ b/shared/src/main/java/com/vaadin/shared/ui/grid/editor/EditorServerRpc.java
@@ -53,4 +53,11 @@ public interface EditorServerRpc extends ServerRpc {
      *            after save action, otherwise it represents a cancel action
      */
     void cancel(boolean afterBeingSaved);
+
+    /**
+     * Asks the server to check the validity of the current values in the
+     * editor. When a check-validity request is sent, the server must respond
+     * with a {@link EditorClientRpc#confirmValidity(boolean) confirm call}.
+     */
+    void checkValidity();
 }
diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridEditorNonBuffered.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridEditorNonBuffered.java
new file mode 100644
index 0000000000..adf49d3870
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/grid/GridEditorNonBuffered.java
@@ -0,0 +1,103 @@
+package com.vaadin.tests.components.grid;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Locale;
+import java.util.Random;
+
+import com.vaadin.data.Binder;
+import com.vaadin.data.Binder.Binding;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractReindeerTestUIWithLog;
+import com.vaadin.tests.util.Person;
+import com.vaadin.tests.util.TestDataGenerator;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.Grid.Column;
+import com.vaadin.ui.TextField;
+import com.vaadin.ui.renderers.NumberRenderer;
+
+public class GridEditorNonBuffered extends AbstractReindeerTestUIWithLog {
+
+    final static String VALIDATION_ERROR_MESSAGE = "Validator error. Name cannot be empty";
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        Grid<Person> grid = createGrid();
+        grid.setItems(createTestData());
+        addComponent(grid);
+    }
+
+    protected Collection<Person> createTestData() {
+        return createTestData(100);
+    }
+
+    protected Collection<Person> createTestData(int size) {
+        Random r = new Random(0);
+        List<Person> testData = new ArrayList<>();
+        for (int i = 0; i < size; i++) {
+            Person person = new Person();
+            person.setFirstName(TestDataGenerator.getFirstName(r));
+            person.setLastName(TestDataGenerator.getLastName(r));
+            person.getAddress().setCity(TestDataGenerator.getCity(r));
+            person.setEmail(person.getFirstName().toLowerCase(Locale.ROOT) + "."
+                    + person.getLastName().toLowerCase(Locale.ROOT)
+                    + "@vaadin.com");
+            person.setPhoneNumber(TestDataGenerator.getPhoneNumber(r));
+
+            person.getAddress()
+                    .setPostalCode(TestDataGenerator.getPostalCode(r));
+            person.getAddress()
+                    .setStreetAddress(TestDataGenerator.getStreetAddress(r));
+            testData.add(person);
+        }
+        return testData;
+    }
+
+    protected Grid<Person> createGrid() {
+        Grid<Person> grid = new Grid<>();
+
+        grid.addColumn(Person::getEmail).setCaption("Email").setId("email");
+
+        Column<Person, String> firstNameColumn = grid
+                .addColumn(Person::getFirstName).setCaption("First Name")
+                .setId("firstName");
+        Column<Person, String> lastNameColumn = grid
+                .addColumn(Person::getLastName).setCaption("Last Name")
+                .setId("lastName");
+
+        grid.addColumn(Person::getPhoneNumber).setCaption("Phone Number")
+                .setId("phone");
+        grid.addColumn(person -> person.getAddress().getStreetAddress())
+                .setCaption("Street Address").setId("street");
+        grid.addColumn(person -> person.getAddress().getPostalCode(),
+                new NumberRenderer()).setCaption("Postal Code").setId("zip");
+        grid.addColumn(person -> person.getAddress().getCity())
+                .setCaption("City").setId("city");
+
+        Binder<Person> binder = new Binder<>();
+
+        TextField firstNameEditor = new TextField();
+        Binding<Person, String> firstNamebinding = binder
+                .forField(firstNameEditor)
+                .withValidator(v -> (v != null && !v.isEmpty()),
+                        VALIDATION_ERROR_MESSAGE)
+                .bind(Person::getFirstName, Person::setFirstName);
+        firstNameColumn.setEditorBinding(firstNamebinding);
+
+        TextField lastNameEditor = new TextField();
+        Binding<Person, String> lastNamebinding = binder
+                .forField(lastNameEditor)
+                .withValidator(v -> (v != null && !v.isEmpty()),
+                        VALIDATION_ERROR_MESSAGE)
+                .bind(Person::getLastName, Person::setLastName);
+        lastNameColumn.setEditorBinding(lastNamebinding);
+
+        grid.getEditor().setBuffered(false);
+        grid.getEditor().setEnabled(true);
+        grid.getEditor().setBinder(binder);
+
+        return grid;
+    }
+
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridEditorNonBufferedTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridEditorNonBufferedTest.java
new file mode 100644
index 0000000000..eaa88adcbc
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridEditorNonBufferedTest.java
@@ -0,0 +1,99 @@
+package com.vaadin.tests.components.grid;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+import org.openqa.selenium.By;
+import org.openqa.selenium.Keys;
+import org.openqa.selenium.WebElement;
+import org.openqa.selenium.interactions.Actions;
+
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.testbench.elements.GridElement.GridCellElement;
+import com.vaadin.testbench.elements.NotificationElement;
+import com.vaadin.testbench.elements.TextFieldElement;
+import com.vaadin.testbench.parallel.TestCategory;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+@TestCategory("grid")
+public class GridEditorNonBufferedTest extends MultiBrowserTest {
+
+    @Override
+    public void setup() throws Exception {
+        super.setup();
+
+        setDebug(true);
+        openTestURL();
+    }
+
+    private void openEditor(int rowIndex, int cellIndex) {
+        GridElement grid = $(GridElement.class).first();
+        GridCellElement cell = grid.getCell(rowIndex, cellIndex);
+        new Actions(driver).moveToElement(cell).doubleClick().build().perform();
+    }
+
+    @Test
+    public void testEditor() {
+        openEditor(5, 1);
+
+        assertTrue("Editor should be opened with a TextField",
+                isElementPresent(TextFieldElement.class));
+
+        assertFalse("Notification was present",
+                isElementPresent(NotificationElement.class));
+    }
+
+    @Test
+    public void testEscClosesEditor() {
+        openEditor(5, 1);
+
+        assertTrue("Editor should be opened with a TextField",
+                isElementPresent(TextFieldElement.class));
+
+        new Actions(getDriver()).sendKeys(Keys.ESCAPE).perform();
+
+        assertFalse("Editor should be closed",
+                isElementPresent(TextFieldElement.class));
+    }
+
+    @Test
+    public void preventNavigationToNextRowIfEditorValueIsInvalid() {
+        // Test with navigation to next row
+        openEditor(5, 2);
+        WebElement field = findElement(By.className("v-textfield-focus"));
+        String selectAll = Keys.chord(Keys.CONTROL, "a");
+        field.sendKeys(selectAll);
+        field.sendKeys(Keys.DELETE);
+        field.sendKeys(Keys.TAB);
+
+        String editorMessage = getEditorMessage();
+
+        assertEquals(
+                "Last Name: " + GridEditorNonBuffered.VALIDATION_ERROR_MESSAGE,
+                editorMessage);
+    }
+
+    @Test
+    public void preventNavigationToPrevRowIfEditorValueIsInvalid() {
+        // Test with navigation to previous row
+        openEditor(5, 1);
+        WebElement field = findElement(By.className("v-textfield-focus"));
+        String selectAll = Keys.chord(Keys.CONTROL, "a");
+        field.sendKeys(selectAll);
+        field.sendKeys(Keys.DELETE);
+        field.sendKeys(Keys.chord(Keys.SHIFT, Keys.TAB));
+        String editorMessage = getEditorMessage();
+
+        assertEquals(
+                "First Name: " + GridEditorNonBuffered.VALIDATION_ERROR_MESSAGE,
+                editorMessage);
+    }
+
+    private String getEditorMessage() {
+        return findElement(
+                By.xpath("//div[@class = 'v-grid-editor-message']/div"))
+                        .getText();
+    }
+}