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

Inconsistency in browser.action: windowId key and enable()/disable() arguments #463

Open
xeenon opened this issue Oct 3, 2023 · 4 comments
Labels
implemented: firefox Implemented in Firefox inconsistency Inconsistent behavior across browsers proposal Proposal for a change or new feature supportive: chrome Supportive from Chrome supportive: safari Supportive from Safari

Comments

@xeenon
Copy link
Collaborator

xeenon commented Oct 3, 2023

There is an inconsistency in the browser.action APIs with the windowId key. Currently, this key is only supported in Firefox but is slated for future implementation in Safari. This feature is essential for extensions aiming to synchronize action customizations across all tabs within a specific window.

In Firefox, the windowId key allows extensions to uniformly apply action states to all tabs within a designated window.

await browser.action.setTitle({
  title: "This Window",
  windowId: someWindowId
});

In Firefox's existing API design, the options dictionary should specify either a tabId or a windowId. This approach ensures that action states are applied on either a per-tab or per-window basis. Safari plans to adopt this behavior.

Contrastingly, the enable() and disable() methods currently only accept a tabId as an argument:

await browser.action.enable(someTabId);
await browser.action.disable(someTabId);

Proposed Changes

To harmonize these APIs, I propose adopting windowId in all action methods to match Firefox's existing behavior. Additionally, a new setEnabled() method is proposed. This method would accept an options dictionary that can include tabId, windowId, and enabled keys.

// Using setEnabled with an options dictionary
await browser.action.setEnabled({
  tabId: someTabId,
  enabled: true
});

// Or for a window
await browser.action.setEnabled({
  windowId: someWindowId,
  enabled: false
});

This new method offers a consistent way to manage action enabled states across both tabs and windows. The existing enable() and disable() methods would be deprecated until removed in a future manifest version.

Alternate Solution

An alternative to introducing setEnabled() would be to update the existing enable() and disable() methods. These methods could accept an options dictionary, in addition to the current tabId argument:

// Using tabId directly (deprecated)
await browser.action.enable(someTabId);

// Using options dictionary with tabId
await browser.action.enable({
  tabId: someTabId
});

// Using options dictionary with windowId
await browser.action.enable({
  windowId: someWindowId
});

This alternative also maintains backward compatibility while providing the flexibility to manage action enabled states across different tabs and windows.

Compatibility Considerations

This proposal aims to extend existing APIs in a backward-compatible manner. Nevertheless, the introduction of the windowId key and an options dictionary in enable() and disable() could cause exceptions in older browsers. The introduction of setEnabled() offers the advantage of being feature-detectable from JavaScript. Developers would be strongly advised to test compatibility.

@xeenon xeenon added inconsistency Inconsistent behavior across browsers implemented: firefox Implemented in Firefox supportive: safari Supportive from Safari proposal Proposal for a change or new feature labels Oct 3, 2023
@tophf
Copy link

tophf commented Oct 4, 2023

AFAIK, Chrome verifies parameters against a schema for many years, maybe from the very beginning, so extending the existing methods wouldn't crash an old Chrome, it would just report an error. I'm not saying it'd be a better solution though, because developers would have to either use try{}catch{} (and what's worse .catch() wouldn't work at least in Chrome as far as I know as the error is thrown by the JS shim) or check the version of the browser, which is an anti-pattern in general.

Regarding the name, maybe it could be just set or setOptions or configure and accept various other aspects like title and color?

@xeenon
Copy link
Collaborator Author

xeenon commented Oct 4, 2023

Yeah, I know Chrome is strict with argument type checks. I think all browsers are in this regard. That's why changing the allow types for enable() and disable() would likely be off the table without a manifest version bump.

@Rob--W Rob--W added the follow-up: chrome Needs a response from a Chrome representative label Oct 26, 2023
@Rob--W
Copy link
Member

Rob--W commented Oct 26, 2023

Questions for Chrome ( follow-up: chrome Needs a response from a Chrome representative ):

  1. Supportive of windowId option in action API? (tabId > windowId > manifest-specified default)
  2. Supportive of overloading the enable/disable methods to also accept an object instead of just an integer option? (the old signature is kept for backcompat and can be removed only in a new manifest version)

Additional thoughts on:

  • clearing a windowId-specific value (e.g. title)? E.g. setting to null?

@rdcronin
Copy link
Contributor

Supportive of windowId option in action API? (tabId > windowId > manifest-specified default)

Yes.

Supportive of overloading the enable/disable methods to also accept an object instead of just an integer option? (the old signature is kept for backcompat and can be removed only in a new manifest version)

Yes.

Re setEnabled() vs updating enable() and disable() -- there's no harm in changing the signature (still backwards compatible), and one of the pieces that hasn't been mentioned here is the isEnabled() function. isEnabled() currently only accepts a tabId in Chrome, so we'd need to change that to accept an object. Since we'd already be changing signatures of existing methods, I'd lean towards just using the existing enable() and disable() rather than introducing a new method.

@rdcronin rdcronin added supportive: chrome Supportive from Chrome and removed follow-up: chrome Needs a response from a Chrome representative labels Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implemented: firefox Implemented in Firefox inconsistency Inconsistent behavior across browsers proposal Proposal for a change or new feature supportive: chrome Supportive from Chrome supportive: safari Supportive from Safari
Projects
None yet
Development

No branches or pull requests

4 participants