From 338ea2aa62337cba37e2e19cf9c6cac5fd884ccd Mon Sep 17 00:00:00 2001 From: Ghislain B Date: Fri, 27 Oct 2023 17:03:34 -0400 Subject: [PATCH] fix: sub-menu event listeners leaking when closing sub-menus (#888) - when a sub-menu is closed, we remove it from the DOM but we also need to remove any click event listeners attached to all list items --- src/controls/slick.gridmenu.ts | 9 ++++++--- src/models/core.interface.ts | 1 + src/plugins/slick.cellmenu.ts | 12 +++++++++--- src/plugins/slick.contextmenu.ts | 12 +++++++++--- src/plugins/slick.headermenu.ts | 7 ++++++- src/slick.core.ts | 33 +++++++++++++++++++++++++------- 6 files changed, 57 insertions(+), 17 deletions(-) diff --git a/src/controls/slick.gridmenu.ts b/src/controls/slick.gridmenu.ts index b961b4d17..741f42601 100644 --- a/src/controls/slick.gridmenu.ts +++ b/src/controls/slick.gridmenu.ts @@ -380,6 +380,7 @@ export class SlickGridMenu { /** Close and destroy all previously opened sub-menus */ destroySubMenus() { + this._bindingEventService.unbindAll('sub-menu'); document.querySelectorAll(`.slick-gridmenu.slick-submenu${this.getGridUidSelector()}`) .forEach(subElm => subElm.remove()); } @@ -387,7 +388,8 @@ export class SlickGridMenu { /** Construct the custom command menu items. */ protected populateCommandsMenu(commandItems: Array, commandListElm: HTMLElement, args: { grid: SlickGrid, level: number }) { // user could pass a title on top of the custom section - const isSubMenu = args.level > 0; + const level = args?.level || 0; + const isSubMenu = level > 0; if (!isSubMenu && (this._gridMenuOptions?.commandTitle || this._gridMenuOptions?.customTitle)) { this._commandTitleElm = document.createElement('div'); this._commandTitleElm.className = 'title'; @@ -470,14 +472,15 @@ export class SlickGridMenu { commandListElm.appendChild(liElm); if (addClickListener) { - this._bindingEventService.bind(liElm, 'click', this.handleMenuItemClick.bind(this, item, args.level) as EventListener); + const eventGroup = isSubMenu ? 'sub-menu' : 'parent-menu'; + this._bindingEventService.bind(liElm, 'click', this.handleMenuItemClick.bind(this, item, level) as EventListener, undefined, eventGroup); } // optionally open sub-menu(s) by mouseover if (this._gridMenuOptions?.subMenuOpenByEvent === 'mouseover') { this._bindingEventService.bind(liElm, 'mouseover', ((e: DOMMouseOrTouchEvent) => { if ((item as GridMenuItem).commandItems || (item as GridMenuItem).customItems) { - this.repositionSubMenu(item, args.level, e); + this.repositionSubMenu(item, level, e); } else if (!isSubMenu) { this.destroySubMenus(); } diff --git a/src/models/core.interface.ts b/src/models/core.interface.ts index 3aeb74739..d3360e303 100644 --- a/src/models/core.interface.ts +++ b/src/models/core.interface.ts @@ -4,6 +4,7 @@ export interface ElementEventListener { element: Element | Window; eventName: string; listener: EventListenerOrEventListenerObject; + groupName?: string; } export interface EditController { diff --git a/src/plugins/slick.cellmenu.ts b/src/plugins/slick.cellmenu.ts index 789d9c617..3ff6b73bd 100644 --- a/src/plugins/slick.cellmenu.ts +++ b/src/plugins/slick.cellmenu.ts @@ -434,12 +434,16 @@ export class SlickCellMenu implements SlickPlugin { /** Destroy all parent menus and any sub-menus */ destroyAllMenus() { this.destroySubMenus(); + + // remove all parent menu listeners before removing them from the DOM + this._bindingEventService.unbindAll('parent-menu'); document.querySelectorAll(`.slick-cell-menu${this.getGridUidSelector()}`) .forEach(subElm => subElm.remove()); } /** Close and destroy all previously opened sub-menus */ destroySubMenus() { + this._bindingEventService.unbindAll('sub-menu'); document.querySelectorAll(`.slick-cell-menu.slick-submenu${this.getGridUidSelector()}`) .forEach(subElm => subElm.remove()); } @@ -623,7 +627,8 @@ export class SlickCellMenu implements SlickPlugin { } // user could pass a title on top of the Commands/Options section - const isSubMenu = args.level > 0; + const level = args?.level || 0; + const isSubMenu = level > 0; if (cellMenu?.[`${itemType}Title`] && !isSubMenu) { this[`_${itemType}TitleElm`] = document.createElement('div'); this[`_${itemType}TitleElm`]!.className = 'slick-menu-title'; @@ -703,14 +708,15 @@ export class SlickCellMenu implements SlickPlugin { commandOrOptionMenuElm.appendChild(liElm); if (addClickListener) { - this._bindingEventService.bind(liElm, 'click', this.handleMenuItemClick.bind(this, item, itemType, args.level) as EventListener); + const eventGroup = isSubMenu ? 'sub-menu' : 'parent-menu'; + this._bindingEventService.bind(liElm, 'click', this.handleMenuItemClick.bind(this, item, itemType, level) as EventListener, undefined, eventGroup); } // optionally open sub-menu(s) by mouseover if (this._cellMenuProperties.subMenuOpenByEvent === 'mouseover') { this._bindingEventService.bind(liElm, 'mouseover', ((e: DOMMouseOrTouchEvent) => { if ((item as MenuCommandItem).commandItems || (item as MenuOptionItem).optionItems) { - this.repositionSubMenu(item, itemType, args.level, e); + this.repositionSubMenu(item, itemType, level, e); this._lastMenuTypeClicked = itemType; } else if (!isSubMenu) { this.destroySubMenus(); diff --git a/src/plugins/slick.contextmenu.ts b/src/plugins/slick.contextmenu.ts index 26bcb9951..952092bd6 100644 --- a/src/plugins/slick.contextmenu.ts +++ b/src/plugins/slick.contextmenu.ts @@ -450,12 +450,16 @@ export class SlickContextMenu implements SlickPlugin { /** Destroy all parent menus and any sub-menus */ destroyAllMenus() { this.destroySubMenus(); + + // remove all parent menu listeners before removing them from the DOM + this._bindingEventService.unbindAll('parent-menu'); document.querySelectorAll(`.slick-context-menu${this.getGridUidSelector()}`) .forEach(subElm => subElm.remove()); } /** Close and destroy all previously opened sub-menus */ destroySubMenus() { + this._bindingEventService.unbindAll('sub-menu'); document.querySelectorAll(`.slick-context-menu.slick-submenu${this.getGridUidSelector()}`) .forEach(subElm => subElm.remove()); } @@ -551,7 +555,8 @@ export class SlickContextMenu implements SlickPlugin { } // user could pass a title on top of the Commands/Options section - const isSubMenu = args.level > 0; + const level = args?.level || 0; + const isSubMenu = level > 0; if (contextMenu?.[`${itemType}Title`] && !isSubMenu) { this[`_${itemType}TitleElm`] = document.createElement('div'); this[`_${itemType}TitleElm`]!.className = 'slick-menu-title'; @@ -631,14 +636,15 @@ export class SlickContextMenu implements SlickPlugin { commandOrOptionMenuElm.appendChild(liElm); if (addClickListener) { - this._bindingEventService.bind(liElm, 'click', this.handleMenuItemClick.bind(this, item, itemType, args.level) as EventListener); + const eventGroup = isSubMenu ? 'sub-menu' : 'parent-menu'; + this._bindingEventService.bind(liElm, 'click', this.handleMenuItemClick.bind(this, item, itemType, level) as EventListener, undefined, eventGroup); } // optionally open sub-menu(s) by mouseover if (this._contextMenuProperties.subMenuOpenByEvent === 'mouseover') { this._bindingEventService.bind(liElm, 'mouseover', ((e: DOMMouseOrTouchEvent) => { if ((item as MenuCommandItem).commandItems || (item as MenuOptionItem).optionItems) { - this.repositionSubMenu(item, itemType, args.level, e); + this.repositionSubMenu(item, itemType, level, e); this._lastMenuTypeClicked = itemType; } else if (!isSubMenu) { this.destroySubMenus(); diff --git a/src/plugins/slick.headermenu.ts b/src/plugins/slick.headermenu.ts index 08e1c6c63..7e1a061f6 100644 --- a/src/plugins/slick.headermenu.ts +++ b/src/plugins/slick.headermenu.ts @@ -171,12 +171,16 @@ export class SlickHeaderMenu implements SlickPlugin { destroyAllMenus() { this.destroySubMenus(); + + // remove all parent menu listeners before removing them from the DOM + this._bindingEventService.unbindAll('parent-menu'); document.querySelectorAll(`.slick-header-menu${this.getGridUidSelector()}`) .forEach(subElm => subElm.remove()); } /** Close and destroy all previously opened sub-menus */ destroySubMenus() { + this._bindingEventService.unbindAll('sub-menu'); document.querySelectorAll(`.slick-header-menu.slick-submenu${this.getGridUidSelector()}`) .forEach(subElm => subElm.remove()); } @@ -425,7 +429,8 @@ export class SlickHeaderMenu implements SlickPlugin { menuElm.appendChild(menuItemElm); if (addClickListener) { - this._bindingEventService.bind(menuItemElm, 'click', this.handleMenuItemClick.bind(this, item, columnDef, level) as EventListener); + const eventGroup = isSubMenu ? 'sub-menu' : 'parent-menu'; + this._bindingEventService.bind(menuItemElm, 'click', this.handleMenuItemClick.bind(this, item, columnDef, level) as EventListener, undefined, eventGroup); } // optionally open sub-menu(s) by mouseover diff --git a/src/slick.core.ts b/src/slick.core.ts index f538d7bd5..6de33430a 100644 --- a/src/slick.core.ts +++ b/src/slick.core.ts @@ -552,9 +552,9 @@ export class BindingEventService { } /** Bind an event listener to any element */ - bind(element: Element | Window, eventName: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions) { + bind(element: Element | Window, eventName: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions, groupName = '') { element.addEventListener(eventName, listener, options); - this._boundedEvents.push({ element, eventName, listener }); + this._boundedEvents.push({ element, eventName, listener, groupName }); } /** Unbind all will remove every every event handlers that were bounded earlier */ @@ -571,11 +571,30 @@ export class BindingEventService { } } - /** 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; - this.unbind(boundedEvent.element, boundedEvent.eventName, boundedEvent.listener); + /** + * Unbind all event listeners that were bounded, optionally provide a group name to unbind all listeners assigned to that specific group only. + */ + unbindAll(groupName?: string | string[]) { + if (groupName) { + const groupNames = Array.isArray(groupName) ? groupName : [groupName]; + + // unbind only the bounded event with a specific group + // Note: we need to loop in reverse order to avoid array reindexing (causing index offset) after a splice is called + for (let i = this._boundedEvents.length - 1; i >= 0; --i) { + const boundedEvent = this._boundedEvents[i]; + if (groupNames.some(g => g === boundedEvent.groupName)) { + const { element, eventName, listener } = boundedEvent; + this.unbind(element, eventName, listener); + this._boundedEvents.splice(i, 1); + } + } + } else { + // unbind everything + while (this._boundedEvents.length > 0) { + const boundedEvent = this._boundedEvents.pop() as ElementEventListener; + const { element, eventName, listener } = boundedEvent; + this.unbind(element, eventName, listener); + } } } }