From a710ab021c542377074837c26283bf51894c1804 Mon Sep 17 00:00:00 2001 From: u237004 Date: Mon, 13 Jan 2025 13:59:55 +0100 Subject: [PATCH] fix(angular/menu): remove dependency on animations module This should allow us to avoid some of the pitfalls of the animations module like occasional memory leaks and reduce the bundle size. --- src/angular/menu/menu-animations.ts | 2 + src/angular/menu/menu-trigger.ts | 112 ++++++++++++++----------- src/angular/menu/menu.html | 9 +- src/angular/menu/menu.scss | 27 +++++- src/angular/menu/menu.spec.ts | 36 ++++++++ src/angular/menu/menu.ts | 124 ++++++++++++++++------------ src/angular/usermenu/usermenu.scss | 35 +++++++- 7 files changed, 236 insertions(+), 109 deletions(-) diff --git a/src/angular/menu/menu-animations.ts b/src/angular/menu/menu-animations.ts index 0bf546f45f..ba3def5ce8 100644 --- a/src/angular/menu/menu-animations.ts +++ b/src/angular/menu/menu-animations.ts @@ -11,6 +11,8 @@ import { /** * Animations used by the sbb-menu component. * @docs-private + * @deprecated No longer being used, to be removed. + * @breaking-change 21.0.0 */ export const sbbMenuAnimations: { readonly transformMenu: AnimationTriggerMetadata; diff --git a/src/angular/menu/menu-trigger.ts b/src/angular/menu/menu-trigger.ts index c59ae733a1..c2e145b8b6 100644 --- a/src/angular/menu/menu-trigger.ts +++ b/src/angular/menu/menu-trigger.ts @@ -42,7 +42,7 @@ import { SCALING_FACTOR_5K, } from '@sbb-esta/angular/core'; import { merge, Observable, of as observableOf, Subscription } from 'rxjs'; -import { filter, takeUntil } from 'rxjs/operators'; +import { filter, take, takeUntil } from 'rxjs/operators'; import { SbbMenu, SbbMenuAnimationState, SbbMenuCloseReason } from './menu'; import { SbbMenuDynamicTrigger } from './menu-dynamic-trigger'; @@ -154,6 +154,7 @@ export class SbbMenuTrigger private _hoverSubscription = Subscription.EMPTY; private _menuCloseSubscription = Subscription.EMPTY; private _scrollStrategy = inject(SBB_MENU_SCROLL_STRATEGY); + private _pendingRemoval: Subscription | undefined; /** * We're specifically looking for a `SbbMenu` here since the generic `SbbMenuPanel` @@ -300,6 +301,7 @@ export class SbbMenuTrigger passiveEventListenerOptions, ); + this._pendingRemoval?.unsubscribe(); this._menuCloseSubscription.unsubscribe(); this._closingActionsSubscription.unsubscribe(); this._hoverSubscription.unsubscribe(); @@ -334,6 +336,34 @@ export class SbbMenuTrigger 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 triggerContext: SbbMenuTriggerContext = + this._type === 'headless' || this.triggersSubmenu() + ? { type: this._type } + : { + width: this._element.nativeElement.clientWidth, + height: this._element.nativeElement.clientHeight, + templateContent: this._triggerContent, + elementContent: this._triggerContent + ? undefined + : this._element.nativeElement.childElementCount === 0 + ? this._element.nativeElement.innerText + : this._sanitizer.bypassSecurityTrustHtml(this._element.nativeElement.innerHTML), + type: this._type, + scalingFactor: this._scalingFactor, + }; + + menu.triggerContext = { ...triggerContext, ...this._inheritedTriggerContext }; + const overlayRef = this._createOverlay(menu); const overlayConfig = overlayRef.getConfig(); const positionStrategy = overlayConfig.positionStrategy as FlexibleConnectedPositionStrategy; @@ -341,17 +371,22 @@ export class SbbMenuTrigger 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._parentSbbMenu : undefined; + menu.focusFirstItem(this._openedBy || 'program'); + this._setMenuElevation(menu); + this._setIsMenuOpen(true); if (menu instanceof SbbMenu) { - menu._startAnimation(); + menu._setIsOpen(true); menu._directDescendantItems.changes.pipe(takeUntil(menu.closed)).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. @@ -387,12 +422,32 @@ export class SbbMenuTrigger /** Closes the menu and does the necessary cleanup. */ private _destroyMenu(reason: SbbMenuCloseReason) { - 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 SbbMenu && this._ownsMenu(menu)) { + this._pendingRemoval = menu._animationDone.pipe(take(1)).subscribe(() => { + overlayRef.detach(); + menu.lazyContent?.detach(); + }); + menu._setIsOpen(false); + } else { + overlayRef.detach(); + menu?.lazyContent?.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 @@ -404,47 +459,6 @@ export class SbbMenuTrigger 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: SbbMenuPanel): 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._parentSbbMenu : undefined; - const triggerContext: SbbMenuTriggerContext = - this._type === 'headless' || this.triggersSubmenu() - ? { type: this._type } - : { - width: this._element.nativeElement.clientWidth, - height: this._element.nativeElement.clientHeight, - templateContent: this._triggerContent, - elementContent: this._triggerContent - ? undefined - : this._element.nativeElement.childElementCount === 0 - ? this._element.nativeElement.innerText - : this._sanitizer.bypassSecurityTrustHtml(this._element.nativeElement.innerHTML), - type: this._type, - scalingFactor: this._scalingFactor, - }; - - menu.triggerContext = { ...triggerContext, ...this._inheritedTriggerContext }; - this._setMenuElevation(menu); - menu.focusFirstItem(this._openedBy || 'program'); - this._setIsMenuOpen(true); } /** Updates the menu elevation based on the amount of parent menus that it has. */ diff --git a/src/angular/menu/menu.html b/src/angular/menu/menu.html index de1e0f0174..effb7c32f4 100644 --- a/src/angular/menu/menu.html +++ b/src/angular/menu/menu.html @@ -4,13 +4,16 @@ [id]="panelId" [class]="_classList" (click)="closed.emit('click')" - [@transformMenu]="_panelAnimationState" - (@transformMenu.start)="_onAnimationStart($event)" - (@transformMenu.done)="_onAnimationDone($event)" + [class.sbb-menu-panel-animations-disabled]="_animationsDisabled" + [class.sbb-menu-panel-exit-animation]="_panelAnimationState === 'void'" + [class.sbb-menu-panel-animating]="_isAnimating" [style.--sbb-menu-trigger-width.px]="triggerContext ? triggerContext.width : 0" [style.--sbb-menu-trigger-height.px]="triggerContext ? triggerContext.height : 0" 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/angular/menu/menu.scss b/src/angular/menu/menu.scss index 886806876a..3ff9305577 100644 --- a/src/angular/menu/menu.scss +++ b/src/angular/menu/menu.scss @@ -5,6 +5,23 @@ sbb-menu { display: none; } +@keyframes _sbb-menu-enter { + from { + opacity: 0; + } + to { + opacity: 1; + } +} +@keyframes _sbb-menu-exit { + from { + opacity: 1; + } + to { + opacity: 0; + } +} + .sbb-menu-panel-wrapper { display: flex; flex-direction: column; @@ -12,11 +29,19 @@ sbb-menu { min-width: calc(#{sbb.pxToRem(150)} * var(--sbb-scaling-factor)); max-width: calc(#{sbb.pxToRem(400)} * var(--sbb-scaling-factor)); outline: 0; + animation: _sbb-menu-enter 120ms cubic-bezier(0, 0, 0.2, 1); // Give the menu a minimum height so that the user can't // collapse it to zero when they scroll away. min-height: sbb.pxToRem(72); + &.sbb-menu-panel-exit-animation { + animation: _sbb-menu-exit 100ms 25ms linear forwards; + } + &.sbb-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: @@ -24,7 +49,7 @@ sbb-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 { + &.sbb-menu-panel-animating { pointer-events: none; } } diff --git a/src/angular/menu/menu.spec.ts b/src/angular/menu/menu.spec.ts index 8b4f07eee9..5ca615f0e4 100644 --- a/src/angular/menu/menu.spec.ts +++ b/src/angular/menu/menu.spec.ts @@ -15,9 +15,11 @@ import { ScrollDispatcher, ViewportRuler } from '@angular/cdk/scrolling'; import { ChangeDetectionStrategy, Component, + Directive, ElementRef, EventEmitter, Input, + OnDestroy, Output, Provider, QueryList, @@ -1203,6 +1205,40 @@ describe('SbbMenu', () => { .toBe(true); })); + it('should detach the lazy content when the menu is closed', fakeAsync(() => { + let destroyCount = 0; + + // Note: for some reason doing `spyOn(item, 'ngOnDestroy')` doesn't work, even though a + // `console.log` shows that the `ngOnDestroy` gets called. We work around it with a custom + // directive that increments a counter. + @Directive({ selector: '[sbb-menu-item]', standalone: false }) + class DestroyChecker implements OnDestroy { + ngOnDestroy(): void { + destroyCount++; + } + } + + const fixture = createComponent(SimpleLazyMenu, undefined, [DestroyChecker]); + fixture.detectChanges(); + fixture.componentInstance.trigger.openMenu(); + fixture.detectChanges(); + tick(500); + fixture.detectChanges(); + + expect(fixture.componentInstance.items.length).toBe(2); + expect(destroyCount).toBe(0); + + fixture.componentInstance.trigger.closeMenu(); + fixture.detectChanges(); + tick(500); + fixture.detectChanges(); + + expect(fixture.componentInstance.items.length) + .withContext('Expected items to be removed from query list') + .toBe(0); + expect(destroyCount).withContext('Expected ngOnDestroy to have been called').toBe(2); + })); + 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/angular/menu/menu.ts b/src/angular/menu/menu.ts index d06e126ac2..2819208f95 100644 --- a/src/angular/menu/menu.ts +++ b/src/angular/menu/menu.ts @@ -1,4 +1,3 @@ -import { AnimationEvent } from '@angular/animations'; import { FocusKeyManager, FocusOrigin, _IdGenerator } from '@angular/cdk/a11y'; import { DOWN_ARROW, ESCAPE, hasModifierKey, LEFT_ARROW, UP_ARROW } from '@angular/cdk/keycodes'; import { NgTemplateOutlet } from '@angular/common'; @@ -6,6 +5,7 @@ import { AfterContentInit, afterNextRender, AfterRenderRef, + ANIMATION_MODULE_TYPE, booleanAttribute, ChangeDetectionStrategy, ChangeDetectorRef, @@ -29,7 +29,6 @@ import { import { merge, Observable, Subject } from 'rxjs'; import { startWith, switchMap } from 'rxjs/operators'; -import { sbbMenuAnimations } from './menu-animations'; import { SbbMenuContent, SBB_MENU_CONTENT } from './menu-content'; import { throwSbbMenuInvalidPositionX, throwSbbMenuInvalidPositionY } from './menu-errors'; import { SbbMenuItem } from './menu-item'; @@ -89,6 +88,12 @@ export type SbbMenuAnimationState = SbbMenuPlainAnimationState | SbbMenuAnimatio /** Reason why the menu was closed. */ export type SbbMenuCloseReason = void | 'click' | 'keydown' | 'tab'; +/** Name of the enter animation `@keyframes`. */ +const ENTER_ANIMATION = '_sbb-menu-enter'; + +/** Name of the exit animation `@keyframes`. */ +const EXIT_ANIMATION = '_sbb-menu-exit'; + @Component({ selector: 'sbb-menu', templateUrl: 'menu.html', @@ -101,7 +106,6 @@ export type SbbMenuCloseReason = void | 'click' | 'keydown' | 'tab'; '[attr.aria-labelledby]': 'null', '[attr.aria-describedby]': 'null', }, - animations: [sbbMenuAnimations.transformMenu], providers: [{ provide: SBB_MENU_PANEL, useExisting: SbbMenu }], standalone: true, imports: [NgTemplateOutlet], @@ -110,11 +114,13 @@ export class SbbMenu implements AfterContentInit, SbbMenuPanel, OnI private _elementRef = inject>(ElementRef); private _defaultOptions = inject(SBB_MENU_DEFAULT_OPTIONS); private _changeDetectorRef = inject(ChangeDetectorRef); + private _injector = inject(Injector); private _keyManager: FocusKeyManager; private _xPosition: SbbMenuPositionX = this._defaultOptions.xPosition; private _yPosition: SbbMenuPositionY = this._defaultOptions.yPosition; private _firstItemFocusRef?: AfterRenderRef; + private _exitFallbackTimeout: ReturnType | undefined; private _previousElevation: string; private _elevationPrefix: string = 'sbb-elevation-z'; private _baseElevation: number = 4; @@ -132,10 +138,10 @@ export class SbbMenu implements AfterContentInit, SbbMenuPanel, OnI _panelAnimationState: SbbMenuAnimationState = 'void'; /** Emits whenever an animation on the menu completes. */ - readonly _animationDone: Subject = new Subject(); + readonly _animationDone = new Subject<'void' | 'enter'>(); /** Whether the menu is animating. */ - _isAnimating: boolean; + _isAnimating: boolean = false; /** Parent menu of the current menu panel. */ parentMenu: SbbMenuPanel | undefined; @@ -251,7 +257,9 @@ export class SbbMenu implements AfterContentInit, SbbMenuPanel, OnI readonly panelId = inject(_IdGenerator).getId('sbb-menu-panel-'); - private _injector = inject(Injector); + /** Whether animations are currently disabled. */ + protected _animationsDisabled: boolean = + inject(ANIMATION_MODULE_TYPE, { optional: true }) === 'NoopAnimations'; constructor(...args: unknown[]); constructor() {} @@ -302,6 +310,7 @@ export class SbbMenu implements AfterContentInit, SbbMenuPanel, OnI this._directDescendantItems.destroy(); this.closed.complete(); this._firstItemFocusRef?.destroy(); + clearTimeout(this._exitFallbackTimeout); } /** Stream that emits whenever the hovered menu item changes. */ @@ -355,15 +364,7 @@ export class SbbMenu implements AfterContentInit, SbbMenuPanel, OnI this._firstItemFocusRef?.destroy(); this._firstItemFocusRef = afterNextRender( () => { - let menuPanel: HTMLElement | null = null; - - if (this._directDescendantItems.length) { - // Because the `sbb-menu` panel is at the DOM insertion point, not inside the overlay, we don't - // have a nice way of getting a hold of the menu 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)) { @@ -436,56 +437,71 @@ export class SbbMenu implements AfterContentInit, SbbMenuPanel, OnI this._changeDetectorRef.markForCheck(); } - /** Starts the enter animation. */ - _startAnimation() { - // @breaking-change 8.0.0 Combine with _resetAnimation. - this._panelAnimationState = - this.triggerContext.animationStartStateResolver?.(this.triggerContext) ?? '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. - const animationStartState = - this._extractPlainAnimationState( - this.triggerContext.animationStartStateResolver?.(this.triggerContext), - ) ?? 'enter'; - if (event.toState === animationStartState && 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); } - if (event.toState === 'void') { - event.element.classList.add('sbb-menu-panel-closing'); - } else { - event.element.classList.remove('sbb-menu-panel-closing'); + // 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(); } - private _extractPlainAnimationState( - state?: SbbMenuAnimationState, - ): SbbMenuPlainAnimationState | null { - if (!state) { - return null; + /** 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 typeof state === 'string' ? state : state.value; + return menuPanel; } /** diff --git a/src/angular/usermenu/usermenu.scss b/src/angular/usermenu/usermenu.scss index 1ee413501f..aaa4da9697 100644 --- a/src/angular/usermenu/usermenu.scss +++ b/src/angular/usermenu/usermenu.scss @@ -127,7 +127,7 @@ text-align: left; @include sbb.ellipsis(); - .sbb-menu-panel-type-usermenu.ng-animating & { + .sbb-menu-panel-type-usermenu.sbb-menu-panel-animating & { text-overflow: clip; // Prevents showing ellipsis during animation } } @@ -230,6 +230,31 @@ } } +@keyframes _sbb-usermenu-enter { + from { + width: var(--sbb-menu-trigger-width); + max-height: var(--sbb-menu-trigger-height); + opacity: 0; + } + to { + width: 288px; + max-height: 100%; + opacity: 1; + } +} +@keyframes _sbb-usermenu-exit { + from { + width: 288px; + max-height: 100%; + opacity: 1; + } + to { + width: var(--sbb-menu-trigger-width); + max-height: var(--sbb-menu-trigger-height); + opacity: 0; + } +} + .sbb-menu-panel-wrapper.sbb-menu-panel-type-usermenu { max-width: 100%; position: absolute; @@ -239,11 +264,17 @@ min-width: calc(var(--sbb-menu-trigger-width) + var(--sbb-usermenu-trigger-padding)); min-height: var(--sbb-menu-trigger-height); + animation: _sbb-usermenu-enter 300ms cubic-bezier(0.785, 0.135, 0.15, 0.86); + + &.sbb-menu-panel-exit-animation { + animation: _sbb-usermenu-exit 300ms cubic-bezier(0.785, 0.135, 0.15, 0.86); + } + .sbb-menu-item { min-width: calc(var(--sbb-usermenu-panel-width) - var(--sbb-border-width) * 2); } - &.ng-animating { + &.sbb-menu-panel-animating { .sbb-panel { overflow: hidden; // Disable scroll bars during animation }