Opened 5 years ago

Closed 5 years ago

#11400 closed Bug (fixed)

removeAllListeners does not remove listeners completely

Reported by: Piotr Jasiun Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.3.3
Component: General Version:
Keywords: Cc:

Description

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.

Attachments (1)

removeAllListeners.html (542 bytes) - added by Piotr Jasiun 5 years ago.

Download all attachments as: .zip

Change History (18)

Changed 5 years ago by Piotr Jasiun

Attachment: removeAllListeners.html added

comment:1 Changed 5 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.3.3
Status: newpending

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.

comment:2 Changed 5 years ago by Piotr Jasiun

Summary: removeAllListeners does not block 'fire'removeAllListeners does not remove listeners completely

comment:3 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by Piotrek Koszuliński

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

Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

comment:5 Changed 5 years ago by Piotrek Koszuliński

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

comment:6 Changed 5 years ago by 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

comment:7 Changed 5 years ago by Piotrek Koszuliński

Status: pendingconfirmed

comment:8 Changed 5 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:9 Changed 5 years ago by 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.

comment:10 Changed 5 years ago by 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.

comment:11 Changed 5 years ago by Piotr Jasiun

Status: assignedreview

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

comment:12 in reply to:  10 Changed 5 years ago by 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.

comment:13 Changed 5 years ago by 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.

Last edited 5 years ago by Piotr Jasiun (previous) (diff)

comment:14 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_failed

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

Except that everything is fine.

comment:15 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

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

comment:16 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_passed

comment:17 Changed 5 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

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

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy