From 243bd2ffd98040c9e4c21f0b246c61a0a9eb0d7d Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 8 Jul 2019 20:41:04 +0200 Subject: [PATCH] fix(menu): keyboard controls not respecting DOM order when items are added at a later point A while ago we reworked the menu not to pick up the items of other child menus. As a result, we had to use a custom way of registering and de-registering the menu items, however that method ends up adding any newly-created items to the end of the item list. This will throw off the keyboard navigation if an item is inserted in the beginning or the middle of the list. With these changes we switch to picking up the items using `ContentChildren` and filtering out all of the indirect descendants. Fixes #11652. --- src/material/menu/menu-item.ts | 2 +- src/material/menu/menu-panel.ts | 10 +++ src/material/menu/menu.spec.ts | 44 ++++++++++++- src/material/menu/menu.ts | 78 ++++++++++++----------- tools/public_api_guard/material/menu.d.ts | 6 +- 5 files changed, 99 insertions(+), 41 deletions(-) diff --git a/src/material/menu/menu-item.ts b/src/material/menu/menu-item.ts index 7f8e5c557cf3..c945cdceed0b 100644 --- a/src/material/menu/menu-item.ts +++ b/src/material/menu/menu-item.ts @@ -77,7 +77,7 @@ export class MatMenuItem extends _MatMenuItemMixinBase private _elementRef: ElementRef, @Inject(DOCUMENT) document?: any, private _focusMonitor?: FocusMonitor, - @Inject(MAT_MENU_PANEL) @Optional() private _parentMenu?: MatMenuPanel) { + @Inject(MAT_MENU_PANEL) @Optional() public _parentMenu?: MatMenuPanel) { // @breaking-change 8.0.0 make `_focusMonitor` and `document` required params. super(); diff --git a/src/material/menu/menu-panel.ts b/src/material/menu/menu-panel.ts index bc98d526bef7..df11370daaaa 100644 --- a/src/material/menu/menu-panel.ts +++ b/src/material/menu/menu-panel.ts @@ -37,6 +37,16 @@ export interface MatMenuPanel { lazyContent?: MatMenuContent; backdropClass?: string; hasBackdrop?: boolean; + + /** + * @deprecated To be removed. + * @breaking-change 8.0.0 + */ addItem?: (item: T) => void; + + /** + * @deprecated To be removed. + * @breaking-change 8.0.0 + */ removeItem?: (item: T) => void; } diff --git a/src/material/menu/menu.spec.ts b/src/material/menu/menu.spec.ts index afcf7cbfee89..aca25be391d5 100644 --- a/src/material/menu/menu.spec.ts +++ b/src/material/menu/menu.spec.ts @@ -865,6 +865,33 @@ describe('MatMenu', () => { expect(item.textContent!.trim()).toBe('two'); })); + + it('should respect the DOM order, rather than insertion order, when moving focus using ' + + 'the arrow keys', fakeAsync(() => { + let fixture = createComponent(SimpleMenuWithRepeater); + + fixture.detectChanges(); + fixture.componentInstance.trigger.openMenu(); + fixture.detectChanges(); + tick(500); + + let menuPanel = document.querySelector('.mat-menu-panel')!; + let items = menuPanel.querySelectorAll('.mat-menu-panel [mat-menu-item]'); + + expect(document.activeElement).toBe(items[0], 'Expected first item to be focused on open'); + + // Add a new item after the first one. + fixture.componentInstance.items.splice(1, 0, 'Calzone'); + fixture.detectChanges(); + + items = menuPanel.querySelectorAll('.mat-menu-panel [mat-menu-item]'); + dispatchKeyboardEvent(menuPanel, 'keydown', DOWN_ARROW); + fixture.detectChanges(); + tick(); + + expect(document.activeElement).toBe(items[1], 'Expected second item to be focused'); + flush(); + })); }); describe('positions', () => { @@ -2298,7 +2325,6 @@ class LazyMenuWithContext { } - @Component({ template: ` @@ -2331,3 +2357,19 @@ class DynamicPanelMenu { class MenuWithCheckboxItems { @ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger; } + + +@Component({ + template: ` + + + + + ` +}) +class SimpleMenuWithRepeater { + @ViewChild(MatMenuTrigger, {static: false}) trigger: MatMenuTrigger; + @ViewChild(MatMenu, {static: false}) menu: MatMenu; + items = ['Pizza', 'Pasta']; +} + diff --git a/src/material/menu/menu.ts b/src/material/menu/menu.ts index da74ebe47034..9c9b6cf6cc4f 100644 --- a/src/material/menu/menu.ts +++ b/src/material/menu/menu.ts @@ -98,11 +98,11 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel private _yPosition: MenuPositionY = this._defaultOptions.yPosition; private _previousElevation: string; - /** Menu items inside the current menu. */ - private _items: MatMenuItem[] = []; + /** All items inside the menu. Includes items nested inside another menu. */ + @ContentChildren(MatMenuItem, {descendants: true}) _allItems: QueryList; - /** Emits whenever the amount of menu items changes. */ - private _itemChanges = new Subject(); + /** Only the direct descendant menu items. */ + private _directDescendantItems = new QueryList(); /** Subscription to tab events on the menu panel */ private _tabSubscription = Subscription.EMPTY; @@ -242,23 +242,41 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel } ngAfterContentInit() { - this._keyManager = new FocusKeyManager(this._items).withWrap().withTypeAhead(); + this._updateDirectDescendants(); + this._keyManager = new FocusKeyManager(this._directDescendantItems).withWrap().withTypeAhead(); this._tabSubscription = this._keyManager.tabOut.subscribe(() => this.closed.emit('tab')); } ngOnDestroy() { + this._directDescendantItems.destroy(); this._tabSubscription.unsubscribe(); this.closed.complete(); } /** Stream that emits whenever the hovered menu item changes. */ _hovered(): Observable { - return this._itemChanges.pipe( - startWith(this._items), - switchMap(items => merge(...items.map(item => item._hovered))) + return this._directDescendantItems.changes.pipe( + startWith(this._directDescendantItems), + switchMap(items => merge(...items.map((item: MatMenuItem) => item._hovered))) ); } + /* + * Registers a menu item with the menu. + * @docs-private + * @deprecated No longer being used. To be removed. + * @breaking-change 9.0.0 + */ + addItem(_item: MatMenuItem) {} + + /** + * Removes an item from the menu. + * @docs-private + * @deprecated No longer being used. To be removed. + * @breaking-change 9.0.0 + */ + removeItem(_item: MatMenuItem) {} + /** Handle a keyboard event from the menu, delegating to the appropriate action. */ _handleKeydown(event: KeyboardEvent) { const keyCode = event.keyCode; @@ -339,35 +357,6 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel } } - /** - * Registers a menu item with the menu. - * @docs-private - */ - addItem(item: MatMenuItem) { - // 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); - } - } - /** * Adds classes to the menu panel based on its position. Can be used by * consumers to add specific styling based on the position. @@ -414,6 +403,21 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel event.element.scrollTop = 0; } } + + /** + * Sets up a stream that will keep track of any newly-added menu items and will update the list + * of direct descendants. We collect the descendants this way, because `_allItems` can include + * items that are part of child menus, and using a custom way of registering items is unreliable + * when it comes to maintaining the item order. + */ + private _updateDirectDescendants() { + this._allItems.changes + .pipe(startWith(this._allItems)) + .subscribe((items: QueryList) => { + this._directDescendantItems.reset(items.filter(item => item._parentMenu === this)); + this._directDescendantItems.notifyOnChanges(); + }); + } } /** @docs-private We show the "_MatMenu" class as "MatMenu" in the docs. */ diff --git a/tools/public_api_guard/material/menu.d.ts b/tools/public_api_guard/material/menu.d.ts index 4e9a2d7c3df2..3de3d880de15 100644 --- a/tools/public_api_guard/material/menu.d.ts +++ b/tools/public_api_guard/material/menu.d.ts @@ -3,6 +3,7 @@ export declare class _MatMenu extends MatMenu { } export declare class _MatMenuBase implements AfterContentInit, MatMenuPanel, OnInit, OnDestroy { + _allItems: QueryList; _animationDone: Subject; _classList: { [key: string]: boolean; @@ -30,12 +31,12 @@ export declare class _MatMenuBase implements AfterContentInit, MatMenuPanel; + _parentMenu?: MatMenuPanel | undefined; _triggersSubmenu: boolean; role: 'menuitem' | 'menuitemradio' | 'menuitemcheckbox'; constructor(_elementRef: ElementRef, document?: any, _focusMonitor?: FocusMonitor | undefined, _parentMenu?: MatMenuPanel | undefined);