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

I noticed this recently on OS X.

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

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

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

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

Here's a try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5930f31837d5

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?

(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

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

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

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

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

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

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

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

(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

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

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?

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.

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.

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?

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?

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

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.

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.

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

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?

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

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