w3c / webextensions

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

Ensure consistency of `action.openPopup` API across browsers #160

Open oliverdunk opened 2 years ago

oliverdunk commented 2 years ago

A Chromium bug was recently opened sharing an action.openPopup API, and a basic implementation has shipped restricted to the dev channel. This is super exciting since it's the first time Chrome extensions look set to get this ability.

Since Firefox has already implemented browserAction.openPopup, I'm curious on a few things:


ℹ️ Update: The initial questions here have been resolved and this issue is now tracking more nuanced behaviour differences between browsers. See the current behaviours and proposed standard behaviours.

oliverdunk commented 2 years ago

@zombie / @Rob--W, it would be great to get your input from the Mozilla side.

@dotproto, I'm not sure if there's anything you can share on behalf of Google? Would you be open to discussions about using the same API namespace as Mozilla or is that off the cards given that you already use that namespace for an internal API? The crbug seems quiet unfortunately.

zombie commented 2 years ago
  • Would Mozilla be willing to either support the new namespace as an alias or try to work with Google to encourage use of the existing API naming?

We already added support for the action manifest key/namespace for MV3 in bug Bug 1706398.

  • One big step the Chromium API has taken is removing the requirement for a user action. The API overview doc says that they "do not currently plan any mitigations for attacks of annoyance" because "extensions can already do far worse than this with the tabs API and window creation." Would Mozilla consider the same?

This seems reasonable, I filed Bug 1755763 to consider it.

Another difference is that Chromium has an optional windowId param (though the proposal doc mentions tabId instead?), which seems useful.

xeenon commented 2 years ago

We (Apple) would implement the windowId version to match Chrome. We could add tabId support too if others agree.

dotproto commented 2 years ago

I want to track down why we the design doc mentions tabId with windowId as a potential future enhancement, but we only implemented windowId. Unfortunately I haven't been able to find the time yet.

Links for later follow-up: crbug.com/1245093

oliverdunk commented 2 years ago

I've been working on a Firefox patch to bring the behaviour closer to the Chromium implementation. Given this has shipped in Safari Technology Preview too, I wanted to investigate the behaviour in other browsers to make sure it was consistent.

Sharing the results here for longevity...

Behaviour Chrome Safari 16+
Calls onClicked handler with popup No No
Calls onClicked handler without popup No No, focuses window
Default window Active tab in topmost window Active tab in topmost window
Can call for private window Unable to test¹ Yes
Closes popup when open Throws error (no active browser window) No, focuses window
Closes popups from other extensions Throws error (no active browser window) Yes
Requires user gesture No No
Grants activeTab permission No Skips permission prompt, API calls hang. Reloading the popup window restores ability to make API calls and request permissions.
User gesture grants activeTab No NA - content script injection is tied to granting host permissions
Callable from content script No No
Behaviour after .disable() call Throws error (no popup for tab) Ignored, popup continues to open
Behaviour when hidden in browser-specific UI Icon is revealed, popup opens Window focused, popup does not open
windowId for minimized window Unable to test¹ Window is maximised
windowId for hidden window Unable to test¹ Popup opens, window only focused if Safari is active app

¹ Notably windowId doesn't seem to be working in Chrome Canary (other than for already focused windows), which I'm fairly confident is a regression from when I last tested. There's already a comment about that and I've left another one.

Rob--W commented 2 years ago

Thanks for documenting the observed behavior @oliverdunk !

Here are some more interesting scenario's that aren't in the table yet:

oliverdunk commented 2 years ago

@Rob--W: I went ahead and updated the table with those! Definitely some interesting results.

oliverdunk commented 2 years ago

Here is my proposed standard based on the behaviour which feels best in each case:

Behaviour Proposed Standard
Calls onClicked handler with popup No
Calls onClicked handler without popup No
Default window Active tab in topmost window
Can call for private window Yes, but only if extension enabled in private contexts
Closes popup when open No, ignores request and potentially throws error
Closes popups from other extensions No, ignores request and potentially throws error
Requires user gesture No
Grants activeTab permission No
Behaviour when clicking browser icon would normally show permission prompt Permission prompt shows, or browser indicates to user popup is trying to open. Granting permission opens popup.
User gesture grants activeTab Optional. Preserved in Firefox for backwards compatibility.
Callable from content script No
Behaviour after .disable() call Ignored, potentially throws helpful error
Behaviour when hidden in browser-specific UI Icon is revealed, popup opens
windowId for minimized window Windows maximized, popup shows. Ignored if browser is not focused app.
windowId for hidden window Window focused, popup shows. Ignored if browser is not focused app.

I've added "Behaviour when clicking browser icon would normally show permission prompt" for the following UI in Safari:

image

The current Safari behaviour is to open the popup skipping this prompt (and not granting permissions), but I'm actually not sure if this is the way to go. Open to feedback there.

Rob--W commented 2 years ago
Behaviour Proposed Standard
Behaviour when clicking browser icon would normally show permission prompt Permission prompt shows, or browser indicates to user popup is trying to open. Granting permission opens popup.

I see potential for abuse here. I'd recommend to do nothing if no popup can be opened.

Rob--W commented 2 years ago

Question: What happens / should happen if an extension calls action.openPopup while another extension has already shown a popup?

oliverdunk commented 1 year ago

Question: What happens / should happen if an extension calls action.openPopup while another extension has already shown a popup?

In Chrome, this throws (with a no active window error if no windowId is specified, and a failed to open popup error if one is given). At least in the first case I feel like this is likely a bug and not the intended behaviour. In Safari, any popups from other extensions are closed.

I've updated the table of current behaviours and added a row in the proposed standard behaviours suggesting other popups are closed. I think this makes sense because stacking popups doesn't usually work well from a UX perspective and I think the fact that some browsers already close popups when the browser loses focus alleviates any data loss concerns.

Rob--W commented 1 year ago

I think that we should throw if the offered/expected behavior is unclear.

The action.openPopup API's purpose is to open an extension popup that the user can interact with. This can only safely be done if there is a window to anchor to, and the popup can be interacted with.

When another extension has a popup open, we should throw IMO.

When the extension already has a popup open, we could focus the popup if that makes sense. And perhaps also the window to which it is anchored.

When a context menu or menu is open, it may be best to throw. Generally, if the user is clearly busy interacting with something, it may be a probably poor UX to interrupt them.

oliverdunk commented 1 year ago

@xeenon, would you mind sharing your thoughts here?

wesinator commented 1 year ago

What is the expected behaviour around popups receiving messages after being opened programmatically with this API ? Is the intended/expected behaviour that messages can be received by the popup after openPopup() is called ?

As a developer, my understanding from https://stackoverflow.com/questions/48620183/chrome-extension-how-to-send-message-from-content-js-to-popup-js-page-action/ is that messages sent to a popup will only be received when a popup is open.

I tested this behavior in Firefox (see code example from https://stackoverflow.com/questions/76709174/webextension-how-to-send-context-menu-selectiontext-to-popup-action) but it seems that the message is lost/not sent when the popup is active from openPopup().

I have a use case for this: a context menu extension that takes the textSelection, processes it through a function, and opens the extension popup to display the output (the alternative methods are displaying output in tab through executeScript or an extension-generated page, or storing the output each time in localstorage and then reading and opening it in the popup, are less ideal for several reasons).

oliverdunk commented 1 year ago

What is the expected behaviour around popups receiving messages after being opened programmatically with this API ? Is the intended/expected behaviour that messages can be received by the popup after openPopup() is called ?

I would absolutely expect the popup to receive messages as if it had been opened by the user. Could you try using setTimeout to delay sending the message, to see if it is a timing issue? I expect that to be the case, but if not, I would definitely open a Firefox bug.

One thing we may want to discuss in the future is if openPopup should block on the popup loading. This seems desirable on first impressions but I actually think it's tricky - there are lots of edge cases if the popup closes before it loads, or an image takes a significant amount of time to load, where it's unclear at what point the promise should resolve.

Juraj-Masiar commented 1 year ago

One thing we may want to discuss in the future is if openPopup should block on the popup loading.

What's the point of having a Promise that doesn't await the task completely? This then leads into some terrible workarounds. I've reported similar issues in Firefox API several times, (example).

I would say, at a very least, the initial JavaScript execution of the popup should be awaited, so that the popup is ready to receive messages. And if the popup is closed before it's loaded, the Promise can be rejected with some "loading interrupted" error.

oliverdunk commented 1 year ago

What's the point of having a Promise that doesn't await the task completely? This then leads into some terrible workarounds. I've reported similar issues in Firefox API several times, (example).

I would say, at a very least, the initial JavaScript execution of the popup should be awaited, so that the popup is ready to receive messages. And if the popup is closed before it's loaded, the Promise can be rejected with some "loading interrupted" error.

There are several examples of this, for example browser.tabs.create which can resolve before the tab has finished loading: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/create#return_value.

I don't yet have a strong opinion on what the behaviour should be - but there are definitely tradeoffs. What if halfway through a script in the loading path of the popup, it makes a blocking XMLHTTPRequest call and hangs for a long period of time? I think in Chrome we actually delay the UI too, in which case delaying the API to match might make sense, but this definitely warrants some more investigation/discussion.

Juraj-Masiar commented 1 year ago

I can only speak as a "user" of these API. And from that point of view, it's very convenient to open new window/tab/popup and send it some work/message/config. And if it works, it's 2 lines of code and a straightforward code flow that's easy to reason about.

Let's compare it with some popular workarounds:

  1. adding timeout:

    • it's easy to implement and it solves most cases
    • it can introduce race condition if the timeout is too low / PC too slow / content too large / stars not aligned
  2. let the popup page ask for the data after it finishes loading

    • it's reliable with almost no delay
    • it's a terrible spaghetti solution that can be used only in the "hello world" application because it breaks code flow, encapsulation and introduces global variable
  3. register webNavigation.onCompleted and await the event

    • requires webNavigation permission and a bunch of code that reduces readability (don't forget to unregister the handler!)

Maybe there is a better way?

Alternatively, let's talk about the disadvantages. And let's be honest, nobody is using blocking XMLHTTPRequest :D, and if they are, they deserve whatever bugs it brings (or am I being too ignorant :)).

oliverdunk commented 1 year ago

I can only speak as a "user" of these API. And from that point of view, it's very convenient to open new window/tab/popup and send it some work/message/config. And if it works, it's 2 lines of code and a straightforward code flow that's easy to reason about and it solves most cases

For sure, I completely understand! This is actually something I originally advocated for when we were working on chrome.sidePanel.open. However, as I discovered more about the tradeoffs, I realised that it would be really hard to provide the expected behaviour as you start introducing all of the things that can go wrong. So I ultimately suggested (for the side panel API) we ship without that for now and can always make changes in the future.

I'm not saying I disagree with you on any of this. Just that there can be complexities, and we should discuss those within the group before deciding :)

oliverdunk commented 3 months ago

@Rob--W Currently Safari allows an extension to call openPopup() for a disabled popup. Firefox allows this, but it silently fails (regardless of the user gesture changes).

I'm trying to decide on the behavior for Chrome to implement - any thoughts?

My gut instinct is to allow this since there's no compelling reason not to and it may be useful in some cases. I don't feel too strongly though, and think ignoring and throwing an error would be another reasonable option.

oliverdunk commented 3 months ago

We've removed the channel restriction from action.openPopup in Chrome and this should be available from Chrome 127: https://chromiumdash.appspot.com/commit/f2a0df499ac990fc92dba0019aec696339cf0beb.

I'm working on some follow-up improvements to error messages. I'm leaving the disabled popup behavior until we have consensus (currently, it rejects with an error).