diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeList.java b/flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeList.java index 86ee06b0610..5c41db35632 100644 --- a/flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeList.java +++ b/flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeList.java @@ -28,7 +28,6 @@ import java.util.Set; import java.util.function.Consumer; import java.util.stream.Collectors; -import java.util.stream.Stream; import com.vaadin.flow.internal.StateNode; import com.vaadin.flow.internal.change.AbstractListChange; @@ -288,8 +287,6 @@ private void setAccessed() { @Override public void collectChanges(Consumer collector) { - boolean hasRemoveAll = false; - // This map contains items wrapped by AbstractListChanges as keys and // index in the following allChanges list as a value (it allows to get // AbstractListChange by the index) @@ -316,7 +313,6 @@ public void collectChanges(Consumer collector) { ((ListAddChange) change).getNewItems() .forEach(item -> indices.put(item, i)); } else if (change instanceof ListClearChange) { - hasRemoveAll = true; allChanges.clear(); indices.clear(); index = 0; @@ -329,17 +325,8 @@ public void collectChanges(Consumer collector) { List> changes; - if (isRemoveAllCalled && !hasRemoveAll) { - changes = Stream - .concat(Stream.of(new ListClearChange<>(this)), - allChanges.stream()) - .filter(this::acceptChange).collect(Collectors.toList()); - } else { - changes = allChanges.stream().filter(this::acceptChange) - .collect(Collectors.toList()); - } - - isRemoveAllCalled = false; + changes = allChanges.stream().filter(this::acceptChange) + .collect(Collectors.toList()); if (isPopulated) { changes.forEach(collector); @@ -369,16 +356,15 @@ public void forEachChild(Consumer action) { @Override public void generateChangesFromEmpty() { + if (isRemoveAllCalled) { + // if list ever had "clear" change then it + // should be stored in the tracker + addChange(new ListClearChange<>(this)); + } if (values != null) { assert !values.isEmpty(); getChangeTracker().add(new ListAddChange<>(this, isNodeValues(), 0, new ArrayList<>(values))); - } else if (isRemoveAllCalled) { - // if list has "clear" change and has no any other changes then - // this change should be stored in the tracker otherwise it will be - // incorrectly postponed - isRemoveAllCalled = false; - addChange(new ListClearChange<>(this)); } else if (!isPopulated) { // make change tracker available so that an empty change can be // reported diff --git a/flow-server/src/test/java/com/vaadin/flow/internal/nodefeature/NodeListAddRemoveTest.java b/flow-server/src/test/java/com/vaadin/flow/internal/nodefeature/NodeListAddRemoveTest.java index 862cd0f0246..19a810f1c13 100644 --- a/flow-server/src/test/java/com/vaadin/flow/internal/nodefeature/NodeListAddRemoveTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/internal/nodefeature/NodeListAddRemoveTest.java @@ -317,7 +317,56 @@ public void clear_collectChanges_resetChangeTracker_clearEventIsCollected() { changes.clear(); nodeList.getNode().collectChanges(changes::add); - // Now there is not anymore clear change (so the previous one is not + // Now there is no anymore clear change (so the previous one is not + // preserved) + Assert.assertEquals(1, changes.size()); + Assert.assertTrue(changes.get(0) instanceof ListAddChange); + } + + @Test + public void clear_collectChanges_resetChangeTracker_reattach_clearEventIsCollected() { + resetToRemoveAfterAddCase(); + + nodeList.add("foo"); + + nodeList.clear(); + + StateTree tree = new StateTree(new UI().getInternals(), + ElementChildrenList.class); + // attach the feature node to the tree + tree.getRootNode().getFeature(ElementChildrenList.class) + .add(nodeList.getNode()); + + nodeList.add("bar"); + + // detach the feature node to the tree + + tree.getRootNode().getFeature(ElementChildrenList.class).remove(0); + // if there was no changes collection after detach (and node is attached + // again) then the node has not been detached de-facto: detach-attach is + // no-op in this case, so to avoid no-op the changes should be collected + // in between + nodeList.getNode().collectChanges(change -> { + }); + + // reattach it back + tree.getRootNode().getFeature(ElementChildrenList.class) + .add(nodeList.getNode()); + + List changes = new ArrayList<>(); + nodeList.getNode().collectChanges(changes::add); + + Assert.assertEquals(3, changes.size()); + Assert.assertEquals(NodeAttachChange.class, changes.get(0).getClass()); + Assert.assertEquals(ListClearChange.class, changes.get(1).getClass()); + Assert.assertEquals(ListAddChange.class, changes.get(2).getClass()); + + changes.clear(); + + nodeList.add("baz"); + + nodeList.getNode().collectChanges(changes::add); + // Now there is no anymore clear change (so the previous one is not // preserved) Assert.assertEquals(1, changes.size()); Assert.assertTrue(changes.get(0) instanceof ListAddChange); @@ -342,6 +391,42 @@ public void clearNodeList_clearChanges_generateChangesFromEmpty_clearChangeIsCol Assert.assertEquals(ListClearChange.class, changes.get(1).getClass()); } + @Test + public void clearNodeList_clearChanges_reatach_generateChangesFromEmpty_clearChangeIsCollected() { + // removes all children + nodeList.clear(); + + StateTree tree = new StateTree(new UI().getInternals(), + ElementChildrenList.class); + // attach the feature node to the tree + tree.getRootNode().getFeature(ElementChildrenList.class) + .add(nodeList.getNode()); + + nodeList.getNode().collectChanges(change -> { + }); + + // detach the feature node to the tree + + tree.getRootNode().getFeature(ElementChildrenList.class).remove(0); + // if there was no changes collection after detach (and node is attached + // again) then the node has not been detached de-facto: detach-attach is + // no-op in this case, so to avoid no-op the changes should be collected + // in between + nodeList.getNode().collectChanges(change -> { + }); + + // reattach it back + tree.getRootNode().getFeature(ElementChildrenList.class) + .add(nodeList.getNode()); + + List changes = new ArrayList<>(); + nodeList.getNode().collectChanges(changes::add); + + Assert.assertEquals(2, changes.size()); + Assert.assertEquals(NodeAttachChange.class, changes.get(0).getClass()); + Assert.assertEquals(ListClearChange.class, changes.get(1).getClass()); + } + @Test public void collectChanges_clearNodeListIsDoneFirst_noClearEventinCollectedChanges() { // removes all children diff --git a/flow-tests/test-root-context/frontend/lit/test-form.js b/flow-tests/test-root-context/frontend/lit/test-form.js new file mode 100644 index 00000000000..43ab9a4e736 --- /dev/null +++ b/flow-tests/test-root-context/frontend/lit/test-form.js @@ -0,0 +1,12 @@ +import {html, LitElement} from 'lit-element'; + +class TestForm extends LitElement { + + render() { + return html` +
Template text
+ `; + } +} + +customElements.define('test-form', TestForm); diff --git a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/littemplate/ReattachView.java b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/littemplate/ReattachView.java new file mode 100644 index 00000000000..8718cfe22ea --- /dev/null +++ b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/littemplate/ReattachView.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.littemplate; + +import com.vaadin.flow.component.html.Div; +import com.vaadin.flow.component.html.NativeButton; +import com.vaadin.flow.router.Route; + +@Route(value = "com.vaadin.flow.uitest.ui.littemplate.ReattachView") +public class ReattachView extends Div { + + public ReattachView() { + TestForm testForm = new TestForm(); + testForm.setId("form-template"); + + NativeButton button = new NativeButton("Click"); + button.addClickListener(ce -> { + if (testForm.isAttached()) { + remove(testForm); + } else { + add(testForm); + } + }); + button.setId("click"); + add(button); + } +} diff --git a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/littemplate/TestForm.java b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/littemplate/TestForm.java new file mode 100644 index 00000000000..8895b2e68ee --- /dev/null +++ b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/littemplate/TestForm.java @@ -0,0 +1,34 @@ +/* + * 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.littemplate; + +import com.vaadin.flow.component.Tag; +import com.vaadin.flow.component.dependency.JsModule; +import com.vaadin.flow.component.html.Div; +import com.vaadin.flow.component.littemplate.LitTemplate; +import com.vaadin.flow.component.template.Id; + +@JsModule("lit/test-form.js") +@Tag("test-form") +public class TestForm extends LitTemplate { + + @Id + private Div div; + + public TestForm() { + div.setText("foo"); + } +} diff --git a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/littemplate/ReattachIT.java b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/littemplate/ReattachIT.java new file mode 100644 index 00000000000..d309c843cfe --- /dev/null +++ b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/littemplate/ReattachIT.java @@ -0,0 +1,54 @@ +/* + * 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.littemplate; + +import org.junit.Assert; +import org.junit.Test; +import org.openqa.selenium.By; +import org.openqa.selenium.WebElement; + +import com.vaadin.flow.testutil.ChromeBrowserTest; +import com.vaadin.testbench.TestBenchElement; + +public class ReattachIT extends ChromeBrowserTest { + + @Test + public void reattachedTemplateHasExplicitlySetText() { + open(); + + WebElement button = findElement(By.id("click")); + + // attach template + button.click(); + + TestBenchElement template = $(TestBenchElement.class) + .id("form-template"); + TestBenchElement div = template.$(TestBenchElement.class).id("div"); + + Assert.assertEquals("foo", div.getText()); + + // detach template + button.click(); + + // re-attach template + button.click(); + + template = $(TestBenchElement.class).id("form-template"); + div = template.$(TestBenchElement.class).id("div"); + Assert.assertEquals("foo", div.getText()); + } + +}