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

So I think I rejigged some of the focus stuff in that patch, and moved it into its own function. I'll look into that now.

"_adjustFocusAfterTabSwitch" is the name of the function in the tabbrowser binding (tabbrowser.xml).

// Focus the location bar if it was previously focused for that tab.

This code seems like the relevant stuff in that function.

So is it running? Browser Toolbox to the rescue! I think I'm going to try attaching remotely, since we're dealing with focus. I'll attach my MacBook's Firefox devtools to my PC's Firefox.

Wow, it'd help if I wasn't on the guest network. That wasted me like 10 minutes. :/ I've removed the guest network from my list of networks.

BAH. Remote debugging is totally horked. Looks like I should try this locally instead. Maybe I can live with that.

newBrowser._urlbarFocused isn't defined… that's why we're not entering this block. What happens in non-e10s? So I think _urlbarFocused would only ever be set if this browser had been looked at before. So I don't think that's what's wrong here.

_adjustFocusAfterSwitch uses focusAndSelectUrlBar to, well, focus and select the URL bar. I wonder if there are other callers to this that are failing - maybe this function isn't the one I need to care about, even though it seems like it should be…

Hm - when printf debugging, it looks like _adjustFocusAfterTabSwitch always just focuses the browser. Interesting… so my suspicion was right: something else is focusing and selecting the URL bar.

I'll set a breakpoint on focusAndSelectUrlBar to see if anybody else calls it.

Yep, we have another caller:

Ah hah, so, theory: old code assumed that _adjustFocusAfterTabSwitch had already run before BrowserOpenTab's openUILinkIn / openLinkIn calls focusAndSelectUrlBar. That assumption is now false because _adjustFocusAfterTabSwitch occurs asynchronously now.

Time to prove it. I've got my printf's in the _adjustFocusAfterTabSwitch method. If I see those print out before I hit my focusAndSelectUrlBar breakpoint in non-e10s, and then see them print out _after_ I hit focusAndSelectUrlBar in e10s, then I've figured it out.

non-e10s:

• Saw printf's and then hit breakpoint

• Saw printf's and then hit breakpoint

NO I was right. I think it was just that the console wasn't flushing fast enough. A breakpoint in the nsFocusManager::SetFocus and repeated calls to DumpJSStack() confirms that we're calling _adjustFocusAfterTabSwitch first in the non-e10s case, and last in the e10s case.

Possible solutions:

• This seems to be only a problem with new tabs, so perhaps we can get tabbrowser.xml to skip _adjustFocusAfterTabSwitch if the tab switch is caused by us opening a new tab?
• Can we make openUILinkIn / openLinkIn call focusAndSelectUrlBar after MozAfterRemotePaint is fired?
• • This worries me - that'd mean that there's the potential for 300 milliseconds of no focus switch (while we wait for the timeout promise to execute), and the chrome interaction is supposed to be as instantaneous as possible…

if (!loadInBackground && w.isBlankPageURL(url))

That's from utilityOverlay.js's openLinkIn function… is it possible to have this wait for MozAfterRemotePaint to focus?

Perhaps we can move the responsibility of focusing the URL bar from the utility overlay and into tabbrowser.xml…

I tried my first idea - add an option to loadOneTab and addTab which adds a one-time ability on that tab to skip focus after it's been opened. Dao might have some reservations about this, but at least it gets the ball rolling.

remote: You can view the progress of your build at the following URL:

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

remote: Alternatively, view them on Treeherder (experimental):

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

I've asked Dao for feedback on my patch, we'll see what he thinks.

Ew - try results are in.

23 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/modules/test/browser_BrowserUITelemetry_buckets.js | leaked window property: aSkipFocusContent - leaked window property: aSkipFocusContent

185 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/indexedDB/test/browser_perwindow_privateBrowsing.js | uncaught exception - ReferenceError: aSkipFocusContent is not defined at chrome://browser/content/tabbrowser.xml:1619

196 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/indexedDB/test/browser_perwindow_privateBrowsing.js | Test timed out - Test timed out

18 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_NetUtil.js | leaked window property: aSkipFocusContent - leaked window property: aSkipFocusContent

Gah, didn't define the aSkipFocusContent variable, so they're being assigned as globals. Lovely. Fixed.

New try push:

remote: You can view the progress of your build at the following URL:

remote: https://tbpl.mozilla.org/?tree=Try&rev=377e920ba4cf

remote: Alternatively, view them on Treeherder (experimental):

remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=377e920ba4cf

Yeah, that looks way better.

Dao's feedback:

"openLinkIn itself could set a flag on the tab return by loadOneTab without adding a parameter specifically for this."

Hrm. Yeah, it could… I don't like the fact that tab switching code diverges between e10s and non-e10s though.

With my way, the behaviour is a little more consistent - with this new parameter, we skip focusing the content for both e10s and non-e10s.

From Unfocused:

Hm - this has the same problem of slightly diverging between what e10s and non-e10s does, since adjustFocusAfterTabSwitch will still run first in the non-e10s case.

Bah, I guess it's not too, too bad. If both Unfocused and Dao are cool with this, I won't be too worried.

I'll push to try once the trees reopen.

remote: You can view the progress of your build at the following URL:

remote: https://tbpl.mozilla.org/?tree=Try&rev=3a5caa60ff64

remote: Alternatively, view them on Treeherder (experimental):

remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3a5caa60ff64

Whoops - didn't realize that the "urlbarFocused" private property is something that _adjustFocusAfterTabSwitch already looks for. I can take advantage of that though, and use it instead of adding a new one and removing it / bailing early.

I'll push it (again) to try once the trees re-open.

remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b1991abdbdc2 BLARH - pushed an old patch.

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

Woo! r+ from Unfocused. Let's land this puppeh.

TODO:

File a bug on busted remote debugging. Filed bug 1054338.

File a bug to make it easier to copy a call stack from devtools debugger. Filed bug 1054329.