Ensure removing a row does not cause exceptions in detail row handling. (#12330)

Fixes: #12328

diff --git a/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java b/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java
index 5290effdfaca02807c8c26293887b55530211c81..b419aef724301ff8807ed89ed308ef87480c7e8f 100644 (file)
--- a/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java
+++ b/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java
@@ -17,6 +17,7 @@ package com.vaadin.client.connectors.grid;
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.TreeMap;
 
 import com.google.gwt.core.client.Scheduler;
@@ -680,6 +681,18 @@ public class DetailsManagerConnector extends AbstractExtensionConnector {
         getWidget().setDetailsVisible(rowIndex, false);
     }
 
+    private void detachDetailsIfFound(String connectorId) {
+        if (indexToDetailConnectorId.containsValue(connectorId)) {
+            for (Entry<Integer, String> entry : indexToDetailConnectorId
+                    .entrySet()) {
+                if (connectorId.equals(entry.getValue())) {
+                    detachDetails(entry.getKey());
+                    return;
+                }
+            }
+        }
+    }
+
     private boolean refreshDetails(int rowIndex) {
         String id = getDetailsComponentConnectorId(rowIndex);
         String oldId = indexToDetailConnectorId.get(rowIndex);
@@ -706,6 +719,11 @@ public class DetailsManagerConnector extends AbstractExtensionConnector {
                 indexToDetailConnectorId.remove(rowIndex);
             } else {
                 // updated, replace reference
+
+                // ensure that the detail contents aren't still attached to some
+                // other row that hasn't been refreshed yet
+                detachDetailsIfFound(id);
+
                 indexToDetailConnectorId.put(rowIndex, id);
                 newOrUpdatedDetails = true;
                 getWidget().resetVisibleDetails(rowIndex);
@@ -714,6 +732,11 @@ public class DetailsManagerConnector extends AbstractExtensionConnector {
             // new Details content, listeners will get attached to the connector
             // when Escalator requests for the Details through
             // CustomDetailsGenerator#getDetails(int)
+
+            // ensure that the detail contents aren't still attached to some
+            // other row that hasn't been refreshed yet
+            detachDetailsIfFound(id);
+
             indexToDetailConnectorId.put(rowIndex, id);
             newOrUpdatedDetails = true;
             getWidget().setDetailsVisible(rowIndex, true);

diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpen.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpen.java
new file mode 100644 (file)
index 0000000..55a6049
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpen.java
@@ -0,0 +1,63 @@
+package com.vaadin.tests.components.grid;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import com.vaadin.data.provider.ListDataProvider;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.HorizontalLayout;
+import com.vaadin.ui.Label;
+
+public class GridRemoveItemAllDetailsOpen extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        List<String> data = new ArrayList<>(
+                Arrays.asList("row1", "row2", "row3", "row4"));
+        Grid<String> grid = createGrid();
+
+        ListDataProvider<String> dataProvider = new ListDataProvider<>(data);
+        grid.setDataProvider(dataProvider);
+        data.forEach(item -> grid.setDetailsVisible(item, true));
+
+        Button removeBtn = new Button("Remove selected item");
+        removeBtn.addClickListener(event -> {
+            data.remove(grid.getSelectedItems().iterator().next());
+            dataProvider.refreshAll();
+            grid.deselectAll();
+        });
+        addComponent(removeBtn);
+        addComponent(grid);
+    }
+
+    private Grid<String> createGrid() {
+        Grid<String> grid = new Grid<>();
+        grid.setHeight("400px");
+        grid.addColumn(item -> item).setCaption("column").setId("column");
+        grid.setDetailsGenerator(item -> {
+            Button closeBtn = new Button("Close");
+            closeBtn.addClickListener(
+                    clickEvent -> grid.setDetailsVisible(item, false));
+            return new HorizontalLayout(new Label("Item details: " + item),
+                    closeBtn);
+        });
+        grid.addItemClickListener(
+                itemClick -> grid.setDetailsVisible(itemClick.getItem(), true));
+        return grid;
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 12328;
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "Removing selected item (first or second)"
+                + "should not cause a client side exception.";
+    }
+}

diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpenTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpenTest.java
new file mode 100644 (file)
index 0000000..25db6e4
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpenTest.java
@@ -0,0 +1,62 @@
+package com.vaadin.tests.components.grid;
+
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+public class GridRemoveItemAllDetailsOpenTest extends MultiBrowserTest {
+
+    @Test
+    public void removeFirstItem() {
+        openTestURL();
+        ButtonElement removeButton = $(ButtonElement.class).first();
+        GridElement grid = $(GridElement.class).first();
+
+        String detailsText = grid.getDetails(2).getText();
+        String expected = "Item details: row3";
+        assertContains(expected, detailsText);
+
+        grid.getCell(0, 0).click();
+        removeButton.click();
+
+        waitUntilLoadingIndicatorNotVisible();
+
+        detailsText = grid.getDetails(1).getText();
+        assertContains(expected, detailsText);
+        detailsText = grid.getDetails(2).getText();
+        expected = "Item details: row4";
+        assertContains(expected, detailsText);
+    }
+
+    @Test
+    public void removeSecondItem() {
+        openTestURL();
+        ButtonElement removeButton = $(ButtonElement.class).first();
+        GridElement grid = $(GridElement.class).first();
+
+        String detailsText = grid.getDetails(2).getText();
+        String expected = "Item details: row3";
+        assertContains(expected, detailsText);
+
+        grid.getCell(1, 0).click();
+        removeButton.click();
+
+        waitUntilLoadingIndicatorNotVisible();
+
+        detailsText = grid.getDetails(1).getText();
+        assertContains(expected, detailsText);
+        detailsText = grid.getDetails(2).getText();
+        expected = "Item details: row4";
+        assertContains(expected, detailsText);
+    }
+
+    private void assertContains(String expected, String detailsText) {
+        assertTrue("Unexpected details contents: " + detailsText
+                + " (expected: " + expected + ")",
+                detailsText.contains(expected));
+    }
+}