Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cdk/overlay): use MutationObserver to detach overlay #30703

Merged
merged 1 commit into from
Mar 27, 2025
Merged

fix(cdk/overlay): use MutationObserver to detach overlay #30703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 64 additions & 54 deletions src/cdk/overlay/overlay-ref.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@
* found in the LICENSE file at https://angular.dev/license
*/

import {Direction, Directionality} from '../bidi';
import {ComponentPortal, Portal, PortalOutlet, TemplatePortal} from '../portal';
import {Location} from '@angular/common';
import {
AfterRenderRef,
ComponentRef,
Expand All @@ -16,19 +15,17 @@ import {
NgZone,
Renderer2,
afterNextRender,
afterRender,
untracked,
} from '@angular/core';
import {Location} from '@angular/common';
import {Observable, Subject, merge, SubscriptionLike, Subscription} from 'rxjs';
import {takeUntil} from 'rxjs/operators';
import {Observable, Subject, Subscription, SubscriptionLike} from 'rxjs';
import {Direction, Directionality} from '../bidi';
import {coerceArray, coerceCssPixelValue} from '../coercion';
import {ComponentPortal, Portal, PortalOutlet, TemplatePortal} from '../portal';
import {BackdropRef} from './backdrop-ref';
import {OverlayKeyboardDispatcher} from './dispatchers/overlay-keyboard-dispatcher';
import {OverlayOutsideClickDispatcher} from './dispatchers/overlay-outside-click-dispatcher';
import {OverlayConfig} from './overlay-config';
import {coerceCssPixelValue, coerceArray} from '../coercion';
import {PositionStrategy} from './position/position-strategy';
import {ScrollStrategy} from './scroll';
import {BackdropRef} from './backdrop-ref';

/** An object where all of its properties cannot be written. */
export type ImmutableObject<T> = {
Expand All @@ -47,6 +44,8 @@ export class OverlayRef implements PortalOutlet {
private _scrollStrategy: ScrollStrategy | undefined;
private _locationChanges: SubscriptionLike = Subscription.EMPTY;
private _backdropRef: BackdropRef | null = null;
private _detachContentMutationObserver: MutationObserver | undefined;
private _detachContentAfterRenderRef: AfterRenderRef | undefined;

/**
* Reference to the parent of the `_host` at the time it was detached. Used to restore
Expand All @@ -60,10 +59,6 @@ export class OverlayRef implements PortalOutlet {
/** Stream of mouse outside events dispatched to this overlay. */
readonly _outsidePointerEvents = new Subject<MouseEvent>();

private _renders = new Subject<void>();

private _afterRenderRef: AfterRenderRef;

/** Reference to the currently-running `afterNextRender` call. */
private _afterNextRenderRef: AfterRenderRef | undefined;

Expand All @@ -87,18 +82,6 @@ export class OverlayRef implements PortalOutlet {
}

this._positionStrategy = _config.positionStrategy;

// Users could open the overlay from an `effect`, in which case we need to
// run the `afterRender` as `untracked`. We don't recommend that users do
// this, but we also don't want to break users who are doing it.
this._afterRenderRef = untracked(() =>
afterRender(
() => {
this._renders.next();
},
{injector: this._injector},
),
);
}

/** The overlay's HTML element */
Expand Down Expand Up @@ -182,6 +165,7 @@ export class OverlayRef implements PortalOutlet {

// Only emit the `attachments` event once all other setup is done.
this._attachments.next();
this._completeDetachContent();

// Track this overlay by the keyboard dispatcher
this._keyboardDispatcher.add(this);
Expand Down Expand Up @@ -242,6 +226,7 @@ export class OverlayRef implements PortalOutlet {

// Only emit after everything is detached.
this._detachments.next();
this._completeDetachContent();

// Remove this overlay from keyboard dispatcher tracking.
this._keyboardDispatcher.remove(this);
Expand Down Expand Up @@ -281,8 +266,7 @@ export class OverlayRef implements PortalOutlet {
}

this._detachments.complete();
this._afterRenderRef.destroy();
this._renders.complete();
this._completeDetachContent();
}

/** Whether the overlay has attached content. */
Expand Down Expand Up @@ -488,34 +472,60 @@ export class OverlayRef implements PortalOutlet {
}
}

/** Detaches the overlay content next time the zone stabilizes. */
/** Detaches the overlay once the content finishes animating and is removed from the DOM. */
private _detachContentWhenEmpty() {
// Normally we wouldn't have to explicitly run this outside the `NgZone`, however
// if the consumer is using `zone-patch-rxjs`, the `Subscription.unsubscribe` call will
// be patched to run inside the zone, which will throw us into an infinite loop.
this._ngZone.runOutsideAngular(() => {
// We can't remove the host here immediately, because the overlay pane's content
// might still be animating. This stream helps us avoid interrupting the animation
// by waiting for the pane to become empty.
const subscription = this._renders
.pipe(takeUntil(merge(this._attachments, this._detachments)))
.subscribe(() => {
// Needs a couple of checks for the pane and host, because
// they may have been removed by the time the zone stabilizes.
if (!this._pane || !this._host || this._pane.children.length === 0) {
if (this._pane && this._config.panelClass) {
this._toggleClasses(this._pane, this._config.panelClass, false);
}

if (this._host && this._host.parentElement) {
this._previousHostParent = this._host.parentElement;
this._host.remove();
}

subscription.unsubscribe();
}
});
});
let rethrow = false;
// Attempt to detach on the next render.
try {
this._detachContentAfterRenderRef = afterNextRender(
() => {
// Rethrow if we encounter an actual error detaching.
rethrow = true;
this._detachContent();
},
{
injector: this._injector,
},
);
} catch (e) {
if (rethrow) {
throw e;
}
// afterNextRender throws if the EnvironmentInjector is has already been destroyed.
// This may happen in tests that don't properly flush all async work.
// In order to avoid breaking those tests, we just detach immediately in this case.
this._detachContent();
}
// Otherwise wait until the content finishes animating out and detach.
if (globalThis.MutationObserver && this._pane) {
this._detachContentMutationObserver ||= new globalThis.MutationObserver(() => {
this._detachContent();
});
this._detachContentMutationObserver.observe(this._pane, {childList: true});
}
}

private _detachContent() {
// Needs a couple of checks for the pane and host, because
// they may have been removed by the time the zone stabilizes.
if (!this._pane || !this._host || this._pane.children.length === 0) {
if (this._pane && this._config.panelClass) {
this._toggleClasses(this._pane, this._config.panelClass, false);
}

if (this._host && this._host.parentElement) {
this._previousHostParent = this._host.parentElement;
this._host.remove();
}

this._completeDetachContent();
}
}

private _completeDetachContent() {
this._detachContentAfterRenderRef?.destroy();
this._detachContentAfterRenderRef = undefined;
this._detachContentMutationObserver?.disconnect();
}

/** Disposes of a scroll strategy. */
Expand Down
17 changes: 13 additions & 4 deletions src/cdk/overlay/overlay.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import {Direction, Directionality} from '../bidi';
import {CdkPortal, ComponentPortal, TemplatePortal} from '../portal';
import {Location} from '@angular/common';
import {SpyLocation} from '@angular/common/testing';
import {
Expand All @@ -21,6 +19,8 @@ import {
waitForAsync,
} from '@angular/core/testing';
import {NoopAnimationsModule} from '@angular/platform-browser/animations';
import {Direction, Directionality} from '../bidi';
import {CdkPortal, ComponentPortal, TemplatePortal} from '../portal';
import {dispatchFakeEvent} from '../testing/private';
import {
Overlay,
Expand Down Expand Up @@ -380,8 +380,9 @@ describe('Overlay', () => {
expect(overlayRef.getDirection()).toBe('ltr');
});

it('should add and remove the overlay host as the ref is being attached and detached', () => {
it('should add and remove the overlay host as the ref is being attached and detached', async () => {
const overlayRef = overlay.create();
const pane = overlayContainerElement.querySelector('.cdk-overlay-pane') as HTMLElement;

overlayRef.attach(componentPortal);
viewContainerFixture.detectChanges();
Expand All @@ -390,13 +391,17 @@ describe('Overlay', () => {
.withContext('Expected host element to be in the DOM.')
.toBeTruthy();

// Simulate animating element that hasn't been removed yet.
pane.appendChild(document.createElement('div'));
overlayRef.detach();

expect(overlayRef.hostElement.parentElement)
.withContext('Expected host element not to have been removed immediately.')
.toBeTruthy();

viewContainerFixture.detectChanges();
pane.children[0].remove();
await new Promise(r => setTimeout(r));

expect(overlayRef.hostElement.parentElement)
.withContext('Expected host element to have been removed once the zone stabilizes.')
Expand Down Expand Up @@ -922,7 +927,7 @@ describe('Overlay', () => {
expect(pane.classList).toContain('custom-class-two');
});

it('should remove the custom panel class when the overlay is detached', () => {
it('should remove the custom panel class when the overlay is detached', async () => {
const config = new OverlayConfig({panelClass: 'custom-panel-class'});
const overlayRef = overlay.create(config);

Expand Down Expand Up @@ -957,12 +962,16 @@ describe('Overlay', () => {
.withContext('Expected class to be added')
.toContain('custom-panel-class');

// Simulate animating element that hasn't been removed yet.
pane.appendChild(document.createElement('div'));
overlayRef.detach();
expect(pane.classList)
.withContext('Expected class not to be removed immediately')
.toContain('custom-panel-class');
await viewContainerFixture.whenStable();

pane.children[0].remove();
await new Promise(r => setTimeout(r));
expect(pane.classList)
.not.withContext('Expected class to be removed on stable')
.toContain('custom-panel-class');
Expand Down
Loading