533460 - Allow custom panels/windows to be used as drag/drop feedback images

Status: RESOLVED FIXED

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

Description

[Neil Deakin]

Sometimes, more complex drag feedback images are useful. Other browsers, for instance, use special feedback when dragging tabs. This allows the feedback image to resize or change during the drag.

I think this would be handled by creating a native window and changing its location during the drag. To do this, we can support using a <panel> as an image, as in:

dataTransfer.setDragImage(somePanelElement, 0, 0)

This shouldn't be too hard; the tricky part is making sure that mouse events are passed to whatever is underneath the panel instead.

Comment 1

[Neil Deakin]

Attachment: Patch: proof of concept

This patch adds the popup displaying part of a possible implementation on Windows and Mac. I don't know how to implement this on Linux -- it requires having some means of getting the mouse position even when over another application.

On Mac, the default drag feedback box appears. This likely would need to be replaced with some blank image.

Mouse coordinate handling at the widget level will still need to be implemented so that mouse movement events do not target the popup instead of what is underneath it. I don't know the specifics of how this would be done.

Comment 2

[Neil Deakin]

Attachment: Example usage

Comment 3

[Frank Yan (:fryn)]

(In reply to comment #1)
> Created attachment 494775 [details] [diff] [review]
> Patch: proof of concept

Using this patch, I am able to implement all the primary features of bug 455694 for Windows and Mac. I'd like to get my patch in for beta 9 or 10. How close is this proof-of-concept patch to something that we could land?

> This patch adds the popup displaying part of a possible implementation on
> Windows and Mac. I don't know how to implement this on Linux -- it requires
> having some means of getting the mouse position even when over another
> application.

Until this can be done for Linux, we'll have to stick with a blank tab drag image on that platform for the tab detach feedback.

> On Mac, the default drag feedback box appears. This likely would need to be
> replaced with some blank image.

It'd be fantastic if you could do this for the final patch.

> Mouse coordinate handling at the widget level will still need to be implemented
> so that mouse movement events do not target the popup instead of what is
> underneath it. I don't know the specifics of how this would be done.

While improved event targeting would definitely make detecting the node over which the mouse is hovering easier, I was able implement everything for bug 455694 using just the proof-of-concept patch.

The proof-of-concept patch causes building on Maemo to fail with the following output excerpt:
../../staticlib/components/libwidget_qt.a(nsDragService.o): In function `.LANCHOR0':
nsDragService.cpp:(.data.rel.ro+0x58): undefined reference to `nsDragService::DragMoved(int, int)'
/scratchbox/compilers/cs2007q3-glibc2.5-arm7/bin/../lib/gcc/arm-none-linux-gnueabi/4.2.1/../../../../arm-none-linux-gnueabi/bin/ld: libxul.so: hidden symbol `nsDragService::DragMoved(int, int)' isn't defined

There is also a a11y mochitest failure. I have yet to determine whether it was caused by the patch for this bug or my patch for bug 455694.

Comment 4

[Neil Deakin]

(In reply to comment #3)
> Using this patch, I am able to implement all the primary features of bug 455694
> for Windows and Mac. I'd like to get my patch in for beta 9 or 10. How close is
> this proof-of-concept patch to something that we could land?

It isn't close at all. Event handling doesn't work. After a brief look at your patch in bug 455694 (and testing it), I would guess that you are relying on it not working. Since you're placing the drag image at (0, 0) it isn't noticeable, but it might be if the image was centred over the mouse. That is, all events are sent to the panel instead of what is underneath it.

Comment 5

[Frank Yan (:fryn)]

(In reply to comment #4)

My patch (patch v1) isn't at all dependent on event targeting not working. The only things it does to accommodate the issue are placing the drag image at (0, 0) and applying a bit of CSS to prevent the panel's contents from being directly under the cursor. Once event targeting is fixed, my patch would still work, and it would only require a small change to center the drag image under the cursor.

I would argue that, since tab animations are a highly wanted feature and major UX win, landing this for chrome code, even with incorrect event handling, would be advantageous.

Comment 6

[Neil Deakin]

Attachment: patch 2

This patch adds support such that events can be ignored on the dragged panels using <panel allowevents="false">

This is implemented on Windows and Mac.

Comment 7

[Neil Deakin]

Attachment: This patch adds Linux support

Some minor flickering occurs the first time the popup is dragged on Linux.

Comment 8

[Neil Deakin]

The change to gtk2/nsWindow.cpp isn't needed.

Also, add a type attribute (set to any value) to the popup otherwise it doesn't work the first time dragging on Linux.

Comment 9

[Frank Yan (:fryn)]

Thanks for working on this, Neil. I'm taking today and Friday off, but I'll get to this and its application in tab drag&drop animations on Saturday.

Comment 10

[Frank Yan (:fryn)]

Comment on attachment 520652 [details] [diff] [review]
This patch adds Linux support

Neil, thanks for working on this.

However, this patch doesn't seem to compile successfully on Windows.
Both tryserver and my local machine came up with:
*** [nsCSSFrameConstructor.obj] Error 2

The combination of this patch with the WIP tab animation builds but doesn't work on Linux; that could be my fault. I'll need to investigate further.

Comment 11

[Neil Deakin]

Probably nsMenuPopupFrame.h needs to be changed to 

  NS_IMETHOD BuildDisplayList(nsDisplayListBuilder*   aBuilder,
                              const nsRect&           aDirtyRect,
                              const nsDisplayListSet& aLists);

instead of returing nsresult.

Comment 12

[Frank Yan (:fryn)]

(In reply to comment #6)
> This patch adds support such that events can be ignored on the dragged panels
> using <panel allowevents="false">

(In reply to comment #8)
> The change to gtk2/nsWindow.cpp isn't needed.
> 
> Also, add a type attribute (set to any value) to the popup otherwise it doesn't
> work the first time dragging on Linux.

(In reply to comment #11)
> Probably nsMenuPopupFrame.h needs to be changed to 
> 
>   NS_IMETHOD BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>                               const nsRect&           aDirtyRect,
>                               const nsDisplayListSet& aLists);
> 
> instead of returing nsresult.

Oh wow. I had forgoten about these comments when importing your patch and writing my patch. My bad. I'll fix it up, test again, and report back.

Comment 13

[Frank Yan (:fryn)]

Attachment: updated patch to reflect changes noted in comments

With this updated patch, I still experience the problem that I described in bug 455694 comment 131.

Comment 14

[Frank Yan (:fryn)]

Attachment: patch updated to tip

Some platform changes bitrotted this, so uploading an updated version.

Check interdiff for changes, including:
|nsGkAtoms::menuPopupFrame|

Comment 15

[Neil Deakin]

The change the nsWidgetAtoms isn't needed anymore with this last patch.

I can help diagnose the Linux problem later this week.

Comment 16

[Frank Yan (:fryn)]

Neil, Dolske noted the following in bug 455694 comment 154:
> on my Windows debug build, when I
> drag the tab I get an assert in nsMenuPopupFrame.cpp:1147 about
> mPrefSize.width/.height not expected to be zero.

I see this too. Do you know what the problem is?

Comment 17

[Neil Deakin]

What is the code for the panel you're using? Is it zero-sized?

Comment 18

[Frank Yan (:fryn)]

(In reply to comment #17)
> What is the code for the panel you're using? Is it zero-sized?

Simply this:
+        <xul:panel type="drag-image" allowevents="false" hidden="true"
+                   class="tab-drag-image" anonid="tab-drag-image"/>

On dragstart, I immediately insert a <canvas/> as its child and unhide the <panel/> while keeping the <canvas/> at zero opacity until the tab is in a state to be detached.

Comment 19

[Neil Deakin]

Attachment: additional patch on top for testing

This patch applies on top of the other one. It doesn't perform the event grab when the popup is opened. Not sure if this is entirely correct but this should allow us to test if the issue on Linux is caused by this.

Comment 20

[Frank Yan (:fryn)]

(In reply to comment #19)
> Created attachment 539843 [details] [diff] [review] [review]
> additional patch on top for testing

The panel now opens, and the drag operation works normally.
The following is probably expected behavior given the diagnostic stuff for testing, but I'll include it just in case:
- the thumbnail isn't used; instead it seems to take a cropped screenshot
- the panel does not toggle in visibility when the drag continues such that the tab would be detached

Comment 21

[Neil Deakin]

Comment on attachment 539843 [details] [diff] [review]
additional patch on top for testing

Karl, can you comment on the change to nsWindow::DragInProgress here? Is there a reason the code is checking the two static variables there?

This change needed by this bug is caused because support is being added for using a panel as a drag image (such that it can be animated/resized/etc while dragging). The panel is opened upon dragstart, but the panel opening causes a grab to occur which cancels the drag immediately.

Also, is there a disadvantage to not applying the grab while dragging?

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 539843 [details] [diff] [review]
additional patch on top for testing

(In reply to comment #21)
> Comment on attachment 539843 [details] [diff] [review] [review]
> additional patch on top for testing
> 
> Karl, can you comment on the change to nsWindow::DragInProgress here? Is
> there a reason the code is checking the two static variables there?

I don't think so.
See bug 497498 comment 17.

> The panel is opened upon dragstart, but the panel opening
> causes a grab to occur which cancels the drag immediately.
> 
> Also, is there a disadvantage to not applying the grab while dragging?

I'm not clear exactly where the popup's grab is coming from, but the drag
widget (mGrabWidget) needs a grab to track the mouse.
The popup does not need mouse events and so should not grab the mouse
(but I guess that's unusual for a popup).


The OpenDragPopup() is a bit awkward after the gtk_drag_begin and
gtk_drag_set_icon_widget, because gtk_drag_begin will show a default cursor,
then gtk_drag_set_icon_widget will try to show our window (but doesn't really
know how to, or we don't react to the show correctly), so I expect there will
be flicker when we do the proper show in OpenDragPopup.

I think what should really happen here is that we register for the
"drag-begin" signal on mHiddenWidget, and OpenDragPopup() then
gtk_drag_set_icon_*() from there.
"if (needsFallbackIcon) gtk_drag_set_icon_default(context)" should be
unnecessary.

I expect StartDragSession() can be safely call before OpenDragPopup.

Comment 23

[Neil Deakin]

Karl, do you mean that the icon should always be set within the drag-begin event, even for the current non-popup drag image code?
 
If so, I'll file a different bug on this since it relates to the existing code.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to comment #23)
> Karl, do you mean that the icon should always be set within the drag-begin
> event, even for the current non-popup drag image code?

Yes, it should.

> If so, I'll file a different bug on this since it relates to the existing
> code.

OK, a different bug is fine thanks, but it is particularly crucial to gtk_drag_set_icon_widget because that will show our widget, and our widget code does not expect the window to be shown behind its back.

Comment 25

[Frank Yan (:fryn)]

Neil, is there a way to programmatically end a drag operation from chrome JS? I want to cancel the drag operation if a tab being dragged gets closed (by window.close or something) during the drag.

Comment 26

[Neil Deakin]

No, native platforms do not provide such a feature.

Comment 27

[Frank Yan (:fryn)]

Neil, I pushed this to the UX branch for testing, and this seems to break Linux Qt builds. While it's not of high priority to me, I suppose it's something we should fix.

Log: http://tinderbox.mozilla.org/showlog.cgi?log=UX/1308220837.1308223171.14704.gz

Thanks for all your work on this!

Comment 28

[Frank Yan (:fryn)]

Neil, I was debugging why dragging tabs with my tab animation patch felt heavy and laggy, and it turns out that dragover (and drag) events are not fired frequently enough. Using mousemove provides much smoother movement, but if I were to switch to plain mouse events, I'd have to do some hacking to enable mouse capture. Do you know if we are simply exposing the OS events to the DOM, or are we doing any throttling or something that would artificially cause this discrepancy?

Comment 29

[Neil Deakin]

We aren't doing anything special, but I can take a look.

Please do not consider a hacked up mouse event thing to be a viable solution. You'll be trading one issue for five new ones.

Comment 30

[Karl Tomlinson (:karlt)]

(In reply to comment #28)
> and it turns out that dragover (and drag) events are not
> fired frequently enough.

Are you sure that the Gecko is sending the appropriate responses to the OS events promptly?  i.e. that the delay is between the reply and the next event, not between the event and the reply?

Comment 31

[Frank Yan (:fryn)]

(In reply to comment #29)
> Please do not consider a hacked up mouse event thing to be a viable
> solution. You'll be trading one issue for five new ones.

Mouse capture isn't necessarily a hack -- <html:input/> elements have it normally (unless -moz-user-select is set to none) and now there's element.setCapture() -- but, yes, it would be difficult to implement robustly.

(In reply to comment #30)
> (In reply to comment #28)
> > and it turns out that dragover (and drag) events are not
> > fired frequently enough.
> 
> Are you sure that the Gecko is sending the appropriate responses to the OS
> events promptly?  i.e. that the delay is between the reply and the next
> event, not between the event and the reply?

Actually, I shouldn't have jumped to that conclusion. I was simply troubled by how I was hitting a wall in how responsive I could make the animations and, on slower machines, it is significantly more laggy than Chromium's, so I swapped out dragover for mousemove just as an experiment, and it was noticeably more responsive. I haven't yet logged or debugged anything from the widget level, so I'm not sure of anything else.

Comment 32

[Neil Deakin]

It doesn't seem slower when I test this, but maybe I'm not looking at the right thing. There is a bit of delay before the drag image actually appears.

I assumed that you were referring to Linux, but maybe you mean something else?
  
Are you sure that the mousemove handler is actually just not faster because it isn't doing the full amount of work?

Comment 33

[Frank Yan (:fryn)]

(In reply to comment #32)
> It doesn't seem slower when I test this, but maybe I'm not looking at the
> right thing. There is a bit of delay before the drag image actually appears.
> 
> I assumed that you were referring to Linux, but maybe you mean something
> else?

I'm not referring to that delay. I'm referring to all platforms and the lag of the tab movement while it's in a state to be moved not detached, so these comments should actually be in bug 455694. My bad.

> Are you sure that the mousemove handler is actually just not faster because
> it isn't doing the full amount of work?

I'm pretty sure. I'll finish putting something together for you to try out.

Well, this lag shouldn't block the landing of the patches for bug 455694, so if a patch for this bug and a patch for bug 666348 are ready, I'd like to get that in and resolve the performance gap asap but in a followup.

Comment 34

[Neil Deakin]

Frank, are you happy with the patch here at least or is there other issues you can think of?

Comment 35

[Frank Yan (:fryn)]

If the Linux issue of the drag operation being cancelled is fixed, yes, I'm happy with the patch here. Any further comments about the performance gap of tab rearranging will go in bug 455694 or a followup for it. Sorry for the confusion.

Comment 36

[Frank Yan (:fryn)]

I just removed the drag & drop API dependency from my latest patch for bug 455694, so I won't be needing this to land that anymore.

To be clear, it would still be awesome to have this API for other purposes, but it's no longer as urgent.

Thanks for all your help, Neil and Karl! :)

Comment 37

[Neil Deakin]

Attachment: updated patch

This patch is similar to the last one but adds a flag on gtk for drag widgets.

Comment 38

[Neil Deakin]

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

Actually I'm going to fix something up first before this should be reviewed.

Comment 39

[Neil Deakin]

Attachment: Updated patch

I simplified this so that only <panel type="drag"> needs to be used.

Comment 40

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

What are we going to do for tests here?

Who's going to use this if Frank isn't?

Comment 41

[Dão Gottwald [:dao]]

> Who's going to use this if Frank isn't?

We want to use it -- not using the drag-and-drop API has disadvantages. It just needs to perform well and work across platforms.

Comment 42

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

Comment on attachment 546191 [details] [diff] [review]
Updated patch

Review of attachment 546191 [details] [diff] [review]:
-----------------------------------------------------------------

Everything other than the mac/GTK stuff looks fine other than that.

::: widget/src/windows/nsDragService.cpp
@@ -297,5 @@
>  
>    // XXX not sure why we bother to cache this, it can change during
>    // the drag
>    mDragAction = aActionType;
> -  mDoingDrag  = PR_TRUE;

Not sure why you're removing this.

::: widget/src/windows/nsWindow.cpp
@@ +539,5 @@
>        parent = NULL;
> +
> +    if (aInitData->mIsDragPopup) {
> +      // This flag makes the window transparent to mouse events
> +      extendedStyle |= WS_EX_TRANSPARENT;

Really? That seems unexpected. Is this documented anywhere?

Comment 43

[Neil Deakin]

> >    mDragAction = aActionType;
> > -  mDoingDrag  = PR_TRUE;
> 
> Not sure why you're removing this.

Not sure either. Probably didn't mean to.


> > +    if (aInitData->mIsDragPopup) {
> > +      // This flag makes the window transparent to mouse events
> > +      extendedStyle |= WS_EX_TRANSPARENT;
> 
> Really? That seems unexpected. Is this documented anywhere?

http://msdn.microsoft.com/en-us/magazine/cc163698.aspx

Comment 44

[Neil Deakin]

(In reply to comment #42)
> >    // XXX not sure why we bother to cache this, it can change during
> >    // the drag
> >    mDragAction = aActionType;
> > -  mDoingDrag  = PR_TRUE;
> 
> Not sure why you're removing this.

Actually I removed it because it is set during the call to StartDragSession immediately following this.

Comment 45

[Dão Gottwald [:dao]]

(In reply to comment #33)
> > Are you sure that the mousemove handler is actually just not faster because
> > it isn't doing the full amount of work?
> 
> I'm pretty sure. I'll finish putting something together for you to try out.

Is this figured out / resolved yet?

Comment 46

[Frank Yan (:fryn)]

(In reply to comment #45)
> (In reply to comment #33)
> > > Are you sure that the mousemove handler is actually just not faster because
> > > it isn't doing the full amount of work?
> > 
> > I'm pretty sure. I'll finish putting something together for you to try out.
> 
> Is this figured out / resolved yet?

For bug 455694, I replaced the drag event handlers with plain mouse event handlers with only the minimal changes required to make things work, and everything immediately became faster. (See anecdotal evidence in bug 455694.) I don't have an standalone testcase though.

Comment 47

[Dão Gottwald [:dao]]

(In reply to comment #46)
> (In reply to comment #45)
> > (In reply to comment #33)
> > > > Are you sure that the mousemove handler is actually just not faster because
> > > > it isn't doing the full amount of work?
> > > 
> > > I'm pretty sure. I'll finish putting something together for you to try out.
> > 
> > Is this figured out / resolved yet?
> 
> For bug 455694, I replaced the drag event handlers with plain mouse event
> handlers with only the minimal changes required to make things work, and
> everything immediately became faster. (See anecdotal evidence in bug
> 455694.) I don't have an standalone testcase though.

How far apart are the mousemove / dragover implementations? Could you add a build-time switch for Neil to try out both? I think we might want such a switch anyway in order to take advantage of native drag and drop when it becomes an option.

Comment 48

[Frank Yan (:fryn)]

(In reply to comment #47)
> How far apart are the mousemove / dragover implementations? Could you add a
> build-time switch for Neil to try out both?

Now they are rather far apart, since after the switch to mouse events, I've made a bunch of improvements.

> I think we might want such a
> switch anyway in order to take advantage of native drag and drop when it
> becomes an option.

I don't think we would ever want to use the drag&drop API for this at all, because there is no reason another application or instance of Firefox should ever accept the drop of a Firefox tab. It's a fundamental unit of the application, and we are providing visual direct manipulation of the tab during the tab, rather than dragging a copy of the data that the tab holds, which is what the drag&drop API was designed to do.

For example, if we were to use drag&drop API again, and some other application or Firefox instance were to accept the type "application/x-moz-tabbrowser-tab", it might cause the application to create some representation of its contained data, while Firefox also creates a window with the tab in the same location. That doesn't make sense. Other applications shouldn't know about the drag occurring at all.

To copy the tab's URL to another application, we enable the favicon and location bar text to be dragged. Discoverability and usability in that realm can certainly be improved, and we definitely need better sharing UI, but that's a whole other problem.

Comment 49

[Dão Gottwald [:dao]]

> For example, if we were to use drag&drop API again, and some other
> application or Firefox instance were to accept the type
> "application/x-moz-tabbrowser-tab", it might cause the application to create
> some representation of its contained data,

I don't see why that would ever happen.

Using mouse events for this is rather a hack. Real drag and drop would solve issues like bug 455694 comment 210 and the second point of bug 455694 comment 219.

Comment 50

[Notlost]

Today's nightly incorporates the patch, or something like it.

Comment 51

[Josh Aas]

Comment on attachment 546191 [details] [diff] [review]
Updated patch

Review of attachment 546191 [details] [diff] [review]:
-----------------------------------------------------------------

Only really looked at changes to *.mm files.

Comment 52

[Karl Tomlinson (:karlt)]

Comment on attachment 546191 [details] [diff] [review]
Updated patch

Function names in the diff would make this easier to review.

Beware that since bug 624329, NS_MOVE events are not dispatched for these
popup windows.  I don't know whether their position is needed, but I'm just
pointing out that views etc that record the screen position won't be updated.

I thought the appropriate place to call OpenDragPopup was from the drag-begin
signal handler before gtk_drag_set_icon_widget (Comment 22 and 24).  Is there
a reason why you choose not to do that?

>+  PRBool                  ShouldApplyThemeRegion();

Left over from something else perhaps?
This method is neither used nor defined AFAICS.

> PRBool
> nsWindow::DragInProgress(void)
> {
>+    nsCOMPtr<nsIDragService> dragService = do_GetService(kCDragServiceCID);
>+    nsCOMPtr<nsIDragSession> dragSession;
>+    dragService->GetCurrentSession(getter_AddRefs(dragSession)); 
>+    if (dragSession)
>+      return PR_TRUE;
>+
>     // sLastDragMotionWindow means the drag arrow is over mozilla
>     // sIsDraggingOutOf means the drag arrow is out of mozilla
>     // both cases mean the dragging is happenning.
>     return (sLastDragMotionWindow || sIsDraggingOutOf);
> }

This makes sLastDragMotionWindow and sIsDraggingOutOf now unnecessary.
I guess that can be dealt with separately, but I'm curious about what
necessitated the change.  Why is CaptureRollupEvents being called?

>     nsPresContext* pc;
>     nsRefPtr<gfxASurface> surface;
>     if (mHasImage || mSelection) {
>       DrawDrag(mSourceNode, mSourceRegion, mScreenX, mScreenY,
>                &dragRect, getter_AddRefs(surface), &pc);
>     }
> 
>-    if (surface) {
>-        PRInt32 sx = mScreenX, sy = mScreenY;
>-        ConvertToUnscaledDevPixels(pc, &sx, &sy);
>+    PRInt32 sx = mScreenX, sy = mScreenY;
>+    ConvertToUnscaledDevPixels(pc, &sx, &sy);

The ConvertToUnscaledDevPixels call passes a pc that may be uninitialized.

(I can't make sense of the units for mImageX/Y, mScreenX/Y, dragRect, and
offsetX/Y here, but that's not new to this patch.)

>+NS_IMETHODIMP
>+nsBaseDragService::DragMoved(PRInt32 aX, PRInt32 aY)
>+{
>+  if (mDragPopup) {
>+    nsIFrame* frame = mDragPopup->GetPrimaryFrame();
>+    if (frame && frame->GetType() == nsGkAtoms::menuPopupFrame) {
>+      (static_cast<nsMenuPopupFrame *>(frame))->MoveTo(aX - mImageX, aY - mImageY, PR_TRUE);
>+    }
>+  }
>+
>+  return NS_OK;
>+}

Can you do something to indicate that this implementation is not suitable for
the GTK port, please, because the GTK DND implementation already moves the
popup.  (I know this is not called in the GTK port, but I imagine "dragMoved"
could end up used for other purposes such as recording the last mouse point.)

Comment 53

[Neil Deakin]

> This makes sLastDragMotionWindow and sIsDraggingOutOf now unnecessary.
> I guess that can be dealt with separately, but I'm curious about what
> necessitated the change.  Why is CaptureRollupEvents being called?

I think that change may not be necessary for this bug any more, although it will be made anyway for bug 497498.

Comment 54

[Neil Deakin]

Attachment: address comments

Comment 55

[Karl Tomlinson (:karlt)]

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

>     if (mHasImage || mSelection) {
>       DrawDrag(mSourceNode, mSourceRegion, mScreenX, mScreenY,
>                &dragRect, getter_AddRefs(surface), &pc);
>     }
>+    else {
>+      nsIPresShell* presShell = GetPresShellForContent(mSourceNode);
>+      if (!presShell)
>+        return;
> 
>-    if (surface) {
>-        PRInt32 sx = mScreenX, sy = mScreenY;
>-        ConvertToUnscaledDevPixels(pc, &sx, &sy);
>+      pc = presShell->GetPresContext();
>+    }
> 
>-        PRInt32 offsetX = sx - dragRect.x;
>-        PRInt32 offsetY = sy - dragRect.y;
>+    PRInt32 sx = mScreenX, sy = mScreenY;
>+    ConvertToUnscaledDevPixels(pc, &sx, &sy);
>+
>+    PRInt32 offsetX = sx - dragRect.x;
>+    PRInt32 offsetY = sy - dragRect.y;
>+
>+    // If a popup is set as the drag image, use its widget. Otherwise, use
>+    // the surface that DrawDrag created.
>+    if (mDragPopup) {
>+        GtkWidget* gtkWidget = nsnull;
>+        nsIFrame* frame = mDragPopup->GetPrimaryFrame();
>+        if (frame) {
>+            // DrawDrag ensured that this is a popup frame.
>+            nsCOMPtr<nsIWidget> widget = frame->GetNearestWidget();
>+            if (widget) {
>+                gtkWidget = (GtkWidget *)widget->GetNativeData(NS_NATIVE_SHELLWIDGET);
>+                if (gtkWidget) {
>+                    OpenDragPopup();
>+                    gtk_drag_set_icon_widget(aContext, gtkWidget, offsetX, offsetY);
>+                }
>+            }
>+        }
>+    }
>+    else if (surface) {

>+  nsIPresShell* GetPresShellForContent(nsIDOMNode* aDOMNode);

I don't think GetPresShellForContent is necessary.
AIUI mDragPopup is only set if DrawDrag is called.
Similarly for "surface".
How about returning early if (!(mHasImage || mSelection))?

Comment 56

[Neil Deakin]

Attachment: Don't call GetPresShellForContent

Comment 57

[Joe Wilson]

if the patch got positive review - why doesn't it land?

Comment 58

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/f86747fb659e

Comment 59

[Nickolay_Ponomarev]

dev-doc-needed: https://developer.mozilla.org/En/DragDrop/DataTransfer#setDragImage%28%29 ?

Comment 60

[Boris Zbarsky [:bzbarsky]]

Attachment: Aurora patch

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 564982 [details] [diff] [review]
Aurora patch

Wrong bug.

Comment 62

[Eric Shepherd [:sheppy]]

Initial reference documentation updates:

https://developer.mozilla.org/en/XUL/Attribute/panel.type
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDragService#dragMoved%28%29

Can someone point to code that uses this so I can write an explanation of how to use this? Do you just pass a reference to the XUL panel to invokeDragSessionWithImage(), as the aImage parameter, and all the magic happens for you?

Comment 63

[Neil Deakin]

You pass the panel to event.dataTransfer.setDragImage. It should likely be added as a short section at the end of https://developer.mozilla.org/En/DragDrop/Drag_Operations#Setting_the_Drag_Feedback_Image

We should probably document that using the drag service directly should be considered deprecated outside of mozilla internal code. All of the functionality is available via the dataTransfer drag/drop api. The one exception is calling getCurrentSession() to check if a drag is currently occurring as there isn't a way to do that otherwise.

Comment 64

[Eric Shepherd [:sheppy]]

Documentation completed:

https://developer.mozilla.org/En/DragDrop/Drag_Operations#Using_XUL_panels_as_drag_images

Mentioned on Firefox 9 for developers.

Also added a note to the nsIDragService docs urging use of the standard API instead.