From 70ff528785cb9abe9f102f19b1ea87caef56a295 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 10 Feb 2021 21:58:15 +0100 Subject: [PATCH] fix(material/tooltip): clear pending timers on destroy (#16756) 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. --- .../mdc-tooltip/tooltip.spec.ts | 27 +++++++++++++++ src/material/tooltip/tooltip.spec.ts | 33 +++++++++++++++++-- src/material/tooltip/tooltip.ts | 20 +++++------ tools/public_api_guard/material/tooltip.d.ts | 4 +-- 4 files changed, 67 insertions(+), 17 deletions(-) diff --git a/src/material-experimental/mdc-tooltip/tooltip.spec.ts b/src/material-experimental/mdc-tooltip/tooltip.spec.ts index b8d833e7588e..0eaba31d6b73 100644 --- a/src/material-experimental/mdc-tooltip/tooltip.spec.ts +++ b/src/material-experimental/mdc-tooltip/tooltip.spec.ts @@ -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', () => { diff --git a/src/material/tooltip/tooltip.spec.ts b/src/material/tooltip/tooltip.spec.ts index 3d10f82891cc..a31e318c02de 100644 --- a/src/material/tooltip/tooltip.spec.ts +++ b/src/material/tooltip/tooltip.spec.ts @@ -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); @@ -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', () => { diff --git a/src/material/tooltip/tooltip.ts b/src/material/tooltip/tooltip.ts index 69a6886cfab3..c20c5abc56fc 100644 --- a/src/material/tooltip/tooltip.ts +++ b/src/material/tooltip/tooltip.ts @@ -764,10 +764,10 @@ export abstract class _TooltipComponentBase implements OnDestroy { tooltipClass: string|string[]|Set|{[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'; @@ -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 @@ -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 @@ -835,6 +829,8 @@ export abstract class _TooltipComponentBase implements OnDestroy { } ngOnDestroy() { + clearTimeout(this._showTimeoutId); + clearTimeout(this._hideTimeoutId); this._onHide.complete(); } diff --git a/tools/public_api_guard/material/tooltip.d.ts b/tools/public_api_guard/material/tooltip.d.ts index 89ef9d0e7784..fa923eb655cf 100644 --- a/tools/public_api_guard/material/tooltip.d.ts +++ b/tools/public_api_guard/material/tooltip.d.ts @@ -45,8 +45,8 @@ export declare abstract class _MatTooltipBase 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 | {