Opened 6 years ago

Last modified 6 years ago

#8226 review_failed New Feature

Allow destroy after object removed from DOM

Reported by: Doug Davis Owned by: Garry Yao
Priority: Normal Milestone:
Component: General Version: 3.6.1
Keywords: Cc: ddavis@…

Description

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.

Attachments (13)

ie_destroy_bug.html (2.2 KB) - added by Alfonso Martínez de Lizarrondo 6 years ago.
test file
ajax_main.html (1.5 KB) - added by Doug Davis 6 years ago.
ajax_div.html (1.5 KB) - added by Doug Davis 6 years ago.
prototype.js (137.0 KB) - added by Doug Davis 6 years ago.
ie_destroy_bug.2.html (2.2 KB) - added by Alfonso Martínez de Lizarrondo 6 years ago.
Updated test
8226.patch (959 bytes) - added by Alfonso Martínez de Lizarrondo 6 years ago.
Proposed patch
destroy_bug.html (3.1 KB) - added by Alfonso Martínez de Lizarrondo 6 years ago.
Updated test file
8226_2.patch (1.9 KB) - added by Alfonso Martínez de Lizarrondo 6 years ago.
New patch
8226_3.patch (3.0 KB) - added by Garry Yao 6 years ago.
8226_4.patch (3.4 KB) - added by Garry Yao 6 years ago.
8226_5.patch (2.1 KB) - added by Alfonso Martínez de Lizarrondo 6 years ago.
Updated patch
destroy_test.html (1.1 KB) - added by Alfonso Martínez de Lizarrondo 6 years ago.
New test case
8226_6.patch (3.4 KB) - added by Garry Yao 6 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 6 years ago by Doug Davis

btw: I'm using firefox 3.6.18

Changed 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: ie_destroy_bug.html added

test file

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

Status: newpending

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

Changed 6 years ago by Doug Davis

Attachment: ajax_main.html added

Changed 6 years ago by Doug Davis

Attachment: ajax_div.html added

Changed 6 years ago by Doug Davis

Attachment: prototype.js added

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

comment:4 Changed 6 years ago by Doug Davis

Cc: ddavis@… added

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

comment:6 Changed 6 years ago by Doug Davis

Resolution: invalid
Status: pendingclosed

Changed 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: ie_destroy_bug.2.html added

Updated test

Changed 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: 8226.patch added

Proposed patch

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

Resolution: invalid
Status: closedreopened

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.

comment:8 Changed 6 years ago by Alfonso Martínez de Lizarrondo

Owner: set to Alfonso Martínez de Lizarrondo
Status: reopenedreview

comment:9 Changed 6 years ago by Garry Yao

Status: reviewreview_failed

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?

Changed 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: destroy_bug.html added

Updated test file

Changed 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: 8226_2.patch added

New patch

comment:10 Changed 6 years ago by Alfonso Martínez de Lizarrondo

Status: review_failedreview

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

Changed 6 years ago by Garry Yao

Attachment: 8226_3.patch added

comment:11 Changed 6 years ago by Garry Yao

Owner: changed from Alfonso Martínez de Lizarrondo to Garry Yao

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

comment:12 Changed 6 years ago by Alfonso Martínez de Lizarrondo

Status: reviewreview_failed

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.

comment:13 Changed 6 years ago by Garry Yao

Status: review_failedreview

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.

comment:14 Changed 6 years ago by Frederico Caldeira Knabben

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

Changed 6 years ago by Garry Yao

Attachment: 8226_4.patch added

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

Changed 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: 8226_5.patch added

Updated patch

Changed 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: destroy_test.html added

New test case

comment:17 Changed 6 years ago by Alfonso Martínez de Lizarrondo

Owner: changed from Garry Yao to Alfonso Martínez de Lizarrondo
Status: reviewassigned

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.

comment:18 Changed 6 years ago by Alfonso Martínez de Lizarrondo

Status: assignedreview

comment:19 Changed 6 years ago by Alfonso Martínez de Lizarrondo

Moved the throw vs alert discussion to #8260

Changed 6 years ago by Garry Yao

Attachment: 8226_6.patch added

comment:20 Changed 6 years ago by Garry Yao

Owner: changed from Alfonso Martínez de Lizarrondo to 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.

comment:21 in reply to:  13 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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.

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