diff --git a/src/cdk-experimental/menu/context-menu.spec.ts b/src/cdk-experimental/menu/context-menu.spec.ts index 3e1b3d2c9a18..f9a0cc5a2c27 100644 --- 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(componentClass: Type) { + 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: ` +
+ +
+ +
+ + +
+ +
+
+ `, +}) +class MenuBarAndContextTriggerShareMenu {} diff --git a/src/cdk-experimental/menu/context-menu.ts b/src/cdk-experimental/menu/context-menu.ts index 4ac2526ea035..8fbc1ade6415 100644 --- 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 new file mode 100644 index 000000000000..c22a07e2da9b --- /dev/null +++ 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 index 879ad7f53314..deb982e5f226 100644 --- 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(componentClass: Type) { + 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; } + +@Component({ + template: ` +
+ +
+
+ +
+ + +
+ +
+
+ `, +}) +class TriggersWithSameMenuDifferentMenuBars {} + +@Component({ + template: ` +
+ + +
+ + +
+ +
+
+ `, +}) +class TriggersWithSameMenuSameMenuBar {} + +// TODO uncomment once we figure out why this is failing in Ivy +// @Component({ +// template: ` +//
+// +//
+ +// +//
+// +//
+//
+// `, +// }) +// class TriggerOpensItsMenu implements AfterViewInit { +// @ViewChild(CdkMenuItem, {read: ElementRef}) trigger: ElementRef; + +// 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 index 8d7fabb541c9..a2ede3f9d879 100644 --- 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 index 76e12a982153..0059f267a3e9 100644 --- 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,8 +202,8 @@ 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; @@ -211,8 +211,8 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, OnDestroy 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)); }) ); } diff --git a/src/cdk-experimental/menu/menu-panel.ts b/src/cdk-experimental/menu/menu-panel.ts index f22b880f1069..c6ea88b1cf3a 100644 --- a/src/cdk-experimental/menu/menu-panel.ts +++ b/src/cdk-experimental/menu/menu-panel.ts @@ -20,7 +20,7 @@ export class CdkMenuPanel { _menu?: Menu; /** Keep track of open Menus. */ - _menuStack: MenuStack; + _menuStack: MenuStack | null; constructor(readonly _templateRef: TemplateRef) {} @@ -35,6 +35,6 @@ export class CdkMenuPanel { // inject the menu stack reference into the child menu and menu items, however this isn't // possible at this time. this._menu._menuStack = this._menuStack; - this._menuStack.push(child); + this._menuStack?.push(child); } } diff --git a/src/cdk-experimental/menu/menu-stack.ts b/src/cdk-experimental/menu/menu-stack.ts index 6d231e235a4a..47ad9a2f7272 100644 --- a/src/cdk-experimental/menu/menu-stack.ts +++ b/src/cdk-experimental/menu/menu-stack.ts @@ -20,7 +20,7 @@ export const enum FocusNext { */ export interface MenuStackItem { /** A reference to the previous Menus MenuStack instance. */ - _menuStack: MenuStack; + _menuStack: MenuStack | null; } /**