fix: ensure Flow client container is pre-attached on navigation (#12228)

Fixes #12080

The client-side Vaadin Router does not attach components until
the `onBeforeEnter` callback is run. However, when the Flow client
initializes a server-side view, it assumes its container and the UI root
are connected to the document. This concerns constructor timing as well
as `AttachEvent` listeners.

This fixes the issue by pre-attaching the hidden container element created in
the `Flow.ts` (the adapter for the client-side router) to the body.

flow-client/src/main/frontend/Flow.ts

@@ -275,17 +275,19 @@ export class Flow {
         const tag = `flow-container-${appId.toLowerCase()}`;
         this.container = flowRoot.$[appId] = document.createElement(tag);
         this.container.id = appId;
-
-        // It might be that components created from server expect that their content has been rendered.
-        // Appending eagerly the container we avoid these kind of errors.
-        // Note that the client router will move this container to the outlet if the navigation succeed
-        this.container.style.display = 'none';
-        document.body.appendChild(this.container);
       }
 
       // hide flow progress indicator
       this.loadingFinished();
     }
+
+    // It might be that components created from server expect that their content has been rendered.
+    // Appending eagerly the container we avoid these kind of errors.
+    // Note that the client router will move this container to the outlet if the navigation succeed
+    if (this.container && !this.container.isConnected) {
+      this.container.style.display = 'none';
+      document.body.appendChild(this.container);
+    }
     return this.response!;
   }
 

flow-client/src/test/frontend/FlowTests.ts

@@ -5,7 +5,11 @@ const { assert } = intern.getPlugin('chai');
 const { sinon } = intern.getPlugin('sinon') as { sinon: SinonStatic };
 
 // API to test
-import { Flow, NavigationParameters } from '../../main/frontend/Flow';
+import {
+  Flow,
+  NavigationParameters,
+  PreventAndRedirectCommands
+} from '../../main/frontend/Flow';
 import { ConnectionState, ConnectionStateStore } from '@vaadin/common-frontend';
 // Intern does not serve webpack chunks, adding deps here in order to
 // produce one chunk, because dynamic imports in Flow.ts  will not work.
@@ -763,6 +767,43 @@ suite('Flow', () => {
     $wnd.dispatchEvent(new Event('online')); // caught by DefaultConnectionStateHandler
     assert.equal($wnd.Vaadin.connectionState.state, ConnectionState.RECONNECTING);
   });
+
+  test("should pre-attach container element on every navigation", async () => {
+    stubServerRemoteFunction('foobar-12345');
+    mockInitResponse('foobar-12345');
+
+    const flow = new Flow();
+    const route = flow.serverSideRoutes[0];
+
+    const flowRouteParams = {pathname: 'Foo', search: ''};
+    const otherRouteParams = {pathname: 'Lorem', search: ''};
+
+    // Initial navigation
+    const container = await route.action(flowRouteParams);
+    assert.isTrue(container.isConnected);
+    assert.equal(container.style.display, 'none');
+
+    // @ts-ignore
+    await container.onBeforeEnter(flowRouteParams, {});
+    assert.isTrue(container.isConnected);
+    assert.equal(container.style.display, '');
+
+    // Leave
+
+    // @ts-ignore
+    await container.onBeforeLeave(otherRouteParams, {});
+    // The router detaches the container, possibly because it renders a client-side view
+    container.parentNode!.removeChild(container);
+
+    await route.action(flowRouteParams);
+    assert.isTrue(container.isConnected);
+    assert.equal(container.style.display, 'none');
+
+    // @ts-ignore
+    await container.onBeforeEnter(flowRouteParams, {});
+    assert.isTrue(container.isConnected);
+    assert.equal(container.style.display, '');
+  });
 });
 
 function stubServerRemoteFunction(

flow-tests/test-ccdm-flow-navigation/src/main/java/com/vaadin/flow/navigate/HelloWorldView.java

@@ -16,23 +16,43 @@
 
 package com.vaadin.flow.navigate;
 
+import com.vaadin.flow.component.Text;
+import com.vaadin.flow.component.html.NativeButton;
+import com.vaadin.flow.component.html.Paragraph;
 import com.vaadin.flow.component.html.Span;
 import com.vaadin.flow.router.PageTitle;
 import com.vaadin.flow.router.Route;
-import com.vaadin.flow.component.html.NativeButton;
 
 @Route(value = "hello")
 @PageTitle("Hello World")
 public class HelloWorldView extends Span {
-
     public static final String NAVIGATE_ABOUT = "navigate-about";
+    public static final String IS_CONNECTED_ON_INIT = "is-connected-on-init";
+    public static final String IS_CONNECTED_ON_ATTACH = "is-connected-on-attach";
+
+    private final Span isConnectedOnInit = new Span("");
+    private final Span isConnectedOnAttach = new Span("");
 
     public HelloWorldView() {
         setId("hello-world-view");
         NativeButton toAbout = new NativeButton("Say hello",
                 e -> getUI().get().navigate("about"));
         toAbout.setId(NAVIGATE_ABOUT);
         add(toAbout);
+
+        isConnectedOnInit.setId(IS_CONNECTED_ON_INIT);
+        updateIsConnected(isConnectedOnInit);
+        add(new Paragraph(new Text("Connected on init: "), isConnectedOnInit));
+
+        isConnectedOnAttach.setId(IS_CONNECTED_ON_ATTACH);
+        isConnectedOnAttach.addAttachListener(
+                event -> updateIsConnected(isConnectedOnAttach));
+        add(new Paragraph(new Text("Connected on attach: "),
+                isConnectedOnAttach));
     }
 
+    private void updateIsConnected(Span output) {
+        output.getElement()
+                .executeJs("this.textContent=String(this.isConnected)");
+    }
 }

flow-tests/test-ccdm-flow-navigation/src/test/java/com/vaadin/flow/navigate/NavigateBetweenViewsIT.java

@@ -21,8 +21,11 @@
 import org.openqa.selenium.By;
 
 import com.vaadin.flow.component.html.testbench.NativeButtonElement;
+import com.vaadin.flow.component.html.testbench.SpanElement;
 import com.vaadin.flow.testutil.ChromeBrowserTest;
 
+import static com.vaadin.flow.navigate.HelloWorldView.IS_CONNECTED_ON_ATTACH;
+import static com.vaadin.flow.navigate.HelloWorldView.IS_CONNECTED_ON_INIT;
 import static com.vaadin.flow.navigate.HelloWorldView.NAVIGATE_ABOUT;
 
 public class NavigateBetweenViewsIT extends ChromeBrowserTest {
@@ -70,8 +73,55 @@ public void openTsView_navigateToFlow_navigationSuccessful() {
                 $(NativeButtonElement.class).id(NAVIGATE_ABOUT).isDisplayed());
     }
 
+    @Test
+    public void openFlowView_isConnectedOnAttach() {
+        getDriver().get(getRootURL() + "/hello");
+        waitForDevServer();
+
+        Assert.assertThat(getDriver().getCurrentUrl(),
+                CoreMatchers.endsWith("/hello"));
+
+        assertIsConnected();
+
+        // Navigate away and back
+        $(NativeButtonElement.class).id(NAVIGATE_ABOUT).click();
+        waitUntil(input -> $("about-view").first().$("a").id("navigate-hello")
+                .isDisplayed());
+
+        getInShadowRoot(findElement(By.tagName("about-view")),
+                By.id("navigate-hello")).click();
+
+        assertIsConnected();
+    }
+
+    @Test
+    public void openTsView_navigateToFlowView_isConnectedOnAttach() {
+        getDriver().get(getRootURL() + "/");
+        waitForDevServer();
+
+        waitUntil(input -> $("about-view").first().$("a").id("navigate-hello")
+                .isDisplayed());
+
+        getInShadowRoot(findElement(By.tagName("about-view")),
+                By.id("navigate-hello")).click();
+
+        getCommandExecutor().waitForVaadin();
+
+        assertIsConnected();
+    }
+
     @Override
     protected String getRootURL() {
         return super.getRootURL() + "/context-path";
     }
+
+    private void assertIsConnected() {
+        assertIsConnectedById(IS_CONNECTED_ON_INIT);
+        assertIsConnectedById(IS_CONNECTED_ON_ATTACH);
+    }
+
+    private void assertIsConnectedById(String id) {
+        Assert.assertTrue(Boolean.parseBoolean(
+                waitUntil(driver -> $(SpanElement.class).id(id)).getText()));
+    }
 }