[FIX] web: router_service: concurrent hashchange

Do an action that will change the hash.
During this, change the url hash to load another action

Before this commit, the hash taken to load the second action was wrong, because the first
action pushed its state instants before the hashchange event is actually triggered.
This is because the hashchange event is triggered in a non blocking stack
(https://html.spec.whatwg.org/multipage/browsing-the-web.html#scroll-to-fragid)

After this commit, the right action is loaded with the right hash

closes odoo/odoo#72878

Signed-off-by: Aaron Bohy (aab) <aab@odoo.com>

addons/web/static/src/core/browser/router_service.js

@@ -110,8 +110,8 @@ async function redirect(env, url, wait = false) {
     browser.location.assign(url);
 }
 
-function getRoute() {
-    const { pathname, search, hash } = browser.location;
+function getRoute(urlObj) {
+    const { pathname, search, hash } = urlObj;
     const searchQuery = parseSearchQuery(search);
     const hashQuery = parseHash(hash);
     return { pathname, search: searchQuery, hash: hashQuery };
@@ -120,10 +120,12 @@ function getRoute() {
 function makeRouter(env) {
     const bus = env.bus;
     const lockedKeys = new Set();
-    let current = getRoute();
+    let current = getRoute(browser.location);
     let pushTimeout;
-    browser.addEventListener("hashchange", () => {
-        current = getRoute();
+    browser.addEventListener("hashchange", (ev) => {
+        browser.clearTimeout(pushTimeout);
+        const loc = new URL(ev.newURL);
+        current = getRoute(loc);
         bus.trigger("ROUTE_CHANGE");
     });
 
@@ -155,7 +157,7 @@ function makeRouter(env) {
             } else {
                 browser.history.replaceState({}, "", url);
             }
-            current = getRoute();
+            current = getRoute(browser.location);
         }
         return function pushOrReplaceState(hash, options) {
             allPushArgs.push([hash, options]);

addons/web/static/tests/setup.js

@@ -58,7 +58,7 @@ function forceLocaleAndTimezoneWithCleanup() {
     });
 }
 
-function makeMockLocation() {
+function makeMockLocation(hasListeners = () => true) {
     const locationLink = Object.assign(document.createElement("a"), {
         href: window.location.origin + window.location.pathname,
         assign(url) {
@@ -71,10 +71,22 @@ function makeMockLocation() {
             return target[p];
         },
         set(target, p, value) {
-            target[p] = value;
             if (p === "hash") {
-                window.dispatchEvent(new HashChangeEvent("hashchange"));
+                const oldURL = new URL(locationLink.href).toString();
+                target[p] = value;
+                const newURL = new URL(locationLink.href).toString();
+
+                if (!hasListeners()) {
+                    return true;
+                }
+                // the event hashchange must be triggered in a nonBlocking stack
+                // https://html.spec.whatwg.org/multipage/browsing-the-web.html#scroll-to-fragid
+                window.setTimeout(() => {
+                    window.dispatchEvent(new HashChangeEvent("hashchange", { oldURL, newURL }));
+                });
+                return true;
             }
+            target[p] = value;
             return true;
         },
     });
@@ -83,13 +95,18 @@ function makeMockLocation() {
 function patchBrowserWithCleanup() {
     const originalAddEventListener = browser.addEventListener;
     const originalRemoveEventListener = browser.removeEventListener;
-    const mockLocation = makeMockLocation();
+
+    let hasHashChangeListeners = false;
+    const mockLocation = makeMockLocation(() => hasHashChangeListeners);
     patchWithCleanup(
         browser,
         {
             // patch addEventListner to automatically remove listeners bound (via
             // browser.addEventListener) during a test (e.g. during the deployment of a service)
-            addEventListener() {
+            addEventListener(evName) {
+                if (evName === "hashchange") {
+                    hasHashChangeListeners = true;
+                }
                 originalAddEventListener(...arguments);
                 registerCleanup(() => {
                     originalRemoveEventListener(...arguments);

addons/web/static/tests/webclient/actions/load_state_tests.js

@@ -8,7 +8,7 @@ import core from "web.core";
 import AbstractAction from "web.AbstractAction";
 import { registerCleanup } from "../../helpers/cleanup";
 import { makeTestEnv } from "../../helpers/mock_env";
-import { click, getFixture, legacyExtraNextTick, patchWithCleanup } from "../../helpers/utils";
+import { click, getFixture, legacyExtraNextTick, patchWithCleanup, nextTick } from "../../helpers/utils";
 import {
     createWebClient,
     doAction,
@@ -966,4 +966,62 @@ QUnit.module("ActionManager", (hooks) => {
             menu_id: 1,
         });
     });
+
+    QUnit.test("concurrent hashchange during action mounting -- 1", async (assert) => {
+        assert.expect(5);
+
+        class MyAction extends Component {
+            mounted() {
+                browser.location.hash = "#action=__test__client__action__&menu_id=1";
+            }
+        }
+        MyAction.template = tags.xml`<div class="not-here" />`;
+        registry.category("actions").add("myAction", MyAction);
+
+        browser.location.hash = "#action=myAction";
+
+        const webClient = await createWebClient({ serverData });
+        assert.containsOnce(webClient, ".not-here");
+        assert.containsNone(webClient, ".test_client_action");
+
+        await nextTick();
+        assert.containsNone(webClient, ".not-here");
+        assert.containsOnce(webClient, ".test_client_action");
+
+        assert.deepEqual(webClient.env.services.router.current.hash, {
+            action: "__test__client__action__",
+            menu_id: 1,
+        });
+    });
+
+    QUnit.test("concurrent hashchange during action mounting -- 2", async (assert) => {
+        assert.expect(5);
+
+        const baseURL = new URL(browser.location.href).toString();
+
+        class MyAction extends Component {
+            mounted() {
+                const newURL = baseURL + "#action=__test__client__action__&menu_id=1";
+                // immediate triggering
+                window.dispatchEvent(new HashChangeEvent("hashchange", { newURL }));
+            }
+        }
+        MyAction.template = tags.xml`<div class="not-here" />`;
+        registry.category("actions").add("myAction", MyAction);
+
+        browser.location.hash = "#action=myAction";
+        const webClient = await createWebClient({ serverData });
+
+        assert.containsOnce(webClient, ".not-here");
+        assert.containsNone(webClient, ".test_client_action");
+
+        await nextTick();
+        assert.containsNone(webClient, ".not-here");
+        assert.containsOnce(webClient, ".test_client_action");
+
+        assert.deepEqual(webClient.env.services.router.current.hash, {
+            action: "__test__client__action__",
+            menu_id: 1,
+        });
+    });
 });

addons/web/static/tests/webclient/helpers.js

@@ -279,7 +279,12 @@ export async function loadState(env, state) {
         env = env.env;
     }
     env.bus.trigger("test:hashchange", state);
+    // wait the asynchronous hashchange
+    // (the event hashchange must be triggered in a nonBlocking stack)
     await nextTick();
+    // wait for the regular rendering
+    await nextTick();
+    // wait for the legacy rendering below owl layer
     await legacyExtraNextTick();
 }