fix: wait for fragment navigation to finish before finishing navigati…

…on command

src/bidiMapper/modules/context/BrowsingContextImpl.ts

@@ -46,6 +46,7 @@ import type {BrowsingContextStorage} from './BrowsingContextStorage.js';
 import {
   NavigationEventName,
   NavigationResult,
+  type NavigationState,
   NavigationTracker,
 } from './NavigationTracker.js';
 
@@ -814,7 +815,7 @@ export class BrowsingContextImpl {
       throw new InvalidArgumentException(`Invalid URL: ${url}`);
     }
 
-    const commandNavigation =
+    const navigationState =
       this.#navigationTracker.createPendingNavigation(url);
 
     // Navigate and wait for the result. If the navigation fails, the error event is
@@ -831,33 +832,26 @@ export class BrowsingContextImpl {
       if (cdpNavigateResult.errorText) {
         // If navigation failed, no pending navigation is left.
         this.#navigationTracker.failNavigation(
-          commandNavigation,
+          navigationState,
           cdpNavigateResult.errorText,
         );
         throw new UnknownErrorException(cdpNavigateResult.errorText);
       }
 
       this.#navigationTracker.navigationCommandFinished(
-        commandNavigation,
+        navigationState,
         cdpNavigateResult.loaderId,
       );
 
       this.#documentChanged(cdpNavigateResult.loaderId);
     })();
 
-    if (wait === BrowsingContext.ReadinessState.None) {
-      return {
-        navigation: commandNavigation.navigationId,
-        url,
-      };
-    }
-
     // Wait for either the navigation is finished or canceled by another navigation.
     const result = await Promise.race([
       // No `loaderId` means same-document navigation.
-      this.#waitNavigation(wait, cdpNavigatePromise),
+      this.#waitNavigation(wait, cdpNavigatePromise, navigationState),
       // Throw an error if the navigation is canceled.
-      commandNavigation.finished,
+      navigationState.finished,
     ]);
 
     if (result instanceof NavigationResult) {
@@ -872,28 +866,44 @@ export class BrowsingContextImpl {
     }
 
     return {
-      navigation: commandNavigation.navigationId,
+      navigation: navigationState.navigationId,
       // Url can change due to redirects. Get the one from commandNavigation.
-      url: commandNavigation.url,
+      url: navigationState.url,
     };
   }
 
   async #waitNavigation(
     wait: BrowsingContext.ReadinessState,
     cdpCommandPromise: Promise<void>,
+    navigationState: NavigationState,
   ) {
-    switch (wait) {
-      case BrowsingContext.ReadinessState.None:
-        return;
-      case BrowsingContext.ReadinessState.Interactive:
-        await cdpCommandPromise;
-        await this.#lifecycle.DOMContentLoaded;
-        return;
-      case BrowsingContext.ReadinessState.Complete:
-        await cdpCommandPromise;
-        await this.#lifecycle.load;
-        return;
+    if (wait === BrowsingContext.ReadinessState.None) {
+      return;
     }
+
+    await cdpCommandPromise;
+    if (navigationState.isFragmentNavigation === true) {
+      // After the cdp command is finished, the `fragmentNavigation` should be already
+      // settled. If it's the fragment navigation, wait for the `navigationStatus` to be
+      // finished, which happens after the fragment navigation happened. No need to wait for
+      // DOM events.
+      await navigationState.finished;
+      return;
+    }
+
+    if (wait === BrowsingContext.ReadinessState.Interactive) {
+      await this.#lifecycle.DOMContentLoaded;
+      return;
+    }
+
+    if (wait === BrowsingContext.ReadinessState.Complete) {
+      await this.#lifecycle.load;
+      return;
+    }
+
+    throw new InvalidArgumentException(
+      `Wait condition ${wait} is not supported`,
+    );
   }
 
   // TODO: support concurrent navigations analogous to `navigate`.
@@ -905,7 +915,7 @@ export class BrowsingContextImpl {
 
     this.#resetLifecycleIfFinished();
 
-    const commandNavigation = this.#navigationTracker.createPendingNavigation(
+    const navigationState = this.#navigationTracker.createPendingNavigation(
       this.#navigationTracker.url,
     );
 
@@ -919,9 +929,9 @@ export class BrowsingContextImpl {
     // Wait for either the navigation is finished or canceled by another navigation.
     const result = await Promise.race([
       // No `loaderId` means same-document navigation.
-      this.#waitNavigation(wait, cdpReloadPromise),
+      this.#waitNavigation(wait, cdpReloadPromise, navigationState),
       // Throw an error if the navigation is canceled.
-      commandNavigation.finished,
+      navigationState.finished,
     ]);
 
     if (result instanceof NavigationResult) {
@@ -934,9 +944,9 @@ export class BrowsingContextImpl {
     }
 
     return {
-      navigation: commandNavigation.navigationId,
+      navigation: navigationState.navigationId,
       // Url can change due to redirects. Get the one from commandNavigation.
-      url: commandNavigation.url,
+      url: navigationState.url,
     };
   }
 

src/bidiMapper/modules/context/NavigationTracker.ts

@@ -46,7 +46,7 @@ export class NavigationResult {
   }
 }
 
-class NavigationState {
+export class NavigationState {
   readonly navigationId = uuidv4();
   readonly #browsingContextId: string;
 
@@ -57,6 +57,7 @@ class NavigationState {
   #isInitial: boolean;
   #eventManager: EventManager;
   #navigated = false;
+  isFragmentNavigation?: boolean;
 
   get finished(): Promise<NavigationResult> {
     return this.#finished;
@@ -161,10 +162,6 @@ export class NavigationTracker {
   // Flags if the initial navigation to `about:blank` is in progress.
   #isInitialNavigation = true;
 
-  navigation = {
-    withinDocument: new Deferred<void>(),
-  };
-
   constructor(
     url: string,
     browsingContextId: string,
@@ -392,6 +389,8 @@ export class NavigationTracker {
       this.#loaderIdToNavigationsMap.set(loaderId, navigation);
     }
 
+    navigation.isFragmentNavigation = loaderId === undefined;
+
     if (loaderId === undefined || this.#currentNavigation === navigation) {
       // If the command's navigation is same-document or is already the current one,
       // nothing to do.

tests/browsing_context/test_navigate.py

@@ -122,16 +122,16 @@ async def test_browsingContext_navigateWaitNone_navigated(
 
 @pytest.mark.asyncio
 async def test_browsingContext_navigateWaitInteractive_navigated(
-        websocket, context_id, html):
-    url = html("<h2>test</h2>")
+        websocket, context_id, html, url_hang_forever,
+        assert_no_more_messages):
+    url = html(f"<h2>test</h2><img src='{url_hang_forever}'/>")
 
     await subscribe(
         websocket,
         ["browsingContext.domContentLoaded", "browsingContext.load"])
 
-    await send_JSON_command(
+    command_id = await send_JSON_command(
         websocket, {
-            "id": 14,
             "method": "browsingContext.navigate",
             "params": {
                 "url": url,
@@ -157,26 +157,15 @@ async def test_browsingContext_navigateWaitInteractive_navigated(
     # Assert command done.
     resp = await read_JSON_message(websocket)
     assert resp == {
-        "id": 14,
+        "id": command_id,
         "type": "success",
         "result": {
             "navigation": navigation_id,
             "url": url
         }
     }
 
-    # Wait for `browsingContext.load` event.
-    resp = await read_JSON_message(websocket)
-    assert resp == {
-        'type': 'event',
-        "method": "browsingContext.load",
-        "params": {
-            "context": context_id,
-            "navigation": navigation_id,
-            "timestamp": ANY_TIMESTAMP,
-            "url": url
-        }
-    }
+    await assert_no_more_messages()
 
 
 @pytest.mark.asyncio

tests/conftest.py

@@ -58,6 +58,21 @@ def local_server_http_another_host() -> Generator[LocalHttpServer, None, None]:
         return
 
 
+@pytest_asyncio.fixture(scope='session')
+def local_server_http_yet_another_host(
+) -> Generator[LocalHttpServer, None, None]:
+    """
+    Returns an instance of a LocalHttpServer without SSL pointing to `::1`
+    """
+    server = LocalHttpServer('::1')
+    yield server
+
+    server.clear()
+    if server.is_running():
+        server.stop()
+        return
+
+
 @pytest_asyncio.fixture(scope='session')
 def local_server_bad_ssl() -> Generator[LocalHttpServer, None, None]:
     """ Returns an instance of a LocalHttpServer with bad SSL certificate. """
@@ -273,12 +288,12 @@ def url_auth_required(local_server_http):
 
 
 @pytest.fixture
-def url_hang_forever(local_server_http):
+def url_hang_forever(local_server_http_yet_another_host):
     """Return a URL that hangs forever."""
     try:
-        yield local_server_http.url_hang_forever()
+        yield local_server_http_yet_another_host.url_hang_forever()
     finally:
-        local_server_http.hang_forever_stop()
+        local_server_http_yet_another_host.hang_forever_stop()
 
 
 @pytest.fixture(scope="session")

tests/tools/test_local_http_server.py

@@ -50,6 +50,20 @@ async def test_local_server_200(websocket, context_id, local_server_http):
            == local_server_http.content_200
 
 
+@pytest.mark.asyncio
+async def test_local_server_http_another_host_200(
+        websocket, context_id, local_server_http_another_host):
+    assert await get_content(websocket, context_id, local_server_http_another_host.url_200()) \
+           == local_server_http_another_host.content_200
+
+
+@pytest.mark.asyncio
+async def test_local_server_http_yet_another_host_200(
+        websocket, context_id, local_server_http_yet_another_host):
+    assert await get_content(websocket, context_id, local_server_http_yet_another_host.url_200()) \
+           == local_server_http_yet_another_host.content_200
+
+
 @pytest.mark.asyncio
 async def test_local_server_custom_content(websocket, context_id,
                                            local_server_http):