Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#13599 closed Bug (fixed)

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

Reported by: Szymon Cofalik Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.5.4
Component: Core : Pasting Version: 4.5.0
Keywords: Cc:

Description

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

Change History (20)

comment:1 Changed 3 years ago by Olek Nowodziński

Component: GeneralCore : Pasting
Milestone: CKEditor 4.5.3
Status: newconfirmed

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

Version: 4.5.2 (GitHub - master)4.5.0

comment:3 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by Szymon Kupś

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

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

comment:5 Changed 3 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:6 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by Tade0

Status: assignedreview

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

Status: reviewreview_failed

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.

Last edited 3 years ago by Piotrek Koszuliński (previous) (diff)

comment:9 Changed 3 years ago by 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?

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

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

comment:11 Changed 3 years ago by 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.

comment:12 Changed 3 years ago by 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.

comment:13 Changed 3 years ago by Tade0

Status: review_failedreview

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.

comment:14 Changed 3 years ago by Piotr Jasiun

Status: reviewreview_failed

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? ;)

comment:15 Changed 3 years ago by Piotr Jasiun

Milestone: CKEditor 4.5.3CKEditor 4.5.4

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

comment:16 Changed 3 years ago by Tade0

Status: review_failedreview

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.

comment:17 Changed 3 years ago by Piotr Jasiun

Resolution: fixed
Status: reviewclosed

Simple and beauty. git:06362a8

comment:18 Changed 3 years ago by 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

comment:19 Changed 3 years ago by Tade0

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

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