[IE8] Anchors are duplicated when DnD

[Piotrek Koszuliński]

After #12806:

1. Open replacebycode sample.
2. Drag anchor.
3. Now you have 2 anchors.

[Jakub Ś]

Reproducible only in IE8

[Artur Delura]

According to my current research I found out, that drag bookmark is created wrongly, it's placed in offset -1:

<p>Broa dcasting and <a/></p>
       ^

Actual result:

<P>
    Broa
    <SPAN style="DISPLAY: none" id=cke_bm_28C data-cke-bookmark="1">&nbsp;</SPAN>
    <SPAN style="DISPLAY: none" id=cke_bm_27S data-cke-bookmark="1">&nbsp;</SPAN>
    dcasting and
    <SPAN style="DISPLAY: none" id=cke_bm_27E data-cke-bookmark="1">&nbsp;</SPAN>
    <IMG class=cke_anchor title=Anchor alt=Anchor src="data:image/gif;base64,R0lGODlhAQABAPABAP///wAAACH5BAEKAAAALAAAAAABAAEAAAICRAEAOw==" data-cke-real-element-type="anchor" data-cke-real-node-type="1" data-cke-realelement="%3Ca%20id%3D%22quotes%22%20data-cke-saved-name%3D%22quotes%22%20name%3D%22quotes%22%3E%3C%2Fa%3E">
</P>

Desired result:

<P>
    Broa
    <SPAN style="DISPLAY: none" id=cke_bm_28C data-cke-bookmark="1">&nbsp;</SPAN>
    dcasting and
    <SPAN style="DISPLAY: none" id=cke_bm_27S data-cke-bookmark="1">&nbsp;</SPAN>
    <IMG class=cke_anchor title=Anchor alt=Anchor src="data:image/gif;base64,R0lGODlhAQABAPABAP///wAAACH5BAEKAAAALAAAAAABAAEAAAICRAEAOw==" data-cke-real-element-type="anchor" data-cke-real-node-type="1" data-cke-realelement="%3Ca%20id%3D%22quotes%22%20data-cke-saved-name%3D%22quotes%22%20name%3D%22quotes%22%3E%3C%2Fa%3E">
    <SPAN style="DISPLAY: none" id=cke_bm_27E data-cke-bookmark="1">&nbsp;</SPAN>
</P>

[Artur Delura]

The problem is described ​here. Text nodes are split on drop by browser (IE8 and IE9). Drag range offsets are broken as well if drag and drop range are on the same level. Function mentioned above does nothing to do with drag range. To fix this issue, we should fix drag range as well there.

[Artur Delura]

Function mentioned above does nothing to do with drag range.

And it's fine because it's purpose is to restore original document stage which was before drop (merge two text node into one).

What I did is to made function fixSplitNodesAfterDrop work only when it is supposed to work. Changes in ​branch:t/13011.

[Piotrek Koszuliński]

I think you wrote the initial conditions in a indirect way. They are blurring the outline because they don't get straight to the point. Additionally, hack like this should be targeted on browsers which need it.

So:

1. Restore the env checks.
2. Rewrite the algorithm in a more direct way:
   • Check container of the drop range. If it's not an element, return.
   • Get element containers of the drag range.
   • Compare them - drop container must equal one of drag range containers. If it doesn't - return.
   • Do the current childCountVary check.
   • Do the fix.

With this straightforward outline you won't need these ASCII comments in the body of this method. The checks are very simpler - they just need to be written in that way.

Also, for a function like this I think the tests are insufficient.

[Piotrek Koszuliński]

Actually you can improve this algorithm even further.

1. Check container of the drop range. If it's not an element, return.
2. Get start element container for the drag range.
3. Check if it equals drop range container and if so, if the child count changed.
4. If so, apply the fix.
5. Repeat 2-4 for the end container of the drag range.

This reduces the ambiguity of childCountVary check.

[Piotrek Koszuliński]

PS. I pushed ​branch:t/13011.

[Artur Delura]

Additionally, hack like this should be targeted on browsers which need it.

You mean that it's insecure to open such fix for all browsers?

[Piotrek Koszuliński]

You mean that it's insecure to open such fix for all browsers?

Not necessarily that it's insecure, but rather that it's unnecessary and may lead to issues (then it becomes insecure :D). Unless there's any evidence that any other browser also modifies the DOM, it's better to keep the fix for the affected browsers only.

[Artur Delura]

Didn't you forgot about checking relative containers in your algorithm? It will break also when drag range is in text node. See areDragAndDropContainersRelative variable.

[Piotrek Koszuliński]

"Get element containers of the drag range."

Note where I put the emphasis. On the "element". We are not interested in text nodes at all. If any of the container is a text node, get its parent element ASAP. This is one of the steps that will simplify this algorithm.

[Artur Delura]

Changes and tests in ​branch:t/13011.

[Piotrek Koszuliński]

​http://tests.ckeditor.dev:1030/tests/plugins/clipboard/isdroprangeaffectedbydragrange bleeds on IE8.

[Piotrek Koszuliński]

Another thing:

var dragEndElement = dragRange.endContainer.type != CKEDITOR.NODE_ELEMENT ? dragRange.endContainer.getParent() : dragRange.endContainer;

->

var dragEndConainer = dragRange.endContainer;

if ( dragEndContainer.type == CKEDITOR.NODE_TEXT )
     dragEndContainer = dragEndContainer.getParent();

Plus, you've got a code repetition there.

[Artur Delura]

Replying to Reinmar:

​http://tests.ckeditor.dev:1030/tests/plugins/clipboard/isdroprangeaffectedbydragrange bleeds on IE8.

Function fixSplitNodesAfterDrop blindly fixed two text nodes even when preDragEndContainerChildCount and preDragStartContainerChildCount was not provided.

I added extra check in function mentioned above which prevent from fixing merging two text nodes at drop position.

if (
	typeof preDragEndContainerChildCount != 'number' ||
	typeof preDragStartContainerChildCount != 'number'
) {
	return;
}

The question is whether function should behave like that if that variables are not provided. Or maybe we should update tests to pass this variables.

[Artur Delura]

So we ended up with adding such check because it makes algorithm safer. Changes and tests in ​branch:t/13011.

[Piotrek Koszuliński]

I force-pushed a rebased ​branch:t/13011.

Now the code and tests look good, but I sam many errors when testing DnD on IE8 manually. Try to DnD some text (there may need to be elements inside) before itself and an error is thrown in range.setStartBefore(). The callstack is as follows:

• range.setStartBefore()
• range.moveToBookmark()
• dropRange.moveToBookmark( dropBookmark ); in internalDrop()

[Artur Delura]

Changes in the same ​branch:t/13011.

[Piotrek Koszuliński]

Fixed on major with ​git:4870adb.