Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#10365 closed Bug (invalid)

setData doesn't clear properly the document listeners

Reported by: Alfonso Martínez de Lizarrondo Owned by:
Priority: Normal Milestone:
Component: Core : Editable Version: 4.0
Keywords: Cc:

Description

The sample file is just the API.html with an extra listener to the 'contentDom' event that logs an entry in the console and then sets a click listener on the body:

contentDom : function(e) {

console.log('contentDom'); var doc = e.editor.document; doc.on('click', function() { console.log('click'); });

}

That setup works correctly in CKEditor 3, but in 4 it only works until setData is called. Then the contentDom event is logged, but the native listeners on the document remain (so they aren't attached again) although I think that the document itself is changed.

I mean: CKEditor thinks that the native listeners are already set for that element, but the element itself has changed and now no longer works until you switch from design to source and back forcing a full reset.

And this is why some people have been complaining that the onchange plugin doesn't work with CKEditor 4.

Attachments (2)

api.html (7.0 KB) - added by Alfonso Martínez de Lizarrondo 7 years ago.
testcase
api7.html (7.1 KB) - added by Jakub Ś 7 years ago.

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by Alfonso Martínez de Lizarrondo

Attachment: api.html added

testcase

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

Hmm... I have a feeling that something has changed in Webkit recently, because previously we haven't seen need to use editable#attachListener for document events. In last two weeks I was fixing two issues with that - in both cases I had to change doc#on to editable#attachListener( doc ), because otherwise second and next listeners haven't been fired (unless iframe was completely replaced). And this issue was reproducible only manually - programatically fired events are triggering right listeners. Weird ;/

One of these tickets: #10330.

So if you agree I would close this issue. In any case - I'll try to find out a little bit more about this issue with document, because it is intriguing.

comment:2 Changed 7 years ago by Alfonso Martínez de Lizarrondo

I'm testing with Firefox, so it's not just webkit.

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

Ok. Then I'll try to find if something changed in CKEditor since v3 regarding multiple listeners on document.

comment:4 Changed 7 years ago by Jakub Ś

Resolution: invalid
Status: newclosed

I think I have made similar "mistake" with AutoSave. As @Reinmar has explained this to me eventListeners should be assigned to editable and not document only. I didn't ask why just accepted it but if possible explanation would be welcome (probably because of design :) )

I'm attaching working sample.

I'm closing this issue as invalid. If you guys have however anything against, please comment or reopen ticket.

Changed 7 years ago by Jakub Ś

Attachment: api7.html added

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

As I wrote before - I'm still going to check this, because in fact your case, Kuba, is another that proves that something has changed recently or in v4. But let's leave this ticket closed for now, I'll keep it in my TODOs.

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

It took us a while, but it looks like we found something: #11400.

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

And yes... now everything makes sense. As I mentioned in comment:1, using editable.attachListeners was helping for attaching a listener to document on second and following contentDom events. It did, because in this case those custom listeners are automatically removed on editable detaching, so new one for the same event and function can be added. If removeAllListeners were correctly removing also custom listeners besides removing only native listeners, then there would be no need for using attachListeners, because after document.removeAllListeners done on editable.detach() everything would be reset to initial state.

Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy