Implement browser.tabs opener functionality

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Frontend
P2
enhancement
2 years ago
a day ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks: 2 bugs)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tabs] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
This includes the `openerTabId` properties of the `tabs.create` and `tabs.update` methods, and of Tab objects.
(Assignee)

Updated

2 years ago
Blocks: 1213477
Whiteboard: [tabs]

Updated

2 years ago
Flags: blocking-webextensions+
Priority: -- → P2
Whiteboard: [tabs] → [tabs] triaged
(Assignee)

Updated

2 years ago
Assignee: nobody → kmaglione+bmo
(Assignee)

Comment 1

2 years ago
Created attachment 8722260 [details]
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/
Attachment #8722260 - Flags: review?(wmccloskey)
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
Attachment #8722260 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

2 years ago
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.
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.
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Updated

2 years ago
Iteration: --- → 48.1 - Mar 21
(Assignee)

Updated

a year ago
Duplicate of this bug: 1258823
(Assignee)

Updated

a year ago
Duplicate of this bug: 1276346
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1293727

Comment 9

11 months ago
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.)

Updated

10 months ago
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Flags: blocking-webextensions+
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1311504
Blocks: 1310331
(Assignee)

Updated

10 months ago
No longer blocks: 1310331

Comment 11

10 months ago
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?

Updated

8 months ago
webextensions: --- → ?
(Assignee)

Updated

8 months ago
Duplicate of this bug: 1325818

Comment 13

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

Comment 14

7 months ago
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

5 months ago
Kris, can we polish this patch off and land?
webextensions: ? → +
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 16

5 months ago
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.
Flags: needinfo?(kmaglione+bmo)
Does Chrome allow for opener removal?
It looks like the attached patch only allows changing the tab and not removing it entirely.
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?
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.
(Assignee)

Updated

4 months ago
Duplicate of this bug: 1360958

Comment 21

3 months ago
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.

Updated

a month ago
Blocks: 1311472

Updated

a month ago
Blocks: 1215059
Severity: normal → enhancement
(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.
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

12 days ago
Attachment #8722260 - Attachment is obsolete: true

Comment 26

12 days ago
mozreview-review
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
Attachment #8894018 - Flags: review?(aswan) → review+

Comment 27

12 days ago
mozreview-review
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.
Attachment #8894017 - Flags: review?(aswan)
(Assignee)

Comment 28

12 days ago
mozreview-review-reply
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.
(Assignee)

Updated

12 days ago
Attachment #8894017 - Flags: review?(gijskruitbosch+bugs)

Comment 29

12 days ago
mozreview-review
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?
Attachment #8894017 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 30

11 days ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

11 days ago
mozreview-review
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'.
Attachment #8894017 - Flags: review?(gijskruitbosch+bugs) → review+
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! :)
(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
You need to log in before you can comment on or make changes to this bug.