Skip to content

Commit

Permalink
fix(material/menu): decouple menu lifecycle from animations
Browse files Browse the repository at this point in the history 
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 committed Dec 9, 2024
1 parent 0c40595 commit eb34a48
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 104 deletions.
7 changes: 3 additions & 4 deletions src/material/menu/menu-content.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export class MatMenuContent implements OnDestroy {
private _changeDetectorRef = inject(ChangeDetectorRef);

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

/** Emits when the menu content has been attached. */
readonly _attached = new Subject<void>();
Expand Down Expand Up @@ -99,8 +99,7 @@ export class MatMenuContent implements OnDestroy {
}

ngOnDestroy() {
if (this._outlet) {
this._outlet.dispose();
}
this.detach();
this._outlet?.dispose();
}
}
101 changes: 44 additions & 57 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 @@ -239,6 +242,10 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
this._overlayRef = null;
}

if (this.menu && this._ownsMenu(this.menu)) {
PANELS_TO_TRIGGERS.delete(this.menu);
}

this._element.nativeElement.removeEventListener(
'touchstart',
this._handleTouchStart,
Expand Down Expand Up @@ -307,7 +314,19 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {

/** Closes the menu. */
closeMenu(): void {
this.menu?.close.emit();
const menu = this.menu;

if (menu) {
// If the trigger still "owns" the panel (e.g. no other trigger opened it while the current
// trigger is still open), we can kick off the regular closing sequence which waits for the
// animation before detaching. Otherwise we remove the overlay immediately, because its
// content will soon be moved over into a new menu, leaving the panel empty.
if (this._ownsMenu(menu)) {
menu.close.emit();
} else {
this._destroyMenu();
}
}
}

/**
Expand Down Expand Up @@ -335,7 +354,6 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
return;
}

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

Expand All @@ -348,30 +366,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 +378,8 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
* the menu was opened via the keyboard.
*/
private _initMenu(menu: MatMenuPanel): void {
// The most recent trigger is considered the "owner" of the panel.
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 +520,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 +577,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 +598,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;
}
}
43 changes: 0 additions & 43 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

0 comments on commit eb34a48

Please sign in to comment.