465186 - Detaching a tab does not open the new window at the drop location (dragging tab to an empty second monitor opens window on the wrong monitor)

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Sylvain Pasche]

If a tab is detached and dropped at a given point, the new window that is opened will be located near the window that contained the tab but not where the tab was dropped.

This is most visible if you have two monitors.  If you drag a tab to the other monitor, the window will open on the wrong one.  Safari does the right thing in this case.

Comment 1

[Ian Macfarlane]

Confirmed - this is a real pain if you have more than one monitor.

Comment 2

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

Really really wanted, but not blocking. Let's hope that the core bug this relies on can be fixed in a timely manner.

Comment 5

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

Mano: any idea why this is happening?

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So I took a look at this yesterday, and we can get the mouse coords. However when changing the window features string to ww.openWindow to include top= & left=, it's not changing the position of the window... I'll do some more investigating.

Comment 7

[Simon Bünzli]

(In reply to comment #6)
> I'll do some more investigating.
We'd like that solution for SessionStore's _openWindowWithState as well, BTW.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Guilty Stack Trace

An update on the investigation - it looks like it's getting the right initial position, but then something else positions the window afterward. So I'm pretty sure this is the guilty stack trace. I'm working on going through this now to see if I can track it down more specifically.

One thing I've noticed is that nsXULWindow::GetPositionAndSize gives the correct coords, but windowElement->GetAttribute("screenX") (and screenY) result in incorrect positioning. The window then gets repositioned based on these incorrect positions. Why? I don't know...
 
It might be that we need to go one step up and never even call LoadPositionFromXUL... I'll talk to a window specialist ASAP

Comment 9

[Sylvain Pasche]

I see you are testing on Mac. I also noticed some strange behaviors with window positioning there (see bug 461285 comment 2). Basically, setting screenX/screenY on the window had no effect when set just after the window is opened. It worked if I waited a few hundred milliseconds and set the screenX/screenY at that moment.

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1 (WIP)

I had a patch in progress in my patch queue, so I thought I'd throw it up here.

In theory, this should be all we need, but as mentioned previously, there appears to be a problem with positioning a window.

Also, we'll want to do more than just position the window where the use drops is. For real [parity-safari] we'll need to do smart positioning to keep windows on screen (maybe make that a pref - surely somebody would want dumb positioning).

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

This bug is super obvious when you tear off a tab, I'm just not sure how many users are tearing their tabs off yet (or even know that now they can).  Might need to switch to a P1 if the feature becomes more widely known.

Comment 13

[Nochum Sossonko [:Natch]]

Paul: did you file any Core bugs about the positioning issues you encountered? That should get done sooner than later if this is to make 3.next.

Comment 14

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #13)
> Paul: did you file any Core bugs about the positioning issues you encountered?
> That should get done sooner than later if this is to make 3.next.

I didn't get much further than what I said in comment #8, so no other bugs got filed. Hopefully I'll have a chance to get back at it soon, though I'm a bit swamped with other stuff.

If anybody wants to poke around at it, feel free. I can't speak for drivers, but I would imagine a fix for this would go into 3.5.1 if found in time.

Comment 15

[Nochum Sossonko [:Natch]]

Attachment: patch

Here's another idea. The reason (I think) the other approach wasn't working is because the screenX and screenY attributes are defined and persisted on the window. That was overriding the position setting from the openWindow call.

This does its thing in BrowserStartUp, after a timeout so that the window is ready to be moved (without the timeout it wouldn't work). I've also made sure to clamp the values to reasonable numbers, as well as a small initial change to get it just right.

Comment 16

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 2

Make it actually work...

windowState isn't available to _after_ unload, so we have to check for it then.

The clamping also needed some tweaking to make sure the window is always accessible. No need to check for the existence of the attributes, as we always set them.

Comment 17

[Asaf Romano (gone)]

Comment on attachment 393914 [details] [diff] [review]
patch ver. 2

Thanks to Neil Rushbrook, I found this out:
http://hg.mozilla.org/mozilla-central/annotate/bc5324d3f8fc/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#l745

So, if you specify either the height or the width of the new window, the persisted values would be left off. Let's go back to the previous approach then.

Comment 18

[Nochum Sossonko [:Natch]]

(In reply to comment #17)
> Thanks to Neil Rushbrook, I found this out:
> http://hg.mozilla.org/mozilla-central/annotate/bc5324d3f8fc/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#l745
> 
> So, if you specify either the height or the width of the new window, the
> persisted values would be left off. Let's go back to the previous approach
> then.

The first WIP patch already tried that (specifying a "width=" in the features) and it didn't work. In my testing it didn't work either, so I'm not sure what exactly you mean...

Comment 19

[Asaf Romano (gone)]

As mentioned on IRC, you'd probably have to use "classic" js window.open/window.openDialog to get this effect working. It might be worth filing a bug on that.

Comment 22

[Nickolay_Ponomarev]

Adding keywords to the summary to make this bug easier to find...

Comment 23

[Nochum Sossonko [:Natch]]

Not going to be able to get to this.

Comment 25

[Nochum Sossonko [:Natch]]

Paul your patch should be good to go, bug 509828 is fixed on trunk.

Comment 26

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #25)
> Paul your patch should be good to go, bug 509828 is fixed on trunk.

Thanks for the reminder and filing & fixing that bug. I'll take a look at it this week. It would definitely be nice to have for 3.6. I'll probably add in the logic you had to keep the window on screen, though long-term I think something more advanced would be nice (re comment #10).

Comment 27

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Beltzner says this has a good chance of landing for 3.6 but it needs to bake on trunk & happen soon. Getting some UX help on how we should behave around the edges.

Comment 28

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

Adding uiwanted; Paul do you have specific questions about those edges? Like, what edge behaviours do you want to get feedback on?

Comment 29

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #28)
> Adding uiwanted; Paul do you have specific questions about those edges? Like,
> what edge behaviours do you want to get feedback on?

I briefly talked to Boriss yesterday about this. Specifically I'm looking for feedback on what we do when a window overflows the available size of the screen. The patch I have right now just builds in a buffer around the edges so you can't lose a window. It's naive but works.

Things to consider: should we resize/move the window so that upon dropping, the new window is always completely visible? Or do we trust that the user meant to have it partially offscreen. Do we size the new window the same as the previous? or just keep doing what we're doing? Do we put a pref in to switch back to current behavior (because people might have gotten used to it)?

Other thoughts, since the cursor holds onto the top left of the "preview", we should position the top left of the window there (easy & working). But that's not really ideal. I like how Safari does it.

Maybe here we keep it simple & naive. Then for the tab animation project we really think about what should be happening and redo the experience.

Comment 30

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.3

Current patch. Only does protection along the right & bottom because it's ok if the window gets positioned at (0,0).

Problem: This doesn't work with multiple monitors (screen.availWidth is only the current/default? monitor).

Comment 31

[Dão Gottwald [:dao]]

(In reply to comment #30)
> Problem: This doesn't work with multiple monitors (screen.availWidth is only
> the current/default? monitor).

I think you need to look at screen.availLeft and screen.availTop.

Comment 32

[Dão Gottwald [:dao]]

Comment on attachment 407415 [details] [diff] [review]
Patch v0.3

>+            if (aLeft !== null) { // explicit null check because could be == 0

The second "=" is unnecessary. In fact, it's incorrect, because undefined !== null.

Comment 33

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #31)
> (In reply to comment #30)
> > Problem: This doesn't work with multiple monitors (screen.availWidth is only
> > the current/default? monitor).
> 
> I think you need to look at screen.availLeft and screen.availTop.

Those help with top/left (eg on OS X, availTop is 22, not 0) but that doesn't
help the multiple monitor situation.

screen only picks up attributes from the monitor the window is on. Do we have
something that will give us a better breakdown on available screen real estate?

Comment 34

[Timothy Nikkel (:tnikkel)]

nsIScreenManager lets you get the screen for a given rectangle with screenForRect. I think you should be able to get the screen object for all the relevant screens using that.

Your current patch is also assuming that screenX/Y are between 0 and the width/height of the current screen. This is not always true. For example, if you have two identical screens, and set your primary screen to be to the right of the other one, then all of the screenX coordinates on the left screen will be negative.

Comment 35

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #34)
> nsIScreenManager lets you get the screen for a given rectangle with
> screenForRect. I think you should be able to get the screen object for all the
> relevant screens using that.

Thanks. Looks perfect. I can use screenForRect with the drop location & window size.

> Your current patch is also assuming that screenX/Y are between 0 and the
> width/height of the current screen. This is not always true. For example, if
> you have two identical screens, and set your primary screen to be to the right
> of the other one, then all of the screenX coordinates on the left screen will
> be negative.

I'm using availTop & availLeft locally, so that should be ok now. You're right though.

Comment 42

[Scott A.]

Any work still being done on this? Would be nice to have in 4.0

Comment 43

[J.M.S.]

(In reply to comment #42)
> Any work still being done on this? Would be nice to have in 4.0

Agreed. Especially with the proliferation of ATI Eyefinity & NVIDIA Surround setups over the last year, multi-monitor configurations are becoming a lot more common.

I find this issue annoying, but I have a friend who switched to Chrome (and I'm hoping to convince him to switch back for FF4) because of speed, tabs on top (something I don't like but am glad it's a preference choice in FF4) & this issue, all of which will be addressed for Firefox 4 except for this issue and he won't switch back until it is. I'm sure there must be quite a few other multi-monitor setup users who've done the same or at least find quite annoying.

Doesn't seem like this issue *should* be that hard to fix, with a little time from a few more higher up eyes (I say *should* because I did read what Paul said and it sounds like this is an issue being made unnecessarily cumbersome by Firefox's massive legacy codebase design causing unexpected conflicts for an unexpected issue).

Comment 44

[RD]

This really is the main reason I keep using chrome over FF, it sounds so small but the silly task of dragging off a tab, having it maximize on the wrong screen, so I have to double click to shrink, then drag, and then double click the bar again to maximize. Its a small pain that makes browsing annoying.

Comment 46

[madmax_noway]

It's a shame this still hasn't been fixed.

Comment 47

[bensrob]

I honestly thought that this was a new bug with Firefox 4.  
At least from 3.4 (i think) onwards for definite, in all honesty i hadn't tried before, I'd been able to do this no problem, first on XP then on Win 7.

I only lost the ability to do so as I updated to 4 RC today

Comment 48

[bensrob]

Did a bit more poking around and it seems that the new window is being created at the default position to create new windows.

Example 1:  Opened window A (full screen on monitor 1)
            Dragged tab to monitor 2
Result:     Full screen window on monitor 1


Example 2:  Opened window A (full screen on monitor 1)
            Moved window A to monitor 2, to center of the screen
State:      Window A is windowed on monitor 2
            dragged new tab to monitor 1
Result:     Window B is the same size as window A, aligned slightly down and .           to the right, i.e. default new window behavior

Example 3:  Opened window A (windowed on monitor 1)
            Moved window A to monitor 2, dropping it on the top of the screen, .           using Windows 7 to make the window full screen
            (this does not change the default screen of the window)
State:      Window A is full screen on monitor 2
            Dragged new tab to monitor 1
Result:     Window B is full screen on monitor 1


I then repeated all the tests using the opposite screens and got the opposite results as expected.

As you can see the windowed/full screen is retained correctly but otherwise it is simply using the default position of the window.


I plan to look into this more but I'm afraid that I haven't seriously coded in a long time so doubt I'll get very far beyond doing tests to try and work out the exact behavior of this bug.

Comment 49

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #48)
> I plan to look into this more but I'm afraid that I haven't seriously coded in
> a long time so doubt I'll get very far beyond doing tests to try and work out
> the exact behavior of this bug.

I appreciate you looking into this, but the behavior is pretty well known. Windows are opened based on previous state and positioning indicated in localstore.rdf

Comment 50

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #30)
> Created attachment 407415 [details] [diff] [review]
> Patch v0.3
> 
> Current patch. Only does protection along the right & bottom because it's ok if
> the window gets positioned at (0,0).
> 
> Problem: This doesn't work with multiple monitors (screen.availWidth is only
> the current/default? monitor).

This still makes the problem less annoying.  Paul, could we recycle this patch, land it, and handle the monitor issue separately?

Comment 51

[Frank Yan (:fryn)]

I've had a patch that fixed this for a while now. Didn't realize it was its own bug.

Comment 52

[Frank Yan (:fryn)]

Ack. Wrong bug. Bugzilla, why you so confusing!

Paul, do you have a basic patch for this?

It'll be even more obvious that we're doing it wrong after we get bug 455694. A basic patch is better than nothing.

Comment 53

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.4 (WIP)

I looked at this again not too long ago and it's using the screen manager to get the right screen size.

On OS X (either our window opening code or OS X itself) seem to handle keeping a window on screen (no resizing unless between monitors) so that's ifdef'd out.

Frank said he might put in support to resize the window down to keep the window as close to drop coordinates as possible.

Comment 55

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

Not exactly sure yet, if uiwanted is still necessary but cc'ing Limi. Also it would be great if we could get this issue fixed for Fx7.

Comment 57

[Jez Ng [:int3]]

Attachment: Patch v0.5

This is an improvement upon zpao's patch. This fixes some errors in the positioning and drops tabs based on the positioning of the cursor instead of the area of the window that overlaps with the various screens. Additionally, it handles the case where the tab being dragged is the only one in its window.

This patch has been tested on OS X and Windows. On Windows, specifying the size of a detached tab via options passed to openDialog() seems problematic -- the webpage in the resulting window does not extend to the bottom of the window. To reproduce this problem:

1. Remove the #ifndef XP_WIN section in the patch
2. Create a browser session with at least two tabs in one window
3. Detach one tab via drag & drop

Result:
The rendered page in the new window does not extend to the bottom of the window.

I haven't quite figured out what happens when we do not specify the size of the new window. Sometimes it takes the size of the window that the tab was dragged from and sometimes it takes the size of a different window. However, in all cases, the resulting window fits within the monitor that it is dropped into, so it doesn't seem _too_ big a deal.

Comment 58

[Dão Gottwald [:dao]]

Jezreel, what's the purpose of the nsGlobalWindow.cpp part of your patch?

Comment 59

[Jez Ng [:int3]]

When there's only one tab in the window being dragged, I call window.moveTo() instead of destroying and recreating the window, as the latter is slower. However, I ran into a problem with the mSuppressLevel lock in nsGlobalWindow. This lock is acquired whenever moveTo() is called and nsGlobalWindow thinks the mouse button is being held down. Of course, the mouse button has been released by the time we receive the 'dragend' event, but currently the window doesn't know this because it looks for the mouseUp event, and since the mouseUp happens when the mouse is outside the global window, the event does not get sent to the window. The change to nsGlobalWindow.cpp fixes that.

This mSuppressLevel lock was actually introduced in bug 329385 by smaug as part of a security fix. The change to nsGlobalWindow.cpp was made after discussing the above problem with him.

Comment 60

[Dão Gottwald [:dao]]

Comment on attachment 539431 [details] [diff] [review]
Patch v0.5

smaug should probably review this part, then.

Comment 61

[Jez Ng [:int3]]

gg

Comment 62

[Jez Ng [:int3]]

Oops, I guess I left some random characters in the textbox when I clicked 'Save Changes'. Anyway, cc'ing smaug...

Comment 63

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 539431 [details] [diff] [review]
Patch v0.5

>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -2536,17 +2536,18 @@
>       gEntropyCollector->RandomUpdate((void*)&(aVisitor.mEvent->time),
>                                       sizeof(PRUint32));
>     }
>   } else if (msg == NS_RESIZE_EVENT) {
>     mIsHandlingResizeEvent = PR_TRUE;
>   } else if (msg == NS_MOUSE_BUTTON_DOWN &&
>              NS_IS_TRUSTED_EVENT(aVisitor.mEvent)) {
>     gMouseDown = PR_TRUE;
>-  } else if (msg == NS_MOUSE_BUTTON_UP &&
>+  } else if ((msg == NS_MOUSE_BUTTON_UP ||
>+             msg == NS_DRAGDROP_END) &&
add one space before the latter msg.

Reviewing only this part, r=me.

Comment 64

[Jez Ng [:int3]]

Attachment: Patch v0.6

Added in the space as per smaug's review.

Comment 65

[Jez Ng [:int3]]

Attachment: Patch v0.7

Previously, the new window created when a tab gets dropped is positioned with the corner of the window next to the mouse cursor. I've modified the patch slightly to drop the window such that the position of the mouse relative to the corner of the dragged tab stays the same before and after the drag. With this change, after the user drops a tab and a new window is created, he can start a new drag session by clicking on the same spot.

Thanks fryn for the suggestion!

Comment 66

[Frank Yan (:fryn)]

(In reply to comment #65)
> Created attachment 540215 [details] [diff] [review] [review]
> Patch v0.7

Nit: I'd prefix dragOffset[X|Y] with an underscore.

I'm surprised that there is a scrollboxPaddingStart property :P

Also, it's good that you freed dragOffset[X|Y] in the drop handler :)

Comment 67

[Jez Ng [:int3]]

I was surprised too -- I chanced upon it while searching for stuff in the file. Apparently the pinned tabs code uses it. (:

Comment 68

[Jez Ng [:int3]]

Attachment: Patch v0.8

Added in underscores as per fryn's suggestion.

Comment 69

[Jez Ng [:int3]]

Attachment: Patch v0.9

So I noticed that by moving / resizing a dragged window with only one tab we could end up with it placed behind existing browser windows. I've fixed it up so that our dragged window retains the focus.

Comment 70

[Dão Gottwald [:dao]]

Comment on attachment 543342 [details] [diff] [review]
Patch v0.9

>+        let bo = this.mTabstrip.boxObject;
>+        let paddingStart = this.mTabstrip.scrollboxPaddingStart;
>+        // _dragOffset[X|Y] give the coordinates that the mouse should be
>+        // positioned relative to the corner of the new window created upon
>+        // dragend such that the mouse appears to have the same position
>+        // relative to the corner of the dragged tab.
>+        tab._dragOffsetX = event.screenX - tab.boxObject.screenX + paddingStart + bo.screenX - window.screenX;
>+        tab._dragOffsetY = event.screenY - tab.boxObject.screenY + bo.screenY - window.screenY;

- The horizontal calculation doesn't take pinned tabs into account.
- The scrollboxPaddingStart usage is fragile, as it may be different depending on whether the tab strip is overflowing.
- "+ bo.screenY - window.screenY" seems redundant, it should always be 0.

>+        var sm = Cc["@mozilla.org/gfx/screenmanager;1"].
>+                   getService(Ci.nsIScreenManager);
>+        var whichScreen = sm.screenForRect(eX, eY, 1, 1);
>+        var sX = {}, sY = {}, sW = {}, sH = {};
>+        whichScreen.GetAvailRect(sX, sY, sW, sH);
>+        // ensure new window entirely within screen
>+        var winWidth = Math.min(window.outerWidth, sW.value);
>+        var winHeight = Math.min(window.outerHeight, sH.value);
>+        var aLeft = Math.min(Math.max(eX - draggedTab._dragOffsetX, sX.value), sX.value + sW.value - winWidth);
>+        var aTop = Math.min(Math.max(eY - draggedTab._dragOffsetY, sY.value), sY.value + sH.value - winHeight);

- The amount of two-letter variable names is slightly overwhelming. sW and sH can be sWidth and sHeight, at least.
- The "a" prefix should be used arguments from the outside, not for variables that you intend to pass to other functions.

I'll address these comments and land it.

Comment 71

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/fa4005b29b5e
http://hg.mozilla.org/mozilla-central/rev/c12172ae966d

Comment 72

[Vlad [QA]]

Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0a2) Gecko/20110707 Firefox/7.0a2

Comment 74

[J.M.S.]

THANK YOU. Finally, this bug has been resolved and was included in the Firefox 7 release as targeted.

To everyone who contributed to this bug fix, thank you so much.

In my opinion, after the memory handling fixes, this is the most significant feature/update introduced in Firefox 7 (certainly as far as usability improvements go) and yet it went completely unmentioned in the main list of the Firefox 7 "what's new" improvements? Who's running Mozilla?

This was an important (and long overdue) feature that should have been advertised (catching Firefox 7 up to Chrome) and should be added there now (better late than never).

- JS

Comment 75

[sabrage]

Why does this problem still exist in 2013...?

Comment 76

["Saptarshi Guha[:joy]"]

Who knows. It still exists in Firefox Nightly (28.0a1 (2013-11-17))
on Ubuntu 12.10.

Comment 77

[sabrage]

Firefox 25.0.1 on Windows 7 here. Glad I'm not the only one.

Comment 78

[Matthew N. [:MattN]]

This still works for me in the single monitor case on Windows 7 and OS X 10.9 but not in the multi-monitor case for 10.7 and 10.9 so I filed bug 949320. When bugs have been fixed for a long time, you should always file new bugs about regressions rather than commenting in ones many years old.

Comment 79

[Shaun Forsyth]

Firefox 84.0.2 and the bug is still there in 2021.. this drives me crazy

Comment 80

[jason cayen]

Yes I think it has resurfaced.

https://www.youtube.com/watch?v=ngMwYX0fJYo

Comment 81

[Shaun Forsyth]

I have further information

This is only a problem for me, when using RDP.