Bug 57185 - drop event isn't fired for contentEditable in edit drag
: drop event isn't fired for contentEditable in edit drag
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 9915 76194 76198
  Show dependency treegraph
 
Reported: 2011-03-27 09:39 PST by
Modified: 2012-02-23 21:57 PST (History)


Attachments
Sample testcase (937 bytes, text/html)
2011-03-27 09:39 PST, Alfonso Martínez de Lizarrondo
no flags Details
work in progress (13.95 KB, patch)
2012-01-12 17:14 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
fixes the bug (14.56 KB, patch)
2012-01-12 21:00 PST, Ryosuke Niwa
abarth: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-03-27 09:39:25 PST
Created an attachment (id=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.
------- Comment #1 From 2012-01-11 12:04:58 PST -------
This is causing us to fail http://samples.msdn.microsoft.com/ietestcenter/html5/dragdrop_harness.htm?url=types_attribute
------- Comment #2 From 2012-01-11 12:07:44 PST -------
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.
------- Comment #3 From 2012-01-11 22:44:43 PST -------
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.
------- Comment #4 From 2012-01-12 15:38:18 PST -------
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.
------- Comment #5 From 2012-01-12 17:14:07 PST -------
Created an attachment (id=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.
------- Comment #6 From 2012-01-12 18:59:40 PST -------
Anne would be someone to talk to re: the spec flaws. :)  You should also consider filing a bug with the w3c explaining your disagreement.
------- Comment #7 From 2012-01-12 21:00:15 PST -------
Created an attachment (id=122366) [details]
fixes the bug
------- Comment #8 From 2012-01-12 21:02:06 PST -------
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.
------- Comment #9 From 2012-01-17 01:35:14 PST -------
(From update of attachment 122366 [details])
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" ?
------- Comment #10 From 2012-01-19 00:21:54 PST -------
(From update of attachment 122366 [details])
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.
------- Comment #11 From 2012-01-19 02:16:42 PST -------
Committed r105396: <http://trac.webkit.org/changeset/105396>
------- Comment #12 From 2012-02-23 21:57:49 PST -------
(In reply to comment #11)
> Committed r105396: <http://trac.webkit.org/changeset/105396>

This caused bug 79443.