Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 11

Status

()

Firefox
General
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: tchung, Assigned: reuben)

Tracking

unspecified
Firefox 11
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

8 years ago
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
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
Whiteboard: [good first bug]

Comment 2

7 years ago
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

7 years ago
(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.
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

7 years ago
(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

7 years ago
Created attachment 512442 [details] [diff] [review]
Fix it
Attachment #512442 - Flags: review?(gavin.sharp)

Comment 7

7 years ago
Created attachment 512445 [details] [diff] [review]
Fix it

Same patch, but after pull/update.
Attachment #512442 - Attachment is obsolete: true
Attachment #512445 - Flags: review?(gavin.sharp)
Attachment #512442 - Flags: review?(gavin.sharp)
Can you just call openUILinkIn("about:blank", "tab");?
Component: Keyboard Navigation → General
QA Contact: keyboard.navigation → general
Comment on attachment 512445 [details] [diff] [review]
Fix it

Seems like dao's suggestion should work, and is much simpler.
Attachment #512445 - Flags: review?(gavin.sharp)

Comment 10

6 years ago
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.
Attachment #512445 - Attachment is obsolete: true
Attachment #521467 - Flags: review?(gavin.sharp)
Attachment #521467 - Flags: feedback?

Updated

6 years ago
Attachment #521467 - Flags: feedback? → review?(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

6 years ago
(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.
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.
> 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 on attachment 521467 [details] [diff] [review]
BrowserOpenTab, focusAndSelectUrlBar refactoring

per previous comments. Please let me know if something is unclear.
Attachment #521467 - Flags: review?(gavin.sharp)
Attachment #521467 - Flags: review?(dao)
Attachment #521467 - Flags: review-
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.
(Assignee)

Comment 17

6 years ago
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?
Assignee: nobody → reuben.morais
Status: NEW → ASSIGNED
Attachment #579192 - Flags: feedback?(dao)
(Assignee)

Comment 18

6 years ago
Created attachment 579196 [details] [diff] [review]
Fix.

Actually, I missed content.focus() being called after my code.
This patch fixes it for real.
Attachment #521467 - Attachment is obsolete: true
Attachment #579192 - Attachment is obsolete: true
Attachment #579196 - Flags: review?(dao)
Attachment #579192 - Flags: feedback?(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.
Attachment #579196 - Flags: review?(dao) → review-
(Assignee)

Comment 20

6 years ago
(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?
You should be able to use w.focusAndSelectUrlBar().
(Assignee)

Comment 22

6 years ago
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 :)
Attachment #579196 - Attachment is obsolete: true
Attachment #579753 - Flags: review?(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.
Attachment #579753 - Flags: review?(dao) → review-
Reuben, are you going to update your patch? It's really close, addressing my previous comment shouldn't be hard.
(Assignee)

Comment 25

6 years ago
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.
Attachment #579753 - Attachment is obsolete: true
Attachment #583110 - Flags: review?(dao)
Comment on attachment 583110 [details] [diff] [review]
Patch v3

thanks!
Attachment #583110 - Flags: review?(dao) → review+
(Assignee)

Comment 27

6 years ago
Created attachment 583117 [details] [diff] [review]
Patch for checkin.

Patch for check-in, carrying r=dao.
Attachment #583110 - Attachment is obsolete: true
Attachment #583117 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/f7a00571c87f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11

Comment 29

6 years ago
Would it be possible to add a pref for this?
You need to log in before you can comment on or make changes to this bug.