1379270 - Urlbar is focused and selected too early on switch to new tab.

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

When opening a new tab, in updateDisplay we call                     this.tabbrowser._adjustFocusAfterTabSwitch(showTab) at https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml?q=path%3Atabbrowser.xml&redirect_type=single#4273

But the tab doesn't switch until somewhere after https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml?q=path%3Atabbrowser.xml&redirect_type=single#8061  (Sadly I lost my breakpoint so I forget where…)

This isn't noticeable normally, because there's no url for the newtab page, but when it's overridden (by a WebExtension, say), then you can see the new url gets unselected, leaving the cursor at the end of the urlbar, leading to the behaviour in bug 1372996.

I'm happy to write the patch, if people can help me figure out where the focusAndSelectUrlBar() call should go…

Comment 1

[:Gijs (out for now; he/him)]

Mike, given you fixed bug 1367621, do you have ideas here?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Attachment: temp.diff

So, I feel like this is wrong enough that I don't want to ask for review yet, but it seems to work, so I figured I'd post it for Mike to take a look at and let me know what I'm doing wrong.  ;D

Comment 3

[Blake Winton (:bwinton) (:☕️)]

(If anyone else wants to comment on how this could be made better, I would also love to hear their ideas!)

Comment 4

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

Yes, the problem here is that the browser currentURI is about:blank (even though we've requested a mozextension://) since the page is still loading. We focus the URL bar immediately. Then the page finishes loading and we update the URL bar.

Updating a text input after it has been selected causes the selection to clear.

Still thinking about what we need to do here. I'd really like to avoid yet another focus() or select() if we can, since that triggers a style flush.

Comment 5

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

Comment on attachment 8884990 [details] [diff] [review]
temp.diff

This focus stuff is a bit of a mess. Sorry it's taken a bit to unravel it - there's some security stuff I've had to ensure we're not re-opening. :/

I think ultimately, what we should try to do is run the _adjustFocusAfterTabSwitch step for the shown tab after the tab select event is fired if we've switched synchronously.

So I think this approach might work:

a) Move the firing of the "select" event inside the requestTab method of the tab switcher, after postActions
b) Set a bool inside of requestTab that's cleared on exit
c) Inside updateDisplay, around [2], if that requestTab bool is set, instead of calling _adjustFocusAfterTabSwitch immediately, maybe set a callback with that _adjustFocusAfterTabSwitch somewhere on the switcher instead
d) Inside requestTab, after postActions, and after we fire the select event, if the callback is set, fire it - that should cause the focus to occur after the "select" event is fired.

That's the best I've got, I'm afraid. I'm open to other, simpler suggestions that don't introduce more focus / select calls!

[1]: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/tabbrowser.xml#8076-8085
[2]: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/tabbrowser.xml#4294

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Attachment: Bug 1379270 - Focus the urlbar a little later. .

Review commit: https://reviewboard.mozilla.org/r/160972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160972/

Comment 8

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

Comment on attachment 8889918 [details]
Bug 1379270 - Focus the urlbar a little later. .

https://reviewboard.mozilla.org/r/160972/#review167522

Assuming a green try run, I'm good with this. Thanks bwinton!

::: browser/base/content/tabbrowser.xml:4310
(Diff revision 1)
>                  let index = Array.indexOf(tabPanel.childNodes, showPanel);
>                  if (index != -1) {
>                    this.log(`Switch to tab ${index} - ${this.tinfo(showTab)}`);
>                    tabPanel.setAttribute("selectedIndex", index);
>                    if (showTab === this.requestedTab) {
> +                    if (this._switchingTab) {

Nit: I think `_requestingTab` might be a bit clearer.

::: browser/base/content/tabbrowser.xml:4311
(Diff revision 1)
>                  if (index != -1) {
>                    this.log(`Switch to tab ${index} - ${this.tinfo(showTab)}`);
>                    tabPanel.setAttribute("selectedIndex", index);
>                    if (showTab === this.requestedTab) {
> +                    if (this._switchingTab) {
> +                      this.tabbrowser.addEventListener("select", () => {

We should add a comment explaining why this is necessary. Specifically, we should point out that if requestTab is on the stack, this means that we're switching the visibility of the tab synchronously, and that we need to wait for the "select" event before shifting focus so that `_adjustFocusAfterTabSwitch` runs with the right information for the tab switch.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8889918 [details]
Bug 1379270 - Focus the urlbar a little later. .

https://reviewboard.mozilla.org/r/160972/#review167522

> Nit: I think `_requestingTab` might be a bit clearer.

Fixed.

> We should add a comment explaining why this is necessary. Specifically, we should point out that if requestTab is on the stack, this means that we're switching the visibility of the tab synchronously, and that we need to wait for the "select" event before shifting focus so that `_adjustFocusAfterTabSwitch` runs with the right information for the tab switch.

Added!

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8889918 [details]
Bug 1379270 - Focus the urlbar a little later. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160972/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ef9bf18b361
Focus the urlbar a little later. r=mconley.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ef9bf18b361