From 77b737dca332a670a40368b4456752619b2b2269 Mon Sep 17 00:00:00 2001
From: Nikita Poltoratsky <parapupum@gmail.com>
Date: Fri, 29 Mar 2019 11:18:31 +0300
Subject: [PATCH] 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
---
 .../dynamic/dynamic-overlay-handler.spec.ts   | 10 +-
 .../dynamic/dynamic-overlay-handler.ts        | 22 ++---
 .../cdk/overlay/overlay-trigger.spec.ts       | 97 +++++++++++++++++++
 .../components/cdk/overlay/overlay-trigger.ts | 82 +++++++++-------
 .../datepicker/datepicker.component.ts        | 13 ++-
 .../components/select/select.component.ts     | 20 ++--
 6 files changed, 178 insertions(+), 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
index a7945a0456..4ec602f9d3 100644
--- 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
index 1e500e469b..278f75d0b7 100644
--- 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
index 7ec8e978cc..dea474178c 100644
--- 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', () => {
diff --git a/src/framework/theme/components/cdk/overlay/overlay-trigger.ts b/src/framework/theme/components/cdk/overlay/overlay-trigger.ts
index af227338f2..6a62e61dfe 100644
--- a/src/framework/theme/components/cdk/overlay/overlay-trigger.ts
+++ b/src/framework/theme/components/cdk/overlay/overlay-trigger.ts
@@ -1,6 +1,6 @@
 import { ComponentRef, Inject, Injectable } from '@angular/core';
-import { EMPTY, fromEvent as observableFromEvent, merge as observableMerge, Observable } from 'rxjs';
-import { debounceTime, delay, filter, repeat, share, switchMap, takeUntil, takeWhile, map } from 'rxjs/operators';
+import { EMPTY, fromEvent as observableFromEvent, merge as observableMerge, Observable, Subject } from 'rxjs';
+import { debounceTime, delay, filter, map, repeat, share, switchMap, takeUntil, takeWhile } from 'rxjs/operators';
 import { NB_DOCUMENT } from '../../../theme.options';
 
 export enum NbTrigger {
@@ -19,6 +19,8 @@ export enum NbTrigger {
 export interface NbTriggerStrategy {
   show$: Observable<never | Event>;
   hide$: Observable<never | Event>;
+
+  destroy();
 }
 
 /**
@@ -27,6 +29,12 @@ export interface NbTriggerStrategy {
  * */
 export abstract class NbTriggerStrategyBase implements NbTriggerStrategy {
 
+  destroy() {
+    this.destroyed$.next();
+  }
+
+  protected destroyed$ = new Subject();
+
   protected isNotOnHostOrContainer(event: Event): boolean {
     return !this.isOnHost(event) && !this.isOnContainer(event);
   }
@@ -43,14 +51,6 @@ export abstract class NbTriggerStrategyBase implements NbTriggerStrategy {
     return this.container() && this.container().location.nativeElement.contains(target);
   }
 
-  protected isElementInBody(element: HTMLElement): boolean {
-    return this.document.body.contains(element);
-  }
-
-  protected isHostInBody(): boolean {
-    return this.isElementInBody(this.host);
-  }
-
   abstract show$: Observable<Event>;
   abstract hide$: Observable<Event>;
 
@@ -72,23 +72,23 @@ export class NbClickTriggerStrategy extends NbTriggerStrategyBase {
   // and then hidden right away
   protected click$: Observable<[boolean, Event]> = observableFromEvent<Event>(this.document, 'click')
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       map((event: Event) => [!this.container() && this.isOnHost(event), event] as [boolean, Event]),
       share(),
+      takeUntil(this.destroyed$),
     );
 
   readonly show$: Observable<Event> = this.click$
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       filter(([shouldShow]) => shouldShow),
       map(([, event]) => event),
+      takeUntil(this.destroyed$),
     );
 
   readonly hide$: Observable<Event> = this.click$
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       filter(([shouldShow, event]) => !shouldShow && !this.isOnContainer(event)),
       map(([, event]) => event),
+      takeUntil(this.destroyed$),
     );
 }
 
@@ -101,25 +101,27 @@ export class NbHoverTriggerStrategy extends NbTriggerStrategyBase {
 
   show$: Observable<Event> = observableFromEvent<Event>(this.host, 'mouseenter')
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       filter(() => !this.container()),
       delay(100),
-      takeUntil(observableFromEvent(this.host, 'mouseleave')),
+      takeUntil(
+        observableMerge(
+          observableFromEvent(this.host, 'mouseleave'),
+          this.destroyed$,
+        ),
+      ),
       repeat(),
     );
 
   hide$: Observable<Event> = observableFromEvent<Event>(this.host, 'mouseleave')
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       switchMap(() => observableFromEvent<Event>(this.document, 'mousemove')
         .pipe(
-          takeWhile(() => this.isHostInBody()),
           debounceTime(100),
           takeWhile(() => !!this.container()),
-          filter(event => this.isNotOnHostOrContainer(event),
-          ),
+          filter(event => this.isNotOnHostOrContainer(event)),
         ),
       ),
+      takeUntil(this.destroyed$),
     );
 }
 
@@ -131,15 +133,20 @@ export class NbHoverTriggerStrategy extends NbTriggerStrategyBase {
 export class NbHintTriggerStrategy extends NbTriggerStrategyBase {
   show$: Observable<Event> = observableFromEvent<Event>(this.host, 'mouseenter')
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       delay(100),
-      takeUntil(observableFromEvent(this.host, 'mouseleave')),
+      takeUntil(
+        observableMerge(
+          observableFromEvent(this.host, 'mouseleave'),
+          this.destroyed$,
+        ),
+      ),
       // this `delay & takeUntil & repeat` operators combination is a synonym for `conditional debounce`
       // meaning that if one event occurs in some time after the initial one we won't react to it
       repeat(),
     );
 
-  hide$: Observable<Event> = observableFromEvent(this.host, 'mouseleave');
+  hide$: Observable<Event> = observableFromEvent(this.host, 'mouseleave')
+    .pipe(takeUntil(this.destroyed$));
 }
 
 
@@ -152,46 +159,58 @@ export class NbFocusTriggerStrategy extends NbTriggerStrategyBase {
 
   protected focusOut$: Observable<Event> = observableFromEvent<Event>(this.host, 'focusout')
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       switchMap(() => observableFromEvent<Event>(this.document, 'focusin')
         .pipe(
           takeWhile(() => !!this.container()),
           filter(event => this.isNotOnHostOrContainer(event)),
         ),
       ),
+      takeUntil(this.destroyed$),
     );
 
   protected clickIn$: Observable<Event> = observableFromEvent<Event>(this.host, 'click')
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       filter(() => !this.container()),
+      takeUntil(this.destroyed$),
     );
 
   protected clickOut$: Observable<Event> = observableFromEvent<Event>(this.document, 'click')
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       filter(() => !!this.container()),
       filter(event => this.isNotOnHostOrContainer(event)),
+      takeUntil(this.destroyed$),
     );
 
   protected tabKeyPress$: Observable<Event> = observableFromEvent<Event>(this.document, 'keydown')
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       filter((event: KeyboardEvent) => event.keyCode === 9),
       filter(() => !!this.container()),
+      takeUntil(this.destroyed$),
     );
 
   show$: Observable<Event> = observableMerge(observableFromEvent<Event>(this.host, 'focusin'), this.clickIn$)
     .pipe(
-      takeWhile(() => this.isHostInBody()),
       filter(() => !this.container()),
       debounceTime(100),
-      takeUntil(observableFromEvent(this.host, 'focusout')),
+      takeUntil(
+        observableMerge(
+          observableFromEvent(this.host, 'focusout'),
+          this.destroyed$,
+        ),
+      ),
       repeat(),
     );
 
   hide$ = observableMerge(this.focusOut$, this.tabKeyPress$, this.clickOut$)
-    .pipe(takeWhile(() => this.isHostInBody()));
+    .pipe(takeUntil(this.destroyed$));
+}
+
+/**
+ * Creates empty show and hide event streams.
+ * */
+export class NbNoopTriggerStrategy extends NbTriggerStrategyBase {
+  show$: Observable<Event> = EMPTY;
+  hide$: Observable<Event> = EMPTY;
 }
 
 @Injectable()
@@ -230,10 +249,7 @@ export class NbTriggerStrategyBuilderService {
       case NbTrigger.FOCUS:
         return new NbFocusTriggerStrategy(this._document, this._host, this._container);
       case NbTrigger.NOOP:
-        return {
-          show$: EMPTY,
-          hide$: EMPTY,
-        };
+        return new NbNoopTriggerStrategy(this._document, this._host, this._container);
       default:
         throw new Error('Trigger have to be provided');
     }
diff --git a/src/framework/theme/components/datepicker/datepicker.component.ts b/src/framework/theme/components/datepicker/datepicker.component.ts
index f37eb29a9f..d72f9922a8 100644
--- a/src/framework/theme/components/datepicker/datepicker.component.ts
+++ b/src/framework/theme/components/datepicker/datepicker.component.ts
@@ -140,6 +140,11 @@ export abstract class NbBasePicker<D, T, P>
    * */
   protected positionStrategy: NbAdjustableConnectedPositionStrategy;
 
+  /**
+   * Trigger strategy used by overlay
+   * */
+  protected triggerStrategy: NbTriggerStrategy;
+
   /**
    * HTML input reference to which datepicker connected.
    * */
@@ -232,6 +237,8 @@ export abstract class NbBasePicker<D, T, P>
     if (this.ref) {
       this.ref.dispose();
     }
+
+    this.triggerStrategy.destroy();
   }
 
   /**
@@ -314,9 +321,9 @@ export abstract class NbBasePicker<D, T, P>
   }
 
   protected subscribeOnTriggers() {
-    const triggerStrategy = this.createTriggerStrategy();
-    triggerStrategy.show$.pipe(takeWhile(() => this.alive)).subscribe(() => this.show());
-    triggerStrategy.hide$.pipe(takeWhile(() => this.alive)).subscribe(() => {
+    this.triggerStrategy = this.createTriggerStrategy();
+    this.triggerStrategy.show$.subscribe(() => this.show());
+    this.triggerStrategy.hide$.subscribe(() => {
       this.blur$.next();
       this.hide();
     });
diff --git a/src/framework/theme/components/select/select.component.ts b/src/framework/theme/components/select/select.component.ts
index 042701027f..e0d5c4e10e 100644
--- a/src/framework/theme/components/select/select.component.ts
+++ b/src/framework/theme/components/select/select.component.ts
@@ -346,6 +346,7 @@ export class NbSelectComponent<T> implements OnInit, AfterViewInit, AfterContent
     this.alive = false;
 
     this.ref.dispose();
+    this.triggerStrategy.destroy();
   }
 
   show() {
@@ -482,18 +483,13 @@ export class NbSelectComponent<T> implements OnInit, AfterViewInit, AfterContent
   }
 
   protected subscribeOnTriggers() {
-    this.triggerStrategy.show$
-      .pipe(takeWhile(() => this.alive))
-      .subscribe(() => this.show());
-
-    this.triggerStrategy.hide$
-      .pipe(takeWhile(() => this.alive))
-      .subscribe(($event: Event) => {
-        this.hide();
-        if (!this.isClickedWithinComponent($event)) {
-          this.onTouched();
-        }
-      });
+    this.triggerStrategy.show$.subscribe(() => this.show());
+    this.triggerStrategy.hide$.subscribe(($event: Event) => {
+      this.hide();
+      if (!this.isClickedWithinComponent($event)) {
+        this.onTouched();
+      }
+    });
   }
 
   protected subscribeOnPositionChange() {