Opened 5 years ago

Closed 5 years ago

#12172 closed New Feature (fixed)

Integration inline widgets with new drag and drop

Reported by: Piotr Jasiun Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: General Version:
Keywords: Cc:

Description (last modified by Jakub Ś)

Part of #11437.

After implementing custom support for D&D (#11460), drag and drop for inline widgets stopped working. It should be brought back and be based on our custom D&D (it should use dataTransfer object, and editor events #12169).

Change History (14)

comment:1 Changed 5 years ago by Jakub Ś

Description: modified (diff)
Status: newconfirmed

comment:2 Changed 5 years ago by Piotr Jasiun

Milestone: CKEditor 4.5.0

comment:3 Changed 5 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:4 Changed 5 years ago by Piotr Jasiun

Status: assignedreview

This ticket turned out to be good test for the new paste/drop API. The conclusion is that it seems to be possible to handle widgets with this drag and drop implementation and the big part on code can be removed from widgets plugin or simplified. But also some changes in the drag and drop implementation was needed:

  • getData/setData needs to handle value 0 and not confused it with false,
  • not only on IE10+ but also on old IEs data type URL does not work properly; URL data type does not works in the same strange case that IE use set text on drop (dragging image in contenteditable=false in contenteditable=true); we have to use Text data on all IEs,
  • native setData needs to be surrounded by try-catch because Firefox throws exceptions when you try to setData on drop.

Some widget tests needs to be modified because on changes but dev code is ready for review.

Version 0, edited 5 years ago by Piotr Jasiun (next)

comment:5 Changed 5 years ago by Piotr Jasiun

Type: BugNew Feature

comment:6 Changed 5 years ago by Piotr Jasiun

Widget tests are modified too, so ticket is ready for full review.

comment:7 Changed 5 years ago by Piotr Jasiun

I rebased branch on the newest version of #12173 and moved changes from benders-ckeditor into ckeditor-dev. Now all changes are in ckeditor t/12172.

comment:8 Changed 5 years ago by Piotr Jasiun

I rebased branch on the newest version of #12173.

comment:9 Changed 5 years ago by Piotr Jasiun

Rebased on major.

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

Status: reviewreview_failed

I pushed some commits to the branch.

  1. Undo manager integration broken on Chrome. Drag widget twice. Press CTRL+Z twice. Actual: widget did not return to its original location.
  2. Undo manager integration broken - drag widget once (don't focus it before). Press CTRL+Z. Caret goes to the beginning of editor (Chrome, FF) or stays at drop position (IEs). Expected: widget should be focused.
  3. On dragstart widget's downcasted version should be passed to dataTransfer, not its id. If you don't want to work on this right now please report a ticket.
  4. After the change of clipboardIdDataType tests started failing on IE8 and IE9. I've got 22 failing cases in http://tests.ckeditor.dev:1030/tests/plugins/clipboard/drop. Also there's an outdated code expecting that this value may still equal "URL".
  5. On IE editor isn't focused after drop from external source.

Now, the only blocker is point 4. Points 1, 2 and 3 can be handled in separate ticket. Point 5 is something that I know we talked about but I don't remember conclusions.

comment:11 in reply to:  10 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

Replying to Reinmar:

I pushed some commits to the branch.

  1. Undo manager integration broken on Chrome. Drag widget twice. Press CTRL+Z twice. Actual: widget did not return to its original location.
  2. Undo manager integration broken - drag widget once (don't focus it before). Press CTRL+Z. Caret goes to the beginning of editor (Chrome, FF) or stays at drop position (IEs). Expected: widget should be focused.

Fixed with widget.focus() on dragstart.

  1. On dragstart widget's downcasted version should be passed to dataTransfer, not its id. If you don't want to work on this right now please report a ticket.

I created a separate ticket: #12392.

  1. After the change of clipboardIdDataType tests started failing on IE8 and IE9. I've got 22 failing cases in http://tests.ckeditor.dev:1030/tests/plugins/clipboard/drop. Also there's an outdated code expecting that this value may still equal "URL".

Tests fixed. Outdated code removed.

  1. On IE editor isn't focused after drop from external source.

When I checked it editor is focused but cursors is hidden. Anyway I believe this problem is not related not widget drag and drop. I do not think that this problem is worth spending time on it, but if you think so it should be reported in the separate ticket.

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

Status: reviewreview_failed

Great. But where's the branch? branch:t/12172 ends with my commits.

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

comment:13 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

Sorry, I forgot to push it.

comment:14 Changed 5 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

It's ok now - tests pass, no more problems with undo manager. Merged to major with git:2ab5c72.

I found one difference this branch introduced - it's now possible to dnd widgets between editors. This is wrong at this stage, when we move the internal form of widget, but it will be ok as soon as we'll start downcasting it, because downcasted version can be properly filtered and upcasted by the new editor.

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