Skip to content

Commit

Permalink
fix(menu): emit ionMenuChange when re-mounted (#28049)
Browse files Browse the repository at this point in the history 
Issue number: resolves #28030

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

`ion-menu` registers itself with the menu controller when
`connectedCallback` fires and then unregisters itself when
`disconnectedCallback` fires. When the menu was removed from the DOM,
`disconnectedCallback` was not always being fired due to
ionic-team/stencil#4070.

`ion-menu-button` checks to see if it should be visible by grabbing the
current menu in
https://github.com/ionic-team/ionic-framework/blob/314055cf7a66a58e4e88c22e6e755fadd494ed70/core/src/components/menu-button/menu-button.tsx#L74.

Since `disconnectedCallback` was not being fired, `ion-menu-button`
would still find the menu even when it was no longer in the DOM. In this
case, the menu was not being unregistered due to `disconnectedCallback`
not firing.

When the linked Stencil bug was resolved in Stencil 4.0.3, the menu
button started to disappear. This is happening because
`disconnectedCallback` is now correctly called when the menu is removed
from the DOM. Since `disconnectedCallback` is called, the menu is
un-registered from the menu controller, and the menu button can no
longer find it.

However, this revealed a long-standing bug where re-adding the menu
would not fire `ionMenuChange` again. As a result, the menu button
remained hidden.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- The menu now fires `ionMenuChange` on `connectedCallback` as long as
`componentDidLoad` has already been run.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Dev build: `7.3.2-dev.11692803611.15c1bc87`

---------

Co-authored-by: Sean Perkins <[email protected]>
  • Loading branch information
@liamdebeasi @sean-perkins
liamdebeasi and sean-perkins authored Aug 24, 2023
1 parent f450f0a commit fa41131
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 1 deletion.
64 changes: 64 additions & 0 deletions core/src/components/menu-button/test/async/index.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="UTF-8" />
<title>Menu - Async</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>
<div id="menu-container">
<ion-menu content-id="main">
<ion-content class="ion-padding"> Menu Content </ion-content>
</ion-menu>
</div>

<div class="ion-page" id="main">
<ion-header>
<ion-toolbar>
<ion-title>Menu - Async</ion-title>
<ion-buttons slot="start" id="buttons-container"></ion-buttons>
</ion-toolbar>
</ion-header>
<ion-content class="ion-padding">
Main Content
<button onclick="trigger()" id="trigger">Add Menu To DOM</button>
</ion-content>
</div>
</ion-app>

<script>
const buttons = document.querySelector('ion-buttons');
const menuButton = document.createElement('ion-menu-button');
const menu = document.querySelector('ion-menu');
const menuContainer = document.querySelector('#menu-container');

let firstLoad = true;

// When the menu loads, immediately remove it from the DOM
document.body.addEventListener('ionMenuChange', () => {
if (firstLoad) {
menuContainer.removeChild(menu);
buttons.appendChild(menuButton);
firstLoad = false;
}
});
const trigger = () => {
menuContainer.append(menu);
};
</script>
</body>
</html>
25 changes: 25 additions & 0 deletions core/src/components/menu-button/test/async/menu-button.e2e.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
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 button: async'), () => {
test('menu button should be visible if menu is moved', async ({ page }) => {
await page.goto(`/src/components/menu-button/test/async`, config);

const menu = page.locator('ion-menu');
const menuButton = page.locator('ion-menu-button');
const triggerButton = page.locator('#trigger');

await expect(menu).not.toBeAttached();
await expect(menuButton).toBeHidden();

await triggerButton.click();

await expect(menu).toBeAttached();
await expect(menuButton).toBeVisible();
});
});
});
16 changes: 15 additions & 1 deletion core/src/components/menu/menu.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export class Menu implements ComponentInterface, MenuI {
private lastOnEnd = 0;
private gesture?: Gesture;
private blocker = GESTURE_CONTROLLER.createBlocker({ disableScroll: true });
private didLoad = false;

isAnimating = false;
width!: number;
Expand Down Expand Up @@ -216,6 +217,7 @@ export class Menu implements ComponentInterface, MenuI {

// register this menu with the app's menu controller
menuController._register(this);
this.menuChanged();

this.gesture = (await import('../../utils/gesture')).createGesture({
el: document,
Expand All @@ -237,10 +239,22 @@ export class Menu implements ComponentInterface, MenuI {
}

async componentDidLoad() {
this.ionMenuChange.emit({ disabled: this.disabled, open: this._isOpen });
this.didLoad = true;
this.menuChanged();
this.updateState();
}

private menuChanged() {
/**
* Inform dependent components such as ion-menu-button
* that the menu is ready. Note that we only want to do this
* once the menu has been rendered which is why we check for didLoad.
*/
if (this.didLoad) {
this.ionMenuChange.emit({ disabled: this.disabled, open: this._isOpen });
}
}

async disconnectedCallback() {
/**
* The menu should be closed when it is
Expand Down

0 comments on commit fa41131

Please sign in to comment.