From f40312fac3a09890f01e19ee29295c7590dbe2f0 Mon Sep 17 00:00:00 2001 From: Vaadin Bot Date: Fri, 29 Nov 2024 12:21:58 +0100 Subject: [PATCH] fix: remove modal component on route refresh (#20540) (#20582) Modal components attached to the UI were not removed or replaced during self-navigation triggered by a route refresh. This change updates navigation handler to ensure modal components are removed and adds a new navigation trigger for route refresh to differentiate programmatic navigations (e.g., forward actions). It also modifies Hotswapper to require a full chain refresh when modal components are present. Fixes #20473 Co-authored-by: Marco Collovati --- .../flow/component/internal/UIInternals.java | 11 ++-- .../com/vaadin/flow/hotswap/Hotswapper.java | 7 ++- .../vaadin/flow/router/NavigationTrigger.java | 9 +++- .../AbstractNavigationStateRenderer.java | 18 +++++-- .../vaadin/flow/hotswap/HotswapperTest.java | 52 +++++++++++++++++- .../internal/NavigationStateRendererTest.java | 54 ++++++++++++++++++- .../uitest/ui/RefreshCurrentRouteView.java | 48 +++++++++++++++++ .../flow/uitest/ui/RefreshCurrentRouteIT.java | 35 ++++++++++++ 8 files changed, 223 insertions(+), 11 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 index fb6c0dadc88..a341e56d5a2 100644 --- 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 @@ -77,7 +77,6 @@ import com.vaadin.flow.router.internal.BeforeEnterHandler; import com.vaadin.flow.router.internal.BeforeLeaveHandler; import com.vaadin.flow.server.Command; -import com.vaadin.flow.server.RouteRegistry; import com.vaadin.flow.server.VaadinService; import com.vaadin.flow.server.VaadinSession; import com.vaadin.flow.server.communication.PushConnection; @@ -1094,6 +1093,12 @@ public void setLastHandledNavigation(Location location) { /** * Re-navigates to the current route. Also re-instantiates the route target * component, and optionally all layouts in the route chain. + *

+ *

+ * If modal components are currently defined for the UI, the whole route + * chain will be refreshed regardless the {@code refreshRouteChain} + * parameter, because otherwise it would not be possible to preserve the + * correct modality cardinality and order. * * @param refreshRouteChain * {@code true} to refresh all layouts in the route chain, @@ -1105,8 +1110,8 @@ public void refreshCurrentRoute(boolean refreshRouteChain) { + "Unable to refresh the current route."); } else { getRouter().navigate(ui, locationForRefresh, - NavigationTrigger.PROGRAMMATIC, null, true, - refreshRouteChain); + NavigationTrigger.REFRESH_ROUTE, null, true, + refreshRouteChain || hasModalComponent()); } } diff --git a/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java b/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java index 10e12d7c026..905592dd7ee 100644 --- a/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java +++ b/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java @@ -350,7 +350,11 @@ private UIRefreshStrategy computeRefreshStrategy(UI ui, .distinct().toList(); UIRefreshStrategy refreshStrategy; - if (!targetChainChangedItems.isEmpty()) { + // A full chain refresh should be triggered if there are modal + // components, since they could be attached to UI or parent layouts + if (ui.hasModalComponent()) { + refreshStrategy = UIRefreshStrategy.PUSH_REFRESH_CHAIN; + } else if (!targetChainChangedItems.isEmpty()) { refreshStrategy = targetChainChangedItems.stream() .allMatch(chainItem -> chainItem == route) ? UIRefreshStrategy.PUSH_REFRESH_ROUTE @@ -362,6 +366,7 @@ private UIRefreshStrategy computeRefreshStrategy(UI ui, refreshStrategy = computeRefreshStrategyForUITree(ui, changedClasses, targetsChain, route); } + // A different layout might have been applied after hotswap if (refreshStrategy == UIRefreshStrategy.SKIP) { RouteRegistry registry = ui.getInternals().getRouter() diff --git a/flow-server/src/main/java/com/vaadin/flow/router/NavigationTrigger.java b/flow-server/src/main/java/com/vaadin/flow/router/NavigationTrigger.java index 5741ab83d51..c4d358b128e 100644 --- a/flow-server/src/main/java/com/vaadin/flow/router/NavigationTrigger.java +++ b/flow-server/src/main/java/com/vaadin/flow/router/NavigationTrigger.java @@ -66,5 +66,12 @@ public enum NavigationTrigger { /** * Navigation is for a reload event on a preserveOnRefresh route. */ - REFRESH + REFRESH, + + /** + * Navigation was triggered via {@link UI#refreshCurrentRoute(boolean)}. + * It's for internal use only. + */ + REFRESH_ROUTE, + } diff --git a/flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java b/flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java index 393acc47286..86092096d7f 100644 --- a/flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java +++ b/flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java @@ -24,7 +24,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Supplier; @@ -41,6 +40,7 @@ import com.vaadin.flow.internal.Pair; import com.vaadin.flow.internal.StateNode; import com.vaadin.flow.internal.UsageStatistics; +import com.vaadin.flow.internal.menu.MenuRegistry; import com.vaadin.flow.router.AfterNavigationEvent; import com.vaadin.flow.router.BeforeEnterEvent; import com.vaadin.flow.router.BeforeEnterObserver; @@ -51,7 +51,6 @@ import com.vaadin.flow.router.ErrorNavigationEvent; import com.vaadin.flow.router.ErrorParameter; import com.vaadin.flow.router.EventUtil; -import com.vaadin.flow.router.HasDynamicTitle; import com.vaadin.flow.router.Location; import com.vaadin.flow.router.LocationChangeEvent; import com.vaadin.flow.router.NavigationEvent; @@ -62,7 +61,6 @@ import com.vaadin.flow.router.PageTitle; import com.vaadin.flow.router.PreserveOnRefresh; import com.vaadin.flow.router.QueryParameters; -import com.vaadin.flow.router.Route; import com.vaadin.flow.router.RouteParameters; import com.vaadin.flow.router.Router; import com.vaadin.flow.router.RouterLayout; @@ -70,7 +68,6 @@ import com.vaadin.flow.server.HttpStatusCode; import com.vaadin.flow.server.RouteRegistry; import com.vaadin.flow.server.VaadinSession; -import com.vaadin.flow.internal.menu.MenuRegistry; import com.vaadin.flow.server.menu.AvailableViewInfo; /** @@ -258,6 +255,19 @@ public int handle(NavigationEvent event) { List routerLayouts = (List) (List) chain .subList(1, chain.size()); + // If a route refresh has been requested, remove all modal components. + // This is necessary because maintaining the correct modality + // cardinality and order is not feasible without knowing who opened them + // and when. + if (ui.hasModalComponent() + && event.getTrigger() == NavigationTrigger.REFRESH_ROUTE) { + Component modalComponent; + while ((modalComponent = ui.getInternals() + .getActiveModalComponent()) != null) { + modalComponent.removeFromParent(); + } + } + // Change the UI according to the navigation Component chain. ui.getInternals().showRouteTarget(event.getLocation(), componentInstance, routerLayouts); diff --git a/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java b/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java index 4ae56203b0c..b46c9615c4d 100644 --- a/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java @@ -38,6 +38,8 @@ import com.vaadin.flow.di.Lookup; import com.vaadin.flow.internal.BrowserLiveReload; import com.vaadin.flow.internal.BrowserLiveReloadAccessor; +import com.vaadin.flow.router.AfterNavigationEvent; +import com.vaadin.flow.router.AfterNavigationObserver; import com.vaadin.flow.router.Layout; import com.vaadin.flow.router.ParentLayout; import com.vaadin.flow.router.Route; @@ -280,6 +282,21 @@ public void onHotswap_pushDisabled_routeClassChanged_UINotRefreshedButLiveReload Mockito.verify(liveReload).refresh(anyBoolean()); } + @Test + public void onHotswap_pushDisabled_routeClassChanged_modalComponents_UINotRefreshedButLiveReloadFullRefreshTriggered() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + RefreshTestingUI ui = initUIAndNavigateTo(session, + MyRouteWithModal.class); + + hotswapper.onHotswap(new String[] { MyRouteWithModal.class.getName() }, + true); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload).refresh(true); + } + @Test public void onHotswap_pushDisabled_autoLayout_classUnrelatedToUIChanged_noReload() throws ServiceException { @@ -470,6 +487,24 @@ public void onHotswap_pushEnabled_routeClassChanged_routeRefreshed() Mockito.verify(liveReload, never()).refresh(anyBoolean()); } + @Test + public void onHotswap_pushEnabled_routeClassChanged_modalComponent_activeChainRefreshed() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + + RefreshTestingUI ui = initUIAndNavigateTo(session, + MyRouteWithModal.class); + ui.enablePush(); + + hotswapper.onHotswap(new String[] { MyRouteWithModal.class.getName() }, + true); + + ui.assertChainRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + } + @Test public void onHotswap_pushEnabled_routeLayoutClassChanged_activeChainRefreshed() throws ServiceException { @@ -823,6 +858,17 @@ public MyRouteWithChild() { } } + @Tag("my-route-with-modal") + public static class MyRouteWithModal extends Component + implements HasComponents, AfterNavigationObserver { + + @Override + public void afterNavigation(AfterNavigationEvent event) { + event.getLocationChangeEvent().getUI().addModal(new MyComponent()); + } + + } + @Tag("my-layout") public static class MyLayout extends Component implements RouterLayout { @@ -903,7 +949,11 @@ public RefreshTestingUI(VaadinSession session) { @Override public void refreshCurrentRoute(boolean refreshRouteChain) { refreshRouteChainRequested = refreshRouteChain; - super.refreshCurrentRoute(refreshRouteChain); + // No need to perform real navigation, tests only need to know if + // the method has been invoked. + // Navigation would fail anyway because of usage of method scoped + // classes. Blocking navigation prevents logs to be bloated by + // exception stack traces. } void assertNotRefreshed() { diff --git a/flow-server/src/test/java/com/vaadin/flow/router/internal/NavigationStateRendererTest.java b/flow-server/src/test/java/com/vaadin/flow/router/internal/NavigationStateRendererTest.java index 48be2a1a6bc..41aa569bd41 100644 --- a/flow-server/src/test/java/com/vaadin/flow/router/internal/NavigationStateRendererTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/router/internal/NavigationStateRendererTest.java @@ -40,7 +40,9 @@ import org.mockito.MockedStatic; import org.mockito.Mockito; +import com.vaadin.flow.component.AttachEvent; import com.vaadin.flow.component.Component; +import com.vaadin.flow.component.DetachEvent; import com.vaadin.flow.component.HasElement; import com.vaadin.flow.component.Tag; import com.vaadin.flow.component.Text; @@ -53,6 +55,7 @@ import com.vaadin.flow.function.DeploymentConfiguration; import com.vaadin.flow.internal.ReflectTools; import com.vaadin.flow.internal.UsageStatistics; +import com.vaadin.flow.internal.menu.MenuRegistry; import com.vaadin.flow.router.AfterNavigationEvent; import com.vaadin.flow.router.AfterNavigationObserver; import com.vaadin.flow.router.BeforeEnterEvent; @@ -81,11 +84,11 @@ import com.vaadin.flow.server.ServiceException; import com.vaadin.flow.server.WrappedSession; import com.vaadin.flow.server.menu.AvailableViewInfo; -import com.vaadin.flow.internal.menu.MenuRegistry; import com.vaadin.flow.server.startup.ApplicationRouteRegistry; import com.vaadin.tests.util.AlwaysLockedVaadinSession; import com.vaadin.tests.util.MockDeploymentConfiguration; import com.vaadin.tests.util.MockUI; + import elemental.json.Json; import elemental.json.JsonValue; @@ -817,6 +820,55 @@ public void handle_clientNavigation_withMatchingFlowRoute() { } } + @Test + public void handle_refreshRoute_modalComponentsDetached() { + beforeEnterCount = new AtomicInteger(); + viewAttachCount = new AtomicInteger(); + + // given a service with instantiator + MockVaadinServletService service = createMockServiceWithInstantiator(); + + // given a locked session + MockVaadinSession session = new AlwaysLockedVaadinSession(service); + session.setConfiguration(new MockDeploymentConfiguration()); + + // given a NavigationStateRenderer mapping to PreservedNestedView + Router router = session.getService().getRouter(); + NavigationStateRenderer renderer = new NavigationStateRenderer( + new NavigationStateBuilder(router) + .withTarget(RootRouteWithParam.class).withPath("") + .build()); + router.getRegistry().setRoute("", RootRouteWithParam.class, null); + + @Tag("modal-component") + class ModalComponent extends Component { + private int attachCount; + private int detachCount; + + @Override + protected void onAttach(AttachEvent attachEvent) { + attachCount++; + super.onAttach(attachEvent); + } + + @Override + protected void onDetach(DetachEvent detachEvent) { + detachCount++; + super.onDetach(detachEvent); + } + } + + ModalComponent modalComponent = new ModalComponent(); + MockUI ui = new MockUI(session); + ui.addModal(modalComponent); + + renderer.handle(new NavigationEvent(router, new Location(""), ui, + NavigationTrigger.REFRESH_ROUTE, null, false, true, true)); + + Assert.assertEquals(1, modalComponent.attachCount); + Assert.assertEquals(1, modalComponent.detachCount); + } + private MockVaadinServletService createMockServiceWithInstantiator() { MockVaadinServletService service = new MockVaadinServletService(); service.init(new MockInstantiator() { diff --git a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/RefreshCurrentRouteView.java b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/RefreshCurrentRouteView.java index dca5cd70578..f654c9065f4 100644 --- a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/RefreshCurrentRouteView.java +++ b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/RefreshCurrentRouteView.java @@ -28,6 +28,7 @@ public class RefreshCurrentRouteView extends Div implements BeforeEnterObserver, final static String NAVIGATE_ID = "navigate"; final static String REFRESH_ID = "refresh"; final static String REFRESH_LAYOUTS_ID = "refreshlayouts"; + final static String OPEN_MODALS_ID = "openmodals"; private int attach, detach, afterNav, beforeEnter, beforeLeave; private final Div id, attachCounter, detachCounter, afterNavCounter, @@ -58,6 +59,11 @@ public RefreshCurrentRouteView() { e -> UI.getCurrent().refreshCurrentRoute(true)); refresh.setId(REFRESH_LAYOUTS_ID); add(refresh); + + NativeButton openModals = new NativeButton("Open modal components", + e -> openModals()); + openModals.setId(OPEN_MODALS_ID); + add(openModals); } protected String getNavigationTarget() { @@ -72,6 +78,12 @@ private Div createCounterSpan(String id) { return counter; } + private void openModals() { + new Dialog(1).open(); + new Dialog(2).open(); + new Dialog(3).open(); + } + @Override protected void onAttach(AttachEvent event) { super.onAttach(event); @@ -81,6 +93,8 @@ protected void onAttach(AttachEvent event) { @Override public void afterNavigation(AfterNavigationEvent event) { afterNavCounter.setText(Integer.toString(++afterNav)); + event.getLocationChangeEvent().getQueryParameter("modal") + .ifPresent(unused -> openModals()); } @Override @@ -98,4 +112,38 @@ protected void onDetach(DetachEvent event) { super.onDetach(event); detachCounter.setText(Integer.toString(++detach)); } + + public static class Dialog extends Div { + + public Dialog(int dialogId) { + setId("modal-" + dialogId); + add(new Div("modal " + dialogId)); + NativeButton button = new NativeButton("Refresh route", + ev -> UI.getCurrent().refreshCurrentRoute(false)); + button.setId("modal-" + dialogId + "-" + REFRESH_ID); + add(button); + + button = new NativeButton("Refresh all", + ev -> UI.getCurrent().refreshCurrentRoute(false)); + button.setId("modal-" + dialogId + "-" + REFRESH_LAYOUTS_ID); + add(button); + + button = new NativeButton("Close", ev -> close()); + button.setId("modal-" + dialogId + "-close"); + add(button); + getStyle().set("position", "fixed").set("inset", "10% 10%") + .setWidth("50%").setHeight("50%") + .setBackgroundColor("green").setBorder("1px solid black") + .setZIndex(dialogId); + } + + public void open() { + UI.getCurrent().addModal(this); + } + + public void close() { + UI.getCurrent().remove(this); + } + } + } diff --git a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RefreshCurrentRouteIT.java b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RefreshCurrentRouteIT.java index fd1b5c680e1..f7181080de0 100644 --- a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RefreshCurrentRouteIT.java +++ b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RefreshCurrentRouteIT.java @@ -13,6 +13,7 @@ import static com.vaadin.flow.uitest.ui.RefreshCurrentRouteView.DETACHCOUNTER_ID; import static com.vaadin.flow.uitest.ui.RefreshCurrentRouteView.ID; import static com.vaadin.flow.uitest.ui.RefreshCurrentRouteView.NAVIGATE_ID; +import static com.vaadin.flow.uitest.ui.RefreshCurrentRouteView.OPEN_MODALS_ID; import static com.vaadin.flow.uitest.ui.RefreshCurrentRouteView.REFRESH_ID; import static com.vaadin.flow.uitest.ui.RefreshCurrentRouteView.REFRESH_LAYOUTS_ID; import static com.vaadin.flow.uitest.ui.RefreshCurrentRouteLayout.ROUTER_LAYOUT_ID; @@ -81,6 +82,40 @@ public void refreshCurrentRoute_ensureNewInstanceAndCorrectEventCounts_newLayout assertInitialEventCounters(); } + public void refreshCurrentRoute_modalComponents_newRouteAndLayout() { + open("modal=true"); + + final String originalId = getString(ID); + final String originalLayoutId = getString(ROUTER_LAYOUT_ID); + + assertInitialEventCounters(); + + waitForElementPresent(By.id("modal-1")); + waitForElementPresent(By.id("modal-2")); + waitForElementPresent(By.id("modal-3")); + $(NativeButtonElement.class).id("modal-3-refresh").click(); + + // UUID should be new since refresh creates new instance + Assert.assertNotEquals(getString(ID), originalId); + // UUID should be new since new layout instances were requested + Assert.assertNotEquals(getString(ROUTER_LAYOUT_ID), originalLayoutId); + + // Event counters should equal original values + assertInitialEventCounters(); + + waitForElementPresent(By.id("modal-1")); + waitForElementPresent(By.id("modal-2")); + waitForElementPresent(By.id("modal-3")); + + $(NativeButtonElement.class).id("modal-3-close").click(); + $(NativeButtonElement.class).id("modal-2-close").click(); + $(NativeButtonElement.class).id("modal-1-refresh").click(); + + waitForElementPresent(By.id("modal-1")); + waitForElementPresent(By.id("modal-2")); + waitForElementPresent(By.id("modal-3")); + } + private void assertInitialEventCounters() { Assert.assertEquals(1, getInt(ATTACHCOUNTER_ID)); Assert.assertEquals(0, getInt(DETACHCOUNTER_ID));