[FIX] menu: dynamically update visible menu items

Before this commit, the menu visible menu items were given as props of the
`Menu` component. This meant that contrarily to the property `isEnabled` of
the menu items, the `isVisible` property was computed once when opening
the menu, and never updated.

This commit fixes this issue by making the `Menu` component compute the
`isVisible` property at each render, dynamically updating which menu items
are visible based on the state of the model.

Odoo task 3204730

closes #2384

Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
Signed-off-by: Minne Adrien (adrm) <[email protected]>

src/components/bottom_bar/bottom_bar.ts

@@ -190,7 +190,7 @@ export class BottomBar extends Component<Props, SpreadsheetChildEnv> {
 
   openContextMenu(x: Pixel, y: Pixel, registry: MenuItemRegistry, menuId?: UID) {
     this.menuState.isOpen = true;
-    this.menuState.menuItems = registry.getAll().filter((x) => x.isVisible(this.env));
+    this.menuState.menuItems = registry.getAll();
     this.menuState.position = { x, y };
     this.menuState.menuId = menuId;
   }

src/components/figures/figure_chart/figure_chart.ts

@@ -102,7 +102,7 @@ export class ChartFigure extends Component<Props, SpreadsheetChildEnv> {
   private openContextMenu(position: DOMCoordinates) {
     const registry = this.getMenuItemRegistry();
     this.menuState.isOpen = true;
-    this.menuState.menuItems = registry.getAll().filter((x) => x.isVisible(this.env));
+    this.menuState.menuItems = registry.getAll();
     this.menuState.position = position;
   }
 

src/components/grid/grid.ts

@@ -523,9 +523,7 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
     }
     this.menuState.isOpen = true;
     this.menuState.position = { x, y };
-    this.menuState.menuItems = registries[type]
-      .getAll()
-      .filter((item) => !item.isVisible || item.isVisible(this.env));
+    this.menuState.menuItems = registries[type].getAll();
   }
 
   copy(cut: boolean, ev: ClipboardEvent) {

src/components/menu/menu.ts

@@ -112,14 +112,18 @@ export class Menu extends Component<Props, SpreadsheetChildEnv> {
     });
   }
 
+  get visibleMenuItems(): MenuItem[] {
+    return this.props.menuItems.filter((x) => x.isVisible(this.env));
+  }
+
   get subMenuPosition(): DOMCoordinates {
     const position = Object.assign({}, this.subMenu.position);
     position.y -= this.subMenu.scrollOffset || 0;
     return position;
   }
 
   get menuHeight(): Pixel {
-    const menuItems = this.props.menuItems;
+    const menuItems = this.visibleMenuItems;
 
     let menuItemsHeight = this.getMenuItemsHeight(menuItems);
 
@@ -168,7 +172,7 @@ export class Menu extends Component<Props, SpreadsheetChildEnv> {
    * and the menu item at a given index.
    */
   private subMenuVerticalPosition(menuIndex: number): Pixel {
-    const menusAbove = this.props.menuItems.slice(0, menuIndex);
+    const menusAbove = this.visibleMenuItems.slice(0, menuIndex);
     return this.position.y + this.getMenuItemsHeight(menusAbove) + MENU_VERTICAL_PADDING;
   }
 

src/components/menu/menu.xml

@@ -1,14 +1,14 @@
 <templates>
   <t t-name="o-spreadsheet-Menu" owl="1">
-    <Popover t-if="props.menuItems.length" t-props="popoverProps">
+    <Popover t-if="visibleMenuItems.length" t-props="popoverProps">
       <div
         t-ref="menu"
         class="o-menu"
         t-on-scroll="onScroll"
         t-on-wheel.stop=""
         t-on-click.stop=""
         t-on-contextmenu.prevent="">
-        <t t-foreach="props.menuItems" t-as="menuItem" t-key="menuItem.id">
+        <t t-foreach="visibleMenuItems" t-as="menuItem" t-key="menuItem.id">
           <t t-set="isMenuRoot" t-value="isRoot(menuItem)"/>
           <t t-set="isMenuEnabled" t-value="isEnabled(menuItem)"/>
           <div

src/components/top_bar/top_bar.ts

@@ -405,9 +405,7 @@ export class TopBar extends Component<Props, SpreadsheetChildEnv> {
     const { left, top, height } = (ev.target as HTMLElement).getBoundingClientRect();
     this.state.menuState.isOpen = true;
     this.state.menuState.position = { x: left, y: top + height };
-    this.state.menuState.menuItems = getMenuChildren(menu, this.env).filter(
-      (item) => !item.isVisible || item.isVisible(this.env)
-    );
+    this.state.menuState.menuItems = getMenuChildren(menu, this.env);
     this.state.menuState.parentMenu = menu;
     this.isSelectingMenu = true;
     this.openedEl = ev.target as HTMLElement;
@@ -452,9 +450,7 @@ export class TopBar extends Component<Props, SpreadsheetChildEnv> {
     this.fillColor = this.style.fillColor || "#ffffff";
     this.textColor = this.style.textColor || "#000000";
 
-    this.menus = topbarMenuRegistry
-      .getAll()
-      .filter((item) => !item.isVisible || item.isVisible(this.env));
+    this.menus = topbarMenuRegistry.getAll();
   }
 
   getMenuName(menu: FullMenuItem) {

tests/components/context_menu.test.ts

@@ -20,6 +20,7 @@ import { mockGetBoundingClientRect } from "../test_helpers/mock_helpers";
 
 let fixture: HTMLElement;
 let model: Model;
+let parent: Component;
 
 mockGetBoundingClientRect({
   "o-menu": (el) => getElPosition(el),
@@ -104,7 +105,7 @@ async function renderContextMenu(
   // x, y are relative to the upper left grid corner, but the menu
   // props must take the top bar into account.
   fixture = makeTestFixture();
-  ({ fixture, model } = await mountComponent(ContextMenuParent, {
+  ({ fixture, model, parent } = await mountComponent(ContextMenuParent, {
     props: {
       x,
       y,
@@ -642,6 +643,42 @@ describe("Context Menu internal tests", () => {
     expect(fixture.querySelector(".o-menu div[data-name='visible_submenu_1']")).toBeTruthy();
     expect(fixture.querySelector(".o-menu div[data-name='invisible_submenu_1']")).toBeFalsy();
   });
+
+  test("Enabled menus are updated at each render", async () => {
+    let enabled = true;
+    const menuItems: FullMenuItem[] = [
+      createFullMenuItem("menuItem", {
+        name: "menuItem",
+        sequence: 1,
+        isEnabled: () => enabled,
+      }),
+    ];
+    await renderContextMenu(300, 300, { menuItems });
+    expect(fixture.querySelector("div[data-name='menuItem']")?.classList).not.toContain("disabled");
+
+    enabled = false;
+    parent.render(true);
+    await nextTick();
+    expect(fixture.querySelector("div[data-name='menuItem']")?.classList).toContain("disabled");
+  });
+
+  test("Visible menus are updated at each render", async () => {
+    let visible = true;
+    const menuItems: FullMenuItem[] = [
+      createFullMenuItem("menuItem", {
+        name: "menuItem",
+        sequence: 1,
+        isVisible: () => visible,
+      }),
+    ];
+    await renderContextMenu(300, 300, { menuItems });
+    expect(fixture.querySelector("div[data-name='menuItem']")).toBeTruthy();
+
+    visible = false;
+    parent.render(true);
+    await nextTick();
+    expect(fixture.querySelector("div[data-name='menuItem']")).toBeFalsy();
+  });
 });
 jest.setTimeout(300000);
 describe("Context Menu position on large screen 1000px/1000px", () => {