Opened 3 years ago

Closed 3 years ago

#13468 closed Bug (fixed)

[IE] Binding drag and drop dataTransfer does not work if text data was set in the meantime.

Reported by: Piotr Jasiun Owned by: Piotr Jasiun
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.5.2
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description

Check this code: http://jsfiddle.net/oqzy8dog/3/

It does not work on IE, because the only way to store data transfer ID is to use text data type. Since we do not want to change this text we simple do not use generated ID and the ID is the content on the Text data or an empty string (this is just a backup solution, because if dataTransfer is reset on dragend we can simple use the same dataTransfer object until it is reset, without comparing ID):

this.id = this.getData( clipboardIdDataType );

// If there is no ID we need to create it. Different browsers needs different ID.
if ( !this.id )
	if ( clipboardIdDataType == 'Text' ) // IE
		this.id = '';

In this case this.id is an empty string, because it was not set when the dataTransfer constructor was called. Then we set the text and when we set up the dataTransfer object for the second time (on drop) the id became this data.

So then we compare IDs in initDragDataTransfer:

if ( this.dragData && dataTransfer.id == this.dragData.id )

they do not match and new object is created, so custom data we set on dragstart will not be available on drop.

There are two solutions.

Solution A: do not use ID

We could consider not using ID at all on IE so the change would be:

if( clipboardIdDataType != 'Text' ) {
	this.id = this.getData( clipboardIdDataType );	
}

but then all external drops will have the same DataTransfer object if user forget to reset it manually on dragend.

Solution B: update ID

Alternatively we could update ID every time user set the text data. This seems to be a little weird, but can work a little better.

Change History (9)

comment:1 Changed 3 years ago by Piotr Jasiun

Status: newconfirmed

Both solutions pushed to t/13468a and t/13468b. Tests and polishing are missing. I'm for solution b.

comment:2 Changed 3 years ago by Piotr Jasiun

Drag and drop sample in the SDK should be updated after this fix.

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

Milestone: CKEditor 4.5.1
Priority: NormalHigh

This of course must be fixed in 4.5.1.

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

Summary: [IE] Biding drag and drop dataTransfer does not work if text data was set in the meantime.[IE] Binding drag and drop dataTransfer does not work if text data was set in the meantime.

comment:6 Changed 3 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:7 Changed 3 years ago by Piotr Jasiun

Status: assignedreview

I decided to choose solution "B". It is better to use Text data for binding events, because we can avoid some bugs. Using solution "A" if user drag external content and use initDragDataTransfer on dragstart (so do what we do in the SDK sample) and forget to resetDragDataTransfer on dragend (so do what we do in the SDK sample ;) ), but not drop content to the editor (just drag one element, then another) two events would use the same dataTransfer event and they should not. With solution B they will not as long as both element has different dragged Text.

I have added tests, and updated the solution, because it has small issue. Changes in t/13468b.

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

You didn't test it on IE9- ;>

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

Resolution: fixed
Status: reviewclosed

Fixed on master with git:ceb1d47.

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