Opened 7 years ago

Closed 6 years ago

#5441 closed Bug (fixed)

Fix for #3812 breaks CKEditor in IE when editor instance is no longer in DOM

Reported by: metavida Owned by: alfonsoml
Priority: Normal Milestone: CKEditor 3.4
Component: General Version: 3.2
Keywords: Confirmed Review+ Cc:

Description

The IE-specific fix for #3812 causes another bug if the CKEditor container isn't a descendant of document.body. This bug only occurs in IE (because we use the moveToElementText function).

Attached is a test HTML file as well as a potential patch. The main shortcoming of the patch is that it does scroll to the top of the page. If someone has a better fix that would be great!

Attachments (5)

ie_destroy_bug.html (4.3 KB) - added by metavida 7 years ago.
An html file that can be used to reproduce the error in IE
ie_destroy_bug.patch (702 bytes) - added by metavida 7 years ago.
ie_destroy_bug.2.patch (1.1 KB) - added by metavida 7 years ago.
A better patch for this bug that also stops unexpected window scrolling.
5441.patch (1.8 KB) - added by garry.yao 7 years ago.
5441_2.patch (2.4 KB) - added by alfonsoml 7 years ago.
Updated patch

Download all attachments as: .zip

Change History (14)

Changed 7 years ago by metavida

An html file that can be used to reproduce the error in IE

Changed 7 years ago by metavida

Changed 7 years ago by metavida

A better patch for this bug that also stops unexpected window scrolling.

comment:1 Changed 7 years ago by alfonsoml

  • Keywords Pending added

What's the use case for allowing CKEditor to be moved out of the document DOM?

Whenever it's moved in the DOM the inner iframe will reload in Firefox, Safari and I think also Opera, so it's just better to destroy the editor and then recreate a new instance at the new location because that's in fact the only option that works.

comment:2 Changed 7 years ago by metavida

The test case I attached is completely contrived. The actual use-case where we're encountering the bug is this:
1) We load the CKEditor into a modal dialog box using AJAX. So the editorInstance.container.ancestors() == [form#dialog_form, div#modal_dialog, body, html]
2) When the user clicks the "Save" button in the dialog box, we save the CKEditor data via an AJAX POST, then remove the modal dialog from the DOM using the removeChild function. Now the editorInstance.container.ancestors() == [form#dialog_form, div#modal_dialog]
3) Finally, we have a cleanup function that removes old CKEditor instances. This function calls editor.destroy() on all CKEDITOR.instances that don't have document.body as an ancestor. This is where we encounter the error.

I realize that we could work around this error by calling the editor.destroy() function before we remove the modal dialog from the DOM, but because we use this pattern many different places in app, that would be a fairly significant change & require us to touch many points in our codebase. Also, beyond our specific case, I don't think it's a safe to assume that the CKEditor will always belong to the DOM when the destroy function is called.

Does this make sense?

comment:3 Changed 7 years ago by fredck

  • Keywords Confirmed added; Pending removed
  • Milestone set to CKEditor 3.4

It makes sense.

Changed 7 years ago by garry.yao

comment:4 Changed 7 years ago by garry.yao

  • Keywords Review? added
  • Owner set to garry.yao
  • Status changed from new to assigned

comment:5 Changed 7 years ago by fredck

#5727 may be fixed by this one.

Changed 7 years ago by alfonsoml

Updated patch

comment:6 Changed 7 years ago by alfonsoml

That solution is very good and fixes the original problem, but the patch was outdated and more important: now the elementsPath gives an error for all the browsers, the revised patch fixes also that issue.

comment:7 Changed 6 years ago by alfonsoml

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

comment:8 Changed 6 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:9 Changed 6 years ago by alfonsoml

  • Resolution set to fixed
  • Status changed from new to closed

Fixed with [5681]

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