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

[wginolas]

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().

[Piotrek Koszuliński]

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.

[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.

[Piotrek Koszuliński]

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

[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;
	},

[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.

[Szymon Cofalik]

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

Another idea would be to return String(data).

[Piotrek Koszuliński]

I don't think so :P

String(undefined)
"undefined"

PS. tests are missing

[Szymon Cofalik]

Test pushed to ​branch:t/13385.

[Szymon Cofalik]

Rebased ​branch:t/13385 with major.

[Piotrek Koszuliński]

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.

[wginolas]

Thank you very much!