Skip to content

Commit 2ed15d8

Browse files
crisbetowagnermaciel
authored andcommitted
fix(cdk/overlay): avoid unnecessary timeouts when disposing of overlay (#23474)
The `OverlayRef.dispose` method is meant to destroy the overlay immediately, without waiting on any events, which is useful for tests or when the overlay has already animated away. The problem is that we use the `detachBackdrop` method which will animate the backdrop away and trigger a `setTimeout` in case the backdrop didn't animate. These changes avoid timeouts by removing the backdrop immediately on destroy. This reduces the number of `flush` calls that we need to be done by the consumer. (cherry picked from commit 2f655c9)
1 parent aad5569 commit 2ed15d8

File tree

2 files changed

+42
-14
lines changed

2 files changed

+42
-14
lines changed

src/cdk/overlay/overlay-ref.ts

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
207207
}
208208

209209
this._disposeScrollStrategy();
210-
this.detachBackdrop();
210+
this._disposeBackdrop(this._backdropElement);
211211
this._locationChanges.unsubscribe();
212212
this._keyboardDispatcher.remove(this);
213213
this._portalOutlet.dispose();
@@ -419,29 +419,19 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
419419

420420
/** Detaches the backdrop (if any) associated with the overlay. */
421421
detachBackdrop(): void {
422-
let backdropToDetach = this._backdropElement;
422+
const backdropToDetach = this._backdropElement;
423423

424424
if (!backdropToDetach) {
425425
return;
426426
}
427427

428428
let timeoutId: number;
429-
let finishDetach = () => {
429+
const finishDetach = () => {
430430
// It may not be attached to anything in certain cases (e.g. unit tests).
431431
if (backdropToDetach) {
432432
backdropToDetach.removeEventListener('click', this._backdropClickHandler);
433433
backdropToDetach.removeEventListener('transitionend', finishDetach);
434-
435-
if (backdropToDetach.parentNode) {
436-
backdropToDetach.parentNode.removeChild(backdropToDetach);
437-
}
438-
}
439-
440-
// It is possible that a new portal has been attached to this overlay since we started
441-
// removing the backdrop. If that is the case, only clear the backdrop reference if it
442-
// is still the same instance that we started to remove.
443-
if (this._backdropElement == backdropToDetach) {
444-
this._backdropElement = null;
434+
this._disposeBackdrop(backdropToDetach);
445435
}
446436

447437
if (this._config.backdropClass) {
@@ -522,6 +512,22 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
522512
}
523513
}
524514
}
515+
516+
/** Removes a backdrop element from the DOM. */
517+
private _disposeBackdrop(backdrop: HTMLElement | null) {
518+
if (backdrop) {
519+
if (backdrop.parentNode) {
520+
backdrop.parentNode.removeChild(backdrop);
521+
}
522+
523+
// It is possible that a new portal has been attached to this overlay since we started
524+
// removing the backdrop. If that is the case, only clear the backdrop reference if it
525+
// is still the same instance that we started to remove.
526+
if (this._backdropElement === backdrop) {
527+
this._backdropElement = null;
528+
}
529+
}
530+
}
525531
}
526532

527533

src/cdk/overlay/overlay.spec.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,6 +577,28 @@ describe('Overlay', () => {
577577
expect(strategy.dispose).not.toHaveBeenCalled();
578578
}));
579579

580+
it('should not throw when disposing multiple times in a row', () => {
581+
const overlayRef = overlay.create();
582+
overlayRef.attach(componentPortal);
583+
584+
expect(overlayContainerElement.textContent).toContain('Pizza');
585+
586+
expect(() => {
587+
overlayRef.dispose();
588+
overlayRef.dispose();
589+
overlayRef.dispose();
590+
}).not.toThrow();
591+
});
592+
593+
it('should not trigger timers when disposing of an overlay', fakeAsync(() => {
594+
const overlayRef = overlay.create({hasBackdrop: true});
595+
overlayRef.attach(templatePortal);
596+
overlayRef.dispose();
597+
598+
// The assertion here is that `fakeAsync` doesn't flag
599+
// any pending timeouts after the test is done.
600+
}));
601+
580602
});
581603

582604
describe('size', () => {

0 commit comments

Comments
 (0)