455884 - nsIDOMDataTransfer's dropEffect not updated after drag operation completes

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

Attachment: nsDragService dropEffect update patch for win v.1

In trying to get tab tear off to work (bug 225680), information regarding the final result of a drag operation is needed. It looks like the current html5 draft would accept the user agent setting nsIDOMDataTransfer's dropEffect attrib to the final result of the operation prior to the dragend event.

Currently in win's nsDragService, the result of the drag operation is ignored.  This patch handles updating dropEffect with the final result so that the source can decide whether or not to open a new window. 

http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#current3

Comment 1

[Neil Deakin]

Is Windows changing the drop effect between the dragover and drop effects to something else?

The drop effect that was set during the last dragenter/dragover event is the actual drop effect that should be used. I'm not clear what the issue is here.

Comment 2

[Asaf Romano (gone)]

You don't get dragover/enter events outside of the browser window.

Comment 3

[Jim Mathies [:jimm]]

>Is Windows changing the drop effect between the dragover and drop effects to
>something else?
>
>The drop effect that was set during the last dragenter/dragover event is the
>actual drop effect that should be used. I'm not clear what the issue is here.

We update the native effect in nsNativeDragTarget, but I don't believe we mess with nsIDOMDataTransfer's dropEffect. 

The problem we're trying to solve is in order to do a tab tear off into a new window, Mano needs to know what the actual result of the operation was (not the "current drag operation" value but the actual result). Without that, he can't make a decision as to whether or not we should tear the tab off and create a new window.  The idea was to only create a new window when the drag result was DROPEFFECT_LINK and the drop landed outside the DOM.

This patch is generic, it doesn't pay attention to the target so I might have to update things so it only messes with dropEffect in the case where the drop occurs outside the document. The question is, under the spec, do we have the right to fill dropEffect with the actual result, or do we have to leave it set to the "last drag operation"?

Comment 4

[Jim Mathies [:jimm]]

"last drag operation" -> "current drag operation"

Comment 5

[Neil Deakin]

> You don't get dragover/enter events outside of the browser window.

OK, that makes sense then, but we should only update the drop effect when dropping outside the browser. (when mDoingDrag is false I think)

I do notice another bug though in that the implementations of EndDragSession are clearing the node and other data even when aDoneDrag is false.

(In reply to comment #3)
> This patch is generic, it doesn't pay attention to the target so I might have
> to update things so it only messes with dropEffect in the case where the drop
> occurs outside the document. The question is, under the spec, do we have the
> right to fill dropEffect with the actual result, or do we have to leave it set
> to the "last drag operation"?

It should be the last drop effect from a dragenter/dragover event.

Comment 6

[Jim Mathies [:jimm]]

>OK, that makes sense then, but we should only update the drop effect when
>dropping outside the browser. (when mDoingDrag is false I think)

>> occurs outside the document. The question is, under the spec, do we have the
>> right to fill dropEffect with the actual result, or do we have to leave it set
>> to the "current drag operation"?
>
>It should be the last drop effect from a dragenter/dragover event.

Just to confirm, you're saying placing the result in dropEffect is against the rules in cases where the drag ends outside the application?

Comment 7

[Neil Deakin]

(In reply to comment #6)
> Just to confirm, you're saying placing the result in dropEffect is against the
> rules in cases where the drag ends outside the application?

No, the opposite. This patch here is ok except it should only update the effect when a drop occurs on another application.

+  else if (winDropRes & DROPEFFECT_MOVE)
+      dropResult = DRAGDROP_ACTION_MOVE;
+  else if (winDropRes & DROPEFFECT_LINK)
+      dropResult = DRAGDROP_ACTION_LINK;

link should be checked before move.

Comment 8

[Asaf Romano (gone)]

(In reply to comment #3)
> >Is Windows changing the drop effect between the dragover and drop effects to
> >something else?
> >
> >The drop effect that was set during the last dragenter/dragover event is the
> >actual drop effect that should be used. I'm not clear what the issue is here.
> 
> We update the native effect in nsNativeDragTarget, but I don't believe we mess
> with nsIDOMDataTransfer's dropEffect. 
> 
> The problem we're trying to solve is in order to do a tab tear off into a new
> window, Mano needs to know what the actual result of the operation was (not the
> "current drag operation" value but the actual result). Without that, he can't
> make a decision as to whether or not we should tear the tab off and create a
> new window.  The idea was to only create a new window when the drag result was
> DROPEFFECT_LINK and the drop landed outside the DOM.

Only when it's _NONE.

Comment 9

[Jim Mathies [:jimm]]

Attachment: nsDragService dropEffect update patch for win v.2

Rev 2. Sort of a one off specific to tab tear off feature, but it works.

Comment 10

[Neil Deakin]

Comment on attachment 339320 [details] [diff] [review]
nsDragService dropEffect update patch for win v.2

>+    nsCOMPtr<nsIDOMNSDataTransfer> nsDataTransfer =
>+      do_QueryInterface(mDataTransfer);

'dataTransfer' rather than 'nsDataTransfer' otherwise it looks like a struct name.
 
>+  NS_IMETHOD SentDropEvent(PRBool aDoneDrop);

This should be in the protected section and just return 'void' not NS_IMETHOD.
Also, the argument isn't needed as you only set it to true. Actually, you could just not use a separate method and just set the field directly as it's only changed in one place. But if you do use a separate method, I think SetDroppedLocally or somesuch would be a better name.

Fix those up and this is OK.

Comment 11

[Jim Mathies [:jimm]]

Attachment: nsDragService dropEffect update patch for win v.3

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 340426 [details] [diff] [review]
nsDragService dropEffect update patch for win v.3

this patch contains a little bit of underground piping to support tab tear off to the desktop. (bug 225680)

Comment 13

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

Comment on attachment 340426 [details] [diff] [review]
nsDragService dropEffect update patch for win v.3

+  PRBool mSentLocalDropEvent;

PRPackedBool in structs please

Comment 14

[Jim Mathies [:jimm]]

Attachment: nsDragService dropEffect update patch for win v.4 [Checkin: Comment 15]

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 340448 [details] [diff] [review]
nsDragService dropEffect update patch for win v.4
[Checkin: Comment 15]

http://hg.mozilla.org/mozilla-central/rev/0483ea862e60

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

See bug 494795 for the remaining OS X and Linux part. Marking verified fixed based on the working tab tearing off on the desktop feature on Windows on 1.9.1.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090525 Shiretoko/3.5pre ID:20090525031033