1460221 - Clicking a link in a pinned tab opens a normal tab in the middle of pinned tabs

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Marco Bonardo [:mak]]

This looks like a very recent regression, likely latest nightly (didn't have time to run mozregression yet).

1. open 2 pinned tabs
2. click a link in the first pinned tab

ER:
a new normal tab should open at the end of the pinned tabs
AR
a new normal tab opens in the middle of pinned tabs

Comment 1

[Mark Banner (:standard8)]

Mozregression:

Last good revision: 8a4ebd5b130d4b2b7ae2f03d54f9163335e43da5
 5:46.61 INFO: First bad revision: 030d771ec8a3a0a02ef4222e22504a66a0acdf86
 5:46.61 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8a4ebd5b130d4b2b7ae2f03d54f9163335e43da5&tochange=030d771ec8a3a0a02ef4222e22504a66a0acdf86

-> Bug 1449700

Comment 2

[Shane Caraveo (:mixedpuppy)]

This is working as expected for me on a clean profile (as well as my regular profile after updating), artifact build of nightly on osx.

If either browser.tabs.insertAfterCurrent or insertRelatedAfterCurrent are true, the tab appears as normal at the end of the pinned tabs (regardless which pinned tab I use).

If both are false, the new tab appears at the end of the tab strip.

So I'd need more information to reproduce (seems standard8 was able to repro to find the regression).

Comment 3

[Shane Caraveo (:mixedpuppy)]

Note: I also set the pref (on the new profile) to restore session on restart.  restarted, still cannot reproduce.  Added additional pinned tabs, still no repro.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

This reproduces for me also, and with a clean profile (tested with mozregression).

Comment 5

[Shane Caraveo (:mixedpuppy)]

Is there a specific site being used to repro?  (I used wikipedia, the very front page where you choose language)
What platform is everyone using? (not that I think this should make a difference)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

I'm on Win10. I was reproducing with the default Nightly start page with mozregression. I also see it on gmail and slack.

Comment 7

[Marco Bonardo [:mak]]

I can reproduce in a clean profile, if the link normally opens in the same pinned tab, just right click and open in a new tab.
The bugzilla dashboard always opens in new tabs and reproduces this easily.

Comment 8

[Marco Bonardo [:mak]]

I forgot, my prefs are default, insertAfterCurrent is false, insertRelatedAfterCurrent is true.

Comment 9

[Mark Banner (:standard8)]

On my own profile, I can reproduce it with middle-clicking bugzilla links, or left-clicking links on SSO dashboard.

On a mozregression newly created test profile, I think I used the nightly start page, created a second tab and loaded youtube, pinned both, created a third, blank new tab. Then just middle-clicked links from the start page.

Comment 10

[Shane Caraveo (:mixedpuppy)]

heh.  rebuilt my nightly and got it.  but my running nightly browser is updated and doesn't repro, looks to be built after this was reported.  At least I can look into it now.

Comment 11

[Jim Mathies [:jimm]]

Possible dupe using control-click - 
Bug 1460328 - Control-click opens new tabs in the wrong location within the tab bar tab list

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1460221 - fix newtab placement when opened from pinned tab,

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

Comment 17

[Matthew Turnbull [Bluefang]]

Looking at the commit for Bug 1449700, It seems like some of the trouble is around the refactor of the related placement code

Old behavior:
https://hg.mozilla.org/mozilla-central/rev/030d771ec8a3#l1.275
-      if (lastRelatedTab)
-        lastRelatedTab.owner = null;
-      else if (openerTab)
-        t.owner = openerTab;
-      this.moveTabTo(t, newTabPos, true);
-      if (openerTab)
-        this._lastRelatedTabMap.set(openerTab, t);

New behavior:
https://hg.mozilla.org/mozilla-central/rev/030d771ec8a3#l1.221
+          if (lastRelatedTab) {
+            lastRelatedTab.owner = null;
+          } else if (openerTab) {
+            t.owner = openerTab;
+            this._lastRelatedTabMap.set(openerTab, t);
+          }

Previously, _lastRelatedTabMap was always updated if openerTab existed. The new behavior only makes the update if lastRelatedTab ALSO does not exist.

Should be:
+          if (lastRelatedTab) {
+            lastRelatedTab.owner = null;
+          } else if (openerTab) {
+            t.owner = openerTab;
+          }
+          if (openerTab) {
+            this._lastRelatedTabMap.set(openerTab, t);
+          }

I think this explains why opening tabs in the background only keeps track of the first related tab, and you get tabs misordered like:

[source] [original_next]
[source] [new_1] [original_next]
[source] [new_1] [new_2] [original_next]
[source] [new_1] [new_3] [new_2] [original_next]
[source] [new_1] [new_4] [new_3] [new_2] [original_next]

Comment 19

[Matthew Turnbull [Bluefang]]

Also, it looks like moveTabTo is what was previously enforcing the separation of pinned and unpinned tabs. To keep similar behavior, the check should probably stay outside of the related placement code.

https://hg.mozilla.org/mozilla-central/rev/030d771ec8a3#l1.241

Change:
+      if (aPinned) {
+        aIndex = Math.min(aIndex, this._numPinnedTabs);
+      }

to:
+      if (aPinned) {
+        aIndex = Math.min(aIndex, this._numPinnedTabs);
+      } else {
+        aIndex = Math.max(aIndex, this._numPinnedTabs);
+      }

Though in moveTabTo, the aPinned case used this._numPinnedTabs - 1, so there's some math somewhere that isn't making sense to me :)

Comment 20

[Clément Lefèvre]

Bug 1460488 might be the same bug, adding as see also, someone will convert to dupe if necessary.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8974576 [details]
Bug 1460221 - fix newtab placement when opened from pinned tab,

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

Comment 22

[Shane Caraveo (:mixedpuppy)]

(In reply to Matthew Turnbull [Bluefang] from comment #17)

> https://hg.mozilla.org/mozilla-central/rev/030d771ec8a3#l1.221
> +          if (lastRelatedTab) {
> +            lastRelatedTab.owner = null;
> +          } else if (openerTab) {
> +            t.owner = openerTab;
> +            this._lastRelatedTabMap.set(openerTab, t);
> +          }
> 
> Previously, _lastRelatedTabMap was always updated if openerTab existed. The
> new behavior only makes the update if lastRelatedTab ALSO does not exist.

ah...good catch.

> I think this explains why opening tabs in the background only keeps track of
> the first related tab, and you get tabs misordered like:

It only ever tracked one, but indeed it wasn't being updated correctly after the first related tab.

(In reply to Matthew Turnbull [Bluefang] from comment #19)

> Though in moveTabTo, the aPinned case used this._numPinnedTabs - 1, so
> there's some math somewhere that isn't making sense to me :)

moveTabTo does indeed have slightly different positioning logic, because it is moving the tab.

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I use pinned tabs, and I noticed recently my regular tabs would sometimes start jumping back and forth ad nauseam. Not sure exactly how to repro, but hopefully this is caused by the same thing and fixed by these patches.

Comment 24

[:Gijs (he/him)]

Comment on attachment 8974576 [details]
Bug 1460221 - fix newtab placement when opened from pinned tab,

https://reviewboard.mozilla.org/r/242920/#review248910

::: browser/base/content/test/tabs/browser_pinnedTabs_clickOpen.js:1
(Diff revision 2)
> +"use strict";

Nit: include CC0 public domain license.

::: browser/base/content/test/tabs/browser_pinnedTabs_clickOpen.js:17
(Diff revision 2)
> +  BrowserTestUtils.synthesizeMouseAtCenter("#link", {}, gBrowser.selectedBrowser);
> +  let newtab1 = await BrowserTestUtils.waitForNewTab(gBrowser, "http://mochi.test:8888/");

Await before click - otherwise I think this might fail in non-e10s.

::: browser/base/content/test/tabs/browser_pinnedTabs_clickOpen.js:33
(Diff revision 2)
> +
> +  // Second test new background tabs open in order.
> +
> +  await BrowserTestUtils.switchTab(gBrowser, tabs[1]);
> +  // Set both ctrlKey and metaKey which is ok for this test to pass different platforms.
> +  BrowserTestUtils.synthesizeMouseAtCenter("#link", {ctrlKey: true, metaKey: true}, gBrowser.selectedBrowser);

This feels like it might break if we change how we handle shortcuts... could just do:

```js
let modifiers = AppConstants.platform == "macosx" ? {metaKey: true} : {ctrlKey: true};
```

and then pass `modifiers`.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8974576 [details]
Bug 1460221 - fix newtab placement when opened from pinned tab,

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/444314e5f95f
fix newtab placement when opened from pinned tab, r=Gijs

Comment 28

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/444314e5f95f

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs (he/him) from comment #24)
> ::: browser/base/content/test/tabs/browser_pinnedTabs_clickOpen.js:1
> (Diff revision 2)
> > +"use strict";
> 
> Nit: include CC0 public domain license.

Am I misunderstanding something here about the need to put a license header at the top of test files? https://www.mozilla.org/en-US/MPL/license-policy/ states that:

"""
Trivial bits of Mozilla Code, such as testcases or snippets of code used in documentation, should be put in the public domain in order to facilitate re-use. To do that, apply the Creative Commons Public Domain Dedication by using the following boilerplate:

    Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/

Note that, for historical reasons, some trivial support code, such as old code samples in developer.mozilla.org, may be under the MIT license.

PD Test Code is Test Code (1) that is Mozilla Code, (2) that does not carry an explicit license header, and (3) that was either committed to a Mozilla Repository on or after 23rd September 2014, or was committed before that date but for which all contributors up to that date were Mozilla employees, contractors or interns. PD Test Code is is placed in the public domain pursuant to the Creative Commons Public Domain Dedication. Test Code which has not been demonstrated to be PD Test Code should be considered to be under the MPL 2.
"""

The test above qualifies for 1, 2, and 3 so as such should not require a license header and would be Public Domain by default. I should also note that the test landed with the MPL 2 header [1], not a PD header.

[1] https://hg.mozilla.org/mozilla-central/diff/444314e5f95f/browser/base/content/test/tabs/browser_pinnedTabs_clickOpen.js#l1.5

Comment 30

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #29)
> (In reply to :Gijs (he/him) from comment #24)
> > ::: browser/base/content/test/tabs/browser_pinnedTabs_clickOpen.js:1
> > (Diff revision 2)
> > > +"use strict";
> > 
> > Nit: include CC0 public domain license.
> 
> Am I misunderstanding something here about the need to put a license header
> at the top of test files? https://www.mozilla.org/en-US/MPL/license-policy/
> states that:

See discussion at bug 1316187 comment 34 and later, with Gerv who (AIUI) wrote those bits of the license policy.

TL;DR: even if our policy states that by default, test files in m-c committed after 2014 are public domain, it's still better to include the license header than not to do so.

Comment 31

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #29)
> I should also note
> that the test landed with the MPL 2 header [1], not a PD header.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/diff/444314e5f95f/browser/base/
> content/test/tabs/browser_pinnedTabs_clickOpen.js#l1.5

This seems unfortunate, and something that could be fixed by a DONTBUILD commit landed on inbound.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs (he/him) from comment #31)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #29)
> > I should also note
> > that the test landed with the MPL 2 header [1], not a PD header.
> > 
> > [1]
> > https://hg.mozilla.org/mozilla-central/diff/444314e5f95f/browser/base/
> > content/test/tabs/browser_pinnedTabs_clickOpen.js#l1.5
> 
> This seems unfortunate, and something that could be fixed by a DONTBUILD
> commit landed on inbound.

Shane, I think this is something you should do since you were the original author of the file and placed the license on it.

Comment 33

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e906e3ef662df3fa8be3216d98af46d3e4270c4e
Bug 1460221 followup fix license header in test file DONTBUILD r=me

Comment 34

[Joel Maher ( :jmaher ) (UTC -8)]

this seems to fail in test-verify:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=59a49b12b26846302393edfbd20b5e72ef6b1d85&filter-searchStr=tv%20linux%20opt

I am not sure if this test failed before the changes, I just wanted to point it out as this might be a flaky test

Comment 35

[Shane Caraveo (:mixedpuppy)]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #34)
> this seems to fail in test-verify:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=59a49b12b26846302393edfbd20b5e72ef6b1d85&filter-
> searchStr=tv%20linux%20opt
> 
> I am not sure if this test failed before the changes, I just wanted to point
> it out as this might be a flaky test

bug 1460728

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e906e3ef662d

Comment 37

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 62.0a1 (2018-05-11), so I'm marking this bug as VERIFIED. Thanks.