494795 - tabs do not tear off unless you drag them vertically out of the tab strip

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

from bug 487263 comment 0 and bug 487263 comment 14:

>Create 3 tabs, drag the center tab along the tab bar to the left or right, 
>release outside the browser tab is not detached.

This occurs on OSX and Linux, but not w32. The problem seems to be that we're only detaching tabs if the mouse leaves the tabstrip vertically; if you drag just horizontally, or even if you pull down vertically and then go back into the tabstrip and exit it from one of the sides, we don't detach.

Blocks Firefox 3.5 release :(

Comment 1

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

Mano can you also fix the wrongly patched code (new window instead of new tab button) to drop it onto the new tab button in this bug? Then we should add bug 494026 to the dependency list.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

That other issue isn't blocking, fwiw, and I'm not sure that the codepaths are at all similar. I'd keep the fixes and patches separate.

Comment 3

[Asaf Romano (gone)]

Unfortunately, this is a bug in the widget level: dropEffect isn't updated when the drop happens outside of the window. This also explains why this is working fine on windows: Jim  fixed it in bug 455884. Back then, I assumed that "none" is always returned elsewhere. That assumption was wrong: the returned dropEffect is simply whatever was set by the last drag event in the same session.

Fortunately, I could easily fix it on mac, patch coming. I didn't look at the linux code yet, hopefully it would be straightforward, help is most welcome.

Comment 4

[Asaf Romano (gone)]

Attachment: cocoa patch (checked in)

Comment 5

[Asaf Romano (gone)]

Pushed to tryserver, but I'm heading to bed now. Please do test it (aromano mozilla com).

Comment 6

[Asaf Romano (gone)]

OK, I'm not going to work on the gtk part. Sorry, it's too complicated to work on it with no real gtk experience, no linux machine and a strong feeling that the platform-version issue Neil faced in bug 479995 would be a problem here as well.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 379620 [details] [diff] [review]
cocoa patch (checked in)

please ignore the printf.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Neil: welcome back from holiday; can you please consider this your number 1 priority?

Comment 9

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

(In reply to comment #5)
> Pushed to tryserver, but I'm heading to bed now. Please do test it (aromano
> mozilla com).

I've tested your tryserver part with all the various d&d operations we support for tabs and it looks good. Right now I cannot see a possible regression.

Comment 10

[Neil Deakin]

Comment on attachment 379620 [details] [diff] [review]
cocoa patch (checked in)

>diff --git a/widget/src/cocoa/nsChildView.mm b/widget/src/cocoa/nsChildView.mm
>--- a/widget/src/cocoa/nsChildView.mm
>+++ b/widget/src/cocoa/nsChildView.mm
>@@ -6296,16 +6296,35 @@
> 
>   if (mDragService) {
>     // set the dragend point from the current mouse location
>     nsDragService* dragService = static_cast<nsDragService *>(mDragService);
>     NSPoint pnt = [NSEvent mouseLocation];
>     FlipCocoaScreenCoordinate(pnt);
>     dragService->SetDragEndPoint(nsIntPoint(NSToIntRound(pnt.x), NSToIntRound(pnt.y)));
> 
>+    // XXX: dropEffect should be updated per |operation|. 
>+    // As things stand though, |operation| isn't well handled within "our"
>+    // events, that is, when the drop happens within the window: it is set
>+    // either to NSDragOperationGeneric or to NSDragOperationNone.
>+    // For that reason, it's not yet possible to override dropEffect it per

remove 'it'

Comment 11

[Neil Deakin]

(In reply to comment #10)
> (From update of attachment 379620 [details] [diff] [review])

> +    dataTransferNS->SetDropEffectInt(nsIDragService::DRAGDROP_ACTION_NONE);

Also, you should null-check dataTransferNS.

Comment 12

[Dietrich Ayala (:dietrich)]

Who's the right person to work on the gtk patch for this?

Comment 13

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

Karl.

Comment 14

[Asaf Romano (gone)]

Comment on attachment 379620 [details] [diff] [review]
cocoa patch (checked in)

Cocoa:
http://hg.mozilla.org/mozilla-central/rev/c06296fd9b28
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/92b062d4764e

Comment 15

[Karl Tomlinson (:karlt)]

My understanding of the issue here is that the "current drag operation" is not
being set when "the current target element is not a DOM element" as described
in Section 7.10.4 step 2.4.

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

However, the current drag operation for not-DOM-element targets is only used
for dragend events, so we need only calculate these current drag operations at
drag end.

I think we should be able to do something similar to the Windows patch for bug
455884.  That would make the code correct but may not give the intended
results.

Currently my KDE desktop (at least) accepts these drops, offering to create a file containing the text (which is not very meaningful).

Dragging tabs from Konqueror to the desktop offers to link or copy the web
page to the Desktop.  That might be what the user expects.

If the intention is that dragging tabs to anything other than a tab bar should
create a new window, then we should also do something to ensure than the
desktop does not accept these drops.

Comment 16

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

(In reply to comment #14)
> (From update of attachment 379620 [details] [diff] [review])
> Cocoa:
> http://hg.mozilla.org/mozilla-central/rev/c06296fd9b28
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/92b062d4764e

Verified on trunk and 1.9.1 with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090527 Minefield/3.6a1pre ID:20090527031500

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

Comment 17

[Asaf Romano (gone)]

Karl, it's already ensured that the desktop doesn't accept the drop. That's done by using private mime types. The problem is that in gtk widget (and previously on cocoa and windows), the dropEffect is not set to "none" as you drop the tab on the desktop, but rather keeps it the way it was last set within the browser window. Thus you get, for example, dropEffect == "move",  if the last dragover-area within the window could accept the drop even though you the tab was dropped outside the window's dragover-control. I recall Jim or Neil saying that the spec doesn't say much about this scenario.

Comment 18

[Asaf Romano (gone)]

Also, I wouldn't recommend reading comments about the detach-behavior in ancient bugs like bug
455884 ;) The behavior changed much since then. Though, the decision to loose the tab-to-file gesture alongside tab-to-native-textfiled gesture remains.

Comment 19

[Neil Deakin]

(In reply to comment #6)
> OK, I'm not going to work on the gtk part. Sorry, it's too complicated to work
> on it with no real gtk experience, no linux machine and a strong feeling that
> the platform-version issue Neil faced in bug 479995 would be a problem here as
> well.

Yes, that issue will affect this as well, since there isn't a way to know if a drag was cancelled, and we shouldn't be setting the drop effect to anything but 'none' in this case. We can handle the case where the drop wasn't cancelled easily enough: gtk provides this information when the drag ends.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't think this issue blocks on Linux, though we should hold it open for the fix, and if possible, get it on 3.5.x

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to comment #15)
> My understanding of the issue here is that the "current drag operation" is not
> being set when "the current target element is not a DOM element" as described
> in Section 7.10.4 step 2.4.

I filed bug 495184 on this (Linux version of this bug), with a patch.

(In reply to comment #19)
> ...if a
> drag was cancelled, and we shouldn't be setting the drop effect to anything but
> 'none' in this case.

That seems reasonable behavior to me, though it isn't how I read the spec.
Anyway, my patch sets dropEffect to none on cancel.

Closing this bug (for Mac) and marking verified1.9.1 based on comment 16.

Comment 22

[Karl Tomlinson (:karlt)]

(In reply to comment #15)
> Currently my KDE desktop (at least) accepts these drops, offering to create a
> file containing the text (which is not very meaningful).

(In reply to comment #17)
> Karl, it's already ensured that the desktop doesn't accept the drop. That's
> done by using private mime types.

It's still using text mime types, so I filed bug 495189.

Comment 23

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

Verified on trunk too with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090527 Minefield/3.6a1pre ID:20090527031500