Skip to content

Commit

Permalink
fix(menu): not closing when overlay is detached externally (#8654)
Browse files Browse the repository at this point in the history
Fixes the menu panel not being closed when its `OverlayRef` is detached externally using `detach`, for example when using the `CloseScrollStrategy`.
  • Loading branch information
crisbeto authored and tinayuangao committed Dec 1, 2017
1 parent 9582b8b commit dd3094f
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 8 deletions.
1 change: 0 additions & 1 deletion src/lib/menu/menu-directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnDestroy {

ngOnDestroy() {
this._tabSubscription.unsubscribe();
this.closed.emit();
this.closed.complete();
}

Expand Down
9 changes: 4 additions & 5 deletions src/lib/menu/menu-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,7 @@ export class MatMenuTrigger implements AfterContentInit, 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.closeMenu());
this._initMenu();

if (this.menu instanceof MatMenu) {
Expand All @@ -218,8 +216,8 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
private _destroyMenu() {
if (this._overlayRef && this.menuOpen) {
this._resetMenu();
this._overlayRef.detach();
this._closeSubscription.unsubscribe();
this._overlayRef.detach();

if (this.menu instanceof MatMenu) {
this.menu._resetAnimation();
Expand Down Expand Up @@ -400,13 +398,14 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
/** Returns a stream that emits whenever an action that should close the menu occurs. */
private _menuClosingActions() {
const backdrop = this._overlayRef!.backdropClick();
const detachments = this._overlayRef!.detachments();
const parentClose = this._parentMenu ? this._parentMenu.close : observableOf();
const hover = this._parentMenu ? this._parentMenu._hovered().pipe(
filter(active => active !== this._menuItemInstance),
filter(() => this._menuOpen)
) : observableOf();

return merge(backdrop, parentClose, hover);
return merge(backdrop, parentClose, hover, detachments);
}

/** Handles mouse presses on the trigger. */
Expand Down
34 changes: 32 additions & 2 deletions src/lib/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
QueryList,
} from '@angular/core';
import {Direction, Directionality} from '@angular/cdk/bidi';
import {OverlayContainer} from '@angular/cdk/overlay';
import {OverlayContainer, Overlay} from '@angular/cdk/overlay';
import {ESCAPE, LEFT_ARROW, RIGHT_ARROW} from '@angular/cdk/keycodes';
import {
MAT_MENU_DEFAULT_OPTIONS,
Expand All @@ -25,7 +25,7 @@ import {
MenuPositionY,
MatMenuItem,
} from './index';
import {MENU_PANEL_TOP_PADDING} from './menu-trigger';
import {MENU_PANEL_TOP_PADDING, MAT_MENU_SCROLL_STRATEGY} from './menu-trigger';
import {MatRipple} from '@angular/material/core';
import {
dispatchKeyboardEvent,
Expand All @@ -35,6 +35,8 @@ import {
createMouseEvent,
dispatchFakeEvent,
} from '@angular/cdk/testing';
import {Subject} from 'rxjs/Subject';
import {ScrollDispatcher} from '@angular/cdk/scrolling';


describe('MatMenu', () => {
Expand Down Expand Up @@ -242,6 +244,34 @@ describe('MatMenu', () => {
expect(document.activeElement).toBe(panel, 'Expected the panel to be focused.');
});

it('should close the menu when using the CloseScrollStrategy', fakeAsync(() => {
const scrolledSubject = new Subject();

TestBed.overrideProvider(ScrollDispatcher, {
deps: [],
useFactory: () => ({scrolled: () => scrolledSubject})
});

TestBed.overrideProvider(MAT_MENU_SCROLL_STRATEGY, {
deps: [Overlay],
useFactory: (overlay: Overlay) => () => overlay.scrollStrategies.close()
});

const fixture = TestBed.createComponent(SimpleMenu);
const trigger = fixture.componentInstance.trigger;

fixture.detectChanges();
trigger.openMenu();
fixture.detectChanges();

expect(trigger.menuOpen).toBe(true);

scrolledSubject.next();
tick(500);

expect(trigger.menuOpen).toBe(false);
}));

describe('positions', () => {
let fixture: ComponentFixture<PositionedMenu>;
let panel: HTMLElement;
Expand Down

0 comments on commit dd3094f

Please sign in to comment.