Cannot drag'n'drop widget if a text caret is placed just after widget instance

[Pawel Pecio]

User is unable to drag and drop widget instance using its handler if a text caret is placed just after the widget (example: some text [Inlinewidget]| rest of the text). On mouse down click there is an exception:

Uncaught IndexSizeError: Failed to execute 'extend' on 'Selection': 1 is larger than the given node's length.

Bug was reported only for Chrome browsers (all the latest versions).

Steps to reproduce:

1. On CKEditor widgets demo page go to the mathematical formulas demo.
2. Select a formula widget instance by click
3. Type right arrow key once to unfocus the widget instance, text caret should appear just after the widget instance
4. Try to drag the widget using drag handler, the exception should be thrown

I've found that the bug is connected with "bookmarks". The "source" of the problem is finalizeNativeDrop function when saveSnapshot event is triggered. One of its handler(s) uses bookmarks and call moveNativeSelectionToBookmark function. Stack trace reports call from instanceReady handler in selection component. There are following code:

			editor.on( 'beforeUndoImage', beforeData );
			editor.on( 'afterUndoImage', afterData );
			editor.on( 'beforeGetData', beforeData, null, null, 0 );
			editor.on( 'getData', afterData );

(... and in afterData function ...)
			if ( fillingChar ) {
				fillingChar.setText( fillingCharBefore );

				if ( selectionBookmark ) {
					moveNativeSelectionToBookmark( editor.document.$, selectionBookmark );
					selectionBookmark = null;
				}
			}

By unknown reason (for me), selectionBookmark is an empty textnode with offset property set to 1. This causes an exception in

sel.extend( bm[ 1 ].node, bm[ 1 ].offset ); 

which cannot move selection to position 1 when the node length is 0.

[Jakub Ś]

I have been able to reproduce this issue from CKEditor 4.4.6 in Blink and Webkit browsers.

[Artur Delura]

There are three related issues: #13389 #13307 #13284

Another one set of steps to reproduce.

1. Open ​http://presets.ckeditor.dev/4.4.8/full-all/ckeditor/samples/plugins/placeholder/placeholder.html
2. Select text "are"
3. Make some placeholder on the selection.
4. Removed first placeholder (Click on it to select, and press backspace/delete)
5. Try D&D placeholder to the left

There is an error in console: Uncaught IndexSizeError: Failed to execute 'extend' on 'Selection': 1 is larger than the given node's length.

[Piotrek Koszuliński]

Let's add this ticket to 4.5.1 so we remember to check all the related issues. Perhaps it is a single issue.

[Szymon Cofalik]

Partial, ugly fix available at ​branch:t/13284. It fixes #13389 and probably #13307. With this fix error doesn't occur but there is other problem which prevents user from dragging, so this is not a full fix.

Below is a description of my struggle with this part of the code:

I tried to debug this weird case and really weird things are happening:

function moveNativeSelectionToBookmark( document, bm ) {
		var sel = document.getSelection(),
			range = document.createRange();

		range.setStart( bm[ 0 ].node, bm[ 0 ].offset );
		range.collapse( true );
		sel.removeAllRanges();
//#1
		sel.addRange( range );
//#2
		sel.extend( bm[ 1 ].node, bm[ 1 ].offset );
	}

Comments added by me.

So in scenario described in this ticked, at #1 bm[ 1 ].node.textContent equals \u200B (ZWS). But after executing sel.addRange( range ) it suddenly becomes empty! It looks like te content was cleared upon addRange(). Since it is empty, extending selection results in error about wrong node's length.

So I decided to comment all lines in the project that has something to do with selectionchange - just to be sure. And the bug still persist.

So I thought this is some kind of a bug in Chrome. I created short script that tries to recreate what is happening in our buggy function:

<div id="myDiv"></div>
<script>
	var d = document.getElementById('myDiv');
	var span = document.createElement('span');
	var tn = document.createTextNode('\u200B');

	span.appendChild(tn);
	d.appendChild(span);

	var sel = document.getSelection();
	var range = document.createRange();

	range.setStart(tn, 1);
	range.collapse(1);

	sel.removeAllRanges();
	sel.addRange(range);

	console.log(tn.textContent == '\u200B');
</script>

But the console showed "true" so ZWS was not replaced. I tried similar code in editor's iframe with no luck.

In order to debug it precisely I set window.xx in mousedown event and then put if (window.xx) debugger; in moveNativeSelectionToBookmark() - so I would stop in correct call (that's because this function is being called dozens of time). I think that you are already predicting what happened... when console made breakpoint everything worked well...

So the only thing I could do is just "ugly" workaround which is available on ​branch:t/13284.

[Szymon Cofalik]

DnD is prevented thanks to moveNativeSelectionToBookmark. To summarize it: after mousedown on a drag handler, in "fillingChar mechanism", the selection is moved to the text node with ZWS. This breaks dragging of the drag handler. When I move the mouse cursor, I select whatever was in bookmarked (ZWS) node.

[Szymon Cofalik]

Pushed some commits to ​branch:t/13284

​git:e191f4be
Fixes drag and drop prevention when selection was on ZWS. I had to prevent moving selection to a fillingChar in an elegant way and this is best solution I came up with. If I clear the selection, "fillingChar" mechanism will not fire.

Does it break UX for a user? Let's assume that the user have selection other than on dragged widget. Then:

• if they just click on drag handler, they focus the widget,
• if they mousedown and click, they focus the widget.

So no matter what the user does, they will "lose" current selection anyway. The only difference might be for snapshots but from my tests it doesn't affect undo manager.

Note: this commit alone fixes bug described in this ticket, but the first commit also fixes a bug (and fixes #13389)

Note: if you know there might be other similar cases in other parts of code, let me know.

Also added test.

[Jonathan]

This looked similar to issue ​#13515 so I grabbed a copy of the fix and tested it to see if that issue was also resolved. It does appear to fix the problem so that the javascript exception does not occur, however, it introduced a minor annoyance where it now returns a \u200b character when calling getData().

[Szymon Cofalik]

Fixes also #13513.

[Szymon Cofalik]

@jbalinski: Could you provide a more expanded description of what exactly you do?

I took a sample provided in attachment from #13513 (the ticket you linked) and checked out to ​branch:t/13284. Then, I paste an image, fire .getData().indexOf('\u200b') and it returns -1 -- seems like it's alright. If you are still experiencing the issue you described, please consider making a new ticket with detailed but minimal scenario and link it in comment to this ticket.

[Jonathan]

I know that I've seen it but I also can't seem to reproduce it anymore. I did find a couple other workflows though when the exception described above is thrown, even with the patch applied. I'm documenting them in ​#13513.

[Piotrek Koszuliński]

Unfortunately I cannot review this ticket and #13513. It's too many changes, too many mentioned TCs, but too few tests supporting them.

The rule (especially important in case of so complex systems) is that if possible (and in this case it must be possible if you were able to reproduce all the problems) each change should have a test. It must be clear why it was made and it must be testable, otherwise I have no way to verify whether it's good and whether it's the only possible way.

For instance, there's ​git:47ba21f103. How am I supposed to verify it? To verify that such case really takes place? I need a MT (or AT if possible, but I don't think it will be possible in this case) where I can see that this specific change fixed something.

The same here: ​git:e614c0a1. It's good that it's a separate commit, because AFAICS it is a follow-up for the previous one, so again, there should be some test which verifies that this code works.

And then we've got ​git:e191f4be. It's absolutely unclear what this code does.

Final note – this and #13513 are very hard tickets and the touched system is very tricky and also very important. All changes must be covered with all possible automated tests and some manual tests for final verification.

PS. It seems that this ticket fixes two things – general issues in the selection system and later DnD of widgets. Consider splitting it into two parts. You could merge #13513 into the general part because I can see you touch the same code.

[Szymon Cofalik]

I've pushed ​branch:t/13284b.

1) I dropped ​​git:47ba21f103 and ​​git:e614c0a1 commits that you have mentioned. As you've written, these were general fixes to the selection mechanism. Since #13513 I think that these are overcomplicated. Solution from #13513 achieves the same.

2) As for ​​git:e191f4be - I described it in one of previous comments. I expanded comment in the code so it is more insightful but in-depth explanation is too long so I will leave it on a trac.

3) Since now this branch fixes only the problem with dragging a widget when cursor is exactly after it, I hope that the single manual test for this case is enough. (I've added some more tests for #13513).

4) I will verify if general fix from #13513 fixes also #13389 and #13307 and take appropriate steps.

[Piotrek Koszuliński]

Another related ticket #12727.

[Piotrek Koszuliński]

Fixed on master with ​git:623ade3.

Thanks for comment:7, because I would need to ask you to perform exactly these steps if you hadn't written it :).