fix(cdk-experimental/menu): do not allow two separate triggers to ope…

…n the same menu (#20300) Fixes the error where two different menu triggers open the same menu which leads to open menus not closing out on hover and other inconsistent behaviour. Opt to throw and error and not allow two triggers to share a menu.
angular · Aug 18, 2020 · 6948186 · 6948186
1 parent 3396b4b
commit 6948186
Show file tree

Hide file tree

Showing 8 changed files with 217 additions and 19 deletions.
diff --git a/src/cdk-experimental/menu/context-menu.spec.ts b/src/cdk-experimental/menu/context-menu.spec.ts
@@ -1,4 +1,4 @@
-import {Component, ViewChild, ElementRef} from '@angular/core';
+import {Component, ViewChild, ElementRef, Type} from '@angular/core';
 import {CdkMenuModule} from './menu-module';
 import {TestBed, async, ComponentFixture} from '@angular/core/testing';
 import {CdkMenu} from './menu';
@@ -351,6 +351,29 @@ describe('CdkContextMenuTrigger', () => {
       expect(getFileMenu()).not.toBeDefined();
     });
   });
+
+  describe('with shared triggered menu', () => {
+    /**
+     * Return a function which builds the given component and renders it.
+     * @param componentClass the component to create
+     */
+    function createComponent<T>(componentClass: Type<T>) {
+      return function () {
+        TestBed.configureTestingModule({
+          imports: [CdkMenuModule],
+          declarations: [componentClass],
+        }).compileComponents();
+
+        TestBed.createComponent(componentClass).detectChanges();
+      };
+    }
+
+    it('should throw an error if context and menubar trigger share a menu', () => {
+      expect(createComponent(MenuBarAndContextTriggerShareMenu)).toThrowError(
+        /CdkMenuPanel is already referenced by different CdkMenuTrigger/
+      );
+    });
+  });
 });
 
 @Component({
@@ -457,3 +480,20 @@ class ContextMenuWithMenuBarAndInlineMenu {
   @ViewChild('inline_menu') nativeInlineMenu: ElementRef;
   @ViewChild('inline_menu_button') nativeInlineMenuButton: ElementRef;
 }
+
+@Component({
+  template: `
+    <div cdkMenuBar>
+      <button cdkMenuItem [cdkMenuTriggerFor]="menu">First</button>
+    </div>
+
+    <div [cdkContextMenuTriggerFor]="menu"></div>
+
+    <ng-template cdkMenuPanel #menu="cdkMenuPanel">
+      <div cdkMenu [cdkMenuPanel]="menu">
+        <button cdkMenuItem></button>
+      </div>
+    </ng-template>
+  `,
+})
+class MenuBarAndContextTriggerShareMenu {}
diff --git a/src/cdk-experimental/menu/context-menu.ts b/src/cdk-experimental/menu/context-menu.ts
@@ -17,6 +17,7 @@ import {
   Inject,
   Injectable,
   InjectionToken,
+  isDevMode,
 } from '@angular/core';
 import {DOCUMENT} from '@angular/common';
 import {Directionality} from '@angular/cdk/bidi';
@@ -33,6 +34,7 @@ import {fromEvent, merge, Subject} from 'rxjs';
 import {takeUntil} from 'rxjs/operators';
 import {CdkMenuPanel} from './menu-panel';
 import {MenuStack, MenuStackItem} from './menu-stack';
+import {throwExistingMenuStackError} from './menu-errors';
 
 /**
  * Check if the given element is part of the cdk menu module or nested within a cdk menu element.
@@ -108,6 +110,11 @@ export class CdkContextMenuTrigger implements OnDestroy {
     return this._menuPanel;
   }
   set menuPanel(panel: CdkMenuPanel) {
+    // If the provided panel already has a stack, that means it already has a trigger configured
+    // TODO refactor once https://github.com/angular/components/pull/20146 lands
+    if (isDevMode() && panel._menuStack) {
+      throwExistingMenuStackError();
+    }
     this._menuPanel = panel;
 
     if (this._menuPanel) {
@@ -324,6 +331,7 @@ export class CdkContextMenuTrigger implements OnDestroy {
 
   ngOnDestroy() {
     this._destroyOverlay();
+    this._resetPanelMenuStack();
 
     this._destroyed.next();
     this._destroyed.complete();
@@ -337,5 +345,18 @@ export class CdkContextMenuTrigger implements OnDestroy {
     }
   }
 
+  /** Set the menu panels menu stack back to null. */
+  private _resetPanelMenuStack() {
+    // If a ContextMenuTrigger is placed in a conditionally rendered view, each time the trigger is
+    // rendered the panel setter for ContextMenuTrigger is called. From the first render onward,
+    // the attached CdkMenuPanel has the MenuStack set. Since we throw an error if a panel already
+    // has a stack set, we want to reset the attached stack here to prevent the error from being
+    // thrown if the trigger re-configures its attached panel (in the case where there is a 1:1
+    // relationship between the panel and trigger).
+    if (this._menuPanel) {
+      this._menuPanel._menuStack = null;
+    }
+  }
+
   static ngAcceptInputType_disabled: BooleanInput;
 }
diff --git a/src/cdk-experimental/menu/menu-errors.ts b/src/cdk-experimental/menu/menu-errors.ts
@@ -0,0 +1,18 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+/**
+ * Throws an exception when a menu panel already has a menu stack.
+ * @docs-private
+ */
+export function throwExistingMenuStackError() {
+  throw Error(
+    'CdkMenuPanel is already referenced by different CdkMenuTrigger. Ensure that a menu is' +
+      ' opened by a single trigger only.'
+  );
+}
diff --git a/src/cdk-experimental/menu/menu-item-trigger.spec.ts b/src/cdk-experimental/menu/menu-item-trigger.spec.ts
@@ -1,4 +1,4 @@
-import {Component, ViewChildren, QueryList, ElementRef} from '@angular/core';
+import {Component, ViewChildren, QueryList, ElementRef, Type} from '@angular/core';
 import {ComponentFixture, TestBed, async} from '@angular/core/testing';
 import {By} from '@angular/platform-browser';
 import {CdkMenuModule} from './menu-module';
@@ -293,6 +293,42 @@ describe('MenuItemTrigger', () => {
         .toEqual(Math.floor(nativeMenus[1].getBoundingClientRect().top));
     });
   });
+
+  describe('with shared triggered menu', () => {
+    /**
+     * Return a function which builds the given component and renders it.
+     * @param componentClass the component to create
+     */
+    function createComponent<T>(componentClass: Type<T>) {
+      return function () {
+        TestBed.configureTestingModule({
+          imports: [CdkMenuModule],
+          declarations: [componentClass],
+        }).compileComponents();
+
+        TestBed.createComponent(componentClass).detectChanges();
+      };
+    }
+
+    it('should throw an error if two triggers in different menubars open the same menu', () => {
+      expect(createComponent(TriggersWithSameMenuDifferentMenuBars)).toThrowError(
+        /CdkMenuPanel is already referenced by different CdkMenuTrigger/
+      );
+    });
+
+    it('should throw an error if two triggers in the same menubar open the same menu', () => {
+      expect(createComponent(TriggersWithSameMenuSameMenuBar)).toThrowError(
+        /CdkMenuPanel is already referenced by different CdkMenuTrigger/
+      );
+    });
+
+    // TODO uncomment once we figure out why this is failing in Ivy
+    // it('should throw an error if a trigger in a submenu references its parent menu', () => {
+    //   expect(createComponent(TriggerOpensItsMenu)).toThrowError(
+    //     /CdkMenuPanel is already referenced by different CdkMenuTrigger/
+    //   );
+    // });
+  });
 });
 
 @Component({
@@ -331,3 +367,62 @@ class MenuBarWithNestedSubMenus {
 
   @ViewChildren(CdkMenuItem) menuItems: QueryList<CdkMenuItem>;
 }
+
+@Component({
+  template: `
+    <div cdkMenuBar>
+      <button cdkMenuItem [cdkMenuTriggerFor]="menu">First</button>
+    </div>
+    <div cdkMenuBar>
+      <button cdkMenuItem [cdkMenuTriggerFor]="menu">Second</button>
+    </div>
+
+    <ng-template cdkMenuPanel #menu="cdkMenuPanel">
+      <div cdkMenu [cdkMenuPanel]="menu">
+        <button cdkMenuItem></button>
+      </div>
+    </ng-template>
+  `,
+})
+class TriggersWithSameMenuDifferentMenuBars {}
+
+@Component({
+  template: `
+    <div cdkMenuBar>
+      <button cdkMenuItem [cdkMenuTriggerFor]="menu">First</button>
+      <button cdkMenuItem [cdkMenuTriggerFor]="menu">Second</button>
+    </div>
+
+    <ng-template cdkMenuPanel #menu="cdkMenuPanel">
+      <div cdkMenu [cdkMenuPanel]="menu">
+        <button cdkMenuItem></button>
+      </div>
+    </ng-template>
+  `,
+})
+class TriggersWithSameMenuSameMenuBar {}
+
+// TODO uncomment once we figure out why this is failing in Ivy
+// @Component({
+//   template: `
+//     <div cdkMenuBar>
+//       <button cdkMenuItem [cdkMenuTriggerFor]="menu"></button>
+//     </div>
+
+//     <ng-template cdkMenuPanel #menu="cdkMenuPanel">
+//       <div cdkMenu [cdkMenuPanel]="menu">
+//         <button cdkMenuItem [cdkMenuTriggerFor]="menu"></button>
+//       </div>
+//     </ng-template>
+//   `,
+// })
+// class TriggerOpensItsMenu implements AfterViewInit {
+//   @ViewChild(CdkMenuItem, {read: ElementRef}) trigger: ElementRef<HTMLButtonElement>;
+
+//   constructor(private readonly _changeDetector: ChangeDetectorRef) {}
+
+//   ngAfterViewInit() {
+//     this.trigger.nativeElement.click();
+//     this._changeDetector.detectChanges();
+//   }
+// }
diff --git a/src/cdk-experimental/menu/menu-item-trigger.ts b/src/cdk-experimental/menu/menu-item-trigger.ts
@@ -16,6 +16,7 @@ import {
   Inject,
   OnDestroy,
   Optional,
+  isDevMode,
 } from '@angular/core';
 import {Directionality} from '@angular/cdk/bidi';
 import {TemplatePortal} from '@angular/cdk/portal';
@@ -30,6 +31,7 @@ import {SPACE, ENTER, RIGHT_ARROW, LEFT_ARROW, DOWN_ARROW, UP_ARROW} from '@angu
 import {CdkMenuPanel} from './menu-panel';
 import {Menu, CDK_MENU} from './menu-interface';
 import {FocusNext} from './menu-stack';
+import {throwExistingMenuStackError} from './menu-errors';
 
 /**
  * A directive to be combined with CdkMenuItem which opens the Menu it is bound to. If the
@@ -59,8 +61,15 @@ export class CdkMenuItemTrigger implements OnDestroy {
     return this._menuPanel;
   }
   set menuPanel(panel: CdkMenuPanel | undefined) {
-    this._menuPanel = panel;
+    // If the provided panel already has a stack, that means it already has a trigger configured.
+    // Note however that there are some edge cases where two triggers **may** share the same menu,
+    // e.g. two triggers in two separate menus.
+    // TODO refactor once https://github.com/angular/components/pull/20146 lands
+    if (isDevMode() && panel?._menuStack) {
+      throwExistingMenuStackError();
+    }
 
+    this._menuPanel = panel;
     if (this._menuPanel) {
       this._menuPanel._menuStack = this._getMenuStack();
     }
@@ -140,7 +149,8 @@ export class CdkMenuItemTrigger implements OnDestroy {
    */
   _toggleOnMouseEnter() {
     const menuStack = this._getMenuStack();
-    if (!menuStack.isEmpty() && !this.isMenuOpen()) {
+    const isSiblingMenuOpen = !menuStack?.isEmpty() && !this.isMenuOpen();
+    if (isSiblingMenuOpen) {
       this._closeSiblingTriggers();
       this.openMenu();
     }
@@ -165,7 +175,7 @@ export class CdkMenuItemTrigger implements OnDestroy {
         if (this._isParentVertical()) {
           event.preventDefault();
           if (this._directionality?.value === 'rtl') {
-            this._getMenuStack().close(this._parentMenu, FocusNext.currentItem);
+            this._getMenuStack()?.close(this._parentMenu, FocusNext.currentItem);
           } else {
             this.openMenu();
             this.menuPanel?._menu?.focusFirstItem('keyboard');
@@ -180,7 +190,7 @@ export class CdkMenuItemTrigger implements OnDestroy {
             this.openMenu();
             this.menuPanel?._menu?.focusFirstItem('keyboard');
           } else {
-            this._getMenuStack().close(this._parentMenu, FocusNext.currentItem);
+            this._getMenuStack()?.close(this._parentMenu, FocusNext.currentItem);
           }
         }
         break;
@@ -206,10 +216,10 @@ export class CdkMenuItemTrigger implements OnDestroy {
     // that means that the parent menu is a menu bar since we don't put the menu bar on the
     // stack
     const isParentMenuBar =
-      !menuStack.closeSubMenuOf(this._parentMenu) && menuStack.peek() !== this._parentMenu;
+      !menuStack?.closeSubMenuOf(this._parentMenu) && menuStack?.peek() !== this._parentMenu;
 
     if (isParentMenuBar) {
-      menuStack.closeAll();
+      menuStack?.closeAll();
     }
   }
 
@@ -278,6 +288,20 @@ export class CdkMenuItemTrigger implements OnDestroy {
 
   ngOnDestroy() {
     this._destroyOverlay();
+    this._resetPanelMenuStack();
+  }
+
+  /** Set the menu panels menu stack back to null. */
+  private _resetPanelMenuStack() {
+    // If a CdkMenuTrigger is placed in a submenu, each time the trigger is rendered (its parent
+    // menu is opened) the panel setter for CdkMenuPanel is called. From the first render onward,
+    // the attached CdkMenuPanel has the MenuStack set. Since we throw an error if a panel already
+    // has a stack set, we want to reset the attached stack here to prevent the error from being
+    // thrown if the trigger re-configures its attached panel (in the case where there is a 1:1
+    // relationship between the panel and trigger).
+    if (this._menuPanel) {
+      this._menuPanel._menuStack = null;
+    }
   }
 
   /** Destroy and unset the overlay reference it if exists */

diff --git a/src/cdk-experimental/menu/menu-item.ts b/src/cdk-experimental/menu/menu-item.ts
@@ -125,7 +125,7 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, OnDestroy
     }
 
     // don't set the tabindex if there are no open sibling or parent menus
-    if (!event || (event && !this._getMenuStack().isEmpty())) {
+    if (!event || !this._getMenuStack()?.isEmpty()) {
       this._tabindex = 0;
     }
   }
@@ -142,7 +142,7 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, OnDestroy
   trigger() {
     if (!this.disabled && !this.hasMenu()) {
       this.triggered.next();
-      this._getMenuStack().closeAll();
+      this._getMenuStack()?.closeAll();
     }
   }
 
@@ -202,17 +202,17 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, OnDestroy
         if (this._isParentVertical() && !this.hasMenu()) {
           event.preventDefault();
           this._dir?.value === 'rtl'
-            ? this._getMenuStack().close(this._parentMenu, FocusNext.previousItem)
-            : this._getMenuStack().closeAll(FocusNext.nextItem);
+            ? this._getMenuStack()?.close(this._parentMenu, FocusNext.previousItem)
+            : this._getMenuStack()?.closeAll(FocusNext.nextItem);
         }
         break;
 
       case LEFT_ARROW:
         if (this._isParentVertical() && !this.hasMenu()) {
           event.preventDefault();
           this._dir?.value === 'rtl'
-            ? this._getMenuStack().closeAll(FocusNext.nextItem)
-            : this._getMenuStack().close(this._parentMenu, FocusNext.previousItem);
+            ? this._getMenuStack()?.closeAll(FocusNext.nextItem)
+            : this._getMenuStack()?.close(this._parentMenu, FocusNext.previousItem);
         }
         break;
     }
@@ -226,11 +226,11 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, OnDestroy
     this._ngZone.runOutsideAngular(() =>
       fromEvent(this._elementRef.nativeElement, 'mouseenter')
         .pipe(
-          filter(() => !this._getMenuStack().isEmpty() && !this.hasMenu()),
+          filter(() => !this._getMenuStack()?.isEmpty() && !this.hasMenu()),
           takeUntil(this._destroyed)
         )
         .subscribe(() => {
-          this._ngZone.run(() => this._getMenuStack().closeSubMenuOf(this._parentMenu));
+          this._ngZone.run(() => this._getMenuStack()?.closeSubMenuOf(this._parentMenu));
         })
     );
   }