Closed Bug 1379270 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: bwinton, Assigned: bwinton)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

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…
Mike, given you fixed bug 1367621, do you have ideas here?
Flags: needinfo?(mconley)
Flags: in-testsuite?
Attached patch temp.diffSplinter Review
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
Attachment #8884990 - Flags: feedback?(mconley)
(If anyone else wants to comment on how this could be made better, I would also love to hear their ideas!)
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 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
Flags: needinfo?(mconley)
Attachment #8884990 - Flags: feedback?(mconley) → feedback+
Assignee: nobody → bwinton
Duplicate of this bug: 1385075
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.
Attachment #8889918 - Flags: review?(mconley) → review+
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!
Pushed by bwinton@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ef9bf18b361
Focus the urlbar a little later. r=mconley.
https://hg.mozilla.org/mozilla-central/rev/2ef9bf18b361
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.