fix(extensions): CellExternalCopyBuffer onKeyDown event leak, fix #635 (#636)

Author: ghiscoding

src/app/examples/grid-frozen.component.ts

@@ -1,6 +1,8 @@
 import { Component, OnInit, OnDestroy, ViewEncapsulation } from '@angular/core';
 import { AngularGridInstance, Column, ColumnEditorDualInput, Editors, FieldType, formatNumber, Formatters, Filters, GridOption } from './../modules/angular-slickgrid';
 
+declare const Slick: any;
+
 @Component({
   templateUrl: './grid-frozen.component.html',
   styleUrls: ['./grid-frozen.component.scss'],
@@ -26,15 +28,19 @@ export class GridFrozenComponent implements OnInit, OnDestroy {
   frozenRowCount = 3;
   isFrozenBottom = false;
   gridObj: any;
+  slickEventHandler: any;
+
+  constructor() {
+    this.slickEventHandler = new Slick.EventHandler();
+  }
 
   ngOnInit(): void {
     this.prepareDataGrid();
   }
 
   ngOnDestroy() {
-    // unsubscribe every SlickGrid subscribed event (or use the Slick.EventHandler)
-    this.gridObj.onMouseEnter.unsubscribe(this.highlightRow);
-    this.gridObj.onMouseLeave.unsubscribe(this.highlightRow);
+    // unsubscribe all SlickGrid events
+    this.slickEventHandler.unsubscribeAll();
     this.highlightRow = null;
   }
 
@@ -45,8 +51,8 @@ export class GridFrozenComponent implements OnInit, OnDestroy {
     // with frozen (pinned) grid, in order to see the entire row being highlighted when hovering
     // we need to do some extra tricks (that is because frozen grids use 2 separate div containers)
     // the trick is to use row selection to highlight when hovering current row and remove selection once we're not
-    this.gridObj.onMouseEnter.subscribe(event => this.highlightRow(event, true));
-    this.gridObj.onMouseLeave.subscribe(event => this.highlightRow(event, false));
+    this.slickEventHandler.subscribe(this.gridObj.onMouseEnter, event => this.highlightRow(event, true));
+    this.slickEventHandler.subscribe(this.gridObj.onMouseLeave, event => this.highlightRow(event, false));
   }
 
   highlightRow(event: Event, isMouseEnter: boolean) {

src/app/modules/angular-slickgrid/editors/dualInputEditor.ts

@@ -1,6 +1,5 @@
-import { getDescendantProperty, setDeepValue } from '../services/utilities';
-import { floatValidator, integerValidator, textValidator } from '../editorValidators';
 import {
+  DOMEvent,
   Column,
   ColumnEditor,
   ColumnEditorDualInput,
@@ -12,6 +11,9 @@ import {
   KeyCode,
   SlickEventHandler,
 } from '../models/index';
+import { BindingEventService } from '../services/bindingEvent.service';
+import { getDescendantProperty, setDeepValue } from '../services/utilities';
+import { floatValidator, integerValidator, textValidator } from '../editorValidators';
 
 // using external non-typed js libraries
 declare const Slick: any;
@@ -21,6 +23,7 @@ declare const Slick: any;
  * KeyDown events are also handled to provide handling for Tab, Shift-Tab, Esc and Ctrl-Enter.
  */
 export class DualInputEditor implements Editor {
+  private _bindEventService: BindingEventService;
   private _eventHandler: SlickEventHandler;
   private _isValueSaveCalled = false;
   private _lastEventType: string | undefined;
@@ -44,8 +47,9 @@ export class DualInputEditor implements Editor {
     }
     this.grid = args.grid;
     this.gridOptions = (this.grid.getOptions() || {}) as GridOption;
-    this.init();
     this._eventHandler = new Slick.EventHandler();
+    this._bindEventService = new BindingEventService();
+    this.init();
     this._eventHandler.subscribe(this.grid.onValidationError, () => this._isValueSaveCalled = true);
   }
 
@@ -105,14 +109,14 @@ export class DualInputEditor implements Editor {
 
     // the lib does not get the focus out event for some reason, so register it here
     if (this.hasAutoCommitEdit) {
-      this._leftInput.addEventListener('focusout', (event: any) => this.handleFocusOut(event, 'leftInput'));
-      this._rightInput.addEventListener('focusout', (event: any) => this.handleFocusOut(event, 'rightInput'));
+      this._bindEventService.bind(this._leftInput, 'focusout', (event: DOMEvent<HTMLInputElement>) => this.handleFocusOut(event, 'leftInput'));
+      this._bindEventService.bind(this._rightInput, 'focusout', (event: DOMEvent<HTMLInputElement>) => this.handleFocusOut(event, 'rightInput'));
     }
 
     setTimeout(() => this._leftInput.select(), 50);
   }
 
-  handleFocusOut(event: any, position: 'leftInput' | 'rightInput') {
+  handleFocusOut(event: DOMEvent<HTMLInputElement>, position: 'leftInput' | 'rightInput') {
     // when clicking outside the editable cell OR when focusing out of it
     const targetClassNames = event.relatedTarget && event.relatedTarget.className || '';
     if (targetClassNames.indexOf('dual-editor') === -1 && this._lastEventType !== 'focusout-right') {
@@ -134,12 +138,7 @@ export class DualInputEditor implements Editor {
   destroy() {
     // unsubscribe all SlickGrid events
     this._eventHandler.unsubscribeAll();
-
-    const columnId = this.columnDef && this.columnDef.id;
-    const elements = document.querySelectorAll(`.dual-editor-text.editor-${columnId}`);
-    if (elements.length > 0) {
-      elements.forEach((elm) => elm.removeEventListener('focusout', this.handleFocusOut.bind(this)));
-    }
+    this._bindEventService.unbindAll();
   }
 
   createInput(position: 'leftInput' | 'rightInput'): HTMLInputElement {

src/app/modules/angular-slickgrid/extensions/__tests__/cellExternalCopyManagerExtension.spec.ts

@@ -1,10 +1,10 @@
 import { TestBed } from '@angular/core/testing';
 import { TranslateService, TranslateModule } from '@ngx-translate/core';
-import { GridOption } from '../../models/gridOption.interface';
+
 import { CellExternalCopyManagerExtension } from '../cellExternalCopyManagerExtension';
 import { ExtensionUtility } from '../extensionUtility';
 import { SharedService } from '../../services/shared.service';
-import { EditCommand, Formatter, SelectedRange } from '../../models';
+import { EditCommand, ExcelCopyBufferOption, Formatter, GridOption, SelectedRange } from '../../models';
 import { Formatters } from '../../formatters';
 
 declare const Slick: any;
@@ -89,7 +89,7 @@ describe('cellExternalCopyManagerExtension', () => {
 
     it('should register the addon', () => {
       const pluginSpy = jest.spyOn(SharedService.prototype.grid, 'registerPlugin');
-      const onRegisteredSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions, 'onExtensionRegistered');
+      const onRegisteredSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions as ExcelCopyBufferOption, 'onExtensionRegistered') as ExcelCopyBufferOption;
 
       const instance = extension.register();
       const addonInstance = extension.getAddonInstance();
@@ -114,9 +114,9 @@ describe('cellExternalCopyManagerExtension', () => {
 
     it('should call internal event handler subscribe and expect the "onCopyCells" option to be called when addon notify is called', () => {
       const handlerSpy = jest.spyOn(extension.eventHandler, 'subscribe');
-      const onCopySpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions, 'onCopyCells');
-      const onCancelSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions, 'onCopyCancelled');
-      const onPasteSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions, 'onPasteCells');
+      const onCopySpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions as ExcelCopyBufferOption, 'onCopyCells');
+      const onCancelSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions as ExcelCopyBufferOption, 'onCopyCancelled');
+      const onPasteSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions as ExcelCopyBufferOption, 'onPasteCells');
 
       const instance = extension.register();
       instance.onCopyCells.notify(mockSelectRangeEvent, new Slick.EventData(), gridStub);
@@ -133,9 +133,9 @@ describe('cellExternalCopyManagerExtension', () => {
 
     it('should call internal event handler subscribe and expect the "onCopyCancelled" option to be called when addon notify is called', () => {
       const handlerSpy = jest.spyOn(extension.eventHandler, 'subscribe');
-      const onCopySpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions, 'onCopyCells');
-      const onCancelSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions, 'onCopyCancelled');
-      const onPasteSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions, 'onPasteCells');
+      const onCopySpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions as ExcelCopyBufferOption, 'onCopyCells');
+      const onCancelSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions as ExcelCopyBufferOption, 'onCopyCancelled');
+      const onPasteSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions as ExcelCopyBufferOption, 'onPasteCells');
 
       const instance = extension.register();
       instance.onCopyCancelled.notify(mockSelectRangeEvent, new Slick.EventData(), gridStub);
@@ -152,9 +152,9 @@ describe('cellExternalCopyManagerExtension', () => {
 
     it('should call internal event handler subscribe and expect the "onPasteCells" option to be called when addon notify is called', () => {
       const handlerSpy = jest.spyOn(extension.eventHandler, 'subscribe');
-      const onCopySpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions, 'onCopyCells');
-      const onCancelSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions, 'onCopyCancelled');
-      const onPasteSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions, 'onPasteCells');
+      const onCopySpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions as ExcelCopyBufferOption, 'onCopyCells');
+      const onCancelSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions as ExcelCopyBufferOption, 'onCopyCancelled');
+      const onPasteSpy = jest.spyOn(SharedService.prototype.gridOptions.excelCopyBufferOptions as ExcelCopyBufferOption, 'onPasteCells');
 
       const instance = extension.register();
       instance.onPasteCells.notify(mockSelectRangeEvent, new Slick.EventData(), gridStub);

src/app/modules/angular-slickgrid/extensions/cellExternalCopyManagerExtension.ts

@@ -10,6 +10,7 @@ import {
   SlickEventHandler,
 } from '../models/index';
 import { ExtensionUtility } from './extensionUtility';
+import { BindingEventService } from '../services/bindingEvent.service';
 import { sanitizeHtmlToText } from '../services/utilities';
 import { SharedService } from '../services/shared.service';
 
@@ -21,13 +22,15 @@ declare const $: any;
 export class CellExternalCopyManagerExtension implements Extension {
   private _addon: any;
   private _addonOptions: ExcelCopyBufferOption | null;
+  private _bindingEventService: BindingEventService;
   private _cellSelectionModel: any;
   private _eventHandler: SlickEventHandler;
   private _commandQueue: EditCommand[];
   private _undoRedoBuffer: EditUndoRedoBuffer;
 
   constructor(private extensionUtility: ExtensionUtility, private sharedService: SharedService) {
     this._eventHandler = new Slick.EventHandler();
+    this._bindingEventService = new BindingEventService();
   }
 
   get addonOptions(): ExcelCopyBufferOption | null {
@@ -49,19 +52,17 @@ export class CellExternalCopyManagerExtension implements Extension {
   dispose() {
     // unsubscribe all SlickGrid events
     this._eventHandler.unsubscribeAll();
+    this._bindingEventService.unbindAll();
 
     if (this._addon && this._addon.destroy) {
       this._addon.destroy();
     }
-
-    this.extensionUtility.nullifyFunctionNameStartingWithOn(this._addonOptions);
-    this._addon = null;
-    this._addonOptions = null;
-
     if (this._cellSelectionModel && this._cellSelectionModel.destroy) {
       this._cellSelectionModel.destroy();
     }
-    document.removeEventListener('keydown', this.hookUndoShortcutKey.bind(this));
+    this.extensionUtility.nullifyFunctionNameStartingWithOn(this._addonOptions);
+    this._addon = null;
+    this._addonOptions = null;
   }
 
   /** Get the instance of the SlickGrid addon (control or plugin). */
@@ -74,7 +75,7 @@ export class CellExternalCopyManagerExtension implements Extension {
       // dynamically import the SlickGrid plugin (addon) with RequireJS
       this.extensionUtility.loadExtensionDynamically(ExtensionName.cellExternalCopyManager);
       this.createUndoRedoBuffer();
-      this.hookUndoShortcutKey();
+      this._bindingEventService.bind(document.body, 'keydown', this.handleKeyDown.bind(this));
 
       this._addonOptions = { ...this.getDefaultOptions(), ...this.sharedService.gridOptions.excelCopyBufferOptions } as ExcelCopyBufferOption;
       this._cellSelectionModel = new Slick.CellSelectionModel();
@@ -191,16 +192,14 @@ export class CellExternalCopyManagerExtension implements Extension {
   }
 
   /** Hook an undo shortcut key hook that will redo/undo the copy buffer using Ctrl+(Shift)+Z keyboard events */
-  private hookUndoShortcutKey() {
-    document.addEventListener('keydown', (e: KeyboardEvent) => {
-      const keyCode = e.keyCode || e.code;
-      if (keyCode === 90 && (e.ctrlKey || e.metaKey)) {
-        if (e.shiftKey) {
-          this._undoRedoBuffer.redo(); // Ctrl + Shift + Z
-        } else {
-          this._undoRedoBuffer.undo(); // Ctrl + Z
-        }
+  private handleKeyDown(e: KeyboardEvent) {
+    const keyCode = e.keyCode || e.code;
+    if (keyCode === 90 && (e.ctrlKey || e.metaKey)) {
+      if (e.shiftKey) {
+        this._undoRedoBuffer.redo(); // Ctrl + Shift + Z
+      } else {
+        this._undoRedoBuffer.undo(); // Ctrl + Z
       }
-    });
+    }
   }
 }

src/app/modules/angular-slickgrid/formatters/index.ts

@@ -82,6 +82,7 @@ export const Formatters = {
    * this.columnDefs = [{ id: 'username', field: 'user.firstName', ... }]
    * OR this.columnDefs = [{ id: 'username', field: 'user', params: { complexField: 'user.firstName' }, ... }]
    */
+  complex: complexObjectFormatter,
   complexObject: complexObjectFormatter,
 
   /**

src/app/modules/angular-slickgrid/models/domEvent.interface.ts

@@ -0,0 +1,4 @@
+export interface DOMEvent<T extends EventTarget> extends Event {
+  target: T
+  relatedTarget: T
+}

src/app/modules/angular-slickgrid/models/elementEventListener.interface.ts

@@ -0,0 +1,5 @@
+export interface ElementEventListener {
+  element: Element;
+  eventName: string;
+  listener: EventListenerOrEventListenerObject;
+}

src/app/modules/angular-slickgrid/models/index.ts

@@ -32,6 +32,7 @@ export * from './currentSorter.interface';
 export * from './customFooterOption.interface';
 export * from './dataViewOption.interface';
 export * from './delimiterType.enum';
+export * from './domEvent.interface';
 export * from './draggableGrouping.interface';
 export * from './editCommand.interface';
 export * from './editor.interface';
@@ -40,6 +41,7 @@ export * from './editorArguments.interface';
 export * from './editorValidator.interface';
 export * from './editorValidatorOutput.interface';
 export * from './editUndoRedoBuffer.interface';
+export * from './elementEventListener.interface';
 export * from './elementPosition.interface';
 export * from './emitterType.enum';
 export * from './emptyWarning.interface';

src/app/modules/angular-slickgrid/services/__tests__/bindingEvent.service.spec.ts

@@ -0,0 +1,60 @@
+import { BindingEventService } from '../bindingEvent.service';
+
+describe('BindingEvent Service', () => {
+  let div: HTMLDivElement;
+  let service: BindingEventService;
+
+  beforeEach(() => {
+    service = new BindingEventService();
+    div = document.createElement('div');
+    document.body.appendChild(div);
+  });
+
+  afterEach(() => {
+    div.remove();
+    service.unbindAll();
+    jest.clearAllMocks();
+  });
+
+  it('should be able to bind an event with listener to an element', () => {
+    const mockElm = { addEventListener: jest.fn() } as unknown as HTMLElement;
+    const mockCallback = jest.fn();
+    const addEventSpy = jest.spyOn(mockElm, 'addEventListener');
+    const elm = document.createElement('input');
+    div.appendChild(elm);
+
+    service.bind(mockElm, 'click', mockCallback);
+
+    expect(addEventSpy).toHaveBeenCalledWith('click', mockCallback, undefined);
+  });
+
+  it('should be able to bind an event with listener and options to an element', () => {
+    const mockElm = { addEventListener: jest.fn() } as unknown as HTMLElement;
+    const mockCallback = jest.fn();
+    const addEventSpy = jest.spyOn(mockElm, 'addEventListener');
+    const elm = document.createElement('input');
+    div.appendChild(elm);
+
+    service.bind(mockElm, 'click', mockCallback, { capture: true, passive: true });
+
+    expect(addEventSpy).toHaveBeenCalledWith('click', mockCallback, { capture: true, passive: true });
+  });
+
+  it('should call unbindAll and expect as many removeEventListener be called', () => {
+    const mockElm = { addEventListener: jest.fn(), removeEventListener: jest.fn() } as unknown as HTMLElement;
+    const addEventSpy = jest.spyOn(mockElm, 'addEventListener');
+    const removeEventSpy = jest.spyOn(mockElm, 'removeEventListener');
+    const mockCallback1 = jest.fn();
+    const mockCallback2 = jest.fn();
+
+    service = new BindingEventService();
+    service.bind(mockElm, 'keyup', mockCallback1);
+    service.bind(mockElm, 'click', mockCallback2, { capture: true, passive: true });
+    service.unbindAll();
+
+    expect(addEventSpy).toHaveBeenCalledWith('keyup', mockCallback1, undefined);
+    expect(addEventSpy).toHaveBeenCalledWith('click', mockCallback2, { capture: true, passive: true });
+    expect(removeEventSpy).toHaveBeenCalledWith('keyup', mockCallback1);
+    expect(removeEventSpy).toHaveBeenCalledWith('click', mockCallback2);
+  });
+});

src/app/modules/angular-slickgrid/services/bindingEvent.service.ts

@@ -0,0 +1,22 @@
+import { ElementEventListener } from '../models/elementEventListener.interface';
+
+export class BindingEventService {
+  private _boundedEvents: ElementEventListener[] = [];
+
+  /** Bind an event listener to any element */
+  bind(element: Element, eventName: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions) {
+    element.addEventListener(eventName, listener, options);
+    this._boundedEvents.push({ element, eventName, listener });
+  }
+
+  /** Unbind all will remove every every event handlers that were bounded earlier */
+  unbindAll() {
+    while (this._boundedEvents.length > 0) {
+      const boundedEvent = this._boundedEvents.pop() as ElementEventListener;
+      const { element, eventName, listener } = boundedEvent;
+      if (element && element.removeEventListener) {
+        element.removeEventListener(eventName, listener);
+      }
+    }
+  }
+}