Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#13385 closed Bug (fixed)

Editor.getSnapshot() should return a string, but it returns a boolean

Reported by: wginolas Owned by: Szymon Cofalik
Priority: Normal Milestone: CKEditor 4.5.2
Component: General Version:
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

If editor is destroyed, editor.getSnapshot() returns a non-string value. Better if it always return a string.

Please read comment:1 and comment:12.

Original TC (we did not confirm it):

We have the following issue, when we combine IE, inline editing and the codesnippt plugin.

  1. Use IE
  2. Insert a Code-Snippet and enter some code.
  3. Do not move the cursor out of the code snippet.
  4. Close the editor, i.e. call ckeditor.destroy().

The following exception is thrown:

 TypeError: Object doesn't support property or method 'replace'
   at Image (http://kartoffel/toro/ckeditor/plugins/undo/plugin.js:788:5)
   at lock (http://kartoffel/toro/ckeditor/plugins/undo/plugin.js:625:7)
   at Anonymous function (http://kartoffel/toro/ckeditor/plugins/undo/plugin.js:170:5)
   at listenerFirer (http://kartoffel/toro/ckeditor/core/event.js:144:6)
   at Anonymous function (http://kartoffel/toro/ckeditor/core/event.js:290:10)
   at fire (http://kartoffel/toro/ckeditor/core/editor_basic.js:24:3)
   at removeHiddenSelectionContainer (http://kartoffel/toro/ckeditor/core/selection.js:265:4)
   at reset (http://kartoffel/toro/ckeditor/core/selection.js:1670:6)
   at unlock (http://kartoffel/toro/ckeditor/core/selection.js:1634:4)
   at unlockSelection (http://kartoffel/toro/ckeditor/core/selection.js:977:4) 

I debugged into the code and noticed, that the editor.getSnapshot() function sometimes returns the boolean value true instead of a string. Especially, when the ckeditor is shutting down currently. undo/plugin.js:788:5 breaks when a boolean is retuned by getSnapshot().

Change History (13)

comment:1 Changed 3 years ago by Piotrek Koszuliński

Status: newconfirmed
Version: 4.4.7

It's true. getSnapshot looks now like this:

	getSnapshot: function() {
		var data = this.fire( 'getSnapshot' );

		if ( typeof data != 'string' ) {
			var element = this.element;
			if ( element && this.elementMode == CKEDITOR.ELEMENT_MODE_REPLACE )
				data = element.is( 'textarea' ) ? element.getValue() : element.getHtml();
		}
		return data;
	},

So if data isn't a string (and when there are no listeners to getSnapshot it is true I think), then getSnapshot() returns a boolean which is wrong.

The harder part here would be to also understand why getSnapshot() is called when editor is (being) destroyed. This doesn't seem right as well, but I guess the answer will not satisfy us, because there will be no feasible solution for that part.

comment:2 Changed 3 years ago by Szymon Cofalik

Owner: set to Szymon Cofalik
Status: confirmedassigned

comment:3 Changed 3 years ago by Szymon Cofalik

I can't reproduce it. I am using full editor with codesnippet plugin and IE (all versions have same result, other browsers too). I use this to destroy instance:

setTimeout(function(){ CKEDITOR.instances.editor1.destroy(); }, 3000);

In those 3 seconds I open code snippet dialog and write some stuff.

The error I got is in CKEDITOR.focusManager.remove: it appears that listeners.blur is undefined (listeners is null). getSnapshot() method is not even reached. Am I doing something wrong?

Log from Chrome:

Uncaught TypeError: Cannot read property 'blur' of null
CKEDITOR.focusManager.remove @ focusmanager.js:238
hideCover @ plugin.js:2140
CKEDITOR.dialog.hide @ plugin.js:1043
(anonymous function) @ plugin.js:3042
listenerFirer @ event.js:144
CKEDITOR.event.fire @ event.js:290
CKEDITOR.tools.extend.destroy @ editor.js:776
(anonymous function) @ VM251:2

I will try to fix the error I am able to reproduce. I also could just make a naive fix so that getSnapshot() always return string.

Last edited 3 years ago by Szymon Cofalik (previous) (diff)

comment:4 Changed 3 years ago by Piotrek Koszuliński

Please focus only on this single error that could be related to getSnapshot(). Other issues should be moved to separate tickets.

comment:5 Changed 3 years ago by Szymon Cofalik

I can't reproduce it. In blind, the only thing I can do is add safe-valve return:

getSnapshot: function() {
		var data = this.fire( 'getSnapshot' );

		if ( typeof data != 'string' ) {
			var element = this.element;
			if ( element && this.elementMode == CKEDITOR.ELEMENT_MODE_REPLACE )
				data = element.is( 'textarea' ) ? element.getValue() : element.getHtml();
			else
				return '';
		}

		return data;
	},

comment:6 Changed 3 years ago by Piotrek Koszuliński

Check this code from the undo plugin:

			var contents = editor.getSnapshot();

			// In IE, we need to remove the expando attributes.
			if ( CKEDITOR.env.ie && contents )
				contents = contents.replace( /\s+data-cke-expando=".*?"/g, '' );

As you can see contents is expected to be a string. Otherwise, error will be thrown. So we can expect that we'll fix some issues by just fixing getSnapshot's return value. This is meant to be a simple fix for an obvious bug in getSnapshot. We can even live without manual test for it. Unit test will be enough.

comment:7 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

Fixed Editor.getSnapshot() so it always returns a string on branch:t/13385

Another idea would be to return String(data).

Last edited 3 years ago by Szymon Cofalik (previous) (diff)

comment:8 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I don't think so :P

String(undefined)
"undefined"

PS. tests are missing

comment:9 Changed 3 years ago by Szymon Cofalik

Test pushed to branch:t/13385.

comment:10 Changed 3 years ago by Szymon Cofalik

Rebased branch:t/13385 with major.

comment:11 Changed 3 years ago by Szymon Cofalik

Status: review_failedreview

comment:12 Changed 3 years ago by Piotrek Koszuliński

Description: modified (diff)
Milestone: CKEditor 4.5.2
Resolution: fixed
Status: reviewclosed
Summary: JavaScriptException in undo/plugin.jsEditor.getSnapshot() should return a string, but it returns a boolean

Fixed on master with git:fafab77.

I was also unable to reproduce the mentioned issue in the mentioned way, but there was definitely a bug in getSnapshot(). Therefore, I'll change the ticket description to match a bug that we fixed. If the bug that @wginals could reproduce will be still reproducible, please open a new ticket.

Please also note that there must also be another bug somewhere because after editor is destroyed nothing should try to get snapshots. However, it would be very hard to make sure that it's always true, because short after editor is destroyed a lot may happen. Therefore, we chose the easier path to make sure that at least there's no error.

comment:13 Changed 3 years ago by wginolas

Thank you very much!

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