Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 12, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f9f761
fix(menu): multiple close events for a single close
ppham27 Sep 10, 2017
50d3214
Responding to review comments from @crisbeto
ppham27 Sep 10, 2017
a396846
Observable.empty() -> Observable.of()
ppham27 Sep 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/lib/menu/menu-directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy {
}

/** Event emitted when the menu is closed. */
@Output() close = new EventEmitter<void | 'click' | 'keydown'>();
@Output() close = new EventEmitter<void | 'click' | 'destroy' | 'keydown'>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea with this one was to emit the reason only if it's user-generated. Given that there is no special behavior for destroy, can you revert it back to void | 'click' | 'keydown'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I was also wondering if it's a bug that destroy will emit a close event when the menu is already closed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's there to notify any child menus to close.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But child menus won't be open unless the menu itself is open? Anyway, I'm more than happy to leave it as it is, as it doesn't really affect any use cases that I can think of.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In most cases the children won't be open without a parent, but it's possible for the parent to be destroyed while the children are still open.


constructor(
private _elementRef: ElementRef,
Expand All @@ -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();
}

Expand Down
25 changes: 18 additions & 7 deletions src/lib/menu/menu-trigger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -216,23 +216,34 @@ 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having the emitEvent parameter, you can remove the this.menu.close.emit call from here and move it only to closeMenu. Also change the doc string to something along the lines of "Closes the menu and does the necessary cleanup".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Should I only emit the event when the menu is already open, though?

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();
}
}
}

/** 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
45 changes: 35 additions & 10 deletions src/lib/menu/menu.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,7 @@ describe('MdMenu', () => {
fixture = TestBed.createComponent(SimpleMenu);
fixture.detectChanges();
fixture.componentInstance.trigger.openMenu();
fixture.componentInstance.closeCallback.calls.reset();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? The callbacks should be reset after each test, because the component is re-compiled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were necessary when because Observable.of(null) was triggering a close event. Replaced with Observable.empty()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observable.of(null) will emit null immediately. Observable.of() won't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying that I should change it to Observable.of() instead of Observable.empty()? To me, Observable.empty() is more readable and makes the intent more clear.

But I don't mind change it to Observable.of() since the behavior would be the same as Observable.empty().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observable.empty() is definitely valid in this case. I just want to avoid bringing in an extra symbol from RxJS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to Observable.of()

});

it('should emit an event when a menu item is clicked', () => {
Expand All @@ -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 = <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 escaped', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"should emit an event when pressing ESCAPE".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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();
});
});
Expand Down Expand Up @@ -989,11 +1004,18 @@ describe('MdMenu', () => {

expect(menus.length).toBe(3, 'Expected three open menus');

instance.rootCloseCallback.calls.reset();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These reset calls shouldn't be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were necessary when because Observable.of(null) was triggering a close event. Replaced with Observable.empty()

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(() => {
Expand Down Expand Up @@ -1044,7 +1066,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 @@ -1137,7 +1159,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 @@ -1150,7 +1172,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 @@ -1159,7 +1181,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 @@ -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;
Expand Down