fix: treat "clear all" as permanent initial state (#10128) (#10135)

Fixes #10119

Co-authored-by: Denis <denis@vaadin.com>

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<NodeChange> 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<NodeChange> collector) {
                 ((ListAddChange<T>) 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<NodeChange> collector) {
 
         List<AbstractListChange<T>> 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<StateNode> 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

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<NodeChange> 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<NodeChange> 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

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`
+        <div id="div">Template text</div> 
+    `;
+  }
+}
+
+customElements.define('test-form', TestForm);

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);
+    }
+}

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");
+    }
+}

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());
+    }
+
+}