Cross-editor D&D of inline widget ends up in error/artifacts

[Szymon Cofalik]

Cross-editor D&D of widgets is not supported. It should fail silently but instead we get errors.

1. Open ​http://tests.ckeditor.dev:1030/tests/plugins/clipboard/manual/draganddrop
2. Without focusing editor, try to D&D inline image from one editor to another.

Expected result: Nothing happened.

Actual result: Error in console or mess/artifacts pasted like data:image/gif;base64,R0lGODlhAQABAPABAP///wAAACH5BAEKAAAALAAAAAABAAEAAAICRAEAOw==s

[Piotrek Koszuliński]

I want to stress this:

Expected result: Nothing happened.

We should not work on support for DnD of widgets between editors in a minor release.

[Szymon Kupś]

In IE10 when dragging inline widget from inline editor to classic editor:

Unable to get property 'document' of undefined or null reference

[Tade0]

I found that the drag range wasn't validated in any way so if the user didn't touch the editor and started dragging an inline widget then there was no selection and consequently no drag range.

Also, if the widget is dropped non-internally the event isn't canceled, so other listeners could catch it despite the fact that no dropping should be done.

My idea is to cancel the drag event if there is no drag range and cancel the drop event if the user drops an inline widget non-internally.

Changes pushed to ​branch:t/13599.

[Piotrek Koszuliński]

First of all:

• wrong code style (don't you have hooks installed?)
• no tests (it needs manual and automated ones)

Second of all, this may be only a part of solution. I wonder if we shouldn't select widget on mouse down (fired on the drag handle) before the data transfer is created. That would make sure that a selection exists so everything else would work. This is much better solution than just aborting everything. However, I'm not actually able to reproduce such issue manually (the widget is selected), what makes a question – are you correct about saying that selection does not exist when we try to get drag range.

[Tade0]

Implemented your solution, works well but I have a problem with the tests. There's a line that specifically says, that an inline widget should not be focused on mouse down of the handler: ​https://github.com/cksource/ckeditor-dev/blob/t/13599/tests/plugins/widget/widgetselection.js#L484

What may be the reason for this?

[Piotrek Koszuliński]

Tough questions. I'll dive into the repo and try to find out.

[Piotr Jasiun]

I remember that selecting widget on mouse down on handler was the reason of some problems, AFAIR on some versions of IE. Too bad there is no more details. If it works for you feel free to remove this test, but take care to check all IEs and both internal and cross-editor there.

[Piotr Jasiun]

And if there is a bug on IE note that you can select that widget on any D&D event. Note that dragstart and dragend will be fired on the source editor. Also having source editor, widget id, and dragged element you should be able to remove dragged widget manually on the drop.

[Tade0]

It seems that the widget gets focused anyway, but the test I mentioned earlier fails to check that.

Still, there's no selection because the sequence of events is as follows:

1. ​https://github.com/cksource/ckeditor-dev/blob/t/13599/plugins/clipboard/plugin.js#L1288
2. ​https://github.com/cksource/ckeditor-dev/blob/t/13599/plugins/widget/plugin.js#L2356

Normally I would just give the second event the higher priority, but it happens that the second event depends on the first(the dataTransfer object used by 2. is initialized in 1.), so I figured that I just split 2. in two and place the code responsible for focusing in a listener with a higher priority.

You can see how this idea looks in ​branch:t/13599a.

This is a proposition. Let me know, if this is the right direction.

[Piotr Jasiun]

In general splitting dragstart event listener makes sens. But it is not widget listener which should be split, but clipboard listener. It should only initDragDataTransfer with the high priority as it do now (2), but save dragRange, dragStartContainerChildCount and dragEndContainerChildCount with the low priority (100) so the selection can be fixed in the meantime (what widgets do, but this may not be the only case).

Also note that you can not call widget.focus() on dragstart without checking if widget exists. It will be called for every D&D, not only widgets D&D, so you code causes errors when I drag something what is not a widget.

And these are different editors in some cases, or just a funny code: ​https://github.com/cksource/ckeditor-dev/blob/d165d05/plugins/widget/plugin.js#L2366-L2367? ;)

[Piotr Jasiun]

I do not think we have time for it now, so I am moving this ticket to the next release.

[Tade0]

I split the clipboard drag listener and made the widget listener cancel when a widget was dragged between editors because there's a listener in the clipboard that would normally fire after that and cause errors.

Tests include a unit test that check whether the event was indeed cancelled and an annotation in one of the already existing manual tests.

Changes pushed to ​branch:t/13599b.

[Piotr Jasiun]

Simple and beauty. ​git:06362a8

[Piotrek Koszuliński]

It must be priority 2? ​https://github.com/ckeditor/ckeditor-dev/commit/06362a8#diff-781e451115d6e69c22f3631a72c0e4e0L1299 Can't it be something more "round"? :D 1 at least? :D

[Tade0]

I figured there's probably some deep meaning to that number, so I tried not to spoil it. :)

[Piotr Jasiun]

Replying to Reinmar:

It must be priority 2? ​https://github.com/ckeditor/ckeditor-dev/commit/06362a8#diff-781e451115d6e69c22f3631a72c0e4e0L1299 Can't it be something more "round"? :D 1 at least? :D

It is directly after 1 :P ​https://github.com/ckeditor/ckeditor-dev/blob/06362a8715809f23d439986a122b87a37eef2e13/plugins/clipboard/plugin.js#L1285

And do we really want to talk about priorities user in the clipboard plugin:

​https://github.com/ckeditor/ckeditor-dev/blob/06362a8715809f23d439986a122b87a37eef2e13/plugins/clipboard/plugin.js#L254

​https://github.com/ckeditor/ckeditor-dev/blob/06362a8715809f23d439986a122b87a37eef2e13/plugins/clipboard/plugin.js#L254

​https://github.com/ckeditor/ckeditor-dev/blob/06362a8715809f23d439986a122b87a37eef2e13/plugins/clipboard/plugin.js#L1285

​https://github.com/ckeditor/ckeditor-dev/blob/06362a8715809f23d439986a122b87a37eef2e13/plugins/clipboard/plugin.js#L254

​https://github.com/ckeditor/ckeditor-dev/blob/06362a8715809f23d439986a122b87a37eef2e13/plugins/clipboard/plugin.js#L905

​https://github.com/ckeditor/ckeditor-dev/blob/06362a8715809f23d439986a122b87a37eef2e13/plugins/clipboard/plugin.js#L1396

​https://github.com/ckeditor/ckeditor-dev/blob/06362a8715809f23d439986a122b87a37eef2e13/plugins/clipboard/plugin.js#L538