Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#13011 closed Bug (fixed)

[IE8] Anchors are duplicated when DnD

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: IE8 Cc:

Description

After #12806:

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

Change History (26)

comment:1 Changed 4 years ago by Jakub Ś

Keywords: IE8 added
Status: newconfirmed

Reproducible only in IE8

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

Milestone: CKEditor 4.5.0 BetaCKEditor 4.5.0

comment:3 Changed 4 years ago by Artur Delura

Priority: NormalHigh

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

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

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

comment:6 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:7 in reply to:  5 Changed 4 years ago by 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.

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

comment:8 Changed 4 years ago by Artur Delura

Status: assignedreview

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

Status: reviewreview_failed

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.

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

comment:11 Changed 4 years ago by Piotrek Koszuliński

PS. I pushed branch:t/13011.

comment:12 Changed 4 years ago by 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?

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

comment:14 Changed 4 years ago by Artur Delura

Status: review_failedassigned

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

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

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

comment:17 Changed 4 years ago by Artur Delura

Status: assignedreview

Changes and tests in branch:t/13011.

comment:18 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed

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

comment:20 Changed 4 years ago by Artur Delura

Status: review_failedassigned

comment:21 in reply to:  18 Changed 4 years ago by 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.

comment:22 Changed 4 years ago by Artur Delura

Status: assignedreview

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

comment:23 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed

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()

comment:24 Changed 4 years ago by Artur Delura

Status: review_failedassigned

comment:25 Changed 4 years ago by Artur Delura

Status: assignedreview

Changes in the same branch:t/13011.

comment:26 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Fixed on major with git:4870adb.

Last edited 3 years ago by Anna Tomanek (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy