1419947 - Extend tabbrowser to track and use tab successors

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P5)

Description

[Ben Lerner]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171115095126

Steps to reproduce:

See https://github.com/jingyu9575/select-after-closing-current/issues/3 for additional details.  Firefox currently hard-codes the activation order when a tab is closed: it always goes to the right.  Extensions like LastTab fixed this by patching the browser code, and Select-after-closing-current can sorta fix this but it's now limited to the browser.tabs API.  That API triggers the onRemoved notification after Firefox activates another tab, so the extension is forced to switch the active tab after some other tab is briefly activated.  That brief activation is enough to change the lastAccessed time of the tab, which affects the Ctrl+Tab order.



Expected results:

I'd like to see any of the following happen:

* Enhance Firefox to allow customizing the tab-access order -- most robust, but most work for Firefox
* Give extensions a way to intercede on "which tab should be activated next?", perhaps with an "onBeforeDetached" even or something that could activate the next desired tab before the default Firefox behavior kicks in -- this is pretty robust, and might even be a way for Firefox itself to choose which tab to activate next
* Give extensions a way to reset the lastAccessed time of a tab, perhaps like `browser.tabs.update(tabId, {lastAccessed: ...five minutes ago...})` -- this is least robust, since it requires querying all tabs to get their lastAccessed time and maintaining that separately from Firefox, but it might be easiest to implement.

Comment 1

[Caitlin Neiman [:caitmuenster]]

Hi Ben, this has been added to the agenda for the December 19, 2017 WebExtensions APIs triage. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* 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/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/1KwfTum8Ow5w4afPAOvShpu_d_MNtahhOIqL3-Em9lLc/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

Comment 2

[Mike Conca [:mconca]]

Policy Review Comments (https://wiki.mozilla.org/WebExtensions/policy)

This request does not appear to run counter to any of the WebExtension policies. It also pertains to tab management and organization, which is currently a high priority on the WebExtensions roadmap.

I would be interested to know the scope and supportability of any of the proposed solutions. There is some user benefit, but the level of investment needs to be appropriate.

Comment 3

[Andrew Swan [:aswan]]

We discussed this briefly yesterday and nobody expressed concerns about letting extensions do this in principle, but there was quite a bit of concern about how this could be done in a maintainable and performant way.
I'm starting from the assumption that a callout to an extension (which entails IPC to the extension process) during tab cycling would be a non-starter for reasons of performance and complexity (e.g., how long do we wait for a response from the extension, what if another UI event that affects the selected tab happens before we get a response, etc).
What are some other options here?  Ben outlines some options in comment 0, another possibility would be to create an API to allow extensions to set the order for cycling -- an extension would have to set the order before the user presses ctrl-tab but if extensions set the order based on tabs being created, destroyed, or moved, that should be relatively infrequent.
We could use some input from somebody who knows this code well, what do you say Dao?

Comment 4

[Ben Lerner]

I had a very similar thought yesterday evening :)  On the assumption that after any switching of tabs, a user will stay on that newly-selected tab for at least a few milliseconds, this proposed API to advise a *subsequent* tab-switching event is well off the critical performance path of "switch to a new tab asap".

Comment 5

[Dão Gottwald [:dao]]

I think we should start by asking what we expect extensions to do with this, lest we don't overengineer. If the "when closing a tab, always select the previously selected one" use case is basically the only motivation here, then I think we could just support that natively with a pref.

Comment 6

[Will Elwood (:Will)]

This is potentially a duplicate of bug 1422509 since that is concerned with implementing the 2nd bullet point in the top comment here, and hence with the desired tabs being properly selected on close the existing Ctrl+Tab behaviour would become useful again.

If not a duplicate, that bug should at least be referenced here to ensure all the use cases for which next tab to select on closing are visible.

Comment 7

[Mike Conca [:mconca]]

(In reply to Andrew Swan [:aswan] from comment #3)
> We discussed this briefly yesterday and nobody expressed concerns about
> letting extensions do this in principle, but there was quite a bit of
> concern about how this could be done in a maintainable and performant way.

I'm adding the [needs-follow-up] tag based on the above comment. That is, in the abstract, there are no objections to some type of API to do this. This does *not* mean it will be implemented. There are still serious engineering concerns that need to be addressed.

Comment 8

[Andrew Swan [:aswan]]

Ben, does the suggestion in comment 4 meet your needs?

Comment 9

[Ben Lerner]

It should, yes.  I'm imagining that the extension I mentioned above can do something like

    function onSwitchTabs() {
      browser.tabs.query({currentWindow: true}).then((tabs) => {
        tabs.sort((t1, t2) => t2.lastAccessed - t1.lastAccessed); // sort into MRU order
        browser.tabs.NEW_API_TO_SET_TAB_ORDER(tabs); // or possible tabs.map((t) => t.id)
      });
    }
    browser.tabs.onActivated.addListener(onSwitchTabs);
    browser.tabs.onMoved.addListener(onSwitchTabs);
    browser.tabs.onDetached.addListener(onSwitchTabs);
    browser.tabs.onRemoved.addListener(onSwitchTabs);
    browser.tabs.onUpdated.addListener(onSwitchTabs);

Assuming the `query` promise resolves quickly enough, this should be enough for the extension to easily keep up with any changes in tab activation, without adding any performance impact to the critical path of "Firefox must respond to a Ctrl+Tab press".

It's worth asking, in the code sketch above, what to do if the array of tabs passed in to the NEW_API doesn't contain all the tabs in the window?  My inclination is to say "I'm specifying these tabs, so drag them to the front of the ctrl+tab order, and move everything else behind them."  Likewise, if any tabs aren't even valid in the current window, just discard them.  (Obviously there are other engineering solutions here, from adding another parameter to the API to specify "frontness" or "backness", or throwing errors instead of discarding invalid data, or whatever.  I'm less concerned about that, and whatever you think fits best into existing Firefox code is fine.)

Does that help?

Comment 10

[Andrew Swan [:aswan]]

Whoops, I mistyped the comment number.  I meant to ask if the suggestion in comment 5 (a built-in feature for the behavior when closing a tab) be adequate?

Comment 11

[Ben Lerner]

Well, it would suffice for my use case, yes. But iirc, LastTab supported additional tab ordering policies besides just "most recently used", and I'm trying to figure out the minimal API that would allow them to work again. Purely selfishly, yes, baking this one policy into Firefox would be ok for me, if that's as far as seems feasible right now.

Comment 13

[Andrew Swan [:aswan]]

But 1419649 covers adding this as a built-in feature.

Comment 14

[Ben Lerner]

Just pointing out here, since that one's now closed: bug 1422509 points out several other policies beyond just MRU.  So apropos comment 5, "most recently used" is not the only motivation people (besides me) have.

Comment 15

[sriram]

For myself, the options in the "select-after-closing-current" extension are very useful.

Right now I have it set to select:

1) The left/right child tab
2) The parent tab
3) The left/right sibling tab
4) The right tab

Being able to select the child/sibling/parent tabs (which, I believe, are new to versions 57+) are even better than the old options in TabMixPlus. Perhaps not many people know about them yet...

Comment 16

[Yuri Khan]

(In reply to Ben Lerner from comment #14)
> Just pointing out here, since that one's now closed: bug 1422509 points out
> several other policies beyond just MRU.  So apropos comment 5, "most
> recently used" is not the only motivation people (besides me) have.

As the opener of that bug, I’d like to say that my personal favored policy is “focus the tab on the left if any, or the first tab”.

Implementing this feature request as stated in the title (consistency with Ctrl+Tab) will not in any way solve my needs or needs of anyone who wanted a different policy listed in Bug 1422509.

Comment 17

[primexx]

This is a critical issue and severely hampers the ability of addons. Looking at the bug tracker for Select-After-Closing-Current, basically all of the problems that it has are because of this bug. And the dev can't do anything to fix it. Please allow more robust tab management either natively or just expose better control to addons!

Comment 18

[larabe]

Any news on the development of this? comment15 said how the request would be useful at least for most people if it were to be implemented as an API or as an option in firefox.

Comment 19

[Andrew Swan [:aswan]]

See comment 7.  Also this is marked as P5, which means it is not a priority for any Mozilla employees.
If anybody in the community is interested in working on it, we'll be happy to work with them however.

Comment 20

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Work with me!

I've written a working (I've been using it for a week!) WebExtension Experiment that adds an API for managing tab order. I tried to take all of the commentary in this thread into account to create an API that is flexible enough for most use cases, doesn't require any IPC when a tab is closed, and has a small enough surface area not to be considered overengineered. The code is available here: <https://gitlab.com/rhendric/successor-tabs-experiment>, and a built XPI (only temporarily installable through about:debugging) is available here: <https://gitlab.com/rhendric/successor-tabs-experiment/uploads/558a5a5d186fe67c3e0e2324be19d852/successor-tabs.xpi>.

The design this experiment implements involves adding a successor tab field to tab objects in the parent process (in my experiment, this is implemented with a WeakMap instead of a literal field). API functions are made available to manage the successors of tabs, and Firefox is altered to activate the successor tab when an active tab is closed, falling back to its current heuristic if no successor tab has been assigned. Additionally, Firefox will splice lines of succession together when a tab is closed: if tab A's successor is tab B, and B's successor is C, when B is closed (active or not), Firefox sets A's successor to C instead. The same thing happens when a tab is moved from a window; succession is not allowed to cross window boundaries (and the API enforces this as well).

Here are the API modifications I propose:

* Add a previousTabId field to the tabs.onActivated event. This field contains the ID of the previously active tab if that tab is still open, or null if that tab is being closed.

* New method tabs.getSuccessor(tabId: integer): Promise<integer>. Returns the ID of the successor tab of the specified tab, or tabs.TAB_ID_NONE if said tab has no successor set.

* New method tabs.setSuccessor(tabId: integer, successorTabId: integer): Promise. If successorTabId is tabs.TAB_ID_NONE, this clears the successor instead. Will throw (in the promise) an error if either tabId is not the ID of an open tab, or if the two tabs are in different windows, or if tabId equals successorTabId (a tab can never be its own successor).

* New method tabs.moveInSuccession(tabIds: Array<integer>, beforeTabId: integer?, insert: boolean?): Promise. Removes an array of tabs from their lines of succession and prepends them in a chain to another tab. For each tab referenced by tabIds, the tab's current predecessors will have their successor set to the tab's current successor, and each tab will then be set to be the successor of the previous tab in the array. Any tabs not in the same window as beforeTab (or the first tab in the array, if no beforeTab) will be skipped. beforeTabId is optional, and defaults to tabs.TAB_ID_NONE (which leaves the last tab in the chain without a successor). insert is also optional, and if true, sets the successor of any current predecessors of the beforeTab to be the first tab in the array, effectively splicing the entire chain into beforeTab's current line of succession.

This sounds more complicated than it is. To get your intuition started, this function is useful for implementing MRU-like succession behavior without a lot of get/setSuccessor IPC calls; any time a tab is activated, if previousTabId is not null, you can call tabs.moveInSuccession([tabId], previousTabId), and that's it—no other calls are necessary. In such an implementation, the successorTab fields end up forming a linked list, and moveInSuccession does what's necessary to prepend to or move items within the list. Of course, this method also supports more sophisticated uses, and successor tabs in general don't have to be a simple linked list; they can form a directed graph with branches and even cycles (but no self-edges).

* New method tabs.moveinSuccessionAfter(afterTabId: integer, tabIds: Array<integer>, insert: boolean?): Promise. Like the previous, removes an array of tabs from their lines of succession, but this method appends them in a chain to another tab. For each tab referenced by tabIds, the tab's current predecessors will again have their successor set to the tab's current successor, and again each tab will then be set to the successor of the previous tab in the array. The first tab in the array will then be set as the successor of afterTab, which is required, but afterTab will not otherwise be removed from its line of succession. Any tabs in tabIds not in the same window as afterTab will be skipped. insert is again optional, and if true, sets the successor of the last tab in the array to the current successor of the afterTab.


Some notes on my extension: I implemented a behavior using this API that activates the most recently used tab, unless the active tab being closed was opened along with any other still unread tabs, in which case the next unread tab in that ‘reading list’ is activated instead. (This is my ideal tab closing behavior.) This is complex enough to show off both moveInSuccession and moveInSuccessionAfter, but only requires at most one such call per tab event (onCreated, onActivated, etc.). getSuccessor and setSuccessor turned out not to be necessary once I had the power of moveInSuccession/After, but I left them in the proposed API because they still seem like a good idea, in case some extension developer wants lower-level access. To implement the changes on the Firefox side, I had to do some ugly monkey patching which would of course be unnecessary if this API is uplifted into Firefox proper; with that in mind, most of the code in src/api/parent.js would be removed in such an event. The code that would need to be kept to implement the API is mostly in src/api/SuccessorTabsModel.js, which is in a separate file so that I could write unit tests for it. Finally, because I didn't find a way to extend the existing tabs API namespace, I created one called successorTabs and, in my WebExtension code, cheat by merging that into tabs; it is of course my hope that these new methods would live in the tabs namespace and not their own.


Feedback please! What would I need to do to get this on the uplift track?

Comment 21

[Caitlin Neiman [:caitmuenster]]

Thank you, Ryan! 

Ddurst, could you advise on next steps for getting this patch reviewed?

Comment 22

[Andrew Swan [:aswan]]

Ryan, thanks for working on this.

There are really two big components to this, one is the tabbrowser changes, the other is the webextension api.  I think the place to start for feedback is the tabbrowser changes.  The most effective way to get those reviewed would be to format them as a mozilla-central patch (as opposed to the Patcher object your extension uses), file a new bug on Firefox:Tabbed Browser, and attach the patch there and ask for review from somebody like :dao.

Comment 23

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Attachment: Bug 1419947 - Extend tabbrowser to track and use tab successors; r?dao

The successor of a tab is similar to the owner of a tab, in that if an active
tab is closed, the successor will be activated next. The differences are that
successor always defaults to being unset (an extension can set it through a
not-yet-implemented API), whereas tabbrowser sets owner to be the opener of
background tabs; successor is always consulted when an active tab is closed,
whereas the use of owner is gated by a pref; and when a tab is closed or moved
to another window, any tabs whose successor is the closed or moved tab inherit
that tab's successor, whereas in the parallel situation with owners, the owner
of such tabs is simply set to null.

Comment 24

[Dão Gottwald [:dao]]

This patch looks like a good start. I'll need to take a closer look later. The only initial reservation I have is introducing the "successor" concept in light of bug 1455264, but it wouldn't be fair to reject this patch because of that.

Comment 25

[Dão Gottwald [:dao]]

Added a few comments to the patch in phabricator.

Comment 26

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

I am not at all sure I'm using Phabricator correctly, but I updated the patch to address your feedback.

Comment 27

[Dão Gottwald [:dao]]

Ryan, sorry for the delay. I've accepted your patch on phabricator with only two small change requests. Can you make these changes? Otherwise I could do that and land the patch myself.

I've also submitted your patch to the try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a7e394816efed2f5ab3d2cc5b07656241a059af

Comment 28

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Changes made!

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15fd39e49766
Extend tabbrowser to track and use tab successors; r=dao

Comment 30

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Andrew, sorry to bother you, but I'm unfamiliar with the machinery here and I'd like to start the review for the WebExtension API half of this work. Should I be waiting for something further to get the first patch into mozilla-central? Or should I submit the second patch here in Bugzilla for initial discussion? (It'll need some feedback; I have some questions about how to write tests and feature parity across desktop and mobile.)

Comment 31

[Rob Wu [:robwu]]

Hi Ryan,

You could already upload the patch to Phabricator for review/discussion.
There is relatively new documentation about getting started with testing and submitting patches; if you haven't seen it before I suggest to check out: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

If you want to split the patches in multiple commits, then the suggested arc tool doesn't easily support that. Use moz-phab instead (https://github.com/mozilla-conduit/review). Or if you use git instead of hg, phlay - https://github.com/mystor/phlay

PS. I added the leave-open keyword to prevent the bug from being closed automatically due to the landed patch above.

Comment 32

[Dão Gottwald [:dao]]

Please file a followup bug for the WebExtension API so that we can properly keep track of patches landing separately (and likely even ending up in different release versions at this point).

Comment 33

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Sorry, I think I misread the instructions to create a new bug earlier. I just created bug 1500479 for the WebExtensions enhancements; WIP patches are there.

Comment 34

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/15fd39e49766