Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Default to Active Tab in scripting.executeScript() #431

Open
erosman opened this issue Jul 25, 2023 · 5 comments
Open

Proposal: Default to Active Tab in scripting.executeScript() #431

erosman opened this issue Jul 25, 2023 · 5 comments
Labels
enhancement Enhancement or change to an existing feature neutral: firefox Not opposed or supportive from Firefox opposed: chrome Opposed by Chrome supportive: safari Supportive from Safari

Comments

@erosman
Copy link

erosman commented Jul 25, 2023

Preamble

Note: When using Manifest V3 or higher, use scripting.executeScript() to execute scripts.

tabs.executeScript()

scripting.executeScript() is introduced as the replacement for tabs.executeScript(), however:

Proposal

Make TabId optional and default to the active tab in scripting.executeScript() like tabs.executeScript()

Purpose

Many extensions only require "activeTab" permission and if necessary, tabs.executeScript() can be executed without any additional process or permission in MV2.

Aforementioned extensions in MV3 scripting.executeScript() would be required to include the active tab ID, e.g. via tabs.query() which would additionally require "tabs" permission.

A common example is onCommand API which does not include any tab information.

@tophf
Copy link

tophf commented Jul 26, 2023

Also suggested in https://crbug.com/1194519.

P.S. Note that there's no need for the tabs permission to get the tab id. The problem with the current design is only the lack of consistency with MV2 and the need to write boilerplate code.

@dotproto
Copy link
Member

dotproto commented Aug 1, 2023

Nit: "active tab" is a little overloaded due to it's close association with the activeTab permission. I'd suggest using "current tab" in order to better reflect the intended meaning.

@oliverdunk, can you speak to the rationale behind making tab selection explicit when calling scripting.executeScript() in Chrome?

@dotproto dotproto added enhancement Enhancement or change to an existing feature follow-up: chrome Needs a response from a Chrome representative and removed needs-triage labels Aug 1, 2023
@xeenon xeenon added the supportive: safari Supportive from Safari label Aug 3, 2023
@Rob--W Rob--W added the neutral: firefox Not opposed or supportive from Firefox label Aug 3, 2023
@rdcronin rdcronin added opposed: chrome Opposed by Chrome and removed follow-up: chrome Needs a response from a Chrome representative labels Sep 13, 2023
@rdcronin
Copy link
Contributor

rdcronin commented Sep 13, 2023

Chrome's opposed to this.

Determining the currently-active tab is fraught with complexity:

  • "Active tab" itself is potentially-ambiguous. OS's can have different ways of determining focused / active windows, and it's unclear what the behavior should be if e.g. there isn't a focused Chrome window (does it grab the active tab of the most-recently focused Chrome window?). This has historically led to a lot of confusing and unexpected behaviors.
  • There's a lot of inherent raciness in defaulting to the active tab. The renderer the extension runs in can't determine the active tab, and the tab that's active at the time the browser process receives the request may or may not be the same tab that was active when the extension called executeScript().

Instead of this, I think we should make it easy for extensions to find the tab they want to inject in. In cases where the extension "probably" wants to inject in the active tab (we've seen this most in response to action.onClicked, contextMenus.onClicked, commands.onCommand**, etc), we pass the tab object into the event so it's trivial to specify the ID. For cases where executeScript is being called from a background context and in response to an event that isn't tied to an event like that, I don't think we have high confidence that the developer wants to target the active tab (for instance, executeScript is often used for injecting scripts into newly opened tabs -- which may be background -- or in response to webnavigation events, which could easily be for background tabs).

One case that was brought up by @carlosjeurissen that isn't well supported here is invoking executeScript() from an extension's popup. I would rather support this (more easily) by extending tabs.getCurrent() (which does not return the active tab, but rather the tab associated with the currently-running script) to return the tab the popup has. This would make the case of injecting in the tab under the popup simple and more reliable than defaulting to the active tab (since the popup may be open a tab in a backgrounded window). I think we should file an issue for that separately.

** Firefox does not currently include a tab for commands.onCommand. Chrome does.

@erosman
Copy link
Author

erosman commented Sep 13, 2023

Instead of this, I think we should make it easy for extensions to find the tab they want to inject in.

It sounds good, and as you have mentioned, often extensions that rely on activeTab permission, use it subsequent to user-actions. Therefore, providing the tab.id with those actions should be a good solution.

Main areas where tabs.Tab would be needed

@tophf
Copy link

tophf commented Sep 13, 2023

@rdcronin, 1) re "make it easy for extensions to find the tab" it would be nice if you add the popup's tabId to chrome.action similarly to chrome.devtools.inspectedWindow.tabId. 2) Re requiring the developers to use chrome.tabs.query or any other explicit method doesn't remove the inherent raciness, it just adds boilerplate code, which will be used in exactly the same incorrect fashion in those scenarios you mentioned. Changing getCurrent behavior only reduces the boilerplate by a word or two, but doesn't change the fact itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or change to an existing feature neutral: firefox Not opposed or supportive from Firefox opposed: chrome Opposed by Chrome supportive: safari Supportive from Safari
Projects
None yet
Development

No branches or pull requests

6 participants