From 0ef54ee29a3bf473efb5d1b29fc386de3341aaae Mon Sep 17 00:00:00 2001 From: Henri Sara Date: Wed, 23 Aug 2017 14:06:00 +0300 Subject: [PATCH] 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 --- .../AbstractOrderedLayoutConnector.java | 34 ++++++------- .../VerticalLayoutRemoveComponent.java | 38 +++++++++++++++ .../VerticalLayoutRemoveComponentTest.java | 48 +++++++++++++++++++ 3 files changed, 104 insertions(+), 16 deletions(-) create mode 100644 uitest/src/main/java/com/vaadin/tests/layouts/VerticalLayoutRemoveComponent.java create mode 100644 uitest/src/test/java/com/vaadin/tests/layouts/VerticalLayoutRemoveComponentTest.java 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 0e3bf73342..7ec6e5fd10 100644 --- 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 index 0000000000..24431a5aa3 --- /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 index 0000000000..5ce565230d --- /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()); + } +} -- 2.39.5