Closed Bug 1344749 Opened 7 years ago Closed 6 years ago

Expose API to customize where new tabs open

Categories

(WebExtensions :: General, enhancement, P2)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox61 verified, firefox62 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- verified
firefox62 --- verified

People

(Reporter: u462496, Assigned: mixedpuppy)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved][browserSettings])

Attachments

(4 files, 3 obsolete files)

Bug 1344749 - Expose API to customize where new tabs open
59 bytes, text/x-review-board-request
mixedpuppy
: review+
dao
: review+
Details
Bug 1344749 - Expose API to customize where new tabs open
59 bytes, text/x-review-board-request
dao
: review+
Details
1344749-followup
2.28 KB, patch
dao
: review+
Details | Diff | Splinter Review
tabs opening in the right order.gif
279.61 KB, image/gif
Details 
Currently in my XUL addon All Tabs Helper I offer the user the ability to opt for where a new tab opens when clicking the "New Tab" button or using keyboard shortcut: next to the current tab, or at the end of the tabs bar (default).

I am requesting an API which will allow me to continue this feature.
Whiteboard: [design-decision-needed]
It just occurred to me that it may be better/simpler to make this a feature of Firefox, as the API for that action would have to be in place anyway.
You can use the tabs API to listen for new tab events and move the tab appropriately. So you are looking for a way to alter the UI to show the choice?
(In reply to Andy McKay [:andym] from comment #2)
> You can use the tabs API to listen for new tab events and move the tab
> appropriately.

Yes indeed.  I had been doing it by relatedToCurrent:true, but I can seen in addTab it would be doing the same thing, appending the new tab at the end, then moving it.

> So you are looking for a way to alter the UI to show the
> choice?

No, that isn't necessary.

Thanks.
Cool, thanks.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Andy, I have finally gotten around to implementing your suggestion, and while it is true, a new tab can be opened and then moved next to the originally current tab, there is a UI/UX problem as outlined below.

Open a new tab next to current by using the suggested method of first opening, then moving the tab, when the original current tab is visible on the tabstrip:

The expected behavior from a user POV would be just as if link is opened in a new tab where the new tab would appear next to the orginal current tab without any scrolling, with the exception of, if the original tab were the last tab on the strip, the new tab would scroll into view, thus scrolling one tab over.

However, if the scrolling position of the tabstrip is such that the last tab is off the end of the tabstrip, the tabstrip will - unexpectedly to the user - scroll to another position, and if there are enough tabs between the original current tab and the end of the tabstrip, the original current tab will disappear off the end of the tabstrip.

From an API POV this is expected behavior, for when using the method you suggested, the new tab is first scrolled into view at the end of the tabstrip.  Then, when the tab gets moved next to the original current tab, if the new position puts the new tab out of view, the tabstrip is again scrolled into view, placing it at the far left of the tab strip, with the original current tab out of view.

Adding a tab next to the current tab - with the expected behavior from a user POV - can be accomplished in tabbrowser code simply by setting relatedToCurrent to true when calling addTab().  No subsequent moving is necessary (this is handled internally without intermediate UI corrections).

AFAIK, there is no way to control scrolling of the tabstrip nor even get the tabstrip scrolling position from a WebExtension.  Unless you can suggest some other method of implementing this with a WebExtension which would accomplish the expected behavior from a user POV (that is not too complex) I would like to reopen this bug.
Flags: needinfo?(amckay)
This sounds like a bug 1387372, would that bug resolve the problem?
Flags: needinfo?(amckay)
(In reply to Andy McKay [:andym] from comment #6)
> This sounds like a bug 1387372, would that bug resolve the problem?

Maybe... I'm not really clear on the goal of that bug, IMO it seems to be going off on a tangent from the core issue the OP seems to be referring to (needing to open a tab next to current).  I'll see what the response is to my comment/suggestion and maybe I'll have a better idea of where it's going.
Thanks for the detailed explanation, let's reopen this.
Status: RESOLVED → REOPENED
Priority: -- → P5
Resolution: WONTFIX → ---
(In reply to Andy McKay [:andym] from comment #8)
> Thanks for the detailed explanation, let's reopen this.

Thanks a lot, Andy.  Regarding bug 1387372, it was actually I who was off on a tangent, but IAE, that bug has a different goal than what I am talking about here.

I was thinking of maybe doing something a little different here than a WebExtension API for this.  What about taking this to a native level, a global browser preference that determines where a tab opens, like browser.tabs.insertRelatedAfterCurrent does with tabs opened in the background when a link is clicked.  It could be something like browser.tabs.insertNewTabAfterCurrent.  I think this would be as simple as checking the pref in the openLinkIn => tabbrowser.loadOneTab chain and setting relatedToCurrent accordingly.

Then as far as an extension goes, is it possible for an extension to set a global pref, given proper permissions of course?
Hi Kevin, this has been added to the agenda for the October 31 WebExtensions APIs triage meeting. Would you be able to join us? 

Here's a few things about the triage to keep in mind: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/Add-ons/Contribute/Triage
* Meeting agenda: https://docs.google.com/document/d/1qqE6fkqr-RNWaFvMpv0Z8O5FLDgQ3AT5eGdbTt7lRGg/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision
Attached patch proof_of_concept_V1.diff (obsolete) — — Splinter Review 
Here is a proof-of-concept.

I designed this around using a pref, as that allows advanced users to take advantage of the feature without needing an addon to implement it for them if they wish.

The tests have some delays built in for now to give visual rendering of the feature.
Assignee: nobody → kevinhowjones
Attachment #8920839 - Flags: feedback?(mixedpuppy)
Attachment #8920839 - Flags: feedback?(amckay)
Comment on attachment 8920839 [details] [diff] [review]
proof_of_concept_V1.diff

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -2257,17 +2257,19 @@ function BrowserOpenTab(event) {

>+  let relatedToCurrent =
>+    gPrefService.getPrefType("browser.tabs.newTabNextToCurrent") == gPrefService.PREF_BOOL &&
>+    gPrefService.getBoolPref("browser.tabs.newTabNextToCurrent");

Use Services.prefs.

>diff --git a/browser/components/extensions/ext-tabs.js b/browser/components/extensions/ext-tabs.js
>--- a/browser/components/extensions/ext-tabs.js
>+++ b/browser/components/extensions/ext-tabs.js
>@@ -936,13 +936,17 @@ this.tabs = class extends ExtensionAPI {

>+        setNewTabNextToCurrent(state) {
>+          Services.prefs.setBoolPref("browser.tabs.newTabNextToCurrent", state);
>+        },

Setting prefs should use the pref management, bsilverberg would be good to ask about that.
Attachment #8920839 - Flags: feedback?(bob.silverberg)
Comment on attachment 8920839 [details] [diff] [review]
proof_of_concept_V1.diff


>diff --git a/browser/components/extensions/schemas/tabs.json b/browser/components/extensions/schemas/tabs.json
>--- a/browser/components/extensions/schemas/tabs.json
>+++ b/browser/components/extensions/schemas/tabs.json
>@@ -916,16 +916,29 @@

>       {
>+        "name": "setNewTabNextToCurrent",
>+        "type": "function",
>+        "description": "Sets/unsets the browser.tabs.newTabNextToCurrent preference.",

Seems I accidentally deleted a comment.  

I'm thinking this belongs in browser settings.  Again Bob for help.
Attachment #8920839 - Flags: feedback?(mixedpuppy) → feedback+
Comment on attachment 8920839 [details] [diff] [review]
proof_of_concept_V1.diff

Review of attachment 8920839 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-tabs.js
@@ +941,5 @@
>  
>            tab.linkedBrowser.messageManager.sendAsyncMessage("Reader:ToggleReaderMode");
>          },
> +
> +        setNewTabNextToCurrent(state) {

I agree with Shane, this should be added to the browserSettings API, not the tabs API. This will allow for multiple extensions to set this and browserSettings will deal with who gets control.

Please let me know if you have any questions when working with the browserSettings API.
Attachment #8920839 - Flags: feedback?(bob.silverberg) → feedback-
Although, I wonder whether this needs to be an API at all. Why not just add this to about:preferences and allow a user to control it that way?
Flags: needinfo?(kevinhowjones)
(In reply to Bob Silverberg [:bsilverberg] from comment #15)
> Although, I wonder whether this needs to be an API at all. Why not just add
> this to about:preferences and allow a user to control it that way?

Well I think that would be great, personally.  It always seemed to me like it would be natural to have that option.  If fact, I have always felt it would be more natural to be the default, but that is just viewing from my workflow.

+1
Flags: needinfo?(kevinhowjones)
Attachment #8920839 - Flags: feedback?(amckay)
(In reply to Kevin Jones from comment #16)
> (In reply to Bob Silverberg [:bsilverberg] from comment #15)
> > Although, I wonder whether this needs to be an API at all. Why not just add
> > this to about:preferences and allow a user to control it that way?
> 
> Well I think that would be great, personally.  It always seemed to me like
> it would be natural to have that option.  If fact, I have always felt it
> would be more natural to be the default, but that is just viewing from my
> workflow.
> 
> +1

I'm holding off on working on any more patches until there is a decision on this.
In examining a number of existing tab-related add-ons on AMO, it appears the most common use cases are to open a tab:
 1) to the left
 2) to the right 
 3) at the end of the current tab strip

If supporting just those use cases makes implementation easier, it would capture the majority of what extension developers do today.
Flags: needinfo?(bob.silverberg)
We agreed to move forward with this as a browserSetting. Ideally this could be set to one of three values: to the left of the current tab, to the right of the current tab, at the end of the tab strip. I'm not sure if all of those are currently supported via preferences. If not we could probably implement just those that are currently supported and open follow-up bugs for the others.
Whiteboard: [design-decision-needed] → [design-decision-approved][browserSettings]
Flags: needinfo?(bob.silverberg)

        Attachment #8932056 -
        Flags: feedback?(bob.silverberg)

    
    

    
  
Comment on attachment 8932056 [details] [diff] [review]
bug_1344749_define_prefs_modify_tabbrowser_fb.diff

This patch defines the internals, the scheme and prefs needed for handling setting tab opening positions for both opening links, and new tabs created in the UI controls.

I am assuming a tab that doesn't have an `openerTab` is a new tab created in the UI.  Please advise if this is a valid assumption.

I am keeping the behavior for opening tabs from links and opening new tabs completely independent of each other as this makes more sense to me.  Each has their own integer pref which determines opening the tab before, after, or at the end of the tabs list.  The current bool `insertRelatedAfterCurrent` has been replaced with int `insertRelatedNextToCurrent`.  (Maybe these can be better named.)

In the case of opening new tabs, they are always opened immediately next to the current tab, rather than `lastRelatedTab`.  This makes the most sense to me.

I am wondering if in the case of opening tabs from links in the __before__ position, if these also should be opened relative to the current tab each time.  This would maintain the same left-to-right orientation (always adding the next tab at the end of the previous ones) as when opening them in the __after__ position.

Anyway, I would appreciate feedback on these thoughts and any others.

        Attachment #8932056 -
        Flags: feedback?(mixedpuppy)

    
    

    
  
I am very excited to see this feature finally become integrated into the browser. My only wish is that the current default behavior of insertRelatedAfterCurrent would not change for links opened in background. Currently, until I switch to another tab, all links opened in bg (with a middle click/ctrl+click etc.) would open in order of opening. (btw, I think "background tabs" is a better name than "related")

For example, if I have tabs `[a] b c` where `[a]` is currently active, and I open in bg tabs `1 2 3` in that order from that tab, I would get `[a] 1 2 3 b c`. That's how Firefox has been opening such links since v3.6, and it's very helpful for e.g. opening multiple pages of a forum thread and then reading them chronologically. Otherwise I'd have to conciosly open links in reverse order of how I would like to read them, which is quite inconvenient.

So as long as that functionality doesn't change I would be happy. Maybe it would be wise to add another state to that int pref to differentiate between whether tabs would open "to the right of the last related tab" (current default behavior) or "immediately to the right". I have tried three WebExtensions to open new blank tabs after the current one, and all of them break this default feature of placing related tabs after each other.

    
    

    
  
Comment on attachment 8932056 [details] [diff] [review]
bug_1344749_define_prefs_modify_tabbrowser_fb.diff

Review of attachment 8932056 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Kevin. I'm not really the one to review this, but I did add a comment.

::: browser/app/profile/firefox.js
@@ +458,5 @@
>  
>  // Tabbed browser
>  pref("browser.tabs.closeWindowWithLastTab", true);
> +pref("browser.tabs.insertRelatedNextToCurrent", 2);
> +pref("browser.tabs.newTabNextToCurrent", 0);

I'm a bit confused about exactly how these work and how they are related to each other. The names don't seem intuitively understandable. Could you add some comments to the file to explain what these are and how they work?

        Attachment #8932056 -
        Flags: feedback?(bob.silverberg)

    
    

    
  
(In reply to Bob Silverberg [:bsilverberg] from comment #23)
> Comment on attachment 8932056 [details] [diff] [review]
> bug_1344749_define_prefs_modify_tabbrowser_fb.diff
> 
> Review of attachment 8932056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Kevin. I'm not really the one to review this, but I did add a comment.
> 
> ::: browser/app/profile/firefox.js
> @@ +458,5 @@
> >  
> >  // Tabbed browser
> >  pref("browser.tabs.closeWindowWithLastTab", true);
> > +pref("browser.tabs.insertRelatedNextToCurrent", 2);
> > +pref("browser.tabs.newTabNextToCurrent", 0);
> 
> I'm a bit confused about exactly how these work and how they are related to
> each other. The names don't seem intuitively understandable. Could you add
> some comments to the file to explain what these are and how they work?

Okay, thanks.  Yes, documentation is good :-).  Renaming might be good also in this case, such as "newTabOpenPosition".

Basically they are (in both cases)

0 - open tab at end of tabs bar
1 - open tab to left of current
2 - open tab to right of current

    
  

        Attachment #8932056 -
        Flags: feedback?(dao+bmo)

    
    

    
  
(In reply to Kevin Jones from comment #24)
> (In reply to Bob Silverberg [:bsilverberg] from comment #23)
> > Comment on attachment 8932056 [details] [diff] [review]
> > bug_1344749_define_prefs_modify_tabbrowser_fb.diff
> > 
> > Review of attachment 8932056 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks Kevin. I'm not really the one to review this, but I did add a comment.
> > 
> > ::: browser/app/profile/firefox.js
> > @@ +458,5 @@
> > >  
> > >  // Tabbed browser
> > >  pref("browser.tabs.closeWindowWithLastTab", true);
> > > +pref("browser.tabs.insertRelatedNextToCurrent", 2);
> > > +pref("browser.tabs.newTabNextToCurrent", 0);
> > 
> > I'm a bit confused about exactly how these work and how they are related to
> > each other. The names don't seem intuitively understandable. Could you add
> > some comments to the file to explain what these are and how they work?
> 
> Okay, thanks.  Yes, documentation is good :-).  Renaming might be good also
> in this case, such as "newTabOpenPosition".
> 
> Basically they are (in both cases)
> 
> 0 - open tab at end of tabs bar
> 1 - open tab to left of current
> 2 - open tab to right of current

Dao:  If you don't mind please take a look at the tabbrowser stuff here.  Please see comment 24 for explanation of prefs.  I'll update the documentation and names in the next one.

    
    

    
  
(In reply to monk-time from comment #22)

> For example, if I have tabs `[a] b c` where `[a]` is currently active, and I
> open in bg tabs `1 2 3` in that order from that tab, I would get `[a] 1 2 3
> b c`. That's how Firefox has been opening such links since v3.6, and it's
> very helpful for e.g. opening multiple pages of a forum thread and then
> reading them chronologically. Otherwise I'd have to conciosly open links in
> reverse order of how I would like to read them, which is quite inconvenient.

I, on the contrary, got accustomed to them opening in the naïve order: `[a] 3 2 1 b c` (possibly because that was all we had before 3.6). Then I switch to 1, read it and close it. Closing a tab focuses the one to the left, so I get tab 2, which is also quite convenient.

Focusing the preceding tab on close is also consistent with the scenario:

* given a tab [a] b c,
* open a link in new tab → a [1] b c,
* read and close 1 → [a] b c.

I’m not a big fan of the idea of focusing this or that tab depending on invisible flags. I can never internalize them and that makes the browser behavior unpredictable and perceived as broken.

> So as long as that functionality doesn't change I would be happy. Maybe it
> would be wise to add another state to that int pref to differentiate between
> whether tabs would open "to the right of the last related tab" (current
> default behavior) or "immediately to the right".

Yes, I’d like a preference.

    
    

    
  
Comment on attachment 8932056 [details] [diff] [review]
bug_1344749_define_prefs_modify_tabbrowser_fb.diff

Also wondering if there is a preferred method for migrating prefs?

    
    

    
  
(In reply to Yuri Khan from comment #26)
> I, on the contrary, got accustomed to them opening in the naïve order: `[a]
> 3 2 1 b c` (possibly because that was all we had before 3.6). Then I switch
> to 1, read it and close it. Closing a tab focuses the one to the left, so I
> get tab 2, which is also quite convenient.

Correct me if I'm wrong, but AFAIK the current default is to focus on the right tab after closing, which plays very well with the default `insertRelatedAfterCurrent: true` (`[a] 1 2 3 b c`). But I agree that for comfortable browsing both orders require a corresponding setting to set which tab to focus on close, which I don't think Firefox currently has, and all webexts for this do so with a visible jerk when they override the focus.

Kevin, if you are going to replace that pref with `insertRelatedNextToCurrent: 2`, does this mean the default behavior in that use case is going to be reversed (`[a] 3 2 1 b c`)? This is very unexpected and would subtly but irritatingly break the habits of all users who don't already use add-ons for this.

    
    

    
  
(In reply to monk-time from comment #28)
> (In reply to Yuri Khan from comment #26)
> > I, on the contrary, got accustomed to them opening in the naïve order: `[a]
> > 3 2 1 b c` (possibly because that was all we had before 3.6). Then I switch
> > to 1, read it and close it. Closing a tab focuses the one to the left, so I
> > get tab 2, which is also quite convenient.
> 
> Correct me if I'm wrong, but AFAIK the current default is to focus on the
> right tab after closing, which plays very well with the default
> `insertRelatedAfterCurrent: true` (`[a] 1 2 3 b c`). But I agree that for
> comfortable browsing both orders require a corresponding setting to set
> which tab to focus on close, which I don't think Firefox currently has, and
> all webexts for this do so with a visible jerk when they override the focus.
> 
> Kevin, if you are going to replace that pref with
> `insertRelatedNextToCurrent: 2`, does this mean the default behavior in that
> use case is going to be reversed (`[a] 3 2 1 b c`)? This is very unexpected
> and would subtly but irritatingly break the habits of all users who don't
> already use add-ons for this.

The way the patch is currently written, the default behavior is preserved in all cases.

    
    

    
  
(In reply to monk-time from comment #28)

> Correct me if I'm wrong, but AFAIK the current default is to focus on the
> right tab after closing, which plays very well with the default
> `insertRelatedAfterCurrent: true` (`[a] 1 2 3 b c`).

Experiment time!

0. I start `firefox --no-remote --ProfileManager` and create a new profile.
1. It opens with two tabs. One, “Thank you for using Firefox Nightly”, conveniently contains prominent links, “Start testing”, “Start coding”, “Start localizing”, and quite a few more. The second tab is titled “Firefox by default shares da…” (privacy notice).
2. One the “Thank you” tab, I middle-click “Start testing”, “Start coding”, and “Start localizing”, in this order. Now I have: `[Thank you] Testing Coding Localizing Privacy`.
3. Assume I’m going to read the other tabs and want to get back to “Thank you”.
4. I click the Testing tab, read it, and close it.
5. Firefox switches to the next tab, Coding.
6. I read and close it.
7. Firefox switches to the next tab, Localizing.
8. I read and close it.
9. Firefox switches to the next tab, Privacy. I did not achieve my goal.

> But I agree that for
> comfortable browsing both orders require a corresponding setting to set
> which tab to focus on close, which I don't think Firefox currently has, and
> all webexts for this do so with a visible jerk when they override the focus.

Yes. Do we have a bug for that?

> Kevin, if you are going to replace that pref with
> `insertRelatedNextToCurrent: 2`, does this mean the default behavior in that
> use case is going to be reversed (`[a] 3 2 1 b c`)? This is very unexpected
> and would subtly but irritatingly break the habits of all users who don't
> already use add-ons for this.

I think this order should be a possibility, with a new value of this preference or with a different preference.

    
    

    
  
(In reply to Kevin Jones from comment #21)
> I am assuming a tab that doesn't have an `openerTab` is a new tab created in
> the UI.  Please advise if this is a valid assumption.

Probably not but I don't really know. This stuff is confusing, see bug 1238314 comment 40. Please ask Kris.

    
    

    
  
Comment on attachment 8932056 [details] [diff] [review]
bug_1344749_define_prefs_modify_tabbrowser_fb.diff

Kris, would you mind taking a look at this?

In addTab I am naively assuming that a condition where there is no openerTab we must be trying to open a new tab from the UI.  Can you advise on this, or suggest how we can deduce this is a new tab from the code in this method?  Otherwise I suppose we could just add a param that gets passed down from BrowserOpenTab in browser.js, which maybe would be safer.

        Attachment #8932056 -
        Flags: feedback?(dao+bmo) → feedback?(kmaglione+bmo)

    
    

    
  
> 0 - open tab at end of tabs bar
> 1 - open tab to left of current
> 2 - open tab to right of current

Lets keep the terminology as before and after rather than left and right.  If the UI is switched to RTL left will end up being right.  before/after will remain meaningful.

    
    

    
  
(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #18)
> In examining a number of existing tab-related add-ons on AMO, it appears the
> most common use cases are to open a tab:
>  1) to the left
>  2) to the right 
>  3) at the end of the current tab strip
> 
> If supporting just those use cases makes implementation easier, it would
> capture the majority of what extension developers do today.

Why would we have an insert before?  Is that really a use case people want?  Why would one do that to themselves?
Flags: needinfo?(mconca)

    
    

    
  
Comment on attachment 8932056 [details] [diff] [review]
bug_1344749_define_prefs_modify_tabbrowser_fb.diff

I'm not terribly happy with new prefs, but if we do have them, lets have insertRelatedTab and insertNewTab.  

Strange that we unset owner on lastRelatedTab, looks like we only allow a tab to own one tab?

I think you're fine relying on openerTab, but am not 100% certain.

        Attachment #8932056 -
        Flags: feedback?(mixedpuppy)

    
    

    
  
(In reply to Shane Caraveo (:mixedpuppy) from comment #34)
> (In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #18)
> > In examining a number of existing tab-related add-ons on AMO, it appears the
> > most common use cases are to open a tab:
> >  1) to the left
> >  2) to the right 
> >  3) at the end of the current tab strip
> > 
> > If supporting just those use cases makes implementation easier, it would
> > capture the majority of what extension developers do today.
> 
> Why would we have an insert before?  Is that really a use case people want? 
> Why would one do that to themselves?

If we eliminated the before case, this bug becomes much simpler and basically I think the first patch I submitted might be sufficient.

IMNSHO, those kinds of distinctions kind of get into the realm of nit-picky "I want it just so", along with wanting options for rtl or ltr for related-to-current order for multiple tab opening.

OTH, having a tab open at the end of the tabs bar versus (somewhere) next to the current tab has a pretty dramatic impact on UX.

    
    

    
  
(In reply to Kevin Jones from comment #36)
> If we eliminated the before case, this bug becomes much simpler and
> basically I think the first patch I submitted might be sufficient.

...of course changing that from a browser.tabs method to a browserSetting.

    
    

    
  
(In reply to Kevin Jones from comment #36)
> IMNSHO, those kinds of distinctions kind of get into the realm of nit-picky
> "I want it just so", along with wanting options for rtl or ltr for
> related-to-current order for multiple tab opening.

I am sorry if my comments caused this reaction, and I see how that distinction would appear nit-picky. It wasn't my intention. I admit I am a bit irritated by a minor disruption in my workflow caused by FF57 and the demise of Tab Mix Plus that could fully control [1] how and where new tabs open, and since no webextension is able to replace it in a clean manner, I thought my feedback on the importance of that rtl/ltr order would be helpful. But maybe it's just a case of "spacebar heating".

[1] http://tabmixplus.org/support/viewpage.php?t=3&p=events-new-tabs

    
    

    
  
(In reply to Kevin Jones from comment #36)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #34)
> > (In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #18)
> > > In examining a number of existing tab-related add-ons on AMO, it appears the
> > > most common use cases are to open a tab:
> > >  1) to the left
> > >  2) to the right 
> > >  3) at the end of the current tab strip
> > > 
> > > If supporting just those use cases makes implementation easier, it would
> > > capture the majority of what extension developers do today.
> > 
> > Why would we have an insert before?  Is that really a use case people want? 
> > Why would one do that to themselves?
> 
> If we eliminated the before case, this bug becomes much simpler and
> basically I think the first patch I submitted might be sufficient.
> 
> IMNSHO, those kinds of distinctions kind of get into the realm of nit-picky
> "I want it just so", along with wanting options for rtl or ltr for
> related-to-current order for multiple tab opening.
> 
> OTH, having a tab open at the end of the tabs bar versus (somewhere) next to
> the current tab has a pretty dramatic impact on UX.

I have no idea why inserting a tab before would be useful, other than to say "everybody is different" and some people seemed to do it. Right now, I'd much rather see us land the "after and end" cases ASAP and open a separate issue to track the "before" case, where we can debate the merits of the feature versus the scope of work required.
Flags: needinfo?(mconca)

    
    

    
  
(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #39)
> I have no idea why inserting a tab before would be useful, other than to say
> "everybody is different" and some people seemed to do it.
Seems like some people like to have reverse tab order in vertical tabs extensions (new tab opens at the top instead of bottom). If they want to open tab next to current, they need it before instead of after.
I, for example, just want ability to open tabs right to the current ([a] 1 2 3 b c variant that mentioned above), which hopefully would be compatible with all vertical tabs extensions. Currently they doing it via hacks (those that have this feature) and compatibility with each other and native tabbar (brings problems when switching from one to another) and with Always Right/Open Tabs Next to Current extensions is just awful (especially in Tab Center Redux, see https://github.com/eoger/tabcenter-redux/issues/163).

    
  
Blocks: 1226546

    
    

    
      
  


  

    
    

    
  
(In reply to Kevin Jones from comment #42)
> Created attachment 8936487 [details]
> Bug 1344749 - about:config pref to open new tab next to current, includes WE
> browserSetting
> 
> Review commit: https://reviewboard.mozilla.org/r/207194/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/207194/

Kris, Mike, Shane, I invite you to have a look at this.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mconca)
Flags: needinfo?(kmaglione+bmo)

    
    

    
  
Comment on attachment 8936487 [details]
Bug 1344749 - about:config pref to open new tab next to current, includes WE browserSetting

Bug 933532 wants to add a pref to open "unrelated tabs", not just new tabs, next to the current one. Not really sure what's preferable.
See Also:  → 933532

    
    

    
  
Ideally people should be able to customize all of them to the way they want. New tabs, bookmarks, you name it. A singular pref will never satisfy everyone.

    
    

    
  
(Cont'd, I sent my previous one accidently)

So I'd say a comprehensive tab open position API for WebExt will be a much better implementation than just keeping adding these prefs.

    
    

    
  
I will no longer be able to spend time contributing to bugs for Mozilla.  I am un-assigning myself from this bug.
Assignee: kevinhowjones → nobody

    
  
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mconca)
Flags: needinfo?(kmaglione+bmo)

    
  

        Attachment #8932056 -
        Flags: feedback?(kmaglione+bmo)

    
    

    
  
With Kevin not being able to finish this off, would you be able to take this on in the new year?
Flags: needinfo?(bob.silverberg)

    
    

    
  
(In reply to Andy McKay [:andym] from comment #48)
> With Kevin not being able to finish this off, would you be able to take this
> on in the new year?

Sure, I'll add it to my queue.
Assignee: nobody → bob.silverberg
Component: WebExtensions: Untriaged → WebExtensions: General
Flags: needinfo?(bob.silverberg)

    
    

    
  
Here is what we have decided to implement:

A new pref will be added, browser.tabs.newUnrelatedTabPosition, which will allow a user to control in what position a new unrelated tab will be opened. Two values will be supported: "afterCurrent" and "endOfTabStrip".

Two new browserSettings APIs will be added: browserSettings.newRelatedTabPosition and browserSettings.newUnrelatedTabPosition, both of which will also accept the two values listed above. "afterCurrent" will be the default value for the former and "endOfTabStrip" will be the default value for the latter.

A separate bug will be opened to discuss adding support for "beforeCurrent" to each of those settings.

    
    

    
  
What does "Unrelated Tab" include, though?

New tab, sidebar middle click, bookmark bar click?

    
    

    
  
I'm not sure the browserSettings type API you mentioned is sufficient (If I misunderstood anything, feel free to correct!).

For example, if an user wants to open new tab at the end, but tabs from bookmark bar click after current (or vise versa, or any combinations), like what they used to be able to do with Tab Mix Plus, the new API doesn't sound like capable.

    
    

    
      
  


  

    
    

    
  
(In reply to fireattack from comment #52)
> What does "Unrelated Tab" include, though?
> 
> New tab, sidebar middle click, bookmark bar click?

Yes, all of those.

    
    

    
  
(In reply to fireattack from comment #53)
> I'm not sure the browserSettings type API you mentioned is sufficient (If I
> misunderstood anything, feel free to correct!).
> 
> For example, if an user wants to open new tab at the end, but tabs from
> bookmark bar click after current (or vise versa, or any combinations), like
> what they used to be able to do with Tab Mix Plus, the new API doesn't sound
> like capable.

For now, we're not looking at making it any more granular than described in comment 51. If an extension overrides where new unrelated tabs open, they all will open in that position.
Blocks: 1431840

    
    

    
  
(In reply to Bob Silverberg [:bsilverberg] from comment #51)

> A separate bug will be opened to discuss adding support for "beforeCurrent"
> to each of those settings.

This has been filed as bug 1431840.

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/214374/#review220326

General NIT: I'm not crazy about the "related" and "unrelated" naming, but that could just be me.  If "related" always means a child tab, I think I'd prefer to see something like defaultNewTabPosition or newTabPosition, and newChildTabPosition.  I do see "related" being used in tabbrowser, maybe it's just the "unrelated" that could go.  In any case, I don't feel like I have vastely better naming on this item, so I'm not going to block.

::: browser/base/content/test/tabs/browser_new_unrelated_tab_position.js:27
(Diff revision 1)
> +  }
> +
> +  await addTab("http://mochi.test:8888/#0");
> +  await addTab("http://mochi.test:8888/#1");
> +  await addTab("http://mochi.test:8888/#2");
> +  await addTab("http://mochi.test:8888/#3");

It might be slightly easier to do a bunch of tab stuff using sessions like I did in browser_ext_tabs_hide.js.

::: browser/base/content/test/tabs/browser_new_unrelated_tab_position.js:32
(Diff revision 1)
> +  await addTab("http://mochi.test:8888/#3");
> +
> +  // Create a new tab page which has a link to "example.com".
> +  let dir = getChromeDir(getResolvedURI(gTestPath));
> +  dir.append("file_new_tab_page.html");
> +  let newTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, Services.io.newFileURI(dir).spec);

Seems like this is opening a chrome url, perhaps:  getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.com");

Not sure it matters.

::: toolkit/components/extensions/schemas/browser_settings.json:134
(Diff revision 1)
>          "$ref": "types.Setting",
>          "description": "Controls the behaviour of image animation in the browser. This setting's value is of type ImageAnimationBehavior, defaulting to <code>normal</code>."
>        },
> +      "newRelatedTabPosition": {
> +        "$ref": "types.Setting",
> +        "description": "Controls where new related tabs are opened. Can be either `afterCurrent` or `endOfTabStrip`, with `afterCurrent` being the default."

What is a "related" tab?  It should be documented (could be in MDN)

        Attachment #8944043 -
        Flags: review?(mixedpuppy) → review+

    
    

    
      
  

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/214374/#review220326

I agree "unrelated" is kind of ugly, and maybe using "child" instead of related would be better, but, as you point out, the existing pref is called "insertRelatedAfterCurrent", so I wanted to keep the pref names consistent. We could use different names for the API, and still call the new pref "newUnrelatedTabPosition", but that feels weird too. I did discuss the names with mconca, and he signed off on them, so I think we'll just go with them for now.

> It might be slightly easier to do a bunch of tab stuff using sessions like I did in browser_ext_tabs_hide.js.

Regarding the test, tbh, I pretty much just lifted the code for the test, and for the changes to tabbrowserl.xml from the patch  attached to bug 933532 which was written by Masayuki Nakano. I assumed that the format of the test was consistent with other tests in the same folder. I think I'll leave it as is for now, and see if any changes are requested by dao.

> Seems like this is opening a chrome url, perhaps:  getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.com");
> 
> Not sure it matters.

Thanks, I updated this.

> What is a "related" tab?  It should be documented (could be in MDN)

Good point. I updated the description of both `newRelatedTabPosition` and `newUnrelatedTabPosition` in browser_settings.json to be more descriptive, and I'll make sure to check the docs on MDN once they've been written to ensure that it is well described.

    
    

    
      
  

        Attachment #8920839 -
        Attachment is obsolete: true

        Attachment #8936487 -
        Attachment is obsolete: true

        Attachment #8932056 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Bob Silverberg [:bsilverberg] from comment #51)
> A new pref will be added, browser.tabs.newUnrelatedTabPosition, which will
> allow a user to control in what position a new unrelated tab will be opened.
> Two values will be supported: "afterCurrent" and "endOfTabStrip".

I don't think we should assume that we'll want to extend this later, so I think this should be a boolean pref.

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #62)
> (In reply to Bob Silverberg [:bsilverberg] from comment #51)
> > A new pref will be added, browser.tabs.newUnrelatedTabPosition, which will
> > allow a user to control in what position a new unrelated tab will be opened.
> > Two values will be supported: "afterCurrent" and "endOfTabStrip".
> 
> I don't think we should assume that we'll want to extend this later, so I
> think this should be a boolean pref.

Fair point. We could make this a boolean pref for now, and if we want to extend it for the API later we can deal with the implementation of that at that time. I'll make that change and update the patch.

    
    

    
      
  

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/214374/#review221622

::: browser/app/profile/firefox.js:468
(Diff revision 4)
> +// append new tab to the end.
>  pref("browser.tabs.insertRelatedAfterCurrent", true);
> +// Open even unrelated links, e.g., bookmarks, history items at next to
> +// current tab if |insertUnreleatedAfterCurrent| is true.  Otherwise,
> +// always append new tab to the end.
> +pref("browser.tabs.insertUnreleatedAfterCurrent", false);

Let's rename this to browser.tabs.insertAfterCurrent, and if set to true, let's make that trump browser.tabs.insertRelatedAfterCurrent as it wouldn't make sense to insert orphan tabs after the current one but child tabs at the end.

        Attachment #8944043 -
        Flags: review?(dao+bmo) → review-

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/214374/#review221622

> Let's rename this to browser.tabs.insertAfterCurrent, and if set to true, let's make that trump browser.tabs.insertRelatedAfterCurrent as it wouldn't make sense to insert orphan tabs after the current one but child tabs at the end.

I can see your logic here, but I'm not sure extension developers will appreciate us taking away granular control. You are suggesting that there be the following three options:
1. Child tabs open next to current, orphan tabs open at the end of the strip. This is the current default behaviour.
2. Both child and orphan tabs open at the end of the strip. This is the current behaviour with `browser.tabs.insertRelatedAfterCurrent` set to false.
3. Both child and orphan tabs open next to current. This would be the behaviour if the new pref, which you're suggesting calling `browser.tabs.insertAfterCurrent`, is set to true.

It would not be possible for an extension to configure Firefox so that child tabs open at the end of the strip and orphan tabs open next to the current one.

I just want to check with Mike to see if he foresees any issues from extension developers if we place this limitation on the API. One advantage of this is that we could create a single API with three possible values, instead of two APIs each with two possible values. It also, however, assumes we're never going to implement bug 1431840, which I know you don't support, but again I want to see what Product has to say about it.

    
    

    
  
Mike, can you please take a look at comment 66 and let us know if you support only allowing for those three states?
Flags: needinfo?(mconca)

    
    

    
  
(In reply to Bob Silverberg [:bsilverberg] from comment #66)
> It would not be possible for an extension to configure Firefox so that child
> tabs open at the end of the strip and orphan tabs open next to the current
> one.

Which is precisely my point, because this combination doesn't seem to make sense. :)

    
    

    
      
  

    
    

    
  
Can we stop deciding which "makes sense" and what does not for users, especially in a ticket that is supposed to extend the API? I really, really hate this mindset. 

(Removed off-topic parts.)

    
    

    
  
(In reply to Bob Silverberg [:bsilverberg] from comment #66)
> I just want to check with Mike to see if he foresees any issues from
> extension developers if we place this limitation on the API. One advantage
> of this is that we could create a single API with three possible values,
> instead of two APIs each with two possible values. It also, however, assumes
> we're never going to implement bug 1431840, which I know you don't support,
> but again I want to see what Product has to say about it.

The WebExtensions API should not place limits on what can and cannot be done from a UX point-of-view. With Firefox, Mozilla strives to provide the best UX possible in the default browser, the UX that is most intuitive to most users.  The WebExtensions API, however, should provide the hooks necessary for developers to create alternate UX experiences for users. These may or may not be useful to any large segment of users, and may or may not make sense to you or I. The point of extensions, though, is to both broaden the capabilities of Firefox as well as alter its behavior so that individuals can shape their own experiences on the Internet.

I appreciate that certain combinations of parameters may not make sense in the default Firefox browser, but we cannot impose that judgment on the API. Please maintain the API as two separate functions with (for the moment) two options each.
Flags: needinfo?(mconca)

    
    

    
  
We don't provide prefs and extension hooks arbitrarily. While obviously extensions allow users to do things differently from the default, so far nobody has presented a reasonable use case for said pref combination, so my position remains that browser.tabs.insertUnreleatedAfterCurrent should be browser.tabs.insertAfterCurrent, and that browser.tabs.insertAfterCurrent=true needs to trump browser.tabs.insertRelatedAfterCurrent.
Blocks: 1311472

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #72)
> We don't provide prefs and extension hooks arbitrarily. While obviously
> extensions allow users to do things differently from the default, so far
> nobody has presented a reasonable use case for said pref combination, so my
> position remains that browser.tabs.insertUnreleatedAfterCurrent should be
> browser.tabs.insertAfterCurrent, and that
> browser.tabs.insertAfterCurrent=true needs to trump
> browser.tabs.insertRelatedAfterCurrent.

Thanks, Dão. The desire for a use case is understandable, but for an API, it is important to provide a complete toolkit for developers so they can implement creative and powerful extensions; extensions that make Firefox an attractive browser to users everywhere. I honestly don't know why someone would want to open child tabs at the end and orphan tabs next to the current tab - but that doesn't matter. Extensions with hundreds of thousands of users did it in the past, and extensions are trying to do it now, only without this API, it provides a terrible user experience (see comment 5). 

I absolutely appreciate that we don't provide prefs arbitrarily. Nothing is free, and there are maintenance and support considerations for nearly every change. Initial technical scope, potential performance implications, and anticipated technical debt should all be factors in the decision. If any of those apply, please let me know.

    
    

    
  
(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #73)
> (In reply to Dão Gottwald [::dao] from comment #72)
> > We don't provide prefs and extension hooks arbitrarily. While obviously
> > extensions allow users to do things differently from the default, so far
> > nobody has presented a reasonable use case for said pref combination, so my
> > position remains that browser.tabs.insertUnreleatedAfterCurrent should be
> > browser.tabs.insertAfterCurrent, and that
> > browser.tabs.insertAfterCurrent=true needs to trump
> > browser.tabs.insertRelatedAfterCurrent.
> 
> Thanks, Dão. The desire for a use case is understandable, but for an API, it
> is important to provide a complete toolkit for developers so they can
> implement creative and powerful extensions; extensions that make Firefox an
> attractive browser to users everywhere. I honestly don't know why someone
> would want to open child tabs at the end and orphan tabs next to the current
> tab - but that doesn't matter. Extensions with hundreds of thousands of
> users did it in the past, and extensions are trying to do it now, only
> without this API, it provides a terrible user experience (see comment 5).

Nothing I've read in this bug so far seems to support the assumption that "open child tabs at the end and orphan tabs next to the current tab" is a setting that anybody actually wants or cares about, so we need more details on that use case. "It doesn't matter" is unsatisfying, because this directly factors into the cost/benefit question that we need to ask when adding APIs. What extensions specifically are we talking about that wouldn't be satisfied with the prefs I suggested?

> I absolutely appreciate that we don't provide prefs arbitrarily. Nothing is
> free, and there are maintenance and support considerations for nearly every
> change. Initial technical scope, potential performance implications, and
> anticipated technical debt should all be factors in the decision. If any of
> those apply, please let me know.

Technical debt is always a concern with tabbrowser, yes.
Flags: needinfo?(mconca)

    
    

    
      
  

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #74)
> What extensions specifically are we talking about that wouldn't be satisfied
> with the prefs I suggested?

Tab mixplus

http://www.tabmixplus.org/support/help/images/events-new-tabs.jpg

I, for one, prefer to open related and new tab next to current, but ones from bookmark at the end.

    
    

    
  
(In reply to fireattack from comment #76)
> (In reply to Dão Gottwald [::dao] from comment #74)
> > What extensions specifically are we talking about that wouldn't be satisfied
> > with the prefs I suggested?
> 
> Tab mixplus
> 
> http://www.tabmixplus.org/support/help/images/events-new-tabs.jpg
> 
> I, for one, prefer to open related and new tab next to current, but ones
> from bookmark at the end.

I should have been clearer: extensions that wouldn't be satisfied with the prefs I suggested but would be with Bob's.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
This bug has already progressed pretty far, so let's stay focused on discussing the patch at hand. Anything beyond that can be separate feature / API requests.

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #85)
> This bug has already progressed pretty far, so let's stay focused on
> discussing the patch at hand. Anything beyond that can be separate feature /
> API requests.

Filed bug 1436738.

    
  
Depends on: 1436738
Blocks: 1436738
No longer depends on: 1436738

    
  
No longer blocks: 1436738
Depends on: 1436738

    
  
No longer depends on: 1436738

    
    

    
  
I will try to summarize the salient points from the latest set of comments since I was asked for more information:

Is there a use case for opening child tabs at the end, orphan tabs next to the current?
* Some people might prefer to open children at the end to create a "read these later" queue, while opening bookmarks and new tabs next to their current tab for ease of navigation. This is particularly useful when the tab strip overflows so that users don't have to scroll.

Are there extensions requesting this?
* Several comments mention TabMixPlus.  This extension offered this type of tab opening flexibility (and even more). Prior to WebExtensions, TabMixPlus had approximately 500,000 users. The current version of TabMixPlus using WebExtensions has approximately 3,000 users. Providing additional tab positioning features as part of the WebExtensions API would help users of TabMixPlus regain lost functionality.

Would a boolean pref satisfy this request?
* openAfterCurrent=true does not permit the use case above
* The original request provided four unique permutations of tab opening position, the boolean pref offers three unique permutations. TabMixPlus users had at least four possible permutations, so it is presumed that providing three will be less satisfactory.
Flags: needinfo?(mconca)

    
    

    
  
(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #87)
> I will try to summarize the salient points from the latest set of comments
> since I was asked for more information:
> 
> Is there a use case for opening child tabs at the end, orphan tabs next to
> the current?
> * Some people might prefer to open children at the end to create a "read
> these later" queue, while opening bookmarks and new tabs next to their
> current tab for ease of navigation. This is particularly useful when the tab
> strip overflows so that users don't have to scroll.

This seems a bit constructed. I'm willing to believe that someone out there might want this. I'm skeptical that this is common enough that we should support it.

> Are there extensions requesting this?
> * Several comments mention TabMixPlus.  This extension offered this type of
> tab opening flexibility (and even more).

Those comments were off-topic. What these users were asking for is not part of what Bob's patch offers.

> Prior to WebExtensions, TabMixPlus
> had approximately 500,000 users. The current version of TabMixPlus using
> WebExtensions has approximately 3,000 users. Providing additional tab
> positioning features as part of the WebExtensions API would help users of
> TabMixPlus regain lost functionality.
> 
> Would a boolean pref satisfy this request?
> * openAfterCurrent=true does not permit the use case above
> * The original request provided four unique permutations of tab opening
> position, the boolean pref offers three unique permutations. TabMixPlus
> users had at least four possible permutations, so it is presumed that
> providing three will be less satisfactory.

There's a more fundamental question we've been dancing around: Are we going to support all of what TabMixPlus was doing? I'm quite positive that the answer to this is no. TMP offered all kinds of fringe customization that cannot realistically supported. That's not because it would be technically unfeasible but because it would be too expensive to maintain such a large API. I thought this was clear when we abandoned legacy extensions. I'm sympathetic to adding extension hooks where we think that makes sense; I'm less sympathetic to the "TMB did it so let's support it" theme.

    
    

    
  
Maintaining a symmetrical set of options would be less expensive than artificially excluding one combination out of four. Dão, this whole bike-shedding around a simple API that would make some workflows viable (including those that we can't imagine right now) is very sad to witness. I really hope that someone more reasonable would handle this bug so that it can finally move ahead.

Also, when Mozilla promised to keep TMP working, there was not a single mention of this sentiment about fringe customizations that you express right now.

    
    

    
  
(In reply to monk-time from comment #89)
> Maintaining a symmetrical set of options would be less expensive than
> artificially excluding one combination out of four.

What makes you think that? "Symmetrical" is pretty meaningless here. It's another use case to support and take care of whenever we touch this code.

> Dão, this whole
> bike-shedding around a simple API that would make some workflows viable
> (including those that we can't imagine right now) is very sad to witness. I
> really hope that someone more reasonable would handle this bug so that it
> can finally move ahead.

"bike-shedding" seems to suggest that you don't really care about said use case either. I understand that this makes you lean towards "just supporting it." I hope you understand that, as one of the people who'll have to maintain this, I'm leaning towards not supporting something we don't really care about.

> Also, when Mozilla promised to keep TMP working, there was not a single
> mention of this sentiment about fringe customizations that you express right
> now.

Where was this promised? I can't imagine that any Firefox engineer who's slightly familiar with tabbrowser was on board with that.

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #90)
> What makes you think that? "Symmetrical" is pretty meaningless here. It's
> another use case to support and take care of whenever we touch this code.

I disagree. There are 2x2 permutations that come out naturally from two types of new tabs (orphan and related tabs) and two places to put them. Two types, two places - that's the symmetry I am talking about. I can't imagine a scenario where only one out of four breaks, and if one were to appear in the future it would be a sign of a deeper issue around opening tabs that has no relation to this API.

As it often happens with such symmetrical cases, artificially constraining this would mean making the code more brittle and complex, not the opposite. I do understand the difficulty of maintaining "fridge" features, but in this case having that feature would be a simpler solution with less maintenance than not having it.

> Where was this promised? I can't imagine that any Firefox engineer who's
> slightly familiar with tabbrowser was on board with that.

Soon after the initial blog post about migrating to WebExtensions (mid-2015) a short list of extensions that would be supported no matter what was often cited in discussions about the move. I can't find the example I had in mind where this promise was more explicit, but these two can be (and often were) read as a promise to support TMP:

https://billmccloskey.wordpress.com/2015/08/21/firefox-add-on-changes/
https://wiki.mozilla.org/WebExtensions/Future

    
    

    
  
(In reply to monk-time from comment #91)
> (In reply to Dão Gottwald [::dao] from comment #90)
> > What makes you think that? "Symmetrical" is pretty meaningless here. It's
> > another use case to support and take care of whenever we touch this code.
> 
> I disagree. There are 2x2 permutations that come out naturally from two
> types of new tabs (orphan and related tabs) and two places to put them. Two
> types, two places - that's the symmetry I am talking about. I can't imagine
> a scenario where only one out of four breaks, and if one were to appear in
> the future it would be a sign of a deeper issue around opening tabs that has
> no relation to this API.
>
> As it often happens with such symmetrical cases, artificially constraining
> this would mean making the code more brittle and complex, not the opposite.
> I do understand the difficulty of maintaining "fridge" features, but in this
> case having that feature would be a simpler solution with less maintenance
> than not having it.

Not supporting that case literally means replacing a ternary operator with two logical operators. There's no inherent brittleness or complexity to this. Supporting it means there's an extra case to keep in mind should we change or fundamentally refactor this code, plus having automated tests for it. No matter how you spin it, supporting another case doesn't make our lives easier in any way.

Engineering aside, there's a cost of providing weird pref combinations that might put users in unintended states (a.k.a. footguns).

> > Where was this promised? I can't imagine that any Firefox engineer who's
> > slightly familiar with tabbrowser was on board with that.
> 
> Soon after the initial blog post about migrating to WebExtensions (mid-2015)
> a short list of extensions that would be supported no matter what was often
> cited in discussions about the move. I can't find the example I had in mind
> where this promise was more explicit, but these two can be (and often were)
> read as a promise to support TMP:
> 
> https://billmccloskey.wordpress.com/2015/08/21/firefox-add-on-changes/
> https://wiki.mozilla.org/WebExtensions/Future

These just recognize TMP as a popular extension that is hard to support, hence the talk about direct or "superpower" access (I believe this motivated WebExtension Experiments). A promise we definitely did make is that we'd look into providing a growing number of APIs. That's what we're doing here. We should absolutely support as many TMP features as we think is reasonable, but we need to draw the line somewhere.

    
    

    
  
Supporting this case means at most as much code as that. If you are testing the three other combinations (are you?), testing the fourth means copying a case for A & !B and flipping it to !A & B.

It's definitely going to be hard to support extensions if every little feature is blocked for months by devs who can't possibly imagine a use for it. Firefox's ability to customize UI has already become very limited, I'm sure there are much better areas to apply feature frugality.

    
    

    
  
Maybe, rather than arguing which limited subset of cases to support, it would be better to solve the problem generically as requested in bug 1387372 comment 14?

    
    

    
  
(In reply to monk-time from comment #93)
> It's definitely going to be hard to support extensions if every little
> feature is blocked for months by devs who can't possibly imagine a use for
> it. Firefox's ability to customize UI has already become very limited, I'm
> sure there are much better areas to apply feature frugality.

I don't know how we arrived at the idea that somebody imagining a use (rather than e.g. demonstrating significant demand) is the bar that needs to be passed for adding extension APIs, let alone not even imagining a use; that's a recipe for catastrophic API design and just unsustainable.

(In reply to Yuri Khan from comment #94)
> Maybe, rather than arguing which limited subset of cases to support, it
> would be better to solve the problem generically as requested in bug 1387372
> comment 14?

The problem with that is that we can only query add-ons asynchronously but we need to know immediately where to open new tabs.

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #95)
> > Maybe, rather than arguing which limited subset of cases to support, it
> > would be better to solve the problem generically as requested in bug 1387372
> > comment 14?
> 
> The problem with that is that we can only query add-ons asynchronously but
> we need to know immediately where to open new tabs.

Every asynchronous API can be used in a synchronous way by awaiting on the returned future, can’t it? Shunt it with a 100ms timeout if necessary, I don’t know. That is an engineering problem that can be solved. Deciding between supporting three vs four cases out of infinity, on the other hand, is nigh impossible.

    
    

    
  
(In reply to Yuri Khan from comment #96)
> Every asynchronous API can be used in a synchronous way by awaiting on the
> returned future, can’t it? Shunt it with a 100ms timeout if necessary, I
> don’t know. That is an engineering problem that can be solved. Deciding
> between supporting three vs four cases out of infinity, on the other hand,
> is nigh impossible.

The UI needs to react immediately, we don't have 100ms to spare here. Plus tabbrowser.addTab is synchronous and making it async would be a huge undertaking.

    
    

    
  
Thank you for your input, Dão. As an experienced developer who knows tabbrowser as well as anyone, we will respect your decision to r- this feature request. I have removed this API from the WebExtensions roadmap and will continue to monitor market channels to determine what APIs we can offer to Firefox developers in the future.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
Resolution: --- → WONTFIX

    
    

    
  
Wait, I don't understand why the much more important use-case of allowing new (empty) tabs to open next to the current tab should get the axe? The discussion above was about introducing a fourth combination (quite an obscure one, I admit) of [child tabs at the end, orphan/blank tabs near]. But the main issue, an ability to have [orphan tabs at the end] was already approved, and I don't recall anyone having any doubts about its usefulness?

I now feel really bad that my inopportune and unwise bickering-for-the-sake-of-it about an unimportant extra detail caused the main UI quirk to be dismissed.  I apologize for being stubborn and dragging discussion away from the main issue to an insignificant detail. 

Mike, I (and likely all 105 users who voted for this issue) hope you would reconsider. Punishing everyone for one fool (me) being too argumentative seems excessively cruel. All available add-ons for opening blank tabs near the current introduce significant visual noise that makes the very common action feel sluggish. That's the main issue.

    
    

    
  
> an ability to have [orphan tabs at the end]
Correction: an ability to have [orphan tabs next to current]

    
    

    
  
(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #98)
> Thank you for your input, Dão. As an experienced developer who knows
> tabbrowser as well as anyone, we will respect your decision to r- this
> feature request. I have removed this API from the WebExtensions roadmap and
> will continue to monitor market channels to determine what APIs we can offer
> to Firefox developers in the future.

Isn't Mr. Dão Gottwald's opinion (at comment #65) just to rename browser.tabs.insertUnreleatedAfterCurrent to browser.tabs.insertAfterCurrent, and make it override browser.tabs.insertRelatedAfterCurrent?

Why the whole thing is closed? If what he suggested is followed and implemented, at least people can have the all the tabs (related or unrelated) to open next to the current.

    
    

    
  
> WONTFIX

Does it mean that there is no plan to allow webextensions to control the position of the new tab? What about browser prefs? Is browser.tabs.insertUnreleatedAfterCurrent added?

    
    

    
  
Mike, I can't seem to find where Dão r-'ed this whole request?!
I can only find comments regarding the approval and implementation (see comment 19 and comment 85).

I mean 'chapeau' to you and Dão for answering really every comment patiently here, but I thought comment 85 was the current status, not?
Flags: needinfo?(mconca)

    
    

    
  
This must be a very sad misunderstanding. The only thing that was debated here was the 4th option, the edge case.

Those of us who work with hundreds of tabs, find the ability to open any kind of new tabs next to current, absolutely critical.
The current situation for this is a VERY annoying visual mess due to the panning of the tabstrip.

It is also very sad to see that a dispute about an edge case, which should be handled outside the main bug in the first place and in a non-blocking way, is causing further delays. Without looking at metrics, I am pretty sure there are thousands upon thousands of users who utilize more than 50 tabs, having UX affected severely by this. UX affecting issues should move much faster than this, specially considering the patch is already there..

    
    

    
  
I think closing this off might have been a bit premature, but I can understand the frustration.

At this point I'd like to ask if Bob would be willing to finish this bug with the changes that Dão requested. With that in place, we can file a follow-up to discuss & possibly implement the edge cases that have been discussed at length here.

To me, this seems the most productive way forward.


Speaking of discussion: I find the number of examples of bad form in this bug to be staggering; if you're inclined to drop in cheap words with childish comments belittling the folk who are trying their hardest to create the best, independent and privacy-minded web browser out there, please count to ten and kill the urge.
Bug comments are supposed to be productive, constructive and above all respectful; be mindful of who you're conversing with. We're taking this serious. It might be you're not, whims aplenty, but hold your peace if so.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → ASSIGNED

    
    

    
  
> if you're inclined to drop in cheap words with childish comments belittling the folk who are trying their hardest to create the best, independent and privacy-minded web browser out there, please count to ten and kill the urge

I apologize once again. My behavior here was totally not acceptable, and I am going to refrain from adding more noise to the discussion. I'm very sorry.

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #105)
> To me, this seems the most productive way forward.

 Agreed. It is rather counter-productive to block an issue for more than a month, discussing a possible edge case, in bad form as well as you very well put it. Discussions about "promises" etc etc are BBS/forum material and don't really belong in a bug tracker. There is also no point in discussing something, in this context, that no one has actually actively requested.

 Mr Dão's requested changes should have been implemented from the start, with a follow-up posted by any actual interested parties for anything additional. So we can move forward.

    
    

    
  
Fixing bug 1387372 should cover this use case.

    
    

    
  
But this one is essentially the fix for bug 1387372.

It is the first of the three possible solutions proposed by Shane Caraveo in bug 1387372 comment 5 and the one actually selected according to comment 19.

    
    

    
      
  

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

This has been updated per the discussion in today's meeting. Can you please take another look, Shane and Dao?

        Attachment #8944043 -
        Flags: review+ → review?(mixedpuppy)

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/214374/#review229666

::: browser/app/profile/firefox.js:464
(Diff revision 5)
>  // Tabbed browser
>  pref("browser.tabs.closeWindowWithLastTab", true);
> +// Open related links to a tab, e.g., link in current tab, at next to the
> +// current tab if |insertRelatedAfterCurrent| is true.  Otherwise, always
> +// append new tab to the end.
>  pref("browser.tabs.insertRelatedAfterCurrent", true);

I think we can/should do this with one pref since insertRelatedAfterCurrent is only used in one place (doesn't have tests either).  This would also simplify the webext bits.  

I would propose:

// enum [atEnd, relatedAfterCurrent, afterCurrent]
pref(browser.tabs.insertTabPosition, atEnd)

Whether it's an integer or string doesn't matter much to me, might to others.

::: toolkit/components/extensions/schemas/browser_settings.json:136
(Diff revision 5)
>        },
>        "newTabPageOverride": {
>          "$ref": "types.Setting",
>          "description": "Returns the value of the overridden new tab page. Read-only."
>        },
> +      "newTabPosition": {

android?

        Attachment #8944043 -
        Flags: review?(mixedpuppy)

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/214374/#review229666

> I think we can/should do this with one pref since insertRelatedAfterCurrent is only used in one place (doesn't have tests either).  This would also simplify the webext bits.  
> 
> I would propose:
> 
> // enum [atEnd, relatedAfterCurrent, afterCurrent]
> pref(browser.tabs.insertTabPosition, atEnd)
> 
> Whether it's an integer or string doesn't matter much to me, might to others.

This is an interesting idea, I'm not sure why it wasn't raised before. If we do that do we have to worry about deprecating/migrating the existing preference, or do we just remove it and anyone who had it set to a non-default value will have to set the new one?

> android?

Sorry, can you be a bit more explicit? What about android? Does tabbrowser exist on android?

    
    

    
  
(In reply to Bob Silverberg [:bsilverberg] from comment #113)
> Comment on attachment 8944043 [details]
> Bug 1344749 - Expose API to customize where new tabs open
> 
> https://reviewboard.mozilla.org/r/214374/#review229666
> 
> > I think we can/should do this with one pref since insertRelatedAfterCurrent is only used in one place (doesn't have tests either).  This would also simplify the webext bits.  
> > 
> > I would propose:
> > 
> > // enum [atEnd, relatedAfterCurrent, afterCurrent]
> > pref(browser.tabs.insertTabPosition, atEnd)
> > 
> > Whether it's an integer or string doesn't matter much to me, might to others.
> 
> This is an interesting idea, I'm not sure why it wasn't raised before. If we
> do that do we have to worry about deprecating/migrating the existing
> preference

That might be a reason to keep two, but IMO it does complicate the code base.  I'd go for migrating the existing pref to reduce complexity long term.

> > android?
> 
> Sorry, can you be a bit more explicit? What about android? Does tabbrowser
> exist on android?

The settings are in toolkit and look available to android (didn't verify), so what happens with their use on android?  I assume nothing, it should at least be a doc note.

    
    

    
  
(In reply to Shane Caraveo (:mixedpuppy) from comment #114)
> (In reply to Bob Silverberg [:bsilverberg] from comment #113)

> > This is an interesting idea, I'm not sure why it wasn't raised before.

Yeah, I just assumed there were other dependencies on the pref.  Seeing the code again I thought "what a PITA juggling two prefs".

    
    

    
  
(In reply to Shane Caraveo (:mixedpuppy) from comment #112)
> I think we can/should do this with one pref since insertRelatedAfterCurrent
> is only used in one place (doesn't have tests either).  This would also
> simplify the webext bits.  
> 
> I would propose:
> 
> // enum [atEnd, relatedAfterCurrent, afterCurrent]
> pref(browser.tabs.insertTabPosition, atEnd)
> 
> Whether it's an integer or string doesn't matter much to me, might to others.

Integers are annoying because you need to look them up to read or write them. Strings are annoying because you still need to look them up to write them and typos are made easily. Integers are traditionally the preferred choice between the two, but clearly neither is ideal. So I'd prefer boolean prefs, but maybe I'm underestimating the complexity of the webextension framework code. Perhaps another option would be to just expose the two prefs as two boolean settings?
Flags: needinfo?(mconca)

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/214374/#review229826

The tabbrowser.xml code has now moved to tabbrowser.js (bug 1392352).

        Attachment #8944043 -
        Flags: review?(dao+bmo)

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #116)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #112)
> > I think we can/should do this with one pref since insertRelatedAfterCurrent
> > is only used in one place (doesn't have tests either).  This would also
> > simplify the webext bits.  
> > 
> > I would propose:
> > 
> > // enum [atEnd, relatedAfterCurrent, afterCurrent]
> > pref(browser.tabs.insertTabPosition, atEnd)
> > 
> > Whether it's an integer or string doesn't matter much to me, might to others.
> 
> Integers are annoying because you need to look them up to read or write
> them. Strings are annoying because you still need to look them up to write
> them and typos are made easily. Integers are traditionally the preferred
> choice between the two, but clearly neither is ideal. So I'd prefer boolean
> prefs, but maybe I'm underestimating the complexity of the webextension
> framework code. Perhaps another option would be to just expose the two prefs
> as two boolean settings?

Although a single pref would be simpler, the webextension code is by no means particularly complicated. Based on the above I am in favour of keeping things as they are, using two boolean prefs and exposing them via a single setting. This also eliminates the need to worry about migrating the existing pref. How do you feel about that, Shane?
Flags: needinfo?(mixedpuppy)

    
    

    
  
A single setting would be simpler IMO.  With two, there will always be that moment of "how do these interact together", whereas one (with strings) will always be clear.  I'm not going to block on this.
Flags: needinfo?(mixedpuppy)

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/214374/#review229930

While I would prefer a single pref I'm not going to block, webext side is fine.  r?dao after the tabbrowser related stuff is updated.

        Attachment #8944043 -
        Flags: review+

    
    

    
      
  

    
    

    
  
(In reply to Shane Caraveo (:mixedpuppy) from comment #114)
> 
> The settings are in toolkit and look available to android (didn't verify),
> so what happens with their use on android?  I assume nothing, it should at
> least be a doc note.

I don't have an Android device to test on (nor an emulator set up), but I too assume this wouldn't work on Android, so that should be documented in the MDN docs for this API. If someone can verify that that would be even better.

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/214374/#review230278

::: browser/base/content/tabbrowser.js:2917
(Diff revision 6)
>      }
>  
> -    // If we're opening a tab related to the an existing tab, move it
> +    // If "browser.tabs.insertAfterCurrent" is true or if we're opening
> +    // a tab related to an existing tab and
> +    // "browser.tabs.insertRelatedAfterCurrent" is true, move it
>      // to a position after that tab.

This is basically verbalizing the if statement's conditions, which doesn't seem very useful. I think we can trim this to just:

// Move the new tab after another tab if needed.

::: browser/base/content/tabbrowser.js:2920
(Diff revision 6)
> +    // a tab related to an existing tab and
> +    // "browser.tabs.insertRelatedAfterCurrent" is true, move it
>      // to a position after that tab.
> -    if (openerTab &&
> -        Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) {
> +    if (Services.prefs.getBoolPref("browser.tabs.insertAfterCurrent") ||
> +        (openerTab &&
> +         Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent"))) {

Since insertRelatedAfterCurrent defaults to true and insertAfterCurrent doesn't, this should normally be cheaper:

    if ((openerTab &&
         Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) ||
        Services.prefs.getBoolPref("browser.tabs.insertAfterCurrent")) {

::: browser/base/content/tabbrowser.js:2923
(Diff revision 6)
> -    if (openerTab &&
> -        Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) {
> +    if (Services.prefs.getBoolPref("browser.tabs.insertAfterCurrent") ||
> +        (openerTab &&
> +         Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent"))) {
> +
> +      let lastRelatedTab =
> +        openerTab ? this._lastRelatedTabMap.get(openerTab) : null;

let lastRelatedTab = openerTab && this._lastRelatedTabMap.get(openerTab);

::: browser/base/content/tabbrowser.js:2926
(Diff revision 6)
> +
> +      let lastRelatedTab =
> +        openerTab ? this._lastRelatedTabMap.get(openerTab) : null;
> +      let prevTabOfNewTab =
> +        lastRelatedTab || openerTab || this.mCurrentTab;
> +      let newTabPos = prevTabOfNewTab._tPos + 1;

just put this on one line:

let newTabPos = (lastRelatedTab || openerTab || this.mCurrentTab)._tPos + 1;

::: browser/base/content/tabbrowser.js:2936
(Diff revision 6)
>          lastRelatedTab.owner = null;
> -      else
> +      else if (openerTab)
>          t.owner = openerTab;
>        this.moveTabTo(t, newTabPos, true);
> +      if (openerTab)
>        this._lastRelatedTabMap.set(openerTab, t);

indentation

        Attachment #8944043 -
        Flags: review?(dao+bmo) → review+

    
    

    
      
  

    
    

    
  
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d138609abfd
Expose API to customize where new tabs open, r=dao,mixedpuppy

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0d138609abfd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 6 years ago

          status-firefox60:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

    
    

    
  
Comment on attachment 8944043 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/214374/#review230598

::: browser/base/content/tabbrowser.js:2929
(Diff revisions 6 - 7)
> -      else if (openerTab)
> +    else if (openerTab)
> -        t.owner = openerTab;
> +      t.owner = openerTab;
> -      this.moveTabTo(t, newTabPos, true);
> +    this.moveTabTo(t, newTabPos, true);
> -      if (openerTab)
> +    if (openerTab)
>        this._lastRelatedTabMap.set(openerTab, t);
> -    }
> +  }

You completely messed up the indentation here. :(

    
    

    
  
Regression

From mozillazine 

"enable
browser.bookmarks.openInTabClosesMenu
browser.tabs.insertAfterCurrent

open a bookmark folder
tabs are opened in reverse order

Just flip browser.tabs.insertAfterCurrent to false
and it is ok

But that pref is very handy and should not break browser.bookmarks.openInTabClosesMenu"
Flags: needinfo?(dao+bmo)

    
    

    
  
Please just file new bugs on regressions. Thanks!
Flags: needinfo?(dao+bmo)

    
    

    
  
Filed bug 1442679
Depends on: 1442679
Depends on: 1442703

    
  
Blocks: 1442843

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #128)
> Comment on attachment 8944043 [details]
> Bug 1344749 - Expose API to customize where new tabs open
> 
> https://reviewboard.mozilla.org/r/214374/#review230598
> 
> ::: browser/base/content/tabbrowser.js:2929
> (Diff revisions 6 - 7)
> > -      else if (openerTab)
> > +    else if (openerTab)
> > -        t.owner = openerTab;
> > +      t.owner = openerTab;
> > -      this.moveTabTo(t, newTabPos, true);
> > +    this.moveTabTo(t, newTabPos, true);
> > -      if (openerTab)
> > +    if (openerTab)
> >        this._lastRelatedTabMap.set(openerTab, t);
> > -    }
> > +  }
> 
> You completely messed up the indentation here. :(

Oops, sorry, I didn't see the problem and my editor and eslint didn't complain, so it slipped through.

    
    

    
  
I think we need to back this out for now. It has caused (at least) two regressions: bug 1442703 and bug 1442679, neither of which were covered by tests (because there were no test failures when landing this bug). 

Is it possible to just back out the commit, or does a reverse patch need to be written?
Flags: needinfo?(dao+bmo)

    
    

    
  
Okay, I'll take care of the backout.
Flags: needinfo?(dao+bmo)

    
    

    
  
Backed out the main commit and the followup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e894068a646008ead0b0cca454f0a9e8ccca0d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7bfe2b00511371afc39f85ff2bff37b695dac63
Status: RESOLVED → REOPENED

          status-firefox60:
          fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---

    
    

    
  
Thank you for taking care of the backout.

    
    

    
  
(In reply to Bob Silverberg [:bsilverberg] from comment #135)
> I think we need to back this out for now. It has caused (at least) two
> regressions: bug 1442703 and bug 1442679, neither of which were covered by
> tests (because there were no test failures when landing this bug). 

For bookmarks and restored tabs, the tabs open in reverse order because this._lastRelatedTabMap is not being modified.  Does it have to be that way? Would

this._lastRelatedTabMap.set(this.selectedTab, t)

work if there is no openerTab?

    
  
Assignee: bob.silverberg → mixedpuppy
Priority: P5 → P2

    
    

    
  
Session restore will restore tabs in the order they were.  However this patch now changes that ordering if there is not an openertab.  We need the tab creation to be aware that it is handling tab creation for session restore so it does not mess with the tab order during that stage.

    
    

    
      
  


  

    
  

        Attachment #8956994 -
        Flags: review?(mixedpuppy)

    
    

    
  
Have you audited our other addTab calls? I think it might be better to let openUILinkIn opt in to this behavior rather than letting SessionStore opt out. Not sure, though.

    
    

    
  
The only time I think this needs to be considered is during bulk-open operations where the order should be determined by that operation rather than by addTab.  Session restore is one, the only other one I can think of is synced tabs, but am unsure if order is important for that.  Based on quick look at how it works, I don't think it is.
Flags: needinfo?(dao+bmo)

    
    

    
  
I don't have a full understanding of the current discussion but can say I sent a tab from my mobile device while this code was landed and it arrived directly next to the currently selected tab (rather than at the end of a long row of tabs) which is exactly how I'd like it. Easy to discover.

    
    

    
  
(In reply to Shane Caraveo (:mixedpuppy) from comment #144)
> The only time I think this needs to be considered is during bulk-open
> operations where the order should be determined by that operation rather
> than by addTab.  Session restore is one, the only other one I can think of
> is synced tabs

Another bulk-open operation is opening a group of tabs by middle-clicking (or right-clicking, Open All in Tabs) a folder of bookmarks. Although I have no opinion what order that should produce.

Also, how does Undo Close Tab behave? (Expected behavior: the restored tab should reappear at the relative position where it was before it was closed, as much as that still makes sense, and not be affected by this option.)

    
    

    
      
  

    
    

    
  
(In reply to Yuri Khan from comment #146)

> Also, how does Undo Close Tab behave? 

It is placed where it was when closed.  I added tests for it.

    
  

        Attachment #8956994 -
        Flags: review?(mixedpuppy)

    
    

    
  
Comment on attachment 8956994 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/225956/#review235204

::: browser/base/content/tabbrowser.js:2214
(Diff revision 2)
>        aSkipBackgroundNotify = params.skipBackgroundNotify;
>        aNextTabParentId = params.nextTabParentId;
>        aNoInitialLabel = params.noInitialLabel;
>        aFocusUrlBar = params.focusUrlBar;
>        aName = params.name;
> +      aBrowserSetState = params.browserSetState;

I'm not a fan of this name, maybe you can come up with something better. bulkOperation?

::: browser/base/content/tabbrowser.js:2481
(Diff revision 2)
> -      else
> +        else
> -        t.owner = openerTab;
> +          t.owner = openerTab;
> -      this.moveTabTo(t, newTabPos, true);
> +        this.moveTabTo(t, newTabPos, true);
> -      this._lastRelatedTabMap.set(openerTab, t);
> +        this._lastRelatedTabMap.set(openerTab, t);
> +      } else if (Services.prefs.getBoolPref("browser.tabs.insertAfterCurrent")) {
> +        this.moveTabTo(t, this.mCurrentTab._tPos + 1, true);

This is a change from Bob's patch that will affect the tab-select behavior when closing a tab that was moved. Is this intentional?
Flags: needinfo?(dao+bmo)

    
    

    
  
(In reply to Dão Gottwald [::dao] from comment #149)
> Comment on attachment 8956994 [details]
> Bug 1344749 - Expose API to customize where new tabs open

> This is a change from Bob's patch that will affect the tab-select behavior
> when closing a tab that was moved. Is this intentional?

It took a moment for me to get that.  I guess I have to add tests for close behavior.

    
    

    
      
  

    
  

        Attachment #8956994 -
        Flags: review?(mixedpuppy)

    
    

    
  
added a couple close tab tests.  reverted changes.  merged to current code.
Flags: needinfo?(dao+bmo)

    
  
Blocks: 1446913

    
    

    
  
Comment on attachment 8956994 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/225956/#review236130

::: browser/base/content/tabbrowser.js:2459
(Diff revision 3)
>  
> -    // 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")) {
> +    // Move the new tab after another tab if needed.
> +    if (!aBulkOrderedOpen &&
> +      (((openerTab &&
> +         Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) ||
> +         Services.prefs.getBoolPref("browser.tabs.insertAfterCurrent")))) {

Too many parens and please fix the indentation...

    
    

    
  
Comment on attachment 8956994 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/225956/#review236138

::: browser/base/content/test/tabs/browser_new_tab_insert_position.js:34
(Diff revision 3)
> +  // that the active tab is loaded and restored.
> +  let promise = Promise.all([
> +    TestUtils.topicObserved("sessionstore-closed-objects-changed"),
> +    BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "SSTabRestored")
> +  ]);
> +  await BrowserTestUtils.removeTab(tab);

BrowserTestUtils.removeTab isn't async.

    
    

    
      
  

    
    

    
  
Comment on attachment 8956994 [details]
Bug 1344749 - Expose API to customize where new tabs open

https://reviewboard.mozilla.org/r/225956/#review236372

::: browser/base/content/tabbrowser.js:2452
(Diff revision 4)
>      }
>  
> -    // 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")) {
> +    // Move the new tab after another tab if needed.
> +    if (!aBulkOrderedOpen && ((openerTab &&
> +        Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) ||
> +        Services.prefs.getBoolPref("browser.tabs.insertAfterCurrent"))) {

Not sure if there's a style guide for this, but to make it easier to see the grouping of the conditions I'd prefer something like this:

    if (!aBulkOrderedOpen &&
        ((openerTab &&
          Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) ||
         Services.prefs.getBoolPref("browser.tabs.insertAfterCurrent"))) {

        Attachment #8956994 -
        Flags: review?(dao+bmo) → review+

    
  

        Attachment #8956994 -
        Flags: review?(mixedpuppy)

    
    

    
      
  

    
  

        Attachment #8956994 -
        Flags: review?(mixedpuppy)

    
    

    
  
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/aee255d8336c
Expose API to customize where new tabs open, r=dao

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/aee255d8336c
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago

          status-firefox61:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

    
    

    
  
Got pretty excited when I saw this landed, but then I restarted to update and my tabs had all shuffled again. :(

    
    

    
  
(In reply to Caspy7 from comment #160)
> Got pretty excited when I saw this landed, but then I restarted to update
> and my tabs had all shuffled again. :(

Session restore shouldn't be causing a problem here now.  I'd need more information to try and replicate what you ran into.
Flags: needinfo?(dao+bmo)

    
    

    
  
(In reply to Shane Caraveo (:mixedpuppy) from comment #161)
> I'd need more information

I, too, can reliably reproduce tab ordering issues with session restore on a new profile of 61.0a1 (2018-03-28), the only change I did being `browser.tabs.insertAfterCurrent;true`.

STR:
1. Open a bunch of orphan tabs: say Google searches for `tab1`, `tab2`... up to, say, `tab6`. Now the *n*th tab is a Google search for `tab*n*`.

2a. Switch to `tab4`, OR
2b. Switch to `tab6`, OR
2c. Switch to `tab1`

3. Press shift+F2 for Developer Toolbar, type `restart`

AR:
The new order of tabs is:

For STR 2a) tab3 tab2 tab1 tab4 tab6 tab5

For STR 2b) tab5 tab4 tab3 tab2 tab1 tab6

For STR 2c) tab1 tab6 tab5 tab4 tab3 tab2

So while the last active tab before a restart is restored at its expected position, all the tabs to its left, if any, and those to its right, if any, are reversed.

    
    

    
  
requested a backout on this until I have time to address the issues.

    
    

    
  
Shane, I've tried to back this out and got the following error:

checking for uncommitted changes
backing out aee255d8336c
applying patch from stdin
unable to find 'toolkit/components/extensions/ext-browserSettings.js' for patching
(use '--prefix' to apply patch relative to the current directory)
2 out of 2 hunks FAILED -- saving rejects to file toolkit/components/extensions/ext-browserSettings.js.rej
patching file toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
Hunk #1 FAILED at 47
1 out of 2 hunks FAILED -- saving rejects to file toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js.rej
abort: patch failed to apply

Please take a look. Thank you.
Flags: needinfo?(mixedpuppy)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1344749-followup
        — 
          — Splinter Review
      

    


  
Relying on this._browserSetState was incorrect as that is only set via tests.  It needs to be always true so session restore works in the right order.

The lazy restore is fine in the test, and avoids the about:blank intermittent (or at least makes it much much harder to reproduce on my linux vm).
Flags: needinfo?(mixedpuppy)

        Attachment #8964410 -
        Flags: review?(dao+bmo)

        Attachment #8964410 -
        Flags: review?(dao+bmo) → review+

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/864320860e94d0dd3a8f05976601ed6a7dc5a79d
Bug 1344749 followup fix for session restore when using insertAfterCurrent and intermittent test failure, r=dao

    
    

    
  
Bug 1442679 was marked as fixed due to the backout, but this new patch doesn't fix it.

    
    

    
  
I have tried to reproduce the scenario mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1344749#c162 with the pref set to False as the default and was unable to reproduce the issue on Firefox 59.0a1 (20180101220336). Can you please provide steps or a webext in order to test and verify this improvement?
Flags: needinfo?(mixedpuppy)

    
    

    
  
covered by tests.
Flags: needinfo?(mixedpuppy) → qe-verify-

    
  
Depends on: 1454873

    
    

    
  
Changing to QE+ based on some etherpad comments by Shane: 

    There were a couple followup bugs about session restore and bulk opening bookmarks.  

    Those were fixed.  I'm concerned there are unknown issues lurking.  

    It might be useful to have some QA running with this pref fliped for a while.
Flags: qe-verify- → qe-verify+
Keywords: dev-doc-needed

    
    

    
  
I have tested using https://bugzilla.mozilla.org/show_bug.cgi?id=1344749#c169 in Firefox 59.0a1 (20180101220336) and was unable to find the bug or reproduce what was stated in https://bugzilla.mozilla.org/show_bug.cgi?id=1344749#c162. Please provide steps or an extension to reproduce the issue and test around what was fixed. Also please link the session restore and bulk opening bookmark bugs.
Flags: needinfo?(mixedpuppy)

    
    

    
  
(In reply to marius.santa from comment #173)
> I have tested using
> https://bugzilla.mozilla.org/show_bug.cgi?id=1344749#c169 in Firefox 59.0a1
> (20180101220336)

This is a new feature in Fx 61.
Flags: needinfo?(mixedpuppy)

    
    

    
  


  
I've tested tab opening scenarios with the pref set to true using https://addons.mozilla.org/en-US/firefox/addon/tab-open-close-control/?src=search.
Also tested bookmark opening scenarios.
Tested end verified in Firefox 62.0a1 (20180524100118)on Windows 10 64Bit and MacOS 10.13.3.

    
  
Status: RESOLVED → VERIFIED

          status-firefox61:
          fixed → verified

          status-firefox62:
          --- → verified

    
  
Product: Toolkit → WebExtensions

    
    

    
  
Docs page: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/newTabPosition

Browser compat data is not merged yet: https://github.com/mdn/browser-compat-data/pull/2431

Please let me know if we need anything else here.
Flags: needinfo?(mixedpuppy)

    
    

    
  
lgtm
Flags: needinfo?(mixedpuppy)
Depends on: 1485683

    
  
No longer blocks: 1226546

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: