Closed Bug 1460221 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 + verified

People

(Reporter: mak, Assigned: mixedpuppy)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
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
Blocks: 1449700
Flags: needinfo?(mixedpuppy)
Version: 55 Branch → Trunk
Priority: -- → P1
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).
Flags: needinfo?(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.
Flags: needinfo?(mak77)
This reproduces for me also, and with a clean profile (tested with mozregression).
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)
I'm on Win10. I was reproducing with the default Nightly start page with mozregression. I also see it on gmail and slack.
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.
Flags: needinfo?(mak77)
I forgot, my prefs are default, insertAfterCurrent is false, insertRelatedAfterCurrent is true.
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.
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.
Assignee: nobody → mixedpuppy
Possible dupe using control-click - 
Bug 1460328 - Control-click opens new tabs in the wrong location within the tab bar tab list
Duplicate of this bug: 1460339
Duplicate of this bug: 1460328
Duplicate of this bug: 1460438
Duplicate of this bug: 1460465
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]
Duplicate of this bug: 1460449
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 :)
Bug 1460488 might be the same bug, adding as see also, someone will convert to dupe if necessary.
See Also: → 1460488
(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.
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 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`.
Attachment #8974576 - Flags: review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/444314e5f95f
fix newtab placement when opened from pinned tab, r=Gijs
Depends on: 1460728
https://hg.mozilla.org/mozilla-central/rev/444314e5f95f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
(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
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
(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.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy)
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
(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
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.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1460630
Duplicate of this bug: 1461558
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.