drop event isn't fired for contentEditable in edit drag
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Jan 2012 10:16:35 +0000 (10:16 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Jan 2012 10:16:35 +0000 (10:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=57185

Reviewed by Adam Barth.

Source/WebCore:

Dispatch drop and dragend events after edit drag per HTML5 spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#drag-and-drop-processing-model

There are two major differences between the spec and WebKit's new behavior:

While the spec says we have to insert the dragged contents immediately after dispatching drop event
and delete the source in the default event handler of dragend event, doing so in WebKit is extremely
difficult because of the way we manage the selection. Instead, we continue to delete the source
and insert the dragged contents immediately after the drop event; this behavior matches that of Firefox 9.

When the dragged contents and the destination of the move is in the same text node, ReplaceSelectionCommand
may end up replacing it with a new text node. But this removal causes a problem when EventHandler uses
the node to dispatch dragend event because the node is "orphaned" from its parent at that point. To mitigate
this issue, we update the dragState's m_dragSrc when the node is orphaned by the edit drag. While this behavior
may differ from the spec and other browsers, not delivering dragend to the editing host seems strictly worse than
dispatching it at the slightly wrong target.

Tests: fast/events/moving-text-should-fire-drop-and-dragend-events-2.html
       fast/events/moving-text-should-fire-drop-and-dragend-events.html

* page/DragController.cpp:
(WebCore::DragController::performDrag): Dispatch drop event even when m_isHandlingDrag is true as long
as DragDestinationActionDHTML is an acceptable action.
(WebCore::DragController::concludeEditDrag): Call updateDragStateAfterEditDragIfNeeded after inserting
the dragged contents. This is necessary when ReplaceSelectionCommand or MoveSelectionCommand modifies
the source node while inserting the dragged contents.
* page/EventHandler.cpp:
(WebCore::EventHandler::performDragAndDrop): Clear the drag state only if drop event's default action
was prevented so that we dispatch dragevent event later.
(WebCore::EventHandler::updateDragStateAfterEditDragIfNeeded): Update dragState's m_dragSrc when the node
is orphaned. See above for the rationale.
* page/EventHandler.h:

LayoutTests:

Added tests ensure moving text in contenteditable regions fire dragstart, drop, and dragend events.

* fast/events/moving-text-should-fire-drop-and-dragend-events-2-expected.txt: Added.
* fast/events/moving-text-should-fire-drop-and-dragend-events-2.html: Added.
* fast/events/moving-text-should-fire-drop-and-dragend-events-expected.txt: Added.
* fast/events/moving-text-should-fire-drop-and-dragend-events.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@105396 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2.html [new file with mode: 0644]
LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/DragController.cpp
Source/WebCore/page/EventHandler.cpp
Source/WebCore/page/EventHandler.h

index 6edaef9783ebb59ea9d31d4b1d80de62e1d9d27b..42c7d64fd6e057f6855b32244b8ae72d7f55a17e 100644 (file)
@@ -1,3 +1,17 @@
+2012-01-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        drop event isn't fired for contentEditable in edit drag
+        https://bugs.webkit.org/show_bug.cgi?id=57185
+
+        Reviewed by Adam Barth.
+
+        Added tests ensure moving text in contenteditable regions fire dragstart, drop, and dragend events.
+
+        * fast/events/moving-text-should-fire-drop-and-dragend-events-2-expected.txt: Added.
+        * fast/events/moving-text-should-fire-drop-and-dragend-events-2.html: Added.
+        * fast/events/moving-text-should-fire-drop-and-dragend-events-expected.txt: Added.
+        * fast/events/moving-text-should-fire-drop-and-dragend-events.html: Added.
+
 2012-01-19  Dominic Mazzoni  <dmazzoni@google.com>
 
         Unreviewed - re-enables tests on chromium-mac by making minor
diff --git a/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2-expected.txt b/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2-expected.txt
new file mode 100644 (file)
index 0000000..9787035
--- /dev/null
@@ -0,0 +1,8 @@
+This tests dragging text within the same contenteditable element. To manually test, move the target below to the destination. You should see source:dragstart, destination:drop, and source:dragend in the log.
+
+Log:
+source:dragstart
+destination:drop
+source:dragend
+
+PASS
diff --git a/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2.html b/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2.html
new file mode 100644 (file)
index 0000000..09efc03
--- /dev/null
@@ -0,0 +1,58 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests dragging text within the same contenteditable element. To manually test, move the target below to the destination.
+You should see source:dragstart, destination:drop, and source:dragend in the log.</p>
+<div id="source" contenteditable><span>Drag and drop this <span id="target">target</span> from here</div>
+<div id="destination" contenteditable><span>to here, the_destination.</span></div>
+<pre id="log">
+Log:
+</pre>
+<script>
+
+var log = document.getElementById('log');
+var source = document.getElementById('source');
+var destination = document.getElementById('destination');
+var eventLogs = [];
+
+function attachLogger(element) {
+    var logger = function(event) {
+        var text = element.id + ':' + event.type;
+        eventLogs.push(text);
+        log.innerHTML += text + '\n';
+    }
+    element.addEventListener('dragstart', logger, false);
+    element.addEventListener('dragend', logger, false);
+    element.addEventListener('drop', logger, false);
+}
+
+attachLogger(source);
+attachLogger(destination);
+
+if (window.eventSender) {
+    layoutTestController.dumpAsText();
+
+    source.focus();
+    var target = document.getElementById('target');
+    getSelection().selectAllChildren(target);
+
+    function mouseMoveToCenterOfElement(element) {
+        eventSender.mouseMoveTo(element.offsetLeft + element.offsetWidth / 2, element.offsetTop + element.offsetHeight / 2);
+    }
+
+    eventSender.dragMode = true;
+    mouseMoveToCenterOfElement(target);
+    eventSender.mouseDown();
+    eventSender.leapForward(500);
+    mouseMoveToCenterOfElement(destination);
+    eventSender.mouseUp();
+
+    source.style.display = 'none';
+    destination.style.display = 'none';
+
+    log.innerHTML += '\n' + (eventLogs.join('') == ['source:dragstart', 'destination:drop', 'source:dragend'].join('') ? 'PASS' : 'FAIL');
+}
+
+</script>
+</body>
+</html>
\ No newline at end of file
diff --git a/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-expected.txt b/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-expected.txt
new file mode 100644 (file)
index 0000000..41ae904
--- /dev/null
@@ -0,0 +1,8 @@
+This tests dragging text within the same contenteditable element. To manually test, move the target below to the destination. You should see dragstart, drop, and dragend in the log.
+
+Log:
+dragstart
+drop
+dragend
+
+PASS
diff --git a/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events.html b/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events.html
new file mode 100644 (file)
index 0000000..1fc2331
--- /dev/null
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests dragging text within the same contenteditable element. To manually test, move the target below to the destination.
+You should see dragstart, drop, and dragend in the log.</p>
+<div contenteditable><span>Drag_and_drop_this_target to here, the_destination.</span></div>
+<pre id="log">
+Log:
+</pre>
+<script>
+
+var log = document.getElementById('log');
+var div = document.querySelector('div');
+var eventLogs = [];
+
+function logEvent(event) {
+    eventLogs.push(event.type);
+    log.innerText += event.type + '\n';
+}
+
+div.addEventListener('dragstart', logEvent, false);
+div.addEventListener('dragend', logEvent, false);
+div.addEventListener('drop', logEvent, false);
+
+if (window.eventSender) {
+    layoutTestController.dumpAsText();
+
+    div.focus();
+    getSelection().collapse(div, 0);
+    getSelection().modify('extend', 'forward', 'word');
+
+    var y = div.offsetTop + div.offsetHeight / 2;
+    eventSender.dragMode = true;
+    eventSender.mouseMoveTo(div.offsetLeft + div.firstChild.offsetWidth / 5, y);
+    eventSender.mouseDown();
+    eventSender.leapForward(500);
+    eventSender.mouseMoveTo(div.offsetLeft + div.firstChild.offsetWidth * 4 / 5, y);
+    eventSender.mouseUp();
+
+    div.style.display = 'none'; // The resultant text will be different depending on font metrics so hide it.
+
+    log.innerHTML += '\n' + (eventLogs.join('') == ['dragstart', 'drop', 'dragend'].join('') ? 'PASS' : 'FAIL');
+}
+
+</script>
+</body>
+</html>
\ No newline at end of file
index 1af04a41958d8bd445e3796ec83f66e6ff8da304..e083763290f0940dd408b480c7c2c94aa3042334 100644 (file)
@@ -1,3 +1,43 @@
+2012-01-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        drop event isn't fired for contentEditable in edit drag
+        https://bugs.webkit.org/show_bug.cgi?id=57185
+
+        Reviewed by Adam Barth.
+
+        Dispatch drop and dragend events after edit drag per HTML5 spec:
+        http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#drag-and-drop-processing-model
+
+        There are two major differences between the spec and WebKit's new behavior:
+
+        While the spec says we have to insert the dragged contents immediately after dispatching drop event
+        and delete the source in the default event handler of dragend event, doing so in WebKit is extremely
+        difficult because of the way we manage the selection. Instead, we continue to delete the source
+        and insert the dragged contents immediately after the drop event; this behavior matches that of Firefox 9.
+
+        When the dragged contents and the destination of the move is in the same text node, ReplaceSelectionCommand
+        may end up replacing it with a new text node. But this removal causes a problem when EventHandler uses
+        the node to dispatch dragend event because the node is "orphaned" from its parent at that point. To mitigate
+        this issue, we update the dragState's m_dragSrc when the node is orphaned by the edit drag. While this behavior
+        may differ from the spec and other browsers, not delivering dragend to the editing host seems strictly worse than
+        dispatching it at the slightly wrong target.
+
+        Tests: fast/events/moving-text-should-fire-drop-and-dragend-events-2.html
+               fast/events/moving-text-should-fire-drop-and-dragend-events.html
+
+        * page/DragController.cpp:
+        (WebCore::DragController::performDrag): Dispatch drop event even when m_isHandlingDrag is true as long
+        as DragDestinationActionDHTML is an acceptable action.
+        (WebCore::DragController::concludeEditDrag): Call updateDragStateAfterEditDragIfNeeded after inserting
+        the dragged contents. This is necessary when ReplaceSelectionCommand or MoveSelectionCommand modifies
+        the source node while inserting the dragged contents.
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::performDragAndDrop): Clear the drag state only if drop event's default action
+        was prevented so that we dispatch dragevent event later.
+        (WebCore::EventHandler::updateDragStateAfterEditDragIfNeeded): Update dragState's m_dragSrc when the node
+        is orphaned. See above for the rationale.
+        * page/EventHandler.h:
+
 2012-01-18  Kinuko Yasuda  <kinuko@chromium.org>
 
         Cleanup: Move chrome-specific filesystem type handling code (for FileSystem API) under chromium directory
index 5d834df8456bd6c8d15b352bf2043416dea406ba..da74a0a936510b66038cd8ba2975d93e21ffbeb2 100644 (file)
@@ -201,19 +201,21 @@ bool DragController::performDrag(DragData* dragData)
 {
     ASSERT(dragData);
     m_documentUnderMouse = m_page->mainFrame()->documentAtPoint(dragData->clientPosition());
-    if (m_isHandlingDrag) {
-        ASSERT(m_dragDestinationAction & DragDestinationActionDHTML);
+    if (m_dragDestinationAction & DragDestinationActionDHTML) {
         m_client->willPerformDragDestinationAction(DragDestinationActionDHTML, dragData);
         RefPtr<Frame> mainFrame = m_page->mainFrame();
+        bool preventedDefault = false;
         if (mainFrame->view()) {
             // Sending an event can result in the destruction of the view and part.
             RefPtr<Clipboard> clipboard = Clipboard::create(ClipboardReadable, dragData, mainFrame.get());
             clipboard->setSourceOperation(dragData->draggingSourceOperationMask());
-            mainFrame->eventHandler()->performDragAndDrop(createMouseEvent(dragData), clipboard.get());
-            clipboard->setAccessPolicy(ClipboardNumb);    // invalidate clipboard here for security
+            preventedDefault = mainFrame->eventHandler()->performDragAndDrop(createMouseEvent(dragData), clipboard.get());
+            clipboard->setAccessPolicy(ClipboardNumb); // Invalidate clipboard here for security
+        }
+        if (m_isHandlingDrag || preventedDefault) {
+            m_documentUnderMouse = 0;
+            return true;
         }
-        m_documentUnderMouse = 0;
-        return true;
     }
 
     if ((m_dragDestinationAction & DragDestinationActionEdit) && concludeEditDrag(dragData)) {
@@ -477,6 +479,7 @@ bool DragController::concludeEditDrag(DragData* dragData)
     VisibleSelection dragCaret = m_page->dragCaretController()->caretPosition();
     m_page->dragCaretController()->clear();
     RefPtr<Range> range = dragCaret.toNormalizedRange();
+    RefPtr<Element> rootEditableElement = innerFrame->selection()->rootEditableElement();
 
     // For range to be null a WebKit client must have done something bad while
     // manually controlling drag behaviour
@@ -519,6 +522,11 @@ bool DragController::concludeEditDrag(DragData* dragData)
             applyCommand(ReplaceSelectionCommand::create(m_documentUnderMouse.get(), createFragmentFromText(range.get(), text),  ReplaceSelectionCommand::SelectReplacement | ReplaceSelectionCommand::MatchStyle | ReplaceSelectionCommand::PreventNesting));
     }
 
+    if (rootEditableElement) {
+        if (Frame* frame = rootEditableElement->document()->frame())
+            frame->eventHandler()->updateDragStateAfterEditDragIfNeeded(rootEditableElement.get());
+    }
+
     return true;
 }
 
index d42b1285690cfda53be5594a7f65d05bc0ebb2de..550f9199bad72a67dd0826cdecbe89dc67fbc55f 100644 (file)
@@ -1911,15 +1911,18 @@ void EventHandler::cancelDragAndDrop(const PlatformMouseEvent& event, Clipboard*
     clearDragState();
 }
 
-void EventHandler::performDragAndDrop(const PlatformMouseEvent& event, Clipboard* clipboard)
+bool EventHandler::performDragAndDrop(const PlatformMouseEvent& event, Clipboard* clipboard)
 {
     Frame* targetFrame;
+    bool preventedDefault = false;
     if (targetIsFrame(m_dragTarget.get(), targetFrame)) {
         if (targetFrame)
-            targetFrame->eventHandler()->performDragAndDrop(event, clipboard);
+            preventedDefault = targetFrame->eventHandler()->performDragAndDrop(event, clipboard);
     } else if (m_dragTarget.get())
-        dispatchDragEvent(eventNames().dropEvent, m_dragTarget.get(), event, clipboard);
-    clearDragState();
+        preventedDefault = dispatchDragEvent(eventNames().dropEvent, m_dragTarget.get(), event, clipboard);
+    if (preventedDefault)
+        clearDragState();
+    return preventedDefault;
 }
 
 void EventHandler::clearDragState()
@@ -2783,7 +2786,14 @@ void EventHandler::dragSourceEndedAt(const PlatformMouseEvent& event, DragOperat
     // that consecutive mousemove events don't reinitiate the drag and drop.
     m_mouseDownMayStartDrag = false;
 }
-    
+
+void EventHandler::updateDragStateAfterEditDragIfNeeded(Element* rootEditableElement)
+{
+    // If inserting the dragged contents removed the drag source, we still want to fire dragend at the root editble element.
+    if (dragState().m_dragSrc && !dragState().m_dragSrc->inDocument())
+        dragState().m_dragSrc = rootEditableElement;
+}
+
 // returns if we should continue "default processing", i.e., whether eventhandler canceled
 bool EventHandler::dispatchDragSrcEvent(const AtomicString& eventType, const PlatformMouseEvent& event)
 {
index 9d05b3e821b14fd253181fe407ea01741632d95b..547bfe57d5b93a01aec53e73204eb2976fcb776e 100644 (file)
@@ -127,7 +127,8 @@ public:
 #if ENABLE(DRAG_SUPPORT)
     bool updateDragAndDrop(const PlatformMouseEvent&, Clipboard*);
     void cancelDragAndDrop(const PlatformMouseEvent&, Clipboard*);
-    void performDragAndDrop(const PlatformMouseEvent&, Clipboard*);
+    bool performDragAndDrop(const PlatformMouseEvent&, Clipboard*);
+    void updateDragStateAfterEditDragIfNeeded(Element* rootEditableElement);
 #endif
 
     void scheduleHoverStateUpdate();