From ed16b622c9bb87bd79ad20573ff70e3fccc209a7 Mon Sep 17 00:00:00 2001 From: Vaadin Bot Date: Wed, 20 Oct 2021 18:57:47 +0300 Subject: [PATCH] fix: reset Dom node on changing StateNode parent (#12043) (#12102) Allows to move children to the grandparent and remove parent at the same time fixes #11983 Co-authored-by: Denis --- .../com/vaadin/client/flow/StateNode.java | 4 ++ .../binding/SimpleElementBindingStrategy.java | 7 +++ .../flow/GwtBasicElementBinderTest.java | 41 +++++++++++++-- .../vaadin/client/flow/GwtStateNodeTest.java | 18 +++++++ .../ui/RemoveElementAfterItsChildrenView.java | 51 +++++++++++++++++++ .../ui/RemoveElementAfterItsChildrenIT.java | 40 +++++++++++++++ 6 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/RemoveElementAfterItsChildrenView.java create mode 100644 flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RemoveElementAfterItsChildrenIT.java diff --git a/flow-client/src/main/java/com/vaadin/client/flow/StateNode.java b/flow-client/src/main/java/com/vaadin/client/flow/StateNode.java index b31bb23c102..46e9f83f4da 100644 --- a/flow-client/src/main/java/com/vaadin/client/flow/StateNode.java +++ b/flow-client/src/main/java/com/vaadin/client/flow/StateNode.java @@ -284,6 +284,10 @@ public StateNode getParent() { * the parent state node */ public void setParent(StateNode parent) { + if (this.parent != null && this.parent.isUnregistered() + && parent != null && this.parent.getId() != parent.getId()) { + setDomNode(null); + } this.parent = parent; } diff --git a/flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java b/flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java index 5be7797b642..f69b75bd45d 100644 --- a/flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java +++ b/flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java @@ -202,6 +202,13 @@ assert hasSameTag(stateNode, htmlNode) : "Element tag name is '" if (boundNodes.has(stateNode)) { return; } + + stateNode.addDomNodeSetListener(node -> { + if (node.getDomNode() == null) { + boundNodes.delete(node); + } + return true; + }); boundNodes.set(stateNode, true); BindingContext context = new BindingContext(stateNode, htmlNode, diff --git a/flow-client/src/test-gwt/java/com/vaadin/client/flow/GwtBasicElementBinderTest.java b/flow-client/src/test-gwt/java/com/vaadin/client/flow/GwtBasicElementBinderTest.java index 289adb7d499..18b6d73de71 100644 --- a/flow-client/src/test-gwt/java/com/vaadin/client/flow/GwtBasicElementBinderTest.java +++ b/flow-client/src/test-gwt/java/com/vaadin/client/flow/GwtBasicElementBinderTest.java @@ -80,11 +80,11 @@ public void testBindExistingProperty() { } public void testBindExistingPropertyWithDifferentType() { - //set number as a property value for DOM elememnt - double value= setNumberValue(element, "bar"); - + // set number as a property value for DOM elememnt + double value = setNumberValue(element, "bar"); + // set string as a StateTree property value - properties.getProperty("bar").setValue(""+value); + properties.getProperty("bar").setValue("" + value); Binder.bind(node, element); @@ -1821,6 +1821,37 @@ public void testReadyCallback_deferredPolymerElement_readyIsCalledAndNotified() assertEquals("foobar", WidgetUtil.getJsProperty(element, "baz")); } + public void testBoundNode_canBeBoundAgainOnceDomNodeIsReset() { + // set some attribute + idAttribute.setValue("foo"); + + Binder.bind(node, element); + + Reactive.flush(); + + assertEquals("foo", element.getId()); + + Element newElement = Browser.getDocument().createElement("div"); + Binder.bind(node, newElement); + + Reactive.flush(); + + // nothing happens since the node is already bound + assertEquals("", newElement.getId()); + + // reset node + node.setDomNode(null); + + newElement = Browser.getDocument().createElement("div"); + + Binder.bind(node, newElement); + + Reactive.flush(); + + // once the dom node has been reset the element can bound again + assertEquals("foo", newElement.getId()); + } + private void assertDeferredPolymerElement_originalReadyIsCalled( Element element) { initPolymer(element); @@ -1979,7 +2010,7 @@ private native String getPropertyType(Object obj, String name) /*-{ return typeof obj[name]; }-*/; - + private native double setNumberValue(Object obj, String name) /*-{ obj[name] =2; diff --git a/flow-client/src/test-gwt/java/com/vaadin/client/flow/GwtStateNodeTest.java b/flow-client/src/test-gwt/java/com/vaadin/client/flow/GwtStateNodeTest.java index ec699524ebd..8341e9dec28 100644 --- a/flow-client/src/test-gwt/java/com/vaadin/client/flow/GwtStateNodeTest.java +++ b/flow-client/src/test-gwt/java/com/vaadin/client/flow/GwtStateNodeTest.java @@ -18,6 +18,8 @@ import com.vaadin.client.ClientEngineTestBase; import com.vaadin.client.Registry; +import elemental.client.Browser; + public class GwtStateNodeTest extends ClientEngineTestBase { private StateTree tree; @@ -39,4 +41,20 @@ public void testNodeData_getNodeData_sameInstance() { assertSame(data, node.getNodeData(TestData.class)); } + public void testSetNewParent_domNodeIsNull() { + StateNode parent = new StateNode(2, tree); + tree.registerNode(parent); + + StateNode node = new StateNode(3, tree); + + node.setDomNode(Browser.getDocument().createElement("div")); + node.setParent(parent); + + assertNotNull(node.getDomNode()); + + tree.unregisterNode(parent); + node.setParent(new StateNode(4, tree)); + assertNull(node.getDomNode()); + } + } diff --git a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/RemoveElementAfterItsChildrenView.java b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/RemoveElementAfterItsChildrenView.java new file mode 100644 index 00000000000..88e4c17adaf --- /dev/null +++ b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/RemoveElementAfterItsChildrenView.java @@ -0,0 +1,51 @@ +/* + * Copyright 2000-2021 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.flow.uitest.ui; + +import com.vaadin.flow.component.html.Div; +import com.vaadin.flow.component.html.NativeButton; +import com.vaadin.flow.router.Route; + +@Route("com.vaadin.flow.uitest.ui.RemoveElementAfterItsChildrenView") +public class RemoveElementAfterItsChildrenView extends Div { + + public RemoveElementAfterItsChildrenView() { + Div container = new Div(); + container.setId("container"); + Div wrapper = new Div(); + wrapper.setId("wrapper"); + Div first = new Div(); + first.setText("First child"); + first.setId("first"); + Div second = new Div(); + second.setText("Second child"); + second.setId("second"); + container.add(wrapper); + wrapper.add(first, second); + + NativeButton button = new NativeButton(); + button.setText("Move children and remove parent"); + button.setId("move-children"); + add(container, button); + + // Then later on click the button, executing the following code + button.addClickListener(event -> { + container.add(first, second); + wrapper.getElement().removeFromParent(); + }); + } + +} diff --git a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RemoveElementAfterItsChildrenIT.java b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RemoveElementAfterItsChildrenIT.java new file mode 100644 index 00000000000..54840cfb3e7 --- /dev/null +++ b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RemoveElementAfterItsChildrenIT.java @@ -0,0 +1,40 @@ +/* + * Copyright 2000-2021 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.flow.uitest.ui; + +import org.junit.Assert; +import org.junit.Test; +import org.openqa.selenium.By; +import org.openqa.selenium.WebElement; + +import com.vaadin.flow.testutil.ChromeBrowserTest; + +public class RemoveElementAfterItsChildrenIT extends ChromeBrowserTest { + + @Test + public void moveChildrenToParentContainerAndRemoveParent_childrenAreMoved() { + open(); + + findElement(By.id("move-children")).click(); + + WebElement container = findElement(By.id("container")); + + Assert.assertEquals(1, container.findElements(By.id("first")).size()); + Assert.assertEquals(1, container.findElements(By.id("second")).size()); + + Assert.assertFalse(isElementPresent(By.id("wrapper"))); + } +}