Skip to content

Commit

Permalink
fix(menu): multiple close events for a single close (angular#6961)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@ppham27 @josephperrott
ppham27 authored and josephperrott committed Sep 15, 2017
1 parent 779d3c3 commit bd56ba1
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 22 deletions.
28 changes: 17 additions & 11 deletions src/lib/menu/menu-trigger.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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.
Expand Down Expand Up @@ -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);
}
Expand Down
42 changes: 31 additions & 11 deletions src/lib/menu/menu.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
});
});
Expand Down Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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"
Expand All @@ -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"
Expand All @@ -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>
Expand All @@ -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;
Expand Down

0 comments on commit bd56ba1

Please sign in to comment.