From 8eb7939b9fc70bb0dd0a79e76df7c92c7f13a940 Mon Sep 17 00:00:00 2001 From: Andy Chrzaszcz Date: Thu, 13 Aug 2020 11:28:10 -0400 Subject: [PATCH] fix(cdk-experimental/menu): do not allow two separate triggers to open the same menu 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. --- .../menu/context-menu.spec.ts | 42 +++++++- src/cdk-experimental/menu/context-menu.ts | 21 ++++ src/cdk-experimental/menu/menu-errors.ts | 18 ++++ .../menu/menu-item-trigger.spec.ts | 97 ++++++++++++++++++- .../menu/menu-item-trigger.ts | 36 +++++-- src/cdk-experimental/menu/menu-item.ts | 16 +-- src/cdk-experimental/menu/menu-panel.ts | 4 +- src/cdk-experimental/menu/menu-stack.ts | 2 +- 8 files changed, 217 insertions(+), 19 deletions(-) create mode 100644 src/cdk-experimental/menu/menu-errors.ts 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; } /**