Opened 14 years ago

Closed 14 years ago

#5441 closed Bug (fixed)

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

Reported by: Marcos Wright Kuhns Owned by: Alfonso Martínez de Lizarrondo
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 Marcos Wright Kuhns 14 years ago.
An html file that can be used to reproduce the error in IE
ie_destroy_bug.patch (702 bytes) - added by Marcos Wright Kuhns 14 years ago.
ie_destroy_bug.2.patch (1.1 KB) - added by Marcos Wright Kuhns 14 years ago.
A better patch for this bug that also stops unexpected window scrolling.
5441.patch (1.8 KB) - added by Garry Yao 14 years ago.
5441_2.patch (2.4 KB) - added by Alfonso Martínez de Lizarrondo 14 years ago.
Updated patch

Download all attachments as: .zip

Change History (14)

Changed 14 years ago by Marcos Wright Kuhns

Attachment: ie_destroy_bug.html added

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

Changed 14 years ago by Marcos Wright Kuhns

Attachment: ie_destroy_bug.patch added

Changed 14 years ago by Marcos Wright Kuhns

Attachment: ie_destroy_bug.2.patch added

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

comment:1 Changed 14 years ago by Alfonso Martínez de Lizarrondo

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 14 years ago by Marcos Wright Kuhns

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 14 years ago by Frederico Caldeira Knabben

Keywords: Confirmed added; Pending removed
Milestone: CKEditor 3.4

It makes sense.

Changed 14 years ago by Garry Yao

Attachment: 5441.patch added

comment:4 Changed 14 years ago by Garry Yao

Keywords: Review? added
Owner: set to Garry Yao
Status: newassigned

comment:5 Changed 14 years ago by Frederico Caldeira Knabben

#5727 may be fixed by this one.

Changed 14 years ago by Alfonso Martínez de Lizarrondo

Attachment: 5441_2.patch added

Updated patch

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

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 14 years ago by Alfonso Martínez de Lizarrondo

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

comment:8 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:9 Changed 14 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: newclosed

Fixed with [5681]

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy