1050447 - Address bar lost focus after opening a new tab using Electrolysis

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Daniel "warmth" Delgado]

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...

Comment 2

[Chris Peterson [:cpeterson]]

I noticed this recently on OS X.

Comment 3

[Alice0775 White]

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

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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...

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Pretty sure this was supposed to be tracked and M2'd in yesterday's meeting.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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

Comment 11

[Dão Gottwald [: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.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Set _urlbarFocused on a newly created about:newtab or about:blank tab. r=?

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Here's a try run:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5930f31837d5

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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?

Comment 21

[Mike Conley (:mconley) (:⚙️)]

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

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Thanks for the review!

remote:   https://hg.mozilla.org/integration/fx-team/rev/3031346b1104

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3031346b1104

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: bug-1050447-notes.tar.gz

Bug notes:

http://people.mozilla.org/~mconley2/bugnotes/bug-1050447.html

Comment 25

[Dão Gottwald [:dao]]

How does this patch work? Where is tab._urlbarFocused being read?

Comment 26

[Mike Conley (: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.

Comment 27

[Dão Gottwald [:dao]]

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

Comment 28

[Mike Conley (:mconley) (:⚙️)]

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

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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 30

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 31

[Dão Gottwald [: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.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

(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?

Comment 33

[Dão Gottwald [: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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a4e46fd6959b

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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 36

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 38

[Dão Gottwald [:dao]]

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.

Comment 40

[Jim Mathies [:jimm]]

This isn't working for me in a new e10s window. ping me when you get in.

Comment 41

[Mike Conley (:mconley) (:⚙️)]

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

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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 45

[Mike Conley (:mconley) (:⚙️)]

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 46

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 47

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 48

[Allison Naaktgeboren :ally]

sure, first thing tomorrow. I'm on pto this afternoon.

Comment 49

[Jim Mathies [:jimm]]

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.

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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 51

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 52

[Dão Gottwald [:dao]]

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."

Comment 54

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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 55

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 56

[Mike Conley (:mconley) (:⚙️)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/785c653b41c3

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/785c653b41c3

Comment 58

[Vasilica Tamas, QA [:vtamas]]

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).