Ticket #7153 (closed Bug: fixed)

Opened 4 years ago

Last modified 3 years ago

Can't dynamically load ckeditor_source.js

Reported by: damo Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.2
Component: General Version: 3.0
Keywords: IBM Cc: satya, jamescun, tmonahan

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

7153.patch (840 bytes) - added by fredck 3 years ago.
7153.html (2.8 KB) - added by garry.yao 3 years ago.
7153_2.patch (1.1 KB) - added by garry.yao 3 years ago.

Change History

comment:1 Changed 4 years ago by krst

  • Keywords Haspatch? added

Changed 3 years ago by fredck

comment:2 Changed 3 years ago by fredck

  • Keywords Haspatch? removed
  • Owner set to fredck
  • Status changed from new to review

Changed 3 years ago by garry.yao

comment:3 Changed 3 years ago by garry.yao

  • Cc changed from satya,jamescun,tmonahan to satya, jamescun, tmonahan
  • Status changed from review to review_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 3 years ago by garry.yao (previous) (diff)

comment:4 Changed 3 years ago by fredck

  • Status changed from review_failed to new
  • Owner fredck deleted

@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 3 years ago by fredck

  • Status changed from new to pending

comment:6 Changed 3 years ago by damo

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

comment:7 Changed 3 years ago by fredck

  • Status changed from pending to review
  • Owner set to fredck

comment:8 Changed 3 years ago by garry.yao

  • Owner changed from fredck 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 3 years ago by garry.yao

comment:9 Changed 3 years ago by garry.yao

comment:10 Changed 3 years ago by fredck

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

comment:11 Changed 3 years ago by garry.yao

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

comment:12 Changed 3 years ago by garry.yao

  • Status changed from review to review_passed
  • Version set to 3.0
  • Milestone set to CKEditor 3.6.2

comment:13 Changed 3 years ago by garry.yao

  • Status changed from review_passed to closed
  • Resolution set to fixed

Fixed with [7228].

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