refactor(cdk): destroy overlay triggers manually (#1316)

* refactor(cdk): destroy overlay triggers manually * fix(cdk): dynamic overlay handler spec * refactor(cdk): make takeUntil last operator * test(cdk): add regression test * refactor(cdk): rename dummy strategy to noop
akveo · Mar 29, 2019 · 77b737d · 77b737d
1 parent 21bf0a5
commit 77b737d
Show file tree

Hide file tree

Showing 6 changed files with 178 additions and 66 deletions.
diff --git a/src/framework/theme/components/cdk/overlay/dynamic/dynamic-overlay-handler.spec.ts b/src/framework/theme/components/cdk/overlay/dynamic/dynamic-overlay-handler.spec.ts
@@ -9,6 +9,7 @@ import {
   Input,
   Type,
 } from '@angular/core';
+import { takeUntil } from 'rxjs/operators';
 
 import { NbRenderableContainer } from '../overlay-container';
 import {
@@ -146,6 +147,8 @@ export class MockTriggerStrategyBuilder {
   show$ = new Subject<any>();
   hide$ = new Subject<any>();
 
+  private destroyed$ = new Subject();
+
   trigger(trigger: NbTrigger): this {
     this._trigger = trigger;
     return this;
@@ -163,9 +166,10 @@ export class MockTriggerStrategyBuilder {
 
   build(): NbTriggerStrategy {
     return {
-      show$: this.show$,
-      hide$: this.hide$,
-    } as NbTriggerStrategy;
+      show$: this.show$.asObservable().pipe(takeUntil(this.destroyed$)),
+      hide$: this.hide$.asObservable().pipe(takeUntil(this.destroyed$)),
+      destroy: () => this.destroyed$.next(),
+    };
   }
 }
 

diff --git a/src/framework/theme/components/cdk/overlay/dynamic/dynamic-overlay-handler.ts b/src/framework/theme/components/cdk/overlay/dynamic/dynamic-overlay-handler.ts
@@ -1,6 +1,4 @@
 import { ElementRef, Injectable, SimpleChange, Type } from '@angular/core';
-import { takeUntil } from 'rxjs/operators';
-import { Subject } from 'rxjs';
 
 import { NbTrigger, NbTriggerStrategy, NbTriggerStrategyBuilderService } from '../overlay-trigger';
 import {
@@ -37,9 +35,9 @@ export class NbDynamicOverlayHandler {
   protected _offset: number = 15;
 
   protected dynamicOverlay: NbDynamicOverlay;
+  protected triggerStrategy: NbTriggerStrategy;
 
   protected positionStrategy: NbAdjustableConnectedPositionStrategy;
-  protected disconnect$ = new Subject();
 
   protected changes: { [key: string]: NbDynamicOverlayChange } = {};
 
@@ -155,7 +153,9 @@ export class NbDynamicOverlayHandler {
   }
 
   disconnect() {
-    this.disconnect$.next();
+    if (this.triggerStrategy) {
+      this.triggerStrategy.destroy();
+    }
   }
 
   destroy() {
@@ -175,22 +175,14 @@ export class NbDynamicOverlayHandler {
   }
 
   protected subscribeOnTriggers(dynamicOverlay: NbDynamicOverlay) {
-
-    const triggerStrategy: NbTriggerStrategy = this.triggerStrategyBuilder
+    this.triggerStrategy = this.triggerStrategyBuilder
       .trigger(this._trigger)
       .host(this._host.nativeElement)
       .container(() => dynamicOverlay.getContainer())
       .build();
 
-    triggerStrategy.show$.pipe(
-      takeUntil(this.disconnect$),
-    )
-      .subscribe(() => dynamicOverlay.show());
-
-    triggerStrategy.hide$.pipe(
-      takeUntil(this.disconnect$),
-    )
-      .subscribe(() => dynamicOverlay.hide());
+    this.triggerStrategy.show$.subscribe(() => dynamicOverlay.show());
+    this.triggerStrategy.hide$.subscribe(() => dynamicOverlay.hide());
   }
 
   protected isContainerRerenderRequired() {

diff --git a/src/framework/theme/components/cdk/overlay/overlay-trigger.spec.ts b/src/framework/theme/components/cdk/overlay/overlay-trigger.spec.ts
@@ -82,6 +82,26 @@ describe('click-trigger-strategy', () => {
 
     expect(spy).toHaveBeenCalledTimes(0);
   });
+
+  it('should not destroy when host reattached', () => {
+    const showSpy = jasmine.createSpy('show');
+    const triggerStrategy = triggerStrategyBuilder
+      .container(() => null)
+      .build();
+
+    triggerStrategy.show$.subscribe(showSpy);
+
+    click(host);
+
+    expect(showSpy).toHaveBeenCalledTimes(1);
+
+    document.body.removeChild(host);
+    document.body.appendChild(host);
+
+    click(host);
+
+    expect(showSpy).toHaveBeenCalledTimes(2);
+  });
 });
 
 describe('hover-trigger-strategy', () => {
@@ -137,6 +157,32 @@ describe('hover-trigger-strategy', () => {
 
     expect(spy).toHaveBeenCalledTimes(0);
   });
+
+  it('should not destroy when host reattached', fakeAsync(() => {
+    const showSpy = jasmine.createSpy('show');
+    const triggerStrategy = triggerStrategyBuilder
+      .container(() => null)
+      .build();
+
+    triggerStrategy.show$.subscribe(showSpy);
+
+    mouseEnter(host);
+
+    // hover trigger strategy has 100 milliseconds delay before firing show$
+    tick(100);
+
+    expect(showSpy).toHaveBeenCalledTimes(1);
+
+    document.body.removeChild(host);
+    document.body.appendChild(host);
+
+    mouseEnter(host);
+
+    // hover trigger strategy has 100 milliseconds delay before firing show$
+    tick(100);
+
+    expect(showSpy).toHaveBeenCalledTimes(2);
+  }));
 });
 
 describe('hint-trigger-strategy', () => {
@@ -176,6 +222,31 @@ describe('hint-trigger-strategy', () => {
     triggerStrategy.hide$.subscribe(done);
     mouseLeave(host);
   });
+
+  it('should not destroy when host reattached', fakeAsync(() => {
+    const showSpy = jasmine.createSpy('show');
+    const triggerStrategy = triggerStrategyBuilder
+      .build();
+
+    triggerStrategy.show$.subscribe(showSpy);
+
+    mouseEnter(host);
+
+    // hint trigger strategy has 100 milliseconds delay before firing show$
+    tick(100);
+
+    expect(showSpy).toHaveBeenCalledTimes(1);
+
+    document.body.removeChild(host);
+    document.body.appendChild(host);
+
+    mouseEnter(host);
+
+    // hint trigger strategy has 100 milliseconds delay before firing show$
+    tick(100);
+
+    expect(showSpy).toHaveBeenCalledTimes(2);
+  }));
 });
 
 describe('focus-trigger-strategy', () => {
@@ -251,6 +322,32 @@ describe('focus-trigger-strategy', () => {
 
     expect(showSpy).toHaveBeenCalledTimes(1);
   }));
+
+  it('should not destroy when host reattached', fakeAsync(() => {
+    const showSpy = jasmine.createSpy('show');
+    const triggerStrategy = triggerStrategyBuilder
+      .container(() => null)
+      .build();
+
+    triggerStrategy.show$.subscribe(showSpy);
+
+    focus(host);
+
+    // focus trigger strategy has 100 milliseconds delay before firing show$
+    tick(100);
+
+    expect(showSpy).toHaveBeenCalledTimes(1);
+
+    document.body.removeChild(host);
+    document.body.appendChild(host);
+
+    focus(host);
+
+    // focus trigger strategy has 100 milliseconds delay before firing show$
+    tick(100);
+
+    expect(showSpy).toHaveBeenCalledTimes(2);
+  }));
 });
 
 describe('noop-trigger-strategy', () => {