Allow to set parent to null for root node (UI element). (#6112)

* Allow to set parent to null for root node (UI element). Fixes #6092 * Move the field into enclosing class to be able to initialize it before class construction
vaadin · Jul 26, 2019 · 233b24d · 233b24d
1 parent 179fd41
commit 233b24d
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 38 deletions.
diff --git a/flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java b/flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java
@@ -382,13 +382,7 @@ public void setSession(VaadinSession session) {
                             + ".");
         } else {
             if (session == null) {
-                try {
-                    ComponentUtil.onComponentDetach(ui);
-                    ui.getChildren().forEach(ComponentUtil::onComponentDetach);
-                } catch (Exception e) {
-                    getLogger().warn("Error while detaching UI from session",
-                            e);
-                }
+                ui.getElement().getNode().setParent(null);
                 // Disable push when the UI is detached. Otherwise the
                 // push connection and possibly VaadinSession will live on.
                 ui.getPushConfiguration().setPushMode(PushMode.DISABLED);
@@ -397,7 +391,9 @@ public void setSession(VaadinSession session) {
             this.session = session;
         }
 
-        if (session != null) {
+        if (session != null)
+
+        {
             ComponentUtil.onComponentAttach(ui, true);
         }
     }
@@ -853,8 +849,9 @@ public void addComponentDependencies(
         } else {
             // In npm mode, add external JavaScripts directly to the page.
             dependencies.getJavaScripts().stream()
-                    .filter(js -> UrlUtil.isExternal(js.value())).forEach(js -> page
-                            .addJavaScript(js.value(), js.loadMode()));
+                    .filter(js -> UrlUtil.isExternal(js.value()))
+                    .forEach(js -> page.addJavaScript(js.value(),
+                            js.loadMode()));
             dependencies.getJsModules().stream()
                     .filter(js -> UrlUtil.isExternal(js.value()))
                     .forEach(js -> page.addJsModule(js.value(), js.loadMode()));

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
@@ -365,8 +365,8 @@ protected void setTree(StateTree tree) {
     }
 
     /**
-     * Removes the node from its parent and unlinks the node (and children)
-     * from the state tree.
+     * Removes the node from its parent and unlinks the node (and children) from
+     * the state tree.
      */
     public void removeFromTree() {
         visitNodeTree(StateNode::reset);
@@ -676,10 +676,10 @@ private void doSetTree(StateTree tree) {
 
         if (owner instanceof StateTree) {
             throw new IllegalStateException(
-                    "Can't move a node from one state tree to another. " +
-                            "If this is intentional, first remove the " +
-                            "node from its current state tree by calling " +
-                            "removeFromTree");
+                    "Can't move a node from one state tree to another. "
+                            + "If this is intentional, first remove the "
+                            + "node from its current state tree by calling "
+                            + "removeFromTree");
         }
         owner = tree;
     }

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
@@ -45,6 +45,7 @@
 public class StateTree implements NodeOwner {
 
     private final class RootNode extends StateNode {
+
         private RootNode(Class<? extends NodeFeature>[] features) {
             super(features);
 
@@ -57,14 +58,22 @@ private RootNode(Class<? extends NodeFeature>[] features) {
 
         @Override
         public void setParent(StateNode parent) {
-            throw new IllegalStateException(
-                    "Can't set the parent of the tree root");
+            if (parent == null) {
+                super.setParent(null);
+                isRootAttached = false;
+            } else {
+                throw new IllegalStateException(
+                        "Can't set the parent of the tree root");
+            }
         }
 
         @Override
         public boolean isAttached() {
-            // Root is always attached
-            return true;
+            // the method is called from the super class (which is bad: call
+            // overridden method at the class init phase), so there the field
+            // from enclosing class is used because it's already initialized at
+            // the class constructor call.
+            return isRootAttached;
         }
 
     }
@@ -135,6 +144,11 @@ public interface ExecutionRegistration extends Registration {
 
     private final UIInternals uiInternals;
 
+    // This field actually belongs to RootNode class but it can'be moved there
+    // because its method isAttached() is called before the RootNode class is
+    // initialization is done.
+    private boolean isRootAttached = true;
+
     /**
      * Creates a new state tree with a set of features defined for the root
      * node.

diff --git a/flow-server/src/test/java/com/vaadin/flow/component/UITest.java b/flow-server/src/test/java/com/vaadin/flow/component/UITest.java
@@ -21,6 +21,7 @@
 import com.vaadin.flow.component.page.History;
 import com.vaadin.flow.component.page.History.HistoryStateChangeEvent;
 import com.vaadin.flow.dom.Element;
+import com.vaadin.flow.dom.ElementDetachEvent;
 import com.vaadin.flow.dom.Node;
 import com.vaadin.flow.dom.NodeVisitor;
 import com.vaadin.flow.dom.impl.AbstractTextElementStateProvider;
@@ -179,7 +180,8 @@ private static void initUI(UI ui, String initialLocation,
             routeConfiguration.update(() -> {
                 routeConfiguration.getHandledRegistry().clean();
                 Arrays.asList(RootNavigationTarget.class,
-                        FooBarNavigationTarget.class).forEach(routeConfiguration::setAnnotatedRoute);
+                        FooBarNavigationTarget.class)
+                        .forEach(routeConfiguration::setAnnotatedRoute);
             });
 
             ui.doInit(request, 0);
@@ -198,7 +200,8 @@ private static void initUI(UI ui, String initialLocation,
     @Test
     public void scrollAttribute() {
         UI ui = new UI();
-        Assert.assertNull("'scroll' attribute shouldn't be set for the "
+        Assert.assertNull(
+                "'scroll' attribute shouldn't be set for the "
                         + "UI element which represents 'body' tag",
                 ui.getElement().getAttribute("scroll"));
     }
@@ -263,8 +266,8 @@ public void locationAfterClientNavigation()
 
         History history = ui.getPage().getHistory();
 
-        history.getHistoryStateChangeHandler().onHistoryStateChange(
-                new HistoryStateChangeEvent(history, null,
+        history.getHistoryStateChangeHandler()
+                .onHistoryStateChange(new HistoryStateChangeEvent(history, null,
                         new Location("foo/bar"), NavigationTrigger.HISTORY));
 
         assertEquals("foo/bar",
@@ -364,6 +367,29 @@ public void unsetSession_detachEventIsFiredForUIChildren()
         assertEquals(childComponent, events.get(0).getSource());
     }
 
+    @Test
+    public void unserSession_datachEventIsFiredForElements() {
+        UI ui = createTestUI();
+
+        List<ElementDetachEvent> events = new ArrayList<>();
+
+        ui.getElement().addDetachListener(events::add);
+        initUI(ui, "", null);
+
+        Component childComponent = new AttachableComponent();
+        ui.add(childComponent);
+        childComponent.getElement().addDetachListener(events::add);
+
+        ui.getSession().access(() -> ui.getInternals().setSession(null));
+
+        // Unlock to run pending access tasks
+        ui.getSession().unlock();
+
+        assertEquals(2, events.size());
+        assertEquals(childComponent.getElement(), events.get(0).getSource());
+        assertEquals(ui.getElement(), events.get(1).getSource());
+    }
+
     @Test
     public void beforeClientResponse_regularOrder() {
         UI ui = createTestUI();
@@ -708,9 +734,8 @@ public void accessLaterRunnable_attachedUnlockedUi_runnableIsRun() {
     public void accessLaterRunnable_detachedUiNoHandler_throws() {
         UI ui = createTestUI();
 
-        SerializableRunnable wrapped = ui
-                .accessLater(() -> Assert.fail("Action should never run"),
-                        null);
+        SerializableRunnable wrapped = ui.accessLater(
+                () -> Assert.fail("Action should never run"), null);
         wrapped.run();
     }
 
@@ -720,9 +745,9 @@ public void accessLaterRunnable_detachedUi_detachHandlerCalled() {
 
         UI ui = createTestUI();
 
-        SerializableRunnable wrapped = ui
-                .accessLater(() -> Assert.fail("Action should never run"),
-                        runCount::incrementAndGet);
+        SerializableRunnable wrapped = ui.accessLater(
+                () -> Assert.fail("Action should never run"),
+                runCount::incrementAndGet);
 
         assertEquals("Handler should not yet have run", 0, runCount.get());
 
@@ -766,9 +791,8 @@ public void accessLaterConsumer_attachedUnlockedUi_runnableIsRun() {
     public void accessLaterConsumer_detachedUiNoHandler_throws() {
         UI ui = createTestUI();
 
-        SerializableConsumer<Object> wrapped = ui
-                .accessLater(value -> Assert.fail("Action should never run"),
-                        null);
+        SerializableConsumer<Object> wrapped = ui.accessLater(
+                value -> Assert.fail("Action should never run"), null);
         wrapped.accept(null);
     }
 
@@ -778,9 +802,9 @@ public void accessLaterConsumer_detachedUi_detachHandlerCalled() {
 
         UI ui = createTestUI();
 
-        SerializableConsumer<Object> wrapped = ui
-                .accessLater(value -> Assert.fail("Action should never run"),
-                        runCount::incrementAndGet);
+        SerializableConsumer<Object> wrapped = ui.accessLater(
+                value -> Assert.fail("Action should never run"),
+                runCount::incrementAndGet);
 
         assertEquals("Handler should not yet have run", 0, runCount.get());
 

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
@@ -24,6 +24,7 @@
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Consumer;
 
 import org.apache.commons.lang3.SerializationUtils;
@@ -93,7 +94,7 @@ protected void populateJson(JsonObject json,
     }
 
     @Test
-    public void testRootNodeState() {
+    public void rootNodeState() {
         StateNode rootNode = tree.getRootNode();
 
         Assert.assertNull("Root node should have no parent",
@@ -109,10 +110,23 @@ public void testRootNodeState() {
     }
 
     @Test(expected = IllegalStateException.class)
-    public void testRootNode_setParent_throws() {
+    public void rootNode_setStateNodeAsParent_throws() {
         tree.getRootNode().setParent(new StateNode());
     }
 
+    @Test
+    public void rootNode_setNullAsParent_nodeIsDetached() {
+        AtomicInteger detachCount = new AtomicInteger();
+        Assert.assertTrue(tree.hasNode(tree.getRootNode()));
+        tree.getRootNode()
+                .addDetachListener(() -> detachCount.incrementAndGet());
+        tree.getRootNode().setParent(null);
+        Assert.assertEquals(1, detachCount.get());
+        Assert.assertFalse(tree.getRootNode().isAttached());
+
+        Assert.assertFalse(tree.hasNode(tree.getRootNode()));
+    }
+
     @Test
     public void attachedNodeIsAttached() {
         StateNode node = StateNodeTest.createEmptyNode();