Ticket #4767 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

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

Reported by: wwalc Owned by: alfonsoml
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

4767.patch (645 bytes) - added by m.nguyen 4 years ago.
The patch for all browser
4767_2.patch (2.6 KB) - added by m.nguyen 4 years ago.
Remove loadPending method
loadscript.zip (979 bytes) - added by m.nguyen 4 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 alfonsoml 4 years ago.
samples of different ways to load CKEditor
4767_3.patch (4.8 KB) - added by alfonsoml 4 years ago.
Proposed patch
4767_4.patch (2.6 KB) - added by alfonsoml 4 years ago.
Revised patch
4767_5.patch (1.6 KB) - added by alfonsoml 4 years ago.
Revised patch

Change History

comment:1 Changed 4 years ago by fredck

  • Milestone set to 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 4 years ago by fredck

  • Summary changed from CKEditor does not work when ckeditor_cource.js is loaded in the <body> tag to CKEditor does not work when ckeditor_source.js is loaded in the <body> tag

Changed 4 years ago by m.nguyen

The patch for all browser

comment:3 Changed 4 years ago by m.nguyen

  • Keywords Review? added

comment:4 Changed 4 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 4 years ago by m.nguyen

Remove loadPending method

Changed 4 years ago by m.nguyen

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

comment:5 Changed 4 years ago by m.nguyen

  • Status changed from new to assigned
  • Keywords Review? added; Review- removed
  • Owner set to m.nguyen

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 4 years ago by m.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 4 years ago by fredck

  • 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 4 years ago by m.nguyen

  • Status changed from assigned to new
  • Owner changed from m.nguyen to new

Changed 4 years ago by alfonsoml

samples of different ways to load CKEditor

Changed 4 years ago by alfonsoml

Proposed patch

comment:9 Changed 4 years ago by alfonsoml

  • Owner changed from new to alfonsoml
  • Keywords Review? added; Review- removed
  • Status changed from new to assigned

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 4 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 4 years ago by alfonsoml

Revised patch

comment:11 Changed 4 years ago by alfonsoml

  • 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 4 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 4 years ago by alfonsoml

Revised patch

comment:13 Changed 4 years ago by alfonsoml

Ok, the new patch just addresses the current ticket.

comment:14 Changed 4 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:15 Changed 4 years ago by alfonsoml

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

Fixed with [5106]

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