1387372 - tab.onCreated evaluation is too late to modify index

Status: RESOLVED DUPLICATE of bug 1344749

(WebExtensions :: General, enhancement, P5)

Description

[Dietrich Ayala (:dietrich)]

I'm attempting to migrate AlwaysRight add-on to WebExtension.

The add-on puts all new tabs immediately to the right of the active tab, including new blank tabs.

The idea in psuedo-code:

tabs.onCreated(tab) {
  tab.index = activeTab.index + 1
}

Super simple, right?

The SDK add-on works as expected.

However, the WebExtension version visually adds the tab to the end of the tab strip and then visually moves it to the right of the active tab.

The experience is incredibly visually jarring.

Can we evaluate onCreated event listeners after the tab element is created and prior to DOM insertion?

Comment 1

[Shane Caraveo (:mixedpuppy)]

I didn't see any issue on osx with remote=true.  Any further reproduction notes?

Actual code:

var activeTab;
browser.tabs.query({active: true}).then(tabs => {
  activeTab = tabs[0];
});
browser.tabs.onCreated.addListener(tab => {
  browser.tabs.move(tab.id, {index: activeTab.index + 1});
});
browser.tabs.onActivated.addListener(info => {
  browser.tabs.get(info.tabId).then(tab => {
    activeTab = tab;
  });
});

Comment 2

[Dietrich Ayala (:dietrich)]

That's my exact code too (except for things like checking if index is same and no-op'ing in that case, and checking window is same).

I realized that it's probably because I have ~300 tabs. You maybe don't have enough tabs to show the problem.

Comment 3

[Shane Caraveo (:mixedpuppy)]

I think you need an addon that closes the oldest tabs after more than 50 are open ;)

Comment 4

[Dietrich Ayala (:dietrich)]

Hey now ;)

What is the correct behavior? If it's to modify prior to DOM insertion, we should do that.

This could also show up if with small numbers of tabs in other conditions.

My concern is about how much the experience of Firefox deteriorates due to unexpected behavior from this race condition, as people really lean into making WebExtensions.

We probably want to have the correct behavior in 57.

Comment 5

[Shane Caraveo (:mixedpuppy)]

I dug into code a bit.  onCreated is called in two instances.  The first is the TabOpen event as I mentioned earlier.  The second is from the load event of a browser window, in which case onCreated is called for each tab in that window.  TabOpen it notified from tabbrowser addTab, after the tab is inserted into the document.  Basically, there is no way to synchronously handle this from webextensions, so you'll get those ui issues.  

Tabbrowser would need to support some mechanism to do this, a couple ideas:
- a pref that would be set via webextension to "always insert right of selected tab".  easy but limited.
- a new event called prior to addTab that could be used by webextensions.  It would need to support an async response.  Then we could add browser.tabs.onBeforeCreated.
- a defaults lookup mechanism that could affect how tabs are created/inserted.  A webextension API could simply define a settings object rather than being called for each tab open.  A background script could call something like browser.tabs.setBehaviors({}).

Comment 6

[sblask]

I'd vote for a preference. I only created my extension (https://addons.mozilla.org/en-US/firefox/addon/open-tabs-next-to-current/) because of the lack of such a preference. This is really basic and each browser should allow the user to configure where tabs are opened and in which order tabs get activated (see https://addons.mozilla.org/en-US/firefox/addon/tab-deque/)

Comment 7

[Caitlin Neiman [:caitmuenster]]

Hi Dietrich! This has been added to the agenda for the August 22 WebExtensions APIs triage meeting. I remember these meetings are at an inconvenient time for you :(, but you and anyone else interested in this bug are invited to join the meeting on Vidyo or IRC and are welcome to leave comments for folks in the meeting to take into account. 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage

Agenda: https://docs.google.com/document/d/11SdY-aRhvPU3SvH8jpj0covj3Teq9_GJl8wMeEeSVwo/edit#

Comment 8

[Dietrich Ayala (:dietrich)]

Thanks Caitlin!

Any of Shane's approaches in comment #5 will do the job.

I recommend prior to implementation that y'all hook up with Shorlander, who might be revisiting default tab-opening behavior, from what I saw in the UX channel on Slack.

Or maybe he can just fill us in on any plans here, so NI?ing him.

Comment 9

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> I dug into code a bit.  onCreated is called in two instances.  The first is
> the TabOpen event as I mentioned earlier.  The second is from the load event
> of a browser window, in which case onCreated is called for each tab in that
> window.  TabOpen it notified from tabbrowser addTab, after the tab is
> inserted into the document.  Basically, there is no way to synchronously
> handle this from webextensions, so you'll get those ui issues.  
> 
> Tabbrowser would need to support some mechanism to do this, a couple ideas:
> - a pref that would be set via webextension to "always insert right of
> selected tab".  easy but limited.
> - a new event called prior to addTab that could be used by webextensions. 
> It would need to support an async response.  Then we could add
> browser.tabs.onBeforeCreated.
> - a defaults lookup mechanism that could affect how tabs are
> created/inserted.  A webextension API could simply define a settings object
> rather than being called for each tab open.  A background script could call
> something like browser.tabs.setBehaviors({}).

Maybe I'm missing the point here, but IIUC the main goal is to open a new tab next to current, correct?

addTab has property relatedToCurrent, which if `true`, will accomplish just that (I have been using it in legacy All Tabs Helper to do this for years).  Can we not just expose a new property tabs.create({relatedToCurrent: true}) which would get passed to the addTab call ??  This would certainly fill my needs.

The problem with moving the tab after the tab has been created (If there are many tabs and and conditions are right) is a disturbing UX experience.  This is not due to race conditions, but to the expected behavior of tabbrowser.[1]  addTab(url, {relatedToCurrent: true}) deals with this internally by not updating the UI until __after__ the tab has been moved.

Maybe I am not understanding the discussion correctly or the needs, but to me it all sounds more complicated than it needs to be.

[1] bug 1344749 comment 5 gives a detailed description of the chain of events creating the unexpected UX (though expected behavior from an API POV) of first creating a tab, then moving it.

Comment 10

[u462496]

(In reply to Kevin Jones from comment #9)
> [1] bug 1344749 comment 5 gives a detailed description of the chain of
> events creating the unexpected UX (though expected behavior from an API POV)
> of first creating a tab, then moving it.

Specifically, paragraph 5.

Comment 11

[sblask]

relatedToCurrent won't do the trick I think unless it's something different from the browser.tabs.insertRelatedAfterCurrent setting in about config. If I enable browser.tabs.insertRelatedAfterCurrent tabs are opened at the end of related tabs, not next to the current. So if I have tab A and open B C D E from it, the resulting order is A B C D E whereas if tabs are always opened next to current, the order would be A E D C B.

Comment 12

[u462496]

(In reply to sblask from comment #11)
> relatedToCurrent won't do the trick I think unless it's something different
> from the browser.tabs.insertRelatedAfterCurrent setting in about config. If
> I enable browser.tabs.insertRelatedAfterCurrent tabs are opened at the end
> of related tabs, not next to the current. So if I have tab A and open B C D
> E from it, the resulting order is A B C D E whereas if tabs are always
> opened next to current, the order would be A E D C B.

Yes, you are correct.  If you did:

gBrowser.addTab("A", {relatedToCurrent: true});
gBrowser.addTab("B", {relatedToCurrent: true});
gBrowser.addTab("C", {relatedToCurrent: true});

the order would end up being C B A

So maybe I am indeed missing the point of this discussion.  Apparently you are talking about opening tabs in the background such as when ctrl/cmd + clicking on a link.

I am talking about when a user opens a new tab, say using cmd/ctrl + T, where the usual behavior is to open a tab, then switch focus to that tab.

Comment 13

[sblask]

You are right, relatedToCurrent would the job for new empty tabs, but not for tabs opened from links (I missed that bit...)

Comment 14

[Dietrich Ayala (:dietrich)]

Sorry for late reply to the ni?.

As y'all have discussed above, the relatedToCurrent option addresses one specific scenario, and operates under a set of assumptions built into the core browser.

For my purposes, I need *all* new tabs from any source (links, external app, background, whatever) to open exactly where my extension specifies.

Does that help to clarify?

Comment 15

[Dietrich Ayala (:dietrich)]

To clarify a bit further, in relation to this specific bug:

> For my purposes, I need *all* new tabs from any source
> (links, external app, background, whatever) to open
> exactly where my extension specifies.

I can actually do this now. However, I can only do it *after* the core browser logic has executed and visually placed the tab in the tab strip.

The proper fix for this bug is likely to provide a way to override the internal tab placement logic, to avoid the negative visual side-effects of moving the tab afterwards.

Maybe this means modifying the event handler timing, or maybe it means building proper hooks into core so it doesn't get futurebroken.

Comment 16

[u462496]

(In reply to Dietrich Ayala (:dietrich) from comment #14)
> Does that help to clarify?

Yes, thank you.

Comment 17

[Croydon]

This causes even more problems down the road when an add-on like AlwaysRight is combined with a vertical tabs solution. Like scrolling to the new tab, which is still moved around **somewhen**, completely unpredictable.

Comment 18

[Stephen Horlander [:shorlander]]

(In reply to Dietrich Ayala (:dietrich) from comment #8)
> Thanks Caitlin!
> 
> Any of Shane's approaches in comment #5 will do the job.
> 
> I recommend prior to implementation that y'all hook up with Shorlander, who
> might be revisiting default tab-opening behavior, from what I saw in the UX
> channel on Slack.
> 
> Or maybe he can just fill us in on any plans here, so NI?ing him.

Hi! We have discussed in the past making changes to tab opening behavior. But we don't have any concrete plans in the near term.

Whether it is a pref or not seems largely up to how much control we want to give add-ons over where a tab should open.

Comment 19

[Shane Caraveo (:mixedpuppy)]

The original purpose of this bug is to open tabs to the right of the activetab. As such, am duping to bug 1344749 which will hopefully land for 60.