Skip to content

Commit

Permalink
fix(material/tooltip): clear pending timers on destroy (#16756)
Browse files Browse the repository at this point in the history
Currently we fire off some timers when a tooltip is shown or hidden, however if it gets
destroyed before the timer has elapsed, we still end up executing some logic (change
detection in this case). These changes clear the timers so that we don't end up triggering
errors by accident.
  • Loading branch information
crisbeto authored Feb 10, 2021
1 parent 1a10027 commit 70ff528
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 17 deletions.
27 changes: 27 additions & 0 deletions src/material-experimental/mdc-tooltip/tooltip.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,33 @@ describe('MDC-based MatTooltip', () => {
expect(classList).toContain('mat-tooltip-panel-left');
}));

it('should clear the show timeout on destroy', fakeAsync(() => {
assertTooltipInstance(tooltipDirective, false);

tooltipDirective.show(1000);
fixture.detectChanges();

// Note that we aren't asserting anything, but `fakeAsync` will
// throw if we have any timers by the end of the test.
fixture.destroy();
}));

it('should clear the hide timeout on destroy', fakeAsync(() => {
assertTooltipInstance(tooltipDirective, false);

tooltipDirective.show();
tick(0);
fixture.detectChanges();
tick(500);

tooltipDirective.hide(1000);
fixture.detectChanges();

// Note that we aren't asserting anything, but `fakeAsync` will
// throw if we have any timers by the end of the test.
fixture.destroy();
}));

});

describe('fallback positions', () => {
Expand Down
33 changes: 30 additions & 3 deletions src/material/tooltip/tooltip.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,17 +113,17 @@ describe('MatTooltip', () => {

fixture.detectChanges();

// wait till animation has finished
// Wait until the animation has finished.
tick(500);

// Make sure tooltip is shown to the user and animation has finished
// Make sure tooltip is shown to the user and animation has finished.
const tooltipElement = overlayContainerElement.querySelector('.mat-tooltip') as HTMLElement;
expect(tooltipElement instanceof HTMLElement).toBe(true);
expect(tooltipElement.style.transform).toBe('scale(1)');

expect(overlayContainerElement.textContent).toContain(initialTooltipMessage);

// After hide called, a timeout delay is created that will to hide the tooltip.
// After hide is called, a timeout delay is created that will to hide the tooltip.
const tooltipDelay = 1000;
tooltipDirective.hide(tooltipDelay);
expect(tooltipDirective._isTooltipVisible()).toBe(true);
Expand Down Expand Up @@ -824,6 +824,33 @@ describe('MatTooltip', () => {
expect(classList).toContain('mat-tooltip-panel-left');
}));

it('should clear the show timeout on destroy', fakeAsync(() => {
assertTooltipInstance(tooltipDirective, false);

tooltipDirective.show(1000);
fixture.detectChanges();

// Note that we aren't asserting anything, but `fakeAsync` will
// throw if we have any timers by the end of the test.
fixture.destroy();
}));

it('should clear the hide timeout on destroy', fakeAsync(() => {
assertTooltipInstance(tooltipDirective, false);

tooltipDirective.show();
tick(0);
fixture.detectChanges();
tick(500);

tooltipDirective.hide(1000);
fixture.detectChanges();

// Note that we aren't asserting anything, but `fakeAsync` will
// throw if we have any timers by the end of the test.
fixture.destroy();
}));

});

describe('fallback positions', () => {
Expand Down
20 changes: 8 additions & 12 deletions src/material/tooltip/tooltip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -764,10 +764,10 @@ export abstract class _TooltipComponentBase implements OnDestroy {
tooltipClass: string|string[]|Set<string>|{[key: string]: any};

/** The timeout ID of any current timer set to show the tooltip */
_showTimeoutId: number | null;
_showTimeoutId: number | undefined;

/** The timeout ID of any current timer set to hide the tooltip */
_hideTimeoutId: number | null;
_hideTimeoutId: number | undefined;

/** Property watched by the animation framework to show or hide the tooltip */
_visibility: TooltipVisibility = 'initial';
Expand All @@ -786,16 +786,13 @@ export abstract class _TooltipComponentBase implements OnDestroy {
*/
show(delay: number): void {
// Cancel the delayed hide if it is scheduled
if (this._hideTimeoutId) {
clearTimeout(this._hideTimeoutId);
this._hideTimeoutId = null;
}
clearTimeout(this._hideTimeoutId);

// Body interactions should cancel the tooltip if there is a delay in showing.
this._closeOnInteraction = true;
this._showTimeoutId = setTimeout(() => {
this._visibility = 'visible';
this._showTimeoutId = null;
this._showTimeoutId = undefined;

// Mark for check so if any parent component has set the
// ChangeDetectionStrategy to OnPush it will be checked anyways
Expand All @@ -809,14 +806,11 @@ export abstract class _TooltipComponentBase implements OnDestroy {
*/
hide(delay: number): void {
// Cancel the delayed show if it is scheduled
if (this._showTimeoutId) {
clearTimeout(this._showTimeoutId);
this._showTimeoutId = null;
}
clearTimeout(this._showTimeoutId);

this._hideTimeoutId = setTimeout(() => {
this._visibility = 'hidden';
this._hideTimeoutId = null;
this._hideTimeoutId = undefined;

// Mark for check so if any parent component has set the
// ChangeDetectionStrategy to OnPush it will be checked anyways
Expand All @@ -835,6 +829,8 @@ export abstract class _TooltipComponentBase implements OnDestroy {
}

ngOnDestroy() {
clearTimeout(this._showTimeoutId);
clearTimeout(this._hideTimeoutId);
this._onHide.complete();
}

Expand Down
4 changes: 2 additions & 2 deletions tools/public_api_guard/material/tooltip.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ export declare abstract class _MatTooltipBase<T extends _TooltipComponentBase> i
}

export declare abstract class _TooltipComponentBase implements OnDestroy {
_hideTimeoutId: number | null;
_showTimeoutId: number | null;
_hideTimeoutId: number | undefined;
_showTimeoutId: number | undefined;
_visibility: TooltipVisibility;
message: string;
tooltipClass: string | string[] | Set<string> | {
Expand Down

0 comments on commit 70ff528

Please sign in to comment.