From 4620df14f77d807fe444a11045e61e8de28b4c9b Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 13 Dec 2024 09:15:01 +0100 Subject: [PATCH] fix(material/menu): remove dependency on animations module (#30163) Second attempt at reworking the menu so it no longer depends on the animations module (after #30099). 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-trigger.ts | 73 ++++++++------- src/material/menu/menu.html | 9 +- src/material/menu/menu.scss | 33 ++++++- src/material/menu/menu.ts | 118 ++++++++++++++++-------- tools/public_api_guard/material/menu.md | 14 +-- 6 files changed, 166 insertions(+), 83 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-trigger.ts b/src/material/menu/menu-trigger.ts index 42bdbe2b8feb..e9fa1aa6cf17 100644 --- a/src/material/menu/menu-trigger.ts +++ b/src/material/menu/menu-trigger.ts @@ -40,7 +40,7 @@ import { } from '@angular/core'; import {normalizePassiveListenerOptions} from '@angular/cdk/platform'; import {merge, Observable, of as observableOf, Subscription} from 'rxjs'; -import {filter, takeUntil} from 'rxjs/operators'; +import {filter, take, takeUntil} from 'rxjs/operators'; import {MatMenu, MenuCloseReason} from './menu'; import {throwMatMenuRecursiveError} from './menu-errors'; import {MatMenuItem} from './menu-item'; @@ -115,6 +115,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { private _closingActionsSubscription = Subscription.EMPTY; private _hoverSubscription = Subscription.EMPTY; private _menuCloseSubscription = Subscription.EMPTY; + private _pendingRemoval: Subscription | undefined; /** * We're specifically looking for a `MatMenu` here since the generic `MatMenuPanel` @@ -247,6 +248,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { passiveEventListenerOptions, ); + this._pendingRemoval?.unsubscribe(); this._menuCloseSubscription.unsubscribe(); this._closingActionsSubscription.unsubscribe(); this._hoverSubscription.unsubscribe(); @@ -285,6 +287,16 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { return; } + this._pendingRemoval?.unsubscribe(); + const previousTrigger = PANELS_TO_TRIGGERS.get(menu); + PANELS_TO_TRIGGERS.set(menu, this); + + // 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(); + } + const overlayRef = this._createOverlay(menu); const overlayConfig = overlayRef.getConfig(); const positionStrategy = overlayConfig.positionStrategy as FlexibleConnectedPositionStrategy; @@ -292,17 +304,22 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { this._setPosition(menu, positionStrategy); overlayConfig.hasBackdrop = menu.hasBackdrop == null ? !this.triggersSubmenu() : menu.hasBackdrop; - overlayRef.attach(this._getPortal(menu)); - if (menu.lazyContent) { - menu.lazyContent.attach(this.menuData); + // We need the `hasAttached` check for the case where the user kicked off a removal animation, + // but re-entered the menu. Re-attaching the same portal will trigger an error otherwise. + if (!overlayRef.hasAttached()) { + overlayRef.attach(this._getPortal(menu)); + menu.lazyContent?.attach(this.menuData); } this._closingActionsSubscription = this._menuClosingActions().subscribe(() => this.closeMenu()); - this._initMenu(menu); + menu.parentMenu = this.triggersSubmenu() ? this._parentMaterialMenu : undefined; + menu.direction = this.dir; + menu.focusFirstItem(this._openedBy || 'program'); + this._setIsMenuOpen(true); 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. @@ -338,12 +355,28 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { /** Closes the menu and does the necessary cleanup. */ private _destroyMenu(reason: MenuCloseReason) { - if (!this._overlayRef || !this.menuOpen) { + const overlayRef = this._overlayRef; + const menu = this._menu; + + if (!overlayRef || !this.menuOpen) { return; } this._closingActionsSubscription.unsubscribe(); - this._overlayRef.detach(); + this._pendingRemoval?.unsubscribe(); + + // Note that we don't wait for the animation to finish if another trigger took + // over the menu, because the panel will end up empty which looks glitchy. + if (menu instanceof MatMenu && this._ownsMenu(menu)) { + this._pendingRemoval = menu._animationDone.pipe(take(1)).subscribe(() => overlayRef.detach()); + menu._setIsOpen(false); + } else { + overlayRef.detach(); + } + + if (menu && this._ownsMenu(menu)) { + PANELS_TO_TRIGGERS.delete(menu); + } // 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 @@ -355,30 +388,6 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { this._openedBy = undefined; this._setIsMenuOpen(false); - - if (this.menu && this._ownsMenu(this.menu)) { - PANELS_TO_TRIGGERS.delete(this.menu); - } - } - - /** - * This method sets the menu state to open and focuses the first item if - * 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'); - this._setIsMenuOpen(true); } // set state rather than toggle to support triggers sharing a menu 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.ts b/src/material/menu/menu.ts index e3ae2558567e..0d3a13d836c7 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,10 +139,10 @@ 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; + _isAnimating = false; /** Parent menu of the current menu panel. */ parentMenu: MatMenuPanel | undefined; @@ -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,58 @@ 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) { + setTimeout(() => { + this._onAnimationDone(isOpen ? ENTER_ANIMATION : EXIT_ANIMATION); + }); } + + this._changeDetectorRef.markForCheck(); } /** @@ -502,4 +525,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..9bc9954d6531 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;