From 7ebbdb76bcdab58514c07b3ed0a6e5a11231627d Mon Sep 17 00:00:00 2001
From: Gilberto Torrezan <gilberto-torrezan@users.noreply.github.com>
Date: Thu, 2 Aug 2018 14:47:08 +0300
Subject: [PATCH] Make sure all children are collected when the parent node is
 inactive (#4473)

Fixes #4448
---
 .../com/vaadin/flow/internal/StateTree.java   | 18 +++++--
 .../vaadin/flow/internal/StateTreeTest.java   | 49 +++++++++++++++++++
 2 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/StateTree.java b/flow-server/src/main/java/com/vaadin/flow/internal/StateTree.java
index 0e765191e36..56c0b84c959 100644
--- a/flow-server/src/main/java/com/vaadin/flow/internal/StateTree.java
+++ b/flow-server/src/main/java/com/vaadin/flow/internal/StateTree.java
@@ -79,7 +79,8 @@ public boolean isAttached() {
      * @see StateTree#beforeClientResponse(StateNode, SerializableConsumer)
      * @see StateTree#runExecutionsBeforeClientResponse()
      */
-    public static final class BeforeClientResponseEntry implements Serializable {
+    public static final class BeforeClientResponseEntry
+            implements Serializable {
         private static final Comparator<BeforeClientResponseEntry> COMPARING_INDEX = Comparator
                 .comparingInt(BeforeClientResponseEntry::getIndex);
 
@@ -235,13 +236,20 @@ public StateNode getNodeById(int id) {
      *            a consumer accepting node changes
      */
     public void collectChanges(Consumer<NodeChange> collector) {
-        Set<StateNode> dirtyNodesSet = collectDirtyNodes();
-
-        dirtyNodesSet.forEach(StateNode::updateActiveState);
+        Set<StateNode> allDirtyNodes = new LinkedHashSet<>();
+        boolean evaluateNewDirtyNodes = true;
+
+        // The updateActiveState method can create new dirty nodes, so they need
+        // to be collected as well
+        while (evaluateNewDirtyNodes) {
+            Set<StateNode> dirtyNodesSet = collectDirtyNodes();
+            dirtyNodesSet.forEach(StateNode::updateActiveState);
+            evaluateNewDirtyNodes = allDirtyNodes.addAll(dirtyNodesSet);
+        }
 
         // TODO fire preCollect events
 
-        dirtyNodesSet.forEach(node -> node.collectChanges(collector));
+        allDirtyNodes.forEach(node -> node.collectChanges(collector));
     }
 
     @Override
diff --git a/flow-server/src/test/java/com/vaadin/flow/internal/StateTreeTest.java b/flow-server/src/test/java/com/vaadin/flow/internal/StateTreeTest.java
index 914b0804a67..57775c9e5ad 100644
--- a/flow-server/src/test/java/com/vaadin/flow/internal/StateTreeTest.java
+++ b/flow-server/src/test/java/com/vaadin/flow/internal/StateTreeTest.java
@@ -24,6 +24,7 @@
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.Consumer;
 
 import org.apache.commons.lang3.SerializationUtils;
 import org.junit.Assert;
@@ -47,6 +48,8 @@
 import com.vaadin.flow.internal.nodefeature.NodeFeature;
 import com.vaadin.tests.util.TestUtil;
 
+import elemental.json.JsonObject;
+
 public class StateTreeTest {
     private StateTree tree = new StateTree(new UI().getInternals(),
             ElementChildrenList.class);
@@ -72,6 +75,23 @@ public boolean isAttached() {
         }
     }
 
+    public static class CollectableNode extends StateNode {
+
+        public CollectableNode() {
+            super(ElementData.class, ElementChildrenList.class);
+        }
+
+        @Override
+        public void collectChanges(Consumer<NodeChange> collector) {
+            collector.accept(new NodeChange(this) {
+                @Override
+                protected void populateJson(JsonObject json,
+                        ConstantPool constantPool) {
+                }
+            });
+        }
+    }
+
     @Test
     public void testRootNodeState() {
         StateNode rootNode = tree.getRootNode();
@@ -504,4 +524,33 @@ public void collectChanges_updateActiveState() {
         Mockito.verify(node2).updateActiveState();
     }
 
+    @Test
+    public void collectChanges_parentIsInactive_childrenAreCollected() {
+        StateNode node1 = new CollectableNode();
+        StateNode node2 = new CollectableNode();
+        StateNode node3 = new CollectableNode();
+
+        node1.setTree(tree);
+        node2.setTree(tree);
+        node3.setTree(tree);
+
+        node1.getFeature(ElementChildrenList.class).add(0, node2);
+        node2.getFeature(ElementChildrenList.class).add(0, node3);
+
+        // cleanup the current dirty nodes
+        tree.collectChanges(node -> {
+        });
+
+        node1.getFeature(ElementData.class).setVisible(false);
+
+        List<StateNode> collectedNodes = new ArrayList<>(3);
+
+        tree.collectChanges(change -> collectedNodes.add(change.getNode()));
+
+        Assert.assertEquals(3, collectedNodes.size());
+        Assert.assertTrue(collectedNodes.contains(node1));
+        Assert.assertTrue(collectedNodes.contains(node2));
+        Assert.assertTrue(collectedNodes.contains(node3));
+    }
+
 }