From c2a850c224f23401b40106467ff9fef77309e511 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 28 Nov 2024 11:06:47 +0100 Subject: [PATCH] fix(material/menu): remove dependency on animations module Reworks the menu so it no longer depends on the animations module. This should allow us to avoid some of the pitfalls of the animations module like occasional memory leaks (e.g. #47748) and reduce the bundle size. --- src/material/menu/menu-animations.ts | 2 + src/material/menu/menu-content.ts | 11 ++- src/material/menu/menu-trigger.ts | 45 +++++----- src/material/menu/menu.html | 9 +- src/material/menu/menu.scss | 33 ++++++- src/material/menu/menu.spec.ts | 25 ------ src/material/menu/menu.ts | 114 ++++++++++++++++-------- tools/public_api_guard/material/menu.md | 16 ++-- 8 files changed, 151 insertions(+), 104 deletions(-) diff --git a/src/material/menu/menu-animations.ts b/src/material/menu/menu-animations.ts index 1ddefea6395b..1e988537cc26 100644 --- a/src/material/menu/menu-animations.ts +++ b/src/material/menu/menu-animations.ts @@ -20,6 +20,8 @@ import { * Animation duration and timing values are based on: * https://material.io/guidelines/components/menus.html#menus-usage * @docs-private + * @deprecated No longer used, will be removed. + * @breaking-change 21.0.0 */ export const matMenuAnimations: { readonly transformMenu: AnimationTriggerMetadata; diff --git a/src/material/menu/menu-content.ts b/src/material/menu/menu-content.ts index fc4369907ddd..54cb136d2fc2 100644 --- a/src/material/menu/menu-content.ts +++ b/src/material/menu/menu-content.ts @@ -19,7 +19,6 @@ import { ViewContainerRef, inject, } from '@angular/core'; -import {Subject} from 'rxjs'; /** * Injection token that can be used to reference instances of `MatMenuContent`. It serves @@ -41,11 +40,11 @@ export class MatMenuContent implements OnDestroy { private _document = inject(DOCUMENT); private _changeDetectorRef = inject(ChangeDetectorRef); - private _portal: TemplatePortal; + private _portal: TemplatePortal | undefined; private _outlet: DomPortalOutlet; - /** Emits when the menu content has been attached. */ - readonly _attached = new Subject(); + /** Number of times the content was attached. */ + _attachCount = 0; constructor(...args: unknown[]); @@ -85,7 +84,7 @@ export class MatMenuContent implements OnDestroy { // it needs to check for new menu items and update the `@ContentChild` in `MatMenu`. this._changeDetectorRef.markForCheck(); this._portal.attach(this._outlet, context); - this._attached.next(); + this._attachCount++; } /** @@ -93,7 +92,7 @@ export class MatMenuContent implements OnDestroy { * @docs-private */ detach() { - if (this._portal.isAttached) { + if (this._portal?.isAttached) { this._portal.detach(); } } diff --git a/src/material/menu/menu-trigger.ts b/src/material/menu/menu-trigger.ts index d7f85f2a22d1..b6acbb73fc0b 100644 --- a/src/material/menu/menu-trigger.ts +++ b/src/material/menu/menu-trigger.ts @@ -295,7 +295,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { this._initMenu(menu); if (menu instanceof MatMenu) { - menu._startAnimation(); + menu._setIsOpen(true); menu._directDescendantItems.changes.pipe(takeUntil(menu.close)).subscribe(() => { // Re-adjust the position without locking when the amount of items // changes so that the overlay is allowed to pick a new optimal position. @@ -337,7 +337,6 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { const menu = this.menu; this._closingActionsSubscription.unsubscribe(); - this._overlayRef.detach(); // Always restore focus if the user is navigating using the keyboard or the menu was opened // programmatically. We don't restore for non-root triggers, because it can prevent focus @@ -350,28 +349,30 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { this._openedBy = undefined; 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); - } + const attachCount = menu.lazyContent ? menu.lazyContent._attachCount : 0; + + menu._animationDone + .pipe( + filter(state => state === 'void'), + take(1), + ) + .subscribe({ + complete: () => { + // Don't detach the lazy content if it got re-attached while the animation + // was running. It may have been attached to a different DOM node. + if (menu.lazyContent && menu.lazyContent._attachCount === attachCount) { + menu.lazyContent.detach(); + } + this._overlayRef?.detach(); + }, + }); + + this._setIsMenuOpen(false); + menu._setIsOpen(false); } else { this._setIsMenuOpen(false); menu?.lazyContent?.detach(); + this._overlayRef.detach(); } } @@ -386,7 +387,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { this._setIsMenuOpen(true); } - // set state rather than toggle to support triggers sharing a menu + /** Sets the current menu state. */ private _setIsMenuOpen(isOpen: boolean): void { if (isOpen !== this._menuOpen) { this._menuOpen = isOpen; diff --git a/src/material/menu/menu.html b/src/material/menu/menu.html index 1c77f023b8b7..d8988e08ef3f 100644 --- a/src/material/menu/menu.html +++ b/src/material/menu/menu.html @@ -3,12 +3,15 @@ class="mat-mdc-menu-panel" [id]="panelId" [class]="_classList" + [class.mat-menu-panel-animations-disabled]="_animationsDisabled" + [class.mat-menu-panel-exit-animation]="_panelAnimationState === 'void'" + [class.mat-menu-panel-animating]="_isAnimating" (click)="closed.emit('click')" - [@transformMenu]="_panelAnimationState" - (@transformMenu.start)="_onAnimationStart($event)" - (@transformMenu.done)="_onAnimationDone($event)" tabindex="-1" role="menu" + (animationstart)="_onAnimationStart($event.animationName)" + (animationend)="_onAnimationDone($event.animationName)" + (animationcancel)="_onAnimationDone($event.animationName)" [attr.aria-label]="ariaLabel || null" [attr.aria-labelledby]="ariaLabelledby || null" [attr.aria-describedby]="ariaDescribedby || null"> diff --git a/src/material/menu/menu.scss b/src/material/menu/menu.scss index 07a323ef2f8c..b5544945ff9c 100644 --- a/src/material/menu/menu.scss +++ b/src/material/menu/menu.scss @@ -31,10 +31,33 @@ mat-menu { } } +@keyframes _mat-menu-enter { + from { + opacity: 0; + transform: scale(0.8); + } + + to { + opacity: 1; + transform: none; + } +} + +@keyframes _mat-menu-exit { + from { + opacity: 1; + } + + to { + opacity: 0; + } +} + .mat-mdc-menu-panel { @include menu-common.base; box-sizing: border-box; outline: 0; + animation: _mat-menu-enter 120ms cubic-bezier(0, 0, 0.2, 1); @include token-utils.use-tokens(tokens-mat-menu.$prefix, tokens-mat-menu.get-token-slots()) { @include token-utils.create-token-slot(border-radius, container-shape); @@ -48,6 +71,14 @@ mat-menu { // We should clean it up eventually and re-approve all the screenshots. will-change: transform, opacity; + &.mat-menu-panel-exit-animation { + animation: _mat-menu-exit 100ms 25ms linear forwards; + } + + &.mat-menu-panel-animations-disabled { + animation: none; + } + // Prevent users from interacting with the panel while it's animating. Note that // people won't be able to click through it, because the overlay pane will catch the click. // This fixes the following issues: @@ -55,7 +86,7 @@ mat-menu { // * Users accidentally tapping on content inside the sub-menu on touch devices, if the // sub-menu overlaps the trigger. The issue is due to touch devices emulating the // `mouseenter` event by dispatching it on tap. - &.ng-animating { + &.mat-menu-panel-animating { pointer-events: none; // If the same menu is assigned to multiple triggers and the user moves quickly between them diff --git a/src/material/menu/menu.spec.ts b/src/material/menu/menu.spec.ts index fc7de98f0928..f3afe4d71fcb 100644 --- a/src/material/menu/menu.spec.ts +++ b/src/material/menu/menu.spec.ts @@ -1237,31 +1237,6 @@ describe('MatMenu', () => { 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(); diff --git a/src/material/menu/menu.ts b/src/material/menu/menu.ts index e3ae2558567e..5545075e77e3 100644 --- a/src/material/menu/menu.ts +++ b/src/material/menu/menu.ts @@ -29,8 +29,8 @@ import { AfterRenderRef, inject, Injector, + ANIMATION_MODULE_TYPE, } from '@angular/core'; -import {AnimationEvent} from '@angular/animations'; import {_IdGenerator, FocusKeyManager, FocusOrigin} from '@angular/cdk/a11y'; import {Direction} from '@angular/cdk/bidi'; import { @@ -48,7 +48,6 @@ import {MatMenuPanel, MAT_MENU_PANEL} from './menu-panel'; import {MenuPositionX, MenuPositionY} from './menu-positions'; import {throwMatMenuInvalidPositionX, throwMatMenuInvalidPositionY} from './menu-errors'; import {MatMenuContent, MAT_MENU_CONTENT} from './menu-content'; -import {matMenuAnimations} from './menu-animations'; /** Reason why the menu was closed. */ export type MenuCloseReason = void | 'click' | 'keydown' | 'tab'; @@ -93,6 +92,12 @@ export function MAT_MENU_DEFAULT_OPTIONS_FACTORY(): MatMenuDefaultOptions { }; } +/** Name of the enter animation `@keyframes`. */ +const ENTER_ANIMATION = '_mat-menu-enter'; + +/** Name of the exit animation `@keyframes`. */ +const EXIT_ANIMATION = '_mat-menu-exit'; + @Component({ selector: 'mat-menu', templateUrl: 'menu.html', @@ -105,17 +110,21 @@ export function MAT_MENU_DEFAULT_OPTIONS_FACTORY(): MatMenuDefaultOptions { '[attr.aria-labelledby]': 'null', '[attr.aria-describedby]': 'null', }, - animations: [matMenuAnimations.transformMenu, matMenuAnimations.fadeInItems], providers: [{provide: MAT_MENU_PANEL, useExisting: MatMenu}], }) export class MatMenu implements AfterContentInit, MatMenuPanel, OnInit, OnDestroy { private _elementRef = inject>(ElementRef); private _changeDetectorRef = inject(ChangeDetectorRef); + private _injector = inject(Injector); private _keyManager: FocusKeyManager; private _xPosition: MenuPositionX; private _yPosition: MenuPositionY; private _firstItemFocusRef?: AfterRenderRef; + private _exitFallbackTimeout: ReturnType | undefined; + + /** Whether animations are currently disabled. */ + protected _animationsDisabled: boolean; /** All items inside the menu. Includes items nested inside another menu. */ @ContentChildren(MatMenuItem, {descendants: true}) _allItems: QueryList; @@ -130,7 +139,7 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI _panelAnimationState: 'void' | 'enter' = 'void'; /** Emits whenever an animation on the menu completes. */ - readonly _animationDone = new Subject(); + readonly _animationDone = new Subject<'void' | 'enter'>(); /** Whether the menu is animating. */ _isAnimating: boolean; @@ -267,8 +276,6 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI readonly panelId: string = inject(_IdGenerator).getId('mat-menu-panel-'); - private _injector = inject(Injector); - constructor(...args: unknown[]); constructor() { @@ -279,6 +286,7 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI this.backdropClass = defaultOptions.backdropClass; this.overlapTrigger = defaultOptions.overlapTrigger; this.hasBackdrop = defaultOptions.hasBackdrop; + this._animationsDisabled = inject(ANIMATION_MODULE_TYPE, {optional: true}) === 'NoopAnimations'; } ngOnInit() { @@ -327,6 +335,7 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI this._directDescendantItems.destroy(); this.closed.complete(); this._firstItemFocusRef?.destroy(); + clearTimeout(this._exitFallbackTimeout); } /** Stream that emits whenever the hovered menu item changes. */ @@ -396,15 +405,7 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI this._firstItemFocusRef?.destroy(); this._firstItemFocusRef = afterNextRender( () => { - let menuPanel: HTMLElement | null = null; - - if (this._directDescendantItems.length) { - // Because the `mat-menuPanel` is at the DOM insertion point, not inside the overlay, we don't - // have a nice way of getting a hold of the menuPanel panel. We can't use a `ViewChild` either - // because the panel is inside an `ng-template`. We work around it by starting from one of - // the items and walking up the DOM. - menuPanel = this._directDescendantItems.first!._getHostElement().closest('[role="menu"]'); - } + const menuPanel = this._resolvePanel(); // If an item in the menuPanel is already focused, avoid overriding the focus. if (!menuPanel || !menuPanel.contains(document.activeElement)) { @@ -456,36 +457,56 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI this._changeDetectorRef.markForCheck(); } - /** Starts the enter animation. */ - _startAnimation() { - // @breaking-change 8.0.0 Combine with _resetAnimation. - this._panelAnimationState = 'enter'; - } + /** Callback that is invoked when the panel animation completes. */ + protected _onAnimationDone(state: string) { + const isExit = state === EXIT_ANIMATION; - /** Resets the panel animation to its initial state. */ - _resetAnimation() { - // @breaking-change 8.0.0 Combine with _startAnimation. - this._panelAnimationState = 'void'; + if (isExit || state === ENTER_ANIMATION) { + if (isExit) { + clearTimeout(this._exitFallbackTimeout); + this._exitFallbackTimeout = undefined; + } + this._animationDone.next(isExit ? 'void' : 'enter'); + this._isAnimating = false; + } } - /** Callback that is invoked when the panel animation completes. */ - _onAnimationDone(event: AnimationEvent) { - this._animationDone.next(event); - this._isAnimating = false; + protected _onAnimationStart(state: string) { + if (state === ENTER_ANIMATION || state === EXIT_ANIMATION) { + this._isAnimating = true; + } } - _onAnimationStart(event: AnimationEvent) { - this._isAnimating = true; - - // Scroll the content element to the top as soon as the animation starts. This is necessary, - // because we move focus to the first item while it's still being animated, which can throw - // the browser off when it determines the scroll position. Alternatively we can move focus - // when the animation is done, however moving focus asynchronously will interrupt screen - // readers which are in the process of reading out the menu already. We take the `element` - // from the `event` since we can't use a `ViewChild` to access the pane. - if (event.toState === 'enter' && this._keyManager.activeItemIndex === 0) { - event.element.scrollTop = 0; + _setIsOpen(isOpen: boolean) { + this._panelAnimationState = isOpen ? 'enter' : 'void'; + + if (isOpen) { + if (this._keyManager.activeItemIndex === 0) { + // Scroll the content element to the top as soon as the animation starts. This is necessary, + // because we move focus to the first item while it's still being animated, which can throw + // the browser off when it determines the scroll position. Alternatively we can move focus + // when the animation is done, however moving focus asynchronously will interrupt screen + // readers which are in the process of reading out the menu already. We take the `element` + // from the `event` since we can't use a `ViewChild` to access the pane. + const menuPanel = this._resolvePanel(); + + if (menuPanel) { + menuPanel.scrollTop = 0; + } + } + } else if (!this._animationsDisabled) { + // Some apps do `* { animation: none !important; }` in tests which will prevent the + // `animationend` event from firing. Since the exit animation is loading-bearing for + // removing the content from the DOM, add a fallback timer. + this._exitFallbackTimeout = setTimeout(() => this._onAnimationDone(EXIT_ANIMATION), 200); + } + + // Animation events won't fire when animations are disabled so we simulate them. + if (this._animationsDisabled) { + this._onAnimationDone(isOpen ? ENTER_ANIMATION : EXIT_ANIMATION); } + + this._changeDetectorRef.markForCheck(); } /** @@ -502,4 +523,19 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI this._directDescendantItems.notifyOnChanges(); }); } + + /** Gets the menu panel DOM node. */ + private _resolvePanel(): HTMLElement | null { + let menuPanel: HTMLElement | null = null; + + if (this._directDescendantItems.length) { + // Because the `mat-menuPanel` is at the DOM insertion point, not inside the overlay, we don't + // have a nice way of getting a hold of the menuPanel panel. We can't use a `ViewChild` either + // because the panel is inside an `ng-template`. We work around it by starting from one of + // the items and walking up the DOM. + menuPanel = this._directDescendantItems.first!._getHostElement().closest('[role="menu"]'); + } + + return menuPanel; + } } diff --git a/tools/public_api_guard/material/menu.md b/tools/public_api_guard/material/menu.md index 668b24addb2f..409dc9c1405d 100644 --- a/tools/public_api_guard/material/menu.md +++ b/tools/public_api_guard/material/menu.md @@ -6,7 +6,6 @@ import { AfterContentInit } from '@angular/core'; import { AfterViewInit } from '@angular/core'; -import { AnimationEvent as AnimationEvent_2 } from '@angular/animations'; import { AnimationTriggerMetadata } from '@angular/animations'; import { Direction } from '@angular/cdk/bidi'; import { EventEmitter } from '@angular/core'; @@ -54,7 +53,8 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI // (undocumented) addItem(_item: MatMenuItem): void; _allItems: QueryList; - readonly _animationDone: Subject; + readonly _animationDone: Subject<"void" | "enter">; + protected _animationsDisabled: boolean; ariaDescribedby: string; ariaLabel: string; ariaLabelledby: string; @@ -88,9 +88,9 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI ngOnDestroy(): void; // (undocumented) ngOnInit(): void; - _onAnimationDone(event: AnimationEvent_2): void; + protected _onAnimationDone(state: string): void; // (undocumented) - _onAnimationStart(event: AnimationEvent_2): void; + protected _onAnimationStart(state: string): void; overlapTrigger: boolean; overlayPanelClass: string | string[]; _panelAnimationState: 'void' | 'enter'; @@ -101,11 +101,11 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI // @deprecated removeItem(_item: MatMenuItem): void; resetActiveItem(): void; - _resetAnimation(): void; // @deprecated (undocumented) setElevation(_depth: number): void; + // (undocumented) + _setIsOpen(isOpen: boolean): void; setPositionClasses(posX?: MenuPositionX, posY?: MenuPositionY): void; - _startAnimation(): void; templateRef: TemplateRef; get xPosition(): MenuPositionX; set xPosition(value: MenuPositionX); @@ -117,7 +117,7 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnI static ɵfac: i0.ɵɵFactoryDeclaration; } -// @public +// @public @deprecated export const matMenuAnimations: { readonly transformMenu: AnimationTriggerMetadata; readonly fadeInItems: AnimationTriggerMetadata; @@ -127,7 +127,7 @@ export const matMenuAnimations: { export class MatMenuContent implements OnDestroy { constructor(...args: unknown[]); attach(context?: any): void; - readonly _attached: Subject; + _attachCount: number; detach(): void; // (undocumented) ngOnDestroy(): void;