[FIX] menu: use capture for events

Before this, the menu were closed by a listener on the `onClick` of window.
This caused 2 main problems :

- on some places, we stopped the event propagation, which prevented the
  menu from closing. This was the case for the topbar composer for example.

- on buttons that opened a menu we needed to add a `.stop`, because if
another menu was already open the menu would instantly close after
changing its props.

Solved this by having the externalListener of menus listen to the `capture`
phase of events. This bypass all the `.stop`, and assures that the old
menu is closed before the new one is opened for click events.

The way it is currently handled should probably be improved when we
implement a proper menu service (`querySelectorAll` on the menus and
we need to customize event payload for menu toggle to work).

Odoo task 3145756

closes #2038

X-original-commit: 3354055
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
Signed-off-by: Minne Adrien (adrm) <adrm@odoo.com>
Co-authored-by: jash <jash@odoo.com>

src/components/bottom_bar/bottom_bar.ts

@@ -1,7 +1,7 @@
 import { Component, onWillUpdateProps, useRef, useState } from "@odoo/owl";
 import { BACKGROUND_GRAY_COLOR, HEADER_WIDTH } from "../../constants";
 import { MenuItemRegistry } from "../../registries/menu_items_registry";
-import { Pixel, SpreadsheetChildEnv } from "../../types";
+import { MenuMouseEvent, Pixel, SpreadsheetChildEnv, UID } from "../../types";
 import { Ripple } from "../animation/ripple";
 import { BottomBarSheet } from "../bottom_bar_sheet/bottom_bar_sheet";
 import { BottomBarStatistic } from "../bottom_bar_statistic/bottom_bar_statistic";
@@ -77,7 +77,7 @@ interface Props {
 }
 
 interface BottomBarMenuState extends MenuState {
-  menuId?: string;
+  menuId: UID | undefined;
 }
 
 export class BottomBar extends Component<Props, SpreadsheetChildEnv> {
@@ -154,14 +154,14 @@ export class BottomBar extends Component<Props, SpreadsheetChildEnv> {
     this.menuState.position = { x, y };
   }
 
-  onSheetContextMenu(sheet: string, registry: MenuItemRegistry, ev: MouseEvent) {
+  onSheetContextMenu(sheetId: UID, registry: MenuItemRegistry, ev: MenuMouseEvent) {
     const target = ev.currentTarget as HTMLElement;
     const { top, left } = target.getBoundingClientRect();
-    if (this.menuState.isOpen && this.menuState.menuId === sheet) {
+    if (ev.closedMenuId === sheetId) {
       this.closeMenu();
       return;
     }
-    this.openContextMenu(left, top, sheet, registry);
+    this.openContextMenu(left, top, sheetId, registry);
   }
 
   closeMenu() {

src/components/bottom_bar/bottom_bar.xml

@@ -85,6 +85,7 @@
         menuItems="menuState.menuItems"
         maxHeight="menuMaxHeight"
         onClose="() => this.closeMenu()"
+        menuId="menuState.menuId"
       />
     </div>
   </t>

src/components/composer/composer/composer.xml

@@ -14,7 +14,7 @@
         t-on-mousewheel.stop=""
         t-on-input="onInput"
         t-on-keyup="onKeyup"
-        t-on-click.stop="onClick"
+        t-on-click="onClick"
         t-on-blur="onBlur"
         t-on-paste.stop=""
         t-on-compositionstart="onCompositionStart"

src/components/helpers/dom_helpers.ts

@@ -46,3 +46,7 @@ export function setElementScrollTop(el: HTMLElement | null, scroll: number) {
   if (!el) return;
   el.scrollTop = scroll;
 }
+
+export function getOpenedMenus(): HTMLElement[] {
+  return Array.from(document.querySelectorAll<HTMLElement>(".o-spreadsheet .o-menu"));
+}

src/components/menu/menu.ts

@@ -7,9 +7,9 @@ import {
   MENU_WIDTH,
 } from "../../constants";
 import { MenuItem } from "../../registries/menu_items_registry";
-import { DOMCoordinates, Pixel, SpreadsheetChildEnv } from "../../types";
+import { DOMCoordinates, MenuMouseEvent, Pixel, SpreadsheetChildEnv, UID } from "../../types";
 import { css } from "../helpers/css";
-import { isChildEvent } from "../helpers/dom_helpers";
+import { getOpenedMenus, isChildEvent } from "../helpers/dom_helpers";
 import { useAbsolutePosition } from "../helpers/position_hook";
 import { Popover, PopoverProps } from "../popover/popover";
 
@@ -76,6 +76,7 @@ interface Props {
   maxHeight?: Pixel;
   onClose: () => void;
   onMenuClicked?: (ev: CustomEvent) => void;
+  menuId?: UID;
 }
 
 export interface MenuState {
@@ -102,8 +103,8 @@ export class Menu extends Component<Props, SpreadsheetChildEnv> {
   private position = useAbsolutePosition(this.menuRef);
 
   setup() {
-    useExternalListener(window, "click", this.onClick);
-    useExternalListener(window, "contextmenu", this.onContextMenu);
+    useExternalListener(window, "click", this.onExternalClick, { capture: true });
+    useExternalListener(window, "contextmenu", this.onExternalClick, { capture: true });
     onWillUpdateProps((nextProps: Props) => {
       if (nextProps.menuItems !== this.props.menuItems) {
         this.closeSubMenu();
@@ -176,24 +177,16 @@ export class Menu extends Component<Props, SpreadsheetChildEnv> {
     return this.position.y + this.getMenuItemsHeight(menusAbove) + MENU_VERTICAL_PADDING;
   }
 
-  private onClick(ev: MouseEvent) {
+  private onExternalClick(ev: MenuMouseEvent) {
     // Don't close a root menu when clicked to open the submenus.
     const el = this.menuRef.el;
-    if (el && isChildEvent(el, ev)) {
+    if (el && getOpenedMenus().some((el) => isChildEvent(el, ev))) {
       return;
     }
+    ev.closedMenuId = this.props.menuId;
     this.close();
   }
 
-  private onContextMenu(ev: MouseEvent) {
-    // Don't close a root menu when clicked to open the submenus.
-    const el = this.menuRef.el;
-    if (el && isChildEvent(el, ev)) {
-      return;
-    }
-    this.closeSubMenu();
-  }
-
   private getMenuItemsHeight(menuItems: MenuItem[]): Pixel {
     const numberOfSeparators = menuItems.filter((m) => m.separator).length;
     return MENU_ITEM_HEIGHT * menuItems.length + MENU_SEPARATOR_HEIGHT * numberOfSeparators;
@@ -270,4 +263,5 @@ Menu.props = {
   maxHeight: { type: Number, optional: true },
   onClose: Function,
   onMenuClicked: { type: Function, optional: true },
+  menuId: { type: String, optional: true },
 };

src/components/menu/menu.xml

@@ -43,6 +43,7 @@
         maxHeight="props.maxHeight"
         onMenuClicked="props.onMenuClicked"
         onClose="() => this.close()"
+        menuId="props.menuId"
       />
     </Popover>
   </t>

src/types/misc.ts

@@ -304,3 +304,7 @@ export interface SortOptions {
   /** If true treat empty cells as "0" instead of undefined */
   emptyCellAsZero?: boolean;
 }
+
+export interface MenuMouseEvent extends MouseEvent {
+  closedMenuId?: UID;
+}

tests/components/bottom_bar.test.ts

@@ -151,6 +151,20 @@ describe("BottomBar component", () => {
     app.destroy();
   });
 
+  test("Can open list of sheet menu if another menu is already open", async () => {
+    const { app } = await mountBottomBar();
+    expect(fixture.querySelectorAll(".o-menu")).toHaveLength(0);
+
+    await click(fixture, ".o-sheet-icon");
+    expect(fixture.querySelectorAll(".o-menu")).toHaveLength(1);
+    expect(fixture.querySelector(".o-menu-item")!.textContent).toEqual("Duplicate");
+
+    await click(fixture, ".o-sheet-item.o-list-sheets");
+    expect(fixture.querySelectorAll(".o-menu")).toHaveLength(1);
+    expect(fixture.querySelector(".o-menu-item")!.textContent).toEqual("Sheet1");
+    app.destroy();
+  });
+
   test("Can move right a sheet", async () => {
     const { app, model } = await mountBottomBar();
     const dispatch = jest.spyOn(model, "dispatch");
@@ -581,6 +595,23 @@ describe("BottomBar component", () => {
     app.destroy();
   });
 
+  test("Can open the list of statistics if another menu is already open", async () => {
+    const model = new Model();
+    const nonMockedDispatch = model.dispatch;
+    const { app } = await mountBottomBar(model);
+    model.dispatch = nonMockedDispatch;
+    setCellContent(model, "A2", "24");
+    selectCell(model, "A2");
+    await nextTick();
+    expect(fixture.querySelectorAll(".o-menu")).toHaveLength(0);
+    await click(fixture, ".o-sheet-icon");
+    expect(fixture.querySelector(".o-menu-item")!.textContent).toEqual("Duplicate");
+
+    await click(fixture, ".o-selection-statistic");
+    expect(fixture.querySelector(".o-menu-item")!.textContent).toEqual("Sum: 24");
+    app.destroy();
+  });
+
   test("Can activate a statistic from the list of statistics", async () => {
     const { app, model } = await mountBottomBar();
     setCellContent(model, "A2", "24");

tests/components/top_bar.test.ts

@@ -48,7 +48,7 @@ type Props = {
 class Parent extends Component<Props, SpreadsheetChildEnv> {
   static template = xml/* xml */ `
     <div class="o-spreadsheet">
-      <TopBar focusComposer="state.focusComposer" onClick="() => {}"/>
+      <TopBar focusComposer="state.focusComposer" onClick="() => {}" onComposerContentFocused="() => {}"/>
     </div>
   `;
   static components = { TopBar };
@@ -110,6 +110,16 @@ describe("TopBar component", () => {
     app.destroy();
   });
 
+  test("Menu should be closed while clicking on composer", async () => {
+    const { app } = await mountParent();
+    expect(fixture.querySelectorAll(".o-menu").length).toBe(0);
+    await click(fixture, ".o-topbar-menu[data-id='file']");
+    expect(fixture.querySelectorAll(".o-menu").length).toBe(1);
+    await click(fixture, ".o-spreadsheet-topbar div.o-composer");
+    expect(fixture.querySelectorAll(".o-menu").length).toBe(0);
+    app.destroy();
+  });
+
   test("merging cell button state is correct", async () => {
     const model = new Model({
       sheets: [