356295 - Drag and Drop (WHATWG)

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Neil Deakin]

In XUL currently it is hard to implement drag and drop as it requires a lot of code, or one can use nsDragAndDrop.js which is a bit easier although requires the use of extra scripts, only works in chrome, and isn't too flexible.

Instead, the drag and drop API WHATWG spec can be used, although I need to get some clarifications on some parts that don't make sense. It can also be used in HTML as well.

Comment 1

[Nico Nekkebreock]

Dear all,
I managed to make it, finally, work for FF 1.5. But like says previously, it is not just copy and paste the code found on http://www.xulplanet.com/tutorials/mozsdk/dragex.php.

Steps to make it work:
1) Copy and paste the code => You end up with those files:
      mainsceen.xul: all the xul code
      dragboard.js : with the list and board observers
      nsDragAndDrop.js: copy and paste of chrome://global/content/nsDragAndDrop.js
      nsTransferable.js: copy and paste of                      chrome://global/content/nsTransferable.js 
2) You should modify nsDragAndDrop.js & nsTransferable.js in such way that before each call to Components.classes you add netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
3) Then it works.

ps(If wanted, I can post a zip of this project to the persons who want.)

Nico

Comment 2

[Neil Deakin]

I have most of this done, but am waiting for 178513 to finish up

Comment 4

[Neil Deakin]

Attachment: work in progress

Implements the spec and adds some functions needed for XUL, listed at http://wiki.mozilla.org/XUL:Specs:TransferData

There are still a few issues:
 - the calls to nsIClipboardDragDropHooks listeners need to be handled.
 - some issues with the dropEffect and effectsAllowed, but much of that is because the spec doesn't make complete sense here

Couldn't find a simple way to break this patch down into smaller chunks unfortunately.

Comment 5

[Neil Deakin]

Attachment: Updated patch with tests

Comment 6

[Neil Deakin]

Attachment: manual test

This manual test checks all of the event state for each of the drag events. Some parts will fail unless wrapped in a chrome window which I'll attach next.

Comment 7

[Neil Deakin]

Attachment: outer chrome test

Has a hardcoded path that needs updating to point to dragdrop-dt-events.xul using a file url (the previous attachment).

Comment 8

[Neil Deakin]

Comment on attachment 319783 [details] [diff] [review]
Updated patch with tests

No reason this can't be reviewed now actually.

Differences from the specification:

1. The dropEffect and effectAllowed properties are initialized differently. The spec doesn't take into account the key state for the cursor, where as this implementation does, (so that one can switch between copy, move and link by pressing modifier keys while dragging).

Spec:
 effectAllowed is initialized to the value it had after the last drag related event.
 dropEffect is initialized to 'none' for dragstart, drag and dragleave events
 dropEffect is initialized to the actual used drag operation for drop and dragend events
 dropEffect is initialized to a value based on the effectAllowed for the dragenter and dragover events
 (spec has a table for this latter one)

Implementation:
 effectAllowed is initialized to the value it had after the dragstart event.
 dropEffect is initialized to 'none' for dragstart, drag and dragleave events (same as spec)
 dropEffect is initialized to the actual used drag operation for drop and dragend events (same as spec)
 dropEffect is initialized to the key modifier state for the dragenter and dragover events

Note that while the drop effect is set properly, this implementation doesn't yet update the cursor image, as I haven't figured out how to get that to work yet. Followup up I think.

2. Several methods, all in nsIDOMNSDataTransfer are needed for XUL chrome usage. These add support for dragging multiple items, as well as dragging non-string data. This is needed for instance for dragging a set of files, which must be a list of nsIFile objects.

3. The spec doesn't allow one to retrieve the data in a dataTransfer during the drag, only after the drop. My implementation does, but stores a principal with the data such that access is allowed only to the data that the caller added. This makes it easier to implement drag and drop between multi-window applications, but may be overkill. I haven't decided whether this is really necessary and it may be easier to just allow any access to chrome only.

4. The spec describes some aspects of UI behaviour with dragging selections and text in textboxes. I'm not sure why the spec defines this (I asked in the mailing list), but I didn't change the UI we currently use.

5. The patch includes tests for parts of drag and drop that can be automated. The other attachments are manual tests for the parts that cannot be automated.

6. I removed support for the drag/drop portion of the nsIClipboardDragDropHooks interface. It's hard to implement as it uses the nsITransferable interface, and it isn't even used anywhere in the existing code. Instead, callers can just add capturing event listeners if they want to trap and prevent drag events.

Comment 9

[Neil Deakin]

Comment on attachment 319783 [details] [diff] [review]
Updated patch with tests

Some Mac changes here, but they are actually for bug 425240.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 319783 [details] [diff] [review]
Updated patch with tests

Few, sort of random comments.

>+ * The Initial Developer of the Original Code is Neil Deakin.
Is this right, or should it be MoCo or MoFo?
>+class nsDOMDataTransfer : public nsIDOMDataTransfer,
>+                          public nsIDOMNSDataTransfer
>+{
>+public:
>+  NS_DECL_ISUPPORTS
I think nsDOMDataTransfer should be cycle collectable.

>+NS_IMETHODIMP
>+nsDOMDragEvent::InitDragEvent(const nsAString & aType,
>+                              PRBool aCanBubble,
>+                              PRBool aCancelable)
Um, shouldn't this init be
void initDragEvent(in DOMString typeArg, in boolean canBubbleArg, in boolean cancelableArg, in AbstractView viewArg, in long detailArg, in DataTransfer dataTransferArg)

>+nsDOMDragEvent::GetDataTransfer(nsIDOMDataTransfer** aDataTransfer)
>+{
...
>+  nsDragEvent* dragEvent = static_cast<nsDragEvent*>(mEvent);
>+  if (!dragEvent->dataTransfer) {
You could return early if (dragEvent->dataTransfer)

>+    // for synthetic events, just create an empty data transfer object
>+    if (mEventIsInternal) {
>+      *aDataTransfer = new nsDOMDataTransfer();
>+      NS_ENSURE_TRUE(aDataTransfer, NS_ERROR_OUT_OF_MEMORY);
>+
>+      NS_ADDREF(*aDataTransfer);
>+      return NS_OK;
>+    }
Why? If you implement InitDragEvent properly, you should return the object
which is passed there.

>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Ilya Konstantinov ([e-mail])
This all doesn't look quite right.

>+class nsDOMDragEvent : public nsIDOMDragEvent,
>+                       public nsDOMMouseEvent
...
>+  nsCOMPtr<nsIDOMDataTransfer> mDataTransfer;
You should add mDataTransfer to cycle collection, or if you remove this
member and always use mEvent->dataTransfer, then that should be added to CC.

>@@ -1254,24 +1269,30 @@ const char* nsDOMEvent::GetEventName(PRU
Update also SetEventType

>+interface nsIDOMDataTransfer : nsISupports
>+  /**
>+   * Remove the data associated with a given format. If format is empty or not
>+   * specified, the data associated with all formats is removed. If data for
>+   * the specified format does not exist, or the data transfer contains no
>+   * data, this method will have no effect.
>+   */
>+  void clearData([optional] in DOMString format);
Why [optional]? Where is that specified?

>+  /**
>+   * Set the data for a given format. If data for the format does not exist,
>+   * it is added at the end, such that the last item in the types list will be
>+   * the new format. If data for the format already exists, the existing data
>+   * is replaced in the same position. That is, the order of the types list is
>+   * not changed.
>+   *
>+   * @throws NS_ERROR_NULL_POINTER if the data is null
>+   */
>+  void setData(in DOMString format, in DOMString data);
HTML5 setData doesn't throw, at least not according to the interface.

>+  void initDragEvent(in DOMString typeArg,
>+                     in boolean canBubbleArg,
>+                     in boolean cancelableArg);
>+
>+  void initDragEventNS(in DOMString namespaceURIArg,
>+                       in DOMString typeArg,
>+                       in boolean canBubbleArg,
>+                       in boolean cancelableArg);
These aren't right.

>Index: dom/public/idl/html/nsIDOMNSHTMLElement.idl
...
>+           // for WHAT-WG drag and drop
>+           attribute boolean          draggable;
>+
You're updating an interface, update iid too?

>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMNSDataTransfer)
So we want to expose mozilla-only methods to web content?

Will continue reviewing after re-reading HTML5 dnd.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 319783 [details] [diff] [review]
Updated patch with tests

>-nsContentAreaDragDrop::SaveURIToFile(nsAString& inSourceURIString,
>-                                     nsIFile* inDestFile)
Is there a reason to remove this method and move its content to elsewhere?

>+nsTransferableFactory::AddStringsToDataTransfer(nsIDOMNode* aDragNode,
>+                                                nsDOMDataTransfer* aDataTransfer)
...
>+  // set all of the data to have the principal of the node where the data came from
>+  nsIContent* node;
>+  CallQueryInterface(aDragNode, &node);
>+  nsIPrincipal* principal = node->NodePrincipal();
CallQueryInterface does AddRef, are you releasing |node| somewhere?
And is it guaranteed that aDragNode isn't null - CallQueryInterface isn't
null-safe.

>+nsDOMDataTransfer::AddElement(nsIDOMElement* aElement)
...
>+  mDragTarget = do_QueryInterface(aElement);
Is this right? AddElement() is there to set elements for drag feedback, but 
at least the comment says that mDragTarget is 
+  // the target of the drag. The drag and dragend events will fire at this.

>+class nsDOMDragEvent : public nsIDOMDragEvent,
>+                       public nsDOMMouseEvent
Why a mouse event? Because drag events have traditionally been mouse events in gecko?
That is not what HTML5 specifies.
If there is a good reason for drag events to be mouse events, perhaps HTML5 should be changed.
That might actually solve at least some part of the problem described in HTML5
"We should have modifier key information in here too (shift/ctrl, etc), like with mouse events and like with the context menu event."

>+  DOM_CLASSINFO_MAP_BEGIN(DragEvent, nsIDOMDragEvent)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMDragEvent)
>+    DOM_CLASSINFO_UI_EVENT_MAP_ENTRIES
>+  DOM_CLASSINFO_MAP_END
Ah, but here only dragevent and uievent interfaces are exposed to content.

Feel free to update the patch to fix existing review comments, or
I can continue reviewing this patch too. But for this patch r-

Comment 12

[Neil Deakin]

> >+NS_IMETHODIMP
> >+nsDOMDragEvent::InitDragEvent(const nsAString & aType,
> >+                              PRBool aCanBubble,
> >+                              PRBool aCancelable)
> Um, shouldn't this init be
> void initDragEvent(in DOMString typeArg, in boolean canBubbleArg, in boolean
> cancelableArg, in AbstractView viewArg, in long detailArg, in DataTransfer
> dataTransferArg)

Bah, that's because the spec changed since I wrote that. Note that, however, the spec doesn't provide a means of creating a DataTransfer.

> >+interface nsIDOMDataTransfer : nsISupports
> >+  /**
> >+   * Remove the data associated with a given format. If format is empty or not
> >+   * specified, the data associated with all formats is removed. If data for
> >+   * the specified format does not exist, or the data transfer contains no
> >+   * data, this method will have no effect.
> >+   */
> >+  void clearData([optional] in DOMString format);
> Why [optional]? Where is that specified?

To be compatible with IE and Safari. I'll comment about that on the mailing list.

> >+   * @throws NS_ERROR_NULL_POINTER if the data is null
> >+   */
> >+  void setData(in DOMString format, in DOMString data);
> HTML5 setData doesn't throw, at least not according to the interface.

It doesn't say what to do. I'm of the opinion that the spec is quite vague about much of the API, yet is too detailed about other aspects.

> 
> >+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMNSDataTransfer)
> So we want to expose mozilla-only methods to web content?

I've thought about it, but don't know what to do here.

> >-nsContentAreaDragDrop::SaveURIToFile(nsAString& inSourceURIString,
> >-                                     nsIFile* inDestFile)
> Is there a reason to remove this method and move its content to elsewhere?

No, I probably moved it thinking I would need to change it, but I'll move it back to a method.

> >+nsDOMDataTransfer::AddElement(nsIDOMElement* aElement)
> ...
> >+  mDragTarget = do_QueryInterface(aElement);
> Is this right? AddElement() is there to set elements for drag feedback, but 
> at least the comment says that mDragTarget is 
> +  // the target of the drag. The drag and dragend events will fire at this.

Again, the spec is quite vague. I asked about it and got a similarly vague answer. Since this method isn't implemented anywhere else, I think we should just define it that way.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are you going to put a new patch here or do you want me to review this one?

I don't see any messages from you about drag/drop on the WHATWG list since August last year. Have you got a bunch of feedback stored up that you need to send or have you been asking on public-html?

Comment 14

[Neil Deakin]

(In reply to comment #13)
> Are you going to put a new patch here or do you want me to review this one?

I think this one should be reviewed fully before I address any comments.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is really awesome.

+  static nsresult GetDragData(nsIDOMAbstractView* aView,
+                              nsIDOMNode* aTarget,
+                              nsIDOMNode* aSelectionTargetNode,
+                              PRBool aIsAltKeyPressed,
+                              nsDOMDataTransfer* aDataTransfer,
+                              PRBool* aCanDrag,
+                              PRBool* aDragSelection,
+                              nsIDOMNode** aDragNode);

Why aren't we just passing nsIDOMWindow* for the view/window (ditto in nsTransferableFactory)? And we should be using nsIContent instead of nsIDOMNode as much as possible.

+  PRBool mIsAltKeyPressed;

PRPackedBool. Just good habits.

 //    with the source URI of the file to save (as a string). We did
-//    that above.
+//    in AddStringsToDataTransfer.

"We did in AddStringsToDataTransfer"?

I think having SaveURIToFile as a separate function was a good thing. Please keep that.

+class nsContentAreaDragDropDataProvider : public nsIFlavorDataProvider
+{

Add comments explaining what this is for

To check aDropEffect and aEffectAllowed, I'd use a helper function that loops over a char* array.

Should we provide spec feedback that HTML5 should support multiple items per datatransfer, or is that just some legacy thing that we have to support but would prefer not to? If HTML5 doesn't add this, I'm not comfortable exposing this extension to Web content without at least moz prefixes.

HTML5 does not document clearData's parameter as being optional. We should either request that be added to the spec, or avoid doing it.

+    // add a new format
+    TransferItem* formatitem = item.AppendElement();
+    NS_ENSURE_TRUE(formatitem, NS_ERROR_OUT_OF_MEMORY);
+
+    formatitem->mFormat = format;
+    formatitem->mPrincipal = aPrincipal;
+    formatitem->mData = aData;

You can share this code across both arms of the enclosing "if".

+ * is it allowed to, yet still allow a chrome caller to retrieve any of the

'it is'

+    // each event should use a clone of the original dataTransfer.
+    initialDataTransferNS->Clone(mEvent->message,
+                                 getter_AddRefs(dragEvent->dataTransfer));

I don't get it. What use then are the HTML5 APIs that mutate the DataTransfer?

+      nsAutoString dropEffect(NS_LITERAL_STRING("none"));
+      if (effectAllowed.EqualsLiteral("none")) {
+        dropEffect.AssignLiteral("none");
+      }
+      else if (action & nsIDragService::DRAGDROP_ACTION_COPY) {
etc

Would all this be simplified if the effect and effectAllowed in nsDOMDataTransfer were stored as nsIDragService bitmasks? The attributes on nsDOMDataTransfer would convert to and from strings (possibly just using an array) and we could access the codes via nsIDOMNSDataTransfer. That would also simplify new code in nsEventStateManager::GenerateDragGesture and nsEventStateManager::PostHandleEvent.

+        event = gestureEvent;

This copies gestureEvent over startEvent. I don't think that's what you intended. Maybe event should just be an nsDragEvent*.

+      if (status != nsEventStatus_eConsumeNoDefault) {
+        // Default handling for the draggesture/dragstart event.

Could the following code go in a helper function? You're adding a lot of code here and it would be nice to break things down a bit. You could also reduce nesting with some early exits.

The tree selection region stuff should definitely go in a helper function. Ick. Does that really need to be handled in nsEventStateManager? It should be #ifdef MOZ_XUL at least.

+  nsCOMPtr<nsISupports> cont = aPresContext->GetContainer();

You already did this, it's called 'container' 20 lines up

+          // If a XUL element is encountered, stop looking for draggable nodes
+          // and just use the original clicked node instead.

why?

+      nsCOMPtr<nsIDragService> dragService =
+        do_GetService("@mozilla.org/widget/dragservice;1");
+      if (!dragService)
+        break;
+
+      nsCOMPtr<nsIDragSession> dragSession;
+      dragService->GetCurrentSession(getter_AddRefs(dragSession));
+      if (!dragSession)
+        break;

Can this block be shared in a helper method?

+  nsAutoString isDraggable;
+  if (GetAttr(kNameSpaceID_None, nsGkAtoms::draggable, isDraggable))
+    *aDraggable = isDraggable.IsEmpty() || isDraggable.EqualsLiteral("true");
+  else
+    *aDraggable = PR_FALSE;

Use FindAttrValueIn

+  if (aDraggable) {
+    return SetAttrHelper(nsGkAtoms::draggable, NS_LITERAL_STRING("true"));
+  }
+
+  return SetAttrHelper(nsGkAtoms::draggable, NS_LITERAL_STRING("false"));

Just do SetAttrHelper(..., aDraggable ? ... : ...);

+  // XXXshould this (and anchor->draggable) check be case sensitive?
+  // images may be dragged unless the draggable attribute is false

I don't think so, but if you think it's unclear, ask and get Ian to clarify the spec if necessary.

+  nsCOMPtr<nsIDOMDragEvent> mouseEvent(do_QueryInterface(aDragEvent));
   rv = dragService->InvokeDragSessionWithSelection(selection, transferableArray,
-                                                   flags, mouseEvent);
+                                                   flags, mouseEvent, nsnull);

rename 'mouseEvent'

In nsTreeBodyFrame::GetSelectionRegion, wouldn't it be simpler to do everything in appunits and then convert to devpixels at the end? BTW shouldn't the scriptableregion be in CSS pixels not device pixels? And can't the toolkit tree XBL binding take care of constructing the right drag image instead of having to have this code plus the code in nsEventStateManager?

+              var length = (typeof data == "string") ? data.length : 4;

What's 4?

Comment 16

[Neil Deakin]

> 
> To check aDropEffect and aEffectAllowed, I'd use a helper function that loops
> over a char* array.

What is this comment in reference to? SetDropEffect/SetEffectAllowed?

> 
> Should we provide spec feedback that HTML5 should support multiple items per
> datatransfer, or is that just some legacy thing that we have to support but
> would prefer not to?

To support dragging multiple items, such as a group of bookmarks, or a group of files.

> +    // each event should use a clone of the original dataTransfer.
> +    initialDataTransferNS->Clone(mEvent->message,
> +                                 getter_AddRefs(dragEvent->dataTransfer));
> 
> I don't get it. What use then are the HTML5 APIs that mutate the DataTransfer?

I don't understand this comment. The Clone method acts like a copy constructor, and copies all the fields to the new dataTransfer, most of which don't have getters/setters.

> 
> Would all this be simplified if the effect and effectAllowed in
> nsDOMDataTransfer were stored as nsIDragService bitmasks? The attributes on
> nsDOMDataTransfer would convert to and from strings (possibly just using an
> array) and we could access the codes via nsIDOMNSDataTransfer. That would also
> simplify new code in nsEventStateManager::GenerateDragGesture and
> nsEventStateManager::PostHandleEvent.

I originally did do this, but it wasn't simpler. Note that nsEventStateManager::PostHandleEvent only has access to the nsIDOMDataTransfer so always goes through the interface.

> In nsTreeBodyFrame::GetSelectionRegion, wouldn't it be simpler to do everything
> in appunits and then convert to devpixels at the end?

Regions don't have any scaling methods, so I don't think that would be simpler.

> And can't the toolkit tree XBL binding take care of constructing
> the right drag image instead of having to have this code plus the
> code in nsEventStateManager?

It isn't constructing the image, it is building a selection region. On Mac, if drag images are disabled, it draws a rectangle around each portion of the region. (This is what Firefox 2 does always).

Are you suggesting that the nsEventStateManager invoke a method on the tree binding to get the region, or something else?

> 
> +              var length = (typeof data == "string") ? data.length : 4;
> 
> What's 4?

I don't know what this should be here. It needs to be non-zero or the native drag code doesn't work. So I just arbitrarily used the 32-bit-pointer-width.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #16)
> > To check aDropEffect and aEffectAllowed, I'd use a helper function that loops
> > over a char* array.
> 
> What is this comment in reference to? SetDropEffect/SetEffectAllowed?

Yes.

> > Should we provide spec feedback that HTML5 should support multiple items per
> > datatransfer, or is that just some legacy thing that we have to support but
> > would prefer not to?
> 
> To support dragging multiple items, such as a group of bookmarks, or a group
> of files.

We could of course use a custom type representing a group of things. Obviously a general multi-item mechanism is better, but I don't know what the tradeoffs are. Sounds like something you should propose on the whatwg list.

> > +    // each event should use a clone of the original dataTransfer.
> > +    initialDataTransferNS->Clone(mEvent->message,
> > +                                 getter_AddRefs(dragEvent->dataTransfer));
> > 
> > I don't get it. What use then are the HTML5 APIs that mutate the
> DataTransfer?
> 
> I don't understand this comment. The Clone method acts like a copy
> constructor, and copies all the fields to the new dataTransfer, most of which
> don't have getters/setters.

OK, but ... after the first event, is the DataTransfer read-only so that later events are unable to change it? Because if they could change it, then this clone operation would mean the changes were not reflected in events after that.

> > Would all this be simplified if the effect and effectAllowed in
> > nsDOMDataTransfer were stored as nsIDragService bitmasks? The attributes on
> > nsDOMDataTransfer would convert to and from strings (possibly just using an
> > array) and we could access the codes via nsIDOMNSDataTransfer. That would also
> > simplify new code in nsEventStateManager::GenerateDragGesture and
> > nsEventStateManager::PostHandleEvent.
> 
> I originally did do this, but it wasn't simpler. Note that
> nsEventStateManager::PostHandleEvent only has access to the nsIDOMDataTransfer
> so always goes through the interface.

Why can't it pull in nsDOMDataTransfer.h? You could even add [noscript] methods to nsIDOMDataTransfer if necessary. I don't understand why this wouldn't be simpler.

> > In nsTreeBodyFrame::GetSelectionRegion, wouldn't it be simpler to do
> > everything in appunits and then convert to devpixels at the end?
> 
> Regions don't have any scaling methods, so I don't think that would be
> simpler.

We already have code that scales regions --- see ConvertNativeRegionToAppRegion in nsViewManager.cpp, so just factor that out into a method on nsIRegion. At some point we should just get rid of nsIRegion and make all users including nsScriptableRegion use nsRegion directly.

> > And can't the toolkit tree XBL binding take care of constructing
> > the right drag image instead of having to have this code plus the
> > code in nsEventStateManager?
> 
> It isn't constructing the image, it is building a selection region. On Mac, if
> drag images are disabled, it draws a rectangle around each portion of the
> region. (This is what Firefox 2 does always).
> 
> Are you suggesting that the nsEventStateManager invoke a method on the tree
> binding to get the region, or something else?

Sorry, I hadn't really thought it through. There is supposed to be enough API here for Web apps to construct custom drag images, but I suppose having trees use that API would actually be pretty hard since the tree rows aren't DOM elements. I guess I'll let this slide, but could you at least factor this evil bit out into nsLayoutUtils::GetSelectionRegionForContent or something?

> > +              var length = (typeof data == "string") ? data.length : 4;
> > 
> > What's 4?
> 
> I don't know what this should be here. It needs to be non-zero or the native
> drag code doesn't work. So I just arbitrarily used the 32-bit-pointer-width.

Please document this as kDummyDataLength or something and explain what it's for.

Comment 18

[Neil Deakin]

Another possibility for supporting multiple items is to use arrays:

event.dataTransfer.setData('application/x-moz-file', [file1, file2, file3])
event.dataTransfer.setData('text/plain', ['name1', 'name2', 'name3'])

The issues here:

1. It requires variants to be supported by getData/setData/clearData, rather than string-only.
2. Wouldn't be compatible, as a caller that wasn't expecting an array to be there wouldn't be able to retrieve the string form. In the example above, they would receive a three element array rather than a string.

There isn't really any way around 1. Somehow, non strings need to be supported for at least file and image dragging.

For 2, we could require a list format string, like 'text/plain-list' or 'list text/plain' or somesuch.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do any native platforms support heterogenous multi-item drag-drop like this? Windows doesn't. If no native platforms support it, then the case for adding it to HTML5 is weak.

How do we use multi-item drag/drop in our code today?

We can encode images and files as data: URLs for string-based consumers (we don't have to store them that way).

Comment 20

[Neil Deakin]

(In reply to comment #19)
> Do any native platforms support heterogenous multi-item drag-drop like this?

Don't know. Cocoa has documentation explaining how to use drag multiple files using arrays.

> How do we use multi-item drag/drop in our code today?

For dragging mutliple bookmarks, files, etc.

> We can encode images and files as data: URLs for string-based consumers (we
> don't have to store them that way).

Er, yes, that's the text/plain format of the data. For other formats, these are non-strings. (Files are nsIFiles, dom nodes are nsIDOMNode, etc).

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For homogeneous collections you can define a new type that represents a collection of objects of another type.

If we *only* use it for multiple bookmarks and files, then I'd be tempted to just do that. But I agree that seems a bit sucky.

Comment 22

[Neil Deakin]

(In reply to comment #21)
> For homogeneous collections you can define a new type that represents a
> collection of objects of another type.

Can you describe what api you mean would be used here?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

stuff like application/x-file-list, application/x-image-list, etc. I'm not all that excited about it though, I'm swinging back to the opinion that we need a good generic solution.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Neil, what do you need to make progress here?

Comment 25

[Neil Deakin]

I'm currently testing changes which remove the extra nsIDOMNSDataTransfer methods, and use 'list text/plain' for instance to specify lists of items.

If setData('text/plain', value) was called, getData('text/plain') will return the value string and getData('list text/plain') will return an array containing the value string as the one and only element.

If setData('list text/plain', [value, value]) was called, getData('text/plain') will return '' and getData('list text/plain') will return the array.

If both setData calls are made, getData will return the appropriate one, so one could support both the spec-defined way that expects a string, and still support multiple drag items using the list retrieval way.

Comment 26

[Josh Aas]

Comment on attachment 319783 [details] [diff] [review]
Updated patch with tests

-  if (aMessage == NS_DRAGDROP_EXIT && dragSession) {
+  if ((aMessage == NS_DRAGDROP_EXIT || aMessage == NS_DRAGDROP_DROP) &&
+       dragSession) {

The last line added here is indented one space too much.

Why is the change to "widget/src/cocoa/nsDragService.h" necessary? You're not adding any uses of that in Cocoa widgets.

-      if (!NS_SUCCEEDED(dragSession->GetCanDrop(&canDrop)) || !canDrop)

In an existing comment a few lines above this line you changed, "assumption" is spelled "assuption". Can you fix that please?

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 319783 [details] [diff] [review]
Updated patch with tests

I'm very eagerly awaiting a new patch.

Comment 28

[Neil Deakin]

Attachment: new patch

This latest patch removes the nsIDOMNSDataTransfer interface and implements multiple items using arrays.

Comment 29

[Neil Deakin]

Attachment: updated manual test

Comment 30

[Neil Deakin]

Attachment: mail and suite patch

Comment 31

[Neil Deakin]

Attachment: up-to-date patch

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 331673 [details] [diff] [review]
up-to-date patch

>+++ b/dom/public/idl/events/nsIDOMDragEvent.idl
...
>+[scriptable, uuid(18FEEFD7-A461-4865-BCF1-4DC8A2F30584)]
>+interface nsIDOMDragEvent : nsIDOMMouseEvent
>+{
>+  readonly attribute nsIDOMDataTransfer dataTransfer;
>+
>+  void initDragEvent(in DOMString typeArg,
>+                     in boolean canBubbleArg,
>+                     in boolean cancelableArg);
>+
>+  void initDragEventNS(in DOMString namespaceURIArg,
>+                       in DOMString typeArg,
>+                       in boolean canBubbleArg,
>+                       in boolean cancelableArg);

This interface isn't right.
http://www.whatwg.org/specs/web-apps/current-work/#dragevent

> class nsMouseEvent : public nsMouseEvent_base
...
>+protected:
>+  nsMouseEvent(PRBool isTrusted, PRUint32 msg, nsIWidget *w,
>+               PRUint8 structType, reasonType aReason)
>+    : nsMouseEvent_base(isTrusted, msg, w, structType),
>+      acceptActivation(PR_FALSE), reason(aReason), context(eNormal),
>+      clickCount(0)
>+  {
>+    if (msg == NS_MOUSE_MOVE) {
>+      flags |= NS_EVENT_FLAG_CANT_CANCEL;
>+    }
>+  }
The new constractor doesn't initialize |exit|.

More comments after breakfast...

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure about this "list" approach. Seems to me that it's basically introducing extra APIs without new functions, so you lose type checking.

I think either we persuade HTML5 to accept multi-item APIs, or we add them ourselves as moz* extensions, or we don't support them at all.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 331673 [details] [diff] [review]
up-to-date patch

>+  nsresult Produce(PRBool* aCanDrag,
>+                   PRBool* aDragSelection,
>+                   nsDOMDataTransfer* aDataTransfer,
>+                   nsIContent** aDragNode);
aDataTransfer is |in| parameter and others are |out|, right?
The |in| parameter could be the first one.

>+  // check if the node is inside a form control. If so, dragging will be
>+  // handled in editor code (nsPlaintextDataTransfer::DoDrag). Don't set
>+  // aCanDrag to false however, as we still want to allow the drag.
>+  nsCOMPtr<nsIContent> findFormNode = mSelectionTargetNode;
>+  nsIContent* findFormParent = findFormNode->GetParent();
>+  while (findFormParent) {
>+    nsCOMPtr<nsIFormControl> form(do_QueryInterface(findFormParent));
>+    if (form && form->GetType() != NS_FORM_OBJECT)
>+      return NS_OK;
>+    findFormParent = findFormParent->GetParent();
>+  }
What about contentEditable or designMode="on", do those need special handling?

>-    nsCOMPtr<nsIDOMNode> curr = selectionStart;
>+    nsCOMPtr<nsIDOMNode> curr = selectionStartNode;
>     nsIDOMNode* next;
> 
>     while (curr) {
>       curr->GetNextSibling(&next);
> 
>       if (next) {
>-        selectionStart = dont_AddRef(next);
>+        selectionStartNode = dont_AddRef(next);
>         break;
>       }
This could use some .swap(). Though, not your code.

>+NS_IMETHODIMP
>+nsDOMDataTransfer::GetData(const nsAString& aFormat, nsIVariant** aData)
>+{
>+  *aData = nsnull;
>+
>+  if (aFormat.IsEmpty())
>+    return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
"The getData(format) method must return the data that is associated with the type format, if any, and must return the empty string otherwise."
So I'd say empty string should be returned.

>+      nsCOMPtr<nsIWritableVariant> variant = do_CreateInstance(NS_VARIANT_CONTRACTID);
>+      NS_ENSURE_TRUE(variant, NS_ERROR_OUT_OF_MEMORY);
>+
>+      variant->SetAsWString(stringdata.get());
>+      NS_IF_ADDREF(*aData = variant);
NS_ADDREF, not NS_IF_ADDREF


>+nsDOMDataTransfer::CheckConvertData(PRBool aExpectingList,
>+                                    nsIVariant* aInData,
>+                                    nsIVariant** aData)
>+{
>+  *aData = aInData;
*aData points now to some value...
>+
>+  nsCOMPtr<nsIWritableVariant> variant;
>+
>+  // if we are expecting a list but the data is not an array, create a new
>+  // array with a single element
>+  PRUint16 type;
>+  aInData->GetDataType(&type);
>+  if (aExpectingList) {
>+    if (type != nsIDataType::VTYPE_ARRAY &&
>+        type != nsIDataType::VTYPE_EMPTY_ARRAY) {
>+      variant = do_CreateInstance(NS_VARIANT_CONTRACTID);
>+      NS_ENSURE_TRUE(variant, NS_ERROR_OUT_OF_MEMORY);
Now the method may return an error code, but return value has been set, but not addrefed


>+NS_IMETHODIMP
>+nsDOMDataTransfer::SetData(const nsAString& aFormat, nsIVariant* aData)
>+{
>+  NS_ENSURE_ARG(aData);
>+
>+  if (aFormat.IsEmpty())
>+    return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
>+
>+  if (mReadOnly)
>+    return NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR;
>+
>+  // don't allow non-chrome to add file data
>+  // XXX perhaps this should also limit any non-string type as well
>+  if ((aFormat.EqualsLiteral("application/x-moz-file-promise") ||
>+       aFormat.EqualsLiteral("application/x-moz-file")) &&
>+       !nsContentUtils::IsCallerChrome()) {
>+    return NS_ERROR_DOM_SECURITY_ERR;
>+  }
Could it be possible to have a whitelist for non-privileged callers?

Comment 35

[Neil Deakin]

> What about contentEditable or designMode="on", do those need special handling?

Perhaps, but I haven't changed any behaviour with respect to that as far as I know.

> Could it be possible to have a whitelist for non-privileged callers?

The formats can be any string value, so a whitelist couldn't be used. The check here is only an added level of checking to avoid data being made available to a site during an accidental drops.

Comment 36

[Neil Deakin]

(In reply to comment #33)
> I'm not sure about this "list" approach. Seems to me that it's basically
> introducing extra APIs without new functions, so you lose type checking.
> 

Yeah, i don't think the list approach turned out too good either.

> I think either we persuade HTML5 to accept multi-item APIs, or we add them
> ourselves as moz* extensions, or we don't support them at all.

I don't think any of those options is suitable. Perhaps if we remove the extra 'list' handling and just allow the data stored to be variants instead of strings, I think that should be sufficient.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That still creates a Mozilla extension that's incompatible with the HTML5 spec in a non-obvious way. What's wrong with the moz vendor prefix?

Comment 38

[Neil Deakin]

Attachment: updated patch

Can't really bring myself to accept the ugliness of 'moz' prefixes, but here is an updated patch with this.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree it's ugly, but I think it's the right and expedient thing to do.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  *aDraggable = AttrValueIs(kNameSpaceID_None, nsGkAtoms::draggable,
+                             nsGkAtoms::_true, eIgnoreCase);

Is this the right thing? It's not clear to me what HTML5 means by "the 'draggable' content attribute has the state 'true'".

All my previous issues are OK, except the one about using bitmasks instead of strings for mDropEffect/mEffectAllowed. The last comment on that issue was mine:
> Why can't it pull in nsDOMDataTransfer.h? You could even add [noscript] methods
> to nsIDOMDataTransfer if necessary. I don't understand why this wouldn't be
> simpler.

Comment 41

[Neil Deakin]

(In reply to comment #40)
> +  *aDraggable = AttrValueIs(kNameSpaceID_None, nsGkAtoms::draggable,
> +                             nsGkAtoms::_true, eIgnoreCase);
> 
> Is this the right thing? It's not clear to me what HTML5 means by "the
> 'draggable' content attribute has the state 'true'".

It means when draggable="true"

> > Why can't it pull in nsDOMDataTransfer.h? You could even add [noscript] methods
> > to nsIDOMDataTransfer if necessary. I don't understand why this wouldn't be
> > simpler.

I investigated this, but it doesn't result in less code. I can make a patch with this change if you like.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 333256 [details] [diff] [review]
updated patch 

>diff --git a/content/events/src/nsDOMDragEvent.cpp
I think you need to add nsDOMDragEvent to cycle collection, because it has
mEvent->dataTransfer
modifying NS_IMPL_CYCLE_COLLECTION_TRAVERSE/UNLINK_BEGIN(nsDOMEvent) in nsDOMEvent.cpp
should be enough.

>+nsDOMDragEvent::InitDragEvent(const nsAString & aType,
>+                              PRBool aCanBubble,
>+                              PRBool aCancelable,
>+                              nsIDOMAbstractView* aView,
>+                              PRInt32 aDetail,
>+                              nsIDOMDataTransfer* aDataTransfer)
>+{
>+  if (mEventIsInternal && mEvent) {
>+    nsDragEvent* dragEvent = static_cast<nsDragEvent*>(mEvent);
>+    dragEvent->dataTransfer = aDataTransfer;
>+  }
>+
>+  return nsDOMUIEvent::InitUIEvent(aType, aCanBubble, aCancelable, aView, aDetail);
Call InitUIEvent first and check its return value - it may fail, for example because
of security reason!

>+nsDOMDragEvent::GetDataTransfer(nsIDOMDataTransfer** aDataTransfer)
...
>+  // for synthetic events, just use the supplied data transfer object or
>+  // create an empty one
>+  if (mEventIsInternal) {
>+    dragEvent->dataTransfer = new nsDOMDataTransfer();
>+    NS_ENSURE_TRUE(dragEvent->dataTransfer, NS_ERROR_OUT_OF_MEMORY);
>+
>+    NS_ADDREF(*aDataTransfer = dragEvent->dataTransfer);
>+    return NS_OK;
>+  }
Why create an empty datatransfer? If the event is initialized properly, 
dragEvent->dataTransfer points to some valid value (and null is a valid value).

>+  // For draggesture and dragstart events, the data transfer object is
>+  // created before the event fires, so it should already be set. For other
>+  // drag events, get the object from the drag session.
Just asking, would it be hard to always initialize the data transfer object?

>diff --git a/toolkit/content/nsDragAndDrop.js b/toolkit/content/nsDragAndDrop.js
A toolkit peer should look these changes

>+class nsDragEvent : public nsMouseEvent
>+{
>+public:
>+  nsDragEvent(PRBool isTrusted, PRUint32 msg, nsIWidget *w,
>+              reasonType aReason)
>+    : nsMouseEvent(isTrusted, msg, w, NS_DRAG_EVENT, aReason)
>+  {
>+    if (msg == NS_DRAGDROP_EXIT_SYNTH ||
>+        msg == NS_DRAGDROP_LEAVE_SYNTH ||
>+        msg == NS_DRAGDROP_END) {
>+      flags |= NS_EVENT_FLAG_CANT_CANCEL;
>+    }
>+  }
>+
>+  nsCOMPtr<nsIDOMDataTransfer> dataTransfer;
> };
I guess nsDragEvents are always eReal, so perhaps the last
parameter is useless and you could pass nsMouseEvent::eReal to nsMouseEvent
ctor.



>diff --git a/widget/src/xpwidgets/nsPrimitiveHelpers.cpp b/widget/src/xpwidgets
...
> nsPrimitiveHelpers :: CreateDataFromPrimitive ( const char* aFlavor, nsISupports* aPrimitive, 
>                                                    void** aDataBuff, PRUint32 aDataLen )
> {
>+  *aDataBuff = nsnull;
>+
>   if ( !aDataBuff )
>     return;
This doesn't make sense. Add *aDataBuff = nsnull; after |if|

Comment 43

[Neil Deakin]

> Why create an empty datatransfer? If the event is initialized properly, 
> dragEvent->dataTransfer points to some valid value (and null is a valid value).

The specification doesn't provide a means of creating a data transfer object. I've asked about it on the list.

> >+  // For draggesture and dragstart events, the data transfer object is
> >+  // created before the event fires, so it should already be set. For other
> >+  // drag events, get the object from the drag session.
> Just asking, would it be hard to always initialize the data transfer object?

I'm not clear what you're asking here. When and what would it be initialized to?

Comment 44

[Neil Deakin]

Comment on attachment 333256 [details] [diff] [review]
updated patch 

Additional review from Neil on the nsDragAndDrop.js changes.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #43)
> > Why create an empty datatransfer? If the event is initialized properly, 
> > dragEvent->dataTransfer points to some valid value (and null is a valid value).
> 
> The specification doesn't provide a means of creating a data transfer object.
> I've asked about it on the list.
Well, anyway, if event is initialized with null dataTransfer, ::GetDataTransfer
should return null.
 
> > >+  // For draggesture and dragstart events, the data transfer object is
> > >+  // created before the event fires, so it should already be set. For other
> > >+  // drag events, get the object from the drag session.
> > Just asking, would it be hard to always initialize the data transfer object?
> 
> I'm not clear what you're asking here. When and what would it be initialized
> to?
You say that datatranfer is created for some events before dispatch. Would it be
possible to create it for all events before dispatch?

Comment 46

[Neil Deakin]

(In reply to comment #45)
> > The specification doesn't provide a means of creating a data transfer object.
> > I've asked about it on the list.
> Well, anyway, if event is initialized with null dataTransfer, ::GetDataTransfer
> should return null.

I can have it do that too. It's not like synthetic events are that useful anyway.

> You say that datatranfer is created for some events before dispatch. Would it
> be
> possible to create it for all events before dispatch?

Possibly, but I do it this way to avoid extra work in the fairly common case where no-one retrieves the dataTransfer.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #46)
> Possibly, but I do it this way to avoid extra work in the fairly common case
> where no-one retrieves the dataTransfer.
ok, fine to me.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #41)
> I investigated this, but it doesn't result in less code. I can make a patch
> with this change if you like.

If you've already got that patch, sure I'd like to see it.

I apologise for being annoying about this, but it's hard to prove or disprove that an approach is simpler without one of us actually writing it.

Comment 49

[Neil Deakin]

Attachment: patch with integer fields

roc, here is the patch with integer fields as well as strings.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why not use an 8-element array {"none", "copy", "move", "copyMove", "link", "copyLink", "linkMove", "all"}? With that, GetDropEffect and GetEffectAllowed become trivial (GetEffectAllowed still needs one special case for UNINITIALIZED). Write a helper function to loop over the array (use EqualsASCII) and then SetEffectAllowed becomes trivial and SetDropEffect becomes trivial plus a check on the resulting effect type. I think that would make this version a clear win.

nsDOMDragEvent::GetDataTransfer can be simplified where we compute the actual action. Make a helper function to do the computation. Then it can just do
  if (aAction & aEffectAllowed)
    return aAction;
  if (aEffectAllowed & nsIDragService::DRAGDROP_ACTION_COPY)
    return nsIDragService::DRAGDROP_ACTION_COPY;
  if (aEffectAllowed & nsIDragService::DRAGDROP_ACTION_LINK)
    return nsIDragService::DRAGDROP_ACTION_LINK;
  if (aEffectAllowed & nsIDragService::DRAGDROP_ACTION_MOVE)
    return nsIDragService::DRAGDROP_ACTION_MOVE;
  return nsIDragService::DRAGDROP_ACTION_NONE;
Right? You should be able to use that in nsEventStateManager::PostHandleEvent as well.

Comment 51

[Neil Deakin]

Attachment: updated patch with integer constants

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 333931 [details] [diff] [review]
updated patch with integer constants

>   /**
>+   * Retrieve the current drag session, or null if no drag is currently occuring
>+   */
>+  static void GetDragSession(nsIDragSession** aDragSession);
This could actually return already_AddRefed<nsIDragSession>,
but doesn't matter much.

>+already_AddRefed<nsIContent>
>+nsTransferableFactory::FindParentLinkNode(nsIContent* inNode)
> {
>-  nsCOMPtr<nsIContent> content(do_QueryInterface(inNode));
>+  nsIContent* content = inNode;
>   if (!content) {
>     // That must have been the document node; nothing else to do here;
>     return nsnull;
>   }
> 
>   for (; content; content = content->GetParent()) {
>-    if (nsContentUtils::IsDraggableLink(content)) {
>-      nsIDOMNode* node = nsnull;
>-      CallQueryInterface(content, &node);
>-      return node;
>-    }
>+    if (nsContentUtils::IsDraggableLink(content))
>+      return content;
Do you miss NS_ADDREF here?

>+nsTransferableFactory::Produce(nsDOMDataTransfer* aDataTransfer,
...
>-  if (!outTrans || !mMouseEvent || !mFlavorDataProvider) {
>-    return NS_ERROR_FAILURE;
>-  }
>+  *aDragNode = nsnull;
... 
>         mHtmlString.Append(mTitleString);
>         mHtmlString.AppendLiteral("</a>");
>+
>+        *aDragNode = draggedNode;
...
>         } else {
>-          nodeToSerialize = draggedNode;
>+          nodeToSerialize = do_QueryInterface(draggedNode);
>         }
>+        *aDragNode = nodeToSerialize;
...
>       if (linkNode) {
>         mIsAnchor = PR_TRUE;
>         GetAnchorURL(linkNode, mUrlString);
>+        *aDragNode = linkNode;
...
>-  return ConvertStringsToTransferable(outTrans);
>+  NS_IF_ADDREF(*aDragNode);
>+
>+  // if there is no drag node, which will be the case for a selection, just
>+  // use the selection target node.
>+  return AddStringsToDataTransfer(
>+           *aDragNode ? *aDragNode : mSelectionTargetNode.get(), aDataTransfer);
>+}
I think it might be better to have nsIContent* dragNode and assign values temporarily
to that and assing non-null value to *aDragNode only just before successful return.
That way the method never returns non-addrefed non-null-*aDragNode - even if someone
adds some NS_ENSURE_TRUE or similar...


>+NS_IMETHODIMP
>+nsDOMDataTransfer::GetDropEffect(nsAString& aDropEffect)
>+{
>+  aDropEffect = NS_ConvertUTF8toUTF16(effects[mDropEffect]);
Couldn't you just use aDropEffect.AssignASCII(effects[mDropEffect]);?

>+NS_IMETHODIMP
>+nsDOMDataTransfer::SetDropEffect(const nsAString& aDropEffect)
>+{
>+  // the valid drop effects all just happen to have length 4 while the
>+  // invalid ones don't, so this is a handy shortcut
>+  if (aDropEffect.Length() != 4)
>+    return NS_OK;
This looks a bit ugly to me. This isn't performance critical code, so
you could just check that the for loops finds a value which is one of the
valid ones.

>+  for (PRUint32 e = 0; e < sizeof(effects) / sizeof (char *); e++) {
Doesn't NS_ARRAY_LENGTH work here?

>+nsDOMDataTransfer::GetEffectAllowed(nsAString& aEffectAllowed)
>+{
>+  if (mEffectAllowed == nsIDragService::DRAGDROP_ACTION_UNINITIALIZED)
>+    aEffectAllowed.AssignLiteral("uninitialized");
>+  else
>+    aEffectAllowed = NS_ConvertUTF8toUTF16(effects[mEffectAllowed]);
.AssignASCII()?

>+nsDOMDataTransfer::SetEffectAllowed(const nsAString& aEffectAllowed)
...
>+  for (PRUint32 e = 0; e < sizeof(effects) / sizeof (char *); e++) {
NS_ARRAY_LENGTH?

>+nsDOMDataTransfer::FillInExternalDragData(TransferItem& aItem, PRUint32 aIndex)
...
>+    const char* format = NS_ConvertUTF16toUTF8(aItem.mFormat).get();
>+    if (strcmp(format, "text/plain") == 0)
>+      format = kUnicodeMime;
>+    else if (strcmp(format, "text/uri-list") == 0)
>+      format = kURLDataMime;
This doesn't look safe. format points to some buffer inside a deleted object.
I think you need to do 
ConvertUTF16toUTF8 utf8format(aItem.mFormat);
const char* format = utf8format.get();
That is at least how ConvertUTF16toUTF8 used to work.
The new String Guide doesn't say anything about it, but the old one does
http://mxr.mozilla.org/mozilla-central/source/xpcom/string/doc/string-guide.html#faq_how_to_call_printf

>+class nsDOMDataTransfer : public nsIDOMDataTransfer,
>+  static char* effects[];
I think this should be called sEffects.

>+  // for synthetic events, just use the supplied data transfer object or
>+  // create an empty one
>+  if (mEventIsInternal) {
>+    NS_IF_ADDREF(*aDataTransfer = dragEvent->dataTransfer);
>+    return NS_OK;
>+  }
The comment is still wrong "or create an empty one"

This whole thing needs testing so we should get this landed ASAP

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+char* nsDOMDataTransfer::effects[] = {
+  "none", "copy", "move", "copyMove", "link", "copyLink", "linkMove", "all"
+};

It's actually a bit more efficient to write this as

static const char gEffectNames[8][9] = {
  "none", "copy", "move", "copyMove", "link", "copyLink", "linkMove", "all"
};

See Olli's comments about NS_ARRAY_LENGTH and AssignASCII.

+  // the action to something that is doesn'tallowed. For a copy, adjust to

Fix typo

+    newDataTransfer->SetDropEffectInt(FilterDropEffect(action, effectAllowed));
+      effectAllowed, FilterDropEffect(action, effectAllowed));

Not sure where that extra line came from. I guess it just needs to be deleted.

FilterDropEffect can be static. Then you can call it from nsEventStateManager::PostHandleEvent to compute the dropEffect from the action (pass "all" for aEffectAllowed). In fact I don't know why you aren't just moving "if (dataTransfer) GetEffectAllowedInt" up and then calling FilterDropEffect to do everything --- is it that we don't want to automatically adjust the action to an allowed action here? If not, why not?

Comment 54

[Neil Deakin]

Attachment: address comments

Comment 55

[Neil Deakin]

> FilterDropEffect can be static. Then you can call it from
> nsEventStateManager::PostHandleEvent to compute the dropEffect from the action
> (pass "all" for aEffectAllowed). In fact I don't know why you aren't just
> moving "if (dataTransfer) GetEffectAllowedInt" up and then calling
> FilterDropEffect to do everything --- is it that we don't want to automatically
> adjust the action to an allowed action here? If not, why not?
> 

FilterDropEffect needs to choose a default action, whereas PostHandleEvent needs to handle 'none' differently. It's possible there's a way here to share this code but I think I'd rather just get this done then spend too much time on it. If you can find a good solution here that would be great too.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 334312 [details] [diff] [review]
address comments

I don't think Olli needs to re-review this. Let's get this in!

Comment 57

[Neil Deakin]

There was also a request for the other Neil to review the toolkit parts as well. Also, there are minor changes to suite/mail patch in the other patch.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

can you get toolkit review from someone else, like Dao or even mconnor? It would be really great to get this landed for alpha 2.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 334312 [details] [diff] [review]
address comments

>+nsDOMDataTransfer::GetTransferables(nsISupportsArray** aArray)
...
>+      // the underlying drag code uses text/unicode, so use that instead of text/plain
>+      const char* format;
>+      if (formatitem.mFormat.EqualsLiteral("text/plain"))
>+        format = kUnicodeMime;
>+      else
>+        format = NS_ConvertUTF16toUTF8(formatitem.mFormat).get();
>+
>+      // if a converter is set for a format, set the converter for the
>+      // transferable and don't add the item
>+      nsCOMPtr<nsIFormatConverter> converter = do_QueryInterface(convertedData);
>+      if (converter) {
>+        transferable->AddDataFlavor(format);
>+        transferable->SetConverter(converter);
>+        continue;
>+      }
>+
>+      nsresult rv = transferable->SetTransferData(format, convertedData, length);
Another NS_ConvertUTF16toUTF8(foo).get() usage. Keep the string alive while using the value from .get()

Comment 60

[[e-mail]]

Comment on attachment 331670 [details] [diff] [review]
mail and suite patch

>     /* COVER YOUR EYES!! */
>-    var data = nsTransferable.get (this.getSupportedFlavours(),
>-                                   nsDragAndDrop.getDragData, true);
>-    data = data.first.first;
>-    
>-    var parsedLocation = this.parseLocation(data.data);
>+    var data = event.dataTransfer.getData("text/x-vloc");
>+    var parsedLocation = this.parseLocation(data);
This needs to be conditional for backward compatibility i.e. something like:
var data = "dataTransfer" in event ? event.dataTarnsfer.getData("text/x-vloc") : nsTransferable.get(this.getSupportedFlavours(), nsDragAndDrop.getDragData, true).first.first.data;

>+    dataTransfer.addElement(event.originalTarget.localName == "treechildren" ?
>+                            event.originalTarget : event.target);
No need for a condition, we should always be dragging tree.body here.

>+  var dt = event.dataTransfer;
You called this dataTransfer in other code, please stick with that naming.

Comment 61

[Neil Deakin]

Attachment: This is the manual test, updated to the latest api

This test  should be used with the 'outer chrome test' attachment earlier.

Comment 62

[[e-mail]]

Comment on attachment 331670 [details] [diff] [review]
mail and suite patch

Enn said on IRC that he would update the patch.

Comment 63

[Neil Deakin]

Attachment: Updated suite/mail patch

Comment 64

[Neil Deakin]

Attachment: final patch ready for checkin!

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 30d900751ca9.

Comment 66

[Dave Camp (:dcamp)]

This was causing unit test failures on linux:

*** 14664 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_dragstart.html | DataTransfer after dragstart event
*** 14665 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_dragstart.html | Error thrown during test: dt is null - got 0, expected 1

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1219188079.1219193156.13687.gz#err3

Backed out as changeset aa14280d31fb

Comment 67

[Dave Camp (:dcamp)]

It's failing on osx too, so not linux-specific.

Comment 68

[Neil Deakin]

The Mac issue doesn't look related. It looks more like just a focus issue on the machine.

Comment 69

[Neil Deakin]

OK, the failing tests are actually just problems with the tests.

The Linux issue is caused because drag and drop is asynchronous on Linux so the test continues even though the drag hasn't started yet.

The issues on Mac/Linux are caused because I changed EventUtils.js to use ClientRects instead of box objects and some computations rely on the slightly different values. I had fixed that but it turns out it I had put the fix in with a different build, so those changes weren't included.

So I'll put up a patch later today with just the changes to the tests needed.

Comment 70

[Neil Deakin]

Attachment: fix up coordinate calculations and drag test on Linux

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd like to check this in but I don't have a mail/suite repo set up. Will they burn if I just check in the main patch, or is there enough compatibility that nothing breaks when I just check in the main patch?

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suppose I can just pull from comm-central and check in there. Maybe.

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed main patch as changeset 6958399a2eb1.

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed mail/suite patch as changeset 1aa84851a365 in comm-central.

Comment 75

[Neil Deakin]

I haven't actually fixed this on Windows yet, so this isn't really a suitable time to be checking in.

Comment 76

[Jim Jeffery not reading bug-mail 1/2/11]

Uh, if this is not fixed on windows per comment #75, then why wasn't the bug re-opened ?

Comment 77

[Jim Jeffery not reading bug-mail 1/2/11]

This may have caused https://bugzilla.mozilla.org/show_bug.cgi?id=452083

Comment 78

[Boris Zbarsky [:bzbarsky]]

I backed this out again, which fixed the tinderbox orange.

Comment 79

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #77)
> This may have caused https://bugzilla.mozilla.org/show_bug.cgi?id=452083
> 

backout seems to have fixed bug 452083 

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #75)
> I haven't actually fixed this on Windows yet, so this isn't really a suitable
> time to be checking in.

Why was checkin-needed on the bug, then?

Comment 81

[Neil Deakin]

Attachment: fix to test

The additional fixes here are to the window_tooltip.xul test. One is that borders were being included and shouldn't have been, which affected Windows, (and Linux although I'm quite sure it didn't have a problem before)

The additional errors on Linux I don't see on my machine. I think this may because some coordinates are getting rounded down because ClientRect holds floats yet nsIDOMWindowUtils::SendMouseEvent takes integer arguments. So instead I use Math.floor to round values. This problem affected another test and solved the problem so I think this should work here.

Comment 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Great! I hope you can reland this soon.

Comment 83

[Neil Deakin]

Checked in and tests pass on all platforms!

Comment 84

[Jim Jeffery not reading bug-mail 1/2/11]

Tests may be fixed, but Drag&Drop is busted again in this hourly:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/20080827061801 Minefield/3.1a2pre Firefox/3.0 ID:20080827061801

D7D of bookmarks seems to work, but draging a link to a tab, or the AddressBar does not work. 

Comment 85

[Neil Deakin]

Works fine for me here on Mac. Please file a separate bug with steps to reproduce.

Comment 86

[Neil Deakin]

There is a manual test needed here that we should also do something with.

Comment 87

[pal-moz]

regression or intended ?
http://forums.mozillazine.org/viewtopic.php?p=4297565#p4297565

Comment 88

[Marcia Knous [:marcia]]

Neil: I will add the manual testcase to the Litmus suite.

Comment 89

[Honza Bambas (:mayhemer)]

Could this bug land cause behavior described in bug 296528 comment 20 ?

Comment 90

[Bill Gianopoulos [:WG9s]]

This appears to have cause the regression reported in bug 455176.