From f3f93dad893051187d08a097434a65670fd0ee7b Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Mon, 2 Oct 2023 15:00:52 -0400 Subject: [PATCH 1/9] test(menu): add failing test --- .../components/menu/test/multiple/index.html | 51 +++++++++++++++++++ .../components/menu/test/multiple/menu.e2e.ts | 48 +++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 core/src/components/menu/test/multiple/index.html create mode 100644 core/src/components/menu/test/multiple/menu.e2e.ts 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..e4736b3784b --- /dev/null +++ b/core/src/components/menu/test/multiple/index.html @@ -0,0 +1,51 @@ + + + + + Menu - Multiple + + + + + + + + + + + + + + Primary + + + Menu Content + + + + + + Secondary + + + 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..be34c603b6a --- /dev/null +++ b/core/src/components/menu/test/multiple/menu.e2e.ts @@ -0,0 +1,48 @@ +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: disable'), () => { + test.beforeEach(async ({ page }) => { + 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 log a warning when trying to present multiple menus on the same side', async ({ page }) => { + const logs: string[] = []; + + page.on('console', (msg) => { + if (msg.type() === 'warning') { + logs.push(msg.text()); + } + }); + + const primaryMenu = page.locator('ion-menu#primary-menu'); + const secondaryMenu = page.locator('ion-menu#secondary-menu'); + + await primaryMenu.evaluate((el: HTMLIonMenuElement) => el.open()); + await secondaryMenu.evaluate((el: HTMLIonMenuElement) => el.open()); + + expect(logs.length).toBe(1); + }); + }); +}); From 9cf14bf986fea008830d07287f5cc7b302e1b58f Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Mon, 2 Oct 2023 15:04:18 -0400 Subject: [PATCH 2/9] fix test --- .../components/menu/test/multiple/menu.e2e.ts | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/core/src/components/menu/test/multiple/menu.e2e.ts b/core/src/components/menu/test/multiple/menu.e2e.ts index be34c603b6a..0bce611ce11 100644 --- a/core/src/components/menu/test/multiple/menu.e2e.ts +++ b/core/src/components/menu/test/multiple/menu.e2e.ts @@ -27,22 +27,16 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => await expect(secondaryMenu).toBeHidden(); }); - test('should log a warning when trying to present multiple menus on the same side', async ({ page }) => { - const logs: string[] = []; - - page.on('console', (msg) => { - if (msg.type() === 'warning') { - logs.push(msg.text()); - } - }); - + 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 secondaryMenu.evaluate((el: HTMLIonMenuElement) => el.open()); + await expect(primaryMenu).toBeVisible(); - expect(logs.length).toBe(1); + await secondaryMenu.evaluate((el: HTMLIonMenuElement) => el.open()); + await expect(primaryMenu).toBeHidden(); + await expect(secondaryMenu).toBeVisible(); }); }); }); From ba31223daa639a3786ffca624510bf5fb0dcf2d6 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Mon, 2 Oct 2023 15:02:42 -0400 Subject: [PATCH 3/9] sync --- core/src/components/menu/menu-interface.ts | 1 - core/src/components/menu/menu.tsx | 13 ------------- core/src/utils/menu-controller/index.ts | 12 ------------ 3 files changed, 26 deletions(-) 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 ec12c982104..6de8e76fb4d 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'; @@ -719,18 +718,6 @@ export class Menu implements ComponentInterface, MenuI { this.forceClosing(); } - 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); - } - } - assert(!this.isAnimating, 'can not be animating'); } diff --git a/core/src/utils/menu-controller/index.ts b/core/src/utils/menu-controller/index.ts index d331405a080..835c73a4a31 100644 --- a/core/src/utils/menu-controller/index.ts +++ b/core/src/utils/menu-controller/index.ts @@ -131,9 +131,6 @@ const createMenuController = () => { const _register = (menu: MenuI) => { if (menus.indexOf(menu) < 0) { - if (!menu.disabled) { - _setActiveMenu(menu); - } menus.push(menu); } }; @@ -145,14 +142,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 +227,6 @@ const createMenuController = () => { _register, _unregister, _setOpen, - _setActiveMenu, }; }; From cb42554974e4e97972af6b929bdf1dc4ebc4ca6d Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 3 Oct 2023 16:11:01 -0400 Subject: [PATCH 4/9] warn if referencing menu by side and multip --- .../components/menu/test/multiple/index.html | 23 +++++++++++++ .../components/menu/test/multiple/menu.e2e.ts | 32 +++++++++++++++++++ core/src/utils/menu-controller/index.ts | 27 +++++++++++++--- 3 files changed, 78 insertions(+), 4 deletions(-) diff --git a/core/src/components/menu/test/multiple/index.html b/core/src/components/menu/test/multiple/index.html index e4736b3784b..9a45a237fa9 100644 --- a/core/src/components/menu/test/multiple/index.html +++ b/core/src/components/menu/test/multiple/index.html @@ -12,6 +12,10 @@ + @@ -34,6 +38,24 @@ Menu Content + + + + Success + + + Menu Content + + + + + + Danger + + + Menu Content + +
@@ -48,4 +70,5 @@
+ diff --git a/core/src/components/menu/test/multiple/menu.e2e.ts b/core/src/components/menu/test/multiple/menu.e2e.ts index 0bce611ce11..a92b1f90f99 100644 --- a/core/src/components/menu/test/multiple/menu.e2e.ts +++ b/core/src/components/menu/test/multiple/menu.e2e.ts @@ -38,5 +38,37 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => 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 835c73a4a31..808093bf4c1 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'; @@ -76,14 +78,31 @@ const createMenuController = () => { 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) { + 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) { + 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" From c9669dedee184f947f48692de04e7ae06ee08a5d Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 3 Oct 2023 16:11:13 -0400 Subject: [PATCH 5/9] typo --- core/src/components/menu/test/multiple/menu.e2e.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/components/menu/test/multiple/menu.e2e.ts b/core/src/components/menu/test/multiple/menu.e2e.ts index a92b1f90f99..f54de5c7518 100644 --- a/core/src/components/menu/test/multiple/menu.e2e.ts +++ b/core/src/components/menu/test/multiple/menu.e2e.ts @@ -5,7 +5,7 @@ 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: disable'), () => { + test.describe(title('menu: multiple'), () => { test.beforeEach(async ({ page }) => { await page.goto(`/src/components/menu/test/multiple`, config); }); From 55704489ea3ac57fa099d52d43f71e33f81b797c Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 5 Oct 2023 18:03:35 -0400 Subject: [PATCH 6/9] add annotations --- core/src/components/menu/test/multiple/menu.e2e.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/core/src/components/menu/test/multiple/menu.e2e.ts b/core/src/components/menu/test/multiple/menu.e2e.ts index f54de5c7518..dd0a6c3d797 100644 --- a/core/src/components/menu/test/multiple/menu.e2e.ts +++ b/core/src/components/menu/test/multiple/menu.e2e.ts @@ -7,6 +7,11 @@ import { configs, test } from '@utils/test/playwright'; configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { test.describe(title('menu: multiple'), () => { test.beforeEach(async ({ page }) => { + testInfo.annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/18974', + }); + await page.goto(`/src/components/menu/test/multiple`, config); }); From 90425eb61f8f3e916b977cb75d9037b468b6af28 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 5 Oct 2023 18:03:47 -0400 Subject: [PATCH 7/9] add testInfo --- core/src/components/menu/test/multiple/menu.e2e.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/components/menu/test/multiple/menu.e2e.ts b/core/src/components/menu/test/multiple/menu.e2e.ts index dd0a6c3d797..d2f18661c1b 100644 --- a/core/src/components/menu/test/multiple/menu.e2e.ts +++ b/core/src/components/menu/test/multiple/menu.e2e.ts @@ -6,7 +6,7 @@ import { configs, test } from '@utils/test/playwright'; */ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { test.describe(title('menu: multiple'), () => { - test.beforeEach(async ({ page }) => { + test.beforeEach(async ({ page }, testInfo) => { testInfo.annotations.push({ type: 'issue', description: 'https://github.com/ionic-team/ionic-framework/issues/18974', From 0787dc8e61948895ac4570d00b06e052c0739fb8 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 10 Oct 2023 11:12:46 -0400 Subject: [PATCH 8/9] avoid multiple warnings --- core/src/utils/menu-controller/index.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/core/src/utils/menu-controller/index.ts b/core/src/utils/menu-controller/index.ts index 808093bf4c1..15288eb7542 100644 --- a/core/src/utils/menu-controller/index.ts +++ b/core/src/utils/menu-controller/index.ts @@ -14,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(); } @@ -72,7 +72,18 @@ 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') { @@ -80,7 +91,7 @@ const createMenuController = () => { // so first try to get the enabled one const menuRefs = menus.filter((m) => m.side === menu && !m.disabled); if (menuRefs.length >= 1) { - 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) @@ -94,7 +105,7 @@ const createMenuController = () => { // so try to get the first menu side found const sideMenuRefs = menus.filter((m) => m.side === menu); if (sideMenuRefs.length >= 1) { - 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) From 01f4a272e2b93ce4fb92f14e557c7c85e98c6d8d Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 10 Oct 2023 11:49:13 -0400 Subject: [PATCH 9/9] log when closing and toggling too --- core/src/utils/menu-controller/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/utils/menu-controller/index.ts b/core/src/utils/menu-controller/index.ts index 15288eb7542..a0e6cf25f78 100644 --- a/core/src/utils/menu-controller/index.ts +++ b/core/src/utils/menu-controller/index.ts @@ -22,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(); } @@ -30,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(); }