Opened 5 years ago

Last modified 5 years ago

#9199 confirmed Bug

filebrowser throws element is undefined because fails to correctly check what attachFileBrowser is looking at

Reported by: TomChiverton Owned by:
Priority: Normal Milestone:
Component: File Browser Version: 3.0
Keywords: Cc:

Description

in http://dev.ckeditor.com/browser/CKEditor/trunk/_source/plugins/filebrowser/plugin.js#L249 this

if ( element.type == 'hbox' || element.type == 'vbox' || element.type == 'fieldset' ) 

should be something like

if ( element == undefined ) continue;
if ( element.type == 'hbox' || element.type == 'vbox' || element.type == 'fieldset' ) 

otherwise random apparently unrelated things like the link create/edit dialogue cause a RTE.

Often happens when CKEditor is used as a component in some other visual framework i.e. Ember

Attachments (2)

ckeditor_patch_for_9199.js (367.0 KB) - added by TomChiverton 5 years ago.
Patched minified version of v3.6.4
9199.patch (602 bytes) - added by Jakub Ś 5 years ago.

Download all attachments as: .zip

Change History (10)

Changed 5 years ago by TomChiverton

Attachment: ckeditor_patch_for_9199.js added

Patched minified version of v3.6.4

comment:1 Changed 5 years ago by Jakub Ś

Component: GeneralFile Browser
Status: newconfirmed
Version: 3.6.5 (SVN - trunk)3.0

I'm guessing FileBrowser could use extra check. I’m attaching small patch.

Instead of:

if ( element.type == 'hbox' || element.type == 'vbox' || element.type == 'fieldset' ) 

checking first if element is available at all:

if ( element && ( element.type == 'hbox' || element.type == 'vbox' || element.type == 'fieldset' ) ) 
Last edited 5 years ago by Jakub Ś (previous) (diff)

Changed 5 years ago by Jakub Ś

Attachment: 9199.patch added

comment:2 Changed 5 years ago by caya

Version: 3.04.1

comment:3 Changed 5 years ago by caya

This bug is still present in the current release 4.1 when using ember. The patch above fixes it, please include it!

comment:4 Changed 5 years ago by Jakub Ś

Version: 4.13.0

Version number is used to indicate when problem stated occurring.

@caya don't change version number only to show us that issue is still reproducible in latest CKEditor. We are aware of that.

comment:5 Changed 5 years ago by caya

OK, sorry about that. Another patch is attached to issue #8940. Is there a chance that this will get fixed fast? Is there anything I could help with?

comment:6 Changed 5 years ago by Jakub Ś

@caya you can have these changes implemented today in CKEditor 4.x. What you can do is fork CKEditor 4 code from https://github.com/ckeditor/ckeditor-dev, and apply fixes there. Next you can build your new release from source just follow http://docs.ckeditor.com/#!/guide/dev_build (it is running build.sh from command line).

At the moment of writing you have to modify:
ckeditor-git\plugins\filebrowser\plugin.js, Line:230 (for this ticket)
ckeditor-git\plugins\filebrowser\plugin.js, Line: 227 and Line: 362 (for ticket #8940)

Nevertheless I will ask my colleague if he can include these fixes in next release.

comment:7 Changed 5 years ago by Jakub Ś

@caya please also note that Line:227 seems to be fixed now. You will find there

for ( var i = elements.length; i--; ) {
...

It does exactly the same thing as

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

but it is probably faster as it doesn't count elements.length on every loop.

comment:8 Changed 5 years ago by Piotrek Koszuliński

By coincidence we were working on #10265 just few days ago. It fixed this one case, but unfortunately it does not fix this one: ​https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/filebrowser/plugin.js#L362

Regarding for loop - we're iterating in reverse order not because of performance, but because it's cleaner - less variables, less code. Of course performance might be better to but it doesn't matter in this trivial case.

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