fix(dynamic overlay): prevent multiple onStable subscriptions (#2494)

akveo · Aug 31, 2020 · f22e87d · f22e87d
1 parent d34a711
commit f22e87d
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 24 deletions.
diff --git a/src/framework/theme/components/cdk/overlay/dynamic/dynamic-overlay.spec.ts b/src/framework/theme/components/cdk/overlay/dynamic/dynamic-overlay.spec.ts
@@ -293,7 +293,7 @@ describe('dynamic-overlay', () => {
 
     expect(instance.content).toBe(newContent);
     expect(renderContentSpy).toHaveBeenCalledTimes(2);
-    expect(updatePositionSpy).toHaveBeenCalledTimes(1);
+    expect(updatePositionSpy).toHaveBeenCalledTimes(2);
   });
 
   it('should set context when shown', () => {
@@ -307,7 +307,7 @@ describe('dynamic-overlay', () => {
 
     expect(instance.context).toBe(newContext);
     expect(renderContentSpy).toHaveBeenCalledTimes(2);
-    expect(updatePositionSpy).toHaveBeenCalledTimes(1);
+    expect(updatePositionSpy).toHaveBeenCalledTimes(2);
   });
 
   it('should set context & content when shown', () => {
@@ -325,7 +325,7 @@ describe('dynamic-overlay', () => {
     expect(instance.context).toBe(newContext);
     expect(instance.content).toBe(newContent);
     expect(renderContentSpy).toHaveBeenCalledTimes(3);
-    expect(updatePositionSpy).toHaveBeenCalledTimes(2);
+    expect(updatePositionSpy).toHaveBeenCalledTimes(4);
   });
 
   it('should set component', () => {

diff --git a/src/framework/theme/components/cdk/overlay/dynamic/dynamic-overlay.ts b/src/framework/theme/components/cdk/overlay/dynamic/dynamic-overlay.ts
@@ -33,6 +33,7 @@ export class NbDynamicOverlay {
   protected positionStrategyChange$ = new Subject();
   protected isShown$ = new BehaviorSubject<boolean>(false);
   protected destroy$ = new Subject<void>();
+  protected overlayDestroy$ = new Subject<NbOverlayRef>();
 
   get isAttached(): boolean {
     return this.ref && this.ref.hasAttached();
@@ -69,6 +70,7 @@ export class NbDynamicOverlay {
     if (this.container) {
       this.updateContext();
     }
+    this.updatePosition();
   }
 
   setContext(context: Object) {
@@ -77,6 +79,7 @@ export class NbDynamicOverlay {
     if (this.container) {
       this.updateContext();
     }
+    this.updatePosition();
   }
 
   setContentAndContext(content: NbOverlayContent, context: Object) {
@@ -85,6 +88,7 @@ export class NbDynamicOverlay {
     if (this.container) {
       this.updateContext();
     }
+    this.updatePosition();
   }
 
   setComponent(componentType: Type<NbRenderableContainer>) {
@@ -175,6 +179,7 @@ export class NbDynamicOverlay {
     this.disposeOverlayRef();
     this.isShown$.complete();
     this.positionStrategyChange$.complete();
+    this.overlayDestroy$.complete();
   }
 
   getContainer() {
@@ -187,7 +192,7 @@ export class NbDynamicOverlay {
       scrollStrategy: this.overlay.scrollStrategies.reposition(),
       ...this.overlayConfig,
     });
-    this.updatePositionWhenStable();
+    this.updatePositionWhenStable(this.ref);
   }
 
   protected renderContainer() {
@@ -218,12 +223,20 @@ export class NbDynamicOverlay {
    * Dimensions of the container may change after content update. So we listen to zone.stable event to
    * reposition the container.
    */
-  protected updatePositionWhenStable() {
+  protected updatePositionWhenStable(overlay: NbOverlayRef) {
+    const overlayDestroy$ = this.overlayDestroy$.pipe(
+      filter((destroyedOverlay: NbOverlayRef) => destroyedOverlay === overlay),
+    );
+
     this.zone.onStable
-      .pipe(takeUntil(this.destroy$))
-      .subscribe(() => {
-        this.ref && this.ref.updatePosition();
-      });
+      .pipe(takeUntil(merge(this.destroy$, overlayDestroy$)))
+      .subscribe(() => this.updatePosition());
+  }
+
+  protected updatePosition() {
+    if (this.ref) {
+      this.ref.updatePosition();
+    }
   }
 
   protected hasOverlayInContainer(): boolean {
@@ -233,6 +246,7 @@ export class NbDynamicOverlay {
   protected disposeOverlayRef() {
     if (this.ref) {
       this.ref.dispose();
+      this.overlayDestroy$.next(this.ref);
       this.ref = null;
       this.container = null;
     }

diff --git a/src/framework/theme/components/context-menu/context-menu.directive.ts b/src/framework/theme/components/context-menu/context-menu.directive.ts
@@ -14,18 +14,25 @@ import {
   OnChanges,
   OnDestroy,
   OnInit,
+  SimpleChanges,
 } from '@angular/core';
 import { filter, takeUntil } from 'rxjs/operators';
 import { Subject } from 'rxjs';
 
 import { NbDynamicOverlay, NbDynamicOverlayController } from '../cdk/overlay/dynamic/dynamic-overlay';
 import { NbDynamicOverlayHandler } from '../cdk/overlay/dynamic/dynamic-overlay-handler';
-import { NbOverlayRef } from '../cdk/overlay/mapping';
+import { NbOverlayConfig, NbOverlayRef } from '../cdk/overlay/mapping';
 import { NbAdjustableConnectedPositionStrategy, NbAdjustment, NbPosition } from '../cdk/overlay/overlay-position';
- import { NbTrigger, NbTriggerValues } from '../cdk/overlay/overlay-trigger';
+import { NbTrigger, NbTriggerValues } from '../cdk/overlay/overlay-trigger';
 import { NbContextMenuComponent } from './context-menu.component';
 import { NbMenuItem, NbMenuService } from '../menu/menu.service';
 
+export interface NbContextMenuContext {
+  items: NbMenuItem[];
+  tag: string;
+  position: NbPosition;
+}
+
 /**
  * Full featured context menu directive.
  *
@@ -126,7 +133,16 @@ export class NbContextMenuDirective implements NbDynamicOverlayController, OnCha
    * Can be top, right, bottom and left.
    * */
   @Input('nbContextMenuPlacement')
-  position: NbPosition = NbPosition.BOTTOM;
+  get position(): NbPosition {
+    return this._position;
+  }
+  set position(value: NbPosition) {
+    if (value !== this.position) {
+      this._position = value;
+      this.updateOverlayContext();
+    }
+  }
+  _position: NbPosition = NbPosition.BOTTOM;
 
   /**
    * Container position will be changes automatically based on this strategy if container can't fit view port.
@@ -140,15 +156,28 @@ export class NbContextMenuDirective implements NbDynamicOverlayController, OnCha
    * Set NbMenu tag, which helps identify menu when working with NbMenuService.
    * */
   @Input('nbContextMenuTag')
-  tag: string;
+  get tag(): string {
+    return this._tag;
+  }
+  set tag(value: string) {
+    if (value !== this.tag) {
+      this._tag = value;
+      this.updateOverlayContext();
+    }
+  }
+  _tag: string;
 
   /**
    * Basic menu items, will be passed to the internal NbMenuComponent.
    * */
   @Input('nbContextMenu')
+  get items(): NbMenuItem[] {
+    return this._items;
+  }
   set items(items: NbMenuItem[]) {
     this.validateItems(items);
     this._items = items;
+    this.updateOverlayContext();
   };
 
   /**
@@ -160,11 +189,22 @@ export class NbContextMenuDirective implements NbDynamicOverlayController, OnCha
   static ngAcceptInputType_trigger: NbTriggerValues;
 
   @Input('nbContextMenuClass')
-  contextMenuClass: string = '';
+  get contextMenuClass(): string {
+    return this._contextMenuClass;
+  }
+  set contextMenuClass(value: string) {
+    if (value !== this.contextMenuClass) {
+      this._contextMenuClass = value;
+      this.overlayConfig = { panelClass: this.contextMenuClass };
+    }
+  }
+  _contextMenuClass: string = '';
 
   protected ref: NbOverlayRef;
   protected container: ComponentRef<any>;
   protected positionStrategy: NbAdjustableConnectedPositionStrategy;
+  protected overlayConfig: NbOverlayConfig = { panelClass: this.contextMenuClass } ;
+  protected overlayContext: NbContextMenuContext = { items: this.items, tag: this.tag, position: this.position };
   protected destroy$ = new Subject<void>();
   private _items: NbMenuItem[] = [];
 
@@ -217,12 +257,8 @@ export class NbContextMenuDirective implements NbDynamicOverlayController, OnCha
       .position(this.position)
       .trigger(this.trigger)
       .adjustment(this.adjustment)
-      .context({
-        position: this.position,
-        items: this._items,
-        tag: this.tag,
-      })
-      .overlayConfig({panelClass: this.contextMenuClass});
+      .context(this.overlayContext)
+      .overlayConfig(this.overlayConfig);
   }
 
   /*
@@ -243,4 +279,8 @@ export class NbContextMenuDirective implements NbDynamicOverlayController, OnCha
       )
       .subscribe(() => this.hide());
   }
+
+  protected updateOverlayContext() {
+    this.overlayContext = { items: this.items, position: this.position, tag: this.tag };
+  }
 }
diff --git a/src/framework/theme/components/popover/popover.directive.ts b/src/framework/theme/components/popover/popover.directive.ts
@@ -14,13 +14,15 @@ import {
   OnInit,
   Output,
   EventEmitter,
+  SimpleChanges,
 } from '@angular/core';
 
 import { NbDynamicOverlay, NbDynamicOverlayController } from '../cdk/overlay/dynamic/dynamic-overlay';
 import { NbDynamicOverlayHandler } from '../cdk/overlay/dynamic/dynamic-overlay-handler';
 import { NbAdjustment, NbPosition, NbPositionValues, NbAdjustmentValues } from '../cdk/overlay/overlay-position';
 import { NbOverlayContent } from '../cdk/overlay/overlay-service';
 import { NbTrigger, NbTriggerValues } from '../cdk/overlay/overlay-trigger';
+import { NbOverlayConfig } from '../cdk/overlay/mapping';
 import { NbPopoverComponent } from './popover.component';
 import { takeUntil, skip } from 'rxjs/operators';
 import { Subject } from 'rxjs';
@@ -178,11 +180,22 @@ export class NbPopoverDirective implements NbDynamicOverlayController, OnChanges
   offset = 15;
 
   @Input('nbPopoverClass')
-  popoverClass: string = '';
+  get popoverClass(): string {
+    return this._popoverClass;
+  }
+  set popoverClass(value: string) {
+    if (value !== this.popoverClass) {
+      this._popoverClass = value;
+      this.overlayConfig = { panelClass: this.popoverClass };
+    }
+  }
+  _popoverClass: string = '';
 
   @Output()
   nbPopoverShowStateChange = new EventEmitter<{ isShown: boolean }>();
 
+  protected overlayConfig: NbOverlayConfig = { panelClass: this.popoverClass }
+
   get isShown(): boolean {
     return !!(this.dynamicOverlay && this.dynamicOverlay.isAttached);
   }
@@ -244,6 +257,6 @@ export class NbPopoverDirective implements NbDynamicOverlayController, OnChanges
       .adjustment(this.adjustment)
       .content(this.content)
       .context(this.context)
-      .overlayConfig({ panelClass: this.popoverClass });
+      .overlayConfig(this.overlayConfig);
   }
 }
diff --git a/src/framework/theme/components/tooltip/tooltip.directive.ts b/src/framework/theme/components/tooltip/tooltip.directive.ts
@@ -14,6 +14,7 @@ import {
   OnInit,
   Output,
   EventEmitter,
+  SimpleChanges,
 } from '@angular/core';
 import { skip, takeUntil } from 'rxjs/operators';
 import { Subject } from 'rxjs';
@@ -23,6 +24,7 @@ import { NbAdjustment, NbPosition, NbPositionValues, NbAdjustmentValues } from '
 import { NbTrigger } from '../cdk/overlay/overlay-trigger';
 import { NbDynamicOverlay } from '../cdk/overlay/dynamic/dynamic-overlay';
 import { NbDynamicOverlayHandler } from '../cdk/overlay/dynamic/dynamic-overlay-handler';
+import { NbOverlayConfig } from '../cdk/overlay/mapping';
 import { NbTooltipComponent } from './tooltip.component';
 import { NbIconConfig } from '../icon/icon.component';
 
@@ -114,7 +116,16 @@ export class NbTooltipDirective implements OnInit, OnChanges, AfterViewInit, OnD
   static ngAcceptInputType_adjustment: NbAdjustmentValues;
 
   @Input('nbTooltipClass')
-  tooltipClass: string = '';
+  get tooltipClass(): string {
+    return this._tooltipClass;
+  }
+  set tooltipClass(value: string) {
+    if (value !== this.tooltipClass) {
+      this._tooltipClass = value;
+      this.overlayConfig = { panelClass: this.tooltipClass };
+    }
+  }
+  _tooltipClass: string = '';
 
   /**
    * Accepts icon name or icon config object
@@ -144,6 +155,8 @@ export class NbTooltipDirective implements OnInit, OnChanges, AfterViewInit, OnD
   @Output()
   nbTooltipShowStateChange = new EventEmitter<{ isShown: boolean }>();
 
+  protected overlayConfig: NbOverlayConfig = { panelClass: this.tooltipClass };
+
   get isShown(): boolean {
     return !!(this.dynamicOverlay && this.dynamicOverlay.isAttached);
   }
@@ -205,6 +218,6 @@ export class NbTooltipDirective implements OnInit, OnChanges, AfterViewInit, OnD
       .adjustment(this.adjustment)
       .content(this.content)
       .context(this.context)
-      .overlayConfig({ panelClass: this.tooltipClass });
+      .overlayConfig(this.overlayConfig);
   }
 }