565575 - Retain location bar focus when switching tabs

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[John Wayne Hill (UX Intern)]

(Note: this is filed as part of the “Paper Cut” bugs — we assume that there may be multiple existing bugs on this. Please make them block this bug, and we will de-dupe if they are indeed exactly the same. Thanks!)

To reproduce:
1. open a new tab
2. cause it to loose focus
3. start typing

Recommendation:
We typing in an empty tab, autofocus the user to the location bar

Comment 1

[Frank Yan (:fryn)]

In order to determine smartly whether to select the search bar instead, it's probably best first to fix the information leak of the search bar's contents currently being app-global.

Comment 2

[Michael Monreal [:micmon]]

This sounds great and I think is what Chrome does. I only wonder if there are potential issues with key nav? Then again, it may very well be especially useful for the key nav usecase...

Comment 3

[Tiago Sá]

It's not exactly what Chrome does. Chrome retains the focus on the location bar when you switch to and from another tab from and to an empty tab, but if the focus on the location bar isn't there to begin with (you have to focus something else manually), it doesn't go to the location bar when you switch to the empty tab again. This is a good thing in their case, because their interface is unusually flawed and they need that new tab screen and whatnot, but since Firefox has about:blank for empty tabs, Firefox should ALWAYS focus on the location bar when opening or switching to an empty tab, even if the focus wasn't there before.

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> It's not exactly what Chrome does. Chrome retains the focus on the location bar
> when you switch to and from another tab from and to an empty tab, but if the
> focus on the location bar isn't there to begin with (you have to focus
> something else manually), it doesn't go to the location bar when you switch to
> the empty tab again.

We did this prior to bug 178324. (See tabbrowser.xml changes in the patch attached there.) Restoring that behavior would be the cleanest way to resolve this bug, I think.

Comment 6

[Frank Yan (:fryn)]

Attachment: patch v1

It's quite late in the cycle, so I'd like to avoid making good the enemy of perfect by attempting to land a large, risky patch. As such, this patch just fixes the common case, which is the name of the bug anyway.

(I also added a gURLBar null check to XULBrowserWindow.setOverLink(), in case the user removed the url bar..)

(In reply to comment #4)
> We did this prior to bug 178324. (See tabbrowser.xml changes in the patch
> attached there.) Restoring that behavior would be the cleanest way to resolve
> this bug, I think.

Thanks for the tip. I'll file a followup later to restore the focused elements more robustly using this.

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 482601 [details] [diff] [review]
patch v1

>+                if (gURLBar && newBrowser.currentURI.spec == "about:blank")
>+                  gURLBar.focus();

I think this want to be an if+else (with the existing fm stuff in the else), if for no other reason than clarity (they both shouldn't be doing things since about:blank shouldn't have any <A>'s, if/else makes that clear without readingaa.)

Comment 8

[Dão Gottwald [:dao]]

about:blank can have content.

> data:text/html,<button onclick="var w=open('about:blank', null, null);w.stop();w.document.documentElement.innerHTML='<a href=http://foo>hi!</a>'">x</button>

Comment 9

[Frank Yan (:fryn)]

(In reply to comment #8)
> about:blank can have content.

Forgot about that. Would something like what URLBarSetURI does be sufficient:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2331
i.e.
if (gURLBar && newBrowser.currentURI.spec == "about:blank" && !content.opener)

This would still focus the url bar for tabs that have content due to manually entered javascript urls, but this is an edge case, and we still blank the url bar in those cases already.

Would we want this for about:privatebrowsing too?

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> (In reply to comment #8)
> > about:blank can have content.
> 
> Forgot about that. Would something like what URLBarSetURI does be sufficient:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2331
> i.e.
> if (gURLBar && newBrowser.currentURI.spec == "about:blank" && !content.opener)

Yep.

> Would we want this for about:privatebrowsing too?

I think we should keep this simple as a stop-gap solution for about:blank.

Comment 11

[Black_Ps`]

What about a fix for 3.6 or addon? Sorry for posting here.

Comment 12

[Frank Yan (:fryn)]

(In reply to comment #11)
> What about a fix for 3.6 or addon? Sorry for posting here.

I'm not sure if this is the type of bug for which patches get backported to an older version. If so, let me know. Considering that shipping a solid 4.0 is top priority, I won't have time to work on an addon for this.

Comment 13

[(no longer active)]

(In reply to comment #12)
> (In reply to comment #11)
> > What about a fix for 3.6 or addon? Sorry for posting here.
> 
> I'm not sure if this is the type of bug for which patches get backported to an
> older version. If so, let me know. Considering that shipping a solid 4.0 is top
> priority, I won't have time to work on an addon for this.

Frank, did you reset the blocking flag intentionally?

Comment 14

[Frank Yan (:fryn)]

(In reply to comment #13)
> Frank, did you reset the blocking flag intentionally?

Ack! Firefox's form cache/restore stabs me again!

Beltzner, could you set it to blocking+ again, please?

I ought to disable that feature. :\

Comment 15

[Frank Yan (:fryn)]

Apparently, my patch even focuses the url bar when opening links in foreground tabs. :\

I guess currentURI.spec is actually set later. Will investigate.

Comment 16

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

You probably want isTabEmpty(this.selectedTab) - that also checks whether the tab is busy, which will catch the case of an initially about:blank tab in the progress of being being navigated somewhere else.

That's probably not good enough to catch the loadOneTab() case (or any other case of addTab(<URI>) immediately followed by a set of selectedTab), though, since the load will have been triggered (via addTab) but the progress events that mark the tab busy won't have been fired yet. For that, you'll need some other indication... I suppose you may be able to check newBrowser.userTypedValue (should only be non-null if a load was triggered), but that's a bit hacky.

Comment 17

[Frank Yan (:fryn)]

Not sure how to fix this. isTabEmpty fixes the case of focusing the url when opening links in foreground tabs, but neither !content.opener nor gURLBar.userTypedValue null check fixes the testcase from comment #8.

Comment 18

[Dão Gottwald [:dao]]

Morphing -- it's going to be easier to just restore pre-bug 178324 behavior for the location bar.

Comment 19

[Dão Gottwald [:dao]]

Attachment: patch

Comment 20

[Frank Yan (:fryn)]

(In reply to comment #19)
> Created attachment 487238 [details] [diff] [review]
> patch
> +              do {
> +                // Focus the location bar if it was previously focused for > that tab.
> +                oldBrowser._urlbarFocused = (gURLBar && gURLBar.focused);
> +                if (newBrowser._urlbarFocused && gURLBar) {
> +                  gURLBar.focus();
> +                  break;
> +                }
> +
> +                // If the find bar is focused, keep it focused.
> +                if (gFindBarInitialized &&
> +                    !gFindBar.hidden &&
> +                    gFindBar.getElement("findbar-textbox").getAttribute("focused") == "true")
> +                  break;


For new tabs, we might want to use focusAndSelectUrlBar() instead of gURLBar.focus(), since that works in fullscreen mode too.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1967

Do we want "do { ... } while (false);" for just 1 break towards the end of a function?

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> For new tabs, we might want to use focusAndSelectUrlBar() instead of
> gURLBar.focus(), since that works in fullscreen mode too.
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1967

Good point.

> Do we want "do { ... } while (false);" for just 1 break towards the end of a
> function?

Not sure what you mean here.

Comment 22

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

He means "just use return rather than using a loop with break, since this is at the end of the function anyways". I don't feel very strongly either way.

Comment 23

[Dão Gottwald [:dao]]

I'd prefer to keep it as is, as that code doesn't necessarily need to remain at the end of the function.

Comment 24

[Dão Gottwald [:dao]]

Attachment: patch v2

fullscreen behavior fixed

Comment 25

[Asaf Romano (gone)]

Comment on attachment 488304 [details] [diff] [review]
patch v2

r=mano

Comment 26

[Dão Gottwald [:dao]]

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

Comment 27

[Mats Palmgren (inactive)]

Did this cause a new intermittent leak (on OSX debug):
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 345083 bytes during test execution
Analyze the leak.TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 146 instances of AtomImpl with size 20 bytes each (2920 bytes total)
Analyze the leak.TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 24 bytes
Analyze the leak.TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of CSSImportRuleImpl with size 44 bytes each (88 bytes total)
Analyze the leak.TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 42 instances of CSSImportantRule with size 16 bytes each (672 bytes total)
Analyze the leak.TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 11 instances of CSSNameSpaceRuleImpl with size 40 bytes each (440 bytes total)

Comment 28

[Mats Palmgren (inactive)]

on Win debug:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289296482.1289301398.28342.gz
on OSX debug:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289295927.1289298437.13584.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289299729.1289302337.32701.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289304987.1289307537.29728.gz

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #27)
> Did this cause a new intermittent leak (on OSX debug):

I don't see how it would...

Comment 30

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

Verified fixed on OS X and Windows with builds like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101111 Firefox/4.0b8pre

Given the automated tests I don't think we need a manual one here.

Comment 31

[Alex Limi (:limi) — Firefox UX Team]

Filed follow-up as bug 615728, since the issue that was originally filed here wasn't actually addressed. :)