[FIX] web: open menus in new tab

Before this commit, menus in navbar and clickable elements on the home
menu were not openable in a new tab through a ctrl-click or through the
middlemouse button.

In order to achieve that, we now use default behavior of <a href/> elements.

As such default behaviour would conflict with <DropdownItem/> clicks,
a global click handler is added in the WebClient constructor which stops
the click event propagation under these specific circumstances
(ctrl-click inside an <a href/> element).

Besides this work, it has been found that the <Dropdown/> root element
should not always be a <div/> as it was, but sometimes it has to be
another HTML element in order to comply with the W3C specs. E.g. a
<Dropdown/> component as a first level child of an <ul/> element must
have a <li/> root element instead of a <div/> one.

Thus, a new prop is added to the <Dropdown/> component (`tag`) allowing
the developer to choose another root element: `<Dropdown tag="'li'"/>`.

closes odoo-dev/odoo#913

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

addons/web/static/src/core/debug/debug_menu.js

@@ -69,12 +69,6 @@ export class DebugMenu extends Component {
             ev.detail.payload.callback();
         }
     }
-
-    onClickOnTagA(ev) {
-        if (!ev.ctrlKey) {
-            ev.preventDefault();
-        }
-    }
 }
 
 DebugMenu.template = "web.DebugMenu";

addons/web/static/src/core/debug/debug_menu.xml

@@ -12,8 +12,8 @@
                     <t t-if="!element.hide">
                         <t t-if="element.type == 'item'">
                             <DropdownItem payload="{ callback: element.callback }">
-                                <a t-if="element.href" t-att-href="element.href" t-esc="element.description" t-on-click="onClickOnTagA"/>
-                                <t t-else="" t-esc="element.description"/>
+                                <a t-if="element.href" t-att-href="element.href" t-esc="element.description"/>
+                                <span t-else="" t-esc="element.description"/>
                             </DropdownItem>
                         </t>
                         <div t-if="element.type == 'separator'" role="separator" class="dropdown-divider"/>

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

@@ -302,6 +302,10 @@ Dropdown.props = {
         type: Function,
         optional: true,
     },
+    tag: {
+        type: String,
+        optional: true,
+    },
     togglerClass: {
         type: String,
         optional: true,

addons/web/static/src/core/dropdown/dropdown.scss

@@ -26,6 +26,9 @@
     @extend .dropdown-item.active;
   }
 
+}
+
+.dropdown-menu .dropdown-item {
   // Using <a> tags inside a '.dropdown-item' elements  should not be necessary
   // since the class in usually applied to the <a> tag itself.
   // The following rules will handle custom layouts needs (eg '.o_user_menu').
@@ -80,6 +83,14 @@
   &_item {
     @extend .dropdown-item;
 
+    // we want the dropdownitem first child to define its own padding
+    // because it can be an <a href/>
+    padding: 0;
+    > *:first-child {
+      padding: $dropdown-item-padding-y $dropdown-item-padding-x;
+      display: flex;
+    }
+
     &.o_dropdown_active {
       @extend .dropdown-item:hover;
     }

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

@@ -2,7 +2,11 @@
 <templates xml:space="preserve">
 
   <t t-name="web.Dropdown" owl="1">
-    <div class="o_dropdown" t-on-dropdown-item-selected="onItemSelected">
+    <t
+      t-tag="props.tag or 'div'"
+      class="o_dropdown"
+      t-on-dropdown-item-selected="onItemSelected"
+    >
       <button
         t-attf-class="o_dropdown_toggler {{ props.togglerClass || '' }}"
         t-att-class="{ 'o_dropdown_active': state.open }"
@@ -17,7 +21,7 @@
       <ul t-if="state.open" class="o_dropdown_menu d-block overflow-auto" t-att-class="props.menuClass">
         <t t-slot="menu" />
       </ul>
-    </div>
+    </t>
   </t>
 
 </templates>

addons/web/static/src/core/dropdown/dropdown_item.xml

@@ -2,10 +2,13 @@
 <templates xml:space="preserve">
 
   <t t-name="web.DropdownItem" owl="1">
-    <li class="o_dropdown_item" t-on-click="onClick" t-att-title="props.title" t-att-data-hotkey="props.hotkey">
-      <span>
-        <t t-slot="default" />
-      </span>
+    <li
+      class="o_dropdown_item"
+      t-on-click="onClick"
+      t-att-title="props.title"
+      t-att-data-hotkey="props.hotkey"
+    >
+      <t t-slot="default" />
     </li>
   </t>
 

addons/web/static/src/legacy/scss/navbar.scss

@@ -93,6 +93,12 @@
     }
   }
 
+  .o_nav_entry {
+    padding: 0;
+    @extend %-main-navbar-entry-base;
+    @extend %-main-navbar-entry-padding;
+  }
+
   .o_dropdown .o_dropdown_menu, .dropdown-menu {
     margin-top: 0;
     border-top: 0;
@@ -100,7 +106,9 @@
   }
 
   .o_dropdown_menu_group_entry {
-    padding-left: $o-dropdown-hpadding * 1.5;
+    > *:first-child {
+      padding-left: $o-dropdown-hpadding * 1.5;
+    }
 
     + .o_dropdown_item:not(.o_dropdown_menu_group_entry) {
       margin-top: .8em;
@@ -150,7 +158,7 @@
     font-weight: $-backend-font-weight-bold;
 
     @include hover() {
-      background-color: transparent;
+      background: none;
     }
   }
 
@@ -158,10 +166,6 @@
     display: flex;
     grid-area: sections;
 
-    > .o_dropdown_item {
-      @extend %-main-navbar-entry-padding;
-    }
-
     .o_more_dropdown_section_group {
       padding: $-backend-entry-vpadding $-backend-entry-hpadding;
       margin-top: .8em;

addons/web/static/src/webclient/navbar/navbar.js

@@ -173,6 +173,14 @@ export class NavBar extends Component {
             this.menuService.selectMenu(menu);
         }
     }
+
+    getMenuItemHref(payload) {
+        const parts = [`menu_id=${payload.id}`];
+        if (payload.action) {
+            parts.push(`action=${payload.action.split(",")[1]}`);
+        }
+        return "#" + parts.join("&");
+    }
 }
 NavBar.template = "web.NavBar";
 NavBar.components = { MenuDropdown, MenuItem };

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

@@ -3,19 +3,25 @@
 
   <t t-name="web.NavBar" owl="1">
     <header>
-      <nav class="o_main_navbar" t-on-dropdown-item-selected="onNavBarDropdownItemSelection" data-command-category="navbar">
+      <nav
+        class="o_main_navbar"
+        t-on-dropdown-item-selected="onNavBarDropdownItemSelection"
+        data-command-category="navbar"
+      >
         <!-- Apps Menu -->
         <t t-call="web.NavBar.AppsMenu">
           <t t-set="apps" t-value="menuService.getApps()" />
         </t>
 
         <!-- App Brand -->
-        <DropdownItem
+        <a
           t-if="currentApp"
-          class="o_menu_brand"
-          payload="currentApp"
+          t-att-href="getMenuItemHref(currentApp)"
           t-esc="currentApp.name"
-          t-ref="menuBrand" />
+          class="o_menu_brand"
+          t-ref="menuBrand"
+          t-on-click="menuService.selectMenu(currentApp)"
+        />
 
         <!-- Current App Sections -->
         <t t-if="currentAppSections.length" t-call="web.NavBar.SectionsMenu">
@@ -37,11 +43,15 @@
       <i class="fa fa-th-large" />
       <t t-set-slot="menu">
         <MenuItem
-          t-foreach="apps" t-as="app" t-key="app.id"
+          t-foreach="apps"
+          t-as="app"
+          t-key="app.id"
           class="o_app"
           t-att-class="{ o_dropdown_active: menuService.getCurrentApp() === app }"
           payload="app"
-          t-esc="app.name" />
+        >
+          <a t-att-href="getMenuItemHref(app)" t-esc="app.name" />
+        </MenuItem>
       </t>
     </Dropdown>
   </t>
@@ -50,22 +60,34 @@
     <ul class="o_menu_sections" t-ref="appSubMenus" role="menu">
 
       <t t-foreach="sections" t-as="section" t-key="section.id">
-        <t t-set="sectionsVisibleCount" t-value="(sections.length - currentAppSectionsExtra.length)" />
+        <t
+          t-set="sectionsVisibleCount"
+          t-value="(sections.length - currentAppSectionsExtra.length)"
+        />
 
         <t t-if="section_index lt Math.min(10, sectionsVisibleCount)">
-          <t t-set="hotkey" t-value="((section_index + 1) % 10).toString()"/>
+          <t t-set="hotkey" t-value="((section_index + 1) % 10).toString()" />
         </t>
         <t t-else="">
-          <t t-set="hotkey" t-value="undefined"/>
+          <t t-set="hotkey" t-value="undefined" />
         </t>
 
         <t t-if="!section.childrenTree.length">
-          <MenuItem title="section.name" payload="section" hotkey="hotkey">
-            <span t-esc="section.name" t-att-data-section="section.id" />
+          <MenuItem title="section.name" class="o_nav_entry" payload="section" hotkey="hotkey">
+            <a
+              t-att-href="getMenuItemHref(section)"
+              t-att-data-section="section.id"
+              t-esc="section.name"
+            />
           </MenuItem>
         </t>
         <t t-else="">
-          <MenuDropdown hotkey="hotkey" title="section.name" xmlid="section.xmlid">
+          <MenuDropdown
+            tag="'li'"
+            hotkey="hotkey"
+            title="section.name"
+            xmlid="section.xmlid"
+          >
             <span t-esc="section.name" t-att-data-section="section.id" />
             <t t-set-slot="menu">
               <t t-call="web.NavBar.SectionsMenu.Dropdown.MenuSlot">
@@ -79,7 +101,7 @@
       <t t-if="currentAppSectionsExtra.length" t-call="web.NavBar.SectionsMenu.MoreDropdown">
         <t t-set="sections" t-value="currentAppSectionsExtra" />
         <t t-if="sectionsVisibleCount lt 10">
-          <t t-set="hotkey" t-value="(sectionsVisibleCount + 1 % 10).toString()"/>
+          <t t-set="hotkey" t-value="(sectionsVisibleCount + 1 % 10).toString()" />
         </t>
       </t>
 
@@ -88,7 +110,9 @@
 
   <t t-name="web.NavBar.SectionsMenu.Dropdown.MenuSlot" owl="1">
     <t t-foreach="items" t-as="item" t-key="item.id">
-      <MenuItem t-if="!item.childrenTree.length" payload="item" t-esc="item.name" />
+      <MenuItem t-if="!item.childrenTree.length" payload="item">
+        <a t-att-href="getMenuItemHref(item)" t-esc="item.name" />
+      </MenuItem>
 
       <t t-else="">
         <div class="o_dropdown_menu_group" t-esc="item.name" />
@@ -97,28 +121,29 @@
           t-as="subItem"
           t-key="subItem.id"
           class="o_dropdown_menu_group_entry"
-          payload="subItem"
-          t-esc="subItem.name" />
+          payload="subItem">
+          <a t-att-href="getMenuItemHref(subItem)" t-esc="subItem.name" />
+        </MenuItem>
       </t>
     </t>
   </t>
 
   <t t-name="web.NavBar.SectionsMenu.MoreDropdown" owl="1">
-    <Dropdown class="o_menu_sections_more" title="'More Menu'" hotkey="hotkey">
-      <i class="fa fa-plus"/>
+    <Dropdown tag="'li'" class="o_menu_sections_more" title="'More Menu'" hotkey="hotkey">
+      <i class="fa fa-plus" />
       <t t-set-slot="menu">
         <t t-foreach="sections" t-as="section" t-key="section.id">
 
           <t t-if="!section.childrenTree.length">
-            <MenuItem
-              class="o_more_dropdown_section"
-              payload="section"
-              t-esc="section.name" />
+            <MenuItem class="o_more_dropdown_section" payload="section">
+              <a t-att-href="getMenuItemHref(section)" t-esc="section.name" />
+            </MenuItem>
           </t>
           <t t-else="">
             <div
               class="o_more_dropdown_section o_more_dropdown_section_group bg-200 font-weight-bold"
-              t-esc="section.name" />
+              t-esc="section.name"
+            />
             <t t-call="web.NavBar.SectionsMenu.Dropdown.MenuSlot">
               <t t-set="items" t-value="section.childrenTree" />
             </t>

addons/web/static/src/webclient/user_menu/user_menu.js

@@ -47,12 +47,6 @@ export class UserMenu extends Component {
     onDropdownItemSelected(ev) {
         ev.detail.payload.callback();
     }
-
-    onClickOnTagA(ev) {
-        if (!ev.ctrlKey) {
-            ev.preventDefault();
-        }
-    }
 }
 UserMenu.template = "web.UserMenu";
 UserMenu.components = { UserMenuItem };

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

@@ -9,8 +9,8 @@
                     <t t-if="!element.hide">
                         <t t-if="element.type == 'item'">
                             <UserMenuItem payload="{ callback: element.callback, id: element.id }">
-                                <a t-if="element.href" t-att-href="element.href" t-esc="element.description" t-on-click="onClickOnTagA"/>
-                                <t t-else="" t-esc="element.description"/>
+                                <a t-if="element.href" t-att-href="element.href" t-esc="element.description"/>
+                                <span t-else="" t-esc="element.description"/>
                             </UserMenuItem>
                         </t>
                         <div t-if="element.type == 'separator'" role="separator" class="dropdown-divider"/>

addons/web/static/src/webclient/webclient.js

@@ -7,7 +7,8 @@ import { ActionContainer } from "./actions/action_container";
 import { NavBar } from "./navbar/navbar";
 import { useEffect } from "@web/core/effect_hook";
 
-const { Component } = owl;
+const { Component, hooks } = owl;
+const { useExternalListener } = hooks;
 const mainComponentRegistry = registry.category("main_components");
 
 export class WebClient extends Component {
@@ -32,6 +33,7 @@ export class WebClient extends Component {
             },
             () => []
         );
+        useExternalListener(window, "click", this.onGlobalClick, { capture: true });
     }
 
     mounted() {
@@ -81,6 +83,23 @@ export class WebClient extends Component {
             return this.menuService.selectMenu(firstApp);
         }
     }
+
+    /**
+     * @param {MouseEvent} ev
+     */
+    onGlobalClick(ev) {
+        // When a ctrl-click occurs inside an <a href/> element
+        // we let the browser do the default behavior and
+        // we do not want any other listener to execute.
+        if (
+            ev.ctrlKey &&
+            ((ev.target instanceof HTMLAnchorElement && ev.target.href) ||
+                (ev.target instanceof HTMLElement && ev.target.closest("a[href]:not([href=''])")))
+        ) {
+            ev.stopImmediatePropagation();
+            return;
+        }
+    }
 }
 WebClient.components = { ActionContainer, NavBar };
 WebClient.template = "web.WebClient";

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

@@ -30,10 +30,33 @@ QUnit.module("Components", (hooks) => {
         class Parent extends owl.Component {}
         Parent.template = owl.tags.xml`<Dropdown/>`;
         parent = await mount(Parent, { env, target });
+        assert.strictEqual(
+            parent.el.outerHTML,
+            '<div class="o_dropdown"><button class="o_dropdown_toggler "><span></span></button></div>'
+        );
         assert.containsOnce(parent.el, "button.o_dropdown_toggler");
         assert.containsNone(parent.el, "ul.o_dropdown_menu");
     });
 
+    QUnit.test("can be rendered (custom tag)", async (assert) => {
+        class Parent extends owl.Component {}
+        Parent.template = owl.tags.xml`<Dropdown tag="'ged'"/>`;
+        parent = await mount(Parent, { env, target });
+        assert.strictEqual(
+            parent.el.outerHTML,
+            '<ged class="o_dropdown"><button class="o_dropdown_toggler "><span></span></button></ged>'
+        );
+        assert.containsOnce(parent.el, "button.o_dropdown_toggler");
+        assert.containsNone(parent.el, "ul.o_dropdown_menu");
+    });
+
+    QUnit.test("DropdownItem can be rendered", async (assert) => {
+        class Parent extends owl.Component {}
+        Parent.template = owl.tags.xml`<DropdownItem>coucou</DropdownItem>`;
+        parent = await mount(Parent, { env, target });
+        assert.strictEqual(parent.el.outerHTML, '<li class="o_dropdown_item">coucou</li>');
+    });
+
     QUnit.test("can be styled", async (assert) => {
         class Parent extends owl.Component {}
         Parent.template = owl.tags.xml`

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

@@ -12,7 +12,7 @@ import { menuService } from "@web/webclient/menu_service";
 import { WebClient } from "@web/webclient/webclient";
 import { clearRegistryWithCleanup, makeTestEnv } from "../helpers/mock_env";
 import { fakeTitleService } from "../helpers/mock_services";
-import { getFixture, patchWithCleanup } from "../helpers/utils";
+import { getFixture, patchWithCleanup, triggerEvent } from "../helpers/utils";
 
 const { Component, tags, mount } = owl;
 const { xml } = tags;
@@ -43,6 +43,7 @@ QUnit.test("can be rendered", async (assert) => {
     const target = getFixture();
     const webClient = await mount(WebClient, { env, target });
     assert.containsOnce(webClient.el, "header > nav.o_main_navbar");
+    webClient.destroy();
 });
 
 QUnit.test("can render a main component", async (assert) => {
@@ -55,6 +56,7 @@ QUnit.test("can render a main component", async (assert) => {
     const target = getFixture();
     const webClient = await mount(WebClient, { env, target });
     assert.containsOnce(webClient.el, ".chocolate");
+    webClient.destroy();
 });
 
 QUnit.test("webclient for the superuser", async (assert) => {
@@ -64,6 +66,7 @@ QUnit.test("webclient for the superuser", async (assert) => {
     const target = getFixture();
     const webClient = await mount(WebClient, { env, target });
     assert.hasClass(webClient.el, "o_is_superuser");
+    webClient.destroy();
 });
 
 QUnit.test("webclient for a non superuser", async (assert) => {
@@ -73,4 +76,55 @@ QUnit.test("webclient for a non superuser", async (assert) => {
     const target = getFixture();
     const webClient = await mount(WebClient, { env, target });
     assert.doesNotHaveClass(webClient.el, "o_is_superuser");
+    webClient.destroy();
+});
+
+QUnit.test("control-click propagation stopped on <a href/>", async (assert) => {
+    assert.expect(8);
+
+    patchWithCleanup(WebClient.prototype, {
+        /** @param {MouseEvent} ev */
+        onGlobalClick(ev) {
+            this._super(ev);
+            if (ev.ctrlKey) {
+                assert.ok(
+                    ev.defaultPrevented === false,
+                    "the global click should not prevent the default behavior on ctrl-click an <a href/>"
+                );
+                // Necessary in order to prevent the test browser to open in new tab on ctrl-click
+                ev.preventDefault();
+            }
+        },
+    });
+
+    class MyComponent extends Component {
+        /** @param {MouseEvent} ev */
+        onclick(ev) {
+            assert.step(ev.ctrlKey ? "ctrl-click" : "click");
+            // Necessary in order to prevent the test browser to open in new tab on ctrl-click
+            ev.preventDefault();
+        }
+    }
+    MyComponent.template = xml`<a href="#" class="MyComponent" t-on-click="onclick" />`;
+    const target = getFixture();
+    let env = await makeTestEnv(baseConfig);
+
+    // Mount the component as standalone and control-click the <a href/>
+    const standaloneComponent = await mount(MyComponent, { env, target });
+    assert.verifySteps([]);
+    await triggerEvent(standaloneComponent.el, "", "click", { ctrlKey: false });
+    await triggerEvent(standaloneComponent.el, "", "click", { ctrlKey: true });
+    assert.verifySteps(["click", "ctrl-click"]);
+    standaloneComponent.destroy();
+
+    // Register the component as a main one, mount the webclient and control-click the <a href/>
+    clearRegistryWithCleanup(mainComponentRegistry);
+    mainComponentRegistry.add("mycomponent", { Component: MyComponent });
+    env = await makeTestEnv(baseConfig);
+    const webClient = await mount(WebClient, { env, target });
+    assert.verifySteps([]);
+    await triggerEvent(webClient.el, ".MyComponent", "click", { ctrlKey: false });
+    await triggerEvent(webClient.el, ".MyComponent", "click", { ctrlKey: true });
+    assert.verifySteps(["click"]);
+    webClient.destroy();
 });

odoo/addons/test_main_flows/static/tests/tours/main_flow.js

@@ -17,7 +17,7 @@ tour.stepUtils.openBuggerMenu("li.breadcrumb-item.active:contains('Quotations')"
     content: _t("Let's create products."),
     position: "bottom",
 }, {
-    trigger: ".o_menu_sections .o_dropdown_item span:contains('Products')",
+    trigger: ".o_menu_sections .o_dropdown_item:contains('Products')",
     content: _t("Let's create products."),
     position: "bottom"
 }, {