Opened 5 years ago

Closed 4 years ago

#11478 closed Bug (fixed)

Can't use jQuery Object as option-property with the jQuery Adapter

Reported by: Encaitar Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.2
Component: General Version: 4.3.2
Keywords: Cc:

Description

You can't use jQuery objects as options when using the adapter:

$( '#editable' ).ckeditor( function() { 
    /* Callback function code. */ 
},
{ 
    uiColor : '#9AB8F3', 
    test: $('body') //Results into  a Uncaught TypeError: Illegal constructor
}); // Use CKEDITOR.inline().

I was upgrading from 3.6 to 4.3 when I got this problem. I used those jQuery objects in my company's plugins. For more info, see: http://stackoverflow.com/questions/21159229/using-own-properties-in-ckeditor-4-plugin-upgrade-from-version-3

In the attached file is a ckeditor_4.3.2_full.zip with one 'bugged' example -> jquery_object.html

Attachments (1)

ckeditor_4.3.2_full.zip (1.2 MB) - added by Encaitar 5 years ago.
CKEditor 4.3.2 full version, with my 'bugged' example (in the example folder)

Download all attachments as: .zip

Change History (11)

Changed 5 years ago by Encaitar

Attachment: ckeditor_4.3.2_full.zip added

CKEditor 4.3.2 full version, with my 'bugged' example (in the example folder)

comment:1 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: newreview

The problem is that in commit https://github.com/ckeditor/ckeditor-dev/commit/aeff33d4c0e346e674d8ba4d1f7c0faeeb021217 was added copy configuration functionality, which simply don't work with jQuery objects. Above functionality use CKEDITOR.tools.clone function. You pass jQuery object which under "0" property contains a value which is HTMLBodyElement. While copying HTMLBodyElement we simply try to construct it which fail with error Illegal constructor.

I proposed solution in branch:t/11478 which is wrapping construction call in try catch.

comment:2 Changed 4 years ago by Fyneworks

A try{}catch{} block will suppress future errors. I propose the alternative "if(obj instanceof jQuery)"

SEE: https://github.com/cksource/ckeditor-dev/pull/2

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

Thanks funeworks!

And my reply:

https://github.com/cksource/ckeditor-dev/pull/2#issuecomment-43179532

We need to do a research what can we clone. I think that now we try to clone simply too much.

comment:4 Changed 4 years ago by Fyneworks

So detecting for jQuery (or any specific object) isn't the solution, you're quite right. I'll watch this thread to see how the story develops. Not a major issue for our installation, we're sticking to an older release for the time being, but curious to see how/if this affects others.

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

Status: reviewreview_failed

I've forgotten to R- the solution proposed in commit:1. Such usage of try-catch will only hide problems rather than fix them.

comment:6 Changed 4 years ago by Artur Delura

Status: review_failedreview

Now we ommit DOM objects and window when copying config. So mentioned above are passed by reference. SOlution proposed in branch:t/11478b

comment:7 Changed 4 years ago by Fyneworks

Looks great! Sorry if this is a silly question but, when .... well, how do I find out when this particular commit has made its way into a stable release?

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

I see that this ticket has not milestone yet, so we'll review it and if it will be good (it looks ok), then it will make it to next minor release (i.e. 4.4.2).

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

Milestone: CKEditor 4.4.2
Status: reviewreview_passed

I rebased branch:t/11478b and pushed additional commit with some small fixes.

comment:10 Changed 4 years ago by Artur Delura

Resolution: fixed
Status: review_passedclosed

Merged to master with git:c41e10c

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