Avoid detaching VL/HL children unnecessarily (#9862)

When child components are removed from a
VerticalLayout/HorizontalLayout, do not remove other children from the
DOM if no other hierarchy changes are made.

Fixes #7713

diff --git a/client/src/main/java/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java b/client/src/main/java/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java
index 0e3bf733422ffeec7e37e7e097970b0d2a190e75..7ec6e5fd10be644a169c582eee7cc61f117b04cb 100644 (file)
--- a/client/src/main/java/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java
+++ b/client/src/main/java/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java
@@ -320,6 +320,24 @@ public abstract class AbstractOrderedLayoutConnector
         Profiler.leave(
                 "AOLC.onConnectorHierarchyChange temporarily remove spacing");
 
+        // first remove extra components to avoid extra detaches and attaches
+        for (ComponentConnector child : previousChildren) {
+            Profiler.enter("AOLC.onConnectorHierarchyChange remove children");
+            if (child.getParent() != this) {
+                Slot slot = layout.getSlot(child.getWidget());
+                slot.setWidgetResizeListener(null);
+                if (slot.hasCaption()) {
+                    slot.setCaptionResizeListener(null);
+                }
+                slot.setSpacingResizeListener(null);
+                child.removeStateChangeHandler(childStateChangeHandler);
+                layout.removeWidget(child.getWidget());
+            }
+            Profiler.leave("AOLC.onConnectorHierarchyChange remove children");
+        }
+        Profiler.leave("AOLC.onConnectorHierarchyChange");
+
+        // reorder remaining components and add any new components
         for (ComponentConnector child : getChildComponents()) {
             Profiler.enter("AOLC.onConnectorHierarchyChange add children");
             Slot slot = layout.getSlot(child.getWidget());
@@ -345,22 +363,6 @@ public abstract class AbstractOrderedLayoutConnector
         }
         Profiler.leave("AOLC.onConnectorHierarchyChange setSpacing");
 
-        for (ComponentConnector child : previousChildren) {
-            Profiler.enter("AOLC.onConnectorHierarchyChange remove children");
-            if (child.getParent() != this) {
-                Slot slot = layout.getSlot(child.getWidget());
-                slot.setWidgetResizeListener(null);
-                if (slot.hasCaption()) {
-                    slot.setCaptionResizeListener(null);
-                }
-                slot.setSpacingResizeListener(null);
-                child.removeStateChangeHandler(childStateChangeHandler);
-                layout.removeWidget(child.getWidget());
-            }
-            Profiler.leave("AOLC.onConnectorHierarchyChange remove children");
-        }
-        Profiler.leave("AOLC.onConnectorHierarchyChange");
-
         updateInternalState();
     }
 

diff --git a/uitest/src/main/java/com/vaadin/tests/layouts/VerticalLayoutRemoveComponent.java b/uitest/src/main/java/com/vaadin/tests/layouts/VerticalLayoutRemoveComponent.java
new file mode 100644 (file)
index 0000000..24431a5
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/layouts/VerticalLayoutRemoveComponent.java
@@ -0,0 +1,38 @@
+package com.vaadin.tests.layouts;
+
+import com.vaadin.tests.components.TestBase;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.TextField;
+import com.vaadin.ui.VerticalLayout;
+
+@SuppressWarnings("serial")
+public class VerticalLayoutRemoveComponent extends TestBase {
+
+    @Override
+    protected void setup() {
+        final VerticalLayout layout = new VerticalLayout();
+        layout.setId("targetLayout");
+        // spacing makes it harder to track only the relevant events
+        layout.setSpacing(false);
+        final TextField tf = new TextField("Caption1");
+        Button b = new Button("Remove field ",
+                event -> layout.removeComponent(tf));
+        layout.addComponent(tf);
+        layout.addComponent(b);
+        layout.addComponent(new TextField("Caption2"));
+        layout.addComponent(new TextField("Caption3"));
+
+        addComponent(layout);
+    }
+
+    @Override
+    protected String getDescription() {
+        return "Clicking on the button should remove one text field but other textfields and their captions should stay intact.";
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 7113;
+    }
+
+}

diff --git a/uitest/src/test/java/com/vaadin/tests/layouts/VerticalLayoutRemoveComponentTest.java b/uitest/src/test/java/com/vaadin/tests/layouts/VerticalLayoutRemoveComponentTest.java
new file mode 100644 (file)
index 0000000..5ce5652
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/layouts/VerticalLayoutRemoveComponentTest.java
@@ -0,0 +1,48 @@
+/*
+ * 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.layouts;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.VerticalLayoutElement;
+import com.vaadin.tests.tb3.SingleBrowserTestPhantomJS2;
+
+public class VerticalLayoutRemoveComponentTest
+        extends SingleBrowserTestPhantomJS2 {
+    @Test
+    public void testRemoveOnlyNecessaryComponentsFromDom() {
+        openTestURL();
+
+        String script = "document.mutationEventCount = 0;"
+                + "var observer = new MutationObserver(function(mutations){"
+                + "mutations.forEach(function(mutation) { document.mutationEventCount += mutation.removedNodes.length; });"
+                + "});"
+                + "observer.observe(arguments[0], { childList: true });";
+
+        executeScript(script,
+                $(VerticalLayoutElement.class).id("targetLayout"));
+
+        $(ButtonElement.class).first().click();
+
+        Long mutationEvents = (Long) executeScript(
+                "return document.mutationEventCount;");
+        Assert.assertEquals(
+                "Parent should only have one mutation event (remove slot)", 1,
+                mutationEvents.intValue());
+    }
+}