Make inactive nodes mark their children as dirty on setInactive

This moves the setting of dirty nodes out of the collectChanges method.

When using long-polling push, the client keeps asking for changes from
the server. The server then collects all the changes and sends to the
client. The problem is: a child node is always considered `dirty`, if
its parent is invisible, and the node is always included in the changes
sent to the client.

Since there's not a single moment where there's nothing to send, the
server keeps sending the changes, and the client keeps asking for the
changes. This creates a busy loop between client and server, which only
ends when the parent node is made visible again.

To prevent this, this patch changes this logic to make the children
nodes only dirty when the parent visibility changes.

flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java

@@ -378,25 +378,6 @@ public void collectChanges(Consumer<NodeChange> collector) {
             return;
         }
         if (isInactive()) {
-            if (!isInactiveSelf) {
-                /*
-                 * We are here if: the node itself is not inactive but it has
-                 * some ascendant which is inactive.
-                 *
-                 * In this case we send only some subset of changes (not from
-                 * all the features). But we should send changes for all
-                 * remaining features. Normally it automatically happens if the
-                 * node becomes "visible". But if it was visible with some
-                 * invisible parent then only the parent becomes dirty (when
-                 * it's set visible) and this child will never participate in
-                 * collection of changes since it's not marked as dirty.
-                 *
-                 * So here such node (which is active itself but its ascendant
-                 * is inactive) we mark as dirty again to be able to collect its
-                 * changes later on when its ascendant becomes active.
-                 */
-                getOwner().markAsDirty(this);
-            }
             if (isInitialChanges) {
                 // send only required (reported) features updates
                 Stream<NodeFeature> initialFeatures = Stream
@@ -712,7 +693,33 @@ private Stream<NodeFeature> getDisalowFeatures() {
     }
 
     private void setInactive(boolean inactive) {
-        isInactiveSelf = inactive;
+        if (isInactiveSelf != inactive) {
+            isInactiveSelf = inactive;
+
+            visitNodeTree(child -> {
+                if (!this.equals(child) && !child.isInactiveSelf) {
+                    /*
+                     * We are here if: the child node itself is not inactive but
+                     * it has some ascendant which is inactive.
+                     *
+                     * In this case we send only some subset of changes (not
+                     * from all the features). But we should send changes for
+                     * all remaining features. Normally it automatically happens
+                     * if the node becomes "visible". But if it was visible with
+                     * some invisible parent then only the parent becomes dirty
+                     * (when it's set visible) and this child will never
+                     * participate in collection of changes since it's not
+                     * marked as dirty.
+                     *
+                     * So here such node (which is active itself but its
+                     * ascendant is inactive) we mark as dirty again to be able
+                     * to collect its changes later on when its ascendant
+                     * becomes active.
+                     */
+                    child.markAsDirty();
+                }
+            });
+        }
     }
 
     /**

flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java

@@ -659,31 +659,32 @@ public void collectChanges_initiallyInactiveViaParentElement_sendOnlyDisalowAndR
 
     private void assertCollectChanges_initiallyInactive(StateNode stateNode,
             ElementPropertyMap properties, Consumer<Boolean> activityUpdater) {
-        ElementData visibility = stateNode.getFeature(ElementData.class);
 
+        properties.setProperty("foo", "bar");
+
+        TestStateTree tree = (TestStateTree) stateNode.getOwner();
+        tree.dirtyNodes.clear();
+
+        ElementData visibility = stateNode.getFeature(ElementData.class);
         activityUpdater.accept(false);
 
         // activity updater may modify visibility of the node itself or its
         // ancestor. The number of changes will depend on whether the subject
         // node is visible or not
         boolean visibilityChanged = !visibility.isVisible();
 
-        properties.setProperty("foo", "bar");
-
-        TestStateTree tree = (TestStateTree) stateNode.getOwner();
-
-        tree.dirtyNodes.clear();
-
         List<NodeChange> changes = new ArrayList<>();
         stateNode.collectChanges(changes::add);
 
         if (visibilityChanged) {
-            Assert.assertEquals(0, tree.dirtyNodes.size());
+            Assert.assertEquals(1, tree.dirtyNodes.size());
+            Assert.assertThat(tree.dirtyNodes, CoreMatchers.hasItem(stateNode));
         } else {
             // the target node should be marked as dirty because it's visible
             // but its parent is inactive
-            Assert.assertEquals(1, tree.dirtyNodes.size());
-            tree.dirtyNodes.contains(stateNode);
+            Assert.assertEquals(2, tree.dirtyNodes.size());
+            stateNode.visitNodeTree(node -> Assert.assertThat(tree.dirtyNodes,
+                    CoreMatchers.hasItem(node)));
         }
 
         Assert.assertEquals(visibilityChanged ? 3 : 2, changes.size());
@@ -699,7 +700,6 @@ private void assertCollectChanges_initiallyInactive(StateNode stateNode,
         Assert.assertTrue("No tag change found", tagFound.isPresent());
         MapPutChange tagChange = tagFound.get();
 
-
         MapPutChange change = (MapPutChange) changes.get(1);
         if (visibilityChanged) {
             Assert.assertThat(changes.get(2),
@@ -784,6 +784,9 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode,
 
         changes.clear();
 
+        TestStateTree tree = (TestStateTree) stateNode.getOwner();
+        tree.dirtyNodes.clear();
+
         // now make the node inactive via the VisibiltyData
 
         activityUpdater.accept(false);
@@ -793,9 +796,6 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode,
 
         properties.setProperty("foo", "baz");
 
-        TestStateTree tree = (TestStateTree) stateNode.getOwner();
-        tree.dirtyNodes.clear();
-
         stateNode.collectChanges(changes::add);
 
         // activity updater may modify visibility of the node itself or its
@@ -808,16 +808,18 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode,
 
         MapPutChange change;
         if (visibilityChanged) {
-            Assert.assertEquals(0, tree.dirtyNodes.size());
+            Assert.assertEquals(1, tree.dirtyNodes.size());
+            Assert.assertThat(tree.dirtyNodes, CoreMatchers.hasItem(stateNode));
             Assert.assertThat(changes.get(0),
                     CoreMatchers.instanceOf(MapPutChange.class));
             change = (MapPutChange) changes.get(0);
             Assert.assertEquals(ElementData.class, change.getFeature());
         } else {
             // the target node should be marked as dirty because it's visible
             // but its parent is inactive
-            Assert.assertEquals(1, tree.dirtyNodes.size());
-            tree.dirtyNodes.contains(stateNode);
+            Assert.assertEquals(2, tree.dirtyNodes.size());
+            stateNode.visitNodeTree(node -> Assert.assertThat(tree.dirtyNodes,
+                    CoreMatchers.hasItem(node)));
         }
 
         changes.clear();
@@ -841,7 +843,8 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode,
                     .equals(change.getFeature()) ? change
                             : (MapPutChange) changes.get(1);
             propertyChange = change.equals(visibilityChange)
-                    ? (MapPutChange) changes.get(1) : change;
+                    ? (MapPutChange) changes.get(1)
+                    : change;
         } else {
             propertyChange = change;
         }