From 23da5621f3653cf6603c12b030faa2bcb76f4024 Mon Sep 17 00:00:00 2001 From: "Lucas Perais (lpe)" Date: Mon, 28 Jun 2021 13:17:43 +0000 Subject: [PATCH] [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) --- .../static/src/core/browser/router_service.js | 14 +++-- addons/web/static/tests/setup.js | 27 +++++++-- .../webclient/actions/load_state_tests.js | 60 ++++++++++++++++++- addons/web/static/tests/webclient/helpers.js | 5 ++ 4 files changed, 94 insertions(+), 12 deletions(-) diff --git a/addons/web/static/src/core/browser/router_service.js b/addons/web/static/src/core/browser/router_service.js index 9b722e9d839..b29f6078c58 100644 --- a/addons/web/static/src/core/browser/router_service.js +++ b/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]); diff --git a/addons/web/static/tests/setup.js b/addons/web/static/tests/setup.js index 8f8fa75a6c8..4d69924bcfe 100644 --- a/addons/web/static/tests/setup.js +++ b/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); diff --git a/addons/web/static/tests/webclient/actions/load_state_tests.js b/addons/web/static/tests/webclient/actions/load_state_tests.js index a1f007f5c57..f20a847ed84 100644 --- a/addons/web/static/tests/webclient/actions/load_state_tests.js +++ b/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`
`; + 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`
`; + 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, + }); + }); }); diff --git a/addons/web/static/tests/webclient/helpers.js b/addons/web/static/tests/webclient/helpers.js index 58272c21e04..b192cee6b45 100644 --- a/addons/web/static/tests/webclient/helpers.js +++ b/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(); }