Address bar lost focus after opening a new tab using Electrolysis

VERIFIED FIXED in Firefox 36

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: superbollos, 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
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1050503
(Assignee)

Updated

4 years ago
tracking-e10s: --- → ?
I noticed this recently on OS X.
Blocks: 653064
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, regressionwindow-wanted
(Assignee)

Updated

4 years ago
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
(Assignee)

Comment 4

4 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

4 years ago
Keywords: regressionwindow-wanted
(Assignee)

Updated

4 years ago
OS: Windows 7 → All
(Assignee)

Comment 5

4 years ago
Pretty sure this was supposed to be tracked and M2'd in yesterday's meeting.
Blocks: 997462
tracking-e10s: ? → +
Duplicate of this bug: 1050798
(Assignee)

Comment 7

4 years ago
Created attachment 8470096 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?
(Assignee)

Comment 8

4 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

4 years ago
Created attachment 8470137 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?
(Assignee)

Updated

4 years ago
Attachment #8470096 - Attachment is obsolete: true
Attachment #8470096 - Flags: feedback?(dao)
(Assignee)

Comment 10

4 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 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
Blocks: 1051181

Updated

4 years ago
Duplicate of this bug: 1051181
(Assignee)

Comment 13

4 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

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 14

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

4 years ago
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)
(Assignee)

Comment 17

4 years ago
Created attachment 8472525 [details] [diff] [review]
Skip focusing content if we're opening a new about:blank or about:newtab tab. r=?
(Assignee)

Updated

4 years ago
Attachment #8472523 - Attachment is obsolete: true
(Assignee)

Comment 18

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

4 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

4 years ago
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)
(Assignee)

Comment 26

4 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)
(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 → ---
(Assignee)

Comment 28

4 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

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #8472525 - Attachment is obsolete: true
(Assignee)

Comment 30

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

4 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)
(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 → ---
(Assignee)

Comment 35

4 years ago
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.
(Assignee)

Comment 36

4 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

4 years ago
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.
(Assignee)

Comment 41

4 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

4 years ago
Attachment #8474155 - Flags: feedback?(jmathies)
Duplicate of this bug: 1055581
Duplicate of this bug: 1055505
(Assignee)

Comment 44

4 years ago
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.
(Assignee)

Comment 45

4 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

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

Feel like giving this a spin, jimm?
Attachment #8475284 - Flags: feedback?(jmathies)
(Assignee)

Updated

4 years ago
Attachment #8474155 - Attachment is obsolete: true
(Assignee)

Comment 47

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

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+
(Assignee)

Comment 50

4 years ago
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.
(Assignee)

Comment 51

4 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

4 years ago
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-

Updated

4 years ago
Duplicate of this bug: 1055712
(Assignee)

Comment 54

4 years ago
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.
(Assignee)

Comment 55

4 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

4 years ago
Attachment #8475448 - Attachment is obsolete: true

Updated

4 years ago
Attachment #8475953 - Flags: review?(dao) → review+
(Assignee)

Comment 56

4 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/785c653b41c3
Whiteboard: [fixed-in-fx-team]
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

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