Fix TabSheet attaching and detaching components (#10988)8.4.4

This patch reverts the fix #10557 and replaces it with a
proper solution from Grid perspective.

Fixes #10987
Fixes #10985

7 files changed, 76 insertions, 41 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 31803c235c..76a79693ca 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
@@ -72,8 +72,11 @@ public class EditorConnector extends AbstractExtensionConnector {
 
                 @Override
                 public void cancel() {
-                    serverInitiated = true;
-                    getParent().getWidget().cancelEditor();
+                    // Canceling an editor that is not open is a no-op.
+                    if (getParent().getWidget().isEditorActive()) {
+                        serverInitiated = true;
+                        getParent().getWidget().cancelEditor();
+                    }
                 }
 
                 @Override
diff --git a/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java b/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java
index 574ed99045..3df69d45f3 100644
--- a/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java
+++ b/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java
@@ -220,11 +220,6 @@ public class DataCommunicator<T> extends AbstractExtension {
     public void attach() {
         super.attach();
         attachDataProviderListener();
-
-        if (getPushRows().isEmpty()) {
-            // Make sure rows are pushed when component is attached.
-            setPushRows(Range.withLength(0, getMinPushSize()));
-        }
     }
 
     @Override
@@ -313,6 +308,11 @@ public class DataCommunicator<T> extends AbstractExtension {
     public void beforeClientResponse(boolean initial) {
         super.beforeClientResponse(initial);
 
+        if (initial && getPushRows().isEmpty()) {
+            // Make sure rows are pushed when component is attached.
+            setPushRows(Range.withLength(0, getMinPushSize()));
+        }
+
         sendDataToClient(initial);
     }
 
diff --git a/server/src/main/java/com/vaadin/ui/Grid.java b/server/src/main/java/com/vaadin/ui/Grid.java
index a6d449f49e..8bfe90cbbb 100644
--- a/server/src/main/java/com/vaadin/ui/Grid.java
+++ b/server/src/main/java/com/vaadin/ui/Grid.java
@@ -2507,6 +2507,16 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
                 .forEach(this::addColumn);
     }
 
+    @Override
+    public void beforeClientResponse(boolean initial) {
+        super.beforeClientResponse(initial);
+
+        if (initial && editor.isOpen()) {
+            // Re-attaching grid. Any old editor should be closed.
+            editor.cancel();
+        }
+    }
+
     /**
      * Sets the property set to use for this grid. Does not create or update
      * columns in any way but will delete and re-create the editor.
@@ -4776,20 +4786,4 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
             column.updateSortable();
         }
     }
-
-    @Override
-    public void setVisible(boolean visible) {
-        if (getEditor().isOpen() && !visible) {
-            getEditor().cancel();
-        }
-        super.setVisible(visible);
-    }
-
-    @Override
-    public void detach() {
-        if (getEditor().isOpen()) {
-            getEditor().cancel();
-        }
-        super.detach();
-    }
 }
diff --git a/server/src/main/java/com/vaadin/ui/TabSheet.java b/server/src/main/java/com/vaadin/ui/TabSheet.java
index 39621b075e..1c3ff4fcd6 100644
--- a/server/src/main/java/com/vaadin/ui/TabSheet.java
+++ b/server/src/main/java/com/vaadin/ui/TabSheet.java
@@ -585,10 +585,6 @@ public class TabSheet extends AbstractComponentContainer
      */
     private void setSelected(Component component) {
         Tab tab = tabs.get(selected);
-        if (tab != null && !Objects.equals(tab.getComponent(), component) && tab.getComponent() != null && tab.getComponent().isAttached()) {
-            tab.getComponent().detach();
-            tab.getComponent().attach(); // ugly hack
-        }
 
         selected = component;
         // Repaint of the selected component is needed as only the selected
@@ -1507,7 +1503,6 @@ public class TabSheet extends AbstractComponentContainer
 
     /*
      * (non-Javadoc)
-     *
      * @see com.vaadin.ui.AbstractComponent#readDesign(org.jsoup.nodes .Element,
      * com.vaadin.ui.declarative.DesignContext)
      */
@@ -1643,7 +1638,6 @@ public class TabSheet extends AbstractComponentContainer
 
     /*
      * (non-Javadoc)
-     *
      * @see com.vaadin.ui.AbstractComponent#getCustomAttributes()
      */
     @Override
@@ -1655,7 +1649,6 @@ public class TabSheet extends AbstractComponentContainer
 
     /*
      * (non-Javadoc)
-     *
      * @see com.vaadin.ui.AbstractComponent#writeDesign(org.jsoup.nodes.Element
      * , com.vaadin.ui.declarative.DesignContext)
      */
diff --git a/server/src/test/java/com/vaadin/ui/AbstractListingTest.java b/server/src/test/java/com/vaadin/ui/AbstractListingTest.java
index 86f6929d38..77cd7779c3 100644
--- a/server/src/test/java/com/vaadin/ui/AbstractListingTest.java
+++ b/server/src/test/java/com/vaadin/ui/AbstractListingTest.java
@@ -30,9 +30,13 @@ public class AbstractListingTest {
 
         /**
          * Used to execute data generation
+         * 
+         * @param initial
+         *            {@code true} to mock initial data request; {@code false}
+         *            for follow-up request.
          */
-        public void runDataGeneration() {
-            super.getDataCommunicator().beforeClientResponse(true);
+        public void runDataGeneration(boolean initial) {
+            super.getDataCommunicator().beforeClientResponse(initial);
         }
 
         @Override
@@ -129,7 +133,7 @@ public class AbstractListingTest {
         CountGenerator generator = new CountGenerator();
         generator.extend(listing);
         listing.setItems("Foo");
-        listing.runDataGeneration();
+        listing.runDataGeneration(true);
         assertEquals("Generator should have been called once", 1,
                 generator.callCount);
     }
@@ -139,7 +143,7 @@ public class AbstractListingTest {
         CountGenerator generator = new CountGenerator();
         listing.setItems("Foo");
         generator.extend(listing);
-        listing.runDataGeneration();
+        listing.runDataGeneration(true);
         assertEquals("Generator should have been called once", 1,
                 generator.callCount);
     }
@@ -149,10 +153,10 @@ public class AbstractListingTest {
         listing.setItems("Foo");
         CountGenerator generator = new CountGenerator();
         generator.extend(listing);
-        listing.runDataGeneration();
+        listing.runDataGeneration(true);
         assertEquals("Generator should have been called once", 1,
                 generator.callCount);
-        listing.runDataGeneration();
+        listing.runDataGeneration(false);
         assertEquals("Generator should not have been called again", 1,
                 generator.callCount);
     }
@@ -163,7 +167,7 @@ public class AbstractListingTest {
         CountGenerator generator = new CountGenerator();
         generator.extend(listing);
         generator.remove();
-        listing.runDataGeneration();
+        listing.runDataGeneration(true);
         assertEquals("Generator should not have been called", 0,
                 generator.callCount);
     }
@@ -173,11 +177,11 @@ public class AbstractListingTest {
         listing.setItems("Foo");
         CountGenerator generator = new CountGenerator();
         generator.extend(listing);
-        listing.runDataGeneration();
+        listing.runDataGeneration(true);
         assertEquals("Generator should have been called once", 1,
                 generator.callCount);
         generator.refresh("Foo");
-        listing.runDataGeneration();
+        listing.runDataGeneration(false);
         assertEquals("Generator should have been called again", 2,
                 generator.callCount);
     }
diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridInTabSheet.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridInTabSheet.java
index e50aa9759b..75977f56e7 100644
--- a/uitest/src/main/java/com/vaadin/tests/components/grid/GridInTabSheet.java
+++ b/uitest/src/main/java/com/vaadin/tests/components/grid/GridInTabSheet.java
@@ -7,6 +7,7 @@ import java.util.stream.IntStream;
 
 import com.vaadin.annotations.Widgetset;
 import com.vaadin.data.ValueProvider;
+import com.vaadin.data.converter.StringToIntegerConverter;
 import com.vaadin.data.provider.DataProvider;
 import com.vaadin.data.provider.ListDataProvider;
 import com.vaadin.server.VaadinRequest;
@@ -15,6 +16,7 @@ import com.vaadin.ui.Button;
 import com.vaadin.ui.Grid.SelectionMode;
 import com.vaadin.ui.Label;
 import com.vaadin.ui.TabSheet;
+import com.vaadin.ui.TextField;
 import com.vaadin.ui.renderers.NumberRenderer;
 
 @Widgetset("com.vaadin.DefaultWidgetSet")
@@ -48,7 +50,12 @@ public class GridInTabSheet extends AbstractTestUIWithLog {
         final Grid<Integer> grid = new Grid<>();
         grid.setSelectionMode(SelectionMode.MULTI);
         grid.addColumn(ValueProvider.identity(), new NumberRenderer())
-                .setId("count");
+                .setId("count").setEditorBinding(
+                        grid.getEditor().getBinder().forField(new TextField())
+                                .withConverter(new StringToIntegerConverter(""))
+                                .bind(i -> i, (i, v) -> {
+                                }));
+        grid.getEditor().setEnabled(true);
 
         LinkedList<Integer> items = IntStream.range(0, 3).boxed()
                 .collect(Collectors.toCollection(LinkedList::new));
diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridInTabSheetTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridInTabSheetTest.java
index 785bda8261..5b277f729f 100644
--- a/uitest/src/test/java/com/vaadin/tests/components/grid/GridInTabSheetTest.java
+++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridInTabSheetTest.java
@@ -2,11 +2,12 @@ package com.vaadin.tests.components.grid;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 
 import org.junit.Test;
+import org.openqa.selenium.Keys;
 
+import com.vaadin.testbench.By;
 import com.vaadin.testbench.elements.ButtonElement;
 import com.vaadin.testbench.elements.GridElement;
 import com.vaadin.testbench.elements.NotificationElement;
@@ -85,6 +86,39 @@ public class GridInTabSheetTest extends MultiBrowserTest {
         assertNoNotification();
     }
 
+    @Test
+    public void testEditorOpenWhenSwitchingTab() {
+        setDebug(true);
+        openTestURL();
+
+        GridElement grid = $(GridElement.class).first();
+
+        grid.getCell(0, 1).doubleClick();
+        assertEquals("Editor should be open", "0",
+                grid.getEditor().getField(1).getAttribute("value"));
+
+        TabSheetElement tabsheet = $(TabSheetElement.class).first();
+        tabsheet.openTab("Label");
+        tabsheet.openTab("Grid");
+
+        grid = $(GridElement.class).first();
+        assertFalse("Editor should be closed.",
+                grid.isElementPresent(By.vaadin("#editor")));
+
+        grid.getCell(1, 1).doubleClick();
+        assertEquals("Editor should open after tab switch", "1",
+                grid.getEditor().getField(1).getAttribute("value"));
+
+        // Close the current editor and reopen on a different row
+        grid.sendKeys(Keys.ESCAPE);
+
+        grid.getCell(0, 1).doubleClick();
+        assertEquals("Editor should move", "0",
+                grid.getEditor().getField(1).getAttribute("value"));
+
+        assertNoErrorNotifications();
+    }
+
     private void removeGridRow() {
         $(ButtonElement.class).caption("Remove row from Grid").first().click();
     }