Skip to content

Commit

Permalink
fix(popover): unregister event listener functions only if present (#2699
Browse files Browse the repository at this point in the history
)

Fixes #2688
  • Loading branch information
maxokorokov authored and pkozlowski-opensource committed Sep 14, 2018
1 parent f8977be commit 5dd4e1e
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 2 deletions.
33 changes: 32 additions & 1 deletion src/popover/popover.spec.ts
Expand Up @@ -2,7 +2,15 @@ import {TestBed, ComponentFixture, inject, fakeAsync, tick} from '@angular/core/
import {createGenericTestComponent, createKeyEvent} from '../test/common';

import {By} from '@angular/platform-browser';
import {Component, ViewChild, ChangeDetectionStrategy, Injectable, OnDestroy, TemplateRef} from '@angular/core';
import {
Component,
ViewChild,
ChangeDetectionStrategy,
Injectable,
OnDestroy,
TemplateRef,
ViewContainerRef
} from '@angular/core';

import {Key} from '../util/key';

Expand Down Expand Up @@ -875,6 +883,22 @@ describe('ngb-popover', () => {
expect(popover.popoverClass).toBe(config.popoverClass);
});
});

describe('non-regression', () => {

/**
* Under very specific conditions ngOnDestroy can be invoked without calling ngOnInit first.
* See discussion in https://github.com/ng-bootstrap/ng-bootstrap/issues/2199 for more details.
*/
it('should not try to call listener cleanup function when no listeners registered', () => {
const fixture = createTestComponent(`
<ng-template #tpl><div ngbPopover="Great tip!"></div></ng-template>
<button (click)="createAndDestroyTplWithAPopover(tpl)"></button>
`);
const buttonEl = fixture.debugElement.query(By.css('button'));
buttonEl.triggerEventHandler('click', {});
});
});
});

@Component({selector: 'test-cmpt', template: ``})
Expand All @@ -886,6 +910,13 @@ export class TestComponent {

@ViewChild(NgbPopover) popover: NgbPopover;

constructor(private _vcRef: ViewContainerRef) {}

createAndDestroyTplWithAPopover(tpl: TemplateRef<any>) {
this._vcRef.createEmbeddedView(tpl, {}, 0);
this._vcRef.remove(0);
}

shown() {}
hidden() {}
}
Expand Down
6 changes: 5 additions & 1 deletion src/popover/popover.ts
Expand Up @@ -308,7 +308,11 @@ export class NgbPopover implements OnInit, OnDestroy, OnChanges {

ngOnDestroy() {
this.close();
this._unregisterListenersFn();
// This check is needed as it might happen that ngOnDestroy is called before ngOnInit
// under certain conditions, see: https://github.com/ng-bootstrap/ng-bootstrap/issues/2199
if (this._unregisterListenersFn) {
this._unregisterListenersFn();
}
this._zoneSubscription.unsubscribe();
}

Expand Down

0 comments on commit 5dd4e1e

Please sign in to comment.