[IMP] web: remove the Portal from dialog

In this commit, we simplify the dialog template by removing the Portal.
The Portal is no longer used, as now we always use the dialog service to
open a new dialog.

also:

- remove the dialog container class from the webclient;
- add the dialog container class to the dialog container;
- move the management of the inactive modal class to the dialog container;
- remove the dialog manager class from the dialog container;

closes odoo-dev/odoo#947

Signed-off-by: Géry Debongnie (ged) <ged@openerp.com>

addons/web/static/src/core/dialog/dialog.js

@@ -4,29 +4,23 @@ import { useHotkey } from "../hotkey_hook";
 import { useService } from "../service_hook";
 import { useActiveElement } from "../ui/ui_service";
 
-const { Component, hooks, misc } = owl;
+const { Component, hooks } = owl;
 const { useRef, useSubEnv } = hooks;
-const { Portal } = misc;
 
 export class Dialog extends Component {
-    constructor(...args) {
-        super(...args);
+    setup() {
         if (this.constructor === Dialog) {
             throw new Error(
                 "Dialog should not be used by itself. Please use the dialog service with a Dialog subclass."
             );
         }
-    }
-    setup() {
         this.modalRef = useRef("modal");
         this.dialogService = useService("dialog");
         useActiveElement("modal");
         useHotkey(
             "escape",
             () => {
-                if (!this.modalRef.el.classList.contains("o_inactive_modal")) {
-                    this.close();
-                }
+                this.close();
             },
             { altIsOptional: true }
         );
@@ -42,30 +36,6 @@ export class Dialog extends Component {
         this.__id = null;
     }
 
-    mounted() {
-        const dialogContainer = document.querySelector(".o_dialog_container");
-        const modals = dialogContainer.querySelectorAll(".o_dialog .modal");
-        const len = modals.length;
-        for (let i = 0; i < len - 1; i++) {
-            const modal = modals[i];
-            modal.classList.add("o_inactive_modal");
-        }
-        dialogContainer.classList.add("modal-open");
-    }
-
-    willUnmount() {
-        const dialogContainer = document.querySelector(".o_dialog_container");
-        const modals = dialogContainer.querySelectorAll(".o_dialog .modal");
-        const len = modals.length;
-        if (len >= 2) {
-            const modal = this.modalRef.el === modals[len - 1] ? modals[len - 2] : modals[len - 1];
-            modal.focus();
-            modal.classList.remove("o_inactive_modal");
-        } else {
-            dialogContainer.classList.remove("modal-open");
-        }
-    }
-
     /**
      * Send an event signaling that the dialog should be closed.
      * @private
@@ -75,7 +45,6 @@ export class Dialog extends Component {
     }
 }
 
-Dialog.components = { Portal };
 Dialog.template = "web.Dialog";
 Dialog.contentClass = null;
 Dialog.fullscreen = false;

addons/web/static/src/core/dialog/dialog.xml

@@ -2,32 +2,30 @@
 <templates xml:space="preserve">
 
     <t t-name="web.Dialog" owl="1">
-        <Portal target="'.o_dialog_container'">
-            <div class="o_dialog">
-                <div role="dialog" class="modal"
-                    tabindex="-1"
-                    t-att-class="{ o_technical_modal: technical, o_modal_full: fullscreen }"
-                    t-ref="modal"
-                    >
-                    <div class="modal-dialog" t-att-class="size">
-                        <div class="modal-content" t-att-class="contentClass">
-                            <header t-if="renderHeader" class="modal-header">
-                                <h4 class="modal-title">
-                                    <t t-esc="title"/>
-                                </h4>
-                                <button type="button" class="close" aria-label="Close" tabindex="-1" t-on-click="close">×</button>
-                            </header>
-                            <main class="modal-body">
-                                <t t-call="{{ constructor.bodyTemplate }}"/>
-                            </main>
-                            <footer t-if="renderFooter" class="modal-footer">
-                                <t t-call="{{ constructor.footerTemplate }}" />
-                            </footer>
-                        </div>
+        <div class="o_dialog">
+            <div role="dialog" class="modal"
+                tabindex="-1"
+                t-att-class="{ o_technical_modal: technical, o_modal_full: fullscreen }"
+                t-ref="modal"
+                >
+                <div class="modal-dialog" t-att-class="size">
+                    <div class="modal-content" t-att-class="contentClass">
+                        <header t-if="renderHeader" class="modal-header">
+                            <h4 class="modal-title">
+                                <t t-esc="title"/>
+                            </h4>
+                            <button type="button" class="close" aria-label="Close" tabindex="-1" t-on-click="close">×</button>
+                        </header>
+                        <main class="modal-body">
+                            <t t-call="{{ constructor.bodyTemplate }}"/>
+                        </main>
+                        <footer t-if="renderFooter" class="modal-footer">
+                            <t t-call="{{ constructor.footerTemplate }}" />
+                        </footer>
                     </div>
                 </div>
             </div>
-        </Portal>
+        </div>
     </t>
 
     <t t-name="web.DialogFooterDefault" owl="1">

addons/web/static/src/core/dialog/dialog_container.js

@@ -51,9 +51,10 @@ export class DialogContainer extends Component {
 }
 DialogContainer.components = { ErrorHandler };
 DialogContainer.template = tags.xml`
-    <div class="o_dialog_manager">
+    <div class="o_dialog_container" t-att-class="{'modal-open': Object.keys(dialogs).length > 0}">
       <t t-foreach="Object.values(dialogs)" t-as="dialog" t-key="dialog.id">
-        <ErrorHandler dialog="dialog" t-on-dialog-closed="onDialogClosed(dialog.id)" callback="errorCallBack(dialog.id)" />
+        <ErrorHandler dialog="dialog" t-on-dialog-closed="onDialogClosed(dialog.id)" callback="errorCallBack(dialog.id)"
+            t-att-class="{o_inactive_modal: !dialog_last}"/>
       </t>
     </div>
     `;

addons/web/static/src/webclient/webclient.xml

@@ -5,7 +5,6 @@
         <body class="o_web_client" t-att-class="{'o_is_superuser': user.userId === 1}">
             <NavBar/>
             <ActionContainer/>
-            <div class="o_dialog_container"/>
             <div>
                 <t t-foreach="Components" t-as="C" t-key="C[0]">
                     <t t-component="C[1].Component" t-props="C[1].props"/>

addons/web/static/tests/core/debug/debug_manager_tests.js

@@ -122,16 +122,12 @@ QUnit.module("DebugMenu", (hooks) => {
     });
 
     QUnit.test("Don't display the DebugMenu if debug mode is disabled", async (assert) => {
-        const dialogContainer = document.createElement("div");
-        dialogContainer.classList.add("o_dialog_container");
-        target.append(dialogContainer);
         const env = await makeTestEnv(testConfig);
         const actionDialog = await mount(ActionDialog, { env, target, props: {} });
         registerCleanup(() => {
             actionDialog.destroy();
-            target.querySelector(".o_dialog_container").remove();
         });
-        assert.containsOnce(target, "div.o_dialog_container .o_dialog");
+        assert.containsOnce(target, ".o_dialog");
         assert.containsNone(target, ".o_dialog .o_debug_manager .fa-bug");
     });
 
@@ -139,9 +135,6 @@ QUnit.module("DebugMenu", (hooks) => {
         "Display the DebugMenu correctly in a ActionDialog if debug mode is enabled",
         async (assert) => {
             assert.expect(8);
-            const dialogContainer = document.createElement("div");
-            dialogContainer.classList.add("o_dialog_container");
-            target.append(dialogContainer);
             debugRegistry.add("global", () => {
                 return {
                     type: "item",
@@ -188,12 +181,11 @@ QUnit.module("DebugMenu", (hooks) => {
             const actionDialog = await mount(Parent, { env, target });
             registerCleanup(() => {
                 actionDialog.destroy();
-                target.querySelector(".o_dialog_container").remove();
             });
-            assert.containsOnce(target, "div.o_dialog_container .o_dialog");
+            assert.containsOnce(target, ".o_dialog");
             assert.containsOnce(target, ".o_dialog .o_debug_manager .fa-bug");
             await click(target, ".o_dialog .o_debug_manager button");
-            const debugManagerEl = target.querySelector(".o_dialog_container .o_debug_manager");
+            const debugManagerEl = target.querySelector(".o_debug_manager");
             assert.containsN(debugManagerEl, "ul.o_dropdown_menu li.o_dropdown_item", 2);
             // Check that global debugManager elements are not displayed (global_1)
             const items =

addons/web/static/tests/core/dialog_service_tests.js

@@ -28,7 +28,6 @@ class PseudoWebClient extends Component {
 }
 PseudoWebClient.template = tags.xml`
         <div>
-            <div class="o_dialog_container"/>
             <div>
                 <t t-foreach="Components" t-as="C" t-key="C[0]">
                     <t t-component="C[1].Component" t-props="C[1].props"/>
@@ -53,45 +52,35 @@ QUnit.module("DialogManager", {
     },
 });
 QUnit.test("Simple rendering with a single dialog", async (assert) => {
-    assert.expect(9);
+    assert.expect(4);
     class CustomDialog extends Dialog {}
     CustomDialog.title = "Welcome";
     pseudoWebClient = await mount(PseudoWebClient, { env, target });
-    assert.containsOnce(target, ".o_dialog_manager");
-    assert.containsNone(target, ".o_dialog_manager portal");
     assert.containsNone(target, ".o_dialog_container .o_dialog");
     env.services.dialog.open(CustomDialog);
     await nextTick();
-    assert.containsOnce(target, ".o_dialog_manager portal");
     assert.containsOnce(target, ".o_dialog_container .o_dialog");
     assert.strictEqual(target.querySelector("header .modal-title").textContent, "Welcome");
     await click(target.querySelector(".o_dialog_container .o_dialog footer button"));
-    assert.containsOnce(target, ".o_dialog_manager");
-    assert.containsNone(target, ".o_dialog_manager portal");
     assert.containsNone(target, ".o_dialog_container .o_dialog");
 });
 
 QUnit.test("Simple rendering and close a single dialog", async (assert) => {
-    assert.expect(9);
+    assert.expect(4);
 
     class CustomDialog extends Dialog {}
     CustomDialog.title = "Welcome";
 
     pseudoWebClient = await mount(PseudoWebClient, { env, target });
-    assert.containsOnce(target, ".o_dialog_manager");
-    assert.containsNone(target, ".o_dialog_manager portal");
     assert.containsNone(target, ".o_dialog_container .o_dialog");
 
     const dialogId = env.services.dialog.open(CustomDialog);
     await nextTick();
-    assert.containsOnce(target, ".o_dialog_manager portal");
     assert.containsOnce(target, ".o_dialog_container .o_dialog");
     assert.strictEqual(target.querySelector("header .modal-title").textContent, "Welcome");
 
     env.services.dialog.close(dialogId);
     await nextTick();
-    assert.containsOnce(target, ".o_dialog_manager");
-    assert.containsNone(target, ".o_dialog_manager portal");
     assert.containsNone(target, ".o_dialog_container .o_dialog");
 
     // Call a second time, the close on the dialog.
@@ -101,7 +90,7 @@ QUnit.test("Simple rendering and close a single dialog", async (assert) => {
 });
 
 QUnit.test("rendering with two dialogs", async (assert) => {
-    assert.expect(12);
+    assert.expect(7);
     class CustomDialog extends Dialog {
         setup() {
             super.setup();
@@ -109,24 +98,19 @@ QUnit.test("rendering with two dialogs", async (assert) => {
         }
     }
     pseudoWebClient = await mount(PseudoWebClient, { env, target });
-    assert.containsOnce(target, ".o_dialog_manager");
-    assert.containsNone(target, ".o_dialog_manager portal");
     assert.containsNone(target, ".o_dialog_container .o_dialog");
     env.services.dialog.open(CustomDialog, { title: "Hello" });
     await nextTick();
-    assert.containsOnce(target, ".o_dialog_manager portal");
     assert.containsOnce(target, ".o_dialog_container .o_dialog");
     assert.strictEqual(target.querySelector("header .modal-title").textContent, "Hello");
     env.services.dialog.open(CustomDialog, { title: "Sauron" });
     await nextTick();
-    assert.containsN(target, ".o_dialog_manager portal", 2);
     assert.containsN(target, ".o_dialog_container .o_dialog", 2);
     assert.deepEqual(
         [...target.querySelectorAll("header .modal-title")].map((el) => el.textContent),
         ["Hello", "Sauron"]
     );
     await click(target.querySelector(".o_dialog_container .o_dialog footer button"));
-    assert.containsOnce(target, ".o_dialog_manager portal");
     assert.containsOnce(target, ".o_dialog_container .o_dialog");
     assert.strictEqual(target.querySelector("header .modal-title").textContent, "Sauron");
 });
@@ -166,6 +150,65 @@ QUnit.test("multiple dialogs can become the UI active element", async (assert) =
     assert.strictEqual(dialogModal, env.services.ui.activeElement);
 });
 
+QUnit.test("Interactions between multiple dialogs", async (assert) => {
+    assert.expect(14);
+    function activity(modals) {
+        const active = [];
+        const names = [];
+        for (let i = 0; i < modals.length; i++) {
+            active[i] = !modals[i].classList.contains("o_inactive_modal");
+            names[i] = modals[i].querySelector(".modal-title").textContent;
+        }
+        return { active, names };
+    }
+
+    class CustomDialog extends Dialog {
+        setup() {
+            super.setup();
+            this.title = this.props.title;
+        }
+    }
+    pseudoWebClient = await mount(PseudoWebClient, { env, target });
+
+    env.services.dialog.open(CustomDialog, { title: "Hello" });
+    await nextTick();
+    env.services.dialog.open(CustomDialog, { title: "Sauron" });
+    await nextTick();
+    env.services.dialog.open(CustomDialog, { title: "Rafiki" });
+    await nextTick();
+
+    let modals = document.querySelectorAll(".modal");
+    assert.containsN(target, ".o_dialog", 3);
+    let res = activity(modals);
+    assert.deepEqual(res.active, [false, false, true]);
+    assert.deepEqual(res.names, ["Hello", "Sauron", "Rafiki"]);
+    assert.hasClass(target.querySelector(".o_dialog_container"), "modal-open");
+
+    let lastDialog = modals[modals.length - 1];
+    lastDialog.dispatchEvent(new KeyboardEvent("keydown", { bubbles: true, key: "Escape" }));
+    await nextTick();
+    modals = document.querySelectorAll(".modal");
+    assert.containsN(target, ".o_dialog", 2);
+    res = activity(modals);
+    assert.deepEqual(res.active, [false, true]);
+    assert.deepEqual(res.names, ["Hello", "Sauron"]);
+    assert.hasClass(target.querySelector(".o_dialog_container"), "modal-open");
+
+    lastDialog = modals[modals.length - 1];
+    await click(lastDialog, "footer button");
+    modals = document.querySelectorAll(".modal");
+    assert.containsN(target, ".o_dialog", 1);
+    res = activity(modals);
+    assert.deepEqual(res.active, [true]);
+    assert.deepEqual(res.names, ["Hello"]);
+    assert.hasClass(target.querySelector(".o_dialog_container"), "modal-open");
+
+    lastDialog = modals[modals.length - 1];
+    await click(lastDialog, "footer button");
+    assert.containsNone(target, ".o_dialog_container .modal");
+    assert.containsOnce(target, ".o_dialog_container");
+});
+
 QUnit.test("dialog component crashes", async (assert) => {
     assert.expect(4);
 

addons/web/static/tests/core/dialog_tests.js

@@ -35,9 +35,6 @@ SimpleDialog.bodyTemplate = owl.tags.xml`<t t-slot="default"/>`;
 QUnit.module("Components", (hooks) => {
     hooks.beforeEach(async () => {
         target = getFixture();
-        const dialogContainer = document.createElement("div");
-        dialogContainer.classList.add("o_dialog_container");
-        target.append(dialogContainer);
         serviceRegistry.add("hotkey", hotkeyService);
         serviceRegistry.add("ui", uiService);
         serviceRegistry.add("dialog", makeFakeDialogService());
@@ -63,7 +60,7 @@ QUnit.module("Components", (hooks) => {
 
         const env = await makeTestEnv();
         parent = await mount(Parent, { env, target, props: {} });
-        assert.containsOnce(target, "div.o_dialog_container .o_dialog");
+        assert.containsOnce(target, ".o_dialog");
         assert.containsOnce(
             target,
             ".o_dialog header .modal-title",
@@ -103,11 +100,11 @@ QUnit.module("Components", (hooks) => {
         assert.containsN(target, ".o_dialog", 2);
         assert.deepEqual(
             [...target.querySelectorAll("header .modal-title")].map((el) => el.textContent),
-            ["Second Title", "First Title"] // mounted is called in reverse order
+            ["First Title", "Second Title"]
         );
         assert.deepEqual(
             [...target.querySelectorAll(".o_dialog .modal-body")].map((el) => el.textContent),
-            [" Hello again! ", " Hello! "] // mounted is called in reverse order
+            [" Hello! ", " Hello again! "]
         );
     });
 
@@ -297,79 +294,6 @@ QUnit.module("Components", (hooks) => {
         assert.hasClass(target.querySelector(".o_dialog .modal"), "o_modal_full");
     });
 
-    QUnit.test("Interactions between multiple dialogs", async function (assert) {
-        assert.expect(14);
-        serviceRegistry.add(
-            "dialog",
-            makeFakeDialogService({
-                close: (id) => {
-                    assert.step("close with id: " + id);
-                    delete parent.dialogIds[id];
-                },
-            }),
-            { force: true }
-        );
-        const env = await makeTestEnv();
-        let id = 0;
-        class MyDialog extends SimpleDialog {
-            setup() {
-                super.setup();
-                this.__id = id;
-            }
-        }
-        class Parent extends owl.Component {
-            constructor() {
-                super(...arguments);
-                this.dialogIds = useState({});
-            }
-        }
-        Parent.template = owl.tags.xml`
-              <div>
-                <MyDialog t-foreach="Object.keys(dialogIds)" t-as="dialogId" t-key="dialogId"/>
-              </div>
-          `;
-        Parent.components = { MyDialog };
-        const parent = await mount(Parent, { env, target });
-        parent.dialogIds[0] = id;
-        await nextTick();
-        id++;
-        parent.dialogIds[1] = id;
-        await nextTick();
-        id++;
-        parent.dialogIds[2] = id;
-        await nextTick();
-        function activity(modals) {
-            const res = [];
-            for (let i = 0; i < modals.length; i++) {
-                res[i] = !modals[i].classList.contains("o_inactive_modal");
-            }
-            return res;
-        }
-        let modals = document.querySelectorAll(".modal");
-        assert.containsN(target, ".o_dialog", 3);
-        assert.deepEqual(activity(modals), [false, false, true]);
-        assert.hasClass(target.querySelector(".o_dialog_container"), "modal-open");
-        let lastDialog = modals[modals.length - 1];
-        lastDialog.dispatchEvent(new KeyboardEvent("keydown", { bubbles: true, key: "Escape" }));
-        await nextTick();
-        await nextTick();
-        modals = document.querySelectorAll(".modal");
-        assert.containsN(target, ".o_dialog", 2);
-        assert.deepEqual(activity(modals), [false, true]);
-        assert.hasClass(target.querySelector(".o_dialog_container"), "modal-open");
-        lastDialog = modals[modals.length - 1];
-        await click(lastDialog, "footer button");
-        modals = document.querySelectorAll(".modal");
-        assert.containsN(target, ".o_dialog", 1);
-        assert.deepEqual(activity(modals), [true]);
-        assert.hasClass(target.querySelector(".o_dialog_container"), "modal-open");
-        parent.unmount();
-        // dialog 0 is closed through the removal of its parent => no callback
-        assert.containsNone(target, ".o_dialog_container .modal");
-        assert.doesNotHaveClass(target.querySelector(".o_dialog_container"), "modal-open");
-        assert.verifySteps(["close with id: 2", "close with id: 1"]);
-    });
-
     QUnit.test("can be the UI active element", async function (assert) {
         assert.expect(3);
         class Parent extends owl.Component {

addons/web/static/tests/core/errors/error_dialogs_tests.js

@@ -25,9 +25,6 @@ const serviceRegistry = registry.category("services");
 QUnit.module("Error dialogs", {
     async beforeEach() {
         target = getFixture();
-        const dialogContainer = document.createElement("div");
-        dialogContainer.classList.add("o_dialog_container");
-        target.append(dialogContainer);
         serviceRegistry.add("ui", uiService);
         serviceRegistry.add("hotkey", hotkeyService);
         serviceRegistry.add("localization", makeFakeLocalizationService());
@@ -54,7 +51,7 @@ QUnit.test("ErrorDialog with traceback", async (assert) => {
     assert.containsNone(target, ".o_dialog");
     env = await makeTestEnv();
     parent = await mount(Parent, { env, target });
-    assert.containsOnce(target, "div.o_dialog_container .o_dialog");
+    assert.containsOnce(target, ".o_dialog");
     assert.strictEqual(target.querySelector("header .modal-title").textContent, "Odoo Error");
     const mainButtons = target.querySelectorAll("main button");
     assert.deepEqual(
@@ -106,7 +103,7 @@ QUnit.test("Client ErrorDialog with traceback", async (assert) => {
     assert.containsNone(target, ".o_dialog");
     env = await makeTestEnv();
     parent = await mount(Parent, { env, target });
-    assert.containsOnce(target, "div.o_dialog_container .o_dialog");
+    assert.containsOnce(target, ".o_dialog");
     assert.strictEqual(
         target.querySelector("header .modal-title").textContent,
         "Odoo Client Error"
@@ -196,7 +193,7 @@ QUnit.test("WarningDialog", async (assert) => {
     assert.containsNone(target, ".o_dialog");
     env = await makeTestEnv();
     parent = await mount(Parent, { env, target });
-    assert.containsOnce(target, "div.o_dialog_container .o_dialog");
+    assert.containsOnce(target, ".o_dialog");
     assert.strictEqual(target.querySelector("header .modal-title").textContent, "User Error");
     assert.containsOnce(target, "main .o_dialog_warning");
     assert.strictEqual(target.querySelector("main").textContent, "Some strange unreadable message");
@@ -242,7 +239,7 @@ QUnit.test("RedirectWarningDialog", async (assert) => {
     env = await makeTestEnv();
     assert.containsNone(target, ".o_dialog");
     parent = await mount(Parent, { env, target });
-    assert.containsOnce(target, "div.o_dialog_container .o_dialog");
+    assert.containsOnce(target, ".o_dialog");
     assert.strictEqual(target.querySelector("header .modal-title").textContent, "Odoo Warning");
     assert.strictEqual(target.querySelector("main").textContent, "Some strange unreadable message");
     let footerButtons = target.querySelectorAll("footer button");
@@ -265,7 +262,7 @@ QUnit.test("Error504Dialog", async (assert) => {
     assert.containsNone(target, ".o_dialog");
     env = await makeTestEnv();
     parent = await mount(Parent, { env, target });
-    assert.containsOnce(target, "div.o_dialog_container .o_dialog");
+    assert.containsOnce(target, ".o_dialog");
     assert.strictEqual(target.querySelector("header .modal-title").textContent, "Request timeout");
     assert.strictEqual(
         target.querySelector("main p").textContent,
@@ -289,7 +286,7 @@ QUnit.test("SessionExpiredDialog", async (assert) => {
     env = await makeTestEnv();
     assert.containsNone(target, ".o_dialog");
     parent = await mount(Parent, { env, target });
-    assert.containsOnce(target, "div.o_dialog_container .o_dialog");
+    assert.containsOnce(target, ".o_dialog");
     assert.strictEqual(
         target.querySelector("header .modal-title").textContent,
         "Odoo Session Expired"