[IMP] Menu: Delegate enabled to children items

src/actions/action.ts

@@ -97,7 +97,7 @@ let nextItemId = 1;
 
 export function createAction(item: ActionSpec): Action {
   const name = item.name;
-  const children = item.children;
+  const children = item.children || [];
   const description = item.description;
   const icon = item.icon;
   const secondaryIcon = item.secondaryIcon;
@@ -117,14 +117,15 @@ export function createAction(item: ActionSpec): Action {
           return undefined;
         }
       : undefined,
-    children: children
-      ? (env) => {
-          return children
-            .map((child) => (typeof child === "function" ? child(env) : child))
-            .flat()
-            .map(createAction);
-        }
-      : () => [],
+    children:
+      children.length > 0
+        ? (env) => {
+            return children
+              .map((child) => (typeof child === "function" ? child(env) : child))
+              .flat()
+              .map(createAction);
+          }
+        : () => [],
     isReadonlyAllowed: item.isReadonlyAllowed || false,
     separator: item.separator || false,
     icon: typeof icon === "function" ? icon : () => icon || "",

src/actions/view_actions.ts

@@ -131,21 +131,18 @@ export const unFreezeRows: ActionSpec = {
     env.model.dispatch("UNFREEZE_ROWS", {
       sheetId: env.model.getters.getActiveSheetId(),
     }),
-  isReadonlyAllowed: true,
   isVisible: (env) =>
     !!env.model.getters.getPaneDivisions(env.model.getters.getActiveSheetId()).ySplit,
 };
 
 export const freezeFirstRow: ActionSpec = {
   name: _t("1 row"),
   execute: (env) => interactiveFreezeColumnsRows(env, "ROW", 1),
-  isReadonlyAllowed: true,
 };
 
 export const freezeSecondRow: ActionSpec = {
   name: _t("2 rows"),
   execute: (env) => interactiveFreezeColumnsRows(env, "ROW", 2),
-  isReadonlyAllowed: true,
 };
 
 export const freezeCurrentRow: ActionSpec = {
@@ -154,7 +151,6 @@ export const freezeCurrentRow: ActionSpec = {
     const { bottom } = env.model.getters.getSelectedZone();
     interactiveFreezeColumnsRows(env, "ROW", bottom + 1);
   },
-  isReadonlyAllowed: true,
 };
 
 export const unFreezeCols: ActionSpec = {
@@ -163,21 +159,18 @@ export const unFreezeCols: ActionSpec = {
     env.model.dispatch("UNFREEZE_COLUMNS", {
       sheetId: env.model.getters.getActiveSheetId(),
     }),
-  isReadonlyAllowed: true,
   isVisible: (env) =>
     !!env.model.getters.getPaneDivisions(env.model.getters.getActiveSheetId()).xSplit,
 };
 
 export const freezeFirstCol: ActionSpec = {
   name: _t("1 column"),
   execute: (env) => interactiveFreezeColumnsRows(env, "COL", 1),
-  isReadonlyAllowed: true,
 };
 
 export const freezeSecondCol: ActionSpec = {
   name: _t("2 columns"),
   execute: (env) => interactiveFreezeColumnsRows(env, "COL", 2),
-  isReadonlyAllowed: true,
 };
 
 export const freezeCurrentCol: ActionSpec = {
@@ -186,7 +179,6 @@ export const freezeCurrentCol: ActionSpec = {
     const { right } = env.model.getters.getSelectedZone();
     interactiveFreezeColumnsRows(env, "COL", right + 1);
   },
-  isReadonlyAllowed: true,
 };
 
 export const viewGridlines: ActionSpec = {

src/components/menu/menu.css

@@ -12,15 +12,26 @@
           var(--os-desktop-menu-item-height) - 2 * var(--os-menu-item-padding-vertical)
         );
       }
-      &:not(.disabled) {
+      & {
         &:hover,
         &.o-menu-item-active {
           background-color: var(--os-button-active-bg);
           color: var(--os-button-active-text-color);
         }
+        .o-menu-item-icon {
+          .o-icon {
+            color: var(--os-icons-color);
+          }
+        }
+        .o-menu-item-description {
+          color: grey;
+        }
       }
       &.disabled {
-        color: var(--os-disabled-text-color);
+        &,
+        .o-menu-item-description {
+          color: var(--os-disabled-text-color);
+        }
       }
       .o-menu-item-name {
         min-width: 40%;
@@ -29,12 +40,6 @@
         display: inline-block;
         margin: 0px 8px 0px 0px;
       }
-      .o-menu-item-description {
-        color: grey;
-      }
-      &.disabled {
-        cursor: not-allowed;
-      }
     }
   }
   &.o-spreadsheet-mobile {

src/components/menu/menu.ts

@@ -118,10 +118,15 @@ export class Menu extends Component<MenuProps, SpreadsheetChildEnv> {
   }
 
   isEnabled(menu: Action) {
-    if (menu.isEnabled(this.env)) {
-      return this.env.model.getters.isReadonly() ? menu.isReadonlyAllowed : true;
+    const children = menu.children?.(this.env);
+    if (children.length) {
+      return children.some((child) => this.isEnabled(child));
+    } else {
+      if (menu.isEnabled(this.env)) {
+        return this.env.model.getters.isReadonly() ? menu.isReadonlyAllowed : true;
+      }
+      return false;
     }
-    return false;
   }
 
   get menuStyle() {

src/components/menu_popover/menu_popover.ts

@@ -245,17 +245,15 @@ export class MenuPopover extends Component<Props, SpreadsheetChildEnv> {
   }
 
   onMouseOver(menu: Action, ev: MouseEvent) {
-    if (this.isEnabled(menu)) {
-      if (this.isParentMenu(this.subMenu, menu)) {
-        this.openingTimeOut.clear();
-        return;
-      }
-      const currentTarget = ev.currentTarget as HTMLElement;
-      if (this.isRoot(menu)) {
-        this.openingTimeOut.schedule(() => {
-          this.openSubMenu(menu, currentTarget);
-        }, TIMEOUT_DELAY);
-      }
+    if (this.isParentMenu(this.subMenu, menu)) {
+      this.openingTimeOut.clear();
+      return;
+    }
+    const currentTarget = ev.currentTarget as HTMLElement;
+    if (this.isRoot(menu)) {
+      this.openingTimeOut.schedule(() => {
+        this.openSubMenu(menu, currentTarget);
+      }, TIMEOUT_DELAY);
     }
   }
 

src/registries/menus/topbar_menu_registry.ts

@@ -22,7 +22,6 @@ topbarMenuRegistry
   .add("file", {
     name: _t("File"),
     sequence: 10,
-    isReadonlyAllowed: true,
   })
   .addChild("settings", ["file"], {
     name: _t("Settings"),
@@ -39,7 +38,6 @@ topbarMenuRegistry
   .add("edit", {
     name: _t("Edit"),
     sequence: 20,
-    isReadonlyAllowed: true,
   })
   .addChild("undo", ["edit"], {
     ...ACTION_EDIT.undo,
@@ -126,7 +124,6 @@ topbarMenuRegistry
   .add("view", {
     name: _t("View"),
     sequence: 30,
-    isReadonlyAllowed: true,
   })
   .addChild("unfreeze_panes", ["view"], {
     ...ACTION_VIEW.unFreezePane,
@@ -225,7 +222,6 @@ topbarMenuRegistry
   .add("insert", {
     name: _t("Insert"),
     sequence: 40,
-    isReadonlyAllowed: true,
   })
   .addChild("insert_row", ["insert"], {
     ...ACTION_INSERT.insertRow,
@@ -346,7 +342,6 @@ topbarMenuRegistry
   .add("format", {
     name: _t("Format"),
     sequence: 50,
-    isReadonlyAllowed: true,
   })
   .addChild("format_number", ["format"], {
     ...formatNumberMenuItemSpec,
@@ -441,7 +436,6 @@ topbarMenuRegistry
   .add("data", {
     name: _t("Data"),
     sequence: 60,
-    isReadonlyAllowed: true,
   })
   .addChild("sort_range", ["data"], {
     ...ACTION_DATA.sortRange,

tests/menus/context_menu_component.test.ts

@@ -449,33 +449,51 @@ describe("Context MenuPopover internal tests", () => {
     expect(menuItem2?.classList).toContain("o-menu-item-active");
   });
 
-  test("submenu does not open when disabled", async () => {
-    const menuItems: Action[] = createActions([
-      makeTestMenuItem("root", {
-        isEnabled: () => false,
-        children: [makeTestMenuItem("subMenu")],
-      }),
-    ]);
-    await renderContextMenu(300, 300, { menuItems });
-    expect(fixture.querySelector(".o-menu div[data-name='root']")!.classList).toContain("disabled");
-    await simulateClick(".o-menu div[data-name='root']");
-    expect(fixture.querySelector(".o-menu div[data-name='subMenu']")).toBeFalsy();
-  });
+  describe("IsEnabled is ignored on parent menu items", () => {
+    test("submenu opens even if the parent is disabled", async () => {
+      const menuItems: Action[] = createActions([
+        makeTestMenuItem("root", {
+          isEnabled: () => false,
+          children: [makeTestMenuItem("subMenu")],
+        }),
+      ]);
+      await renderContextMenu(300, 300, { menuItems });
+      expect(fixture.querySelector(".o-menu div[data-name='root']")!).not.toHaveClass("disabled");
+      await simulateClick(".o-menu div[data-name='root']");
+      expect(fixture.querySelector(".o-menu div[data-name='subMenu']")).toBeTruthy();
+    });
 
-  test("submenu does not open when hovering write only parent", async () => {
-    const menuItems: Action[] = createActions([
-      makeTestMenuItem("root", {
-        isReadonlyAllowed: false,
-        isEnabled: () => true,
-        children: [makeTestMenuItem("subMenu")],
-      }),
-    ]);
-    await renderContextMenu(300, 300, { menuItems });
-    model.updateMode("readonly");
-    await nextTick();
-    expect(fixture.querySelector(".o-menu div[data-name='root']")!.classList).toContain("disabled");
-    await mouseOverMenuElement(".o-menu div[data-name='root']");
-    expect(fixture.querySelector(".o-menu div[data-name='subMenu']")).toBeFalsy();
+    test("in readonly, submenu opens if the children are readonly and the parent write only", async () => {
+      const menuItems: Action[] = createActions([
+        makeTestMenuItem("root", {
+          isReadonlyAllowed: false,
+          isEnabled: () => false,
+          children: [makeTestMenuItem("subMenu", { isReadonlyAllowed: true })],
+        }),
+      ]);
+      await renderContextMenu(300, 300, { menuItems });
+      model.updateMode("readonly");
+      await nextTick();
+      expect(fixture.querySelector(".o-menu div[data-name='root']")!).not.toHaveClass("disabled");
+      await mouseOverMenuElement(".o-menu div[data-name='root']");
+      expect(fixture.querySelector(".o-menu div[data-name='subMenu']")).toBeTruthy();
+    });
+
+    test("in readonly, submenu opens if the children are readonly and the parent write only", async () => {
+      const menuItems: Action[] = createActions([
+        makeTestMenuItem("root", {
+          isReadonlyAllowed: false,
+          isEnabled: () => true,
+          children: [makeTestMenuItem("subMenu")],
+        }),
+      ]);
+      await renderContextMenu(300, 300, { menuItems });
+      model.updateMode("readonly");
+      await nextTick();
+      expect(fixture.querySelector(".o-menu div[data-name='root']")!).toHaveClass("disabled");
+      await mouseOverMenuElement(".o-menu div[data-name='root']");
+      expect(fixture.querySelector(".o-menu div[data-name='subMenu']")).toBeTruthy();
+    });
   });
 
   test("submenu does not close when sub item hovered", async () => {

tests/menus/menu_component.test.ts

@@ -1,3 +1,4 @@
+import { Model } from "@odoo/o-spreadsheet-engine";
 import { createActions } from "../../src/actions/action";
 import { Menu } from "../../src/components/menu/menu";
 import { simulateClick } from "../test_helpers/dom_helper";
@@ -24,4 +25,28 @@ describe("Menu component", () => {
     await simulateClick(selector);
     expect(callback).not.toHaveBeenCalled();
   });
+
+  test("Execute is not called when menu item is ont readonly allowed", async () => {
+    const callback = jest.fn();
+    const menuItems = createActions([
+      {
+        id: "test_menu",
+        name: "Test Menu",
+        isEnabled: () => true,
+        execute: () => {},
+      },
+    ]);
+
+    const selector = ".o-menu div[data-name='test_menu']";
+    const model = new Model();
+    model.updateMode("readonly");
+    const { fixture } = await mountComponent(Menu, {
+      props: { menuItems, onClose: () => {}, onClickMenu: () => callback() },
+      env: { model },
+    });
+
+    expect(fixture.querySelector(selector)!.classList).toContain("disabled");
+    await simulateClick(selector);
+    expect(callback).not.toHaveBeenCalled();
+  });
 });