Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#10603 closed Bug (invalid)

With multiple editors, after attaching an image in the first, the second editor is broken

Reported by: Tim Dionne Owned by:
Priority: Normal Milestone:
Component: UI : Dialogs Version: 3.6.2
Keywords: Cc:

Description

We use CKEDITOR with some custom plugins. We have a plugin that replaces the image plugin with our own. When the image button is pressed, our custom dialog is rendered, and along the way, CKEDITOR.dialog() is called. In there, some code is executed depending on whether you have configured it to remove some dialog tabs (in config.removeDialogTabs), which we have done. And in that code, there are 2 for loops that use a non-local variable 'i'. This is causing problems for us because of the way the CKEDITOR.config object is minified. In fact, it is minified to the variable 'i'. So, when this code executes, the config object gets blown away, and reassigned to an integer because of the non-local scope. We often have more than one rich editor on the page, and when this happens, all new editors are hosed because the config objects for them are prototyped from i. Seems fairly dangerous to use non-local variables in this code. I see that most other for loops declare i in the local scope using the 'var' keyword.

I have coded a fix and will attach as an svn diff.

I created the patch from trunk, but it exists in previous versions as well. We are using 3.6.2.

Attachments (1)

ckeditor.patch (834 bytes) - added by Tim Dionne 11 years ago.
Patch file

Download all attachments as: .zip

Change History (5)

Changed 11 years ago by Tim Dionne

Attachment: ckeditor.patch added

Patch file

comment:1 Changed 11 years ago by Piotrek Koszuliński

Resolution: invalid
Status: newclosed

This patch changes nothing. The i variable is defined in the CKEDITOR.dialog function and this covers both loops - this is a local variable. Those loops are placed directly in that function so as JavaScript has function scope, not a block scope it has no difference.

You're writing about minifying - which minifier do you use? None should replace CKEDITOR.config with something else, because this is not a local variable.

PS. You're using very old CKEditor version which is not supported any more.

Last edited 11 years ago by Piotrek Koszuliński (previous) (diff)

comment:2 Changed 11 years ago by Tim Dionne

Thanks for the response Reinmar. We use the packager that is distributed with ckeditor, and do not change ckeditor.pack. I just ran the packager on a fresh trunk checkout, and notice how this line of code in CKEDITOR.editor.prototype._init() (editor.js) gets minified:

Before packaging:

488                         this.config = CKEDITOR.tools.prototypedCopy( CKEDITOR.config );

After packaging (packaged ckeditor.js):

3101           z.config = e.prototypedCopy(i); // z is assigned to this on line 3090

Note that 'i' has been used in place of CKEDITOR.config. The 'i' that is used in the for loop I mentioned, because it is not declared with var, when it is assigned a new value, the runtime looks up the scope chain and finds the config object, and then replaces it. I have stepped through the code and watched it happen.

Last edited 11 years ago by Tim Dionne (previous) (diff)

comment:3 Changed 11 years ago by Jakub Ś

@tdionne, Reinmar has also mentioned one thing - try upgrading to latest CKE 4.x.

Problem should be gone there. We certainly won't be providing fixes for older versions of editor which in fact might be working in newer CKE 4.x or even CKE 3.6.x

comment:4 Changed 11 years ago by Piotrek Koszuliński

@tdionne - I haven't got the possibility to check the old (3.x) builder, but if there really is such a bug (I'm sceptical though), then you definitely should upgrade to CKEditor 4.x. Since CKEditor 4.0 we use Google Closure Compiler instead of custom minifier implementation, so such bugs are very unlikely.

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