Skip to content

Commit

Permalink
fix(datepicker): memory leak in popup mode
Browse files Browse the repository at this point in the history 
Fixes the datepicker leaking memory when it is opened and closed in popup mode. The leak seems to come from the fact that we're detaching, rather than disposing of, the overlay when it is closed. We did this in the past to support an animation while closing the calendar, but it seems to cause the animations engine further down the line to retain the elements memory since they aren't being removed immediately.

These changes resolve the issue by triggering the exit animation, waiting for it to finish and disposing of the overlay. We weren't gaining too much by reusing the overlay from the previous approach, because the calendar was being removed and re-created every time anyway.

Fixes angular#17896.
  • Loading branch information
@crisbeto
crisbeto committed Jan 10, 2020
1 parent a909f81 commit 8dcf15c
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 51 deletions.
106 changes: 58 additions & 48 deletions src/material/datepicker/datepicker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
ViewChild,
ViewContainerRef,
ViewEncapsulation,
ChangeDetectorRef,
} from '@angular/core';
import {
CanColor,
Expand Down Expand Up @@ -92,7 +93,8 @@ const _MatDatepickerContentMixinBase: CanColorCtor & typeof MatDatepickerContent
styleUrls: ['datepicker-content.css'],
host: {
'class': 'mat-datepicker-content',
'[@transformPanel]': '"enter"',
'[@transformPanel]': '_animationState',
'(@transformPanel.done)': '_animationDone.next()',
'[class.mat-datepicker-content-touch]': 'datepicker.touchUi',
},
animations: [
Expand All @@ -105,7 +107,7 @@ const _MatDatepickerContentMixinBase: CanColorCtor & typeof MatDatepickerContent
inputs: ['color'],
})
export class MatDatepickerContent<D> extends _MatDatepickerContentMixinBase
implements AfterViewInit, CanColor {
implements AfterViewInit, OnDestroy, CanColor {

/** Reference to the internal calendar component. */
@ViewChild(MatCalendar) _calendar: MatCalendar<D>;
Expand All @@ -116,13 +118,38 @@ export class MatDatepickerContent<D> extends _MatDatepickerContentMixinBase
/** Whether the datepicker is above or below the input. */
_isAbove: boolean;

constructor(elementRef: ElementRef) {
/** Current state of the animation. */
_animationState: 'enter' | 'void' = 'enter';

/** Emits when an animation has finished. */
_animationDone = new Subject<void>();

constructor(
elementRef: ElementRef,
/**
* @deprecated `_changeDetectorRef` parameter to become required.
* @breaking-change 11.0.0
*/
private _changeDetectorRef?: ChangeDetectorRef) {
super(elementRef);
}

ngAfterViewInit() {
this._calendar.focusActiveCell();
}

ngOnDestroy() {
this._animationDone.complete();
}

_startExitAnimation() {
this._animationState = 'void';

// @breaking-change 11.0.0 Remove null check for `_changeDetectorRef`.
if (this._changeDetectorRef) {
this._changeDetectorRef.markForCheck();
}
}
}


Expand Down Expand Up @@ -250,14 +277,11 @@ export class MatDatepicker<D> implements OnDestroy, CanColor {
}

/** A reference to the overlay when the calendar is opened as a popup. */
_popupRef: OverlayRef;
private _popupRef: OverlayRef | null;

/** A reference to the dialog when the calendar is opened as a dialog. */
private _dialogRef: MatDialogRef<MatDatepickerContent<D>> | null;

/** A portal containing the calendar for this datepicker. */
private _calendarPortal: ComponentPortal<MatDatepickerContent<D>>;

/** Reference to the component instantiated in popup mode. */
private _popupComponentRef: ComponentRef<MatDatepickerContent<D>> | null;

Expand Down Expand Up @@ -292,14 +316,10 @@ export class MatDatepicker<D> implements OnDestroy, CanColor {
}

ngOnDestroy() {
this._destroyPopup();
this.close();
this._inputSubscription.unsubscribe();
this._disabledChange.complete();

if (this._popupRef) {
this._popupRef.dispose();
this._popupComponentRef = null;
}
}

/** Selects the given date */
Expand Down Expand Up @@ -356,16 +376,15 @@ export class MatDatepicker<D> implements OnDestroy, CanColor {
if (!this._opened) {
return;
}
if (this._popupRef && this._popupRef.hasAttached()) {
this._popupRef.detach();
if (this._popupComponentRef && this._popupRef) {
const instance = this._popupComponentRef.instance;
instance._startExitAnimation();
instance._animationDone.pipe(take(1)).subscribe(() => this._destroyPopup());
}
if (this._dialogRef) {
this._dialogRef.close();
this._dialogRef = null;
}
if (this._calendarPortal && this._calendarPortal.isAttached) {
this._calendarPortal.detach();
}

const completeClose = () => {
// The `_opened` could've been reset already if
Expand Down Expand Up @@ -409,30 +428,24 @@ export class MatDatepicker<D> implements OnDestroy, CanColor {

this._dialogRef.afterClosed().subscribe(() => this.close());
this._dialogRef.componentInstance.datepicker = this;
this._setColor();
this._dialogRef.componentInstance.color = this.color;
}

/** Open the calendar as a popup. */
private _openAsPopup(): void {
if (!this._calendarPortal) {
this._calendarPortal = new ComponentPortal<MatDatepickerContent<D>>(MatDatepickerContent,
this._viewContainerRef);
}

if (!this._popupRef) {
this._createPopup();
}

if (!this._popupRef.hasAttached()) {
this._popupComponentRef = this._popupRef.attach(this._calendarPortal);
this._popupComponentRef.instance.datepicker = this;
this._setColor();

// Update the position once the calendar has rendered.
this._ngZone.onStable.asObservable().pipe(take(1)).subscribe(() => {
this._popupRef.updatePosition();
});
}
const portal = new ComponentPortal<MatDatepickerContent<D>>(MatDatepickerContent,
this._viewContainerRef);

this._destroyPopup();
this._createPopup();
const ref = this._popupComponentRef = this._popupRef!.attach(portal);
ref.instance.datepicker = this;
ref.instance.color = this.color;

// Update the position once the calendar has rendered.
this._ngZone.onStable.asObservable().pipe(take(1)).subscribe(() => {
this._popupRef!.updatePosition();
});
}

/** Create the popup. */
Expand Down Expand Up @@ -466,6 +479,14 @@ export class MatDatepicker<D> implements OnDestroy, CanColor {
});
}

/** Destroys the current popup overlay. */
private _destroyPopup() {
if (this._popupRef) {
this._popupRef.dispose();
this._popupRef = this._popupComponentRef = null;
}
}

/** Create the popup PositionStrategy. */
private _createPopupPositionStrategy(): PositionStrategy {
return this._overlay.position()
Expand Down Expand Up @@ -510,17 +531,6 @@ export class MatDatepicker<D> implements OnDestroy, CanColor {
return (this._dateAdapter.isDateInstance(obj) && this._dateAdapter.isValid(obj)) ? obj : null;
}

/** Passes the current theme color along to the calendar overlay. */
private _setColor(): void {
const color = this.color;
if (this._popupComponentRef) {
this._popupComponentRef.instance.color = color;
}
if (this._dialogRef) {
this._dialogRef.componentInstance.color = color;
}
}

static ngAcceptInputType_disabled: BooleanInput;
static ngAcceptInputType_touchUi: BooleanInput;
}
10 changes: 7 additions & 3 deletions tools/public_api_guard/material/datepicker.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ export declare class MatDatepicker<D> implements OnDestroy, CanColor {
readonly _disabledChange: Subject<boolean>;
readonly _maxDate: D | null;
readonly _minDate: D | null;
_popupRef: OverlayRef;
_selected: D | null;
readonly _selectedChanged: Subject<D>;
calendarHeaderComponent: ComponentType<any>;
Expand Down Expand Up @@ -144,12 +143,17 @@ export declare const matDatepickerAnimations: {
readonly fadeInCalendar: AnimationTriggerMetadata;
};

export declare class MatDatepickerContent<D> extends _MatDatepickerContentMixinBase implements AfterViewInit, CanColor {
export declare class MatDatepickerContent<D> extends _MatDatepickerContentMixinBase implements AfterViewInit, OnDestroy, CanColor {
_animationDone: Subject<void>;
_animationState: 'enter' | 'void';
_calendar: MatCalendar<D>;
_isAbove: boolean;
datepicker: MatDatepicker<D>;
constructor(elementRef: ElementRef);
constructor(elementRef: ElementRef,
_changeDetectorRef?: ChangeDetectorRef | undefined);
_startExitAnimation(): void;
ngAfterViewInit(): void;
ngOnDestroy(): void;
static ɵcmp: i0.ɵɵComponentDefWithMeta<MatDatepickerContent<any>, "mat-datepicker-content", ["matDatepickerContent"], { "color": "color"; }, {}, never>;
static ɵfac: i0.ɵɵFactoryDef<MatDatepickerContent<any>>;
}
Expand Down

0 comments on commit 8dcf15c

Please sign in to comment.