Opened 7 years ago

Closed 7 years ago

#5559 closed Bug (fixed)

setData affected by iframe loading

Reported by: garry.yao Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: 3.2
Keywords: Confirmed IE Review+ Cc: damo, joek

Description

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.

Attachments (5)

5559.patch (1.5 KB) - added by garry.yao 7 years ago.
5559_2.patch (2.1 KB) - added by garry.yao 7 years ago.
5559_3.patch (2.7 KB) - added by garry.yao 7 years ago.
5559_4.patch (2.0 KB) - added by garry.yao 7 years ago.
5559_5.patch (2.1 KB) - added by garry.yao 7 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 7 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from new to assigned
  • Version set to 3.2

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.

Changed 7 years ago by garry.yao

comment:2 Changed 7 years ago by garry.yao

  • Keywords Review? added

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

comment:3 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

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.

Changed 7 years ago by garry.yao

comment:4 follow-up: Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

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

comment:5 in reply to: ↑ 4 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

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.

Changed 7 years ago by garry.yao

comment:6 Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

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

comment:7 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed
  • 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?

comment:8 follow-up: Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

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.

Changed 7 years ago by garry.yao

comment:9 in reply to: ↑ 8 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

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

     
    289289                                                if ( iframe )
    290290                                                        iframe.remove();
    291291
    292                                                 frameLoaded = 0;
    293 
    294292                                                var setDataFn = !CKEDITOR.env.gecko && CKEDITOR.tools.addFunction( function( doc )
    295293                                                        {
    296294                                                                CKEDITOR.tools.removeFunction( setDataFn );
     
    322320                                                // With FF, it's better to load the data on iframe.load. (#3894,#4058)
    323321                                                CKEDITOR.env.gecko && iframe.on( 'load', function( ev )
    324322                                                        {
     323                                                                frameLoaded = 1;
    325324                                                                ev.removeListener();
    326325
    327326                                                                var doc = iframe.getFrameDocument().$;
     
    345344                                        // Editing area bootstrap code.
    346345                                        var contentDomReady = function( domWindow )
    347346                                        {
    348                                                 if ( frameLoaded )
     347                                                if ( !frameLoaded )
    349348                                                        return;
    350                                                 frameLoaded = 1;
     349                                                frameLoaded = 0;
    351350
    352351                                                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.

Changed 7 years ago by garry.yao

comment:10 Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

Ok, I got your points.

comment:11 Changed 7 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:12 Changed 7 years ago by garry.yao

  • Cc damo joek added
  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed with [5395].

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