Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cdk-experimental/menu): do not allow two separate triggers to open the same menu #20300

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 41 additions & 1 deletion src/cdk-experimental/menu/context-menu.spec.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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 {}
21 changes: 21 additions & 0 deletions src/cdk-experimental/menu/context-menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
Inject,
Injectable,
InjectionToken,
isDevMode,
} from '@angular/core';
import {DOCUMENT} from '@angular/common';
import {Directionality} from '@angular/cdk/bidi';
Expand All @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -324,6 +331,7 @@ export class CdkContextMenuTrigger implements OnDestroy {

ngOnDestroy() {
this._destroyOverlay();
this._resetPanelMenuStack();

this._destroyed.next();
this._destroyed.complete();
Expand All @@ -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;
}
18 changes: 18 additions & 0 deletions src/cdk-experimental/menu/menu-errors.ts
Original file line number Diff line number Diff line change
@@ -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.'
);
}
97 changes: 96 additions & 1 deletion src/cdk-experimental/menu/menu-item-trigger.spec.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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();
// }
// }
36 changes: 30 additions & 6 deletions src/cdk-experimental/menu/menu-item-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
Inject,
OnDestroy,
Optional,
isDevMode,
} from '@angular/core';
import {Directionality} from '@angular/cdk/bidi';
import {TemplatePortal} from '@angular/cdk/portal';
Expand All @@ -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
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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();
}
Expand All @@ -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');
Expand All @@ -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;
Expand All @@ -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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you verify that your calls to methods on menuStack? will work in an expected way if menuStack is not defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the instance that there is no menustack there's nothing to do and the ? prevents the methods from being called.


if (isParentMenuBar) {
menuStack.closeAll();
menuStack?.closeAll();
}
}

Expand Down Expand Up @@ -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 */
Expand Down
16 changes: 8 additions & 8 deletions src/cdk-experimental/menu/menu-item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand All @@ -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();
}
}

Expand Down Expand Up @@ -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;
}
Expand All @@ -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));
})
);
}
Expand Down