Fix detecting changed events (#9050)

* Fix detecting changed events

Because `this._listeners` may contain both event handlers from options and internal event handlers for responsive support, the `setsEqual` check would often fail, causing event handlers to be unnecessarily detached and reattached and fired.

If I'm understanding correctly, this is the root cause of #9049.

* Use a separate object for responsive listeners

Correctly update events when responsive property changes as well as when requested events change.

* Code review feedback

diff --git a/src/core/core.controller.js b/src/core/core.controller.js
index 89071b55c8dc9bdb7a7f1cf8b9c1c23856b8dc1e..7cd76a4cbc47bb240a420bfa0d087926443f7f6f 100644 (file)
--- a/src/core/core.controller.js
+++ b/src/core/core.controller.js
@@ -116,8 +116,9 @@ class Chart {
     this.chartArea = undefined;
     this._active = [];
     this._lastEvent = undefined;
-    /** @type {{attach?: function, detach?: function, resize?: function}} */
     this._listeners = {};
+    /** @type {?{attach?: function, detach?: function, resize?: function}} */
+    this._responsiveListeners = undefined;
     this._sortedMetasets = [];
     this.scales = {};
     this.scale = undefined;
@@ -478,8 +479,8 @@ class Chart {
     const existingEvents = new Set(Object.keys(me._listeners));
     const newEvents = new Set(me.options.events);
 
-    if (!setsEqual(existingEvents, newEvents)) {
-      // The events array has changed. Rebind it
+    if (!setsEqual(existingEvents, newEvents) || !!this._responsiveListeners !== me.options.responsive) {
+      // The configured events have changed. Rebind.
       me.unbindEvents();
       me.bindEvents();
     }
@@ -889,6 +890,18 @@ class Chart {
         * @private
         */
   bindEvents() {
+    this.bindUserEvents();
+    if (this.options.responsive) {
+      this.bindResponsiveEvents();
+    } else {
+      this.attached = true;
+    }
+  }
+
+  /**
+   * @private
+   */
+  bindUserEvents() {
     const me = this;
     const listeners = me._listeners;
     const platform = me.platform;
@@ -897,53 +910,66 @@ class Chart {
       platform.addEventListener(me, type, listener);
       listeners[type] = listener;
     };
-    const _remove = (type, listener) => {
-      if (listeners[type]) {
-        platform.removeEventListener(me, type, listener);
-        delete listeners[type];
-      }
-    };
 
-    let listener = function(e, x, y) {
+    const listener = function(e, x, y) {
       e.offsetX = x;
       e.offsetY = y;
       me._eventHandler(e);
     };
 
     each(me.options.events, (type) => _add(type, listener));
+  }
 
-    if (me.options.responsive) {
-      listener = (width, height) => {
-        if (me.canvas) {
-          me.resize(width, height);
-        }
-      };
+  /**
+   * @private
+   */
+  bindResponsiveEvents() {
+    const me = this;
+    if (!me._responsiveListeners) {
+      me._responsiveListeners = {};
+    }
+    const listeners = me._responsiveListeners;
+    const platform = me.platform;
 
-      let detached; // eslint-disable-line prefer-const
-      const attached = () => {
-        _remove('attach', attached);
+    const _add = (type, listener) => {
+      platform.addEventListener(me, type, listener);
+      listeners[type] = listener;
+    };
+    const _remove = (type, listener) => {
+      if (listeners[type]) {
+        platform.removeEventListener(me, type, listener);
+        delete listeners[type];
+      }
+    };
 
-        me.attached = true;
-        me.resize();
+    const listener = (width, height) => {
+      if (me.canvas) {
+        me.resize(width, height);
+      }
+    };
 
-        _add('resize', listener);
-        _add('detach', detached);
-      };
+    let detached; // eslint-disable-line prefer-const
+    const attached = () => {
+      _remove('attach', attached);
 
-      detached = () => {
-        me.attached = false;
+      me.attached = true;
+      me.resize();
 
-        _remove('resize', listener);
-        _add('attach', attached);
-      };
+      _add('resize', listener);
+      _add('detach', detached);
+    };
 
-      if (platform.isAttached(me.canvas)) {
-        attached();
-      } else {
-        detached();
-      }
+    detached = () => {
+      me.attached = false;
+
+      _remove('resize', listener);
+      _add('attach', attached);
+    };
+
+    if (platform.isAttached(me.canvas)) {
+      attached();
     } else {
-      me.attached = true;
+      detached();
     }
   }
 
@@ -952,15 +978,16 @@ class Chart {
         */
   unbindEvents() {
     const me = this;
-    const listeners = me._listeners;
-    if (!listeners) {
-      return;
-    }
 
+    each(me._listeners, (listener, type) => {
+      me.platform.removeEventListener(me, type, listener);
+    });
     me._listeners = {};
-    each(listeners, (listener, type) => {
+
+    each(me._responsiveListeners, (listener, type) => {
       me.platform.removeEventListener(me, type, listener);
     });
+    me._responsiveListeners = undefined;
   }
 
   updateHoverStyle(items, mode, enabled) {