1238314 - Implement browser.tabs opener functionality

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P2)

Description

[Kris Maglione [:kmag]]

This includes the `openerTabId` properties of the `tabs.create` and `tabs.update` methods, and of Tab objects.

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1238314: [webext] Implement tabs API `openerTabId` properties. r?billm

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

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8722260 [details]
MozReview Request: Bug 1238314: [webext] Implement tabs API `openerTabId` properties. r?billm

https://reviewboard.mozilla.org/r/35935/#review33025

::: browser/components/extensions/ext-utils.js:589
(Diff revision 1)
> +    let opener = tab.owner || this._tabOpeners.get(tab);

I don't think the owner property is a reliable way to do this. See here for example:
http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1824

I'm also unsure if the owner is even set if a page does window.open. I don't think it is.

This bug could require some platform support. needinfo me if you need help with the window opening APIs. e10s is the hard case. Here's where the logic is:
http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#5507

Comment 3

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/35935/#review33025

> I don't think the owner property is a reliable way to do this. See here for example:
> http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1824
> 
> I'm also unsure if the owner is even set if a page does window.open. I don't think it is.
> 
> This bug could require some platform support. needinfo me if you need help with the window opening APIs. e10s is the hard case. Here's where the logic is:
> http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#5507

Aside from it being cleared after an unrelated tab is opened, I think `owner` serves exactly the same purpose as `opener` does in Chrome.

As for `window.open`, it sets `owner` to whatever the currently-selected tab is, whether or not it's the actual opener. That should probably be fixed, but I think it's a separate bug.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I just don't think the owner property of <tab>s was designed for this purpose. It's a property that serves a very specific purpose in the Firefox UI. Here's another issue: switching tabs nulls out the owner:
http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1041
(I think the selected condition is only true if we're switching to the same tab as the current one.)

In addition, if a window.opened tab is opened in the background (controlled by a pref), then the owner will always be null.

Comment 5

[Kris Maglione [:kmag]]

You have some good points, but I think that either way we need to map this to the owner at some point.

When opening a tab, especially when it's related to the current tab, setting the owner to the opener gets us the expected tab placement behavior, which is the thing that I'm most concerned about right now.

And when the owner is set when a tab is first opened, it's always set to what we'd consider the owner in the Chrome API (ignoring the bugs relating to background tabs).

I suppose we probably shouldn't be setting the owner property on `tabs.update`, since that doesn't result in any useful documented behavior, from the add-ons perspective.

I think the best solution is probably to keep using aOwner, but try to make the behavior more consistent, and probably store it in a separate 'opener' property even in cases where we wouldn't set .owner.

Comment 9

[YUKI "Piro" Hiroshi]

Any progress? This feature is quite required for addons which open new tabs related to the current page. Currently there is no way to open new tab related to the current tab in some cases.

For example: a context menu command to open tabs from selection.
https://addons.mozilla.org/firefox/addon/text-link/
Currently there is only one way to open a related tab: window.open() in a content script. But it is blocked for context menu commands, because extra context menu items must be defined in a background script. (To open a new tab from context menu, chrome.tabs.create() is not useful because the new tab is always opened at the end of the tab bar, even if it is quite related to the current tab. Thus I thought to use window.open() instead, and I sent a message from the background script to a content script via chrome.tabs.sendMessaeg(). Then window.open() in the script is blocked because it is not triggered by any user action in the content area.)

Comment 11

[brunoais]

After reading the whole bug I still don't get it. What is preventing this from being made? Is it the difficulty of finding what the "current tab" is before opening the new tab?

Comment 13

[zmstefan]

Any plans on fixing this? As in WebExtensions documentation it says openerTabId property is supported in Firefox?

Comment 14

[Philippe Masset]

Just voted for this bug too, since it's one thing we're missing for API parity when porting a Chrome extension to WebExt.

(Additionally and quite surprisingly, instead of `openerTabId` being simply ignored and triggering a warning while it's unsupported, it instead throws "Type error for parameter createProperties" and prevents the tab from opening altogether, forcing the use of UA sniffing where I'd expect graceful degradation.)

Comment 15

[Andy McKay]

Kris, can we polish this patch off and land?

Comment 16

[Kris Maglione [:kmag]]

With something similar to the implementation in the attached patch, yes. Making the changes to tie it to window.opener would take me a good week and require some deep platform changes.

Comment 17

[Jonathan Kingston [:jkt] he/him]

Does Chrome allow for opener removal?
It looks like the attached patch only allows changing the tab and not removing it entirely.

Comment 18

[Jonathan Kingston [:jkt] he/him]

Also worth noting in Chrome:
- openerTabId isn't present for window.open popups
- Running update {openerTabId: x} returns in the value of tab.create()
  - However it doesn't change the value of window.opener in the actual tab?

Comment 19

[Nicholas Nethercote [inactive]]

I've been trying out Tree Tabs (https://addons.mozilla.org/en-US/firefox/addon/tree-tabs/) and it currently can't distinguish between a tab opened via middle-clicking a link (which I want to open as a child tab) from a tab opened via Ctrl-T or a bookmark (which I want to open as a top-level tab at the bottom of the tab list). I was told by the Tree Tabs author that this was due to the following.

> There is no way to distinguish where tab came from in current API. In 
> Chrome/Opera/Vivaldi tab opened from middle click gives openerId, Firefox
> does not.

Is this functionality covered by this bug? I'm not sure. If it's different, let me know and I'm happy to file a separate bug.

Comment 21

[kroppy]

Any news on this? Without it new tabs in my extension open at the end of the list as Nicholas pointed out. I have to make all tabs open as children which is just bad.

Comment 22

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #19)
> I've been trying out Tree Tabs
> (https://addons.mozilla.org/en-US/firefox/addon/tree-tabs/) and it currently
> can't distinguish between a tab opened via middle-clicking a link (which I
> want to open as a child tab) from a tab opened via Ctrl-T or a bookmark
> (which I want to open as a top-level tab at the bottom of the tab list).

Just to reiterate: we're about to enter the 57 dev cycle, which will kill off legacy extensions. Tree Tab appears to be the best option for Tree Style Tab users, but I found it had two show-stopping problems:

- The lack of control over where new tabs get opened (this bug).

- Ctrl-Tab cycles through tabs in the order they were opened (i.e. oldest first), rather than the on-screen order, which is really confusing. I'm not sure if identifying the on-screen order is possible in WebExtensions; kroppy was unsure how to do this when I last asked.

Comment 23

[Manish Goregaokar [:manishearth]]

The issue is that Tree Tabs maintains a separate tab list and does not attempt to keep the browser tabs in order, because it is expensive to do so.

https://gitlab.com/kroppy/TreeTabsNightly/issues/1

We'd need an API for remembering tab IDs that works through a restart, or something similar.

Comment 24

[Kris Maglione [:kmag]]

Attachment: Bug 1238314: Part 1 - Track opener tabs separately from owner and selected tab.

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

Comment 25

[Kris Maglione [:kmag]]

Attachment: Bug 1238314: Part 2 - Implement browser.tabs openerTabId functionality.

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

Comment 26

[Andrew Swan [:aswan]]

Comment on attachment 8894018 [details]
Bug 1238314: Part 2 - Implement browser.tabs openerTabId functionality.

https://reviewboard.mozilla.org/r/165106/#review170490

::: browser/components/extensions/ext-utils.js:243
(Diff revision 1)
> +    if (tab.ownerDocument !== openerTab.ownerDocument) {
> +      throw new Error("Tab must be in the same window as its opener");
> +    }

This is already checked in the one place that calls this function, one check should be sufficient.

::: browser/components/extensions/test/browser/browser_ext_tabs_opener.js:20
(Diff revision 1)
> +
> +    background() {
> +      let activeTab;
> +      let tabId;
> +      let tabIds;
> +      browser.tabs.query({lastFocusedWindow: true}).then(tabs => {

why not write this with async/await

Comment 27

[Andrew Swan [:aswan]]

Comment on attachment 8894017 [details]
Bug 1238314: Part 1 - Track opener tabs separately from owner and selected tab.

https://reviewboard.mozilla.org/r/165104/#review170492

Somebody who already knows this code better than i do should look at this.

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8894018 [details]
Bug 1238314: Part 2 - Implement browser.tabs openerTabId functionality.

https://reviewboard.mozilla.org/r/165106/#review170490

> why not write this with async/await

Because I wrote it about 2 years ago, when that wasn't an option, and it didn't seem worth the effort to rewrite.

Comment 29

[:Gijs (he/him)]

Comment on attachment 8894017 [details]
Bug 1238314: Part 1 - Track opener tabs separately from owner and selected tab.

https://reviewboard.mozilla.org/r/165104/#review170520

I'm not confident some of these changes are OK without more comments / explanation, and there's a few issues even if the changes I'm worried about are OK, so r- for now.

::: browser/base/content/tabbrowser.xml:85
(Diff revision 1)
> -      <field name="_lastRelatedTab">
> -        null
> +      <field name="_lastRelatedTabMap">
> +        new WeakMap();

You've left useless null setters later on in the file, per the last few hits on https://dxr.mozilla.org/mozilla-central/search?q=_lastRelatedTab&=mozilla-central .

::: browser/base/content/tabbrowser.xml:2684
(Diff revision 1)
>              // Check if we're opening a tab related to the current tab and
>              // move it to after the current tab.
>              // aReferrerURI is null or undefined if the tab is opened from
>              // an external application or bookmark, i.e. somewhere other
>              // than the current tab.
> -            if ((aRelatedToCurrent == null ? aReferrerURI : aRelatedToCurrent) &&
> +            if ((openerTab || aReferrerURI) &&

Why is collating this with aOwner and aOpenerBrowser here OK?

Also, please update the comment above this code to continue to match this block.

::: browser/base/content/tabbrowser.xml:2693
(Diff revision 1)
> -              if (this._lastRelatedTab)
> -                this._lastRelatedTab.owner = null;
> +              let lastRelatedTab = this._lastRelatedTabMap.get(opener);
> +              let newTabPos = (lastRelatedTab || opener)._tPos + 1;
> +              if (lastRelatedTab)
> +                lastRelatedTab.owner = null;
>                else
> -                t.owner = this.selectedTab;
> +                t.owner = opener;

This is changing, too (specifically, `t.owner` might now be `aOwner` or `aOpenerBrowser` instead of `selectedTab` as before). Why is that change OK and/or necessary?

Generally, the mixing of the different concepts here makes the code hard to follow. Comments in either the code or the commit message about what these different things are and if/when/how they should/shouldn't be confused/mixed would be really helpful.

::: browser/base/content/tabbrowser.xml:2695
(Diff revision 1)
> +              if (lastRelatedTab)
> +                lastRelatedTab.owner = null;
>                else
> -                t.owner = this.selectedTab;
> +                t.owner = opener;
>                this.moveTabTo(t, newTabPos);
> -              this._lastRelatedTab = t;
> +              this._lastRelatedTabMap.set(opener, t);

This will lose the related tab when tabs get moved to another window, because the respective gBrowser won't have this information. Should we keep that information when the tab gets moved?

Comment 30

[Kris Maglione [:kmag]]

Comment on attachment 8894017 [details]
Bug 1238314: Part 1 - Track opener tabs separately from owner and selected tab.

https://reviewboard.mozilla.org/r/165104/#review170520

> You've left useless null setters later on in the file, per the last few hits on https://dxr.mozilla.org/mozilla-central/search?q=_lastRelatedTab&=mozilla-central .

Ugh. Sorry. I wound up having to manually rebase this. I thought I'd gotten everything.

> Why is collating this with aOwner and aOpenerBrowser here OK?
> 
> Also, please update the comment above this code to continue to match this block.

So, essentially, what we want to do here is handle opening tabs next to their openers (or, really, after all related tabs opened by their openers). Currently, we only handle that properly when the opener tab is the currently selected tab. When a background tab calls window.open(), we handle it badly, and in different bad ways depending on the exact circumstances.

I was initially just going to ignore that for the initial implementation, and go with something that was Good Enough for most use cases, but Bill objected.

So this changes the way we handle related tabs so that we tie it to the opener tab, whether or not that tab is current. If we're passed `relatedToCurrent: true`, we make the current tab the opener, and everything behaves the same as before. If a background tab calls window.open(), we get the opener tab from the openerBrowser we were passed, and everything behaves the same as if the window.open() call had happened when that browser was in the foreground.

> This is changing, too (specifically, `t.owner` might now be `aOwner` or `aOpenerBrowser` instead of `selectedTab` as before). Why is that change OK and/or necessary?
> 
> Generally, the mixing of the different concepts here makes the code hard to follow. Comments in either the code or the commit message about what these different things are and if/when/how they should/shouldn't be confused/mixed would be really helpful.

Same reason as above. We already set t.owner to aOwner above, so that's not really changing unless we pass owner and relatedToCurrent at the same time. And even when we do, ownerTab should always be the selected tab in that case.

To some extent, I agree that mixing concepts here makes the code hard to follow. But I think it was already quite hard to follow before, and if anything is slightly easier now.

What I really want to do is get rid of ownerTab and relatedToCurrent entirely, and get rid of the aReferrerURI checks, and just use openerBrowser for all of the relevant logic. But that requires changing way more code than I want to touch in this bug.

> This will lose the related tab when tabs get moved to another window, because the respective gBrowser won't have this information. Should we keep that information when the tab gets moved?

No. This is only used for figuring out the relative positioning of tabs opened in the same window as the tabs they're related to. If the tab is moved to a different window, it ceases to be useful.

Comment 31

[Kris Maglione [:kmag]]

Comment on attachment 8894017 [details]
Bug 1238314: Part 1 - Track opener tabs separately from owner and selected tab.

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

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8894018 [details]
Bug 1238314: Part 2 - Implement browser.tabs openerTabId functionality.

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

Comment 33

[:Gijs (he/him)]

Comment on attachment 8894017 [details]
Bug 1238314: Part 1 - Track opener tabs separately from owner and selected tab.

https://reviewboard.mozilla.org/r/165104/#review170574

Thanks!

::: browser/base/content/tabbrowser.xml:2473
(Diff revision 2)
> +            // determining positioning, and inherited attributes such as the
> +            // user context ID.
> +            //
> +            // If we're passed an explicit owner tab, that takes precedence,
> +            // and we treat that as the opener. If not, and we have a browser
> +            // opener (which is usually the top-level browser from a remote

Nit: top-level browser doesn't make sense - top-level window does, but this is a browser, so I would just remove 'top-level'.

Comment 34

[Nicholas Nethercote [inactive]]

FWIW, I just switched from Nightly to Release because Tree Style Tab was rendered unusable by the latest Nightly update -- the tabs weren't showing at all. So it's great to see progress here! :)

Comment 35

[Botond Ballo [:botond]]

(In reply to Nicholas Nethercote [:njn] from comment #34)
> FWIW, I just switched from Nightly to Release because Tree Style Tab was
> rendered unusable by the latest Nightly update -- the tabs weren't showing
> at all. So it's great to see progress here! :)

Note that this breakage has been fixed [1] in the latest Tree Style Tab nightly [2].

[1] https://github.com/piroor/treestyletab/issues/1304
[2] http://piro.sakura.ne.jp/xul/xpi/nightly/treestyletab.xpi

Comment 36

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/009497a32dc73ffebf1bf2cbc9ed42e3d1d3dbcb
Bug 1238314: Part 1 - Track opener tabs separately from owner and selected tab. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6da9b2c39e6c77d69ad12797f09442c5e241088
Bug 1238314: Part 2 - Implement browser.tabs openerTabId functionality. r=aswan

Comment 37

[Jonathan Kingston [:jkt] he/him]

Will this patch permit an updated opener to communicate with it's newly minted connection via opener.postMessage? If not, will it prevent a removed opener from communicating?

If so we should probably add a test for that, we could write lots of interesting experiments with this. However I get a feeling this feature is out of scope.

Comment 38

[Kris Maglione [:kmag]]

No, not unless the tab was opened via window.open() or a link click.

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/009497a32dc7
https://hg.mozilla.org/mozilla-central/rev/d6da9b2c39e6

Comment 40

[Dão Gottwald [:dao]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #30)
> > Generally, the mixing of the different concepts here makes the code hard to follow. Comments in either the code or the commit message about what these different things are and if/when/how they should/shouldn't be confused/mixed would be really helpful.
> 
> Same reason as above. We already set t.owner to aOwner above, so that's not
> really changing unless we pass owner and relatedToCurrent at the same time.
> And even when we do, ownerTab should always be the selected tab in that case.
> 
> To some extent, I agree that mixing concepts here makes the code hard to
> follow. But I think it was already quite hard to follow before, and if
> anything is slightly easier now.

So I just looked into bug 1396375 and discovered this code with a bit of a shock. (Can we please from now on have Firefox::Tabbed Browser bugs for these kind of changes?) Having both "owner" and "opener" seems very confusing; I fail to see how this makes things easier.

> What I really want to do is get rid of ownerTab and relatedToCurrent
> entirely, and get rid of the aReferrerURI checks, and just use openerBrowser
> for all of the relevant logic. But that requires changing way more code than
> I want to touch in this bug.

Have you filed a bug on this?

Comment 41

[Nicholas Nethercote [inactive]]

Thank you for this fix. I'm enjoying having Tree Style Tab working in FF57!

Comment 42

[Michael Froman [:mjf]]

(In reply to Nicholas Nethercote [:njn] from comment #41)
> Thank you for this fix. I'm enjoying having Tree Style Tab working in FF57!

Agreed.  A breath of fresh air!  Thank you!

Comment 43

[Will Bamberg [:wbamberg]]

I've noted that this is supported in Firefox 57 in:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/query
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/update
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create

Please let me know if that covers it.

Comment 44

[kroppy]

(In reply to Will Bamberg [:wbamberg] from comment #43)

> Please let me know if that covers it.

Yes. Thank you.

Comment 45

[Andy McKay]

lgtm, thanks

Comment 46

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #40)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #30)
> > > Generally, the mixing of the different concepts here makes the code hard to follow. Comments in either the code or the commit message about what these different things are and if/when/how they should/shouldn't be confused/mixed would be really helpful.
> > 
> > Same reason as above. We already set t.owner to aOwner above, so that's not
> > really changing unless we pass owner and relatedToCurrent at the same time.
> > And even when we do, ownerTab should always be the selected tab in that case.
> > 
> > To some extent, I agree that mixing concepts here makes the code hard to
> > follow. But I think it was already quite hard to follow before, and if
> > anything is slightly easier now.
> 
> So I just looked into bug 1396375 and discovered this code with a bit of a
> shock. (Can we please from now on have Firefox::Tabbed Browser bugs for
> these kind of changes?) Having both "owner" and "opener" seems very
> confusing; I fail to see how this makes things easier.
> 
> > What I really want to do is get rid of ownerTab and relatedToCurrent
> > entirely, and get rid of the aReferrerURI checks, and just use openerBrowser
> > for all of the relevant logic. But that requires changing way more code than
> > I want to touch in this bug.
> 
> Have you filed a bug on this?

Gijs, can you take care of filing a bug on cleaning this up -- ideally with a plan rather than just stating that the current setup is less than ideal? I'm doubtful that anyone understands the current setup and that's rather unhealthy.

Comment 47

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #46)
> Gijs, can you take care of filing a bug on cleaning this up -- ideally with
> a plan rather than just stating that the current setup is less than ideal?
> I'm doubtful that anyone understands the current setup and that's rather
> unhealthy.

I filed bug 1455264.