Skip to content

fix(menu): unable to open same sub-menu from different triggers and not picking up indirect descendant items #10132

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

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 32 additions & 20 deletions src/cdk/a11y/key-manager/list-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,22 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
// Buffer for the letters that the user has pressed when the typeahead option is turned on.
private _pressedLetters: string[] = [];

constructor(private _items: QueryList<T>) {
_items.changes.subscribe((newItems: QueryList<T>) => {
if (this._activeItem) {
const itemArray = newItems.toArray();
const newIndex = itemArray.indexOf(this._activeItem);

if (newIndex > -1 && newIndex !== this._activeItemIndex) {
this._activeItemIndex = newIndex;
constructor(private _items: QueryList<T> | T[]) {
// We allow for the items to be an array because, in some cases, the consumer may
// not have access to a QueryList of the items they want to manage (e.g. when the
// items aren't being collected via `ViewChildren` or `ContentChildren`).
if (_items instanceof QueryList) {
_items.changes.subscribe((newItems: QueryList<T>) => {
if (this._activeItem) {
const itemArray = newItems.toArray();
const newIndex = itemArray.indexOf(this._activeItem);

if (newIndex > -1 && newIndex !== this._activeItemIndex) {
this._activeItemIndex = newIndex;
}
}
}
});
});
}
}

/**
Expand Down Expand Up @@ -132,7 +137,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
filter(() => this._pressedLetters.length > 0),
map(() => this._pressedLetters.join(''))
).subscribe(inputString => {
const items = this._items.toArray();
const items = this._getItemsArray();

// Start at 1 because we want to start searching at the item immediately
// following the current active item.
Expand Down Expand Up @@ -288,7 +293,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
updateActiveItem(item: T): void;

updateActiveItem(item: any): void {
const itemArray = this._items.toArray();
const itemArray = this._getItemsArray();
const index = typeof item === 'number' ? item : itemArray.indexOf(item);

this._activeItemIndex = index;
Expand All @@ -310,17 +315,18 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
* currently active item and the new active item. It will calculate differently
* depending on whether wrap mode is turned on.
*/
private _setActiveItemByDelta(delta: -1 | 1, items = this._items.toArray()): void {
this._wrap ? this._setActiveInWrapMode(delta, items)
: this._setActiveInDefaultMode(delta, items);
private _setActiveItemByDelta(delta: -1 | 1): void {
this._wrap ? this._setActiveInWrapMode(delta) : this._setActiveInDefaultMode(delta);
}

/**
* Sets the active item properly given "wrap" mode. In other words, it will continue to move
* down the list until it finds an item that is not disabled, and it will wrap if it
* encounters either end of the list.
*/
private _setActiveInWrapMode(delta: -1 | 1, items: T[]): void {
private _setActiveInWrapMode(delta: -1 | 1): void {
const items = this._getItemsArray();

for (let i = 1; i <= items.length; i++) {
const index = (this._activeItemIndex + (delta * i) + items.length) % items.length;
const item = items[index];
Expand All @@ -337,17 +343,18 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
* continue to move down the list until it finds an item that is not disabled. If
* it encounters either end of the list, it will stop and not wrap.
*/
private _setActiveInDefaultMode(delta: -1 | 1, items: T[]): void {
this._setActiveItemByIndex(this._activeItemIndex + delta, delta, items);
private _setActiveInDefaultMode(delta: -1 | 1): void {
this._setActiveItemByIndex(this._activeItemIndex + delta, delta);
}

/**
* Sets the active item to the first enabled item starting at the index specified. If the
* item is disabled, it will move in the fallbackDelta direction until it either
* finds an enabled item or encounters the end of the list.
*/
private _setActiveItemByIndex(index: number, fallbackDelta: -1 | 1,
items = this._items.toArray()): void {
private _setActiveItemByIndex(index: number, fallbackDelta: -1 | 1): void {
const items = this._getItemsArray();

if (!items[index]) {
return;
}
Expand All @@ -362,4 +369,9 @@ export class ListKeyManager<T extends ListKeyManagerOption> {

this.setActiveItem(index);
}

/** Returns the items as an array. */
private _getItemsArray(): T[] {
return this._items instanceof QueryList ? this._items.toArray() : this._items;
}
}
78 changes: 59 additions & 19 deletions src/lib/menu/menu-directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ import {
OnDestroy,
OnInit,
Output,
QueryList,
TemplateRef,
QueryList,
ViewChild,
ViewEncapsulation,
} from '@angular/core';
Expand All @@ -36,7 +36,7 @@ import {matMenuAnimations} from './menu-animations';
import {MatMenuContent} from './menu-content';
import {throwMatMenuInvalidPositionX, throwMatMenuInvalidPositionY} from './menu-errors';
import {MatMenuItem} from './menu-item';
import {MatMenuPanel} from './menu-panel';
import {MAT_MENU_PANEL, MatMenuPanel} from './menu-panel';
import {MenuPositionX, MenuPositionY} from './menu-positions';


Expand Down Expand Up @@ -84,18 +84,27 @@ const MAT_MENU_BASE_ELEVATION = 2;
styleUrls: ['menu.css'],
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
exportAs: 'matMenu',
animations: [
matMenuAnimations.transformMenu,
matMenuAnimations.fadeInItems
],
exportAs: 'matMenu'
providers: [
{provide: MAT_MENU_PANEL, useExisting: MatMenu}
]
})
export class MatMenu implements OnInit, AfterContentInit, MatMenuPanel, OnDestroy {
export class MatMenu implements OnInit, AfterContentInit, MatMenuPanel<MatMenuItem>, OnDestroy {
private _keyManager: FocusKeyManager<MatMenuItem>;
private _xPosition: MenuPositionX = this._defaultOptions.xPosition;
private _yPosition: MenuPositionY = this._defaultOptions.yPosition;
private _previousElevation: string;

/** Menu items inside the current menu. */
private _items: MatMenuItem[] = [];

/** Emits whenever the amount of menu items changes. */
private _itemChanges = new Subject<MatMenuItem[]>();

/** Subscription to tab events on the menu panel */
private _tabSubscription = Subscription.EMPTY;

Expand All @@ -106,7 +115,10 @@ export class MatMenu implements OnInit, AfterContentInit, MatMenuPanel, OnDestro
_panelAnimationState: 'void' | 'enter' = 'void';

/** Emits whenever an animation on the menu completes. */
_animationDone = new Subject<void>();
_animationDone = new Subject<AnimationEvent>();

/** Whether the menu is animating. */
_isAnimating: boolean;

/** Parent menu of the current menu panel. */
parentMenu: MatMenuPanel | undefined;
Expand Down Expand Up @@ -142,7 +154,11 @@ export class MatMenu implements OnInit, AfterContentInit, MatMenuPanel, OnDestro
/** @docs-private */
@ViewChild(TemplateRef) templateRef: TemplateRef<any>;

/** List of the items inside of a menu. */
/**
* List of the items inside of a menu.
* @deprecated
* @deletion-target 7.0.0
*/
@ContentChildren(MatMenuItem) items: QueryList<MatMenuItem>;

/**
Expand Down Expand Up @@ -218,7 +234,7 @@ export class MatMenu implements OnInit, AfterContentInit, MatMenuPanel, OnDestro
}

ngAfterContentInit() {
this._keyManager = new FocusKeyManager<MatMenuItem>(this.items).withWrap().withTypeAhead();
this._keyManager = new FocusKeyManager<MatMenuItem>(this._items).withWrap().withTypeAhead();
this._tabSubscription = this._keyManager.tabOut.subscribe(() => this.close.emit('tab'));
}

Expand All @@ -229,16 +245,10 @@ export class MatMenu implements OnInit, AfterContentInit, MatMenuPanel, OnDestro

/** Stream that emits whenever the hovered menu item changes. */
_hovered(): Observable<MatMenuItem> {
if (this.items) {
return this.items.changes.pipe(
startWith(this.items),
switchMap(items => merge(...items.map(item => item._hovered)))
);
}

return this._ngZone.onStable
.asObservable()
.pipe(take(1), switchMap(() => this._hovered()));
return this._itemChanges.pipe(
startWith(this._items),
switchMap(items => merge(...items.map(item => item._hovered)))
);
}

/** Handle a keyboard event from the menu, delegating to the appropriate action. */
Expand Down Expand Up @@ -322,6 +332,35 @@ export class MatMenu implements OnInit, AfterContentInit, MatMenuPanel, OnDestro
}
}

/**
* Registers a menu item with the menu.
Copy link
Member

Choose a reason for hiding this comment

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

Expand to talk about why this registration is necessary?

* @docs-private
*/
addItem(item: MatMenuItem) {
Copy link
Member

Choose a reason for hiding this comment

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

Does needing to call these methods constitute a breaking change for people implementing their own menu panel?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. We never required the items (the QueryList we used before to collected the items) as a part of the MatMenuPanel and we didn't support custom menu panels for nested menus either.

// We register the items through this method, rather than picking them up through
// `ContentChildren`, because we need the items to be picked up by their closest
// `mat-menu` ancestor. If we used `@ContentChildren(MatMenuItem, {descendants: true})`,
// all descendant items will bleed into the top-level menu in the case where the consumer
// has `mat-menu` instances nested inside each other.
if (this._items.indexOf(item) === -1) {
this._items.push(item);
this._itemChanges.next(this._items);
}
}

/**
* Removes an item from the menu.
* @docs-private
*/
removeItem(item: MatMenuItem) {
const index = this._items.indexOf(item);

if (this._items.indexOf(item) > -1) {
this._items.splice(index, 1);
this._itemChanges.next(this._items);
}
}

/** Starts the enter animation. */
_startAnimation() {
// @deletion-target 7.0.0 Combine with _resetAnimation.
Expand All @@ -335,7 +374,8 @@ export class MatMenu implements OnInit, AfterContentInit, MatMenuPanel, OnDestro
}

/** Callback that is invoked when the panel animation completes. */
_onAnimationDone() {
this._animationDone.next();
_onAnimationDone(event: AnimationEvent) {
this._animationDone.next(event);
this._isAnimating = false;
}
}
13 changes: 12 additions & 1 deletion src/lib/menu/menu-item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
OnDestroy,
ViewEncapsulation,
Inject,
Optional,
} from '@angular/core';
import {
CanDisable,
Expand All @@ -23,6 +24,7 @@ import {
} from '@angular/material/core';
import {Subject} from 'rxjs';
import {DOCUMENT} from '@angular/common';
import {MAT_MENU_PANEL, MatMenuPanel} from './menu-panel';

// Boilerplate for applying mixins to MatMenuItem.
/** @docs-private */
Expand Down Expand Up @@ -70,7 +72,8 @@ export class MatMenuItem extends _MatMenuItemMixinBase
constructor(
private _elementRef: ElementRef,
@Inject(DOCUMENT) document?: any,
private _focusMonitor?: FocusMonitor) {
private _focusMonitor?: FocusMonitor,
@Inject(MAT_MENU_PANEL) @Optional() private _parentMenu?: MatMenuPanel<MatMenuItem>) {

// @deletion-target 7.0.0 make `_focusMonitor` and `document` required params.
super();
Expand All @@ -82,6 +85,10 @@ export class MatMenuItem extends _MatMenuItemMixinBase
_focusMonitor.monitor(this._getHostElement(), false);
}

if (_parentMenu && _parentMenu.addItem) {
_parentMenu.addItem(this);
}

this._document = document;
}

Expand All @@ -99,6 +106,10 @@ export class MatMenuItem extends _MatMenuItemMixinBase
this._focusMonitor.stopMonitoring(this._getHostElement());
}

if (this._parentMenu && this._parentMenu.removeItem) {
this._parentMenu.removeItem(this);
}

this._hovered.complete();
}

Expand Down
12 changes: 10 additions & 2 deletions src/lib/menu/menu-panel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,23 @@
* found in the LICENSE file at https://angular.io/license
*/

import {EventEmitter, TemplateRef} from '@angular/core';
import {EventEmitter, TemplateRef, InjectionToken} from '@angular/core';
import {MenuPositionX, MenuPositionY} from './menu-positions';
import {Direction} from '@angular/cdk/bidi';
import {FocusOrigin} from '@angular/cdk/a11y';
import {MatMenuContent} from './menu-content';

/**
* Injection token used to provide the parent menu to menu-specific components.
* @docs-private
*/
export const MAT_MENU_PANEL = new InjectionToken<MatMenuPanel>('MAT_MENU_PANEL');

/**
* Interface for a custom menu panel that can be used with `matMenuTriggerFor`.
* @docs-private
*/
export interface MatMenuPanel {
export interface MatMenuPanel<T = any> {
xPosition: MenuPositionX;
yPosition: MenuPositionY;
overlapTrigger: boolean;
Expand All @@ -31,4 +37,6 @@ export interface MatMenuPanel {
lazyContent?: MatMenuContent;
backdropClass?: string;
hasBackdrop?: boolean;
addItem?: (item: T) => void;
removeItem?: (item: T) => void;
}
Loading