Opened 3 years ago

Closed 3 years ago

#13140 closed Bug (fixed)

Error thrown when dropping a block widget right after itself

Reported by: Piotrek Koszuliński Owned by: Artur Delura
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.5.0
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description

  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)

Change History (16)

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

Status: newconfirmed

Reproduced on Chrome after #13042.

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

Priority: NormalHigh

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

DUP reported #13211.

comment:4 Changed 3 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

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

Last edited 3 years ago by Artur Delura (previous) (diff)

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

Last edited 3 years ago by Artur Delura (previous) (diff)

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

Last edited 3 years ago by Artur Delura (previous) (diff)

comment:8 Changed 3 years ago by Artur Delura

Status: assignedreview

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

Status: reviewreview_failed

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.

comment:10 Changed 3 years ago by Artur Delura

Status: review_failedassigned

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

Last edited 3 years ago by Artur Delura (previous) (diff)

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

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

comment:13 Changed 3 years ago by Artur Delura

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

comment:14 Changed 3 years ago by Artur Delura

Status: assignedreview

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

I rebased and cleaned up the branch.

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

Resolution: fixed
Status: reviewclosed

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.

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