Skip to content

Commit

Permalink
fix(tab-link): avoid potential memory leak (#1877)
Browse files Browse the repository at this point in the history
* Similarly to #1838, the `tab-link` destroy handler may not be called in certain situations, because it is being inherited from the MdRipple class. This PR explicitly calls the destroy handler.
* Adds a unit test to make sure that the ripples are being cleaned up properly.
  • Loading branch information
crisbeto authored and tinayuangao committed Nov 28, 2016
1 parent cf1b4b9 commit e332f15
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 4 deletions.
26 changes: 25 additions & 1 deletion src/lib/core/ripple/ripple.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,11 @@ describe('MdRipple', () => {
beforeEach(() => {
TestBed.configureTestingModule({
imports: [MdRippleModule.forRoot()],
declarations: [BasicRippleContainer, RippleContainerWithInputBindings],
declarations: [
BasicRippleContainer,
RippleContainerWithInputBindings,
RippleContainerWithNgIf,
],
});
});

Expand Down Expand Up @@ -180,6 +184,20 @@ describe('MdRipple', () => {
expect(pxStringToFloat(ripple.style.width)).toBeCloseTo(2 * expectedRadius, 1);
expect(pxStringToFloat(ripple.style.height)).toBeCloseTo(2 * expectedRadius, 1);
});

it('cleans up the event handlers when the container gets destroyed', () => {
fixture = TestBed.createComponent(RippleContainerWithNgIf);
fixture.detectChanges();

rippleElement = fixture.debugElement.nativeElement.querySelector('[md-ripple]');
rippleBackground = rippleElement.querySelector('.md-ripple-background');

fixture.componentInstance.isDestroyed = true;
fixture.detectChanges();

rippleElement.dispatchEvent(createMouseEvent('mousedown'));
expect(rippleBackground.classList).not.toContain('md-ripple-active');
});
});

describe('configuring behavior', () => {
Expand Down Expand Up @@ -367,3 +385,9 @@ class RippleContainerWithInputBindings {
backgroundColor = '';
@ViewChild(MdRipple) ripple: MdRipple;
}

@Component({ template: `<div id="container" md-ripple *ngIf="!isDestroyed"></div>` })
class RippleContainerWithNgIf {
@ViewChild(MdRipple) ripple: MdRipple;
isDestroyed = false;
}
33 changes: 32 additions & 1 deletion src/lib/tabs/tab-nav-bar/tab-nav-bar.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ describe('MdTabNavBar', () => {
TestBed.configureTestingModule({
imports: [MdTabsModule.forRoot()],
declarations: [
SimpleTabNavBarTestApp
SimpleTabNavBarTestApp,
TabLinkWithNgIf,
],
});

Expand Down Expand Up @@ -38,6 +39,25 @@ describe('MdTabNavBar', () => {
expect(component.activeIndex).toBe(2);
});
});

it('should clean up the ripple event handlers on destroy', () => {
let fixture: ComponentFixture<TabLinkWithNgIf> = TestBed.createComponent(TabLinkWithNgIf);
fixture.detectChanges();

let link = fixture.debugElement.nativeElement.querySelector('[md-tab-link]');
let rippleBackground = link.querySelector('.md-ripple-background');
let mouseEvent = document.createEvent('MouseEvents');

fixture.componentInstance.isDestroyed = true;
fixture.detectChanges();

mouseEvent.initMouseEvent('mousedown', false, false, window, 0, 0, 0, 0, 0, false, false,
false, false, 0, null);

link.dispatchEvent(mouseEvent);

expect(rippleBackground.classList).not.toContain('md-ripple-active');
});
});

@Component({
Expand All @@ -53,3 +73,14 @@ describe('MdTabNavBar', () => {
class SimpleTabNavBarTestApp {
activeIndex = 0;
}

@Component({
template: `
<nav md-tab-nav-bar>
<a md-tab-link *ngIf="!isDestroyed">Link</a>
</nav>
`
})
class TabLinkWithNgIf {
isDestroyed = false;
}
19 changes: 17 additions & 2 deletions src/lib/tabs/tab-nav-bar/tab-nav-bar.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
import {Component, Input, ViewChild, ElementRef, ViewEncapsulation, Directive} from '@angular/core';
import {
Component,
Input,
ViewChild,
ElementRef,
ViewEncapsulation,
Directive,
OnDestroy,
} from '@angular/core';

import {MdInkBar} from '../ink-bar';
import {MdRipple} from '../../core/ripple/ripple';

Expand Down Expand Up @@ -50,8 +59,14 @@ export class MdTabLink {
@Directive({
selector: '[md-tab-link]',
})
export class MdTabLinkRipple extends MdRipple {
export class MdTabLinkRipple extends MdRipple implements OnDestroy {
constructor(private _element: ElementRef) {
super(_element);
}

// In certain cases the parent destroy handler
// may not get called. See Angular issue #11606.
ngOnDestroy() {
super.ngOnDestroy();
}
}

0 comments on commit e332f15

Please sign in to comment.