Opened 7 years ago

Closed 6 years ago

#7153 closed Bug (fixed)

Can't dynamically load ckeditor_source.js

Reported by: Damian Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.2
Component: General Version: 3.0
Keywords: IBM Cc: Satya Minnekanti, James Cunningham, Teresa Monahan

Description

ckeditor_source.js uses document.write() when loading, this causes clearing of window variables when called at the end of a document load.

Proposal is that ckeditor_source.js (and ckeditor_basic_source.js) does something like this instead:

var head = document.getElementsByTagNam('head')[0];
var script = document.createElement('script');
script.type = 'text/javascript';
script.src = CKEDITOR.getUrl('_source/core/loader.js');
head.appendChild(script);

Attachments (3)

7153.patch (840 bytes) - added by Frederico Caldeira Knabben 6 years ago.
7153.html (2.8 KB) - added by Garry Yao 6 years ago.
7153_2.patch (1.1 KB) - added by Garry Yao 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by Krzysztof Studnik

Keywords: Haspatch? added

Changed 6 years ago by Frederico Caldeira Knabben

Attachment: 7153.patch added

comment:2 Changed 6 years ago by Frederico Caldeira Knabben

Keywords: Haspatch? removed
Owner: set to Frederico Caldeira Knabben
Status: newreview

Changed 6 years ago by Garry Yao

Attachment: 7153.html added

comment:3 Changed 6 years ago by Garry Yao

Cc: satya,jamescun,tmonahansatya, jamescun, tmonahan
Status: reviewreview_failed

The patch doesn't work for "interactive" document state which is quite a common case when using "DOMContentReady" event or padding body script block illustrated with the sample file.

7153.html is not working in at least FF4/IE8/IE9.

Last edited 6 years ago by Garry Yao (previous) (diff)

comment:4 Changed 6 years ago by Frederico Caldeira Knabben

Owner: Frederico Caldeira Knabben deleted
Status: review_failednew

@garry.yao, I have some doubts about the possibility of supporting your TC. I did some research and I didn't find a good solution that will fit all needs.

The patch makes it possible to load the editor *after* document load. This is a common use case as well.

@damo, you may tell us which kind of support we should have here. Maybe the patch already gives what you want. Let us know. Thanks.

comment:5 Changed 6 years ago by Frederico Caldeira Knabben

Status: newpending

comment:6 Changed 6 years ago by Damian

Thanks Fred... this patch has been verified on our end and looks good.

comment:7 Changed 6 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Status: pendingreview

comment:8 Changed 6 years ago by Garry Yao

Owner: changed from Frederico Caldeira Knabben to Garry Yao

@garry.yao, I have some doubts about the possibility of supporting your TC. I did some research and I didn't find a good solution that will fit all needs.

Can you check if the new patch is enough for resolving this case?

Changed 6 years ago by Garry Yao

Attachment: 7153_2.patch added

comment:9 Changed 6 years ago by Garry Yao

comment:10 Changed 6 years ago by Frederico Caldeira Knabben

I would ask to please reconsider patch 1 as it has already been verified. Additionally, the provided tests are not working.

comment:11 Changed 6 years ago by Garry Yao

R+ for the first patch, the extended tc opens #8285.

comment:12 Changed 6 years ago by Garry Yao

Milestone: CKEditor 3.6.2
Status: reviewreview_passed
Version: 3.0

comment:13 Changed 6 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7228].

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