Skip to content

Commit

Permalink
fix: Remove old router layouts content before navigation (#10973)
Browse files Browse the repository at this point in the history 
Recursively removes content of the all nested router layouts of the given old content when the navigation occurs.
This is needed to let Dependency Injection frameworks (Spring and CDI add-ons) to re-create managed components with no duplicates/leftovers.

Fixes vaadin/cdi#345

Co-authored-by: @roeltje25
  • Loading branch information
@mshabarov
mshabarov committed May 17, 2021
1 parent 2838989 commit 5f5fdcd
Show file tree
Hide file tree
Showing 4 changed files with 483 additions and 4 deletions.
37 changes: 33 additions & 4 deletions flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -691,7 +692,7 @@ public void showRouteTarget(Location viewLocation, Component target,

// Assemble previous parent-child relationships to enable detecting
// changes
Map<RouterLayout, HasElement> oldChildren = new HashMap<>();
Map<RouterLayout, HasElement> oldChildren = new IdentityHashMap<>();
for (int i = 0; i < routerTargetChain.size() - 1; i++) {
HasElement child = routerTargetChain.get(i);
RouterLayout parent = (RouterLayout) routerTargetChain.get(i + 1);
Expand All @@ -706,6 +707,17 @@ public void showRouteTarget(Location viewLocation, Component target,
routerTargetChain.addAll(layouts);
}

// If the old and the new router target chains are not intersect,
// meaning that the new chain doesn't contain the root router
// layout node of the old chain, this aims to recursively remove
// content of the all nested router layouts of the given old content
// to be detached. This is needed to let Dependency Injection
// frameworks to re-create managed components with no
// duplicates/leftovers.
if (oldRoot != null && !routerTargetChain.contains(oldRoot)) {
oldChildren.forEach(RouterLayout::removeRouterLayoutContent);
}

// Ensure the entire chain is connected
HasElement previous = null;
for (HasElement current : routerTargetChain) {
Expand All @@ -725,9 +737,7 @@ public void showRouteTarget(Location viewLocation, Component target,

if (oldContent != newContent) {
RouterLayout layout = (RouterLayout) current;
if (oldContent != null) {
layout.removeRouterLayoutContent(oldContent);
}
removeChildrenContentFromRouterLayout(layout, oldChildren);
layout.showRouterLayoutContent(newContent);
}
}
Expand Down Expand Up @@ -1096,4 +1106,23 @@ private void configurePush(HasElement root) {
pushConfiguration.setTransport(push.get().transport());
}
}

private void removeChildrenContentFromRouterLayout(
final RouterLayout targetRouterLayout,
final Map<RouterLayout, HasElement> oldChildren) {
HasElement oldContent = oldChildren.get(targetRouterLayout);
RouterLayout removeFrom = targetRouterLayout;
// Recursively remove content of the all nested router
// layouts of the given old content to be detached. This
// is needed to let Dependency Injection frameworks to
// re-create managed components with no
// duplicates/leftovers.
while (oldContent != null) {
removeFrom.removeRouterLayoutContent(oldContent);
if (oldContent instanceof RouterLayout) {
removeFrom = (RouterLayout) oldContent;
}
oldContent = oldChildren.get(oldContent);
}
}
}
151 changes: 151 additions & 0 deletions flow-server/src/test/java/com/vaadin/flow/component/internal/UIInternalsTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.vaadin.flow.component.internal;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
Expand All @@ -15,6 +16,7 @@
import org.mockito.MockitoAnnotations;

import com.vaadin.flow.component.Component;
import com.vaadin.flow.component.HasElement;
import com.vaadin.flow.component.PushConfiguration;
import com.vaadin.flow.component.Tag;
import com.vaadin.flow.component.Text;
Expand All @@ -24,6 +26,7 @@
import com.vaadin.flow.dom.Element;
import com.vaadin.flow.function.DeploymentConfiguration;
import com.vaadin.flow.router.Location;
import com.vaadin.flow.router.ParentLayout;
import com.vaadin.flow.router.Route;
import com.vaadin.flow.router.RouterLayout;
import com.vaadin.flow.server.VaadinContext;
Expand Down Expand Up @@ -55,6 +58,54 @@ public static class RouteTarget1 extends Component {

}

@Tag(Tag.DIV)
static class MainLayout extends Component implements RouterLayout {
static String ID = "main-layout-id";

public MainLayout() {
setId(ID);
}
}

@Tag(Tag.DIV)
@ParentLayout(MainLayout.class)
static class SubLayout extends Component implements RouterLayout {
static String ID = "sub-layout-id";

public SubLayout() {
setId(ID);
}
}

@Tag(Tag.DIV)
@Route(value = "child", layout = SubLayout.class)
static class FirstView extends Component {
static String ID = "child-view-id";

public FirstView() {
setId(ID);
}
}

@Tag(Tag.DIV)
static class AnotherLayout extends Component implements RouterLayout {
static String ID = "another-layout-id";

public AnotherLayout() {
setId(ID);
}
}

@Tag(Tag.DIV)
@Route(value = "another", layout = MainLayout.class)
static class AnotherView extends Component {
static String ID = "another-view-id";

public AnotherView() {
setId(ID);
}
}

@Before
public void init() {
MockitoAnnotations.initMocks(this);
Expand Down Expand Up @@ -168,6 +219,106 @@ public void showRouteTarget_clientSideBootstrap() {
Mockito.verify(pushConfig, Mockito.never()).setPushMode(Mockito.any());
}

@Test
public void showRouteTarget_navigateToAnotherViewWithinSameLayoutHierarchy_detachedRouterLayoutChildrenRemoved() {
MainLayout mainLayout = new MainLayout();
SubLayout subLayout = new SubLayout();
FirstView firstView = new FirstView();
AnotherView anotherView = new AnotherView();

List<RouterLayout> oldLayouts = Arrays.asList(subLayout, mainLayout);
List<RouterLayout> newLayouts = Collections.singletonList(mainLayout);

Location location = Mockito.mock(Location.class);
setUpInitialPush();

internals.showRouteTarget(location, firstView, oldLayouts);
List<HasElement> activeRouterTargetsChain = internals
.getActiveRouterTargetsChain();

// Initial router layouts hierarchy is checked here in order to be
// sure the sub layout and it's child view is in place BEFORE
// navigation and old content cleanup
Assert.assertArrayEquals("Unexpected initial router targets chain",
new HasElement[] { firstView, subLayout, mainLayout },
activeRouterTargetsChain.toArray());

Assert.assertEquals(
"Expected one child element for main layout before navigation",
1, mainLayout.getElement().getChildren().count());
@SuppressWarnings("OptionalGetWithoutIsPresent")
Element subLayoutElement = mainLayout.getElement().getChildren()
.findFirst().get();
Assert.assertEquals("Unexpected sub layout element", SubLayout.ID,
subLayoutElement.getAttribute("id"));
Assert.assertEquals(
"Expected one child element for sub layout before navigation",
1, subLayoutElement.getChildren().count());
@SuppressWarnings("OptionalGetWithoutIsPresent")
Element firstViewElement = subLayoutElement.getChildren().findFirst()
.get();
Assert.assertEquals("Unexpected first view element", FirstView.ID,
firstViewElement.getAttribute("id"));

// Trigger navigation
internals.showRouteTarget(location, anotherView, newLayouts);
activeRouterTargetsChain = internals.getActiveRouterTargetsChain();
Assert.assertArrayEquals(
"Unexpected router targets chain after navigation",
new HasElement[] { anotherView, mainLayout },
activeRouterTargetsChain.toArray());

// Check that the old content (sub layout) is detached and it's
// children are also detached
Assert.assertEquals(
"Expected one child element for main layout after navigation",
1, mainLayout.getElement().getChildren().count());
@SuppressWarnings("OptionalGetWithoutIsPresent")
Element anotherViewElement = mainLayout.getElement().getChildren()
.findFirst().get();
Assert.assertEquals("Unexpected another view element", AnotherView.ID,
anotherViewElement.getAttribute("id"));
Assert.assertEquals(
"Expected no child elements for sub layout after navigation", 0,
subLayout.getElement().getChildren().count());
}

@Test
public void showRouteTarget_navigateToAnotherLayoutHierarchy_detachedLayoutHierarchyChildrenRemoved() {
MainLayout mainLayout = new MainLayout();
SubLayout subLayout = new SubLayout();
FirstView firstView = new FirstView();
AnotherLayout anotherLayout = new AnotherLayout();
AnotherView anotherView = new AnotherView();

List<RouterLayout> oldLayouts = Arrays.asList(subLayout, mainLayout);
List<RouterLayout> newLayouts = Collections
.singletonList(anotherLayout);

Location location = Mockito.mock(Location.class);
setUpInitialPush();

// Initial navigation
internals.showRouteTarget(location, firstView, oldLayouts);
// Navigate to another view outside of the initial router hierarchy
internals.showRouteTarget(location, anotherView, newLayouts);
List<HasElement> activeRouterTargetsChain = internals
.getActiveRouterTargetsChain();
Assert.assertArrayEquals(
"Unexpected router targets chain after navigation",
new HasElement[] { anotherView, anotherLayout },
activeRouterTargetsChain.toArray());

// Check that both main layout, sub layout and it's child view are
// detached
Assert.assertEquals(
"Expected no child elements for main layout after navigation",
0, mainLayout.getElement().getChildren().count());
Assert.assertEquals(
"Expected no child elements for sub layout after navigation", 0,
subLayout.getElement().getChildren().count());
}

private PushConfiguration setUpInitialPush() {
DeploymentConfiguration config = Mockito
.mock(DeploymentConfiguration.class);
Expand Down

0 comments on commit 5f5fdcd

Please sign in to comment.