Error thrown when dropping a block widget right after itself

[Piotrek Koszuliński]

1. Open ​http://tests.ckeditor.dev:1030/tests/plugins/clipboard/manual/draganddrop
2. Start dragging the image widget.
3. Drop it right below itself.

An error is thrown: Uncaught TypeError: Cannot read property 'getParent' of nullrange.js:1905 CKEDITOR.dom.range.setStartBeforerange.js:923 CKEDITOR.dom.range.moveToBookmarkplugin.js:1405 (anonymous function)

[Piotrek Koszuliński]

Reproduced on Chrome after #13042.

[Piotrek Koszuliński]

DUP reported #13211.

[Artur Delura]

The problem is simply because dragRange is the same as dropRange. During the internal D&D we create a bookmark for dragRange and then clear dragRange contents to move it somewhere else. When contents is removed also bookmark is (unintentionally). Then we try to relate to bookmark which doesn't exists already.

To fix this issue we can check whether dragRange and dropRange are the same. If so we don't have to do nothing, because it means that user want to drop content in the same place.

The question is whether it's okay that dropRange is not collapsed. If it's wrong then we should fix it as well, or maybe just this.

[Artur Delura]

With fresh mind I found out that everything is fine with ranges. But still true is that bookmark is removed and then tried to be used.

This ​comment is very incisive. The problem here is following:

Step 0: Beginning:

<div>
    <widget></widget>
</div>

Step 1: Creating drag boomark (this code made mess with range offsets - see next bookmark creation)

<div>
    <bookmark drag start>
    <widget></widget>
    <bookmark drag end>
</div>

Step 2: Creating drop boomark

<div>
    <bookmark drag start>
    <widget></widget>
    <bookmark drop />
    <bookmark drag end>
</div>

Drop bookmark should be after drag end bookmark but it's in wrong position because adding <bookmark drag start> reorganised offset (++)

---

If we switch order of step first and the second we will fix this issue, but creates a new one the other way around (drag not right after but right before).

<div>
    <bookmark drag start>
    <bookmark drop />
    <widget></widget>
    <bookmark drag end>
</div>

We could introduce some mechanism for updating selection on bookmark creation but it might be painfull and time consuming. What I suggest instead is to right before starting internal drag, check whether there is something to do with it i.e. whether it will change something.

We can simply do it by comparing (drag end with drop end) or (drap start with drop start). If they are the same we don't have to do nothing.

[Artur Delura]

Internal function internalDrop creates a widget on drop. I can't cancel it's execution, because it restores a widget which is destroyed on drag event.

I end up with solution "If we switch order of step first and the second we will fix this issue" because in current algorithm there is such checks which change order of creating bookmarks, but it didn't catch case described in this issue.

Changes and tests in ​branch:t/13140.

[Piotrek Koszuliński]

I force pushed ​branch:t/13140 with some fixes in comments. The change makes sense, but the tests not.

• 'test isRangeBefore 4' - this one is weird. The comment does not match the ranges that are set. And the test pass even though it shouldn't.
• 'test isRangeBefore same position (#13140)' - the name of this test is misleading - these ranges are not at the same position - they are adjacent.
• You don't need to collapse range before setting its end container (you do this in the second test).
• There's no automated test for the whole TC.

[Artur Delura]

I renamed isRangeBefore function to isDropRangeAffectedByDragRange. Also I created entirely new file to test this function. Tests are a bit complicated, and not only check whether mentioned above returns proper value, but also executes internalDrop function. I call this function to check whether nothing fails.

Unfortunatelly even if nothing fails, this method doesn't always works well. In some cases marked with ? characted it loss some parts of drop range during internal drop. It's quite questionable whether we should work on this issues, because it likely not reproducible in a real world.

I'm asking for review tests which I've written because I won't write any fix code before knowing how it should exactly work.

Current changes in ​branch:t/13140b.

[Piotrek Koszuliński]

I rebased and force pushed ​branch:t/13140b.

[Artur Delura]

Okay, so I rewritten our famous function, so now all tests pass. Changes in the same ​branch:t/13140b.

[Piotrek Koszuliński]

I rebased and cleaned up the branch.

[Piotrek Koszuliński]

There was still some work left to be done, because tests weren't passing on IE8-9, but I managed to finish this by myself. Please check my changes though.

Fixed on major with ​git:36b41b2.