fix: replace event service with fromEvent and unsubscribe on dialog destroy

Signed-off-by: Akshat Patel <[email protected]>

Author: Akshat55

src/dialog/dialog.directive.ts

@@ -17,6 +17,7 @@ import { DialogService } from "./dialog.service";
 import { CloseMeta, CloseReasons, DialogConfig } from "./dialog-config.interface";
 import { EventService } from "carbon-components-angular/utils";
 import { Dialog } from "./dialog.component";
+import { fromEvent, Subscription } from "rxjs";
 
 /**
  * A generic directive that can be inherited from to create dialogs (for example, a tooltip or popover)
@@ -125,6 +126,8 @@ export class DialogDirective implements OnInit, OnDestroy, OnChanges {
 	 */
 	protected dialogRef: ComponentRef<Dialog>;
 
+	private subscriptions: Subscription[] = [];
+
 	/**
 	 * Creates an instance of DialogDirective.
 	 * @param elementRef
@@ -136,8 +139,11 @@ export class DialogDirective implements OnInit, OnDestroy, OnChanges {
 		protected elementRef: ElementRef,
 		protected viewContainerRef: ViewContainerRef,
 		protected dialogService: DialogService,
+		/**
+		 * Deprecated as of v5
+		 */
 		protected eventService: EventService
-	) {}
+	) { }
 
 	ngOnChanges(changes: SimpleChanges) {
 		// set the config object (this can [and should!] be added to in child classes depending on what they need)
@@ -172,6 +178,7 @@ export class DialogDirective implements OnInit, OnDestroy, OnChanges {
 		this.updateConfig();
 	}
 
+
 	/**
 	 * Sets the config object and binds events for hovering or clicking before
 	 * running code from child class.
@@ -180,49 +187,55 @@ export class DialogDirective implements OnInit, OnDestroy, OnChanges {
 		// fix for safari hijacking clicks
 		this.dialogService.singletonClickListen();
 
-		const element = this.elementRef.nativeElement;
+		const element: HTMLElement = this.elementRef.nativeElement;
 
-		this.eventService.on(element, "keydown", (event: KeyboardEvent) => {
-			if (event.target === this.dialogConfig.parentRef.nativeElement &&
-				(event.key === "Tab" || event.key === "Tab" && event.shiftKey) ||
-				event.key === "Escape") {
-				this.close({
-					reason: CloseReasons.interaction,
-					target: event.target
-				});
-			}
-		});
+		this.subscriptions.push(
+			fromEvent(element, "keydown").subscribe((event: KeyboardEvent) => {
+				if (event.target === this.dialogConfig.parentRef.nativeElement &&
+					(event.key === "Tab" || event.key === "Tab" && event.shiftKey) ||
+					event.key === "Escape") {
+					this.close({
+						reason: CloseReasons.interaction,
+						target: event.target
+					});
+				}
+			})
+		);
 
 		// bind events for hovering or clicking the host
 		if (this.trigger === "hover" || this.trigger === "mouseenter") {
-			this.eventService.on(element, "mouseenter", this.open.bind(this));
-			this.eventService.on(element, this.closeTrigger, (event) => {
-				this.close({
-					reason: CloseReasons.interaction,
-					target: event.target
-				});
-			});
-			this.eventService.on(element, "focus", this.open.bind(this));
-			this.eventService.on(element, "blur", (event) => {
-				this.close({
-					reason: CloseReasons.interaction,
-					target: event.target
-				});
-			});
+			this.subscriptions.push(
+				fromEvent(element, "mouseenter").subscribe(() => this.open()),
+				fromEvent(element, this.closeTrigger).subscribe((event) => {
+					this.close({
+						reason: CloseReasons.interaction,
+						target: event.target
+					});
+				}),
+				fromEvent(element, "focus").subscribe(() => this.open()),
+				fromEvent(element, "blur").subscribe((event) => {
+					this.close({
+						reason: CloseReasons.interaction,
+						target: event.target
+					});
+				})
+			);
 		} else {
-			this.eventService.on(element, "click", (event) => {
-				this.toggle({
-					reason: CloseReasons.interaction,
-					target: event.target
-				});
-			});
-			this.eventService.on(element, "keydown", (event: KeyboardEvent) => {
-				if (event.key === "Enter" || event.key === " ") {
-					setTimeout(() => {
-						this.open();
+			this.subscriptions.push(
+				fromEvent(element, "click").subscribe((event) => {
+					this.toggle({
+						reason: CloseReasons.interaction,
+						target: event.target
 					});
-				}
-			});
+				}),
+				fromEvent(element, "keydown").subscribe((event: KeyboardEvent) => {
+					if (event.key === "Enter" || event.key === " ") {
+						setTimeout(() => {
+							this.open();
+						});
+					}
+				})
+			);
 		}
 
 		DialogDirective.dialogCounter++;
@@ -241,6 +254,7 @@ export class DialogDirective implements OnInit, OnDestroy, OnChanges {
 		this.close({
 			reason: CloseReasons.destroyed
 		});
+		this.subscriptions.forEach((subscription) => subscription.unsubscribe());
 	}
 
 	/**
@@ -259,7 +273,7 @@ export class DialogDirective implements OnInit, OnDestroy, OnChanges {
 
 		// Handles emitting all the close events to clean everything up
 		// Also enforce accessibility on close by updating an aria attr on the nativeElement.
-		this.dialogRef.instance.close.subscribe((meta: CloseMeta) => {
+		const subscription = this.dialogRef.instance.close.subscribe((meta: CloseMeta) => {
 			if (!this.dialogRef) { return; }
 			if (this.dialogConfig.shouldClose && this.dialogConfig.shouldClose(meta)) {
 				// close the dialog, emit events, and clear out the open states
@@ -269,6 +283,7 @@ export class DialogDirective implements OnInit, OnDestroy, OnChanges {
 				this.onClose.emit();
 				this.isOpenChange.emit(false);
 			}
+			subscription.unsubscribe();
 		});
 
 		return this.dialogRef;
@@ -298,13 +313,13 @@ export class DialogDirective implements OnInit, OnDestroy, OnChanges {
 	 * Empty method for child classes to override and specify additional init steps.
 	 * Run after DialogDirective completes it's ngOnInit.
 	 */
-	protected onDialogInit() {}
+	protected onDialogInit() { }
 
 	/**
 	 * Empty method for child to override and specify additional on changes steps.
 	 * run after DialogDirective completes it's ngOnChanges.
 	 */
-	protected onDialogChanges(_changes: SimpleChanges) {}
+	protected onDialogChanges(_changes: SimpleChanges) { }
 
-	protected updateConfig() {}
+	protected updateConfig() { }
 }

src/dialog/overflow-menu/overflow-menu.stories.ts

@@ -33,7 +33,11 @@ export default {
 const Template = (args) => ({
 	props: args,
 	template: `
+		<button (click)="show=!show">Click me!</button>
+		<br/><br/><br/><br/><br/><br/>
+
 		<cds-overflow-menu
+			*ngIf="show"
 			[placement]="placement"
 			[open]="open"
 			[flip]="flip"
@@ -50,10 +54,13 @@ const Template = (args) => ({
 			<cds-overflow-menu-option type="danger" (selected)="selected($event)">Danger option</cds-overflow-menu-option>
 		</cds-overflow-menu>
 		<cds-placeholder></cds-placeholder>
+
+
 	`
 });
 export const Basic = Template.bind({});
 Basic.args = {
+	show: true,
 	open: false,
 	flip: false,
 	offset: {