Opened 8 years ago

Closed 8 years ago

#4767 closed Bug (fixed)

CKEditor does not work when ckeditor_source.js is loaded in the <body> tag

Reported by: Wiktor Walc Owned by: Alfonso Martínez de Lizarrondo
Priority: Normal Milestone: CKEditor 3.2
Component: General Version: SVN (CKEditor) - OLD
Keywords: Confirmed Review+ Cc:

Description

When

<script type="text/javascript" src="ckeditor_source.js"></script>

is placed inside of the <body> tag, CKEditor doesn't load (tested on FF 3.5.5, IE8, Opera 10.. probably doesn't work in every browser).

Error in Firefox:

CKEDITOR.env is undefined
http://example.com/ckeditor_31/_source/core/loader.js
Line 165

Attachments (7)

4767.patch (645 bytes) - added by Minh Nguyen 8 years ago.
The patch for all browser
4767_2.patch (2.6 KB) - added by Minh Nguyen 8 years ago.
Remove loadPending method
loadscript.zip (979 bytes) - added by Minh Nguyen 8 years ago.
This is the demo for different thing between creating a script by using 'document.write' and creating dynamic script tag
load samples.zip (5.2 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
samples of different ways to load CKEditor
4767_3.patch (4.8 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Proposed patch
4767_4.patch (2.6 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Revised patch
4767_5.patch (1.6 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Revised patch

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.2

The problem is that we should use document.write in that case, but the loader is using document.body to check whether to use it or not.

comment:2 Changed 8 years ago by Frederico Caldeira Knabben

Summary: CKEditor does not work when ckeditor_cource.js is loaded in the <body> tagCKEditor does not work when ckeditor_source.js is loaded in the <body> tag

Changed 8 years ago by Minh Nguyen

Attachment: 4767.patch added

The patch for all browser

comment:3 Changed 8 years ago by Minh Nguyen

Keywords: Review? added

comment:4 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

The patch works well for this case, while it brings a significant drawback, by simply force 'document.write' to load script will bring regression when 'ckeditor_source.js' is created dynamically after page has loaded (aka. lazy load), the things should be fixed are instead:

  1. The 'document.body' check;
  2. The incorrect 'CKEDITOR.env' reliance.

Changed 8 years ago by Minh Nguyen

Attachment: 4767_2.patch added

Remove loadPending method

Changed 8 years ago by Minh Nguyen

Attachment: loadscript.zip added

This is the demo for different thing between creating a script by using 'document.write' and creating dynamic script tag

comment:5 Changed 8 years ago by Minh Nguyen

Keywords: Review? added; Review- removed
Owner: set to Minh Nguyen
Status: newassigned

In this case we use 'document.write' to keep the browser having one process to render. When we use other ways, some browsers open a new process to load script. Then we'll have error if access to variables in the exented file has not yet been loaded.

comment:6 Changed 8 years ago by Minh Nguyen

We won't go for loadPending method because it's useless in this case. We need to continue using 'document.write' as we have used in line 23 of ckeditor_source.js before.

comment:7 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

document.write CANNOT be used as the exclusive loading method here. This brings issues, as previously pointed by Garry.

To have a better understanding, please check our docs to understand all the possible loading methods we offer with CKEditor. The document.write approach will work with the "Inline" method only. It'll instead break the page when using the "Deferred", "On Demand" or "Timeout" methods.

To test it, its enough to replace "ckeditor_source.js" with "ckeditor_basic_source.js" in the trunk samples. After the patch, things will get broken.

comment:8 Changed 8 years ago by Minh Nguyen

Owner: changed from Minh Nguyen to new
Status: assignednew

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: load samples.zip added

samples of different ways to load CKEditor

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 4767_3.patch added

Proposed patch

comment:9 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; Review- removed
Owner: changed from new to Alfonso Martínez de Lizarrondo
Status: newassigned

The zip includes 5 different ways of loading the editor with the scripts in the body, the fifth one is the one that requires more changes, as a document.write was used in ckeditor_source and ckeditor_basic_source to load _source/core/loader.js

The check for document.body was done in order to check if the page is fully loaded, in that situation we can't use document.write because it will blank the current document. We can change it checking also for document.readyState=='complete'. In loader.js this means that if it's possible it will try to add the scripts inline, without using timeouts or asynchronous loading. In the ckeditor_source and ckeditor_basic_source it's a must to avoid the document.write

The problem with document.readyState is that it isn't supported in Firefox < 3.6, so this feature won't be available for those versions.

Finally the core/ckeditor_basic.js is there to trigger the call to the function if the window has already been loaded by the time that file is loaded.

Maybe the changes in ckeditor_source and ckeditor_basic_source can be simplified to always use body.appendChild instead of document.write (bringing us one step closer to make CKEditor work in contentType="application/xhtml+xml" #4576) but that should be tested carefully to avoid breaking anything that currently works.

comment:10 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

Great sample pages here to illustrate! while I'm against the 5th one, in which you were even lazy loading the basic codes. According to our current design, I have to say you're adding a new feature with this sample here, since the basic codes (ckeditor_basic.js) is supposed to be always presented in host document, there're two reasons behind this:

  1. It was small enough to be excluded from considering lazy load;
  2. It contains bootstrap APIs like CKEDITOR.replace, which was supposed to immediately available in place (which makes dynamic script load infeasible).


To fix the problem described at the ticket here without targeting the lazy load function states above, your changes to loader.js in the patch is already enough.

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 4767_4.patch added

Revised patch

comment:11 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; Review- removed

I've left in the changes to ckeditor_basic.js as they can be a little protection against situations where the page has finished loading by the time that the file is executed, they might not be easy to reproduce, but it should be a safe check: if the document is finished, then don't try to wait for the onload event because it will never fire.

I'll try to create testcases from the load samples 1-4 as soon as I finish to setup the testing environment.

comment:12 Changed 8 years ago by Garry Yao

as they can be a little protection against situations where the page has finished loading by the time that the file is executed...

I'm OK if you can provide an example, for me that's barely possible.

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 4767_5.patch added

Revised patch

comment:13 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Ok, the new patch just addresses the current ticket.

comment:14 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:15 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: assignedclosed

Fixed with [5106]

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