Last Comment Bug 490225 - CMD-T opens a new window instead of a new tab when Downloads window is in focus
: CMD-T opens a new window instead of a new tab when Downloads window is in focus
Status: RESOLVED FIXED
[good first bug]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 2 votes (vote)
: Firefox 11
Assigned To: Reuben Morais [:reuben]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-26 09:24 PDT by Tony Chung [:tchung]
Modified: 2012-01-10 14:46 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix it (1.32 KB, patch)
2011-02-15 03:26 PST, Gleb M. Borisov
no flags Details | Diff | Splinter Review
Fix it (1.32 KB, patch)
2011-02-15 03:46 PST, Gleb M. Borisov
no flags Details | Diff | Splinter Review
BrowserOpenTab, focusAndSelectUrlBar refactoring (1.99 KB, patch)
2011-03-24 05:27 PDT, Gleb M. Borisov
dao+bmo: review-
Details | Diff | Splinter Review
Partial fix. (1.96 KB, patch)
2011-12-05 18:57 PST, Reuben Morais [:reuben]
no flags Details | Diff | Splinter Review
Fix. (3.01 KB, patch)
2011-12-05 19:08 PST, Reuben Morais [:reuben]
dao+bmo: review-
Details | Diff | Splinter Review
Patch. (3.01 KB, patch)
2011-12-07 10:53 PST, Reuben Morais [:reuben]
dao+bmo: review-
Details | Diff | Splinter Review
Patch v3 (2.80 KB, patch)
2011-12-20 03:33 PST, Reuben Morais [:reuben]
dao+bmo: review+
Details | Diff | Splinter Review
Patch for checkin. (1.69 KB, patch)
2011-12-20 04:57 PST, Reuben Morais [:reuben]
reuben.bmo: review+
Details | Diff | Splinter Review

Description Tony Chung [:tchung] 2009-04-26 09:24:40 PDT
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: gavin@gavinsharp.com] 2009-04-26 10:05:45 PDT
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 2010-03-16 20:58:33 PDT
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) 2010-03-16 21:07:47 PDT
(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: gavin@gavinsharp.com] 2010-03-21 03:18:58 PDT
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) 2010-03-21 04:24:46 PDT
(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 2011-02-15 03:26:44 PST
Created attachment 512442 [details] [diff] [review]
Fix it
Comment 7 Gleb M. Borisov 2011-02-15 03:46:19 PST
Created attachment 512445 [details] [diff] [review]
Fix it

Same patch, but after pull/update.
Comment 8 Dão Gottwald [:dao] 2011-02-28 05:57:33 PST
Can you just call openUILinkIn("about:blank", "tab");?
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-03-10 12:27:09 PST
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 2011-03-24 05:27:33 PDT
Created attachment 521467 [details] [diff] [review]
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] 2011-03-24 05:49:41 PDT
(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 2011-03-25 15:59:36 PDT
(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] 2011-03-26 02:35:28 PDT
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] 2011-03-26 02:36:25 PDT
> 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] 2011-04-29 04:39:55 PDT
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 ) 2011-10-28 11:52:20 PDT
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] 2011-12-05 18:57:52 PST
Created attachment 579192 [details] [diff] [review]
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] 2011-12-05 19:08:25 PST
Created attachment 579196 [details] [diff] [review]
Fix.

Actually, I missed content.focus() being called after my code.
This patch fixes it for real.
Comment 19 Dão Gottwald [:dao] 2011-12-06 00:57:41 PST
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] 2011-12-06 02:34:39 PST
(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: gavin@gavinsharp.com] 2011-12-07 10:37:17 PST
You should be able to use w.focusAndSelectUrlBar().
Comment 22 Reuben Morais [:reuben] 2011-12-07 10:53:12 PST
Created attachment 579753 [details] [diff] [review]
Patch.

(In reply to Gavin Sharp (use gavin@gavinsharp.com 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] 2011-12-11 07:53:30 PST
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] 2011-12-20 01:10:34 PST
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] 2011-12-20 03:33:33 PST
Created attachment 583110 [details] [diff] [review]
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] 2011-12-20 04:16:39 PST
Comment on attachment 583110 [details] [diff] [review]
Patch v3

thanks!
Comment 27 Reuben Morais [:reuben] 2011-12-20 04:57:52 PST
Created attachment 583117 [details] [diff] [review]
Patch for checkin.

Patch for check-in, carrying r=dao.
Comment 28 Dão Gottwald [:dao] 2011-12-20 05:07:45 PST
http://hg.mozilla.org/mozilla-central/rev/f7a00571c87f
Comment 29 Notlost 2012-01-10 14:46:20 PST
Would it be possible to add a pref for this?

Note You need to log in before you can comment on or make changes to this bug.