Merge pull request #190 from ckeditor/t/144-new

Other: Aligned behaviors of `EmitterMixin` methods responsible for adding end removing listeners. Closes #144.

The `emitter.on()` now has the same behavior as `emitter.listenTo( emitter )` as well as `emitter.off()` is the same as `emitter.stopListening( emitter )`. This made `emitter.stopListening()` correctly remove all listeners added in any way to it which prevents memory leaks.

Author: Reinmar

src/dom/emittermixin.js

@@ -40,6 +40,7 @@ const DomEmitterMixin = extend( {}, EmitterMixin, {
 	 * Registers a callback function to be executed when an event is fired in a specific Emitter or DOM Node.
 	 * It is backwards compatible with {@link module:utils/emittermixin~EmitterMixin#listenTo}.
 	 *
+	 * @method module:utils/dom/emittermixin~EmitterMixin#listenTo
 	 * @param {module:utils/emittermixin~Emitter|Node} emitter The object that fires the event.
 	 * @param {String} event The name of the event.
 	 * @param {Function} callback The function to be called on event.
@@ -49,20 +50,20 @@ const DomEmitterMixin = extend( {}, EmitterMixin, {
 	 * order they were added.
 	 * @param {Boolean} [options.useCapture=false] Indicates that events of this type will be dispatched to the registered
 	 * listener before being dispatched to any EventTarget beneath it in the DOM tree.
-	 *
-	 * @method module:utils/dom/emittermixin~EmitterMixin#listenTo
 	 */
-	listenTo( ...args ) {
-		const emitter = args[ 0 ];
-
+	listenTo( emitter, ...rest ) {
 		// Check if emitter is an instance of DOM Node. If so, replace the argument with
 		// corresponding ProxyEmitter (or create one if not existing).
 		if ( isDomNode( emitter ) || isWindow( emitter ) ) {
-			args[ 0 ] = this._getProxyEmitter( emitter ) || new ProxyEmitter( emitter );
+			const proxy = this._getProxyEmitter( emitter ) || new ProxyEmitter( emitter );
+
+			proxy.attach( ...rest );
+
+			emitter = proxy;
 		}
 
 		// Execute parent class method with Emitter (or ProxyEmitter) instance.
-		EmitterMixin.listenTo.apply( this, args );
+		EmitterMixin.listenTo.call( this, emitter, ...rest );
 	},
 
 	/**
@@ -74,17 +75,14 @@ const DomEmitterMixin = extend( {}, EmitterMixin, {
 	 * * To stop listening to all events fired by a specific object.
 	 * * To stop listening to all events fired by all object.
 	 *
+	 * @method module:utils/dom/emittermixin~EmitterMixin#stopListening
 	 * @param {module:utils/emittermixin~Emitter|Node} [emitter] The object to stop listening to. If omitted, stops it for all objects.
 	 * @param {String} [event] (Requires the `emitter`) The name of the event to stop listening to. If omitted, stops it
 	 * for all events from `emitter`.
 	 * @param {Function} [callback] (Requires the `event`) The function to be removed from the call list for the given
 	 * `event`.
-	 *
-	 * @method module:utils/dom/emittermixin~EmitterMixin#stopListening
 	 */
-	stopListening( ...args ) {
-		const emitter = args[ 0 ];
-
+	stopListening( emitter, event, callback ) {
 		// Check if emitter is an instance of DOM Node. If so, replace the argument with corresponding ProxyEmitter.
 		if ( isDomNode( emitter ) || isWindow( emitter ) ) {
 			const proxy = this._getProxyEmitter( emitter );
@@ -94,11 +92,15 @@ const DomEmitterMixin = extend( {}, EmitterMixin, {
 				return;
 			}
 
-			args[ 0 ] = proxy;
+			emitter = proxy;
 		}
 
 		// Execute parent class method with Emitter (or ProxyEmitter) instance.
-		EmitterMixin.stopListening.apply( this, args );
+		EmitterMixin.stopListening.call( this, emitter, event, callback );
+
+		if ( emitter instanceof ProxyEmitter ) {
+			emitter.detach( event );
+		}
 	},
 
 	/**
@@ -173,21 +175,14 @@ extend( ProxyEmitter.prototype, EmitterMixin, {
 	 * It attaches a native DOM listener to the DOM Node. When fired,
 	 * a corresponding Emitter event will also fire with DOM Event object as an argument.
 	 *
+	 * @method module:utils/dom/emittermixin~ProxyEmitter#attach
 	 * @param {String} event The name of the event.
 	 * @param {Function} callback The function to be called on event.
 	 * @param {Object} [options={}] Additional options.
-	 * @param {module:utils/priorities~PriorityString|Number} [options.priority='normal'] The priority of this event callback. The higher
-	 * the priority value the sooner the callback will be fired. Events having the same priority are called in the
-	 * order they were added.
 	 * @param {Boolean} [options.useCapture=false] Indicates that events of this type will be dispatched to the registered
 	 * listener before being dispatched to any EventTarget beneath it in the DOM tree.
-	 *
-	 * @method module:utils/dom/emittermixin~ProxyEmitter#on
 	 */
-	on( event, callback, options = {} ) {
-		// Execute parent class method first.
-		EmitterMixin.on.call( this, event, callback, options );
-
+	attach( event, callback, options = {} ) {
 		// If the DOM Listener for given event already exist it is pointless
 		// to attach another one.
 		if ( this._domListeners && this._domListeners[ event ] ) {
@@ -211,34 +206,27 @@ extend( ProxyEmitter.prototype, EmitterMixin, {
 	/**
 	 * Stops executing the callback on the given event.
 	 *
+	 * @method module:utils/dom/emittermixin~ProxyEmitter#detach
 	 * @param {String} event The name of the event.
-	 * @param {Function} callback The function to stop being called.
-	 *
-	 * @method module:utils/dom/emittermixin~ProxyEmitter#off
 	 */
-	off( event, callback ) {
-		// Execute parent class method first.
-		EmitterMixin.off.call( this, event, callback );
-
+	detach( event ) {
 		let events;
 
 		// Remove native DOM listeners which are orphans. If no callbacks
 		// are awaiting given event, detach native DOM listener from DOM Node.
-		// See: {@link on}.
+		// See: {@link attach}.
 
 		if ( this._domListeners[ event ] && ( !( events = this._events[ event ] ) || !events.callbacks.length ) ) {
 			this._domListeners[ event ].removeListener();
 		}
 	},
 
 	/**
-	 * Create a native DOM listener callback. When the native DOM event
+	 * Creates a native DOM listener callback. When the native DOM event
 	 * is fired it will fire corresponding event on this ProxyEmitter.
 	 * Note: A native DOM Event is passed as an argument.
 	 *
 	 * @private
-	 * @param {String} event
-	 *
 	 * @method module:utils/dom/emittermixin~ProxyEmitter#_createDomListener
 	 * @param {String} event The name of the event.
 	 * @param {Boolean} useCapture Indicates whether the listener was created for capturing event.
@@ -251,7 +239,7 @@ extend( ProxyEmitter.prototype, EmitterMixin, {
 
 		// Supply the DOM listener callback with a function that will help
 		// detach it from the DOM Node, when it is no longer necessary.
-		// See: {@link off}.
+		// See: {@link detach}.
 		domListener.removeListener = () => {
 			this._domNode.removeEventListener( event, domListener, useCapture );
 			delete this._domListeners[ event ];

src/emittermixin.js

@@ -24,21 +24,8 @@ const EmitterMixin = {
 	/**
 	 * Registers a callback function to be executed when an event is fired.
 	 *
-	 * Events can be grouped in namespaces using `:`.
-	 * When namespaced event is fired, it additionally fires all callbacks for that namespace.
-	 *
-	 *		myEmitter.on( 'myGroup', genericCallback );
-	 *		myEmitter.on( 'myGroup:myEvent', specificCallback );
-	 *
-	 *		// genericCallback is fired.
-	 *		myEmitter.fire( 'myGroup' );
-	 *		// both genericCallback and specificCallback are fired.
-	 *		myEmitter.fire( 'myGroup:myEvent' );
-	 *		// genericCallback is fired even though there are no callbacks for "foo".
-	 *		myEmitter.fire( 'myGroup:foo' );
-	 *
-	 * An event callback can {@link module:utils/eventinfo~EventInfo#stop stop the event} and
-	 * set the {@link module:utils/eventinfo~EventInfo#return return value} of the {@link #fire} method.
+	 * Shorthand for {@link #listenTo `this.listenTo( this, event, callback, options )`} (it makes the emitter
+	 * listen on itself).
 	 *
 	 * @method #on
 	 * @param {String} event The name of the event.
@@ -49,34 +36,7 @@ const EmitterMixin = {
 	 * order they were added.
 	 */
 	on( event, callback, options = {} ) {
-		createEventNamespace( this, event );
-		const lists = getCallbacksListsForNamespace( this, event );
-		const priority = priorities.get( options.priority );
-
-		callback = {
-			callback,
-			priority
-		};
-
-		// Add the callback to all callbacks list.
-		for ( const callbacks of lists ) {
-			// Add the callback to the list in the right priority position.
-			let added = false;
-
-			for ( let i = 0; i < callbacks.length; i++ ) {
-				if ( callbacks[ i ].priority < priority ) {
-					callbacks.splice( i, 0, callback );
-					added = true;
-
-					break;
-				}
-			}
-
-			// Add at the end, if right place was not found.
-			if ( !added ) {
-				callbacks.push( callback );
-			}
-		}
+		this.listenTo( this, event, callback, options );
 	},
 
 	/**
@@ -101,33 +61,41 @@ const EmitterMixin = {
 		};
 
 		// Make a similar on() call, simply replacing the callback.
-		this.on( event, onceCallback, options );
+		this.listenTo( this, event, onceCallback, options );
 	},
 
 	/**
 	 * Stops executing the callback on the given event.
+	 * Shorthand for {@link #stopListening `this.stopListening( this, event, callback )`}.
 	 *
 	 * @method #off
 	 * @param {String} event The name of the event.
 	 * @param {Function} callback The function to stop being called.
 	 */
 	off( event, callback ) {
-		const lists = getCallbacksListsForNamespace( this, event );
-
-		for ( const callbacks of lists ) {
-			for ( let i = 0; i < callbacks.length; i++ ) {
-				if ( callbacks[ i ].callback == callback ) {
-					// Remove the callback from the list (fixing the next index).
-					callbacks.splice( i, 1 );
-					i--;
-				}
-			}
-		}
+		this.stopListening( this, event, callback );
 	},
 
 	/**
 	 * Registers a callback function to be executed when an event is fired in a specific (emitter) object.
 	 *
+	 * Events can be grouped in namespaces using `:`.
+	 * When namespaced event is fired, it additionally fires all callbacks for that namespace.
+	 *
+	 *		// myEmitter.on( ... ) is a shorthand for myEmitter.listenTo( myEmitter, ... ).
+	 *		myEmitter.on( 'myGroup', genericCallback );
+	 *		myEmitter.on( 'myGroup:myEvent', specificCallback );
+	 *
+	 *		// genericCallback is fired.
+	 *		myEmitter.fire( 'myGroup' );
+	 *		// both genericCallback and specificCallback are fired.
+	 *		myEmitter.fire( 'myGroup:myEvent' );
+	 *		// genericCallback is fired even though there are no callbacks for "foo".
+	 *		myEmitter.fire( 'myGroup:foo' );
+	 *
+	 * An event callback can {@link module:utils/eventinfo~EventInfo#stop stop the event} and
+	 * set the {@link module:utils/eventinfo~EventInfo#return return value} of the {@link #fire} method.
+	 *
 	 * @method #listenTo
 	 * @param {module:utils/emittermixin~Emitter} emitter The object that fires the event.
 	 * @param {String} event The name of the event.
@@ -137,7 +105,7 @@ const EmitterMixin = {
 	 * the priority value the sooner the callback will be fired. Events having the same priority are called in the
 	 * order they were added.
 	 */
-	listenTo( emitter, event, callback, options ) {
+	listenTo( emitter, event, callback, options = {} ) {
 		let emitterInfo, eventCallbacks;
 
 		// _listeningTo contains a list of emitters that this object is listening to.
@@ -180,7 +148,34 @@ const EmitterMixin = {
 		eventCallbacks.push( callback );
 
 		// Finally register the callback to the event.
-		emitter.on( event, callback, options );
+		createEventNamespace( emitter, event );
+		const lists = getCallbacksListsForNamespace( emitter, event );
+		const priority = priorities.get( options.priority );
+
+		const callbackDefinition = {
+			callback,
+			priority
+		};
+
+		// Add the callback to all callbacks list.
+		for ( const callbacks of lists ) {
+			// Add the callback to the list in the right priority position.
+			let added = false;
+
+			for ( let i = 0; i < callbacks.length; i++ ) {
+				if ( callbacks[ i ].priority < priority ) {
+					callbacks.splice( i, 0, callbackDefinition );
+					added = true;
+
+					break;
+				}
+			}
+
+			// Add at the end, if right place was not found.
+			if ( !added ) {
+				callbacks.push( callbackDefinition );
+			}
+		}
 	},
 
 	/**
@@ -189,7 +184,7 @@ const EmitterMixin = {
 	 * * To stop listening to a specific callback.
 	 * * To stop listening to a specific event.
 	 * * To stop listening to all events fired by a specific object.
-	 * * To stop listening to all events fired by all object.
+	 * * To stop listening to all events fired by all objects.
 	 *
 	 * @method #stopListening
 	 * @param {module:utils/emittermixin~Emitter} [emitter] The object to stop listening to. If omitted, stops it for all objects.
@@ -211,13 +206,14 @@ const EmitterMixin = {
 
 		// All params provided. off() that single callback.
 		if ( callback ) {
-			emitter.off( event, callback );
+			removeCallback( emitter, event, callback );
 		}
 		// Only `emitter` and `event` provided. off() all callbacks for that event.
 		else if ( eventCallbacks ) {
 			while ( ( callback = eventCallbacks.pop() ) ) {
-				emitter.off( event, callback );
+				removeCallback( emitter, event, callback );
 			}
+
 			delete emitterInfo.callbacks[ event ];
 		}
 		// Only `emitter` provided. off() all events for that emitter.
@@ -246,7 +242,7 @@ const EmitterMixin = {
 	 * @param {String|module:utils/eventinfo~EventInfo} eventOrInfo The name of the event or `EventInfo` object if event is delegated.
 	 * @param {...*} [args] Additional arguments to be passed to the callbacks.
 	 * @returns {*} By default the method returns `undefined`. However, the return value can be changed by listeners
-	 * through modification of the {@link module:utils/eventinfo~EventInfo#return}'s value (the event info
+	 * through modification of the {@link module:utils/eventinfo~EventInfo#return `evt.return`}'s property (the event info
 	 * is the first param of every callback).
 	 */
 	fire( eventOrInfo, ...args ) {
@@ -277,7 +273,7 @@ const EmitterMixin = {
 					// Remove the called mark for the next calls.
 					delete eventInfo.off.called;
 
-					this.off( event, callbacks[ i ].callback );
+					removeCallback( this, event, callbacks[ i ].callback );
 				}
 
 				// Do not execute next callbacks if stop() was called.
@@ -508,7 +504,6 @@ function createEventNamespace( source, eventName ) {
 // Gets an array containing callbacks list for a given event and it's more specific events.
 // I.e. if given event is foo:bar and there is also foo:bar:abc event registered, this will
 // return callback list of foo:bar and foo:bar:abc (but not foo).
-// Returns empty array if given event has not been yet registered.
 function getCallbacksListsForNamespace( source, eventName ) {
 	const eventNode = getEvents( source )[ eventName ];
 
@@ -570,6 +565,25 @@ function fireDelegatedEvents( destinations, eventInfo, fireArgs ) {
 	}
 }
 
+// Removes callback from emitter for given event.
+//
+// @param {module:utils/emittermixin~Emitter} emitter
+// @param {String} event
+// @param {Function} callback
+function removeCallback( emitter, event, callback ) {
+	const lists = getCallbacksListsForNamespace( emitter, event );
+
+	for ( const callbacks of lists ) {
+		for ( let i = 0; i < callbacks.length; i++ ) {
+			if ( callbacks[ i ].callback == callback ) {
+				// Remove the callback from the list (fixing the next index).
+				callbacks.splice( i, 1 );
+				i--;
+			}
+		}
+	}
+}
+
 /**
  * Interface representing classes which mix in {@link module:utils/emittermixin~EmitterMixin}.
  *