Skip to content

fix(menu) menus on the same side are not automatically disabled #28269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion core/src/components/menu/menu-interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ export interface MenuControllerI {
_setOpen(menu: MenuI, shouldOpen: boolean, animated: boolean): Promise<boolean>;
_register(menu: MenuI): void;
_unregister(menu: MenuI): void;
_setActiveMenu(menu: MenuI): void;

getMenus(): Promise<HTMLIonMenuElement[]>;
getOpenSync(): HTMLIonMenuElement | undefined;
Expand Down
13 changes: 0 additions & 13 deletions core/src/components/menu/menu.tsx
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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() {
Expand Down
74 changes: 74 additions & 0 deletions core/src/components/menu/test/multiple/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="UTF-8" />
<title>Menu - Multiple</title>
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no, viewport-fit=cover"
/>
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet" />
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet" />
<script src="../../../../../scripts/testing/scripts.js"></script>
<script nomodule src="../../../../../dist/ionic/ionic.js"></script>
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
<script type="module">
import { menuController } from '../../../../dist/ionic/index.esm.js';
window.menuController = menuController;
</script>
</head>

<body>
<ion-app>
<ion-menu side="start" menu-id="primary-menu" id="primary-menu" content-id="main">
<ion-header>
<ion-toolbar color="primary">
<ion-title>Primary</ion-title>
</ion-toolbar>
</ion-header>
<ion-content class="ion-padding"> Menu Content </ion-content>
</ion-menu>

<ion-menu side="start" menu-id="secondary-menu" id="secondary-menu" content-id="main">
<ion-header>
<ion-toolbar color="secondary">
<ion-title>Secondary</ion-title>
</ion-toolbar>
</ion-header>
<ion-content class="ion-padding"> Menu Content </ion-content>
</ion-menu>

<ion-menu disabled="true" side="end" menu-id="success-menu" id="success-menu" content-id="main">
<ion-header>
<ion-toolbar color="success">
<ion-title>Success</ion-title>
</ion-toolbar>
</ion-header>
<ion-content class="ion-padding"> Menu Content </ion-content>
</ion-menu>

<ion-menu disabled="true" side="end" menu-id="danger-menu" id="danger-menu" content-id="main">
<ion-header>
<ion-toolbar color="danger">
<ion-title>Danger</ion-title>
</ion-toolbar>
</ion-header>
<ion-content class="ion-padding"> Menu Content </ion-content>
</ion-menu>

<div class="ion-page" id="main">
<ion-header>
<ion-toolbar>
<ion-buttons slot="start">
<ion-menu-button menu="primary-menu" color="primary"></ion-menu-button>
<ion-menu-button menu="secondary-menu" color="secondary"></ion-menu-button>
</ion-buttons>
<ion-title>Menu - Multiple</ion-title>
</ion-toolbar>
</ion-header>
<ion-content class="ion-padding">Content</ion-content>
</div>
</ion-app>
</body>
<scrip
</html>
79 changes: 79 additions & 0 deletions core/src/components/menu/test/multiple/menu.e2e.ts
Original file line number Diff line number Diff line change
@@ -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'), () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include an annotation for the issue. Not sure if it can go on the describe level or it should be inside the beforeEach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reminder! Added in 5570448 and 90425eb

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);
});
});
});
58 changes: 38 additions & 20 deletions core/src/utils/menu-controller/index.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have the work tracked to update the docs in light of this change, particularly the Multiple Menus section?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reminder! I meant to do this as part of my work, but I forgot. Here's the PR: ionic-team/ionic-docs#3186

Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -12,23 +14,23 @@ const createMenuController = () => {
const menus: MenuI[] = [];

const open = async (menu?: string | null): Promise<boolean> => {
const menuEl = await get(menu);
const menuEl = await get(menu, true);
if (menuEl) {
return menuEl.open();
}
return false;
};

const close = async (menu?: string | null): Promise<boolean> => {
const menuEl = await (menu !== undefined ? get(menu) : getOpen());
const menuEl = await (menu !== undefined ? get(menu, true) : getOpen());
if (menuEl !== undefined) {
return menuEl.close();
}
return false;
};

const toggle = async (menu?: string | null): Promise<boolean> => {
const menuEl = await get(menu);
const menuEl = await get(menu, true);
if (menuEl) {
return menuEl.toggle();
}
Expand Down Expand Up @@ -70,20 +72,48 @@ const createMenuController = () => {
return false;
};

const get = async (menu?: string | null): Promise<HTMLIonMenuElement | undefined> => {
/**
* 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<HTMLIonMenuElement | undefined> => {
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"
Expand Down Expand Up @@ -131,9 +161,6 @@ const createMenuController = () => {

const _register = (menu: MenuI) => {
if (menus.indexOf(menu) < 0) {
if (!menu.disabled) {
_setActiveMenu(menu);
}
menus.push(menu);
}
};
Expand All @@ -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<boolean> => {
if (isAnimatingSync()) {
return false;
Expand Down Expand Up @@ -238,7 +257,6 @@ const createMenuController = () => {
_register,
_unregister,
_setOpen,
_setActiveMenu,
};
};

Expand Down