removeAllListeners does not remove listeners completely

[Piotr Jasiun]

var a = CKEDITOR.document.getById( 'a' );

a.on( 'click', function( evt ) {
	console.log( 'clicked' );
} );

a.removeAllListeners();

a.fire( 'click' );
//get 'clicked' on the console.

After removeAllListeners indeed it is not possible to fire click event by clicking on the link but I can still call it by code (fire()).

See attached file.

[Piotrek Koszuliński]

Indeed, it doesn't work as we would expect. However, it may work correctly, as expected by its author. If I understand correctly, this methods main goal is to free references to unblock memory. It has the same name as CKEDITOR.event's method, but it may have different application. Also note that theoretically no one should fire native events on DOM nodes - only browser should do that. However, we do that in some cases and that's ok. So most likely this method should also remove our listeners.

[Piotr Jasiun]

Because we fire native events often in tests, we should make removeAllListeners works the same way for browser events and fire method. Otherwise editor in tests may works differently than in browser.

[Piotrek Koszuliński]

It will anyway ;) Firing mocked and not bubbling event is far from "natural". But I agree that less surprises is better.

[Piotrek Koszuliński]

Some time ago we noticed a weird issue #10365. It may be related to this ticket.

[Piotrek Koszuliński]

Ok, it struck me that those issues are indeed related and that domObj#removeAllListeners really should remove custom listeners as well. Explanation: ​http://dev.ckeditor.com/ticket/10365#comment:7

[Piotrek Koszuliński]

PJ, please remember about checking when this issue was introduced. We should know if this wasn't intentional - e.g. it may help with avoiding permission denied errors on IEs, but then it would be a workaround rather than real fix, so it'd be still worth fixing this.

[Piotr Jasiun]

Last modification of this method was in 2010 and it wasn't connected with this issue, so I believe that it was there from the beginning. Anyway I will check if fix doesn't break anything on IE.

[Piotr Jasiun]

Fixed. Changes in ​t/11400 and corresponding test branch.

[Piotrek Koszuliński]

Replying to pjasiun:

Last modification of this method was in 2010 and it wasn't connected with this issue, so I believe that it was there from the beginning. Anyway I will check if fix doesn't break anything on IE.

I asked about git bisect search. The commit which changed this behaviour could be unrelated to this method, but still it might change something intentionally.

[Piotr Jasiun]

I can not do bisect, because this issue existed since this method was implemented. It was introduces in (​git:b9bc4e8a). It had been created in domobject.js before it was created in event.js (​git:2b3d24fb, 2012) what is partially the reason of this issue. I mean now removeAllListeners in event.js works as I expect and remove listeners completely but the one in domobject.js not, probably because no one was thinking about fire method when he was working on it. Probably at the begging removeAllListeners was just a DOM operation.

[Piotrek Koszuliński]

IMO domObject#removeAllListeners should call event.prototype.removeAllListeners.

Except that everything is fine.

[Piotr Jasiun]

I changed it and rebase both dev and tests on master.

[Piotrek Koszuliński]

Fixed on master with ​git:ea89af0 on dev and e354684 on tests.