Added utility to manage collection of listeners

Following on @robertknight
[suggestion](https://github.com/hypothesis/client/pull/3035#pullrequestreview-599668554)
I created a tiny utility to make easier to unregister event listeners.

src/annotator/bucket-bar.js

@@ -2,6 +2,7 @@ import { render } from 'preact';
 import Buckets from './components/Buckets';
 
 import { anchorBuckets } from './util/buckets';
+import { ListenerCollection } from './util/listener-collection';
 
 /**
  * @typedef BucketBarOptions
@@ -23,20 +24,18 @@ export default class BucketBar {
     this.guest = guest;
     container.appendChild(this.element);
 
-    this.updateFunc = () => this.update();
+    this._listeners = new ListenerCollection();
 
-    window.addEventListener('resize', this.updateFunc);
-    window.addEventListener('scroll', this.updateFunc);
-    contentContainer.addEventListener('scroll', this.updateFunc);
+    this._listeners.add(window, 'resize', () => this.update());
+    this._listeners.add(window, 'scroll', () => this.update());
+    this._listeners.add(contentContainer, 'scroll', () => this.update());
 
     // Immediately render the buckets for the current anchors.
     this._update();
   }
 
   destroy() {
-    window.removeEventListener('resize', this.updateFunc);
-    window.removeEventListener('scroll', this.updateFunc);
-    this._contentContainer.removeEventListener('scroll', this.updateFunc);
+    this._listeners.removeAll();
     this.element.remove();
   }
 

src/annotator/pdf-sidebar.js

@@ -36,7 +36,7 @@ export default class PdfSidebar extends Sidebar {
     this.sideBySideActive = false;
 
     this.subscribe('sidebarLayoutChanged', state => this.fitSideBySide(state));
-    this._registerEvent(window, 'resize', () => this.fitSideBySide());
+    this._listeners.add(window, 'resize', () => this.fitSideBySide());
   }
 
   /**

src/annotator/plugin/pdf.js

@@ -6,6 +6,7 @@ import WarningBanner from '../components/WarningBanner';
 import Delegator from '../delegator';
 import RenderingStates from '../pdfjs-rendering-states';
 import { createShadowRoot } from '../util/shadow-root';
+import { ListenerCollection } from '../util/listener-collection';
 
 import PDFMetadata from './pdf-metadata';
 
@@ -63,17 +64,16 @@ export default class PDF extends Delegator {
       );
     };
 
-    document.addEventListener(
+    this._listeners = new ListenerCollection();
+    this._listeners.add(
+      document,
       'selectionchange',
       this._updateAnnotationLayerVisibility
     );
   }
 
   destroy() {
-    document.removeEventListener(
-      'selectionchange',
-      this._updateAnnotationLayerVisibility
-    );
+    this._listeners.removeAll();
     this.pdfViewer.viewer.classList.remove('has-transparent-text-layer');
     this.observer.disconnect();
   }

src/annotator/selection-observer.js

+import { ListenerCollection } from './util/listener-collection';
+
 /**
  * Return the current selection or `null` if there is no selection or it is empty.
  *
@@ -73,9 +75,10 @@ export class SelectionObserver {
     };
 
     this._document = document_;
+    this._listeners = new ListenerCollection();
     this._events = ['mousedown', 'mouseup', 'selectionchange'];
     for (let event of this._events) {
-      document_.addEventListener(event, this._eventHandler);
+      this._listeners.add(document_, event, this._eventHandler);
     }
 
     // Report the initial selection.
@@ -83,9 +86,7 @@ export class SelectionObserver {
   }
 
   disconnect() {
-    for (let event of this._events) {
-      this._document.removeEventListener(event, this._eventHandler);
-    }
+    this._listeners.removeAll();
     this._cancelPendingCallback();
   }
 

src/annotator/sidebar.js

@@ -10,6 +10,7 @@ import Delegator from './delegator';
 import { ToolbarController } from './toolbar';
 import { createShadowRoot } from './util/shadow-root';
 import BucketBar from './bucket-bar';
+import { ListenerCollection } from './util/listener-collection';
 
 /**
  * @typedef {import('./guest').default} Guest
@@ -20,13 +21,6 @@ import BucketBar from './bucket-bar';
  * @prop {number} height
  */
 
-/**
- * @typedef RegisteredListener
- * @prop {Window|HTMLElement} eventTarget
- * @prop {string} eventType
- * @prop {(event: any) => void} listener
- */
-
 // Minimum width to which the iframeContainer can be resized.
 export const MIN_RESIZE = 280;
 
@@ -108,8 +102,7 @@ export default class Sidebar extends Delegator {
 
     this.guest = guest;
 
-    /** @type {RegisteredListener[]} */
-    this.registeredListeners = [];
+    this._listeners = new ListenerCollection();
 
     this.subscribe('panelReady', () => {
       // Show the UI
@@ -167,7 +160,7 @@ export default class Sidebar extends Delegator {
       this.toolbarWidth = 0;
     }
 
-    this._registerEvent(window, 'resize', () => this._onResize());
+    this._listeners.add(window, 'resize', () => this._onResize());
 
     this._gestureState = {
       // Initial position at the start of a drag/pan resize event (in pixels).
@@ -198,7 +191,7 @@ export default class Sidebar extends Delegator {
 
   destroy() {
     this.bucketBar?.destroy();
-    this._unregisterEvents();
+    this._listeners.removeAll();
     this._hammerManager?.destroy();
     if (this.hypothesisSidebar) {
       this.hypothesisSidebar.remove();
@@ -208,23 +201,6 @@ export default class Sidebar extends Delegator {
     super.destroy();
   }
 
-  /**
-   * @param {Window|HTMLElement} eventTarget
-   * @param {string} eventType
-   * @param {(event: any) => void} listener
-   */
-  _registerEvent(eventTarget, eventType, listener) {
-    eventTarget.addEventListener(eventType, listener);
-    this.registeredListeners.push({ eventTarget, eventType, listener });
-  }
-
-  _unregisterEvents() {
-    this.registeredListeners.forEach(({ eventTarget, eventType, listener }) => {
-      eventTarget.removeEventListener(eventType, listener);
-    });
-    this.registeredListeners = [];
-  }
-
   _setupSidebarEvents() {
     annotationCounts(document.body, this.guest.crossframe);
     sidebarTrigger(document.body, () => this.open());
@@ -267,7 +243,7 @@ export default class Sidebar extends Delegator {
     const toggleButton = this.toolbar.sidebarToggleButton;
     if (toggleButton) {
       // Prevent any default gestures on the handle.
-      this._registerEvent(toggleButton, 'touchmove', e => e.preventDefault());
+      this._listeners.add(toggleButton, 'touchmove', e => e.preventDefault());
 
       this._hammerManager = new Hammer.Manager(toggleButton).on(
         'panstart panend panleft panright',

src/annotator/test/pdf-sidebar-test.js

@@ -2,15 +2,13 @@ import PdfSidebar from '../pdf-sidebar';
 import Delegator from '../delegator';
 
 import { mockBaseClass } from '../../test-util/mock-base';
+import { ListenerCollection } from '../util/listener-collection';
 
 class FakeSidebar extends Delegator {
   constructor(element, guest, config) {
     super(element, config);
     this.guest = guest;
-  }
-
-  _registerEvent(target, event, callback) {
-    target.addEventListener(event, callback);
+    this._listeners = new ListenerCollection();
   }
 }
 

src/annotator/test/sidebar-test.js

@@ -625,27 +625,6 @@ describe('Sidebar', () => {
     });
   });
 
-  describe('register/unregister events', () => {
-    it('triggers registered event listener', () => {
-      const sidebar = createSidebar();
-      const listener = sinon.stub();
-      sidebar._registerEvent(window, 'resize', listener);
-
-      window.dispatchEvent(new Event('resize'));
-      assert.calledOnce(listener);
-    });
-
-    it('unregisters event listeners', () => {
-      const sidebar = createSidebar();
-      const listener = sinon.stub();
-      sidebar._registerEvent(window, 'resize', listener);
-      sidebar.destroy();
-
-      window.dispatchEvent(new Event('resize'));
-      assert.notCalled(listener);
-    });
-  });
-
   describe('layout change notifier', () => {
     let layoutChangeHandlerSpy;
 

src/annotator/util/listener-collection.js

+/**
+ * @typedef Listener
+ * @prop {EventTarget} eventTarget
+ * @prop {string} eventType
+ * @prop {(event: Event) => void} listener
+ */
+
+/**
+ * Utility that provides a way to conveniently remove a set of DOM event
+ * listeners when they are no longer needed.
+ */
+export class ListenerCollection {
+  constructor() {
+    /** @type {Listener[]} */
+    this._listeners = [];
+  }
+
+  /**
+   * @param {Listener['eventTarget']} eventTarget
+   * @param {Listener['eventType']} eventType
+   * @param {Listener['listener']} listener
+   * @param {AddEventListenerOptions} [options]
+   */
+  add(eventTarget, eventType, listener, options) {
+    eventTarget.addEventListener(eventType, listener, options);
+    this._listeners.push({ eventTarget, eventType, listener });
+  }
+
+  removeAll() {
+    this._listeners.forEach(({ eventTarget, eventType, listener }) => {
+      eventTarget.removeEventListener(eventType, listener);
+    });
+    this._listeners = [];
+  }
+}

src/annotator/util/test/listener-collection-test.js

+import { ListenerCollection } from '../../util/listener-collection';
+
+describe('ListenerCollection', () => {
+  let listeners;
+
+  beforeEach(() => {
+    listeners = new ListenerCollection();
+  });
+
+  afterEach(() => {
+    listeners.removeAll();
+  });
+
+  it('registers and triggers event listener', () => {
+    const listener = sinon.stub();
+    listeners.add(window, 'resize', listener);
+
+    window.dispatchEvent(new Event('resize'));
+    assert.calledOnce(listener);
+  });
+
+  it('unregisters event listeners', () => {
+    const listener1 = sinon.stub();
+    const listener2 = sinon.stub();
+    listeners.add(window, 'resize', listener1);
+    listeners.add(window, 'resize', listener2);
+    listeners.removeAll();
+
+    window.dispatchEvent(new Event('resize'));
+    assert.notCalled(listener1);
+    assert.notCalled(listener2);
+  });
+});

src/shared/discovery.js

@@ -8,6 +8,8 @@
  * @param {string} token - A random identifier used by this frame.
  */
 
+import { ListenerCollection } from '../annotator/util/listener-collection';
+
 /**
  * Discovery finds frames in the current tab/window that can be annotated (the
  * "clients") or can fetch annotations from the backend (the "server").
@@ -65,6 +67,7 @@ export default class Discovery {
     }
 
     this._onMessage = this._onMessage.bind(this);
+    this._listeners = new ListenerCollection();
   }
 
   /**
@@ -86,7 +89,7 @@ export default class Discovery {
     this.onDiscovery = onDiscovery;
 
     // Listen for messages from other frames.
-    this.target.addEventListener('message', this._onMessage, false);
+    this._listeners.add(this.target, 'message', this._onMessage);
     this._beacon();
   }
 
@@ -95,7 +98,7 @@ export default class Discovery {
    */
   stopDiscovery() {
     this.onDiscovery = null;
-    this.target.removeEventListener('message', this._onMessage);
+    this._listeners.removeAll();
   }
 
   /**

src/shared/test/discovery-test.js

@@ -32,8 +32,7 @@ describe('shared/discovery', () => {
       assert.calledWith(
         fakeTopWindow.addEventListener,
         'message',
-        sinon.match.func,
-        false
+        sinon.match.func
       );
     });
   });

src/sidebar/components/ThreadList.js

@@ -10,6 +10,7 @@ import {
 } from '../helpers/visible-threads';
 
 import ThreadCard from './ThreadCard';
+import { ListenerCollection } from '../../annotator/util/listener-collection';
 
 /** @typedef {import('../helpers/build-thread').Thread} Thread */
 
@@ -148,6 +149,7 @@ function ThreadList({ threads }) {
   // Attach listeners such that whenever the scroll container is scrolled or the
   // window resized, a recalculation of visible threads is triggered
   useEffect(() => {
+    const listeners = new ListenerCollection();
     const scrollContainer = getScrollContainer();
 
     const updateScrollPosition = debounce(
@@ -159,12 +161,11 @@ function ThreadList({ threads }) {
       { maxWait: 100 }
     );
 
-    scrollContainer.addEventListener('scroll', updateScrollPosition);
-    window.addEventListener('resize', updateScrollPosition);
+    listeners.add(scrollContainer, 'scroll', updateScrollPosition);
+    listeners.add(window, 'resize', updateScrollPosition);
 
     return () => {
-      scrollContainer.removeEventListener('scroll', updateScrollPosition);
-      window.removeEventListener('resize', updateScrollPosition);
+      listeners.removeAll();
       updateScrollPosition.cancel();
     };
   }, []);

src/sidebar/util/observe-element-size.js

+import { ListenerCollection } from '../../annotator/util/listener-collection';
+
 /**
  * Watch for changes in the size (`clientWidth` and `clientHeight`) of
  * an element.
@@ -19,6 +21,7 @@ export default function observeElementSize(element, onSizeChanged) {
     observer.observe(element);
     return () => observer.disconnect();
   }
+  const listeners = new ListenerCollection();
 
   // Fallback method which listens for the most common events that result in
   // element size changes:
@@ -44,8 +47,8 @@ export default function observeElementSize(element, onSizeChanged) {
     }
   };
 
-  element.addEventListener('load', check);
-  window.addEventListener('resize', check);
+  listeners.add(element, 'load', check);
+  listeners.add(window, 'resize', check);
   const observer = new MutationObserver(check);
   observer.observe(element, {
     characterData: true,
@@ -54,8 +57,7 @@ export default function observeElementSize(element, onSizeChanged) {
   });
 
   return () => {
-    element.removeEventListener('load', check);
-    window.removeEventListener('resize', check);
+    listeners.removeAll();
     observer.disconnect();
   };
 }