1422211 - browser.tabs.move() affects tabs-newtab-button position

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[magicp]

Attachment: tabs-move-behavior.png

Steps to reproduce:
1. Load "tabs-tabs-tabs" from WebExtensions Examples (https://github.com/mdn/webextensions-examples/tree/master/tabs-tabs-tabs)
2. Select "Move active tab to the end of the window" in extension's popup.

Actual Results:
In case of one tab, tabs-newtab-button moves to the begin of the window.
In case of two or more tabs, tabs-newtab-button does not move. (It's expected result)

Expected Results:
Is this intentional? If not, browser.tabs.move() does not affect tabs-newtab-button.

Comment 1

[Bob Silverberg [:bsilverberg]]

I have reproduced this on Nightly on OS X. Looking at the code, I suspect there is a bug in tabbrowser which is causing this. We can fix the reported problem in our API code, but I fear that we'd leave an underlying bug.

In ext-tabs.js, we call gBrowser.moveTabTo [1] to move the tab, passing a value for aIndex. When there is only one tab in the window, and tabs.move is passed a value of -1 for index, we pass a value of 1 for aIndex to gBrowser.moveTabTo, which causes this behaviour. We can fix our code to not call gBrowser.moveTabTo at all if there is only 1 tab in the window (and the tab isn't being moved between windows), which we should probably do as an optimization, but that doesn't address the fact that calling  gBrowser.moveTabTo should never end up with the newtab button appearing to the left of the tab.

I'll produce a patch to make the change I mentioned above, but it sounds like tabbrowser should also be fixed. What do you think, Gijs?

[1] https://searchfox.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#653

Comment 2

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1422211 - Do not try to move a tab when it is the only tab in a window,

The problem reported in this bug is that the newTab button moves to an invalid position when
there's only 1 tab in a window and you try to move it to the end of the tab strip via tabs.move.

This may be caused by a bug in tabbrowser, which is being investigated separately, but it
brought to light the fact that we're trying to move a tab when it's the only tab in a
window, and that just doesn't make sense.

This patch updates the code so we don't try to move a tab if it's the only tab in the
window.

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

Comment 3

[:Gijs (he/him)]

(In reply to Bob Silverberg [:bsilverberg] from comment #1)
> I have reproduced this on Nightly on OS X. Looking at the code, I suspect
> there is a bug in tabbrowser which is causing this. We can fix the reported
> problem in our API code, but I fear that we'd leave an underlying bug.
> 
> In ext-tabs.js, we call gBrowser.moveTabTo [1] to move the tab, passing a
> value for aIndex. When there is only one tab in the window, and tabs.move is
> passed a value of -1 for index, we pass a value of 1 for aIndex to
> gBrowser.moveTabTo, which causes this behaviour. We can fix our code to not
> call gBrowser.moveTabTo at all if there is only 1 tab in the window (and the
> tab isn't being moved between windows), which we should probably do as an
> optimization, but that doesn't address the fact that calling 
> gBrowser.moveTabTo should never end up with the newtab button appearing to
> the left of the tab.
> 
> I'll produce a patch to make the change I mentioned above, but it sounds
> like tabbrowser should also be fixed. What do you think, Gijs?
> 
> [1]
> https://searchfox.org/mozilla-central/source/browser/components/extensions/
> ext-tabs.js#653


It looks like the moveTabTo code is just wrong when the index you pass is >= the number of tabs in the tabstrip. I actually don't understand why/how exactly. Specifically, as far as I can tell this should either work or break whenever it gets passed an index >= the number of tabs. However, in practice, I can only break things if there is only 1 tab in the window. It's not clear to me why that is. It might be a XBL bug? Which is a bit scary, tbh...

Dão, am I missing something stupid here? Blaming XBL doesn't feel very satisfactory...

Comment 4

[:Gijs (he/him)]

To be slightly more precise:

https://dxr.mozilla.org/mozilla-central/rev/a21f4e2ce5186e2dc9ee411b07e9348866b4ef30/browser/base/content/tabbrowser.xml#4044-4046

          // use .item() instead of [] because dragging to the end of the strip goes out of
          // bounds: .item() returns null (so it acts like appendChild), but [] throws
          this.tabContainer.insertBefore(aTab, this.tabs.item(aIndex));

looks to me like whenever we pass an Index >= the number of tabs, we run:

gBrowser.tabContainer.insertBefore(aTab, null)

which will append the tab. I don't see why that'd behave differently once there's more than 1 tab in the window.

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #4)
> looks to me like whenever we pass an Index >= the number of tabs, we run:
> 
> gBrowser.tabContainer.insertBefore(aTab, null)
> 
> which will append the tab. I don't see why that'd behave differently once
> there's more than 1 tab in the window.

Probably related to bug 472020.

Comment 6

[Mozilla Autoland]

hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: waiting for lock on working directory of /repo/hg/mozilla/try held by process '10896' on host 'hgssh4.dmz.scl3.mozilla.com/effffffc'
remote: abort: working directory of /repo/hg/mozilla/try: timed out waiting for lock held by 'hgssh4.dmz.scl3.mozilla.com/effffffc:10896'
abort: stream ended unexpectedly (got 0 bytes, expected 4)

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8933630 [details]
Bug 1422211 - Do not try to move a tab when it is the only tab in a window,

https://reviewboard.mozilla.org/r/204574/#review210238

This looks ok, can we get a test?

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8933630 [details]
Bug 1422211 - Do not try to move a tab when it is the only tab in a window,

https://reviewboard.mozilla.org/r/204574/#review210238

I'm not sure how to write a test for it. Even without this patch, when moving a single tab nothing actually changes about the tab, rather it's the newTab button that changes its position. I compared the layout of the xul for the TabsToolbar in both scenarios, and they are identical, so it doesn't look like we can inspect the toolbar to check for this fix. Plus, I'm not sure it would be good to have a test that reaches into the xul markup to verify anything as that could easily change. Do you have any ideas for how we could write a test to verify that the new tab button does not move to the left of the tab?

Comment 9

[Bob Silverberg [:bsilverberg]]

Gijs, we're going to "fix" this in our code, so calling browser.tabs.move will not result in this issue, but it does sound like there's still an underlying issue. The comments above don't really seem to address it, and bug 472020 is closed. I'll leave it in your hands to decide whether a separate bug for tabbrowser needs to be opened.

Comment 10

[:Gijs (he/him)]

I filed bug 1422747 on tabbrowser / XBL.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8933630 [details]
Bug 1422211 - Do not try to move a tab when it is the only tab in a window,

https://reviewboard.mozilla.org/r/204574/#review210744

Seems bug 1422747 will fully address the problem.  Given there are aparently tests [currently disabled] for this, I'm fine with this.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a160faf5ac5
Do not try to move a tab when it is the only tab in a window, r=mixedpuppy

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/4a160faf5ac5

Comment 14

[CosminB]

Attachment: Bug1422211OneTab.gif

I can reproduce this issue on Firefox 57.0.1 (20171128222554) under Win 7 64-bit and Firefox 58.0b9 (20171204150510) under Mac OS X 10.13.1

This issue is verified as fixed on Firefox 59.0a1 (20171206100053) under Win 7 64-bit and Mac OS X 10.13.1.

Please see the attached video.