Closed
Bug 1050447
Opened 10 years ago
Closed 10 years ago
Address bar lost focus after opening a new tab using Electrolysis
Categories
(Firefox :: General, defect)
Tracking
()
VERIFIED
FIXED
Firefox 34
People
(Reporter: superbollos, Assigned: mconley)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(2 files, 8 obsolete files)
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•10 years ago
|
Hardware: x86 → x86_64
Assignee | ||
Updated•10 years ago
|
tracking-e10s:
--- → ?
Comment 2•10 years ago
|
||
I noticed this recently on OS X.
Blocks: fxe10s
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression,
regressionwindow-wanted
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mconley
Comment 3•10 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
Assignee | ||
Comment 4•10 years ago
|
||
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...
Assignee | ||
Updated•10 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Updated•10 years ago
|
OS: Windows 7 → All
Assignee | ||
Comment 5•10 years ago
|
||
Pretty sure this was supposed to be tracked and M2'd in yesterday's meeting.
Blocks: old-e10s-m2
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8470096 -
Attachment is obsolete: true
Attachment #8470096 -
Flags: feedback?(dao)
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8470137 -
Attachment is obsolete: true
Attachment #8470137 -
Flags: feedback?(ttaubert)
Assignee | ||
Comment 17•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8472523 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
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)
Assignee | ||
Comment 19•10 years ago
|
||
Here's a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5930f31837d5
Comment 20•10 years ago
|
||
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+
Assignee | ||
Comment 21•10 years ago
|
||
(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.
Assignee | ||
Comment 22•10 years ago
|
||
Thanks for the review!
remote: https://hg.mozilla.org/integration/fx-team/rev/3031346b1104
Whiteboard: [fixed-in-fx-team]
Comment 23•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Assignee | ||
Comment 24•10 years ago
|
||
bugnotes |
Comment 25•10 years ago
|
||
How does this patch work? Where is tab._urlbarFocused being read?
Flags: needinfo?(mconley)
Assignee | ||
Comment 26•10 years ago
|
||
(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)
Comment 27•10 years ago
|
||
(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•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 28•10 years ago
|
||
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
Assignee | ||
Comment 29•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8472525 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
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 31•10 years ago
|
||
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-
Assignee | ||
Comment 32•10 years ago
|
||
(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)
Comment 33•10 years ago
|
||
(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)
Comment 34•10 years ago
|
||
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a4e46fd6959b
Target Milestone: Firefox 34 → ---
Assignee | ||
Comment 35•10 years ago
|
||
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.
Assignee | ||
Comment 36•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8474145 -
Attachment is obsolete: true
Updated•10 years ago
|
Component: Untriaged → General
Comment 38•10 years ago
|
||
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)
Comment 40•10 years ago
|
||
This isn't working for me in a new e10s window. ping me when you get in.
Assignee | ||
Comment 41•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Attachment #8474155 -
Flags: feedback?(jmathies)
Assignee | ||
Comment 44•10 years ago
|
||
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.
Assignee | ||
Comment 45•10 years ago
|
||
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.
Assignee | ||
Comment 46•10 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=?
Feel like giving this a spin, jimm?
Attachment #8475284 -
Flags: feedback?(jmathies)
Assignee | ||
Updated•10 years ago
|
Attachment #8474155 -
Attachment is obsolete: true
Assignee | ||
Comment 47•10 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=?
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)
Comment 48•10 years ago
|
||
sure, first thing tomorrow. I'm on pto this afternoon.
Comment 49•10 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+
Assignee | ||
Comment 50•10 years ago
|
||
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.
Assignee | ||
Comment 51•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8475284 -
Attachment is obsolete: true
Attachment #8475284 -
Flags: review?(ally)
Comment 52•10 years ago
|
||
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-
Assignee | ||
Comment 54•10 years ago
|
||
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.
Assignee | ||
Comment 55•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8475448 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8475953 -
Flags: review?(dao) → review+
Assignee | ||
Comment 56•10 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 57•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Updated•10 years ago
|
Flags: qe-verify+
Depends on: 1069089
Comment 58•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•