Skip to content

Commit

Permalink
fix(popover): properly destroy popovers using container option
Browse files Browse the repository at this point in the history 
Closes #931
  • Loading branch information
@pkozlowski-opensource
pkozlowski-opensource committed Oct 21, 2016
1 parent 23d412b commit 19bb887
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 44 deletions.
89 changes: 50 additions & 39 deletions src/popover/popover.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('ngb-popover', () => {
});
});

function getWindow(fixture) { return fixture.nativeElement.querySelector('ngb-popover-window'); }
function getWindow(element) { return element.querySelector('ngb-popover-window'); }

describe('basic functionality', () => {

Expand All @@ -63,7 +63,7 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
const windowEl = getWindow(fixture);
const windowEl = getWindow(fixture.nativeElement);

expect(windowEl).toHaveCssClass('popover');
expect(windowEl).toHaveCssClass('popover-top');
Expand All @@ -73,7 +73,7 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});

it('should open and close a popover - default settings and content from a template', () => {
Expand All @@ -85,7 +85,7 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
const windowEl = getWindow(fixture);
const windowEl = getWindow(fixture.nativeElement);

expect(windowEl).toHaveCssClass('popover');
expect(windowEl).toHaveCssClass(`popover-${defaultConfig.placement}`);
Expand All @@ -95,7 +95,7 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});

it('should properly destroy TemplateRef content', () => {
Expand All @@ -107,12 +107,12 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();
expect(spyService.called).toBeFalsy();

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
expect(spyService.called).toBeTruthy();
});

Expand All @@ -122,15 +122,15 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();
});

it('should not leave dangling popovers in the DOM', () => {
Expand All @@ -140,11 +140,11 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();

fixture.componentInstance.show = false;
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});

it('should properly cleanup popovers with manual triggers', () => {
Expand All @@ -155,11 +155,11 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('mouseenter', {});
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();

fixture.componentInstance.show = false;
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});
});

Expand All @@ -172,7 +172,7 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
const windowEl = getWindow(fixture);
const windowEl = getWindow(fixture.nativeElement);

expect(windowEl).toHaveCssClass('popover');
expect(windowEl).toHaveCssClass('popover-left');
Expand All @@ -185,7 +185,7 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
const windowEl = getWindow(fixture);
const windowEl = getWindow(fixture.nativeElement);

expect(windowEl).toHaveCssClass('popover');
expect(windowEl).toHaveCssClass('popover-left');
Expand All @@ -202,12 +202,23 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
let windowEl = getWindow(fixture);
expect(windowEl).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
expect(getWindow(window.document.querySelector(selector))).not.toBeNull();
});

it('should properly destroy popovers when the "container" option is used', () => {
const selector = 'body';
const fixture =
createTestComponent(`<div *ngIf="show" ngbPopover="Great tip!" container="` + selector + `"></div>`);
const directive = fixture.debugElement.query(By.directive(NgbPopover));

const container = window.document.querySelector(selector);
windowEl = container.querySelector('ngb-popover-window');
expect(windowEl.parentNode).toBe(container);
directive.triggerEventHandler('click', {});
fixture.detectChanges();

expect(getWindow(document.querySelector(selector))).not.toBeNull();
fixture.componentRef.instance.show = false;
fixture.detectChanges();
expect(getWindow(document.querySelector(selector))).toBeNull();
});

});
Expand All @@ -223,12 +234,12 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();
expect(shownSpy).toHaveBeenCalled();

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
expect(hiddenSpy).toHaveBeenCalled();
});

Expand All @@ -245,7 +256,7 @@ describe('ngb-popover', () => {
fixture.componentInstance.popover.open();
fixture.detectChanges();

expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();
expect(shownSpy).toHaveBeenCalled();
expect(shownSpy.calls.count()).toEqual(1);
expect(hiddenSpy).not.toHaveBeenCalled();
Expand All @@ -260,7 +271,7 @@ describe('ngb-popover', () => {

fixture.componentInstance.popover.close();
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
expect(shownSpy).not.toHaveBeenCalled();
expect(hiddenSpy).not.toHaveBeenCalled();
});
Expand Down Expand Up @@ -292,11 +303,11 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});

it('should non-default toggle triggers', () => {
Expand All @@ -305,11 +316,11 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('mouseenter', {});
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});

it('should support multiple triggers', () => {
Expand All @@ -318,11 +329,11 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('mouseenter', {});
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();

directive.triggerEventHandler('click', {});
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});

it('should not use default for manual triggers', () => {
Expand All @@ -331,7 +342,7 @@ describe('ngb-popover', () => {

directive.triggerEventHandler('mouseenter', {});
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});

it('should allow toggling for manual triggers', () => {
Expand All @@ -342,11 +353,11 @@ describe('ngb-popover', () => {

button.click();
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();

button.click();
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});

it('should allow open / close for manual triggers', () => {
Expand All @@ -357,11 +368,11 @@ describe('ngb-popover', () => {

buttons[0].click(); // open
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();

buttons[1].click(); // close
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});

it('should not throw when open called for manual triggers and open popover', () => {
Expand All @@ -372,11 +383,11 @@ describe('ngb-popover', () => {

button.click(); // open
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();

button.click(); // open
fixture.detectChanges();
expect(getWindow(fixture)).not.toBeNull();
expect(getWindow(fixture.nativeElement)).not.toBeNull();
});

it('should not throw when closed called for manual triggers and closed popover', () => {
Expand All @@ -387,7 +398,7 @@ describe('ngb-popover', () => {

button.click(); // close
fixture.detectChanges();
expect(getWindow(fixture)).toBeNull();
expect(getWindow(fixture.nativeElement)).toBeNull();
});
});

Expand Down
11 changes: 6 additions & 5 deletions src/popover/popover.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,6 @@ export class NgbPopover implements OnInit, OnDestroy {
positionElements(
this._elementRef.nativeElement, this._windowRef.location.nativeElement, this.placement,
this.container === 'body');

if (this.container === 'body') {
let windowEl = this._windowRef.location.nativeElement;
window.document.querySelector(this.container).appendChild(windowEl);
}
}
});
}
Expand All @@ -108,6 +103,11 @@ export class NgbPopover implements OnInit, OnDestroy {
this._windowRef = this._popupService.open(this.ngbPopover);
this._windowRef.instance.placement = this.placement;
this._windowRef.instance.title = this.popoverTitle;

if (this.container === 'body') {
window.document.querySelector(this.container).appendChild(this._windowRef.location.nativeElement);
}

// we need to manually invoke change detection since events registered via
// Renderer::listen() are not picked up by change detection with the OnPush strategy
this._windowRef.changeDetectorRef.markForCheck();
Expand Down Expand Up @@ -149,6 +149,7 @@ export class NgbPopover implements OnInit, OnDestroy {
}

ngOnDestroy() {
this.close();
this._unregisterListenersFn();
this._zoneSubscription.unsubscribe();
}
Expand Down

0 comments on commit 19bb887

Please sign in to comment.