Error is thrown and widget is lost when DnD if it's the only content of editor

[Piotrek Koszuliński]

1. Open the placeholder sample.
2. Clear all contents.
3. Insert one placeholder.
4. Try to DnD it.

An error is thrown and the placeholder disappears.

Reproduced on Chrome and FF.

[Artur Delura]

It might be a duplicate of #13453.

[Artur Delura]

This is mine :D

[Artur Delura]

So the problem is directly in extractHtmlFromRange method. If we got following content in the editable and selection:

<p>[<span>foo</span>]<span data-cke-bookmark="1">bar</span></p>

So range is anchored in <p> element with offsets 0 and 1. If we call editable.extractHtmlFromRange( range, 1), we end up with removed bookmark span.

The funny fact is that element is removed only when it has data-cke-bookmark attribute. If we change it to something different everything works well.

Update 1 As far as I can see extract method creates a bookmark which adjoin our bookmark span, so at the end it's removed accidentally.

Update 2 ​here we whole paragraph is removed.

[Artur Delura]

So problem is in check[Start|End]OfBlock in range. It checks whether start or end block is empty. Method checks also bookmark containers and treat it as an empty ones. In use case listed above, there is only one bookmark within <p> element.

The problem is that this bookmark was created previously and shouldnt be taken into consideration.

Process look as follow:

1. D&D occurs so two bookmark are being created - drag and drop.
2. Then extractHtmlFromRange is called and within this function another bookmark is created.
3. While extracting - bookmarks are also removed.

But should be removed only this ones which was created in extractHtmlFromRange method. In our use case drop bookmark is removed as well.

check[Start|End]OfBlock method is not that clever to distinguish bookmarks which should be removed from that ones which shouldn't be.

[Artur Delura]

I commented out ​bookmark checking and run all tests and everything pass. So the question is for what purpose we remove bookmarks?

[Artur Delura]

Changes in ​branch:t/13465. Solution is simple, extractHtmlFromRange doesn't remove bookmarks. We got yet another one possible solutions here, but I'm not sure. We could call method extractHtmlFromRange without second parameter which is responsible for removing empty block.

[Piotrek Koszuliński]

I pushed one additional test (manual one) to ​branch:t/13465.

The idea what behaviour must be changed was good. Unfortunately R-, because it's extractHtmlFromRanger what should be fixed, not checkEnd/StartOfBlock. A change like the one proposed could blow up half of the editor, because you don't know where that method is used.

[Tade0]

I cherry picked to a new branch the commits that create the tests and will be adding my work on top of that.

[Tade0]

I tested dragging the widget before itself and by the time ​https://github.com/cksource/ckeditor-dev/blob/t/13465/core/editable.js#L747 is reached the bookmark is already gone.

Given this changing the code so that the empty block isn't removed solves only the problem with dragging the widget after itself.

Searching now for a solution that handles dragging both before and after.

[Tade0]

I found an interesting quirk. This is how the DOM may look at ​https://github.com/cksource/ckeditor-dev/blob/t/13465/core/editable.js#L723 when dragging the widget before itself:

<p>

<span data-cke-bookmark="1" style="display: none;">&nbsp;</span>

<span data-cke-bookmark="1" style="display: none;">&nbsp;</span>

<span tabindex="-1" contenteditable="false" data-cke-widget-wrapper="1" data-cke-filter="off" class="cke_widget_wrapper cke_widget_inline cke_widget_focused cke_widget_selected" data-cke-display-name="placeholder" data-cke-widget-id="0">

<span data-cke-bookmark="1" id="cke_bm_29C" style="display: none;">&nbsp;</span>

<span class="cke_placeholder cke_widget_element" data-cke-widget-keep-attr="0" data-widget="placeholder" data-cke-widget-data="%7B%22name%22%3A%22foobar%22%2C%22classes%22%3Anull%7D">foobar?</span>

<span class="cke_reset cke_widget_drag_handler_container" style="top: -12px; left: 0px; display: block; background: url(​http://tests.ckeditor.dev:1030/apps/ckeditor/plugins/widget/images/handle.png) rgba(220, 220, 220, 0.498039);">

<img class="cke_reset cke_widget_drag_handler" data-cke-widget-drag-handler="1" src="data:image/gif;base64,R0lGODlhAQABAPABAP///wAAACH5BAEKAAAALAAAAAABAAEAAAICRAEAOw==" width="15" title="Click and drag to move" height="15" draggable="true">

</span>

</span>

<span data-cke-bookmark="1" style="display: none;">&nbsp;</span>

</p>

Note the bookmark inside the widget wrapper.

[Tade0]

In conclusion this seems to be two separate problems:

1. Sometimes the drop bookmark lands in a block that is considered "empty" and both those elements are removed.
2. In other situations the bookmark lands inside the widget (like in comment:14).

It's possible to solve 1. by preserving the children of the removed block.

2. requires further investigation since normally it should be impossible to drop anything inside an element, that has contenteditable="false".

[Tade0]

This line: ​https://github.com/cksource/ckeditor-dev/blob/t/13465/plugins/clipboard/plugin.js#L1740 causes problem 2.

It's a native method, so we have little control over where it places the drop range.

[Piotrek Koszuliński]

So I think we have to simply abort if something is wrong. As soon as possible we should verify the ranges - that e.g. drop range is outside of drag range.

[Piotrek Koszuliński]

PS. Turns out that #13453 is about dropRange being found inside dragRange. Therefore, let's focus here on the first problem - extractHtmlFromRange removing blocks with bookmarks.

[Tade0]

Solved problem 1. by checking if the block that is to be removed contains bookmarks (or, actually, <span> elements).

Changes pushed to ​branch:t/13465a.

[Piotrek Koszuliński]

I rebased ​branch:t/13465a and pushed two more commits there.

There are three problems:

1. Some tests fail in ​http://tests.ckeditor.dev:1030/tests/core/editable/getextracthtmlfromrange - fixed in the second commit.
2. You can use path.block.getElementsByTag() - fixed in the second commit.
3. The bookmarks check must be more precise (must look for data-cke-bookmark) - added test which shows this.

Some hints:

• element.data( 'cke-bookmar' )
• CKEDITOR.dom.walker.bookmark
• use walker or iterate over the returned spans. The latter will be simpler.

[Tade0]

I went with iterating over the spans eventually.

Tests are green on Chrome, Firefox, IE11, Edge and IE8.

Squashed my commit with yours, changes pushed to ​branch:t/13465b so as to not remove your changes completely from the history.

[Piotrek Koszuliński]

Looks quite ok. The last thing will be extracting that loop into a separate private function so it's called only when all other conditions are met and does not bloat its current surrounding.

[Tade0]

Changes pushed to ​branch:t/13465b.

[Olek Nowodziński]

I rebased ​branch:t/13465b on latest master and pushed a commit to save a few bytes in core.

Still, an error is thrown once dropped the widget before itself in IE11 (​http://tests.ckeditor.dev:1030/tests/tickets/13465/1).

PS. Same on Edge.

[Tade0]

Have you checked whether it's because of the problem described in comment:14 and #13453?

[Olek Nowodziński]

Replying to Tade0:

Have you checked whether it's because of the problem described in comment:14 and #13453?

Nope :)

[Piotrek Koszuliński]

Nope, it isn't caused by #13453 or nope, you haven't checked?

[Olek Nowodziński]

Replying to Reinmar:

Nope, it isn't caused by #13453 or nope, you haven't checked?

Haven't checked.

[Tade0]

Replying to a.nowodzinski:

Haven't checked.

Could you verify that the error message is caused by the problem from #13453?

[Piotrek Koszuliński]

Fixed on master with ​git:b32d85d. The only issue I could find now is the ability to drop content outside of body in IE11 (or something similar). I'll report a separate ticket for this.

[Piotrek Koszuliński]

Reported the followup - #13640.