diff --git a/core/src/components/menu/menu-interface.ts b/core/src/components/menu/menu-interface.ts index 87813331a33..0361f207fb1 100644 --- a/core/src/components/menu/menu-interface.ts +++ b/core/src/components/menu/menu-interface.ts @@ -30,7 +30,6 @@ export interface MenuControllerI { _setOpen(menu: MenuI, shouldOpen: boolean, animated: boolean): Promise; _register(menu: MenuI): void; _unregister(menu: MenuI): void; - _setActiveMenu(menu: MenuI): void; getMenus(): Promise; getOpenSync(): HTMLIonMenuElement | undefined; diff --git a/core/src/components/menu/menu.tsx b/core/src/components/menu/menu.tsx index 350984b5a02..1dc46a41e06 100644 --- a/core/src/components/menu/menu.tsx +++ b/core/src/components/menu/menu.tsx @@ -1,7 +1,6 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core'; import { Build, Component, Element, Event, Host, Listen, Method, Prop, State, Watch, h } from '@stencil/core'; import { getTimeGivenProgression } from '@utils/animation/cubic-bezier'; -import { doc } from '@utils/browser'; import { GESTURE_CONTROLLER } from '@utils/gesture'; import type { Attributes } from '@utils/helpers'; import { inheritAriaAttributes, assert, clamp, isEndSide as isEnd } from '@utils/helpers'; @@ -762,18 +761,6 @@ export class Menu implements ComponentInterface, MenuI { */ this.afterAnimation(false); } - - if (doc?.contains(this.el)) { - /** - * Only set the active menu if the menu element is - * present in the DOM. Otherwise if it was destructively - * re-hydrated (through Angular Universal), then ignore - * setting the removed node as the active menu. - */ - if (!this.disabled) { - menuController._setActiveMenu(this); - } - } } render() { diff --git a/core/src/components/menu/test/multiple/index.html b/core/src/components/menu/test/multiple/index.html new file mode 100644 index 00000000000..9a45a237fa9 --- /dev/null +++ b/core/src/components/menu/test/multiple/index.html @@ -0,0 +1,74 @@ + + + + + Menu - Multiple + + + + + + + + + + + + + + + Primary + + + Menu Content + + + + + + Secondary + + + Menu Content + + + + + + Success + + + Menu Content + + + + + + Danger + + + Menu Content + + +
+ + + + + + + Menu - Multiple + + + Content +
+ + + diff --git a/core/src/components/menu/test/multiple/menu.e2e.ts b/core/src/components/menu/test/multiple/menu.e2e.ts new file mode 100644 index 00000000000..d2f18661c1b --- /dev/null +++ b/core/src/components/menu/test/multiple/menu.e2e.ts @@ -0,0 +1,79 @@ +import { expect } from '@playwright/test'; +import { configs, test } from '@utils/test/playwright'; + +/** + * This behavior does not vary across modes/directions + */ +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('menu: multiple'), () => { + test.beforeEach(async ({ page }, testInfo) => { + testInfo.annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/18974', + }); + + await page.goto(`/src/components/menu/test/multiple`, config); + }); + + test('should present each menu on the same side individually', async ({ page }) => { + const primaryMenu = page.locator('ion-menu#primary-menu'); + const secondaryMenu = page.locator('ion-menu#secondary-menu'); + + await primaryMenu.evaluate((el: HTMLIonMenuElement) => el.open()); + await expect(primaryMenu).toBeVisible(); + + await primaryMenu.evaluate((el: HTMLIonMenuElement) => el.close()); + await expect(primaryMenu).toBeHidden(); + + await secondaryMenu.evaluate((el: HTMLIonMenuElement) => el.open()); + await expect(secondaryMenu).toBeVisible(); + + await secondaryMenu.evaluate((el: HTMLIonMenuElement) => el.close()); + await expect(secondaryMenu).toBeHidden(); + }); + + test('should close first menu when showing another menu on same side', async ({ page }) => { + const primaryMenu = page.locator('ion-menu#primary-menu'); + const secondaryMenu = page.locator('ion-menu#secondary-menu'); + + await primaryMenu.evaluate((el: HTMLIonMenuElement) => el.open()); + await expect(primaryMenu).toBeVisible(); + + await secondaryMenu.evaluate((el: HTMLIonMenuElement) => el.open()); + await expect(primaryMenu).toBeHidden(); + await expect(secondaryMenu).toBeVisible(); + }); + + test('passing side to the menuController when multiple menus have that side should result in a warning', async ({ + page, + }) => { + const logs: string[] = []; + + page.on('console', (msg) => { + if (msg.type() === 'warning') { + logs.push(msg.text()); + } + }); + + await page.evaluate(() => (window as any).menuController.open('start')); + + expect(logs.length).toBe(1); + }); + + test('passing side to the menuController when multiple disabled menus have that side should result in a warning', async ({ + page, + }) => { + const logs: string[] = []; + + page.on('console', (msg) => { + if (msg.type() === 'warning') { + logs.push(msg.text()); + } + }); + + await page.evaluate(() => (window as any).menuController.open('end')); + + expect(logs.length).toBe(1); + }); + }); +}); diff --git a/core/src/utils/menu-controller/index.ts b/core/src/utils/menu-controller/index.ts index d331405a080..a0e6cf25f78 100644 --- a/core/src/utils/menu-controller/index.ts +++ b/core/src/utils/menu-controller/index.ts @@ -1,3 +1,5 @@ +import { printIonWarning } from '@utils/logging'; + import type { MenuI } from '../../components/menu/menu-interface'; import type { AnimationBuilder, BackButtonEvent } from '../../interface'; import { MENU_BACK_BUTTON_PRIORITY } from '../hardware-back-button'; @@ -12,7 +14,7 @@ const createMenuController = () => { const menus: MenuI[] = []; const open = async (menu?: string | null): Promise => { - const menuEl = await get(menu); + const menuEl = await get(menu, true); if (menuEl) { return menuEl.open(); } @@ -20,7 +22,7 @@ const createMenuController = () => { }; const close = async (menu?: string | null): Promise => { - const menuEl = await (menu !== undefined ? get(menu) : getOpen()); + const menuEl = await (menu !== undefined ? get(menu, true) : getOpen()); if (menuEl !== undefined) { return menuEl.close(); } @@ -28,7 +30,7 @@ const createMenuController = () => { }; const toggle = async (menu?: string | null): Promise => { - const menuEl = await get(menu); + const menuEl = await get(menu, true); if (menuEl) { return menuEl.toggle(); } @@ -70,20 +72,48 @@ const createMenuController = () => { return false; }; - const get = async (menu?: string | null): Promise => { + /** + * Finds and returns the menu specified by "menu" if registered. + * @param menu - The side or ID of the desired menu + * @param logOnMultipleSideMenus - If true, this function will log a warning + * if "menu" is a side but multiple menus on the same side were found. Since this function + * is used in multiple places, we default this log to false so that the calling + * functions can choose whether or not it is appropriate to log this warning. + */ + const get = async ( + menu?: string | null, + logOnMultipleSideMenus: boolean = false + ): Promise => { await waitUntilReady(); if (menu === 'start' || menu === 'end') { // there could be more than one menu on the same side // so first try to get the enabled one - const menuRef = find((m) => m.side === menu && !m.disabled); - if (menuRef) { - return menuRef; + const menuRefs = menus.filter((m) => m.side === menu && !m.disabled); + if (menuRefs.length >= 1) { + if (menuRefs.length > 1 && logOnMultipleSideMenus) { + printIonWarning( + `menuController queried for a menu on the "${menu}" side, but ${menuRefs.length} menus were found. The first menu reference will be used. If this is not the behavior you want then pass the ID of the menu instead of its side.`, + menuRefs.map((m) => m.el) + ); + } + + return menuRefs[0].el; } // didn't find a menu side that is enabled // so try to get the first menu side found - return find((m) => m.side === menu); + const sideMenuRefs = menus.filter((m) => m.side === menu); + if (sideMenuRefs.length >= 1) { + if (sideMenuRefs.length > 1 && logOnMultipleSideMenus) { + printIonWarning( + `menuController queried for a menu on the "${menu}" side, but ${sideMenuRefs.length} menus were found. The first menu reference will be used. If this is not the behavior you want then pass the ID of the menu instead of its side.`, + sideMenuRefs.map((m) => m.el) + ); + } + + return sideMenuRefs[0].el; + } } else if (menu != null) { // the menuId was not left or right // so try to get the menu by its "id" @@ -131,9 +161,6 @@ const createMenuController = () => { const _register = (menu: MenuI) => { if (menus.indexOf(menu) < 0) { - if (!menu.disabled) { - _setActiveMenu(menu); - } menus.push(menu); } }; @@ -145,14 +172,6 @@ const createMenuController = () => { } }; - const _setActiveMenu = (menu: MenuI) => { - // if this menu should be enabled - // then find all the other menus on this same side - // and automatically disable other same side menus - const side = menu.side; - menus.filter((m) => m.side === side && m !== menu).forEach((m) => (m.disabled = true)); - }; - const _setOpen = async (menu: MenuI, shouldOpen: boolean, animated: boolean): Promise => { if (isAnimatingSync()) { return false; @@ -238,7 +257,6 @@ const createMenuController = () => { _register, _unregister, _setOpen, - _setActiveMenu, }; };