Bug 57185 – drop event isn't fired for contentEditable in edit drag

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Alfonso Martínez de Lizarrondo]

Created attachment 87076 [details]
Sample testcase

The only way to catch the onDrop event for contentEditable elements is to listen for ondragover and cancel that event, but in that case the normal behavior of allowing drag&drop is broken.

In https://bugs.webkit.org/show_bug.cgi?id=3582#c5 it's stated that this is the expected behavior by the spec, but that's hardly useful for real usage. Firefox and IE both fire the onDrop event when something is really dropped into the content.

An example of why we want to listen for that event: http://dev.ckeditor.com/ticket/7422 in order to provide Undo/Redo management we must track the changes to the document. Due to this bug, in Webkit we can opt to not cancel the onDragOver event and so the onDrop isn't fired, or cancel it and then the user can't drag anything.

[Eric Seidel (no email)]

This is causing us to fail http://samples.msdn.microsoft.com/ietestcenter/html5/dragdrop_harness.htm?url=types_attribute

[Eric Seidel (no email)]

Appears this also causes us to fail http://samples.msdn.microsoft.com/ietestcenter/html5/dragdrop/setData_dragstart.htm

I suspect most of the IETC DnD tests hit this bug.

[Ryosuke Niwa]

I think we ought to be firing drop event in this case per current HTML5 spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#drag-and-drop-processing-model

Look at the step 2 after the sentence "Otherwise, the drag operation might be a success; run these substeps:"; this step fires drop event and is executed for drag & drop operations within editable regions before DOM mutations are made.

[Ryosuke Niwa]

It turned out that this is a really hard bug to fix :(

The spec requires deletion to happen inside the default event handler of dragend event instead of atomically happening immediately after inserting the dragged content once the drop event is fired. However, inserting the content will "orphan" the original selection and we won't be able to delete the dragged content in some cases; in particular, this happens in the attached case where the dragged content and the destination coincide in the same text node.

Recovering the selection after the fact is particularly challenging because when ReplaceSelectionCommand::doApply is called, the frame's selection is changed to that of the drag caret's (i.e. destination's). Even if we had stored indexForVisiblePosition, we'll need to adjust the index when the drag source appears after the destination.

[Ryosuke Niwa]

Created attachment 122342 [details]
work in progress

This regresses undo after drag & drop. I think the current spec is flawed. It's extremely hard for us to split deletion and insertion of contents during drag and still get all editing behaviors (selection, undo, etc...) right. Given that Firefox 9 does it "atomically" in the dragend event, I'm inclined to copy that behavior instead.

[Eric Seidel (no email)]

Anne would be someone to talk to re: the spec flaws. :)  You should also consider filing a bug with the w3c explaining your disagreement.

[Ryosuke Niwa]

Created attachment 122366 [details]
fixes the bug

[Ryosuke Niwa]

There are so many bugs around this feature and I wasn't able to fix them all but I think this is a good step forward.

[Adam Barth]

Comment on attachment 122366 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=122366&action=review

This was not nearly as scary as it looked at first.  I'm not an expert here.  Please let me know if you'd prefer to have someone more expert in this area review your patch.

> Source/WebCore/page/EventHandler.cpp:1915
> +    bool result = false;

Should this be called "preventedDefault" rather than "result" ?

[Ryosuke Niwa]

Comment on attachment 122366 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=122366&action=review

>> Source/WebCore/page/EventHandler.cpp:1915
>> +    bool result = false;
> 
> Should this be called "preventedDefault" rather than "result" ?

Will fix before landing.

[Ryosuke Niwa]

Committed r105396: <http://trac.webkit.org/changeset/105396>

(In reply to comment #11)
> Committed r105396: <http://trac.webkit.org/changeset/105396>

This caused bug 79443.