Last Comment Bug 375681 - Add drag and dragend events
: Add drag and dragend events
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Drag and Drop (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: Neil Deakin
:
Mentors:
Depends on: 503034
Blocks: 356295
  Show dependency treegraph
 
Reported: 2007-03-28 07:41 PDT by Neil Deakin
Modified: 2009-07-08 04:22 PDT (History)
8 users (show)
enndeakin: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add drag and dragend events (64.39 KB, patch)
2007-03-28 07:41 PDT, Neil Deakin
bugs: review-
Details | Diff | Splinter Review
testcase (3.13 KB, text/html)
2007-03-28 07:43 PDT, Neil Deakin
no flags Details
fix issues, and use extra events for dragstart, dragleave and drop (75.58 KB, patch)
2007-03-29 07:06 PDT, Neil Deakin
bugs: review-
Details | Diff | Splinter Review
fix issues (166.12 KB, patch)
2007-04-03 10:59 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
right patch, not sure what happened there (72.82 KB, patch)
2007-04-03 13:10 PDT, Neil Deakin
bugs: review+
Details | Diff | Splinter Review
add event documentation (75.36 KB, patch)
2007-04-04 06:36 PDT, Neil Deakin
roc: superreview+
Details | Diff | Splinter Review
fix a small bug (75.66 KB, patch)
2007-04-11 17:17 PDT, Neil Deakin
bugs: review+
roc: superreview+
Details | Diff | Splinter Review
add "nsGUIEvent.h" to the list of includes in nsDragService.cpp (748 bytes, patch)
2007-04-22 07:03 PDT, Walter Meinl
mozilla: review+
Details | Diff | Splinter Review
patch to be checked in (18.75 KB, patch)
2008-03-19 07:08 PDT, Neil Deakin
no flags Details | Diff | Splinter Review

Description Neil Deakin 2007-03-28 07:41:15 PDT
Created attachment 259903 [details] [diff] [review]
add drag and dragend events

This is part one of a drag events patch. The second part will add an nsDragEvent.

Add support for the drag and dragend events which are fired on the source node during a drag and at the end of a drag

See http://www.whatwg.org/specs/web-apps/current-work/#dnd

Notes:
 - changes the event names (event.type) to the whatwg spec names for all the drag events (dragstart instead of draggesture). In HTML, the spec names can be used, but in XUL the old attribute names (ondraggesture) can be used instead for compatibility.
 - the spec says that drag should fire event every 350ms but here I just fire it when the mouse moves. Also it isn't cancellable since I don't think it's possible to do that
Comment 1 Neil Deakin 2007-03-28 07:43:31 PDT
Created attachment 259904 [details]
testcase

HTML testcase, to be run in chrome
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-03-28 08:15:32 PDT
Comment on attachment 259903 [details] [diff] [review]
add drag and dragend events


>diff -u -p -8 -r1.82 nsIEventStateManager.h
>--- content/events/public/nsIEventStateManager.h	16 Aug 2006 03:20:18 -0000	1.82
>+++ content/events/public/nsIEventStateManager.h	28 Mar 2007 14:03
...
>+
>+  NS_IMETHOD FireDragEvent(PRUint32 aMsg, nsIDOMNode* aSourceNode,
>+                           nsPresContext* aPresContext) = 0;

I think you could achieve the similar functionality with
nsCOMPtr<nsPIDOMEventTarget> pitarget = do_QueryInterface(sourceNode);
pitarget->DispatchDOMEvent() ...
(I know, I've marked that method deprecated.)

Or using presShell's HandleDOMEventWithTarget


>@@ -1139,16 +1146,27 @@ nsEventListenerManager::CompileEventHand
>         attrName = nsGkAtoms::onerror;
>       else if (aName == nsGkAtoms::onSVGResize)
>         attrName = nsGkAtoms::onresize;
>       else if (aName == nsGkAtoms::onSVGScroll)
>         attrName = nsGkAtoms::onscroll;
>       else if (aName == nsGkAtoms::onSVGZoom)
>         attrName = nsGkAtoms::onzoom;
> #endif // MOZ_SVG
>+#ifdef MOZ_XUL
>+      if (content->IsNodeOfType(nsINode::eXUL)) {
>+        // XUL uses different event names for drag and drop
>+        if (aName == nsGkAtoms::ondragstart)
>+          attrName = nsGkAtoms::ondraggesture;
>+        else if (aName == nsGkAtoms::ondragleave)
>+          attrName = nsGkAtoms::ondragexit;
>+        else if (aName == nsGkAtoms::ondrop)
>+          attrName = nsGkAtoms::ondragdrop;
>+      }
>+#endif // MOZ_XUL


I really don't like this change. SVG's events are mess, but they are mess
because of the terrible spec. We can do better with XUL, I hope.
If we must support both old and new event handler attribute, would it be bad to
dispatch two different events? Event dispatching isn't that slow.


>+NS_IMETHODIMP
>+nsBaseDragService::FireDragEventAtSource(PRUint32 aMsg)
>+{
>+  if (mSourceNode) {
>+    nsIDocument* doc;
>+    CallQueryInterface(mSourceDocument, &doc);

This leaks |doc|.


> 
> static nsIPresShell*
> GetPresShellForContent(nsIDOMNode* aDOMNode)
> {
>   nsIContent* content;
>   CallQueryInterface(aDOMNode, &content);
> 
Hmm, this leaks |content|.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-03-28 14:13:15 PDT
The ultimate goal here is to support WHATWG-compatible drag and drop that can be used by Web content, right?
Comment 4 Neil Deakin 2007-03-28 14:42:05 PDT
(In reply to comment #3)
> The ultimate goal here is to support WHATWG-compatible drag and drop that can
> be used by Web content, right?
> 

Yes, I'm just implementing it in small easy to review pieces.

Comment 5 Neil Deakin 2007-03-29 07:06:50 PDT
Created attachment 260014 [details] [diff] [review]
fix issues, and use extra events for dragstart, dragleave and drop
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-03-29 10:12:07 PDT
Comment on attachment 260014 [details] [diff] [review]
fix issues, and use extra events for dragstart, dragleave and drop

Random comments about the code, not yet a full review


>@@ -2218,16 +2228,34 @@ nsEventStateManager::PostHandleEvent(nsP
... 
>   case NS_DRAGDROP_DROP:
>+    {
>+      // now fire the dragdrop event, for compatibility with XUL
>+      nsCOMPtr<nsIContent> targetContent;
>+      mCurrentTarget->GetContentForEvent(presContext, aEvent,
>+                                         getter_AddRefs(targetContent));
Are you sure mCurrentTarget is not null?


>+
>+      nsMouseEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_DRAGDROP_DRAGDROP,
>+                         mCurrentTarget->GetWindow(), nsMouseEvent::eReal);
>+      FillInEventFromGestureDown(&event);
>+
>+      nsEventStatus status = nsEventStatus_eIgnore;
>+      nsIPresShell* presShell = mPresContext->GetPresShell();
>+      presShell->HandleEventWithTarget(&event, mCurrentTarget,
>+                                       targetContent, &status);

nsIPresShell* should be nsCOMPtr<nsIPresShell>
see http://lxr.mozilla.org/seamonkey/source/layout/base/nsIPresShell.h#517
And I think you need to check that presShell is not null :(


>+nsEventStateManager::FireDragEnterOrExit(nsPresContext* aPresContext,
...
>+  if (aTargetContent != aRelatedTarget) {
>+    //XXX This event should still go somewhere!!
>+    if (aTargetContent)
>+      nsEventDispatcher::Dispatch(aTargetContent, aPresContext, &event,
>+                                  nsnull, &status);
>+
>+    // adjust the drag hover
>+    if (status != nsEventStatus_eConsumeNoDefault)
>+      SetContentState((aMsg == NS_DRAGDROP_ENTER) ? aTargetContent : nsnull,
>+                      NS_EVENT_STATE_DRAGOVER);
>+  }
>+
>+  // Finally dispatch the event to the frame
>+  if (aTargetFrame)
>+    aTargetFrame->HandleEvent(aPresContext, &event, &status);

I know this code just moved place, but I think event callback could be used here
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventStateManager.cpp&rev=1.686&mark=2614,2657,2659#2614
At least that would make dispatching drag events to work like most other events
(frame's HandleEvent is called after default event handling but before system group's
event handling.) Though, I didn't check how dnd is handled in frames.


>+nsBaseDragService::FireDragEventAtSource(PRUint32 aMsg)
...
>+      nsIPresShell* presShell = doc->GetShellAt(0);
>+      if (presShell) {
...
>+        return presShell->HandleDOMEventWithTarget(content, &event, &status);

nsIPresShell* should be nsCOMPtr<nsIPresShell>
http://lxr.mozilla.org/seamonkey/source/layout/base/nsIPresShell.h#526
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-04-02 09:57:11 PDT
(In reply to comment #6)
>  Though, I didn't check how dnd is handled in frames.
> 

So only nsTreeBodyFrame uses drag-events.
Comment 8 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-04-02 10:21:22 PDT
Comment on attachment 260014 [details] [diff] [review]
fix issues, and use extra events for dragstart, dragleave and drop

>       // get the widget from the target frame
>-      nsEventStatus status = nsEventStatus_eIgnore;
>-      nsMouseEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_DRAGDROP_GESTURE,
>-                         mCurrentTarget->GetWindow(), nsMouseEvent::eReal);
>-      FillInEventFromGestureDown(&event);
>+      nsMouseEvent startEvent(NS_IS_TRUSTED_EVENT(aEvent), NS_DRAGDROP_START,
>+                              mCurrentTarget->GetWindow(), nsMouseEvent::eReal);
>+      FillInEventFromGestureDown(&startEvent);
>+
>+      nsMouseEvent gestureEvent(NS_IS_TRUSTED_EVENT(aEvent), NS_DRAGDROP_GESTURE,
>+                                mCurrentTarget->GetWindow(), nsMouseEvent::eReal);
>+      FillInEventFromGestureDown(&gestureEvent);

I think you need to keep a strong ref to mCurrentTarget->GetWindow().
So maybe nsCOMPtr<nsIWidget> widget = mCurrentTarget->GetWindow();
and then use widget as a parameter for events.

>   case NS_DRAGDROP_DROP:
>+    {
>+      // now fire the dragdrop event, for compatibility with XUL
>+      nsCOMPtr<nsIContent> targetContent;
>+      mCurrentTarget->GetContentForEvent(presContext, aEvent,
>+                                         getter_AddRefs(targetContent));
>+
>+      nsMouseEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_DRAGDROP_DRAGDROP,
>+                         mCurrentTarget->GetWindow(), nsMouseEvent::eReal);

Also here, strong ref to mCurrentTarget->GetWindow()

>+      FillInEventFromGestureDown(&event);
>+
>+      nsEventStatus status = nsEventStatus_eIgnore;
>+      nsIPresShell* presShell = mPresContext->GetPresShell();

Strong ref to presshell.


>+      presShell->HandleEventWithTarget(&event, mCurrentTarget,
>+                                       targetContent, &status);
>+
>+      // fall through and call GenerateDragDropEnterExit

I usually don't like this in switch-case, but here it is ok, IMO.


>+nsEventStateManager::FireDragEnterOrExit(nsPresContext* aPresContext,
>+                                         nsGUIEvent* aEvent,
>+                                         PRUint32 aMsg,
>+                                         nsIContent* aRelatedTarget,
>+                                         nsIContent* aTargetContent,
>+                                         nsIFrame* aTargetFrame)
>+{
>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  nsMouseEvent event(NS_IS_TRUSTED_EVENT(aEvent), aMsg,
>+                     aEvent->widget, nsMouseEvent::eReal);
>+  event.refPoint = aEvent->refPoint;
>+  event.isShift = ((nsMouseEvent*)aEvent)->isShift;
>+  event.isControl = ((nsMouseEvent*)aEvent)->isControl;
>+  event.isAlt = ((nsMouseEvent*)aEvent)->isAlt;
>+  event.isMeta = ((nsMouseEvent*)aEvent)->isMeta;
>+  event.relatedTarget = aRelatedTarget;
>+
>+  mCurrentTargetContent = aTargetContent;
>+
>+  if (aTargetContent != aRelatedTarget) {
>+    //XXX This event should still go somewhere!!
>+    if (aTargetContent)
>+      nsEventDispatcher::Dispatch(aTargetContent, aPresContext, &event,
>+                                  nsnull, &status);
>+
>+    // adjust the drag hover
>+    if (status != nsEventStatus_eConsumeNoDefault)
>+      SetContentState((aMsg == NS_DRAGDROP_ENTER) ? aTargetContent : nsnull,
>+                      NS_EVENT_STATE_DRAGOVER);
>+  }
>+
>+  // Finally dispatch the event to the frame
>+  if (aTargetFrame)
>+    aTargetFrame->HandleEvent(aPresContext, &event, &status);

It is possible that aTargetFrame points to a deleted frame here, if dispatching event
deleted something. Maybe the last parameter could be nsWeakFrame& aFrame, that should
prevent use of deleted objects. And either dispatch event manually to 
frame or use nsDispatchingCallback. (The change to use nsDispatchingCallback
can be a different bug too.)
Comment 9 Neil Deakin 2007-04-03 10:59:52 PDT
Created attachment 260479 [details] [diff] [review]
fix issues
Comment 10 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-04-03 13:00:56 PDT
nsSidebar.js shouldn't be in the patch, nor
nsXULTemplateBuilder.cpp or nsXULTreeBuilder.cpp
Hmm, there is also jsconsole-clhandler.js...
Kind of hard to review. Could you upload a new patch?
Comment 11 Neil Deakin 2007-04-03 13:10:54 PDT
Created attachment 260503 [details] [diff] [review]
right patch, not sure what happened there
Comment 12 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-04-03 14:48:11 PDT
Comment on attachment 260503 [details] [diff] [review]
right patch, not sure what happened there

r+ for content/ changes, if you document somewhere (maybe in nsIDOMDragListener)  what is the order of the events and what events are dispatched because of XUL backward compatibility.
Comment 13 Neil Deakin 2007-04-04 06:36:08 PDT
Created attachment 260583 [details] [diff] [review]
add event documentation
Comment 14 Neil Deakin 2007-04-11 17:17:16 PDT
Created attachment 261320 [details] [diff] [review]
fix a small bug

OK, this patch is identical except it fixes a small bug in nsEventStateManager::PostHandleEvent which initializes the event's point incorrectly. This causes tab drag/drop to not work.
Comment 15 Neil Deakin 2007-04-11 23:31:02 PDT
Checked in
Comment 16 José Jeria 2007-04-14 02:20:55 PDT
These events can only be added using addEventListener? It does not work when adding the events in the HTML (for example <img ondragstart="doX();">).
Comment 17 Neil Deakin 2007-04-14 17:32:52 PDT
The attribute form isn't supported yet for HTML elements. Until bug 356295 is implemented the events don't have much use from web pages.
Comment 18 Walter Meinl 2007-04-21 16:13:38 PDT
(In reply to comment #15)
> Checked in
> 

This breaks the OS/2 build:

D:/usr/src/mozilla/widget/src/os2/nsDragService.cpp
D:/usr/src/mozilla/widget/src/os2/nsDragService.cpp: In member function `
   virtual nsresult nsDragService::InvokeDragSession(nsIDOMNode*, 
   nsISupportsArray*, nsIScriptableRegion*, unsigned int)':
D:/usr/src/mozilla/widget/src/os2/nsDragService.cpp:240: error: `
   NS_DRAGDROP_END' undeclared (first use this function)
D:/usr/src/mozilla/widget/src/os2/nsDragService.cpp:240: error: (Each 
   undeclared identifier is reported only once for each function it appears 
   in.)
make.exe[6]: *** [nsDragService.o] Error 1
make.exe[6]: Leaving directory `D:/mozbuild/widget/src/os2'

I think nsGUIEvent.h has to be added to the includes in widget/src/os2/nsDragService.cpp
Comment 19 Neil Deakin 2007-04-21 16:56:22 PDT
Walter, can you make a patch?
Comment 20 Walter Meinl 2007-04-22 07:03:36 PDT
Created attachment 262425 [details] [diff] [review]
add "nsGUIEvent.h" to the list of includes in nsDragService.cpp

your patch added
   mDoingDrag = PR_TRUE;
   HWND hwndDest = DrgDrag(mDragWnd, pDragInfo, &dragimage, 1, VK_BUTTON2,
                   (void*)0x80000000L); // Don't lock the desktop PS
+  FireDragEventAtSource(NS_DRAGDROP_END);
   mDoingDrag = PR_FALSE;
 
     // do clean up;  if the drop completed,

NS_DRAGDROP_END is defined in nsGUIEvent.h
I guess the OS/2 build fails, cause nsGUIEvent.h is not in the includes of nsDragService.cpp
I'm not totally sure, if its the propper fix, cause OS/2 was the only platform where NS_DRAGDROP_END was added in nsDragService.cpp. However deduced from the error report I'd guess it should be.
Neil, whom to ask for review?
Comment 21 Neil Deakin 2007-04-22 10:47:39 PDT
Checked in the last patch
Comment 22 L A Walsh 2007-05-20 15:10:22 PDT
Should this allow dragging the URL from the address bar onto a Windows (XP) desktop?

BTW: 
Neil Deakin wrote:
 - the spec says that drag should fire event every 350ms but here I just fire
it when the mouse moves. Also it isn't cancellable since I don't think it's
possible to do that
----

One can cancel a Drag'N'Drop event by hitting the escape key while dragging.  I use it on occasion:  1) if my mouse accidentally selected an icon while I moved, 2) if I  selected the wrong icon or 3) the target I want to drop on isn't visible.

Comment 23 Neil Deakin 2008-03-19 07:08:06 PDT
Created attachment 310464 [details] [diff] [review]
patch to be checked in
Comment 24 Neil Deakin 2008-03-19 07:08:41 PDT
Oops, wrong bug.

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