diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java b/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java index d94f23ed6d7..9ab9ac6f3d5 100644 --- a/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java +++ b/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java @@ -378,25 +378,6 @@ public void collectChanges(Consumer 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 initialFeatures = Stream @@ -712,7 +693,33 @@ private Stream 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(); + } + }); + } } /** diff --git a/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java b/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java index 8700e18d661..c2137e866a6 100644 --- a/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java @@ -659,8 +659,13 @@ public void collectChanges_initiallyInactiveViaParentElement_sendOnlyDisalowAndR private void assertCollectChanges_initiallyInactive(StateNode stateNode, ElementPropertyMap properties, Consumer 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 @@ -668,22 +673,18 @@ private void assertCollectChanges_initiallyInactive(StateNode stateNode, // node is visible or not boolean visibilityChanged = !visibility.isVisible(); - properties.setProperty("foo", "bar"); - - TestStateTree tree = (TestStateTree) stateNode.getOwner(); - - tree.dirtyNodes.clear(); - List 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,7 +808,8 @@ 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); @@ -816,8 +817,9 @@ private void assertCollectChanges_initiallyVisible(StateNode 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))); } 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; } diff --git a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/LongPollingPushView.java b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/LongPollingPushView.java new file mode 100644 index 00000000000..3411d9c9494 --- /dev/null +++ b/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); + } +} diff --git a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/LongPollingPushIT.java b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/LongPollingPushIT.java new file mode 100644 index 00000000000..9f5d9693c61 --- /dev/null +++ b/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(); + } + +}