Skip to content

fix(menu): not unsubscribing from close stream if trigger is destroyed #14107

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 1 commit into from
Jan 16, 2019
Merged

fix(menu): not unsubscribing from close stream if trigger is destroyed #14107

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 5 additions & 4 deletions src/lib/menu/menu-trigger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
private _portal: TemplatePortal;
private _overlayRef: OverlayRef | null = null;
private _menuOpen: boolean = false;
private _closeSubscription = Subscription.EMPTY;
private _closingActionsSubscription = Subscription.EMPTY;
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to refactor this to use the takeUntil(destroyed) pattern now? Looks like there's a few manually managed subscriptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it for two one them @jelbourn. The _menuCloseSubscription has to stay, because we have to unsubscribe from it when the menu is swapped out.

Copy link
Member Author

@crisbeto crisbeto Nov 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I've had to revert it back to what I had initially in the PR. It seems like we have some logic that depends on us unsubscribing from these streams at the right time, which isn't only when the trigger is destroyed. It got caught by a couple of the unit tests.

private _hoverSubscription = Subscription.EMPTY;
private _menuCloseSubscription = Subscription.EMPTY;
private _scrollStrategy: () => ScrollStrategy;
Expand Down Expand Up @@ -195,6 +195,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
passiveEventListenerOptions);

this._cleanUpSubscriptions();
this._closingActionsSubscription.unsubscribe();
}

/** Whether the menu is open. */
Expand Down Expand Up @@ -233,7 +234,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
this.menu.lazyContent.attach(this.menuData);
}

this._closeSubscription = this._menuClosingActions().subscribe(() => this.closeMenu());
this._closingActionsSubscription = this._menuClosingActions().subscribe(() => this.closeMenu());
this._initMenu();

if (this.menu instanceof MatMenu) {
Expand Down Expand Up @@ -266,7 +267,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {

const menu = this.menu;

this._closeSubscription.unsubscribe();
this._closingActionsSubscription.unsubscribe();
this._overlayRef.detach();

if (menu instanceof MatMenu) {
Expand Down Expand Up @@ -466,7 +467,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {

/** Cleans up the active subscriptions. */
private _cleanUpSubscriptions(): void {
this._closeSubscription.unsubscribe();
this._closingActionsSubscription.unsubscribe();
this._hoverSubscription.unsubscribe();
}

Expand Down