Skip to content

Commit e4ed617

Browse files
andy9775andy9775
committed
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.
1 parent cb8de61 commit e4ed617

File tree

8 files changed

+210
-19
lines changed

8 files changed

+210
-19
lines changed

‎src/cdk-experimental/menu/context-menu.spec.ts

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import {Component, ViewChild, ElementRef} from '@angular/core';
1+
import {Component, ViewChild, ElementRef, Type} from '@angular/core';
22
import {CdkMenuModule} from './menu-module';
33
import {TestBed, async, ComponentFixture} from '@angular/core/testing';
44
import {CdkMenu} from './menu';
@@ -351,6 +351,27 @@ describe('CdkContextMenuTrigger', () => {
351351
expect(getFileMenu()).not.toBeDefined();
352352
});
353353
});
354+
355+
describe('with shared triggered menu', () => {
356+
/**
357+
* Return a function which builds the given component and renders it.
358+
* @param componentClass the component to create
359+
*/
360+
function createComponent<T>(componentClass: Type<T>) {
361+
return function () {
362+
TestBed.configureTestingModule({
363+
imports: [CdkMenuModule],
364+
declarations: [componentClass],
365+
}).compileComponents();
366+
367+
TestBed.createComponent(componentClass).detectChanges();
368+
};
369+
}
370+
371+
it('should throw an error if context and menubar trigger share a menu', () => {
372+
expect(createComponent(MenuBarAndContextTriggerShareMenu)).toThrowError();
373+
});
374+
});
354375
});
355376

356377
@Component({
@@ -457,3 +478,20 @@ class ContextMenuWithMenuBarAndInlineMenu {
457478
@ViewChild('inline_menu') nativeInlineMenu: ElementRef;
458479
@ViewChild('inline_menu_button') nativeInlineMenuButton: ElementRef;
459480
}
481+
482+
@Component({
483+
template: `
484+
<div cdkMenuBar>
485+
<button cdkMenuItem [cdkMenuTriggerFor]="menu">First</button>
486+
</div>
487+
488+
<div [cdkContextMenuTriggerFor]="menu"></div>
489+
490+
<ng-template cdkMenuPanel #menu="cdkMenuPanel">
491+
<div cdkMenu [cdkMenuPanel]="menu">
492+
<button cdkMenuItem></button>
493+
</div>
494+
</ng-template>
495+
`,
496+
})
497+
class MenuBarAndContextTriggerShareMenu {}

‎src/cdk-experimental/menu/context-menu.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
Inject,
1818
Injectable,
1919
InjectionToken,
20+
isDevMode,
2021
} from '@angular/core';
2122
import {DOCUMENT} from '@angular/common';
2223
import {Directionality} from '@angular/cdk/bidi';
@@ -33,6 +34,7 @@ import {fromEvent, merge, Subject} from 'rxjs';
3334
import {takeUntil} from 'rxjs/operators';
3435
import {CdkMenuPanel} from './menu-panel';
3536
import {MenuStack, MenuStackItem} from './menu-stack';
37+
import {throwExistingMenuStackError} from './menu-errors';
3638

3739
/**
3840
* Check if the given element is part of the cdk menu module or nested within a cdk menu element.
@@ -108,6 +110,10 @@ export class CdkContextMenuTrigger implements OnDestroy {
108110
return this._menuPanel;
109111
}
110112
set menuPanel(panel: CdkMenuPanel) {
113+
// If the provided panel already has a stack, that means it already has a trigger configured
114+
if (panel._menuStack && isDevMode()) {
115+
throwExistingMenuStackError();
116+
}
111117
this._menuPanel = panel;
112118

113119
if (this._menuPanel) {
@@ -324,6 +330,7 @@ export class CdkContextMenuTrigger implements OnDestroy {
324330

325331
ngOnDestroy() {
326332
this._destroyOverlay();
333+
this._resetPanelMenuStack();
327334

328335
this._destroyed.next();
329336
this._destroyed.complete();
@@ -337,5 +344,18 @@ export class CdkContextMenuTrigger implements OnDestroy {
337344
}
338345
}
339346

347+
/** Set the menu panels menu stack back to null. */
348+
private _resetPanelMenuStack() {
349+
// If a ContextMenuTrigger is placed in a conditionally rendered view, each time the trigger is
350+
// rendered the panel setter for ContextMenuTrigger is called. From the first render onward,
351+
// the attached CdkMenuPanel has the MenuStack set. Since we throw an error if a panel already
352+
// has a stack set, we want to reset the attached stack here to prevent the error from being
353+
// thrown if the trigger re-configures its attached panel (in the case where there is a 1:1
354+
// relationship between the panel and trigger).
355+
if (this._menuPanel) {
356+
this._menuPanel._menuStack = null;
357+
}
358+
}
359+
340360
static ngAcceptInputType_disabled: BooleanInput;
341361
}

‎src/cdk-experimental/menu/menu-errors.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
/**
10+
* Throws an exception when a menu panel already has a menu stack.
11+
* @docs-private
12+
*/
13+
export function throwExistingMenuStackError() {
14+
throw Error(
15+
'CdkMenuPanel is already referenced by different CdkMenuTrigger. Ensure that a menu is' +
16+
' opened by a single trigger only.'
17+
);
18+
}

‎src/cdk-experimental/menu/menu-item-trigger.spec.ts

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,12 @@
1-
import {Component, ViewChildren, QueryList, ElementRef} from '@angular/core';
1+
import {
2+
Component,
3+
ViewChildren,
4+
QueryList,
5+
ElementRef,
6+
Type,
7+
AfterContentInit,
8+
ViewChild,
9+
} from '@angular/core';
210
import {ComponentFixture, TestBed, async} from '@angular/core/testing';
311
import {By} from '@angular/platform-browser';
412
import {CdkMenuModule} from './menu-module';
@@ -293,6 +301,35 @@ describe('MenuItemTrigger', () => {
293301
.toEqual(Math.floor(nativeMenus[1].getBoundingClientRect().top));
294302
});
295303
});
304+
305+
describe('with shared triggered menu', () => {
306+
/**
307+
* Return a function which builds the given component and renders it.
308+
* @param componentClass the component to create
309+
*/
310+
function createComponent<T>(componentClass: Type<T>) {
311+
return function () {
312+
TestBed.configureTestingModule({
313+
imports: [CdkMenuModule],
314+
declarations: [componentClass],
315+
}).compileComponents();
316+
317+
TestBed.createComponent(componentClass).detectChanges();
318+
};
319+
}
320+
321+
it('should throw an error if two triggers in different menubars open the same menu', () => {
322+
expect(createComponent(TriggersWithSameMenuDifferentMenuBars)).toThrowError();
323+
});
324+
325+
it('should throw an error if two triggers in the same menubar open the same menu', () => {
326+
expect(createComponent(TriggersWithSameMenuSameMenuBar)).toThrowError();
327+
});
328+
329+
it('should throw an error if a trigger in a submenu references its parent menu', () => {
330+
expect(createComponent(TriggerOpensItsMenu)).toThrowError();
331+
});
332+
});
296333
});
297334

298335
@Component({
@@ -331,3 +368,59 @@ class MenuBarWithNestedSubMenus {
331368

332369
@ViewChildren(CdkMenuItem) menuItems: QueryList<CdkMenuItem>;
333370
}
371+
372+
@Component({
373+
template: `
374+
<div cdkMenuBar>
375+
<button cdkMenuItem [cdkMenuTriggerFor]="menu">First</button>
376+
</div>
377+
<div cdkMenuBar>
378+
<button cdkMenuItem [cdkMenuTriggerFor]="menu">Second</button>
379+
</div>
380+
381+
<ng-template cdkMenuPanel #menu="cdkMenuPanel">
382+
<div cdkMenu [cdkMenuPanel]="menu">
383+
<button cdkMenuItem></button>
384+
</div>
385+
</ng-template>
386+
`,
387+
})
388+
class TriggersWithSameMenuDifferentMenuBars {}
389+
390+
@Component({
391+
template: `
392+
<div cdkMenuBar>
393+
<button cdkMenuItem [cdkMenuTriggerFor]="menu">First</button>
394+
<button cdkMenuItem [cdkMenuTriggerFor]="menu">Second</button>
395+
</div>
396+
397+
<ng-template cdkMenuPanel #menu="cdkMenuPanel">
398+
<div cdkMenu [cdkMenuPanel]="menu">
399+
<button cdkMenuItem></button>
400+
</div>
401+
</ng-template>
402+
`,
403+
})
404+
class TriggersWithSameMenuSameMenuBar {}
405+
406+
@Component({
407+
template: `
408+
<div cdkMenuBar>
409+
<button cdkMenuItem [cdkMenuTriggerFor]="menu"></button>
410+
</div>
411+
412+
<ng-template cdkMenuPanel #menu="cdkMenuPanel">
413+
<div cdkMenu [cdkMenuPanel]="menu">
414+
<button cdkMenuItem [cdkMenuTriggerFor]="menu"></button>
415+
</div>
416+
</ng-template>
417+
`,
418+
})
419+
class TriggerOpensItsMenu implements AfterContentInit {
420+
@ViewChild(CdkMenuItem, {read: ElementRef}) trigger: ElementRef<HTMLElement>;
421+
422+
/** Immediately open the menu from the menu bar trigger. */
423+
ngAfterContentInit() {
424+
this.trigger.nativeElement.click();
425+
}
426+
}

‎src/cdk-experimental/menu/menu-item-trigger.ts

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
Inject,
1717
OnDestroy,
1818
Optional,
19+
isDevMode,
1920
} from '@angular/core';
2021
import {Directionality} from '@angular/cdk/bidi';
2122
import {TemplatePortal} from '@angular/cdk/portal';
@@ -30,6 +31,7 @@ import {SPACE, ENTER, RIGHT_ARROW, LEFT_ARROW, DOWN_ARROW, UP_ARROW} from '@angu
3031
import {CdkMenuPanel} from './menu-panel';
3132
import {Menu, CDK_MENU} from './menu-interface';
3233
import {FocusNext} from './menu-stack';
34+
import {throwExistingMenuStackError} from './menu-errors';
3335

3436
/**
3537
* A directive to be combined with CdkMenuItem which opens the Menu it is bound to. If the
@@ -59,8 +61,14 @@ export class CdkMenuItemTrigger implements OnDestroy {
5961
return this._menuPanel;
6062
}
6163
set menuPanel(panel: CdkMenuPanel | undefined) {
62-
this._menuPanel = panel;
64+
// If the provided panel already has a stack, that means it already has a trigger configured.
65+
// Note however that there are some edge cases where two triggers **may** share the same menu,
66+
// e.g. two triggers in two separate menus.
67+
if (panel?._menuStack && isDevMode()) {
68+
throwExistingMenuStackError();
69+
}
6370

71+
this._menuPanel = panel;
6472
if (this._menuPanel) {
6573
this._menuPanel._menuStack = this._getMenuStack();
6674
}
@@ -140,7 +148,7 @@ export class CdkMenuItemTrigger implements OnDestroy {
140148
*/
141149
_toggleOnMouseEnter() {
142150
const menuStack = this._getMenuStack();
143-
if (!menuStack.isEmpty() && !this.isMenuOpen()) {
151+
if (!menuStack?.isEmpty() && !this.isMenuOpen()) {
144152
this._closeSiblingTriggers();
145153
this.openMenu();
146154
}
@@ -165,7 +173,7 @@ export class CdkMenuItemTrigger implements OnDestroy {
165173
if (this._isParentVertical()) {
166174
event.preventDefault();
167175
if (this._directionality?.value === 'rtl') {
168-
this._getMenuStack().close(this._parentMenu, FocusNext.currentItem);
176+
this._getMenuStack()?.close(this._parentMenu, FocusNext.currentItem);
169177
} else {
170178
this.openMenu();
171179
this.menuPanel?._menu?.focusFirstItem('keyboard');
@@ -180,7 +188,7 @@ export class CdkMenuItemTrigger implements OnDestroy {
180188
this.openMenu();
181189
this.menuPanel?._menu?.focusFirstItem('keyboard');
182190
} else {
183-
this._getMenuStack().close(this._parentMenu, FocusNext.currentItem);
191+
this._getMenuStack()?.close(this._parentMenu, FocusNext.currentItem);
184192
}
185193
}
186194
break;
@@ -206,10 +214,10 @@ export class CdkMenuItemTrigger implements OnDestroy {
206214
// that means that the parent menu is a menu bar since we don't put the menu bar on the
207215
// stack
208216
const isParentMenuBar =
209-
!menuStack.closeSubMenuOf(this._parentMenu) && menuStack.peek() !== this._parentMenu;
217+
!menuStack?.closeSubMenuOf(this._parentMenu) && menuStack?.peek() !== this._parentMenu;
210218

211219
if (isParentMenuBar) {
212-
menuStack.closeAll();
220+
menuStack?.closeAll();
213221
}
214222
}
215223

@@ -278,6 +286,20 @@ export class CdkMenuItemTrigger implements OnDestroy {
278286

279287
ngOnDestroy() {
280288
this._destroyOverlay();
289+
this._resetPanelMenuStack();
290+
}
291+
292+
/** Set the menu panels menu stack back to null. */
293+
private _resetPanelMenuStack() {
294+
// If a CdkMenuTrigger is placed in a submenu, each time the trigger is rendered (its parent
295+
// menu is opened) the panel setter for CdkMenuPanel is called. From the first render onward,
296+
// the attached CdkMenuPanel has the MenuStack set. Since we throw an error if a panel already
297+
// has a stack set, we want to reset the attached stack here to prevent the error from being
298+
// thrown if the trigger re-configures its attached panel (in the case where there is a 1:1
299+
// relationship between the panel and trigger).
300+
if (this._menuPanel) {
301+
this._menuPanel._menuStack = null;
302+
}
281303
}
282304

283305
/** Destroy and unset the overlay reference it if exists */

‎src/cdk-experimental/menu/menu-item.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, OnDestroy
125125
}
126126

127127
// don't set the tabindex if there are no open sibling or parent menus
128-
if (!event || (event && !this._getMenuStack().isEmpty())) {
128+
if (!event || (event && !this._getMenuStack()?.isEmpty())) {
129129
this._tabindex = 0;
130130
}
131131
}
@@ -142,7 +142,7 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, OnDestroy
142142
trigger() {
143143
if (!this.disabled && !this.hasMenu()) {
144144
this.triggered.next();
145-
this._getMenuStack().closeAll();
145+
this._getMenuStack()?.closeAll();
146146
}
147147
}
148148

@@ -202,17 +202,17 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, OnDestroy
202202
if (this._isParentVertical() && !this.hasMenu()) {
203203
event.preventDefault();
204204
this._dir?.value === 'rtl'
205-
? this._getMenuStack().close(this._parentMenu, FocusNext.previousItem)
206-
: this._getMenuStack().closeAll(FocusNext.nextItem);
205+
? this._getMenuStack()?.close(this._parentMenu, FocusNext.previousItem)
206+
: this._getMenuStack()?.closeAll(FocusNext.nextItem);
207207
}
208208
break;
209209

210210
case LEFT_ARROW:
211211
if (this._isParentVertical() && !this.hasMenu()) {
212212
event.preventDefault();
213213
this._dir?.value === 'rtl'
214-
? this._getMenuStack().closeAll(FocusNext.nextItem)
215-
: this._getMenuStack().close(this._parentMenu, FocusNext.previousItem);
214+
? this._getMenuStack()?.closeAll(FocusNext.nextItem)
215+
: this._getMenuStack()?.close(this._parentMenu, FocusNext.previousItem);
216216
}
217217
break;
218218
}
@@ -226,11 +226,11 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, OnDestroy
226226
this._ngZone.runOutsideAngular(() =>
227227
fromEvent(this._elementRef.nativeElement, 'mouseenter')
228228
.pipe(
229-
filter(() => !this._getMenuStack().isEmpty() && !this.hasMenu()),
229+
filter(() => !this._getMenuStack()?.isEmpty() && !this.hasMenu()),
230230
takeUntil(this._destroyed)
231231
)
232232
.subscribe(() => {
233-
this._ngZone.run(() => this._getMenuStack().closeSubMenuOf(this._parentMenu));
233+
this._ngZone.run(() => this._getMenuStack()?.closeSubMenuOf(this._parentMenu));
234234
})
235235
);
236236
}

0 commit comments

Comments
 (0)