Skip to content

Commit 6948186

Browse files
andy9775andy9775
authored
fix(cdk-experimental/menu): do not allow two separate triggers to open 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.
1 parent 3396b4b commit 6948186

File tree

8 files changed

+217
-19
lines changed

8 files changed

+217
-19
lines changed

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

Lines changed: 41 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,29 @@ 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+
/CdkMenuPanel is already referenced by different CdkMenuTrigger/
374+
);
375+
});
376+
});
354377
});
355378

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

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

Lines changed: 21 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,11 @@ 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+
// TODO refactor once https://github.com/angular/components/pull/20146 lands
115+
if (isDevMode() && panel._menuStack) {
116+
throwExistingMenuStackError();
117+
}
111118
this._menuPanel = panel;
112119

113120
if (this._menuPanel) {
@@ -324,6 +331,7 @@ export class CdkContextMenuTrigger implements OnDestroy {
324331

325332
ngOnDestroy() {
326333
this._destroyOverlay();
334+
this._resetPanelMenuStack();
327335

328336
this._destroyed.next();
329337
this._destroyed.complete();
@@ -337,5 +345,18 @@ export class CdkContextMenuTrigger implements OnDestroy {
337345
}
338346
}
339347

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

‎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: 96 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import {Component, ViewChildren, QueryList, ElementRef} from '@angular/core';
1+
import {Component, ViewChildren, QueryList, ElementRef, Type} from '@angular/core';
22
import {ComponentFixture, TestBed, async} from '@angular/core/testing';
33
import {By} from '@angular/platform-browser';
44
import {CdkMenuModule} from './menu-module';
@@ -293,6 +293,42 @@ describe('MenuItemTrigger', () => {
293293
.toEqual(Math.floor(nativeMenus[1].getBoundingClientRect().top));
294294
});
295295
});
296+
297+
describe('with shared triggered menu', () => {
298+
/**
299+
* Return a function which builds the given component and renders it.
300+
* @param componentClass the component to create
301+
*/
302+
function createComponent<T>(componentClass: Type<T>) {
303+
return function () {
304+
TestBed.configureTestingModule({
305+
imports: [CdkMenuModule],
306+
declarations: [componentClass],
307+
}).compileComponents();
308+
309+
TestBed.createComponent(componentClass).detectChanges();
310+
};
311+
}
312+
313+
it('should throw an error if two triggers in different menubars open the same menu', () => {
314+
expect(createComponent(TriggersWithSameMenuDifferentMenuBars)).toThrowError(
315+
/CdkMenuPanel is already referenced by different CdkMenuTrigger/
316+
);
317+
});
318+
319+
it('should throw an error if two triggers in the same menubar open the same menu', () => {
320+
expect(createComponent(TriggersWithSameMenuSameMenuBar)).toThrowError(
321+
/CdkMenuPanel is already referenced by different CdkMenuTrigger/
322+
);
323+
});
324+
325+
// TODO uncomment once we figure out why this is failing in Ivy
326+
// it('should throw an error if a trigger in a submenu references its parent menu', () => {
327+
// expect(createComponent(TriggerOpensItsMenu)).toThrowError(
328+
// /CdkMenuPanel is already referenced by different CdkMenuTrigger/
329+
// );
330+
// });
331+
});
296332
});
297333

298334
@Component({
@@ -331,3 +367,62 @@ class MenuBarWithNestedSubMenus {
331367

332368
@ViewChildren(CdkMenuItem) menuItems: QueryList<CdkMenuItem>;
333369
}
370+
371+
@Component({
372+
template: `
373+
<div cdkMenuBar>
374+
<button cdkMenuItem [cdkMenuTriggerFor]="menu">First</button>
375+
</div>
376+
<div cdkMenuBar>
377+
<button cdkMenuItem [cdkMenuTriggerFor]="menu">Second</button>
378+
</div>
379+
380+
<ng-template cdkMenuPanel #menu="cdkMenuPanel">
381+
<div cdkMenu [cdkMenuPanel]="menu">
382+
<button cdkMenuItem></button>
383+
</div>
384+
</ng-template>
385+
`,
386+
})
387+
class TriggersWithSameMenuDifferentMenuBars {}
388+
389+
@Component({
390+
template: `
391+
<div cdkMenuBar>
392+
<button cdkMenuItem [cdkMenuTriggerFor]="menu">First</button>
393+
<button cdkMenuItem [cdkMenuTriggerFor]="menu">Second</button>
394+
</div>
395+
396+
<ng-template cdkMenuPanel #menu="cdkMenuPanel">
397+
<div cdkMenu [cdkMenuPanel]="menu">
398+
<button cdkMenuItem></button>
399+
</div>
400+
</ng-template>
401+
`,
402+
})
403+
class TriggersWithSameMenuSameMenuBar {}
404+
405+
// TODO uncomment once we figure out why this is failing in Ivy
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 AfterViewInit {
420+
// @ViewChild(CdkMenuItem, {read: ElementRef}) trigger: ElementRef<HTMLButtonElement>;
421+
422+
// constructor(private readonly _changeDetector: ChangeDetectorRef) {}
423+
424+
// ngAfterViewInit() {
425+
// this.trigger.nativeElement.click();
426+
// this._changeDetector.detectChanges();
427+
// }
428+
// }

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

Lines changed: 30 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,15 @@ 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+
// TODO refactor once https://github.com/angular/components/pull/20146 lands
68+
if (isDevMode() && panel?._menuStack) {
69+
throwExistingMenuStackError();
70+
}
6371

72+
this._menuPanel = panel;
6473
if (this._menuPanel) {
6574
this._menuPanel._menuStack = this._getMenuStack();
6675
}
@@ -140,7 +149,8 @@ export class CdkMenuItemTrigger implements OnDestroy {
140149
*/
141150
_toggleOnMouseEnter() {
142151
const menuStack = this._getMenuStack();
143-
if (!menuStack.isEmpty() && !this.isMenuOpen()) {
152+
const isSiblingMenuOpen = !menuStack?.isEmpty() && !this.isMenuOpen();
153+
if (isSiblingMenuOpen) {
144154
this._closeSiblingTriggers();
145155
this.openMenu();
146156
}
@@ -165,7 +175,7 @@ export class CdkMenuItemTrigger implements OnDestroy {
165175
if (this._isParentVertical()) {
166176
event.preventDefault();
167177
if (this._directionality?.value === 'rtl') {
168-
this._getMenuStack().close(this._parentMenu, FocusNext.currentItem);
178+
this._getMenuStack()?.close(this._parentMenu, FocusNext.currentItem);
169179
} else {
170180
this.openMenu();
171181
this.menuPanel?._menu?.focusFirstItem('keyboard');
@@ -180,7 +190,7 @@ export class CdkMenuItemTrigger implements OnDestroy {
180190
this.openMenu();
181191
this.menuPanel?._menu?.focusFirstItem('keyboard');
182192
} else {
183-
this._getMenuStack().close(this._parentMenu, FocusNext.currentItem);
193+
this._getMenuStack()?.close(this._parentMenu, FocusNext.currentItem);
184194
}
185195
}
186196
break;
@@ -206,10 +216,10 @@ export class CdkMenuItemTrigger implements OnDestroy {
206216
// that means that the parent menu is a menu bar since we don't put the menu bar on the
207217
// stack
208218
const isParentMenuBar =
209-
!menuStack.closeSubMenuOf(this._parentMenu) && menuStack.peek() !== this._parentMenu;
219+
!menuStack?.closeSubMenuOf(this._parentMenu) && menuStack?.peek() !== this._parentMenu;
210220

211221
if (isParentMenuBar) {
212-
menuStack.closeAll();
222+
menuStack?.closeAll();
213223
}
214224
}
215225

@@ -278,6 +288,20 @@ export class CdkMenuItemTrigger implements OnDestroy {
278288

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

283307
/** 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 || !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)