[IMP] Menu: Delegate enabled to children items

Currently, if a menu is evaluated as disabled, we disable it and prevent
the possibility to see its children by hover. However, there are some
issues with this:
- the user is mislead because we keep showing the arrow that suggest the
  presence of children items
- the user can simply not navigate through the submenus to see what kind
  of feature could be available
- if a menu has several children some of which are readonlyAllowed, then
  the readonlyAllowed condition has to be set on the parent as well; the
  'enabling' condition therefore becomes a combination set on both the
  parent and children
- if a parent menu is computed as 'enabled' it will not be greyed out
  (suggests that it has useful children) even if the said children are
  not enabled.

This revision changes the computation of 'enabled' state of item by
looking at the full tree of children and applies the following logic;
A parent menu is disabled if every leaf of the tree is disabled. A
single 'enabled' leaf (that is a child without children of its own) will
make all its parent enabled as well, up to the root. We also allow all
submenus to be opened regardless of the state of the parent.

The user will therefore have a visual hint that there are active
submenus or not (if all children are disabled, so will be the parent)
and they will also have the possibility to have a look at all children
to explore the possibilities.

Task: 5331717

Author: rrahir

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

@@ -18,9 +18,18 @@
           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);
+        cursor: not-allowed;
       }
       .o-menu-item-name {
         min-width: 40%;
@@ -29,12 +38,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,19 @@ 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) {
+      const areChildrenEnabled = children.length
+        ? children.some((child) => this.isEnabled(child))
+        : true;
+
+      return areChildrenEnabled;
+    } 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();
+  });
 });