setData affected by iframe loading

[Garry Yao]

Environment

IE with browser cache enabled.

Reproducing Procedures

1. Open the 'api' sample page;
2. Click on "Set Editor Contents" to send editor the default content provided;
3. Refresh the page;
4. Modify the default content by adding 'Another' before word 'Test';
5. Click on "Set Editor Contents" again to send the modified content;
   • Actual Result: The document content is the same with the first time's result.

[Garry Yao]

This's a regression of [5159], affected by the way how we load data in IE, actually by using the the "src=javascript:void(...)" approach, sometimes IE will just use a cached content instead of really executing the passed codes.

[Garry Yao]

Avoid loading the data from "src" resolves the problem, the patch has been well tested in all browsers.

[Frederico Caldeira Knabben]

After the patch, with IE8:

1. Load the API sample.
2. Click the "Set Editor Contents" button.
3. Refresh the page.
4. Click the "Set Editor Contents" button again.

A js error is thrown.

[Garry Yao]

I don't understand the reason of caching such as 'html' element, performance boost really?

[Frederico Caldeira Knabben]

Replying to garry.yao:

I don't understand the reason of caching such as 'html' element, performance boost really?

Yes, performance, but nothing to be worried about.

In any case, the new patch doesn't make any difference for me. I still have the problem described on my previous comment.

[Garry Yao]

Changes of the previous patch is for the bug reproduced by skipping Step 3 of the previous TC.
New patch targeting both issues.

[Frederico Caldeira Knabben]

• There is a new "isFrameLoading" variable which is never used.

• The "frameLoaded" variable usage now gets confusing. It's set to 0 to indicate that it's loaded. For clarity, it should be set to 1 inside the load event function, and then reset to 0 inside contentDomReady after a !frameLoaded check.

• I still don't understand the reason why _source/core/dom/document.js is being changed. I've reverted the changes and tried the previous tc without step 3 and it worked well, with no errors. Can you provide us a tc that justified it?

[Garry Yao]

The "frameLoaded" indicate three status:

1. undefined - The actual loading is not started yet. (this's useful to reject invalid invoking from cached iframe content)
2. 0 - Indicate the loading has started and is not yet finished.
3. 1 - Indicate the data is already loaded into frame.

It's turned out the changes to wysiwyg plugin in the 2nd patch also fix the problem that changes to document.js was targeting. To make situation less confused, I've excluded the changes outside the new patch, but the bug could be easily triggered by making changes in the future.

[Frederico Caldeira Knabben]

Replying to garry.yao:

The "frameLoaded" indicate three status

Yes, I understood your logic, but at the code logic instead, the status "undefined" and "1" from your explanation have exactly the same meaning and results, making the tri-state useless.

Just to clarify, what I'm talking about, here you have the relative change:

D:/CKEditor/SVN/CKEditor/trunk/_source/plugins/wysiwygarea/plugin.js

@@ -289 +289 @@
                                                 if ( iframe )
                                                         iframe.remove();
 
-                                                frameLoaded = 0;
-
                                                 var setDataFn = !CKEDITOR.env.gecko && CKEDITOR.tools.addFunction( function( doc )
                                                         {
                                                                 CKEDITOR.tools.removeFunction( setDataFn );
@@ -322 +320 @@
                                                 // With FF, it's better to load the data on iframe.load. (#3894,#4058)
                                                 CKEDITOR.env.gecko && iframe.on( 'load', function( ev )
                                                         {
+                                                                frameLoaded = 1;
                                                                 ev.removeListener();
 
                                                                 var doc = iframe.getFrameDocument().$;
@@ -345 +344 @@
                                         // Editing area bootstrap code.
                                         var contentDomReady = function( domWindow )
                                         {
-                                                if ( frameLoaded )
+                                                if ( !frameLoaded )
                                                         return;
-                                                frameLoaded = 1;
+                                                frameLoaded = 0;
 
                                                 editor.fire( 'ariaWidget', iframe );

For the rest, everything is ok. Even the current patch is acceptable, but the above change may still be considered just to KISS.

[Garry Yao]

Ok, I got your points.

[Garry Yao]

Fixed with [5395].