w3c / webextensions

Charter and administrivia for the WebExtensions Community Group (WECG)
Other
579 stars 50 forks source link

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

Open xeenon opened 9 months ago

xeenon commented 9 months ago

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.

tophf commented 9 months ago

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

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

Questions for Chrome (https://github.com/w3c/webextensions/labels/follow-up%3A%20chrome):

  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:

rdcronin commented 1 week ago

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.