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 <[email protected]>
vaadin · Nov 29, 2024 · f40312f · f40312f
1 parent 991e55a
commit f40312f
Show file tree

Hide file tree

Showing 8 changed files with 223 additions and 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
@@ -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.
+     * <p>
+     * </p>
+     * 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
@@ -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
@@ -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/...server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java b/...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,15 +61,13 @@
 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;
 import com.vaadin.flow.server.Constants;
 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<RouterLayout> routerLayouts = (List<RouterLayout>) (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
@@ -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
@@ -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/...ts/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/RefreshCurrentRouteView.java b/...ts/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);
+        }
+    }
+
 }