Opened 7 years ago

Closed 7 years ago

#5162 closed Bug (fixed)

Error in ajax sample

Reported by: damo Owned by: fredck
Priority: Normal Milestone: CKEditor 3.2
Component: General Version: SVN (CKEditor) - OLD
Keywords: IBM Confirmed Cc: JoeK


To reproduce:

  1. Open Ajax sample on nightly build
  2. Add some content and destroy the editor
  3. Create the editor

the following error occurs in FF

this.document.getWindow().$ is undefined
[Break on this error] if(m)return;;m=e.setTimeout(...urn s.type;var t=2,u=this.getNative();\r\n

in IE the data on the editor is not set.

Attachments (2)

5162.patch (1.7 KB) - added by alfonsoml 7 years ago.
Proposed patch
5162_2.patch (1.3 KB) - added by fredck 7 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by fredck

  • Cc JoeK added; JOek removed
  • Keywords Confirmed added

Issue introduced with [5015].

comment:2 Changed 7 years ago by alfonsoml

  • Keywords Review? added
  • Owner set to alfonsoml
  • Status changed from new to assigned

I don't know if this should be fixed at the core (or how to fix it properly in an easy way).

The issue in Firefox seems to be that the call to setData the second time is done between the initialization of the instance and so it reaches a point in the code where the inner frame is loaded, but not finished with its initialization (or something weird along those lines, the document has null as its defaultView)

So in order to fix it, it's enough to do the setData call using the instanceReady event:

	editor = CKEDITOR.appendTo( 'editor' , {  on:{'instanceReady':function(e) { e.editor.setData( html )} } });

Besides that check, the first time that setData is called in this kind of environments, the Undo button shouldn't be available, so the call should be changed to

	editor.setData( html, null, false);

Which also fixes the problem (without using the event) because it no longer tries to create the initial snapshot.

So the correct code for the example should be

	editor = CKEDITOR.appendTo( 'editor' , {  on:{'instanceReady':function(e) { e.editor.setData( html, null, false )} } });

And that should always work.

The problem in IE is fixed by that code, but it was pointing to a little problem that can be fixed in the getSnapshot function: it returns false if the event doesn't return anything and the element has been appended, so it should be protected like the getData function.

Finally, maybe we could add a third parameter to CKEDITOR.appendTo allowing to specify the initial value for those instances, if that's OK it should be handled in a future ticket.

Another approach would be to internally delay any call to setData if the editor isn't initialized, but that seems more complex in my view (it will require a field to store the state of the editor and then adjust the behavior of setData according to that field with the instanceReady event)

I'm proposing a patch that just changes the sample and does the protection in getSnapshot, but that doesn't mean that there can be other better ideas.

Changed 7 years ago by alfonsoml

Proposed patch

comment:3 Changed 7 years ago by fredck

  • Owner changed from alfonsoml to fredck
  • Status changed from assigned to new

You analysis is correct Alfonso, but the proposed solution could be a bit better, without making developers life harder.

I'll be proposing another patch for it.

comment:4 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

Changed 7 years ago by fredck

comment:5 Changed 7 years ago by fredck

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

With the patch, the undo snapshot saving fails silently if there is no source of snapshot available, which is the basic problem here.

comment:6 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

The FF error is still there with this patch. I'm working on something else now.

comment:7 Changed 7 years ago by fredck

  • Keywords Review- removed
  • Resolution set to fixed
  • Status changed from assigned to closed

To not impact on the testing phase, I've fixed it directly with [5144]. Please feed free to reopen the ticket if the changes are not correct.

comment:8 Changed 7 years ago by damo

  • Resolution fixed deleted
  • Status changed from closed to reopened

This is still reproducible on IE.

comment:9 Changed 7 years ago by fredck

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

WFM after [5144]. In fact it breaks for me here (5143), but not here (5144).

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