Fix detach of grid when it contains frozen columns (#8803)

* Fix detach of grid when it contains frozen columns

Fixes #8748

5 files changed, 142 insertions, 68 deletions

diff --git a/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java b/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java
index 1b2574f4a8..d619b50b5d 100644
--- a/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java
+++ b/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java
@@ -140,8 +140,12 @@ public class ColumnConnector extends AbstractExtensionConnector {
     @Override
     public void onUnregister() {
         super.onUnregister();
-
-        parent.removeColumn(column);
+        if (parent.getParent() != null) {
+            // If the grid itself was unregistered there is no point in spending
+            // time to remove columns (and have problems with frozen columns)
+            // before throwing everything away
+            parent.removeColumn(column);
+        }
         column = null;
     }
 
diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridDetach.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridDetach.java
deleted file mode 100644
index 6c210d6cea..0000000000
--- a/uitest/src/main/java/com/vaadin/tests/components/grid/GridDetach.java
+++ /dev/null
@@ -1,32 +0,0 @@
-package com.vaadin.tests.components.grid;
-
-import com.vaadin.annotations.Widgetset;
-import com.vaadin.server.VaadinRequest;
-import com.vaadin.tests.components.AbstractTestUI;
-import com.vaadin.tests.data.bean.Person;
-import com.vaadin.ui.Button;
-import com.vaadin.ui.Grid;
-
-/**
- * This UI is the application entry point. A UI may either represent a browser
- * window (or tab) or some part of a html page where a Vaadin application is
- * embedded.
- * <p>
- * The UI is initialized using {@link #init(VaadinRequest)}. This method is
- * intended to be overridden to add component to the user interface and
- * initialize non-component functionality.
- */
-@Widgetset("com.vaadin.DefaultWidgetSet")
-public class GridDetach extends AbstractTestUI {
-
-    @Override
-    protected void setup(VaadinRequest vaadinRequest) {
-        Grid<Person> grid = new Grid<>();
-        grid.addColumn(Person::getFirstName).setCaption("asd");
-        grid.addColumn(Person::getAge).setCaption("foobar");
-
-        addComponent(grid);
-        addComponent(new Button("Detach grid", e -> removeComponent(grid)));
-    }
-
-}
diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridRemoveColumnAndDetach.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridRemoveColumnAndDetach.java
new file mode 100644
index 0000000000..cc7d3c17ca
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/grid/GridRemoveColumnAndDetach.java
@@ -0,0 +1,61 @@
+package com.vaadin.tests.components.grid;
+
+import java.util.stream.Collectors;
+
+import com.vaadin.annotations.Widgetset;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.tests.data.bean.Person;
+import com.vaadin.tests.data.bean.Sex;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Grid;
+
+@Widgetset("com.vaadin.DefaultWidgetSet")
+public class GridRemoveColumnAndDetach extends AbstractTestUIWithLog {
+
+    private Grid<Person> grid;
+
+    @Override
+    protected void setup(VaadinRequest vaadinRequest) {
+        grid = new Grid<>();
+        grid.addColumn(Person::getFirstName).setCaption("First").setWidth(200);
+        grid.addColumn(Person::getLastName).setCaption("Last").setHidden(true)
+                .setWidth(200);
+        grid.addColumn(Person::getEmail).setCaption("Email").setWidth(200);
+        grid.addColumn(Person::getAge).setCaption("foobar").setWidth(400);
+
+        grid.setItems(new Person("1", "2", "3", 4, Sex.FEMALE, null));
+        grid.setFrozenColumnCount(3);
+        logFrozenColumns();
+        addComponent(grid);
+
+        Button detachButton = new Button("Detach grid",
+                e -> removeComponent(grid));
+        detachButton.setId("detach");
+        addComponent(detachButton);
+        for (int i = 0; i < 4; i++) {
+            final int idx = i;
+            Button button = new Button("Remove col " + i, e -> {
+                grid.removeColumn(grid.getColumns().get(idx));
+                logFrozenColumns();
+            });
+            button.setId("remove" + i);
+            addComponent(button);
+        }
+    }
+
+    private void logFrozenColumns() {
+        String msg = "Server side frozen columns: ";
+        msg += grid.getColumns().stream().limit(grid.getFrozenColumnCount())
+                .map(column -> {
+                    String caption = column.getCaption();
+                    if (column.isHidden()) {
+                        caption += " (hidden)";
+                    }
+                    return caption;
+                }).collect(Collectors.joining(", "));
+
+        log(msg);
+    }
+
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridDetachTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridDetachTest.java
deleted file mode 100644
index 4d5739acd2..0000000000
--- a/uitest/src/test/java/com/vaadin/tests/components/grid/GridDetachTest.java
+++ /dev/null
@@ -1,34 +0,0 @@
-/*
- * Copyright 2000-2016 Vaadin Ltd.
- *
- * Licensed under the Apache License, Version 2.0 (the "License"); you may not
- * use this file except in compliance with the License. You may obtain a copy of
- * the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
- * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
- * License for the specific language governing permissions and limitations under
- * the License.
- */
-package com.vaadin.tests.components.grid;
-
-import org.junit.Test;
-
-import com.vaadin.testbench.By;
-import com.vaadin.testbench.elements.ButtonElement;
-import com.vaadin.tests.tb3.SingleBrowserTest;
-
-public class GridDetachTest extends SingleBrowserTest {
-
-    @Test
-    public void gridDetachesWithoutErrors() {
-        openTestURL("debug");
-        $(ButtonElement.class).first().click();
-
-        assertElementNotPresent(By.className("v-grid"));
-        assertNoErrorNotifications();
-    }
-}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveColumnAndDetachTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveColumnAndDetachTest.java
new file mode 100644
index 0000000000..930b1c7036
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveColumnAndDetachTest.java
@@ -0,0 +1,75 @@
+/*
+ * Copyright 2000-2016 Vaadin Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.tests.components.grid;
+
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+import com.vaadin.testbench.By;
+import com.vaadin.testbench.TestBenchElement;
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+public class GridRemoveColumnAndDetachTest extends SingleBrowserTest {
+
+    @Test
+    public void gridDetachesWithoutErrors() {
+        openTestURL("debug");
+        $(ButtonElement.class).id("detach").click();
+
+        assertElementNotPresent(By.className("v-grid"));
+        assertNoErrorNotifications();
+    }
+
+    @Test
+    public void frozenColumnCountAfterRemovingHiddenColumn() {
+        openTestURL("debug");
+        assertVisibleFrozenColumns(2);
+        $(ButtonElement.class).id("remove1").click();
+        assertVisibleFrozenColumns(2);
+
+    }
+
+    @Test
+    public void frozenColumnCountAfterWhenRemovingFrozenColumn() {
+        openTestURL("debug");
+        assertVisibleFrozenColumns(2);
+        $(ButtonElement.class).id("remove0").click();
+        assertVisibleFrozenColumns(1);
+
+    }
+
+    private void assertVisibleFrozenColumns(int nrFrozenColumns) {
+        GridElement grid = $(GridElement.class).first();
+        for (int i = 0; i < nrFrozenColumns; i++) {
+            TestBenchElement cell = grid.getCell(0, i);
+            assertTrue("Column " + i + " should be frozen",
+                    cell.hasClassName("frozen"));
+        }
+        assertTrue("Only " + nrFrozenColumns + " should be frozen", grid
+                .getCell(0, nrFrozenColumns - 1).hasClassName("last-frozen"));
+    }
+
+    @Test
+    public void frozenColumnCountAfterWhenRemovingNonFrozenColumn() {
+        openTestURL("debug");
+        assertVisibleFrozenColumns(2);
+        $(ButtonElement.class).id("remove3").click();
+        assertVisibleFrozenColumns(2);
+    }
+}