Skip to content

fix(material/menu): decouple menu lifecycle from animations #30148

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
Dec 10, 2024
Merged

fix(material/menu): decouple menu lifecycle from animations #30148

Show file tree
Hide file tree
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
11 changes: 5 additions & 6 deletions src/material/menu/menu-content.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ export class MatMenuContent implements OnDestroy {
private _document = inject(DOCUMENT);
private _changeDetectorRef = inject(ChangeDetectorRef);

private _portal: TemplatePortal<any>;
private _outlet: DomPortalOutlet;
private _portal: TemplatePortal<any> | undefined;
private _outlet: DomPortalOutlet | undefined;

/** Emits when the menu content has been attached. */
readonly _attached = new Subject<void>();
Expand Down Expand Up @@ -93,14 +93,13 @@ export class MatMenuContent implements OnDestroy {
* @docs-private
*/
detach() {
if (this._portal.isAttached) {
if (this._portal?.isAttached) {
this._portal.detach();
}
}

ngOnDestroy() {
if (this._outlet) {
this._outlet.dispose();
}
this.detach();
this._outlet?.dispose();
}
}
100 changes: 41 additions & 59 deletions src/material/menu/menu-trigger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ import {
ViewContainerRef,
} from '@angular/core';
import {normalizePassiveListenerOptions} from '@angular/cdk/platform';
import {asapScheduler, merge, Observable, of as observableOf, Subscription} from 'rxjs';
import {delay, filter, take, takeUntil} from 'rxjs/operators';
import {merge, Observable, of as observableOf, Subscription} from 'rxjs';
import {filter, takeUntil} from 'rxjs/operators';
import {MatMenu, MenuCloseReason} from './menu';
import {throwMatMenuRecursiveError} from './menu-errors';
import {MatMenuItem} from './menu-item';
Expand Down Expand Up @@ -81,6 +81,9 @@ const passiveEventListenerOptions = normalizePassiveListenerOptions({passive: tr
*/
export const MENU_PANEL_TOP_PADDING = 8;

/** Mapping between menu panels and the last trigger that opened them. */
const PANELS_TO_TRIGGERS = new WeakMap<MatMenuPanel, MatMenuTrigger>();

/** Directive applied to an element that should trigger a `mat-menu`. */
@Directive({
selector: `[mat-menu-trigger-for], [matMenuTriggerFor]`,
Expand Down Expand Up @@ -234,9 +237,8 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
}

ngOnDestroy() {
if (this._overlayRef) {
this._overlayRef.dispose();
this._overlayRef = null;
if (this.menu && this._ownsMenu(this.menu)) {
PANELS_TO_TRIGGERS.delete(this.menu);
}

this._element.nativeElement.removeEventListener(
Expand All @@ -248,6 +250,11 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
this._menuCloseSubscription.unsubscribe();
this._closingActionsSubscription.unsubscribe();
this._hoverSubscription.unsubscribe();

if (this._overlayRef) {
this._overlayRef.dispose();
this._overlayRef = null;
}
}

/** Whether the menu is open. */
Expand Down Expand Up @@ -335,7 +342,6 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
return;
}

const menu = this.menu;
this._closingActionsSubscription.unsubscribe();
this._overlayRef.detach();

Expand All @@ -348,30 +354,10 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
}

this._openedBy = undefined;
this._setIsMenuOpen(false);

if (menu instanceof MatMenu) {
menu._resetAnimation();

if (menu.lazyContent) {
// Wait for the exit animation to finish before detaching the content.
menu._animationDone
.pipe(
filter(event => event.toState === 'void'),
take(1),
// Interrupt if the content got re-attached.
takeUntil(menu.lazyContent._attached),
)
.subscribe({
next: () => menu.lazyContent!.detach(),
// No matter whether the content got re-attached, reset the menu.
complete: () => this._setIsMenuOpen(false),
});
} else {
this._setIsMenuOpen(false);
}
} else {
this._setIsMenuOpen(false);
menu?.lazyContent?.detach();
if (this.menu && this._ownsMenu(this.menu)) {
PANELS_TO_TRIGGERS.delete(this.menu);
}
}

Expand All @@ -380,6 +366,15 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
* the menu was opened via the keyboard.
*/
private _initMenu(menu: MatMenuPanel): void {
const previousTrigger = PANELS_TO_TRIGGERS.get(menu);

// If the same menu is currently attached to another trigger,
// we need to close it so it doesn't end up in a broken state.
if (previousTrigger && previousTrigger !== this) {
previousTrigger.closeMenu();
}

PANELS_TO_TRIGGERS.set(menu, this);
menu.parentMenu = this.triggersSubmenu() ? this._parentMaterialMenu : undefined;
menu.direction = this.dir;
menu.focusFirstItem(this._openedBy || 'program');
Expand Down Expand Up @@ -520,10 +515,9 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
const detachments = this._overlayRef!.detachments();
const parentClose = this._parentMaterialMenu ? this._parentMaterialMenu.closed : observableOf();
const hover = this._parentMaterialMenu
? this._parentMaterialMenu._hovered().pipe(
filter(active => active !== this._menuItemInstance),
filter(() => this._menuOpen),
)
? this._parentMaterialMenu
._hovered()
.pipe(filter(active => this._menuOpen && active !== this._menuItemInstance))
: observableOf();

return merge(backdrop, parentClose as Observable<MenuCloseReason>, hover, detachments);
Expand Down Expand Up @@ -578,35 +572,14 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
/** Handles the cases where the user hovers over the trigger. */
private _handleHover() {
// Subscribe to changes in the hovered item in order to toggle the panel.
if (!this.triggersSubmenu() || !this._parentMaterialMenu) {
return;
}

this._hoverSubscription = this._parentMaterialMenu
._hovered()
// Since we might have multiple competing triggers for the same menu (e.g. a sub-menu
// with different data and triggers), we have to delay it by a tick to ensure that
// it won't be closed immediately after it is opened.
.pipe(
filter(active => active === this._menuItemInstance && !active.disabled),
delay(0, asapScheduler),
)
.subscribe(() => {
this._openedBy = 'mouse';

// If the same menu is used between multiple triggers, it might still be animating
// while the new trigger tries to re-open it. Wait for the animation to finish
// before doing so. Also interrupt if the user moves to another item.
if (this.menu instanceof MatMenu && this.menu._isAnimating) {
// We need the `delay(0)` here in order to avoid
// 'changed after checked' errors in some cases. See #12194.
this.menu._animationDone
.pipe(take(1), delay(0, asapScheduler), takeUntil(this._parentMaterialMenu!._hovered()))
.subscribe(() => this.openMenu());
} else {
if (this.triggersSubmenu() && this._parentMaterialMenu) {
this._hoverSubscription = this._parentMaterialMenu._hovered().subscribe(active => {
if (active === this._menuItemInstance && !active.disabled) {
this._openedBy = 'mouse';
this.openMenu();
}
});
}
}

/** Gets the portal that should be attached to the overlay. */
Expand All @@ -620,4 +593,13 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {

return this._portal;
}

/**
* Determines whether the trigger owns a specific menu panel, at the current point in time.
* This allows us to distinguish the case where the same panel is passed into multiple triggers
* and multiple are open at a time.
*/
private _ownsMenu(menu: MatMenuPanel): boolean {
return PANELS_TO_TRIGGERS.get(menu) === this;
}
}
48 changes: 1 addition & 47 deletions src/material/menu/menu.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1219,49 +1219,6 @@ describe('MatMenu', () => {
.toBe(true);
}));

it('should detach the lazy content when the menu is closed', fakeAsync(() => {
const fixture = createComponent(SimpleLazyMenu);

fixture.detectChanges();
fixture.componentInstance.trigger.openMenu();
fixture.detectChanges();
tick(500);

expect(fixture.componentInstance.items.length).toBeGreaterThan(0);

fixture.componentInstance.trigger.closeMenu();
fixture.detectChanges();
tick(500);
fixture.detectChanges();

expect(fixture.componentInstance.items.length).toBe(0);
}));

it('should wait for the close animation to finish before considering the panel as closed', fakeAsync(() => {
const fixture = createComponent(SimpleLazyMenu);
fixture.detectChanges();
const trigger = fixture.componentInstance.trigger;

expect(trigger.menuOpen).withContext('Expected menu to start off closed').toBe(false);

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

expect(trigger.menuOpen).withContext('Expected menu to be open').toBe(true);

trigger.closeMenu();
fixture.detectChanges();

expect(trigger.menuOpen)
.withContext('Expected menu to be considered open while the close animation is running')
.toBe(true);
tick(500);
fixture.detectChanges();

expect(trigger.menuOpen).withContext('Expected menu to be closed').toBe(false);
}));

it('should focus the first menu item when opening a lazy menu via keyboard', async () => {
const fixture = createComponent(SimpleLazyMenu);
fixture.autoDetectChanges();
Expand Down Expand Up @@ -1741,15 +1698,12 @@ describe('MatMenu', () => {
}));

it('should complete the callback when the menu is destroyed', fakeAsync(() => {
const emitCallback = jasmine.createSpy('emit callback');
const completeCallback = jasmine.createSpy('complete callback');

fixture.componentInstance.menu.closed.subscribe(emitCallback, null, completeCallback);
fixture.componentInstance.menu.closed.subscribe(null, null, completeCallback);
fixture.destroy();
tick(500);

expect(emitCallback).toHaveBeenCalledWith(undefined);
expect(emitCallback).toHaveBeenCalledTimes(1);
expect(completeCallback).toHaveBeenCalled();
}));
});
Expand Down
Loading