fix(menu): do not error if disabled or swipeGesture is changed mid-animation (#28268)

Issue number: resolves #20092, resolves #19676, resolves #19000

---------

<!-- 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. -->

Menu is currently throwing errors because it expects no animations to be
running when any state changes happen (such as changing `disabled` or
`swipeGesture`).

For example, if you set `swipeGesture="false"` mid-gesture then the menu
will error. Alternatively, if you set `disabled="true"` mid-open
animation then the menu will error also. This is undesirable because it
can cause visual flickering and other undesirable behaviors as noted in
the linked threads.

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

- Any in-progress animation is cancelled if the state updates such that
the animation is no longer relevant (i.e. `disabled` is set to `true`
while the menu is opening)
- Removed relevant assertions
- Added tests

## 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.4.3-dev.11696264821.1755dd6a`

Author: liamdebeasi

core/src/components/menu/menu.tsx

@@ -40,6 +40,15 @@ export class Menu implements ComponentInterface, MenuI {
   private blocker = GESTURE_CONTROLLER.createBlocker({ disableScroll: true });
   private didLoad = false;
 
+  /**
+   * Flag used to determine if an open/close
+   * operation was cancelled. For example, if
+   * an app calls "menu.open" then disables the menu
+   * part way through the animation, then this would
+   * be considered a cancelled operation.
+   */
+  private operationCancelled = false;
+
   isAnimating = false;
   width!: number;
   _isOpen = false;
@@ -432,6 +441,17 @@ export class Menu implements ComponentInterface, MenuI {
 
     await this.loadAnimation();
     await this.startAnimation(shouldOpen, animated);
+
+    /**
+     * If the animation was cancelled then
+     * return false because the operation
+     * did not succeed.
+     */
+    if (this.operationCancelled) {
+      this.operationCancelled = false;
+      return false;
+    }
+
     this.afterAnimation(shouldOpen);
 
     return true;
@@ -472,18 +492,24 @@ export class Menu implements ComponentInterface, MenuI {
     const easingReverse = mode === 'ios' ? iosEasingReverse : mdEasingReverse;
     const ani = (this.animation as Animation)!
       .direction(isReversed ? 'reverse' : 'normal')
-      .easing(isReversed ? easingReverse : easing)
-      .onFinish(() => {
-        if (ani.getDirection() === 'reverse') {
-          ani.direction('normal');
-        }
-      });
+      .easing(isReversed ? easingReverse : easing);
 
     if (animated) {
       await ani.play();
     } else {
       ani.play({ sync: true });
     }
+
+    /**
+     * We run this after the play invocation
+     * instead of using ani.onFinish so that
+     * multiple onFinish callbacks do not get
+     * run if an animation is played, stopped,
+     * and then played again.
+     */
+    if (ani.getDirection() === 'reverse') {
+      ani.direction('normal');
+    }
   }
 
   private _isActive() {
@@ -643,8 +669,6 @@ export class Menu implements ComponentInterface, MenuI {
   }
 
   private afterAnimation(isOpen: boolean) {
-    assert(this.isAnimating, '_before() should be called while animating');
-
     // keep opening/closing the menu disabled for a touch more yet
     // only add listeners/css if it's enabled and isOpen
     // and only remove listeners/css if it's not open
@@ -713,10 +737,30 @@ export class Menu implements ComponentInterface, MenuI {
       this.gesture.enable(isActive && this.swipeGesture);
     }
 
-    // Close menu immediately
-    if (!isActive && this._isOpen) {
-      // close if this menu is open, and should not be enabled
-      this.forceClosing();
+    /**
+     * If the menu is disabled but it is still open
+     * then we should close the menu immediately.
+     * Additionally, if the menu is in the process
+     * of animating {open, close} and the menu is disabled
+     * then it should still be closed immediately.
+     */
+    if (!isActive) {
+      /**
+       * It is possible to disable the menu while
+       * it is mid-animation. When this happens, we
+       * need to set the operationCancelled flag
+       * so that this._setOpen knows to return false
+       * and not run the "afterAnimation" callback.
+       */
+      if (this.isAnimating) {
+        this.operationCancelled = true;
+      }
+
+      /**
+       * If the menu is disabled then we should
+       * forcibly close the menu even if it is open.
+       */
+      this.afterAnimation(false);
     }
 
     if (doc?.contains(this.el)) {
@@ -730,19 +774,6 @@ export class Menu implements ComponentInterface, MenuI {
         menuController._setActiveMenu(this);
       }
     }
-
-    assert(!this.isAnimating, 'can not be animating');
-  }
-
-  private forceClosing() {
-    assert(this._isOpen, 'menu cannot be closed');
-
-    this.isAnimating = true;
-
-    const ani = (this.animation as Animation)!.direction('reverse');
-    ani.play({ sync: true });
-
-    this.afterAnimation(false);
   }
 
   render() {

core/src/components/menu/test/disable/index.html

@@ -0,0 +1,41 @@
+<!DOCTYPE html>
+<html lang="en" dir="ltr">
+  <head>
+    <meta charset="UTF-8" />
+    <title>Menu - Disable</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>
+  </head>
+
+  <body>
+    <ion-app>
+      <ion-menu side="start" id="start-menu" menu-id="start-menu" content-id="main">
+        <ion-header>
+          <ion-toolbar color="primary">
+            <ion-title>Menu</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></ion-menu-button>
+            </ion-buttons>
+            <ion-title>Menu - Disable</ion-title>
+          </ion-toolbar>
+        </ion-header>
+        <ion-content class="ion-padding">Content</ion-content>
+      </div>
+    </ion-app>
+  </body>
+</html>

core/src/components/menu/test/disable/menu.e2e.ts

@@ -0,0 +1,66 @@
+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/disable`, config);
+    });
+
+    test('should disable when menu is fully open', async ({ page }) => {
+      const logs: string[] = [];
+
+      page.on('console', (msg) => {
+        if (msg.type() === 'error') {
+          logs.push(msg.text());
+        }
+      });
+
+      const menu = page.locator('ion-menu');
+
+      // Should be visible on initial presentation
+      await menu.evaluate((el: HTMLIonMenuElement) => el.open());
+      await expect(menu).toBeVisible();
+
+      // Disabling menu should hide it
+      await menu.evaluate((el: HTMLIonMenuElement) => (el.disabled = true));
+      await expect(menu).toBeHidden();
+
+      // Re-enabling menu and opening it show make it visible
+      await menu.evaluate((el: HTMLIonMenuElement) => (el.disabled = false));
+      await menu.evaluate((el: HTMLIonMenuElement) => el.open());
+      await expect(menu).toBeVisible();
+
+      expect(logs.length).toBe(0);
+    });
+
+    test('should disable when menu is animating', async ({ page }) => {
+      const logs: string[] = [];
+
+      page.on('console', (msg) => {
+        if (msg.type() === 'error') {
+          logs.push(msg.text());
+        }
+      });
+
+      const menu = page.locator('ion-menu');
+
+      // Opening and quickly disabling menu should hide it
+      menu.evaluate((el: HTMLIonMenuElement) => {
+        el.open();
+        setTimeout(() => (el.disabled = true), 0);
+      });
+      await expect(menu).toBeHidden();
+
+      // Re-enabling menu and opening it show make it visible
+      await menu.evaluate((el: HTMLIonMenuElement) => (el.disabled = false));
+      await menu.evaluate((el: HTMLIonMenuElement) => el.open());
+      await expect(menu).toBeVisible();
+
+      expect(logs.length).toBe(0);
+    });
+  });
+});