fix(menu): multiple close events for a single close (#6961)

* fix(menu): multiple close events for a single close

Don't emit a closed event when another event will be emitted. Previously, if one clicked on a menu item, one would get two events: `undefined` and `click` in that order. One would see similar behavior for `keydown` or clicking the backdrop. Unit tests were updated to prevent a regression.

* Responding to review comments from @crisbeto

* Observable.empty() -> Observable.of()
_closeMenu -> _destroyMenu

Author: ppham27

src/lib/menu/menu-trigger.ts

@@ -43,8 +43,8 @@ import {MdMenuItem} from './menu-item';
 import {MdMenuPanel} from './menu-panel';
 import {MenuPositionX, MenuPositionY} from './menu-positions';
 import {throwMdMenuMissingError} from './menu-errors';
-import {of as observableOf} from 'rxjs/observable/of';
 import {merge} from 'rxjs/observable/merge';
+import {of as observableOf} from 'rxjs/observable/of';
 import {Subscription} from 'rxjs/Subscription';
 
 /** Injection token that determines the scroll handling while the menu is open. */
@@ -153,7 +153,7 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy {
     this._checkMenu();
 
     this.menu.close.subscribe(reason => {
-      this.closeMenu();
+      this._destroyMenu();
 
       // If a click closed the menu, we should close the entire chain of nested menus.
       if (reason === 'click' && this._parentMenu) {
@@ -205,7 +205,9 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy {
   openMenu(): void {
     if (!this._menuOpen) {
       this._createOverlay().attach(this._portal);
-      this._closeSubscription = this._menuClosingActions().subscribe(() => this.menu.close.emit());
+      this._closeSubscription = this._menuClosingActions().subscribe(() => {
+        this.menu.close.emit();
+      });
       this._initMenu();
 
       if (this.menu instanceof MdMenu) {
@@ -216,23 +218,27 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy {
 
   /** Closes the menu. */
   closeMenu(): void {
+    this.menu.close.emit();
+  }
+
+  /** Focuses the menu trigger. */
+  focus() {
+    this._element.nativeElement.focus();
+  }
+
+  /** Closes the menu and does the necessary cleanup. */
+  private _destroyMenu() {
     if (this._overlayRef && this.menuOpen) {
       this._resetMenu();
       this._overlayRef.detach();
       this._closeSubscription.unsubscribe();
-      this.menu.close.emit();
 
       if (this.menu instanceof MdMenu) {
         this.menu._resetAnimation();
       }
     }
   }
 
-  /** Focuses the menu trigger. */
-  focus() {
-    this._element.nativeElement.focus();
-  }
-
   /**
    * This method sets the menu state to open and focuses the first item if
    * the menu was opened via the keyboard.
@@ -400,11 +406,11 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy {
   /** Returns a stream that emits whenever an action that should close the menu occurs. */
   private _menuClosingActions() {
     const backdrop = this._overlayRef!.backdropClick();
-    const parentClose = this._parentMenu ? this._parentMenu.close : observableOf(null);
+    const parentClose = this._parentMenu ? this._parentMenu.close : observableOf();
     const hover = this._parentMenu ? RxChain.from(this._parentMenu.hover())
         .call(filter, active => active !== this._menuItemInstance)
         .call(filter, () => this._menuOpen)
-        .result() : observableOf(null);
+        .result() : observableOf();
 
     return merge(backdrop, parentClose, hover);
   }

src/lib/menu/menu.spec.ts

@@ -98,7 +98,7 @@ describe('MdMenu', () => {
     expect(overlayContainerElement.textContent).toBe('');
   }));
 
-  it('should close the menu when pressing escape', fakeAsync(() => {
+  it('should close the menu when pressing ESCAPE', fakeAsync(() => {
     const fixture = TestBed.createComponent(SimpleMenu);
     fixture.detectChanges();
     fixture.componentInstance.trigger.openMenu();
@@ -494,26 +494,40 @@ describe('MdMenu', () => {
       menuItem.click();
       fixture.detectChanges();
 
-      expect(fixture.componentInstance.closeCallback).toHaveBeenCalled();
+      expect(fixture.componentInstance.closeCallback).toHaveBeenCalledWith('click');
+      expect(fixture.componentInstance.closeCallback).toHaveBeenCalledTimes(1);
     });
 
     it('should emit a close event when the backdrop is clicked', () => {
-      const backdrop = <HTMLElement>overlayContainerElement.querySelector('.cdk-overlay-backdrop');
+      const backdrop = overlayContainerElement
+          .querySelector('.cdk-overlay-backdrop') as HTMLElement;
 
       backdrop.click();
       fixture.detectChanges();
 
-      expect(fixture.componentInstance.closeCallback).toHaveBeenCalled();
+      expect(fixture.componentInstance.closeCallback).toHaveBeenCalledWith(undefined);
+      expect(fixture.componentInstance.closeCallback).toHaveBeenCalledTimes(1);
+    });
+
+    it('should emit an event when pressing ESCAPE', () => {
+      const menu = overlayContainerElement.querySelector('.mat-menu-panel') as HTMLElement;
+
+      dispatchKeyboardEvent(menu, 'keydown', ESCAPE);
+      fixture.detectChanges();
+
+      expect(fixture.componentInstance.closeCallback).toHaveBeenCalledWith('keydown');
+      expect(fixture.componentInstance.closeCallback).toHaveBeenCalledTimes(1);
     });
 
     it('should complete the callback when the menu is destroyed', () => {
-      let emitCallback = jasmine.createSpy('emit callback');
-      let completeCallback = jasmine.createSpy('complete callback');
+      const emitCallback = jasmine.createSpy('emit callback');
+      const completeCallback = jasmine.createSpy('complete callback');
 
       fixture.componentInstance.menu.close.subscribe(emitCallback, null, completeCallback);
       fixture.destroy();
 
-      expect(emitCallback).toHaveBeenCalled();
+      expect(emitCallback).toHaveBeenCalledWith(undefined);
+      expect(emitCallback).toHaveBeenCalledTimes(1);
       expect(completeCallback).toHaveBeenCalled();
     });
   });
@@ -995,6 +1009,9 @@ describe('MdMenu', () => {
       tick(500);
 
       expect(overlay.querySelectorAll('.mat-menu-panel').length).toBe(0, 'Expected no open menus');
+      expect(instance.rootCloseCallback).toHaveBeenCalledTimes(1);
+      expect(instance.levelOneCloseCallback).toHaveBeenCalledTimes(1);
+      expect(instance.levelTwoCloseCallback).toHaveBeenCalledTimes(1);
     }));
 
     it('should toggle a nested menu when its trigger is added after init', fakeAsync(() => {
@@ -1059,7 +1076,7 @@ describe('MdMenu default overrides', () => {
 @Component({
   template: `
     <button [mdMenuTriggerFor]="menu" #triggerEl>Toggle menu</button>
-    <md-menu class="custom-one custom-two" #menu="mdMenu" (close)="closeCallback()">
+    <md-menu class="custom-one custom-two" #menu="mdMenu" (close)="closeCallback($event)">
       <button md-menu-item> Item </button>
       <button md-menu-item disabled> Disabled </button>
     </md-menu>
@@ -1152,7 +1169,7 @@ class CustomMenu {
       [mdMenuTriggerFor]="levelTwo"
       #alternateTrigger="mdMenuTrigger">Toggle alternate menu</button>
 
-    <md-menu #root="mdMenu">
+    <md-menu #root="mdMenu" (close)="rootCloseCallback($event)">
       <button md-menu-item
         id="level-one-trigger"
         [mdMenuTriggerFor]="levelOne"
@@ -1165,7 +1182,7 @@ class CustomMenu {
         #lazyTrigger="mdMenuTrigger">Three</button>
     </md-menu>
 
-    <md-menu #levelOne="mdMenu">
+    <md-menu #levelOne="mdMenu" (close)="levelOneCloseCallback($event)">
       <button md-menu-item>Four</button>
       <button md-menu-item
         id="level-two-trigger"
@@ -1174,7 +1191,7 @@ class CustomMenu {
       <button md-menu-item>Six</button>
     </md-menu>
 
-    <md-menu #levelTwo="mdMenu">
+    <md-menu #levelTwo="mdMenu" (close)="levelTwoCloseCallback($event)">
       <button md-menu-item>Seven</button>
       <button md-menu-item>Eight</button>
       <button md-menu-item>Nine</button>
@@ -1192,12 +1209,15 @@ class NestedMenu {
   @ViewChild('rootTrigger') rootTrigger: MdMenuTrigger;
   @ViewChild('rootTriggerEl') rootTriggerEl: ElementRef;
   @ViewChild('alternateTrigger') alternateTrigger: MdMenuTrigger;
+  readonly rootCloseCallback = jasmine.createSpy('root menu closed callback');
 
   @ViewChild('levelOne') levelOneMenu: MdMenu;
   @ViewChild('levelOneTrigger') levelOneTrigger: MdMenuTrigger;
+  readonly levelOneCloseCallback = jasmine.createSpy('level one menu closed callback');
 
   @ViewChild('levelTwo') levelTwoMenu: MdMenu;
   @ViewChild('levelTwoTrigger') levelTwoTrigger: MdMenuTrigger;
+  readonly levelTwoCloseCallback = jasmine.createSpy('level one menu closed callback');
 
   @ViewChild('lazy') lazyMenu: MdMenu;
   @ViewChild('lazyTrigger') lazyTrigger: MdMenuTrigger;