933532 - Open external links and bookmarks to the right-hand side of the current tab, rather than at the end of the tab bar.

Status: RESOLVED DUPLICATE of bug 1344749

(Firefox :: Tabbed Browser, enhancement)

Comment 1

[Albert Scheiner [:alberts]]

originally reported for version 25, bumped it up to 50 now as I have tested with it - please revert if that's not the way it's done.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Due to the fix of bug 528005, I believe that this suggested behavior is better than current behavior for consistency.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef69effc647585ea1716af3d4d414557f6dd6452

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I really hate this bug especially now, WebExtension doesn't addons allow to override this behavior. So, I really want to implement this as an optional feature.

Note that I don't want to discuss whether this behavior should be default behavior or not since I'm really busy on Quantum Flow of editor module and I want to use new behavior as soon as possible.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b3f61e51d504d0fcb76ed2af397eaf71f75996f

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 933532 - Add a preference to open new tab which is not related to any existing tabs immediately to right of current tab

Currently, we have a pref, "browser.tabs.insertRelatedAfterCurrent", to open
new tab for a link in current tab immediately to the right.  This is also used
for navigation buttons too.

However, opening new tab from bookmarks, history itmes and Alt+Enter in URL bar
and search bar is always appended to rightmost of the tab bar.  So, there should
be a new pref, "browser.tabs.insertUnreleatedAfterCurrent", to open new tab
form them immediately to the right of current tab.

Note that this patch just adds a pref for users who want the new behavior
for now.  Whether it should be default behavior or not should be discussed
later.

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

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8936184 [details]
Bug 933532 - Add a preference to open new tab which is not related to any existing tabs immediately to right of current tab

https://reviewboard.mozilla.org/r/206880/#review212934

::: browser/base/content/tabbrowser.xml:2871
(Diff revision 1)
>                  Cu.reportError(ex);
>                }
>              }
>  
>              // If we're opening a tab related to the an existing tab, move it
>              // to a position after that tab.

nit: update comment

::: browser/base/content/tabbrowser.xml:2875
(Diff revision 1)
>              // If we're opening a tab related to the an existing tab, move it
>              // to a position after that tab.
> -            if (openerTab &&
> -                Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) {
> -
> -              let lastRelatedTab = this._lastRelatedTabMap.get(openerTab);
> +            if ((openerTab &&
> +                 Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) ||
> +                (!openerTab &&
> +                 Services.prefs.getBoolPref("browser.tabs.insertUnreleatedAfterCurrent"))) {

this can be simplified:

openerTab ?
Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent") :
Services.prefs.getBoolPref("browser.tabs.insertUnrelatedAfterCurrent")

::: browser/base/content/test/newtab/browser_newtab_insert_position.js:4
(Diff revision 1)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> -add_task(async function() {
> +async function doTest(aInsertRelatedAfterCurrent, aInsertUnrelatedAfterCurrent) {

Please put this test in browser/base/content/test/tabs/

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Please put this test in browser/base/content/test/tabs/

The test really depend on browser/base/content/test/newtab/head.js for using new tab's grid to open its related tab. So, if I move it to browser/base/content/test/tabs/, I also need to copy head.js too. Do you think that it should be in /tabs/ even with copying the head.js?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8936184 [details]
Bug 933532 - Add a preference to open new tab which is not related to any existing tabs immediately to right of current tab

https://reviewboard.mozilla.org/r/206880/#review212934

> this can be simplified:
> 
> openerTab ?
> Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent") :
> Services.prefs.getBoolPref("browser.tabs.insertUnrelatedAfterCurrent")

Indeed, it's really nice. I should use it in C++ code too.

> Please put this test in browser/base/content/test/tabs/

As I said in the previous comment, moving it to the directly causes duplicating head.js but I assume that you don't like such duplication. So, I'll request review next patch without this change.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8936184 [details]
Bug 933532 - Add a preference to open new tab which is not related to any existing tabs immediately to right of current tab

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

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8936184 [details]
Bug 933532 - Add a preference to open new tab which is not related to any existing tabs immediately to right of current tab

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

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8936184 [details]
Bug 933532 - Add a preference to open new tab which is not related to any existing tabs immediately to right of current tab

Ah, I misunderstood what "newtab" means the original test.

Okay, I'll create new automated tests in the "tabs".

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6787df50e89028e3dab4e12a88b708e80c10319

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8936184 [details]
Bug 933532 - Add a preference to open new tab which is not related to any existing tabs immediately to right of current tab

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

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1611060ad4d8b17bbc85ecba070cdfee22c42f4b

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8936184 [details]
Bug 933532 - Add a preference to open new tab which is not related to any existing tabs immediately to right of current tab

Hmm, really oddly, the test causes permanent orange due to failing to load new page in the new tab. Although, it won't fail on local environment when I try to run it alone...

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fe3d00cceb8cc6be704d9e3a05b91045eebe9c0

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64e7a1fff93858a0ba0cacd56626a5bad1293960

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e4f26772d2e41c82bb077a5acb8248698e81d9

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae9ae7f786763df020bb0942d7162b35591ec5c3

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8936184 [details]
Bug 933532 - Add a preference to open new tab which is not related to any existing tabs immediately to right of current tab

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206880/diff/4-5/

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

dao: ping

Comment 23

[Dão Gottwald [:dao]]

I'm not sure this is the right approach vs. bug 1344749 (as noted in bug 1344749 comment 44). Maybe post to the firefox-dev mailing list to get broader input on this?

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 8936184 [details]
Bug 933532 - Add a preference to open new tab which is not related to any existing tabs immediately to right of current tab

https://reviewboard.mozilla.org/r/206880/#review217462

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Dão Gottwald [::dao] from comment #23)
> I'm not sure this is the right approach vs. bug 1344749 (as noted in bug
> 1344749 comment 44). Maybe post to the firefox-dev mailing list to get
> broader input on this?

Sure. My patch does NOT expose the new feature as WebExt API. And I agree with bug 1344749 comment 46. So, if bug 1344749 will be fixed soon, I don't mind to wait it.

On the other hand, :bsilverberg said that he put the bug into his queue. If the priority is not so high and needs a couple of release cycles, I'd really like you to allow my patch to land.


bsilverberg: When are you going to fix bug 1344749 in your plan?

Comment 26

[Bob Silverberg [:bsilverberg]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #25)
> 
> bsilverberg: When are you going to fix bug 1344749 in your plan?

I do hope to get to this this quarter, i.e., in time for 61, but looking at the comments in bug 1344749, and here, it looks like we need to have some discussion and figure out exactly what direction we want to take with all of these items involving new tab position. I think that needs to be done before we can consider landing this as is.

Mike, do you think we can schedule some time to discuss that with the relevant stakeholders?

Comment 27

[Mike Conca [:mconca]]

Good idea, Bob. I'll sync up with you privately to coordinate something.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Bob Silverberg [:bsilverberg] from comment #26)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #25)
> > 
> > bsilverberg: When are you going to fix bug 1344749 in your plan?
> 
> I do hope to get to this this quarter, i.e., in time for 61,

Thanks, it's enough.

> but looking at the comments in bug 1344749, and here, it looks like we need to have some
> discussion and figure out exactly what direction we want to take with all of
> these items involving new tab position. I think that needs to be done before
> we can consider landing this as is.

If discussion would become longer, I'd be happy if you'd implement only hidden pref (like my patch), then, it'd be exposed as WebExtension API later.

Comment 29

[Bob Silverberg [:bsilverberg]]

This should be covered entirely by bug 1344749.

Comment 30

[larabe]

The Implementation of this code causes this issue https://bugzilla.mozilla.org/show_bug.cgi?id=1485683 where tabs are not getting opened next to the tab that opens it but next to the most recent opened tab from that tab.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Should be discussed in bug 1485683.