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.

Author: crisbeto

src/lib/menu/menu-directive.ts

@@ -104,11 +104,11 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, 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<MatMenuItem>;
 
-  /** Emits whenever the amount of menu items changes. */
-  private _itemChanges = new Subject<MatMenuItem[]>();
+  /** Only the direct descendant menu items. */
+  private _directDescendantItems = new QueryList<MatMenuItem>();
 
   /** Subscription to tab events on the menu panel */
   private _tabSubscription = Subscription.EMPTY;
@@ -248,23 +248,41 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, OnI
   }
 
   ngAfterContentInit() {
-    this._keyManager = new FocusKeyManager<MatMenuItem>(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<MatMenuItem> {
-    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;
@@ -334,35 +352,6 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, 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.
@@ -409,4 +398,19 @@ export class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuItem>, 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<MatMenuItem>) => {
+        this._directDescendantItems.reset(items.filter(item => item._parentMenu === this));
+        this._directDescendantItems.notifyOnChanges();
+      });
+  }
 }

src/lib/menu/menu-item.ts

@@ -78,7 +78,7 @@ export class MatMenuItem extends _MatMenuItemMixinBase
     private _elementRef: ElementRef<HTMLElement>,
     @Inject(DOCUMENT) document?: any,
     private _focusMonitor?: FocusMonitor,
-    @Inject(MAT_MENU_PANEL) @Optional() private _parentMenu?: MatMenuPanel<MatMenuItem>) {
+    @Inject(MAT_MENU_PANEL) @Optional() public _parentMenu?: MatMenuPanel<MatMenuItem>) {
 
     // @breaking-change 8.0.0 make `_focusMonitor` and `document` required params.
     super();

src/lib/menu/menu-panel.ts

@@ -37,6 +37,16 @@ export interface MatMenuPanel<T = any> {
   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;
 }

src/lib/menu/menu.spec.ts

@@ -728,6 +728,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', () => {
@@ -2157,7 +2184,6 @@ class LazyMenuWithContext {
 }
 
 
-
 @Component({
   template: `
     <button [matMenuTriggerFor]="one">Toggle menu</button>
@@ -2190,3 +2216,19 @@ class DynamicPanelMenu {
 class MenuWithCheckboxItems {
   @ViewChild(MatMenuTrigger) trigger: MatMenuTrigger;
 }
+
+
+@Component({
+  template: `
+    <button [matMenuTriggerFor]="menu">Toggle menu</button>
+    <mat-menu #menu="matMenu">
+      <button *ngFor="let item of items" mat-menu-item>{{item}}</button>
+    </mat-menu>
+  `
+})
+class SimpleMenuWithRepeater {
+  @ViewChild(MatMenuTrigger) trigger: MatMenuTrigger;
+  @ViewChild(MatMenu) menu: MatMenu;
+  items = ['Pizza', 'Pasta'];
+}
+

tools/public_api_guard/lib/menu.d.ts

@@ -32,12 +32,12 @@ export declare class MatMenu implements AfterContentInit, MatMenuPanel<MatMenuIt
     _onAnimationStart(event: AnimationEvent): void;
     _resetAnimation(): void;
     _startAnimation(): void;
-    addItem(item: MatMenuItem): void;
+    addItem(_item: MatMenuItem): void;
     focusFirstItem(origin?: FocusOrigin): void;
     ngAfterContentInit(): void;
     ngOnDestroy(): void;
     ngOnInit(): void;
-    removeItem(item: MatMenuItem): void;
+    removeItem(_item: MatMenuItem): void;
     resetActiveItem(): void;
     setElevation(depth: number): void;
     setPositionClasses(posX?: MenuPositionX, posY?: MenuPositionY): void;