Make inactive nodes mark their children as dirty on setInactive

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

flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/LongPollingPushView.java

@@ -0,0 +1,45 @@
+/*
+ * Copyright 2000-2018 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.component.html.Span;
+import com.vaadin.flow.component.page.Push;
+import com.vaadin.flow.router.Route;
+import com.vaadin.flow.shared.ui.Transport;
+
+/**
+ * Class for reproducing the bug https://github.com/vaadin/flow/issues/4353
+ */
+@Route("com.vaadin.flow.uitest.ui.LongPollingPushView")
+@Push(transport = Transport.LONG_POLLING)
+public class LongPollingPushView extends AbstractDivView {
+
+    public LongPollingPushView() {
+        Div parent = new Div();
+        Span child = new Span("Some text");
+        child.setId("child");
+        parent.add(child);
+        add(parent);
+        parent.setVisible(false);
+
+        NativeButton visibility = new NativeButton("Toggle visibility",
+                e -> parent.setVisible(!parent.isVisible()));
+        visibility.setId("visibility");
+        add(visibility);
+    }
+}

flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/LongPollingPushIT.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright 2000-2018 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 LongPollingPushIT extends ChromeBrowserTest {
+
+    @Test
+    public void openPage_thereAreNoErrorsInTheConsole() {
+        open();
+        checkLogsForErrors();
+
+        waitForElementNotPresent(By.id("child"));
+
+        WebElement button = findElement(By.id("visibility"));
+        button.click();
+
+        waitForElementPresent(By.id("child"));
+
+        WebElement span = findElement(By.id("child"));
+        Assert.assertEquals("Some text", span.getText());
+        checkLogsForErrors();
+    }
+
+}