Skip to content

Commit a880f87

Browse files
committed
fix(cdk/overlay): backdrop timeouts not being cleared in some cases
When we start the detach sequence of a backdrop, we bind a `transitionend` event as well as a 500ms timeout in case the event doesn't fire. If the event fires before 500ms, we clear the timeout so users don't have to flush it in tests, however we had the following problems: 1. Transparent backdrops don't have a transition so they were always being cleaned up after the 500ms timeout. 2. We weren't clearing the timeout if the overlay was disposed while the backdrop transition is running. This meant that the timeout would still be in memory, even though the element was removed from the DOM. 3. We had a memory leak where the `click` and `transitionend` events weren't being removed from the backdrop if the overlay is disposed while detaching. Basically all Material components had one of these two issues. These changes resolve the problems by: 1. Clearing the timeout when the overlay is disposed. 2. Setting a 1ms transition on the transparent overlay so its `transitionend` can fire (almost) instantly. 3. Removing the `click` and `transitionend` events when the backdrop is disposed.
1 parent 69598f1 commit a880f87

File tree

3 files changed

+29
-25
lines changed

3 files changed

+29
-25
lines changed

src/cdk/overlay/_index.scss

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -96,13 +96,8 @@ $backdrop-animation-timing-function: cubic-bezier(0.25, 0.8, 0.25, 1) !default;
9696
}
9797

9898
.cdk-overlay-transparent-backdrop {
99-
// Note: as of Firefox 57, having the backdrop be `background: none` will prevent it from
100-
// capturing the user's mouse scroll events. Since we also can't use something like
101-
// `rgba(0, 0, 0, 0)`, we work around the inconsistency by not setting the background at
102-
// all and using `opacity` to make the element transparent.
103-
&, &.cdk-overlay-backdrop-showing {
104-
opacity: 0;
105-
}
99+
// Leave a short transition for transparent backdrops so the `transitionend` event fires.
100+
transition-duration: 1ms;
106101
}
107102

108103
// Overlay parent element used with the connected position strategy. Used to constrain the

src/cdk/overlay/overlay-ref.ts

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,17 @@ export type ImmutableObject<T> = {
3131
*/
3232
export class OverlayRef implements PortalOutlet, OverlayReference {
3333
private _backdropElement: HTMLElement | null = null;
34+
private _backdropTimeout: number | undefined;
3435
private readonly _backdropClick = new Subject<MouseEvent>();
3536
private readonly _attachments = new Subject<void>();
3637
private readonly _detachments = new Subject<void>();
3738
private _positionStrategy: PositionStrategy | undefined;
3839
private _scrollStrategy: ScrollStrategy | undefined;
3940
private _locationChanges: SubscriptionLike = Subscription.EMPTY;
4041
private _backdropClickHandler = (event: MouseEvent) => this._backdropClick.next(event);
42+
private _backdropTransitionendHandler = (event: TransitionEvent) => {
43+
this._disposeBackdrop(event.target as HTMLElement | null);
44+
};
4145

4246
/**
4347
* Reference to the parent of the `_host` at the time it was detached. Used to restore
@@ -418,26 +422,10 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
418422
return;
419423
}
420424

421-
let timeoutId: number;
422-
const finishDetach = () => {
423-
// It may not be attached to anything in certain cases (e.g. unit tests).
424-
if (backdropToDetach) {
425-
backdropToDetach.removeEventListener('click', this._backdropClickHandler);
426-
backdropToDetach.removeEventListener('transitionend', finishDetach);
427-
this._disposeBackdrop(backdropToDetach);
428-
}
429-
430-
if (this._config.backdropClass) {
431-
this._toggleClasses(backdropToDetach!, this._config.backdropClass, false);
432-
}
433-
434-
clearTimeout(timeoutId);
435-
};
436-
437425
backdropToDetach.classList.remove('cdk-overlay-backdrop-showing');
438426

439427
this._ngZone.runOutsideAngular(() => {
440-
backdropToDetach!.addEventListener('transitionend', finishDetach);
428+
backdropToDetach!.addEventListener('transitionend', this._backdropTransitionendHandler);
441429
});
442430

443431
// If the backdrop doesn't have a transition, the `transitionend` event won't fire.
@@ -447,7 +435,11 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
447435
// Run this outside the Angular zone because there's nothing that Angular cares about.
448436
// If it were to run inside the Angular zone, every test that used Overlay would have to be
449437
// either async or fakeAsync.
450-
timeoutId = this._ngZone.runOutsideAngular(() => setTimeout(finishDetach, 500));
438+
this._backdropTimeout = this._ngZone.runOutsideAngular(() =>
439+
setTimeout(() => {
440+
this._disposeBackdrop(backdropToDetach);
441+
}, 500),
442+
);
451443
}
452444

453445
/** Toggles a single CSS class or an array of classes on an element. */
@@ -505,6 +497,8 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
505497
/** Removes a backdrop element from the DOM. */
506498
private _disposeBackdrop(backdrop: HTMLElement | null) {
507499
if (backdrop) {
500+
backdrop.removeEventListener('click', this._backdropClickHandler);
501+
backdrop.removeEventListener('transitionend', this._backdropTransitionendHandler);
508502
backdrop.remove();
509503

510504
// It is possible that a new portal has been attached to this overlay since we started
@@ -514,6 +508,11 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
514508
this._backdropElement = null;
515509
}
516510
}
511+
512+
if (this._backdropTimeout) {
513+
clearTimeout(this._backdropTimeout);
514+
this._backdropTimeout = undefined;
515+
}
517516
}
518517
}
519518

src/cdk/overlay/overlay.spec.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,16 @@ describe('Overlay', () => {
348348
// `fakeAsync` will throw if we have an unflushed timer.
349349
}));
350350

351+
it('should clear the backdrop timeout if the overlay is disposed', fakeAsync(() => {
352+
const overlayRef = overlay.create({hasBackdrop: true});
353+
overlayRef.attach(componentPortal);
354+
overlayRef.detach();
355+
overlayRef.dispose();
356+
357+
// Note: we don't `tick` or `flush` here. The assertion is that
358+
// `fakeAsync` will throw if we have an unflushed timer.
359+
}));
360+
351361
it('should be able to use the `Overlay` provider during app initialization', () => {
352362
/** Dummy provider that depends on `Overlay`. */
353363
@Injectable()

0 commit comments

Comments
 (0)