Address bar lost focus after opening a new tab using Electrolysis

VERIFIED FIXED in Firefox 36

Status

()

Firefox
General
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Daniel "warmth" Delgado, Assigned: mconley)

Tracking

(Blocks: 1 bug, {regression})

34 Branch
Firefox 34
x86_64
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(e10s+, firefox36 verified)

Details

Attachments

(2 attachments, 8 obsolete attachments)

30.64 KB, application/x-gzip
Details
4.55 KB, patch
dao
: review+
mconley
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140807030202

Steps to reproduce:

Since the latest version of the Nightly x64, opening a new tab via CTRL+N or the new tab button, doesn't give the focus to the URL (Address) bar as it supposed (used) to do...
(Reporter)

Updated

4 years ago
Hardware: x86 → x86_64
Duplicate of this bug: 1050503
tracking-e10s: --- → ?
I noticed this recently on OS X.
Blocks: 653064
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, regressionwindow-wanted
Assignee: nobody → mconley

Comment 3

4 years ago
Regression window(fx)
Good:
https://hg.mozilla.org/integration/fx-team/rev/4bd09430f063
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140805142615
Bad:
https://hg.mozilla.org/integration/fx-team/rev/06b6aa5c734d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140805144123
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=4bd09430f063&tochange=06b6aa5c734d

Regressed by: Bug 1009628
Blocks: 1009628
Ok, I think I've figured out what's going wrong here.

BrowserOpenTab calls openUILinkIn, which calls openLinkIn, which calls focusAndSelectUrlBar(), which does exactly what it says it's going to do.

In the e10s case, the openUILinkIn call occurs after the _adjustFocusAfterTabSwitch is called. So the last focus is the focus to the URL bar.

In the e10s case, with bug 1009628, we wait for the tab to be ready to paint before we call _adjustFocusAfterTabSwitch. That means that _adjustFocusAfterTabSwitch is called last, which means that the content gets focused.

So we've accidentally changed the order of events here. Let me try to come up with some options on fixing this now...
Keywords: regressionwindow-wanted
OS: Windows 7 → All
Pretty sure this was supposed to be tracked and M2'd in yesterday's meeting.
Blocks: 997462
tracking-e10s: ? → +
Duplicate of this bug: 1050798
Created attachment 8470096 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?
Comment on attachment 8470096 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

So this is my first naive solution to this problem. For new tabs that we're opening, if the URL is about:newtab or about:blank, we skip focusing the content when it's ready the first time. We do this by setting a _skipFocus property on the tab object, which is detected and removed in _adjustFocusAfterTabSwitch before returning early.

I'm not 100% sure this is the right way to go about things. I've got a try push here to see if this breaks any assumptions our tests make:

https://tbpl.mozilla.org/?tree=Try&rev=c9589a0610e2

My other idea was that maybe we can have setting the URL bar focus be something that _adjustFocusAfterTabSwitch takes care of, instead of making openLinkIn take care of it.

Dao, any thoughts on this?
Attachment #8470096 - Flags: feedback?(dao)
Created attachment 8470137 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?
Attachment #8470096 - Attachment is obsolete: true
Attachment #8470096 - Flags: feedback?(dao)
Comment on attachment 8470137 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Some silly mistakes in that last patch resulted in some major orange in that try push. I failed to define aSkipFocusContent in some scopes, which caused it to be assigned globally, causing leaks and other mayhem.

I've re-pushed to try with these errors corrected:

https://tbpl.mozilla.org/?tree=Try&rev=377e920ba4cf
Attachment #8470137 - Flags: feedback?(dao)
Comment on attachment 8470137 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

openLinkIn itself could set a flag on the tab return by loadOneTab without adding a parameter specifically for this.
Attachment #8470137 - Flags: feedback?(dao)

Updated

4 years ago
Duplicate of this bug: 1051181
Comment on attachment 8470137 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

(In reply to Dão Gottwald [:dao] (on vacation till August 15) from comment #11)
> Comment on attachment 8470137 [details] [diff] [review]
> Skip focusing content if we're opening a new about:blank or about:newtab
> tab. r=?
> 
> openLinkIn itself could set a flag on the tab return by loadOneTab without
> adding a parameter specifically for this.

Hm, yes, it could - although that results in more divergence between the code that handles e10s tabs and non-e10s tabs, since that new flag wouldn't have a chance to be read by _adjustFocusAfterTabSwitch for non-e10s (since it'll be run synchronously). So we end up specially setting this flag for e10s, which sounds non-obvious.

I see you're on PTO now, so I'll redirect my feedback / review requests.

What do you think of this, ttaubert?
Attachment #8470137 - Flags: feedback?(ttaubert)
Status: NEW → ASSIGNED
Comment on attachment 8470137 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Thoughts if there are better ways of doing this, Blair?
Attachment #8470137 - Flags: feedback?(bmcbride)
Comment on attachment 8470137 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Review of attachment 8470137 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, I guess this works. Feels a bit iffy though.

An alternative could be to give focusAndSelectUrlBar() the tab as an argument, and have it set ._urlbarFocused - which _adjustFocusAfterTabSwitch() will then use.
Attachment #8470137 - Flags: feedback?(bmcbride) → feedback+
Created attachment 8472523 [details] [diff] [review]
Set _urlbarFocused on a newly created about:newtab or about:blank tab. r=?
Attachment #8470137 - Attachment is obsolete: true
Attachment #8470137 - Flags: feedback?(ttaubert)
Created attachment 8472525 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?
Attachment #8472523 - Attachment is obsolete: true
Comment on attachment 8472525 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

So this was pretty straight forward - I just poke the private _urlbarFocused attribute on the tab if it's a new one. Doesn't feel great mucking about with a private attribute, but I can't see any other way around that except by adding another argument to loadOneTab and friends.
Attachment #8472525 - Flags: review?(bmcbride)
Comment on attachment 8472525 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Review of attachment 8472525 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, that ended up not so bad.

Any chance of a test?
Attachment #8472525 - Flags: review?(bmcbride) → review+
(In reply to Blair McBride [:Unfocused] from comment #20)
> Comment on attachment 8472525 [details] [diff] [review]
> Skip focusing content if we're opening a new about:blank or about:newtab
> tab. r=?
> 
> Review of attachment 8472525 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ah, that ended up not so bad.
> 
> Any chance of a test?

Yep! browser/browser/base/content/test/general/browser_bug565575.js already tests this, but we don't have e10s tests enabled for mochitest-bc just yet, which is why we didn't see this. When those get flipped on, we'll have test coverage.
Thanks for the review!

remote:   https://hg.mozilla.org/integration/fx-team/rev/3031346b1104
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3031346b1104
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
How does this patch work? Where is tab._urlbarFocused being read?
Flags: needinfo?(mconley)
(In reply to Dão Gottwald [:dao] (on vacation till August 15) from comment #25)
> How does this patch work? Where is tab._urlbarFocused being read?

This patch works by hooking into a pre-existing private property of the tab object. That property is used when switching back to about:blank or about:newtab tabs.

The property is read in _adjustFocusAfterTabSwitch.
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) from comment #26)
> (In reply to Dão Gottwald [:dao] (on vacation till August 15) from comment
> #25)
> > How does this patch work? Where is tab._urlbarFocused being read?
> 
> The property is read in _adjustFocusAfterTabSwitch.

No, it isn't. That code reads newBrowser._urlbarFocused.

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You're right. This patch didn't fix the problem at all. Strange - I felt like I'd tested it, but you're right about _urlbarFocused only being read off the browser and not the tab.

Sorry - that was careless of me. Backing out...

remote:   https://hg.mozilla.org/integration/fx-team/rev/a4e46fd6959b
Created attachment 8474145 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

This fixes a change in order of focus events for e10s after bug 1009628 landed.
We were accidentally focusing the content after focusing the URL bar for new
tabs. We now skip focusing the content entirely when opening a new tab.
Attachment #8472525 - Attachment is obsolete: true
Comment on attachment 8474145 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Ok, did a thorough testing this time - both with and without an e10s window.

Fixes the issue with an e10s window, and browser/base/content/test/general/browser_bug565575.js (which exercises the URL bar select/focus state on new tab openings), all passes for non-e10s.

I simply needed to set the _urlbarFocused property on the associated browser for the tab, and not the tab itself.
Attachment #8474145 - Flags: review?(dao)
Comment on attachment 8474145 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

newBrowser._urlbarFocused makes tabbrowser.xml close the autocomplete popup, which doesn't seem right in case the user started typing after openLinkIn called focusAndSelectUrlBar.
Attachment #8474145 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] (on vacation till August 15) from comment #31)
> Comment on attachment 8474145 [details] [diff] [review]
> Skip focusing content if we're opening a new about:blank or about:newtab
> tab. r=?
> 
> newBrowser._urlbarFocused makes tabbrowser.xml close the autocomplete popup,
> which doesn't seem right in case the user started typing after openLinkIn
> called focusAndSelectUrlBar.

I see, yes. We want the chrome to act synchronously here, so I guess that means we have to go with my original approach of skipping focus adjustment altogether.

Are you still interested in your original suggestion of setting some property on the tab (or browser!) after openLinkIn is called, and having _adjustFocusAfterTabSwitch return early (and delete the property) if it finds it?
Flags: needinfo?(dao)
(In reply to Mike Conley (:mconley) from comment #32)
> Are you still interested in your original suggestion of setting some
> property on the tab (or browser!) after openLinkIn is called, and having
> _adjustFocusAfterTabSwitch return early (and delete the property) if it
> finds it?

yes
Flags: needinfo?(dao)
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a4e46fd6959b
Target Milestone: Firefox 34 → ---
Created attachment 8474155 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

This fixes a change in order of focus events for e10s after bug 1009628 landed.
We were accidentally focusing the content after focusing the URL bar for new
tabs. We now skip focusing the content entirely when opening a new tab.
Comment on attachment 8474155 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

So something more along these lines?
Attachment #8474155 - Flags: review?(dao)
Attachment #8474145 - Attachment is obsolete: true
Component: Untriaged → General
Duplicate of this bug: 1055277
Comment on attachment 8474155 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

jimm asked me to let him do first-pass reviews. He'll pass the review back to me when he's done.
Attachment #8474155 - Flags: review?(dao) → feedback?(jmathies)

Updated

4 years ago
Duplicate of this bug: 1055440

Comment 40

4 years ago
This isn't working for me in a new e10s window. ping me when you get in.
(In reply to Jim Mathies [:jimm] from comment #40)
> This isn't working for me in a new e10s window. ping me when you get in.

Argh - I never tested with my browser.newtab.url set to about:newtab. I had it set to about:home.

Thanks for catching this - let me come up with something else.
Attachment #8474155 - Flags: feedback?(jmathies)

Updated

4 years ago
Duplicate of this bug: 1055581
Duplicate of this bug: 1055505
Created attachment 8475284 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

This fixes a change in order of focus events for e10s after bug 1009628 landed.
We were accidentally focusing the content after focusing the URL bar for new
tabs. We now skip focusing the content entirely when opening a new tab.
Sorry for the delay, was chasing my tail trying to figure out why about:home wouldn't get its search input focused after loading. Filed bug 1055659 about that, since it appears to be separate.
Comment on attachment 8475284 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Feel like giving this a spin, jimm?
Attachment #8475284 - Flags: feedback?(jmathies)
Attachment #8474155 - Attachment is obsolete: true
Comment on attachment 8475284 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Hey ally, now that you've got new shiny gavin-endowed browser/ review powers, feel like taking a crack at this one?
Attachment #8475284 - Flags: review?(ally)
sure, first thing tomorrow. I'm on pto this afternoon.

Comment 49

4 years ago
Comment on attachment 8475284 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Review of attachment 8475284 [details] [diff] [review]:
-----------------------------------------------------------------

This is working as expected with both e10s and non-e10s windows.

::: browser/base/content/utilityOverlay.js
@@ +341,5 @@
> +                                inBackground: loadInBackground,
> +                                allowThirdPartyFixup: aAllowThirdPartyFixup,
> +                                relatedToCurrent: aRelatedToCurrent,
> +                                skipAnimation: aSkipTabAnimation,
> +                                allowMixedContent: aAllowMixedContent });

nit, I would prefer we wrap these down (old metro front end standard) such that the code isn't hanging out like it is here, for example:

newTab = browser.loadOneTab(url, {
  referrerURI: aReferrerURI,
  ..
});

you can also get rid of the 'browser' assignment. I'll leave it up to you though.
Attachment #8475284 - Flags: feedback?(jmathies) → feedback+
Created attachment 8475448 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. feedback=jimm, r=?

This fixes a change in order of focus events for e10s after bug 1009628 landed.
We were accidentally focusing the content after focusing the URL bar for new
tabs. We now skip focusing the content entirely when opening a new tab.
Comment on attachment 8475448 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. feedback=jimm, r=?

Thanks jimm - the suggestions are good, and I've taken them.
Attachment #8475448 - Attachment description: Skip focusing content if we're opening a new about:blank or about:newtab tab. r=? → Skip focusing content if we're opening a new about:blank or about:newtab tab. feedback=jimm, r=?
Attachment #8475448 - Flags: review?(ally)
Attachment #8475448 - Flags: feedback+
Attachment #8475284 - Attachment is obsolete: true
Attachment #8475284 - Flags: review?(ally)
Comment on attachment 8475448 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. feedback=jimm, r=?

>-        fm.setFocus(newBrowser, focusFlags);
>+        if (newTab._skipContentFocus) {
>+          // It's possible the tab we're switching to is ready to focus asynchronously,
>+          // when we've already focused something else. In that case, this
>+          // _skipContentFocus property can be set so that we skip focusing the
>+          // content after we switch tabs.
>+          delete newTab._skipContentFocus;
>+        } else {
>+          fm.setFocus(newBrowser, focusFlags);
>+        }

Please make this an early return after the "Otherwise, focus the content area" comment.

>+      // Remote tab content does not focus synchronously, so we set the flag
>+      // on this tab to skip focusing the content if we want to focus the URL
>+      // bar instead.
>+      newTab._skipContentFocus = true;

That comment can be somewhat misleading. fm.setFocus(newBrowser, focusFlags) should be synchronous as far as tabbrowser is concerned, it's just that we call it asynchronously.

How about something like: "Remote browsers are switched to asynchronously, and we need to ensure that the location bar remains focused in that case rather than the content area being focused."
Attachment #8475448 - Flags: review?(ally) → review-
Duplicate of this bug: 1055712
Created attachment 8475953 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. feedback=jimm, r=?

This fixes a change in order of focus events for e10s after bug 1009628 landed.
We were accidentally focusing the content after focusing the URL bar for new
tabs. We now skip focusing the content entirely when opening a new tab.
Comment on attachment 8475953 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. feedback=jimm, r=?

Like this?
Attachment #8475953 - Attachment description: Skip focusing content if we're opening a new about:blank or about:newtab tab. r=? → Skip focusing content if we're opening a new about:blank or about:newtab tab. feedback=jimm, r=?
Attachment #8475953 - Flags: review?(dao)
Attachment #8475953 - Flags: feedback+
Attachment #8475448 - Attachment is obsolete: true

Updated

4 years ago
Attachment #8475953 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/785c653b41c3
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Depends on: 1057616
Flags: qe-verify+
I was able to reproduce this bug on Nightly 34.0a1 (2014-08-07), using Ubuntu 13.10 x64 and Windows 7 x64.

Verified fixed on Windows 7 x64, Ubuntu 13.10 x64 and Mac OSX 10.8.5 using Nightly 36.0a1 (2014-11-07).
Status: RESOLVED → VERIFIED
status-firefox36: --- → verified

Updated

3 years ago
Blocks: 1108555
You need to log in before you can comment on or make changes to this bug.