FCKeditorAPI is not cleaned up when removing editor instances manually from the page

[Frederico Caldeira Knabben]

The fix for #183 broken the FCKeditorAPI "Instances" object cleanup. The "_test/manual/fckeditorapi/test1.html" test page shows the problem.

[Martin Kou]

I can't really think of any simple hack to fix this without breaking #183 - which unfortunately is also quite important.

There seems to be no way of knowing whether FCKeditor is running under a browser or an embedded browser control, so we can't just target the onbeforeunload trick to Microsoft's WebBrowser control. There are two alternatives that I can think of, but they may bring problems themselves:

1. Use the topmost window hosting FCKeditorAPI to detect whether an editor iframe who'd raised the unload event is really removed from the DOM tree, with a timeout. If it is not removed, then don't delete it from the FCKeditorAPI.Instances dictionary. Potential problem: Is it possible for editor instances to be removed without removing its iframe element?
2. We define a method for removing FCKeditor instances and ask the web developers to use it in our documentation instead of using element.removeChild($editorIframe). Potential problem: A lot of people don't read the documentation...

Related to point 2. I've found from a support request last week that removing FCKeditor instances by element.removeChild($editorIframe) doesn't work at present, as it will cause JavaScript errors.

[Martin Kou]

Proof-of-concept patch

[Martin Kou]

I've written a proof-of-concept patch for alternative one. It works in IE6 and FF3. The patch looks clumsy but much of the clumsiness is there due to the need to avoid crashing the browser (with C++ errors no less!) while the browser tries to access freed memory in an unloaded frame, due to setTimeout().

The patch can be further simplified but a lot of tests will be needed to make sure it doesn't crash browsers. Also more tests will be needed to see if it works on IE7, Safari and Opera.

[Martin Kou]

Found two more problems with the proof-of-concept code:

1. Opera 9.50 seems to not trigger the unload event when iframes are being removed.
2. It doesn't work in Safari at all.

[Martin Kou]

I guess I should go with the second approach now. As it turns out, even the onunload event in IE is somewhat faulty - it doesn't fire off at the moment the iframe is removed. It seems to fire off only when enough iframes are removed (triggered by GC, perhaps?).

[Alfonso Martínez de Lizarrondo]

check also #550. That problem existed before #183 was fixed

[Martin Kou]

I've coded a RemoveInstance method under FCKeditorAPI that looks like this:

                                'RemoveInstance : function( name )' +
                                '{' +
                                        'var instance = this.Instances[name] ;' +
                                        'delete this.Instances[name] ;' +
                                        'if ( instance && instance.GetInstanceObject )' +
                                        '{' +
                                                'var el = instance.GetInstanceObject( "frameElement" ) ;' +
                                                'el.parentNode.removeChild( el ) ;' +
                                        '}' +   
                                '}' +                   

This satisfies the requirement of the fckeditorapi test if the test case was modified to use FCKeditor.RemoveInstance() instead of plain DOM iframe removal.

However, this still doesn't solve the problem of shared objects (e.g. FCK) being erased away and causing JavaScript errors in multiple editor scenarios. Say, you run FCKeditorAPI.RemoveInstance( 'FCKeditor_1' ) in sample11.html with the above function definition, and click the remaining editing area. You'll get a bunch of JavaScript errors saying some shared objects are no longer defined.

[Martin Kou]

After some discussion with Fred yesterday, it was decided that reversing the fix to #183 would be the best approach for now. For those who really need to view their website with Microsoft's WebBrowser control, a configuration directive MsWebBrowserControlOnUnloadKludge has been added for them to force the old behavior.

[Martin Kou]

Oops... I left an alert() in the patch... let me clean that up first.

[Frederico Caldeira Knabben]

I've tried to apply the patch, but it seems to be broken (no trac preview, for example).

Anyway, we could have a more generic name for the configuration instead of "MsWebBrowserControlOnUnloadKludge". What about "MsWebBrowserControlCompat"?

Also, there is no need of comments in the configuration file. Let's leave it well documented in the docs site.

[Martin Kou]

Fixed with [2095].

Click here for more info about our SVN system.