From 1f9f76111180436645f45fff926e972b9dfb1b33 Mon Sep 17 00:00:00 2001 From: Philip Pham Date: Sat, 9 Sep 2017 20:59:55 -0700 Subject: [PATCH 1/3] 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. --- src/lib/menu/menu-directive.ts | 4 +-- src/lib/menu/menu-trigger.ts | 25 +++++++++++++------ src/lib/menu/menu.spec.ts | 45 ++++++++++++++++++++++++++-------- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/src/lib/menu/menu-directive.ts b/src/lib/menu/menu-directive.ts index adfa8565bfac..395c88638465 100644 --- a/src/lib/menu/menu-directive.ts +++ b/src/lib/menu/menu-directive.ts @@ -139,7 +139,7 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy { } /** Event emitted when the menu is closed. */ - @Output() close = new EventEmitter(); + @Output() close = new EventEmitter(); constructor( private _elementRef: ElementRef, @@ -152,7 +152,7 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy { ngOnDestroy() { this._tabSubscription.unsubscribe(); - this.close.emit(); + this.close.emit('destroy'); this.close.complete(); } diff --git a/src/lib/menu/menu-trigger.ts b/src/lib/menu/menu-trigger.ts index 045b872296e6..cfcf7dcb67f3 100644 --- a/src/lib/menu/menu-trigger.ts +++ b/src/lib/menu/menu-trigger.ts @@ -153,7 +153,7 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy { this._checkMenu(); this.menu.close.subscribe(reason => { - this.closeMenu(); + this._closeMenu(false); // If a click closed the menu, we should close the entire chain of nested menus. if (reason === 'click' && this._parentMenu) { @@ -216,11 +216,27 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy { /** Closes the menu. */ closeMenu(): void { + this._closeMenu(true); + } + + /** Focuses the menu trigger. */ + focus() { + this._element.nativeElement.focus(); + } + + /** + * Gives the option of setting emitEvent to false to avoid emitting an event + * when an event will already be emitted. + */ + private _closeMenu(emitEvent: boolean) { if (this._overlayRef && this.menuOpen) { this._resetMenu(); this._overlayRef.detach(); this._closeSubscription.unsubscribe(); - this.menu.close.emit(); + + if (emitEvent) { + this.menu.close.emit(); + } if (this.menu instanceof MdMenu) { this.menu._resetAnimation(); @@ -228,11 +244,6 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy { } } - /** 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. diff --git a/src/lib/menu/menu.spec.ts b/src/lib/menu/menu.spec.ts index 6006dcab4f6b..fc494fb54192 100644 --- a/src/lib/menu/menu.spec.ts +++ b/src/lib/menu/menu.spec.ts @@ -485,6 +485,7 @@ describe('MdMenu', () => { fixture = TestBed.createComponent(SimpleMenu); fixture.detectChanges(); fixture.componentInstance.trigger.openMenu(); + fixture.componentInstance.closeCallback.calls.reset(); }); it('should emit an event when a menu item is clicked', () => { @@ -493,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 = 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 escaped', () => { + 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('destroy'); + expect(emitCallback).toHaveBeenCalledTimes(1); expect(completeCallback).toHaveBeenCalled(); }); }); @@ -989,11 +1004,18 @@ describe('MdMenu', () => { expect(menus.length).toBe(3, 'Expected three open menus'); + instance.rootCloseCallback.calls.reset(); + instance.levelOneCloseCallback.calls.reset(); + instance.levelTwoCloseCallback.calls.reset(); + instance.rootTrigger.closeMenu(); fixture.detectChanges(); 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(() => { @@ -1044,7 +1066,7 @@ describe('MdMenu default overrides', () => { @Component({ template: ` - + @@ -1137,7 +1159,7 @@ class CustomMenu { [mdMenuTriggerFor]="levelTwo" #alternateTrigger="mdMenuTrigger">Toggle alternate menu - + - + - + @@ -1177,12 +1199,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; From 50d3214157ea8a72d31a2dea65e31383ad229598 Mon Sep 17 00:00:00 2001 From: Philip Pham Date: Sun, 10 Sep 2017 13:18:50 -0700 Subject: [PATCH 2/3] Responding to review comments from @crisbeto --- src/lib/menu/menu-directive.ts | 4 ++-- src/lib/menu/menu-trigger.ts | 23 ++++++++++------------- src/lib/menu/menu.spec.ts | 11 +++-------- tools/package-tools/rollup-globals.ts | 1 + 4 files changed, 16 insertions(+), 23 deletions(-) diff --git a/src/lib/menu/menu-directive.ts b/src/lib/menu/menu-directive.ts index 395c88638465..adfa8565bfac 100644 --- a/src/lib/menu/menu-directive.ts +++ b/src/lib/menu/menu-directive.ts @@ -139,7 +139,7 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy { } /** Event emitted when the menu is closed. */ - @Output() close = new EventEmitter(); + @Output() close = new EventEmitter(); constructor( private _elementRef: ElementRef, @@ -152,7 +152,7 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy { ngOnDestroy() { this._tabSubscription.unsubscribe(); - this.close.emit('destroy'); + this.close.emit(); this.close.complete(); } diff --git a/src/lib/menu/menu-trigger.ts b/src/lib/menu/menu-trigger.ts index cfcf7dcb67f3..0b83e829385c 100644 --- a/src/lib/menu/menu-trigger.ts +++ b/src/lib/menu/menu-trigger.ts @@ -43,7 +43,7 @@ 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 {empty} from 'rxjs/observable/empty'; import {merge} from 'rxjs/observable/merge'; import {Subscription} from 'rxjs/Subscription'; @@ -153,7 +153,7 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy { this._checkMenu(); this.menu.close.subscribe(reason => { - this._closeMenu(false); + this._closeMenu(); // 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,7 +218,7 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy { /** Closes the menu. */ closeMenu(): void { - this._closeMenu(true); + this.menu.close.emit(); } /** Focuses the menu trigger. */ @@ -225,19 +227,14 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy { } /** - * Gives the option of setting emitEvent to false to avoid emitting an event - * when an event will already be emitted. + * Closes the menu and does the necessary cleanup. */ - private _closeMenu(emitEvent: boolean) { + private _closeMenu() { if (this._overlayRef && this.menuOpen) { this._resetMenu(); this._overlayRef.detach(); this._closeSubscription.unsubscribe(); - if (emitEvent) { - this.menu.close.emit(); - } - if (this.menu instanceof MdMenu) { this.menu._resetAnimation(); } @@ -411,11 +408,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 : empty(); const hover = this._parentMenu ? RxChain.from(this._parentMenu.hover()) .call(filter, active => active !== this._menuItemInstance) .call(filter, () => this._menuOpen) - .result() : observableOf(null); + .result() : empty(); return merge(backdrop, parentClose, hover); } diff --git a/src/lib/menu/menu.spec.ts b/src/lib/menu/menu.spec.ts index fc494fb54192..68b6ebd87d68 100644 --- a/src/lib/menu/menu.spec.ts +++ b/src/lib/menu/menu.spec.ts @@ -97,7 +97,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(); @@ -485,7 +485,6 @@ describe('MdMenu', () => { fixture = TestBed.createComponent(SimpleMenu); fixture.detectChanges(); fixture.componentInstance.trigger.openMenu(); - fixture.componentInstance.closeCallback.calls.reset(); }); it('should emit an event when a menu item is clicked', () => { @@ -509,7 +508,7 @@ describe('MdMenu', () => { expect(fixture.componentInstance.closeCallback).toHaveBeenCalledTimes(1); }); - it('should emit an event when escaped', () => { + it('should emit an event when pressing ESCAPE', () => { const menu = overlayContainerElement.querySelector('.mat-menu-panel') as HTMLElement; dispatchKeyboardEvent(menu, 'keydown', ESCAPE); @@ -526,7 +525,7 @@ describe('MdMenu', () => { fixture.componentInstance.menu.close.subscribe(emitCallback, null, completeCallback); fixture.destroy(); - expect(emitCallback).toHaveBeenCalledWith('destroy'); + expect(emitCallback).toHaveBeenCalledWith(undefined); expect(emitCallback).toHaveBeenCalledTimes(1); expect(completeCallback).toHaveBeenCalled(); }); @@ -1004,10 +1003,6 @@ describe('MdMenu', () => { expect(menus.length).toBe(3, 'Expected three open menus'); - instance.rootCloseCallback.calls.reset(); - instance.levelOneCloseCallback.calls.reset(); - instance.levelTwoCloseCallback.calls.reset(); - instance.rootTrigger.closeMenu(); fixture.detectChanges(); tick(500); diff --git a/tools/package-tools/rollup-globals.ts b/tools/package-tools/rollup-globals.ts index 55e513bb9537..581e8eb5ae08 100644 --- a/tools/package-tools/rollup-globals.ts +++ b/tools/package-tools/rollup-globals.ts @@ -51,6 +51,7 @@ export const rollupGlobals = { 'rxjs/Observer': 'Rx', 'rxjs/Scheduler': 'Rx', 'rxjs/observable/combineLatest': 'Rx.Observable', + 'rxjs/observable/empty': 'Rx.Observable', 'rxjs/observable/forkJoin': 'Rx.Observable', 'rxjs/observable/fromEvent': 'Rx.Observable', 'rxjs/observable/merge': 'Rx.Observable', From a396846254b136bfb13c6be23ee34a467db6aed0 Mon Sep 17 00:00:00 2001 From: Philip Pham Date: Sun, 10 Sep 2017 14:01:11 -0700 Subject: [PATCH 3/3] Observable.empty() -> Observable.of() _closeMenu -> _destroyMenu --- src/lib/menu/menu-trigger.ts | 14 ++++++-------- tools/package-tools/rollup-globals.ts | 1 - 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/lib/menu/menu-trigger.ts b/src/lib/menu/menu-trigger.ts index 0b83e829385c..67b8f17db2b2 100644 --- a/src/lib/menu/menu-trigger.ts +++ b/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 {empty} from 'rxjs/observable/empty'; 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) { @@ -226,10 +226,8 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy { this._element.nativeElement.focus(); } - /** - * Closes the menu and does the necessary cleanup. - */ - private _closeMenu() { + /** Closes the menu and does the necessary cleanup. */ + private _destroyMenu() { if (this._overlayRef && this.menuOpen) { this._resetMenu(); this._overlayRef.detach(); @@ -408,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 : empty(); + 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() : empty(); + .result() : observableOf(); return merge(backdrop, parentClose, hover); } diff --git a/tools/package-tools/rollup-globals.ts b/tools/package-tools/rollup-globals.ts index 581e8eb5ae08..55e513bb9537 100644 --- a/tools/package-tools/rollup-globals.ts +++ b/tools/package-tools/rollup-globals.ts @@ -51,7 +51,6 @@ export const rollupGlobals = { 'rxjs/Observer': 'Rx', 'rxjs/Scheduler': 'Rx', 'rxjs/observable/combineLatest': 'Rx.Observable', - 'rxjs/observable/empty': 'Rx.Observable', 'rxjs/observable/forkJoin': 'Rx.Observable', 'rxjs/observable/fromEvent': 'Rx.Observable', 'rxjs/observable/merge': 'Rx.Observable',