Ticket #8226 (review_failed New Feature)

Opened 3 years ago

Last modified 3 years ago

Allow destroy after object removed from DOM

Reported by: douglassdavis 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

ie_destroy_bug.html (2.2 KB) - added by alfonsoml 3 years ago.
test file
ajax_main.html (1.5 KB) - added by douglassdavis 3 years ago.
ajax_div.html (1.5 KB) - added by douglassdavis 3 years ago.
prototype.js (137.0 KB) - added by douglassdavis 3 years ago.
ie_destroy_bug.2.html (2.2 KB) - added by alfonsoml 3 years ago.
Updated test
8226.patch (959 bytes) - added by alfonsoml 3 years ago.
Proposed patch
destroy_bug.html (3.1 KB) - added by alfonsoml 3 years ago.
Updated test file
8226_2.patch (1.9 KB) - added by alfonsoml 3 years ago.
New patch
8226_3.patch (3.0 KB) - added by garry.yao 3 years ago.
8226_4.patch (3.4 KB) - added by garry.yao 3 years ago.
8226_5.patch (2.1 KB) - added by alfonsoml 3 years ago.
Updated patch
destroy_test.html (1.1 KB) - added by alfonsoml 3 years ago.
New test case
8226_6.patch (3.4 KB) - added by garry.yao 3 years ago.

Change History

comment:1 Changed 3 years ago by douglassdavis

btw: I'm using firefox 3.6.18

Changed 3 years ago by alfonsoml

test file

comment:2 Changed 3 years ago by alfonsoml

  • Status changed from new to pending

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 3 years ago by douglassdavis

Changed 3 years ago by douglassdavis

Changed 3 years ago by douglassdavis

comment:3 Changed 3 years ago by douglassdavis

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 3 years ago by douglassdavis

  • Cc ddavis@… added

comment:5 Changed 3 years ago by douglassdavis

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 3 years ago by douglassdavis

  • Status changed from pending to closed
  • Resolution set to invalid

Changed 3 years ago by alfonsoml

Updated test

Changed 3 years ago by alfonsoml

Proposed patch

comment:7 Changed 3 years ago by alfonsoml

  • Status changed from closed to reopened
  • Resolution invalid deleted

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 3 years ago by alfonsoml

  • Status changed from reopened to review
  • Owner set to alfonsoml

comment:9 Changed 3 years ago by garry.yao

  • Status changed from review to review_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 3 years ago by alfonsoml

Updated test file

Changed 3 years ago by alfonsoml

New patch

comment:10 Changed 3 years ago by alfonsoml

  • Status changed from review_failed to review

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 3 years ago by garry.yao

comment:11 Changed 3 years ago by garry.yao

  • Owner changed from alfonsoml to garry.yao

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

comment:12 Changed 3 years ago by alfonsoml

  • Status changed from review to review_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 follow-up: ↓ 21 Changed 3 years ago by garry.yao

  • Status changed from review_failed to review

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:15 Changed 3 years ago by alfonsoml

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 3 years ago by garry.yao

comment:16 Changed 3 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 3 years ago by alfonsoml

Updated patch

Changed 3 years ago by alfonsoml

New test case

comment:17 Changed 3 years ago by alfonsoml

  • Status changed from review to assigned
  • Owner changed from garry.yao to alfonsoml

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 3 years ago by alfonsoml

  • Status changed from assigned to review

comment:19 Changed 3 years ago by alfonsoml

Moved the throw vs alert discussion to #8260

Changed 3 years ago by garry.yao

comment:20 Changed 3 years ago by garry.yao

  • Owner changed from alfonsoml 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 3 years ago by fredck

  • Status changed from review to review_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 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy