Opened 10 years ago

Closed 10 years ago

#5345 closed Bug (fixed)

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

Reported by: omnishelley Owned by: Alfonso Martínez de Lizarrondo
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: SVN (CKEditor) - OLD
Keywords: Confirmed Review+ Cc:

Description

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

Attachments (3)

5345.patch (6.6 KB) - added by Alfonso Martínez de Lizarrondo 10 years ago.
Proposed patch
5345_2.patch (2.6 KB) - added by Garry Yao 10 years ago.
5345_3.patch (4.1 KB) - added by Alfonso Martínez de Lizarrondo 10 years ago.
Revised patch

Download all attachments as: .zip

Change History (16)

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

Keywords: Pending added

Can you provide a test page to reproduce the problem?

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

comment:2 Changed 10 years ago by 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;

}

}

comment:3 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Keywords: Confirmed added; Pending removed
Milestone: CKEditor 3.xCKEditor 3.3

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()

Changed 10 years ago by Alfonso Martínez de Lizarrondo

Attachment: 5345.patch added

Proposed patch

comment:4 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added
Owner: set to Alfonso Martínez de Lizarrondo
Status: newassigned

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.

Changed 10 years ago by Garry Yao

Attachment: 5345_2.patch added

comment:5 Changed 10 years ago by 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.

comment:6 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

Changed 10 years ago by Alfonso Martínez de Lizarrondo

Attachment: 5345_3.patch added

Revised patch

comment:7 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; Review- removed

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.

comment:8 Changed 10 years ago by Alfonso Martínez de Lizarrondo

#5444 has been marked as dup

comment:9 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

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.

comment:10 Changed 10 years ago by 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.

comment:11 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; Review- removed

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

comment:12 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

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.

comment:13 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: assignedclosed

Fixed with [5340]

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