Skip to content

Commit

Permalink
fix(material/menu): decouple menu lifecycle from animations (angular#…
Browse files Browse the repository at this point in the history 
…30148)

Reworks the menu so that its removal isn't bound by animations. The current approach is somewhat brittle and makes it difficult to eventually switch to a fully CSS-based animation.
  • Loading branch information
@crisbeto
crisbeto authored Dec 10, 2024
1 parent 0296713 commit 7739315
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 112 deletions.
11 changes: 5 additions & 6 deletions src/material/menu/menu-content.ts
View file
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
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
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

0 comments on commit 7739315

Please sign in to comment.