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.

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.

(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.

(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.

(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.

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.

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

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.

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 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.

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

(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.

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

Some platform changes bitrotted this, so uploading an updated version. Check interdiff for changes, including: |nsGkAtoms::menuPopupFrame|

The change the nsWidgetAtoms isn't needed anymore with this last patch. I can help diagnose the Linux problem later this week.

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?

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

(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.

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.

(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 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 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.

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.

(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.

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.

No, native platforms do not provide such a feature.

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!

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?

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.

(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?

(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.

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?

(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.

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

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.

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! :)

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

Comment on attachment 544823 [details] [diff] [review] updated patch Actually I'm going to fix something up first before this should be reviewed.

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

What are we going to do for tests here? Who's going to use this if Frank isn't?

> 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 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?

> > 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

(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.

(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?

(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.

(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.

(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.

> 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.

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

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 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.)

> 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 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))?

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

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

Comment on attachment 564982 [details] [diff] [review] Aurora patch Wrong bug.

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?

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.

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.