nsIDOMDataTransfer's dropEffect not updated after drag operation completes

VERIFIED FIXED in mozilla1.9.1b1

Status

()

VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
mozilla1.9.1b1
x86
Windows Vista
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 339262 [details] [diff] [review]
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
Attachment #339262 - Flags: review?(enndeakin)

Comment 1

10 years ago
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.
You don't get dragover/enter events outside of the browser window.
(Assignee)

Comment 3

10 years ago
>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"?
(Assignee)

Comment 4

10 years ago
"last drag operation" -> "current drag operation"

Comment 5

10 years ago
> 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.
(Assignee)

Updated

10 years ago
Attachment #339262 - Flags: review?(enndeakin)
(Assignee)

Comment 6

10 years ago
>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

10 years ago
(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.
(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.
(Assignee)

Comment 9

10 years ago
Created attachment 339320 [details] [diff] [review]
nsDragService dropEffect update patch for win v.2

Rev 2. Sort of a one off specific to tab tear off feature, but it works.
Assignee: nobody → jmathies
(Assignee)

Updated

10 years ago
Attachment #339262 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #339320 - Flags: review?(enndeakin)

Comment 10

10 years ago
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.
Attachment #339320 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

10 years ago
Blocks: 455694
(Assignee)

Updated

10 years ago
No longer blocks: 455694
(Assignee)

Updated

10 years ago
Blocks: 225680
(Assignee)

Comment 11

10 years ago
Created attachment 340426 [details] [diff] [review]
nsDragService dropEffect update patch for win v.3
Attachment #339320 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
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)
Attachment #340426 - Flags: superreview?(roc)
(Assignee)

Updated

10 years ago
Flags: wanted-firefox3.1?
Comment on attachment 340426 [details] [diff] [review]
nsDragService dropEffect update patch for win v.3

+  PRBool mSentLocalDropEvent;

PRPackedBool in structs please
Attachment #340426 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 14

10 years ago
Created attachment 340448 [details] [diff] [review]
nsDragService dropEffect update patch for win v.4
[Checkin: Comment 15]
Attachment #340426 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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
Attachment #340448 - Attachment description: nsDragService dropEffect update patch for win v.4 → nsDragService dropEffect update patch for win v.4 [Checkin: Comment 15]
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Component: Tabbed Browser → Widget: Win32
Flags: wanted-firefox3.1?
Keywords: checkin-needed
Product: Firefox → Core
QA Contact: tabbed.browser → win32
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → mozilla1.9.1b1
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.