Allow destroy after object removed from DOM

[Doug Davis]

related to: ​http://dev.ckeditor.com/changeset/5681

I would like to 1) Be able to call destroy() after the editor has been removed from the DOM. Or even better... 2) If the editor has been removed from the DOM already, and a new editor with the same name gets created, the old one will automatically be destroyed. Not being in the DOM anymore, it's really of no use anyway.

It's a very common pattern to have a form that is submitted by ajax (a div is updated) and that the user must keep submitting the form until they fill out all of the inputs correctly.

With FCKEditor, I could do this. With CKEditor, I must first search for all editors within the div to be updated and destroy them before updating the div in order to avoid an error.

Even if I try to destroy the old editor that is no longer in the DOM, I get the error i.contentWindow is null.

[Doug Davis]

btw: I'm using firefox 3.6.18

[Alfonso Martínez de Lizarrondo]

test file

[Alfonso Martínez de Lizarrondo]

I've added here the sample file provided for the ticket that you have mentioned, adjusting it to use the current code instead of a fixed old version.

It's working fine for me with both IE8 and Firefox 5, so it's either a problem with some customization that you have done, or you'll need to provide a better testcase to reproduce the problem.

Also, you should try with an updated version of Firefox

[Doug Davis]

Ajax example added w/3 files.

1. ajax_main.html - The main file to view
2. ajax_div.html - The div to be replaced
3. prototype.js - My project uses the prototype library.

ajax_main.html does an ajax update using the prototype library to replace a div with the contents of ajax_div.html. I used the ckeditor php library to create the ckeditor code.

It doesn't work regardless of if I try to destroy the existing editor first.

What causes this error?

I will try to come up with an example that uses your ie_destroy_bug file.

[Doug Davis]

The problem was I didn't specify the noUpdate parameter on destroy. Since the textarea and ckeditor were removed from DOM, there was nothing to update.

So, this is not an error. Ignore.

[Alfonso Martínez de Lizarrondo]

Updated test

[Alfonso Martínez de Lizarrondo]

Proposed patch

[Alfonso Martínez de Lizarrondo]

Although it's possible to avoid the problem by calling .destroy(true), I think that the patch is easy enough that it can avoid a headache for other people that doesn't realize about the noUpdate parameter.

[Garry Yao]

Tcs provided by Alfonso are added with [7186:7187].

2) If the editor has been removed from the DOM already, and a new editor with the same name gets created?

Guess we'd address this feature request also?

[Alfonso Martínez de Lizarrondo]

Updated test file

[Alfonso Martínez de Lizarrondo]

New patch

[Alfonso Martínez de Lizarrondo]

The test file now includes also that situation. The new patch alters ckeditor.js to handle previous instances that aren't in the DOM

[Garry Yao]

Tc updated with [7189], new simplified patch for review.

[Alfonso Martínez de Lizarrondo]

That patch doesn't look a simplification to me.

It extends the Element prototype with an isOffline method that isn't used for anything else, I don't understand also why adding a public method on the editor instance checkDetached() can be interesting. If the API is bigger, it costs more time to the people to find out the interesting things instead of focusing just on the properties and methods that they are meant to use.

With the patch the theme.destroy isn't called if the editor is detached, but I think that it might be more prone to create memory leaks if we don't try to destroy everything that we can, also now the order of theme.destroy and this.fire('destroy') have been switched, so it needs to be tested that now everything keeps working the same way.

Finally, I would strongly suggest to switch the silent throw of an error with a clear alert because most of the people aren't aware of the javascript console, and so they keep asking questions that are clearly due to trying to create twice an instance and they don't realize of the error.

[Garry Yao]

element::isOffline would be a necessary add-on which I've encountered several other patches where it would be reused. Regard "checkDetached", I can move it private if u like.

Memory leaks protection are hooked to the "destroy" event which still get fired if I'm correct.

In which form does the error throw is out of scope of this ticket, though I can explain the main benefits of not using alert is the possibility of catching this error from developer code.

[Frederico Caldeira Knabben]

​http://ckeditor.t/tt/8226/1.html

[Alfonso Martínez de Lizarrondo]

The theme.destroy is performing two calls to clearCustomData, so unless you are absolutely sure that skipping that won't create a memory leak I see no reason to skip that call. It's just as simple as leaving the code as is.

And although theorically rising an error is interesting for developers that use error handling, the fact is that you can easily read the forums and see how many people ignore the existence of the error console and they ask why their second call to replace doesn't work. If the had an alert in front of them they could have found the problem sooner. For those developers that care about the errors, it's better to avoid them by checking that there's no editor instance with the desired name.

[Garry Yao]

The theme.destroy is performing two calls to clearCustomData, so unless you are absolutely sure that skipping that won't create a memory leak I see no reason to skip that call. It's just as simple as leaving the code as is.

Ok, I've moved the cleanup procedures onto the "destroy" event.

I understand your concern, so let's continue it with another ticket probably.

[Alfonso Martínez de Lizarrondo]

Updated patch

[Alfonso Martínez de Lizarrondo]

New test case

[Alfonso Martínez de Lizarrondo]

The 8226_4.patch is missing the changes to the wysiwygarea plugin from the first patch, so it fails the testcase.

The new testcase shows how the other changes might have unintended consequences: Now the noUpdate parameter isn't used and the contents of the textarea are changed always if it remains in the DOM.

The new patch does just the minimum set of changes to address this ticket and add the isOffline method.

[Alfonso Martínez de Lizarrondo]

Moved the throw vs alert discussion to #8260

[Garry Yao]

The 8226_4.patch is missing the changes to the wysiwygarea plugin from the first patch, so it fails the testcase.

WFM with the ​tc. I'm removing it as I seen that part isn't necessary anymore.

The new testcase shows how the other changes might have unintended consequences...

My fault, so it's enough to just address the mistake instead of rejecting other parts of the patch.

just the minimum set of changes to address this ticket and add the isOffline method...

Ok...sometimes the minimum wasn't the best fit, considering that following call stacks that cause the problem (and where you're trying to fix):

• editor.updateElement() -> ... -> editor.getData()
• theme.destroy -> ...

It's a matter of being a bit bullet-proof here, we can easily have others who don't aware of this pitfall when adding code to the above methods later, e.g. accessing an invalid element without defensive check.

[Frederico Caldeira Knabben]

Replying to garry.yao:

element::isOffline would be a necessary add-on which I've encountered several other patches where it would be reused. Regard "checkDetached", I can move it private if u like.

Despite the "possible" usage of these public APIs, we should not have them introduced if they're not "effectively" used. If you want to support the inclusion of them, then we need a dedicated ticket to change all of the code that will in fact benefit of it. Until them, that's just talk with no results.