Closed Bug 933532 Opened 11 years ago Closed 6 years ago

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

Categories

(Firefox :: Tabbed Browser, enhancement)

Product:
Firefox
Component:
Tabbed Browser
Version:
50 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1344749

People

(Reporter: leehw, Unassigned)

References

Details

Attachments

(1 obsolete file) 

      No description provided.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 25 Branch → 50 Branch
Due to the fix of bug 528005, I believe that this suggested behavior is better than current behavior for consistency.
See Also: → 528005
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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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/
Attachment #8936184 - Flags: review?(dao+bmo) → review-
> 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?
Flags: needinfo?(dao+bmo)
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.
See Also: → 1344749
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".
Attachment #8936184 - Flags: review?(dao+bmo) → review-
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...
Attachment #8936184 - Flags: review?(dao+bmo) → review-
Flags: needinfo?(dao+bmo)
dao: ping
Flags: needinfo?(dao+bmo)
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?
Flags: needinfo?(dao+bmo)
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
Attachment #8936184 - Flags: review?(dao+bmo)
(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?
Flags: needinfo?(bob.silverberg)
(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?
Flags: needinfo?(bob.silverberg) → needinfo?(mconca)
Good idea, Bob. I'll sync up with you privately to coordinate something.
Flags: needinfo?(mconca)
(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.
Assignee: masayuki → nobody
Status: ASSIGNED → NEW
This should be covered entirely by bug 1344749.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
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.
Flags: needinfo?(masayuki)
Flags: needinfo?(dao+bmo)
Should be discussed in bug 1485683.
Flags: needinfo?(masayuki)
Flags: needinfo?(dao+bmo)
Assignee: nobody → nobody
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: