490225 - CMD-T opens a new window instead of a new tab when Downloads window is in focus

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tony Chung [:tchung]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4

When the downloads window is in focus, opening a new tab (CMD-T on mac), is opening a new browser window instead of a new tab.   It should open a new tab instead of a new window

Reproducible: Always

Steps to Reproduce:
1. launch beta 4
2. open tools > Download window, and bring to focus
3. hit keyboard shortcut to open a new tab (CMD-T on mac, CTRL-T on win)
4. Verify a new browser window is opened, instead of a new tab
Actual Results:  
New window opened

Expected Results:  
New Tab opened

Comment 1

[:Gavin Sharp [email: [e-mail]]]

We could use nsIBrowserGlue::getMostRecentBrowserWindow instead of a gBrowser check, I suppose.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1781

Comment 2

[jonesey]

This seems like the reporter is expecting results that he should not expect.  Is there a way to mark this "working as designed"?  Does it even make sense to add a tab to the Downloads window?  

Cmd-T with no windows open should generate a new window.  Ditto for Cmd-T with the Downloads window open, right?

Comment 3

[philippe (part-time)]

(In reply to comment #2)

The expected behaviour is to open a new tab in the active window (if one exists). Currently, Fx always opens a new window, even if there already is an existing window.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

So to be crystal clear, the way to fix this would be to change BrowserOpenTab() to call getMostRecentBrowserWindow() and open a new tab in that window if it exists, before it falls back to opening a new window.

Comment 5

[philippe (part-time)]

(In reply to comment #4)
> So to be crystal clear, the way to fix this would be to change BrowserOpenTab()
> to call getMostRecentBrowserWindow() and open a new tab in that window if it
> exists, before it falls back to opening a new window.
Yes. That is how Camino and Safari behave.

Comment 6

[Gleb M. Borisov]

Attachment: Fix it

Comment 7

[Gleb M. Borisov]

Attachment: Fix it

Same patch, but after pull/update.

Comment 8

[Dão Gottwald [:dao]]

Can you just call openUILinkIn("about:blank", "tab");?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 512445 [details] [diff] [review]
Fix it

Seems like dao's suggestion should work, and is much simpler.

Comment 10

[Gleb M. Borisov]

Attachment: BrowserOpenTab, focusAndSelectUrlBar refactoring

Using openUILinkIn is really easy.

Also I've changed focusAndSelectUrlBar to accept unnecessary parameter aWindow (to focus urlbar in given window). I'm not sure that it's right way, but I don't know how to handle situation when we should focus urlbar from another window (such as Downloads or Error Console window). I've added Dão to 'feedback' field. May be he knows right way to do it.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> I'm not sure that it's right way, but I
> don't know how to handle situation when we should focus urlbar from another
> window (such as Downloads or Error Console window).

How about calling focusAndSelectUrlBar in BrowserStartup when loading about:blank?

Comment 12

[Gleb M. Borisov]

(In reply to comment #11)
> (In reply to comment #10)
> > I'm not sure that it's right way, but I
> > don't know how to handle situation when we should focus urlbar from another
> > window (such as Downloads or Error Console window).
> 
> How about calling focusAndSelectUrlBar in BrowserStartup when loading
> about:blank?

BrowserStartup called only when new window initializing. This bug about reusing existing window, so BrowserStartup is not suitable in this case.

Comment 13

[Dão Gottwald [:dao]]

A new window will be opened if no other is around.
For the other case, the openLinkIn function in utilityOverlay.js could focus the url bar.

Comment 14

[Dão Gottwald [:dao]]

> For the other case, the openLinkIn function in utilityOverlay.js could focus
> the url bar.

... either automatically for about:blank or depending on a parameter.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 521467 [details] [diff] [review]
BrowserOpenTab, focusAndSelectUrlBar refactoring

per previous comments. Please let me know if something is unclear.

Comment 16

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

This is still not right in the 8.x Beta channel, as of today at least.  

FWIW, "Page Info" and some of the other modal windows also trigger this behaviour.  Tabby chrome windows like "Tools > Add-ons" do not.

Comment 17

[Reuben Morais [:reuben]]

Attachment: Partial fix.

I'm taking this as this bug has been bugging me for a while ;-)

dao, calling focusAndSelectUrlBar() in BrowserStartup solves the case where a new window is created, but calling it in openUILinkIn does not work for the other case, since the previous is still the one in the global context.

Any tips on how to proceed?

Comment 18

[Reuben Morais [:reuben]]

Attachment: Fix.

Actually, I missed content.focus() being called after my code.
This patch fixes it for real.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 579196 [details] [diff] [review]
Fix.

Looks pretty good overall, just a few refinements:

> function BrowserOpenTab()
> {
>-  if (!gBrowser) {
>-    // If there are no open browser windows, open a new one
>-    window.openDialog("chrome://browser/content/", "_blank",
>-                      "chrome,all,dialog=no", "about:blank");
>-    return;
>-  }
>-  gBrowser.loadOneTab("about:blank", {inBackground: false});
>+  openUILinkIn("about:blank", "tab");
>   focusAndSelectUrlBar();
> }

Looks like you can now remove focusAndSelectUrlBar here.

Note that your openUILinkIn call can open a background tab, depending on browser.tabs.loadBookmarksInBackground. I intent to fix this in bug 707672.

>@@ -298,16 +298,21 @@ function openLinkIn(url, where, params) 
>   // content but don't raise the window, since the URI we just loaded may have
>   // resulted in a new frontmost window (e.g. "javascript:window.open('');").
>   var fm = Components.classes["@mozilla.org/focus-manager;1"].
>              getService(Components.interfaces.nsIFocusManager);
>   if (window == fm.activeWindow)
>     w.content.focus();
>   else
>     w.gBrowser.selectedBrowser.focus();
>+
>+  if (url == "about:blank") {
>+    w.gURLBar.focus();
>+    w.gURLBar.select();
>+  }

You should use focusAndSelectUrlBar here. However, you shouldn't do this in case a background tab was opened.

Comment 20

[Reuben Morais [:reuben]]

(In reply to Dão Gottwald [:dao] from comment #19)
> >@@ -298,16 +298,21 @@ function openLinkIn(url, where, params) 
> >   // content but don't raise the window, since the URI we just loaded may have
> >   // resulted in a new frontmost window (e.g. "javascript:window.open('');").
> >   var fm = Components.classes["@mozilla.org/focus-manager;1"].
> >              getService(Components.interfaces.nsIFocusManager);
> >   if (window == fm.activeWindow)
> >     w.content.focus();
> >   else
> >     w.gBrowser.selectedBrowser.focus();
> >+
> >+  if (url == "about:blank") {
> >+    w.gURLBar.focus();
> >+    w.gURLBar.select();
> >+  }
> 
> You should use focusAndSelectUrlBar here. However, you shouldn't do this in
> case a background tab was opened.

focusAndSelectUrlBar operates on the global gURLBar object, which at that time is not the same as w.gURLBar. When this happens from a different window, are we running on a different browser context?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

You should be able to use w.focusAndSelectUrlBar().

Comment 22

[Reuben Morais [:reuben]]

Attachment: Patch.

(In reply to Gavin Sharp (use [e-mail] for email) from comment #21)
> You should be able to use w.focusAndSelectUrlBar().

Oh, silly me, everything makes much more sense now. Thanks :)

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 579753 [details] [diff] [review]
Patch.

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1248,16 +1248,19 @@ function BrowserStartup() {
>               window.arguments[4] || false);
>       content.focus();
>     }
>     // Note: loadOneOrMoreURIs *must not* be called if window.arguments.length >= 3.
>     // Such callers expect that window.arguments[0] is handled as a single URI.
>     else
>       loadOneOrMoreURIs(uriToLoad);
>   }
>+  else {
>+    focusAndSelectUrlBar();
>+  }

Turns out, this isn't needed, delayedStartup already takes care of this.

>--- a/browser/base/content/utilityOverlay.js
>+++ b/browser/base/content/utilityOverlay.js
>@@ -298,16 +298,20 @@ function openLinkIn(url, where, params) 
>   // content but don't raise the window, since the URI we just loaded may have
>   // resulted in a new frontmost window (e.g. "javascript:window.open('');").
>   var fm = Components.classes["@mozilla.org/focus-manager;1"].
>              getService(Components.interfaces.nsIFocusManager);
>   if (window == fm.activeWindow)
>     w.content.focus();
>   else
>     w.gBrowser.selectedBrowser.focus();
>+
>+  if (!loadInBackground && url == "about:blank") {
>+    w.focusAndSelectUrlBar();
>+  }
> }

You need to take where=="current" into account, where we don't care about loadInbackground.

Comment 24

[Dão Gottwald [:dao]]

Reuben, are you going to update your patch? It's really close, addressing my previous comment shouldn't be hard.

Comment 25

[Reuben Morais [:reuben]]

Attachment: Patch v3

(In reply to Dão Gottwald [:dao] from comment #24)
> Reuben, are you going to update your patch? It's really close, addressing my
> previous comment shouldn't be hard.

Yes! Very busy, etc etc.

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 583110 [details] [diff] [review]
Patch v3

thanks!

Comment 27

[Reuben Morais [:reuben]]

Attachment: Patch for checkin.

Patch for check-in, carrying r=dao.

Comment 28

[Dão Gottwald [:dao]]

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

Comment 29

[Notlost]

Would it be possible to add a pref for this?