delete editor.config.elementsPath_filters on editor.distroy based on r5242

[omnishelley]

On destroy function, set items of editor.config.elementsPath_filters to null, but not reset length=0.

So reopening a editor with CKEDITOR.replace, fire the event of "selectionChange" on elementspath/plugin.js, throw a exception.

Repair this bug only needs a statement on 508 line "_source/core/editor.js" :

items = editor.config.elementsPath_filters; for ( index= 0 ; index < items.length ; index++ ){

items[ index ] = null;

}

items.length=0; add by ominshelley

[Alfonso Martínez de Lizarrondo]

Can you provide a test page to reproduce the problem?

Any property of an editor object shouldn't matter after it has been destroyed.

[omnishelley]

add debugger on _source/plugins/font/plugin.js:

onRender : function() {

editor.on( 'selectionChange', function( ev ) {

debugger; ...

}

}

run on _sample/divreplace.html (replace '../ckeditor.js' with '../ckeditor_source.js').

you may find: click part 1 and display ckeditor, select text in the editor, fire the debugger event. but click part 2 and display ckeditor, select text in the editor again, cann't fire the debugger event.

why cann't fire the font's selectionChange event? because it firstly run selectionChange event of elementsPath/plugin.js. on elementsPath/plugin.js, it throw a exception:

for ( var i = 0; i < filters.length; i++ ) {

if ( filters[ i ]( element ) === false ) here, throw a exception.

{

ignore = 1; break;

}

}

[Alfonso Martínez de Lizarrondo]

Ok, the problem here is that the editor.config is created with tools.prototypedCopy from CKEDITOR.config, and when a filter is added to an instance, it ends up also in the global object, and the next time that an editor is created that array isn't empty any longer, it includes the previous data (that in editor.destroy() has been set to null).

So this is an interesting problem that deserves a real fix: if the skins.html sample is loaded, the editor.config.elementsPath_filters contains 3 instances of the same function, one added for each editor.

After fixing it, maybe it's no longer needed to clean the array in the editor.destroy()

[Alfonso Martínez de Lizarrondo]

Proposed patch

[Alfonso Martínez de Lizarrondo]

The patch moves the filters to editor._.elementsPath.filters to avoid having them shared among instances. After that there's no need to clean up anything on editor.destroy().

I've included also a fix for the SCAYT plugin because it fails when an editor is destroyed.

[Garry Yao]

Ok, the problem here is that the editor.config is created with tools.prototypedCopy from CKEDITOR.config

This's an really important catch, it reveals a common weakness of prototype inheritance that it's actually based on shallow copy, what we need here is instead a deep clone of the global configuration object, which guarantees two things:

1. That all config entries could be more than primitive types (e.g. array and hash we're already used);
2. And all of them could be customized by either assignment or modification.

[Alfonso Martínez de Lizarrondo]

Revised patch

[Alfonso Martínez de Lizarrondo]

The patch is based on the code by Garry, but I've removed the code that allowed to set a single function in config.elementsPath_filters and I've included part of the previous cleanup to avoid the use of variables defined as a closure.

[Alfonso Martínez de Lizarrondo]

#5444 has been marked as dup

[Frederico Caldeira Knabben]

It's wrong to change prototypedCopy to clone for the configurations. prototypedCopy makes it possible to extend the configurations of all editors, at any moment in the loading process. By changing it to clone we loose functionality with no added benefit.

[Frederico Caldeira Knabben]

Having elementsPath_filters in the CKEDITOR.config was a bad decision. It would be enough to have an API function for that. Now we have complications.

This is not a kind of configuration thing which users will be using the config file. It's much more a plugin thing, which can be easily handled in the API.

We still have time to revert [5228] and all changes related to elementsPath_filters (like things wrongly included in editor.js), introducing a new "global" function CKEDITOR.plugins.elementspath.addFilter. It doesn't need to be editor specific; it's ok to have it global.

[Alfonso Martínez de Lizarrondo]

What about using then the first proposed patch (that does just that: remove the filters for elementPath from the config) ​http://dev.fckeditor.net/attachment/ticket/5345/5345.patch

[Frederico Caldeira Knabben]

I'm ok for attachment:5345.patch​. We can develop it later to make it a public API function, instead of using the editor._ object directly.

[Alfonso Martínez de Lizarrondo]

Fixed with [5340]