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

VERIFIED FIXED in Firefox 59

Status

defect
P3
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: magicp.jp, Assigned: bsilverberg)

Tracking

(Depends on 1 bug)

unspecified
mozilla59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 verified)

Details

(Whiteboard: [tabs])

Attachments

(3 attachments)

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.
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
Assignee: nobody → bob.silverberg
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P3
Whiteboard: [tabs]
(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...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dao+bmo)
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.
(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.
Flags: needinfo?(dao+bmo)
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 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 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?
Flags: needinfo?(mixedpuppy)
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.
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1422747
I filed bug 1422747 on tabbrowser / XBL.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Attachment #8933630 - Flags: review?(mixedpuppy) → review+
Flags: needinfo?(mixedpuppy)
Pushed by bsilverberg@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/4a160faf5ac5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Posted image 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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.