From 127601850db1972f755d4885d13e5ce84125305f Mon Sep 17 00:00:00 2001 From: crisbeto Date: Fri, 29 Jun 2018 18:14:24 +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/lib/menu/menu-directive.ts | 60 ++++++++++++++-------------------- src/lib/menu/menu-item.ts | 2 +- src/lib/menu/menu-panel.ts | 10 ++++++ src/lib/menu/menu.spec.ts | 40 +++++++++++++++++++++++ 4 files changed, 75 insertions(+), 37 deletions(-) diff --git a/src/lib/menu/menu-directive.ts b/src/lib/menu/menu-directive.ts index e9687b2f7e86..0996cea0a0b2 100644 --- a/src/lib/menu/menu-directive.ts +++ b/src/lib/menu/menu-directive.ts @@ -104,11 +104,11 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI 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; @@ -238,19 +238,21 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI } 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), + return this._directDescendantItems.changes.pipe( + startWith(this._directDescendantItems), switchMap(items => merge(...items.map(item => item._hovered))) ); } @@ -325,35 +327,6 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI } } - /** - * 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. @@ -396,4 +369,19 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI 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(); + }); + } } diff --git a/src/lib/menu/menu-item.ts b/src/lib/menu/menu-item.ts index 227417c20fff..2f51f78fffdc 100644 --- a/src/lib/menu/menu-item.ts +++ b/src/lib/menu/menu-item.ts @@ -73,7 +73,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 7.0.0 make `_focusMonitor` and `document` required params. super(); diff --git a/src/lib/menu/menu-panel.ts b/src/lib/menu/menu-panel.ts index bc98d526bef7..70f375097dab 100644 --- a/src/lib/menu/menu-panel.ts +++ b/src/lib/menu/menu-panel.ts @@ -37,6 +37,16 @@ export interface MatMenuPanel { lazyContent?: MatMenuContent; backdropClass?: string; hasBackdrop?: boolean; + + /** + * @deprecated To be removed. + * @deletion-target 8.0.0 + */ addItem?: (item: T) => void; + + /** + * @deprecated To be removed. + * @deletion-target 8.0.0 + */ removeItem?: (item: T) => void; } diff --git a/src/lib/menu/menu.spec.ts b/src/lib/menu/menu.spec.ts index 4b10cf88d178..01d112350511 100644 --- a/src/lib/menu/menu.spec.ts +++ b/src/lib/menu/menu.spec.ts @@ -544,6 +544,32 @@ 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'); + })); }); describe('positions', () => { @@ -1972,3 +1998,17 @@ class LazyMenuWithContext { @ViewChild('triggerTwo') triggerTwo: MatMenuTrigger; } +@Component({ + template: ` + + + + + ` +}) +class SimpleMenuWithRepeater { + @ViewChild(MatMenuTrigger) trigger: MatMenuTrigger; + @ViewChild(MatMenu) menu: MatMenu; + items = ['Pizza', 'Pasta']; +} +