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

Ensure consistency of action.openPopup API across browsers #160

Open
oliverdunk opened this issue Feb 6, 2022 · 21 comments
Open

Ensure consistency of action.openPopup API across browsers #160

oliverdunk opened this issue Feb 6, 2022 · 21 comments
Labels
enhancement Enhancement or change to an existing feature inconsistency Inconsistent behavior across browsers

Comments

@oliverdunk
Copy link
Member

oliverdunk commented Feb 6, 2022

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:

  • 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? In an ideal world I would rather see Chromium implement the Firefox API, since that has already shipped and seems to work well. However, they seem to be wanting to diverge from the private API they have already implemented so I'm not sure how feasible that is.
  • 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? It immediately solves some of the native messaging use cases raised at the start of Discuss and encourage adoption of browserAction.openPopup() #15.

ℹ️ 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
Copy link
Member Author

@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.

@xeenon xeenon added inconsistency Inconsistent behavior across browsers enhancement Enhancement or change to an existing feature future topics labels Feb 9, 2022
@zombie
Copy link
Collaborator

zombie commented Feb 16, 2022

  • 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
Copy link
Collaborator

xeenon commented Feb 18, 2022

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

@dotproto
Copy link
Member

dotproto commented Mar 2, 2022

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
Copy link
Member Author

oliverdunk commented May 15, 2022

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.

@oliverdunk oliverdunk changed the title Ask Mozilla for their position on action.openPopup API Ensure consistency of action.openPopup API across browsers May 15, 2022
@Rob--W
Copy link
Member

Rob--W commented May 17, 2022

Thanks for documenting the observed behavior @oliverdunk !

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

  • When a user gesture is present, is the activeTab permission granted and effective when action.openPopup is called?
  • When the popup is disabled via a .disable() call in the API, what does openPopup do?
  • When the extension button is hidden via browser-specific UI, what does openPopup do?
  • When the window is in the background, what happens when openPopup is called with a windowId?
  • When the window is minimized, what happens when openPopup is called with a windowId?

@oliverdunk
Copy link
Member Author

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

@oliverdunk
Copy link
Member Author

oliverdunk commented Jun 7, 2022

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
Copy link
Member

Rob--W commented Jun 13, 2022

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
Copy link
Member

Rob--W commented Jun 30, 2022

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

@oliverdunk
Copy link
Member Author

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
Copy link
Member

Rob--W commented Nov 7, 2022

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
Copy link
Member Author

@xeenon, would you mind sharing your thoughts here?

@wesinator
Copy link

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
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

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
Copy link

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
  1. 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
  1. 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
Copy link
Member Author

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
Copy link
Member Author

@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
Copy link
Member Author

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).

AriYoung00 pushed a commit to AriYoung00/WebKit that referenced this issue Jun 27, 2024
…h a call to `openPopup()`.

https://webkit.org/b/275914
rdar://127836084
w3c/webextensions#160

Reviewed by NOBODY (OOPS!).

Adds a static member to WebExtensionContext which signifies whether or
not any extension has an open popup, along with other changes to
WebExtensionContext to update this variable.

* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIActionCocoa.mm:
(WebKit::WebExtensionContext::actionOpenPopup): Modified to check the
state of popupIsOpen
* Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionActionCocoa.mm:
(WebKit::WebExtensionAction::readyToPresentPopup): Modified to set the
state of s_isPopupOpen when a popup is presented
(WebKit::WebExtensionAction::closePopup): Modified to set the state of
s_isPopupOpen when a popup is closed
* Source/WebKit/UIProcess/Extensions/WebExtensionContext.h:
(WebKit::WebExtensionContext::popupOpening): Setter for s_isPopupOpen
(WebKit::WebExtensionContext::popupClosed):  Setter for s_isPopupOpen
(WebKit::WebExtensionContext::popupIsOpen const): Getter for
s_isPopupOpen
AriYoung00 pushed a commit to AriYoung00/WebKit that referenced this issue Jun 27, 2024
…h a call to `openPopup()`.

https://webkit.org/b/275914
rdar://127836084

w3c/webextensions#160

Reviewed by NOBODY (OOPS!).

Adds a static member to WebExtensionContext which signifies whether or
not any extension has an open popup, along with other changes to
WebExtensionContext to update this variable.

* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIActionCocoa.mm:
(WebKit::WebExtensionContext::actionOpenPopup): Modified to check the
state of popupIsOpen
* Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionActionCocoa.mm:
(WebKit::WebExtensionAction::readyToPresentPopup): Modified to set the
state of s_isPopupOpen when a popup is presented
(WebKit::WebExtensionAction::closePopup): Modified to set the state of
s_isPopupOpen when a popup is closed
* Source/WebKit/UIProcess/Extensions/WebExtensionContext.h:
(WebKit::WebExtensionContext::popupOpening): Setter for s_isPopupOpen
(WebKit::WebExtensionContext::popupClosed):  Setter for s_isPopupOpen
(WebKit::WebExtensionContext::popupIsOpen const): Getter for
s_isPopupOpen
AriYoung00 pushed a commit to AriYoung00/WebKit that referenced this issue Jun 27, 2024
…h a call to `openPopup()`.

https://webkit.org/b/275914
rdar://127836084

w3c/webextensions#160

Reviewed by NOBODY (OOPS!).

Adds a static member to WebExtensionContext which signifies whether or
not any extension has an open popup, along with other changes to
WebExtensionContext to update this variable.

* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIActionCocoa.mm:
(WebKit::WebExtensionContext::actionOpenPopup): Modified to check the
state of popupIsOpen
* Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionActionCocoa.mm:
(WebKit::WebExtensionAction::readyToPresentPopup): Modified to set the
state of s_isPopupOpen when a popup is presented
(WebKit::WebExtensionAction::closePopup): Modified to set the state of
s_isPopupOpen when a popup is closed
* Source/WebKit/UIProcess/Extensions/WebExtensionContext.h:
(WebKit::WebExtensionContext::popupOpening): Setter for s_isPopupOpen
(WebKit::WebExtensionContext::popupClosed):  Setter for s_isPopupOpen
(WebKit::WebExtensionContext::popupIsOpen const): Getter for
s_isPopupOpen
webkit-commit-queue pushed a commit to AriYoung00/WebKit that referenced this issue Jun 27, 2024
…h a call to `openPopup()`.

https://webkit.org/b/275914
rdar://127836084

w3c/webextensions#160

Reviewed by Timothy Hatcher.

Adds a static member to WebExtensionContext which signifies whether or
not any extension has an open popup, along with other changes to
WebExtensionContext to update this variable.

* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIActionCocoa.mm:
(WebKit::WebExtensionContext::actionOpenPopup): Modified to check the
state of popupIsOpen
* Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionActionCocoa.mm:
(WebKit::WebExtensionAction::readyToPresentPopup): Modified to set the
state of s_isPopupOpen when a popup is presented
(WebKit::WebExtensionAction::closePopup): Modified to set the state of
s_isPopupOpen when a popup is closed
* Source/WebKit/UIProcess/Extensions/WebExtensionContext.h:
(WebKit::WebExtensionContext::popupOpening): Setter for s_isPopupOpen
(WebKit::WebExtensionContext::popupClosed):  Setter for s_isPopupOpen
(WebKit::WebExtensionContext::popupIsOpen const): Getter for
s_isPopupOpen

Canonical link: https://commits.webkit.org/280422@main
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 inconsistency Inconsistent behavior across browsers
Projects
None yet
Development

No branches or pull requests

8 participants