465608 - pressing esc while dragging a tab should cancel the drag / detach operation (on windows)

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[Sylvain Pasche]

Pressing that key when dragging a tab to detach it should cancel the operation and not open the tab in a new window.  That's the common shortcut for canceling a drag operation on the three platforms.

Comment 1

[Nochum Sossonko [:Natch]]

This is a huge regression IMHO as now moving tabs (i.e. 1st tab to be 2nd tab) and then hitting escape results in a detach operation. Obviously there's also the problem with not being able to cancel a real detach operation...

Comment 3

[Jo Hermans]

see also bug 465184 (not exactly the same, but they're working on canceling too)

Comment 6

[Nochum Sossonko [:Natch]]

AFAICT there is no way to tell if the drag ended from a mouseup or from esc, this should be implemented regardless of this bug, filed bug 471848 for that.

Comment 7

[Kim]

Esc works as expected when dragging selected text and when dragging tabs to reorder them ("drop-marker" is visible), on Linux at least. So, taking a look at the code that handles that might help?
Wouldn't removing the event listener for dragend when esc is pressed work, for the time being?

Comment 8

[Nochum Sossonko [:Natch]]

(In reply to comment #7)
> Esc works as expected when dragging selected text and when dragging tabs to
> reorder them ("drop-marker" is visible), on Linux at least. So, taking a look
> at the code that handles that might help?

Esc is handled by the drag service itself, the reason this case is different than others is that it has to handle *every* drop and so reacts to dragend on the source node, unlike any other drag that only drops on appropriate drop targets, which then fire the ondrop on the target node. Hitting esc fires a dragend event, but not a drop event.
> Wouldn't removing the event listener for dragend when esc is pressed work, for
> the time being?

Problem is, AFAICT, there is no way to do that (events don't get fired during a drag).

Comment 9

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

The patch in bug 471499 will fix this for when ESC is pressed while the mouse is within the browser window. I'm morphing this a little bit to repersent that.

I filed bug 474442 to track the broader issue of ESC cancelling at all times, which depends on bug 471848.

Comment 10

[Nochum Sossonko [:Natch]]

Bug 471499 doesn't fix this, except for when the tab is dragged on the tabbar itself. Otherwise esc does nothing, not while in content, or in the rest of chrome.

Comment 11

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

Fixed by bug 471499

Comment 12

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

(In reply to comment #10)
> Bug 471499 doesn't fix this, except for when the tab is dragged on the tabbar
> itself. Otherwise esc does nothing, not while in content, or in the rest of
> chrome.

Hm - I tested it pretty thoroughly on the tryserver builds on OSX and Windows, and it seemed to work well. Let's wait and test it on a build before re-opening?

Comment 13

[Nochum Sossonko [:Natch]]

So, basically for some reason when you hit escape aEvent.clientX reports "0" and aEvent.clientY reports "-131" on Windows. That's the reason this isn't working on windows. I profiled this with the error console.

Error: browserWindowOnDragLeave: true
0 -- -131
Source File: chrome://browser/content/tabbrowser.xml
Line: 2225

I'll wait though for a nightly, maybe something's just wrong with my build.

Comment 14

[IU]

For me this bug definitely is not fixed.  In fact, the opposite happens: pressing ESC, while dragging, guarantees that the tab WILL detach.

To observe the stated issue, drag (and hold) a tab to any target (e.g. a textarea, explorer, bookmark toolbar, etc.) where detach would not ordinary be invoked.  While still holding, press ESC.  The tab detaches.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090123 Firefox/3.2a1pre ID:20090123033224

Comment 15

[-fullmetaljacket-]

(In reply to comment #14)
> For me this bug definitely is not fixed.  In fact, the opposite happens:
> pressing ESC, while dragging, guarantees that the tab WILL detach.

i confirm that behavior

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre ID:20090123033224

Comment 16

[Martijn Wargers (dead)]

I can also verify that the bug is still not fixed in current trunk build.
So I'm reopening this bug, because we already have a lot of confirmations that this bug is not fixed.

Comment 17

[Martijn Wargers (dead)]

Attachment: patch

This fixes it for me, but it's obviously a windows only patch.
It also doesn't really make sense to set the dropEffect to DRAGDROP_ACTION_MOVE, maybe there should a DRAGDROP_ACTION_CANCEL dropEffect or something?

Comment 18

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

This is FIXED on OSX, for sure. I'll go test on my Windows VM, but I'm pretty sure I did that already and it's FIXED there as well. Are you sure you aren't mistaking this for bug 475066?

Comment 19

[Martijn Wargers (dead)]

Yes, I'm sure.

Comment 20

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

Mano: any ideas why this is happening? I've confirmed it on Vista, so it looks like it's Windows only ...

Comment 21

[Vladimir Kondratyev]

The problem become even worse: ESC now reloads tab and opens it in separate window.

I can confirm this behavior under Windows XP with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090126 Shiretoko/3.1b3pre build.

Comment 22

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

--> P2, we'll get this for RC1.

Comment 23

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

Enn: can you take a peek at this? Mano suspects that it's something to do with backend code that you've got more insight into.

Comment 25

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

So, from talking to Mano, if the pointer leaves the window we'll always detach unless a shortcut is created (note that presently shortcuts are created by default when dragging to the desktop due to bug 475066). If it doesn't, then we only detach on drop.

On Windows, we seem to be creating new windows all the time when ESC is pressed. When outside the window, I suspect (Mano: confirm?) that this is likely because the ESC cancels the creation of the shortcut, thus bypassing bug 475066 and triggering the detach. When inside the window, Mano suspects it's because ESC is firing a bogus event.

Neil: can you guys work this out?

Jimm: any opinions? We might need your help testing.

Comment 26

[Jens Müller (:tessarakt)]

As far as I can see, the only way to "cancel" a drag without detaching the tab or changing anything else is to let the tab drop at the left side of the tab itself (i.e., "between" the tab left to it and itself"). 

I understand that you might want to detach a tab by just dragging it _somewhere_, but I don't think a tab should be detached if you let it drop on itself ... That's just counterintuitive.

Please tell me whether this is inside the scope of this bug, otherwise I would open a new one ...

Btw, my observations are based on Windows Firefox 3.1 beta 2.

Comment 27

[Martijn Wargers (dead)]

Jens, that is already fixed in the latest trunk builds and isn't actually not related to this bug (it was fixed in a different bug).
If you want to test the latest 1.9.1 build, then look here:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-1.9.1/

Comment 28

[Jens Müller (:tessarakt)]

Thanks for the quick answer. Yeah, the first few comments of bug 471499 (the summary of that is just so general that I didn't find it when searching, sorry) look sensible. It's not that urgent, I'll just wait for the next beta to test it.

Comment 29

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

Mano: you said you'd expected to have a new patch here soon?

Comment 30

[Asaf Romano (gone)]

Attachment: patch

Comment 31

[Neil Deakin]

Comment on attachment 363885 [details] [diff] [review]
patch

>-            var dt = aEvent.dataTransfer;
>+            var dt = aEvent.dataTransfer;ss

I don't think this change was intentional ;)

Comment 32

[Asaf Romano (gone)]

I filed bug 479995 to get this working on Linux.

Comment 33

[Mike Connor [:mconnor]]

Comment on attachment 363885 [details] [diff] [review]
patch


>+            if (aEvent.mozUserCancelled || !this._dragLeftWindow ||
>+                this.mTabs.length == 1)

minor style kvetch, when conditionals break across lines I find it easier read if each case is on it's own line.

>-            var dt = aEvent.dataTransfer;
>+            var dt = aEvent.dataTransfer;ss

*cough*

Comment 34

[Asaf Romano (gone)]

Hey Neil, I accidentally credited the checkin of this on the branch to you, sorry.

191: c1a6a8145159
mozilla-central: afc23b21a39c

Comment 35

[juan becerra [:juanb]]

Added Litmus test 7544

Comment 36

[Jim Jeffery not reading bug-mail 1/2/11]

This appears to not be fixed.  Still opens a new window, and now does not retain the tear-off in History->Recently Closed tabs

1.  Drag a tab to active tab-window
2.  Press ESC 
3.  Tear off occurs 
4.  Tab is not in Recently Closed tabs Ctrl+Sht+t does not recover tab.

No errors in console2.

Using changeset:  http://hg.mozilla.org/mozilla-central/rev/18481b5e9b5c

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090226 Minefield/3.2a1pre Firefox/3.0.4 ID:20090226125006

Vista HP SP1

Comment 37

[Jim Jeffery not reading bug-mail 1/2/11]

Correction:  Ctrl+Sft+t does restore the tab, sorry - had my pinky-finger in wrong place.

Tab still tears off however.

Comment 38

[Asaf Romano (gone)]

Attachment: make it work...

mozilla-central: 1e0d54adf926
191: 6059303bf76f

Comment 39

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

Verified fixed with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090301 Minefield/3.2a1pre ID:20090301020403

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090301 Shiretoko/3.1b3pre ID:20090301020400

Comment 40

[George Carstoiu]

Build identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b9) Gecko/20100101 Firefox/4.0b9

Issue is present in Linux.