|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
This includes the `openerTabId` properties of the `tabs.create` and `tabs.update` methods, and of Tab objects.
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/
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
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.
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.
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.)
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?
Any plans on fixing this? As in WebExtensions documentation it says openerTabId property is supported in Firefox?
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.)
Kris, can we polish this patch off and land?
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.