Skip to content

Commit

Permalink
fix(angular/menu): remove dependency on animations module
Browse files Browse the repository at this point in the history
This should allow us to avoid some of the pitfalls of the animations module like occasional memory leaks and reduce the bundle size.
  • Loading branch information
mhaertwig committed Jan 13, 2025
1 parent 5719fda commit a710ab0
Show file tree
Hide file tree
Showing 7 changed files with 236 additions and 109 deletions.
2 changes: 2 additions & 0 deletions src/angular/menu/menu-animations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
112 changes: 63 additions & 49 deletions src/angular/menu/menu-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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`
Expand Down Expand Up @@ -300,6 +301,7 @@ export class SbbMenuTrigger
passiveEventListenerOptions,
);

this._pendingRemoval?.unsubscribe();
this._menuCloseSubscription.unsubscribe();
this._closingActionsSubscription.unsubscribe();
this._hoverSubscription.unsubscribe();
Expand Down Expand Up @@ -334,24 +336,57 @@ 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;

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.
Expand Down Expand Up @@ -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
Expand All @@ -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. */
Expand Down
9 changes: 6 additions & 3 deletions src/angular/menu/menu.html
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
27 changes: 26 additions & 1 deletion src/angular/menu/menu.scss
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,51 @@ 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;
align-items: flex-start;
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:
// * Users accidentally opening sub-menus when the `overlapTrigger` option is enabled.
// * 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;
}
}
Expand Down
36 changes: 36 additions & 0 deletions src/angular/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ import { ScrollDispatcher, ViewportRuler } from '@angular/cdk/scrolling';
import {
ChangeDetectionStrategy,
Component,
Directive,
ElementRef,
EventEmitter,
Input,
OnDestroy,
Output,
Provider,
QueryList,
Expand Down Expand Up @@ -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();
Expand Down
Loading

0 comments on commit a710ab0

Please sign in to comment.