fix(common): unbindAll with a group name should remove all tagged o…

…nes (#1152)

- previous PR #1150 brought this new feature but it was found that if the bounded events to unbind are at the end of the array and we called `unbindAll`, it wasn't removing all espected events because calling `splice` whithin the logic was reindexing the array causing an index offset and it missed unbounding them all.
- the best approach is to loop in reverse order which has no consequences on the array indexes and will not cause offset

Co-authored-by: ghiscoding <[email protected]>

packages/common/src/services/__tests__/bindingEvent.service.spec.ts

@@ -81,36 +81,67 @@ describe('BindingEvent Service', () => {
     expect(mockElm.removeEventListener).toHaveBeenCalledWith('click', mockCallback2);
   });
 
-  it('should call unbindAll with a group name and expect that group listeners to be removed but others kept', () => {
+  it('should call unbindAll with a single group name and expect that group listeners to be removed but others kept', () => {
     const mockElm1 = { addEventListener: jest.fn(), removeEventListener: jest.fn() } as unknown as HTMLElement;
-    const mockElm2 = { addEventListener: jest.fn(), removeEventListener: jest.fn() } as unknown as HTMLElement;
     const mockCallback1 = jest.fn();
     const mockCallback2 = jest.fn();
     const mockCallback3 = jest.fn();
     const mockCallback4 = jest.fn();
     const mockCallback5 = jest.fn();
 
     service = new BindingEventService();
-    service.bind(mockElm1, 'keyup', mockCallback1, false, 'same-group');
+    service.bind(mockElm1, 'keyup', mockCallback1, false, 'wonderful');
     service.bind(mockElm1, 'keydown', mockCallback2, { capture: true, passive: true }, 'magic');
-    service.bind(mockElm2, 'click', mockCallback3, { capture: true, passive: true }); // no group
-    service.bind(mockElm2, 'mouseover', mockCallback4, { capture: false, passive: true }, 'same-group');
-    service.bind(mockElm2, 'mouseout', mockCallback5, { capture: false, passive: false }, 'wonderful');
+    service.bind(mockElm1, 'click', mockCallback3, { capture: true, passive: true }); // no group
+    service.bind(mockElm1, 'mouseover', mockCallback4, { capture: false, passive: true }, 'mouse-group');
+    service.bind(mockElm1, 'mouseout', mockCallback5, { capture: false, passive: false }, 'mouse-group');
 
     expect(service.boundedEvents.length).toBe(5);
-    expect(mockElm1.addEventListener).toHaveBeenCalledWith('keyup', mockCallback1, false); // same-group
+    expect(mockElm1.addEventListener).toHaveBeenCalledWith('keyup', mockCallback1, false);
     expect(mockElm1.addEventListener).toHaveBeenCalledWith('keydown', mockCallback2, { capture: true, passive: true });
-    expect(mockElm2.addEventListener).toHaveBeenCalledWith('click', mockCallback3, { capture: true, passive: true });
-    expect(mockElm2.addEventListener).toHaveBeenCalledWith('mouseover', mockCallback4, { capture: false, passive: true }); // same-group
-    expect(mockElm2.addEventListener).toHaveBeenCalledWith('mouseout', mockCallback5, { capture: false, passive: false });
+    expect(mockElm1.addEventListener).toHaveBeenCalledWith('click', mockCallback3, { capture: true, passive: true });
+    expect(mockElm1.addEventListener).toHaveBeenCalledWith('mouseover', mockCallback4, { capture: false, passive: true }); // mouse-group
+    expect(mockElm1.addEventListener).toHaveBeenCalledWith('mouseout', mockCallback5, { capture: false, passive: false }); // mouse-group
 
-    service.unbindAll('same-group');
+    service.unbindAll('mouse-group');
 
     expect(service.boundedEvents.length).toBe(3);
-    expect(mockElm1.removeEventListener).toHaveBeenCalledWith('keyup', mockCallback1); // same-group
-    expect(mockElm1.removeEventListener).not.toHaveBeenCalledWith();
-    expect(mockElm2.removeEventListener).not.toHaveBeenCalledWith();
-    expect(mockElm2.removeEventListener).toHaveBeenCalledWith('mouseover', mockCallback4); // same-group
-    expect(mockElm2.removeEventListener).not.toHaveBeenCalledWith();
+    expect(mockElm1.removeEventListener).not.toHaveBeenCalledWith('keyup', mockCallback1);
+    expect(mockElm1.removeEventListener).not.toHaveBeenCalledWith('keydown', mockCallback2);
+    expect(mockElm1.removeEventListener).not.toHaveBeenCalledWith('click', mockCallback3);
+    expect(mockElm1.removeEventListener).toHaveBeenCalledWith('mouseover', mockCallback4); // mouse-group
+    expect(mockElm1.removeEventListener).toHaveBeenCalledWith('mouseout', mockCallback5); // mouse-group
+  });
+
+  it('should call unbindAll with a multiple group names and expect those group listeners to be removed but others kept', () => {
+    const mockElm1 = { addEventListener: jest.fn(), removeEventListener: jest.fn() } as unknown as HTMLElement;
+    const mockCallback1 = jest.fn();
+    const mockCallback2 = jest.fn();
+    const mockCallback3 = jest.fn();
+    const mockCallback4 = jest.fn();
+    const mockCallback5 = jest.fn();
+
+    service = new BindingEventService();
+    service.bind(mockElm1, 'keyup', mockCallback1, false, 'wonderful');
+    service.bind(mockElm1, 'keydown', mockCallback2, { capture: true, passive: true }, 'magic');
+    service.bind(mockElm1, 'click', mockCallback3, { capture: true, passive: true }); // no group
+    service.bind(mockElm1, 'mouseover', mockCallback4, { capture: false, passive: true }, 'mouse-group');
+    service.bind(mockElm1, 'mouseout', mockCallback5, { capture: false, passive: false }, 'mouse-group');
+
+    expect(service.boundedEvents.length).toBe(5);
+    expect(mockElm1.addEventListener).toHaveBeenCalledWith('keyup', mockCallback1, false);
+    expect(mockElm1.addEventListener).toHaveBeenCalledWith('keydown', mockCallback2, { capture: true, passive: true });
+    expect(mockElm1.addEventListener).toHaveBeenCalledWith('click', mockCallback3, { capture: true, passive: true });
+    expect(mockElm1.addEventListener).toHaveBeenCalledWith('mouseover', mockCallback4, { capture: false, passive: true }); // mouse-group
+    expect(mockElm1.addEventListener).toHaveBeenCalledWith('mouseout', mockCallback5, { capture: false, passive: false }); // mouse-group
+
+    service.unbindAll(['magic', 'mouse-group']);
+
+    expect(service.boundedEvents.length).toBe(2);
+    expect(mockElm1.removeEventListener).not.toHaveBeenCalledWith('keyup', mockCallback1);
+    expect(mockElm1.removeEventListener).toHaveBeenCalledWith('keydown', mockCallback2); // magic
+    expect(mockElm1.removeEventListener).not.toHaveBeenCalledWith('click', mockCallback3);
+    expect(mockElm1.removeEventListener).toHaveBeenCalledWith('mouseover', mockCallback4); // mouse-group
+    expect(mockElm1.removeEventListener).toHaveBeenCalledWith('mouseout', mockCallback5); // mouse-group
   });
 });

packages/common/src/services/bindingEvent.service.ts

@@ -52,16 +52,20 @@ export class BindingEventService {
   /**
    * 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) {
+  unbindAll(groupName?: string | string[]) {
     if (groupName) {
+      const groupNames = Array.isArray(groupName) ? groupName : [groupName];
+
       // unbind only the bounded event with a specific group
-      this._boundedEvents.forEach((boundedEvent, idx) => {
-        if (boundedEvent.groupName === groupName) {
+      // 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(idx, 1);
+          this._boundedEvents.splice(i, 1);
         }
-      });
+      }
     } else {
       // unbind everything
       while (this._boundedEvents.length > 0) {