Closed Bug 1341126 Opened 3 years ago Closed 3 years ago

add open API to browser/page/sidebarAction

Categories

(WebExtensions :: Frontend, defect)

49 Branch
defect
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

(Blocks 3 open bugs, )

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved] triaged)

Attachments

(1 file)

Chrome has a private api, browserAction.openPopup, which has been in discussion for making available to extensions.  This bug is to define the issues and requirements for the same or similar API in Firefox.

An initial read through the chromium discussion I've pulled out the following issues.  Proposed solutions (from that conversation) are marked with *.

Issues:

- multiple extensions calling openPopup
  * openPopup fails if a popup is open
- extensions calling openPopup while firefox UI is open (e.g. geoloction prompt)
  * openPopup fails if primary ui is open
- identification of extension when buttons are located in overflow/hamburger menu
  * Dont allow openPopup when button is not visible in toolbar
- attributing the opening of a panel to an action
  * require user action to call openPopup
- abusive practices
  - e.g. opening a panel on every page load
  - looking like trusted ui
  * require user action to call openPopup
- intrusive practices
  - e.g. stealing focus from a form while user is typing
  * require user action to call openPopup
- popups get activeTab permission
  * require user action to call openPopup
- chrome conversation died out, we risk incompatible implementations of the same api
I'm actually having a hard time figuring out why requiring user action wouldn't solve all the issues listed in the chromium bug.
For what it's worth, this is one of the APIs SnoozeTabs would like to implement some missing functionality.
Whiteboard: [design-decision-needed]
This has been added to the March 7 WebExtensions Triage mtg. agenda. 

Agenda: https://docs.google.com/document/d/1zzfedbTKAHUmm4UctxSOIJV3iKayXQ8TuXPodXW8wC0/edit#
See Also: → 1343817
We should also understand what will happen with the pocket addon, which could bump priority on this.
Whiteboard: [design-decision-needed] → [design-decision-approved]
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

Looking for general feedback on this approach.
Attachment #8845285 - Flags: feedback?(kmaglione+bmo)
Attachment #8845285 - Flags: feedback?(aswan)
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review121790

::: browser/components/extensions/ext-browserAction.js:567
(Diff revision 1)
> +        let popup = BrowserAction.for(extension).getProperty(null, "popup");
> +        if (!popup) {
> +          return;
> +        }
> +        let window = windowTracker.topWindow;
> +        BrowserAction.for(extension).triggerAction(window);

This is going to wind up granting the activeTab permission if it's not already granted. Please add a test for this.

Also, if we only want to trigger the popup variant here, let's separate that out into a separate method rather than using triggerAction.

::: browser/components/extensions/ext-c-browserAction.js:12
(Diff revision 1)
> +        let window = Services.wm.getMostRecentWindow(null);
> +        let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                         .getInterface(Ci.nsIDOMWindowUtils);
> +        if (!dwu.isHandlingUserInput) {

Let's just transmit this information to the parent process from the binding layer, and either store it on the context, or set the internal flag while the method call is being dispatched.
Attachment #8845285 - Flags: feedback?(kmaglione+bmo) → feedback+
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

This is really Kris's domain, clearing the f? since he already responded
Attachment #8845285 - Flags: feedback?(aswan)
Assignee: nobody → mixedpuppy
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Whiteboard: [design-decision-approved] → [design-decision-approved] triaged
webextensions: --- → ?
webextensions: ? → ---
If I understand the patch correctly it only allows to toggle the sidebar. In order to be sure if the sidebar is being opened or being closed there needs to be at least a way to read the current state of the sidebar in the current window (closed, opened). And as far as I know there is no such way currently without workarounds.
(In reply to Croydon from comment #9)
> If I understand the patch correctly it only allows to toggle the sidebar. In
> order to be sure if the sidebar is being opened or being closed there needs
> to be at least a way to read the current state of the sidebar in the current
> window (closed, opened). And as far as I know there is no such way currently
> without workarounds.

There are workarounds, you can use load/unload and send a message to the background when opened/closed.  But I agree, it makes it easy if there is a state variable.  The problem is that sidebars can be different in different windows, so this would have to be attached to some other API where it makes better sense.  Maybe add a sidebarState to Window:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/windows/Window

The value would only be "open" if the addon's sidebar is open, it would be "closed" if some other sidebar is open or no sidebar is open.
Depends on: 1350151
Duplicate of this bug: 1376156
(In reply to Croydon from comment #9)
> If I understand the patch correctly it only allows to toggle the sidebar. In
> order to be sure if the sidebar is being opened or being closed there needs
> to be at least a way to read the current state of the sidebar in the current
> window (closed, opened). And as far as I know there is no such way currently
> without workarounds.

+100
Duplicate of this bug: 1343817
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review121790

> This is going to wind up granting the activeTab permission if it's not already granted. Please add a test for this.
> 
> Also, if we only want to trigger the popup variant here, let's separate that out into a separate method rather than using triggerAction.

I've been thinking about this.  The extension has to have activeTab permission, and it has to be a user initiated action.  I think it's fine to give these (in fact makes more sense) activeTab in that case.
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

alternate reviewer
Attachment #8845285 - Flags: review?(tomica)
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review172994

::: browser/components/extensions/test/browser/browser_ext_browserAction_simple.js:69
(Diff revision 3)
> +            browser.browserAction.openPopup(),
> +            "browserAction.openPopup may only be called from a user input handler",
> +            "The error is informative.");

Nit: Only one level of extra indentation for function arguments.

::: browser/components/extensions/test/browser/browser_ext_browserAction_simple.js:73
(Diff revision 3)
> +      await browser.test.assertRejects(
> +            browser.browserAction.openPopup(),
> +            "browserAction.openPopup may only be called from a user input handler",
> +            "The error is informative.");
> +
> +      browser.runtime.onMessage.addListener(async (msg) => {

Nit: No parens around single-argument arrow function argument lists. Also, the `async` is unnecessary.

::: browser/components/extensions/test/browser/browser_ext_browserAction_simple.js:80
(Diff revision 3)
> +        browser.test.sendMessage("popup");
> +      });
> +
> +      browser.test.sendMessage("browserAction.openPopup");
> +    });
> +    await browser.tabs.create({active: true, url: "tab.html"});

Nit: `active: true` is the default, and the `await` is not necessary.

::: browser/components/extensions/test/browser/browser_ext_browserAction_simple.js:130
(Diff revision 3)
> +  await extension.awaitMessage("browserAction.openPopup");
> +  let open = extension.awaitMessage("popup");
> +
> +  await BrowserTestUtils.synthesizeMouseAtCenter("#open", {}, gBrowser.selectedBrowser);
> +  await open;
> +  is(browserAction.tabManager.hasActiveTabPermission(gBrowser.selectedTab), true, "Active tab was granted permission");

This concerns me... But I guess as long as they're handling user input, they more or less have access to the active tab, anyway...

But this could probably be used to gain elevated privileges when clicking a link in a sidebar, or a subframe...

I suppose there's not much we can do about that, but it still makes me uncomfortable.

::: browser/components/extensions/test/browser/browser_ext_pageAction_simple.js:71
(Diff revision 3)
> +            browser.pageAction.openPopup(),
> +            "pageAction.openPopup may only be called from a user input handler",
> +            "The error is informative.");
> +
> +      browser.runtime.onMessage.addListener(async (msg) => {
> +        browser.test.assertEq(msg, "from-popup", "correct message received");
> +        await browser.tabs.remove(tabId);
> +        browser.test.sendMessage("popup");
> +      });
> +
> +      browser.test.sendMessage("pageAction.openPopup");
> +    });
> +    await browser.tabs.create({active: true, url: "tab.html"});

Same issues as above.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction.js:101
(Diff revision 3)
> +add_task(async function test_openPopup_requires_user_interaction() {
> +  async function backgroundScript() {
> +    browser.tabs.onUpdated.addListener(async (tabId, changeInfo, tabInfo) => {
> +      if (changeInfo.status != "complete") {
> +        return;
> +      }
> +
> +      await browser.test.assertRejects(
> +            browser.sidebarAction.toggleSidebar(),
> +            "sidebarAction.toggleSidebar may only be called from a user input handler",
> +            "The error is informative.");
> +
> +      browser.runtime.onMessage.addListener(async (msg) => {
> +        browser.test.assertEq(msg, "from-sidebar", "correct message received");
> +        await browser.tabs.remove(tabId);
> +        browser.test.sendMessage("sidebar");
> +      });
> +
> +      browser.test.sendMessage("sidebarAction.toggleSidebar");
> +    });
> +    await browser.tabs.create({active: true, url: "tab.html"});
> +  }
> +
> +  let extensionData = {
> +    background: backgroundScript,
> +    manifest: {
> +      "sidebar_action": {
> +        "default_panel": "sidebar.html",
> +      },
> +    },
> +
> +    files: {
> +      "tab.html": `
> +      <!DOCTYPE html>
> +      <html><head><meta charset="utf-8"></head><body>
> +      <button id="open">open</button>
> +      <script src="tab.js"></script>
> +      </body></html>
> +      `,
> +      "sidebar.html": `
> +      <!DOCTYPE html>
> +      <html><head><meta charset="utf-8"></head><body>
> +      <script src="sidebar.js"></script>
> +      </body></html>
> +      `,
> +      "tab.js": function() {
> +        document.getElementById("open").addEventListener("click", () => { // eslint-disable-line mozilla/balanced-listeners
> +          browser.sidebarAction.toggleSidebar();
> +        });
> +      },
> +      "sidebar.js": function() {
> +        browser.runtime.sendMessage("from-sidebar");
> +      },
> +    },
> +  };
> +
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);
> +  await extension.startup();
> +  await extension.awaitMessage("sidebarAction.toggleSidebar");
> +  let open = extension.awaitMessage("sidebar");
> +  await BrowserTestUtils.synthesizeMouseAtCenter("#open", {}, gBrowser.selectedBrowser);
> +  await open;
> +  await extension.unload();
> +});

Can you maybe coalesce these three test into a helper function that handles all three cases? This much duplicated code makes me itch...

::: browser/components/extensions/test/browser/browser_ext_sidebarAction.js:109
(Diff revision 3)
> +            browser.sidebarAction.toggleSidebar(),
> +            "sidebarAction.toggleSidebar may only be called from a user input handler",
> +            "The error is informative.");
> +
> +      browser.runtime.onMessage.addListener(async (msg) => {
> +        browser.test.assertEq(msg, "from-sidebar", "correct message received");
> +        await browser.tabs.remove(tabId);
> +        browser.test.sendMessage("sidebar");
> +      });
> +
> +      browser.test.sendMessage("sidebarAction.toggleSidebar");
> +    });
> +    await browser.tabs.create({active: true, url: "tab.html"});

And again.
Attachment #8845285 - Flags: review?(kmaglione+bmo) → review+
Will the openPopup and toggleSidebar functions be synchronous, or are they async? (If they are async, can we have them return a Promise for when the action is completed? (or a callback like the chrome version?)

For my use-case, I want to wait for the popup is open before sending it a message, so that would be necessary (and while sending messages back and forth would work, I think others would like this functionality too). Thanks for the progress! Waiting for it to land.
(In reply to greg.luto from comment #19)
> Will the openPopup and toggleSidebar functions be synchronous, or are they
> async? (If they are async, can we have them return a Promise for when the
> action is completed? (or a callback like the chrome version?)
> 
> For my use-case, I want to wait for the popup is open before sending it a
> message, so that would be necessary (and while sending messages back and
> forth would work, I think others would like this functionality too). Thanks
> for the progress! Waiting for it to land.

No, they don't resolve a promise on open.  The content of your panel can send a message when it's ready to receive.
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review173044

I have a few questions/suggestions that I'd like to get answered/addressed, but r=me if this is passing tests (which would confuse me).

::: browser/components/extensions/ext-browserAction.js:676
(Diff revision 3)
>          },
> +
> +        openPopup: function() {
> +          let window = windowTracker.topWindow;
> +          browserAction.triggerAction(window);
> +        },

I don't see a good reason why we shouldn't return the window?  Chrome does so, and both our schema and mdn already document it.

::: browser/components/extensions/ext-sidebarAction.js:385
(Diff revision 3)
>  
>            let panel = sidebarAction.getProperty(nativeTab, "panel");
>            return Promise.resolve(panel);
>          },
> +
> +        toggleSidebar: function() {

toggle or open?  Since sidebars can be opened per window, it might be hard for devs to keep track of the state (we don't even have the close event).

Also, agree that *actions should open attached to the focused window, but for sidebars, it might make sense to allow opening in other windows too?

::: browser/components/extensions/schemas/browser_action.json:410
(Diff revision 3)
>        },
>        {
>          "name": "openPopup",
>          "type": "function",
> -        "description": "Opens the extension popup window in the active window but does not grant tab permissions.",
> +        "description": "Opens the extension popup window in the active window.",
>          "unsupported": true,

no longer "unsupported", and shouldn't this include "requiresUserInput"?

::: browser/components/extensions/schemas/browser_action.json:421
(Diff revision 3)
>              "parameters": [
>                {
>                  "name": "popupView",
>                  "type": "object",
>                  "optional": true,
>                  "description": "JavaScript 'window' object for the popup window if it was succesfully opened.",

Please remove this if we decide not to return the `window`.

::: browser/components/extensions/schemas/page_action.json:224
(Diff revision 3)
> +      {
> +        "name": "openPopup",
> +        "type": "function",
> +        "requireUserInput": true,
> +        "description": "Opens the extension page action in the active window.",
> +        "async": "true",

I think we should keep the schemas consistent.

::: browser/components/extensions/test/browser/browser_ext_browserAction_simple.js:68
(Diff revision 3)
> +      await browser.test.assertRejects(
> +            browser.browserAction.openPopup(),

I don't actually understand what ensures that we throw here, since the schema doesn't require user input?

::: browser/components/extensions/test/browser/browser_ext_browserAction_simple.js:107
(Diff revision 3)
> +      <html><head><meta charset="utf-8"></head><body>
> +      <script src="popup.js"></script>
> +      </body></html>
> +      `,
> +      "tab.js": function() {
> +        document.getElementById("open").addEventListener("click", () => { // eslint-disable-line mozilla/balanced-listeners

nit: you can use the `{once: true}` option, so you wouldn't need the eslint exception.

::: browser/components/extensions/test/browser/browser_ext_browserAction_simple.js:130
(Diff revision 3)
> +  await extension.awaitMessage("browserAction.openPopup");
> +  let open = extension.awaitMessage("popup");
> +
> +  await BrowserTestUtils.synthesizeMouseAtCenter("#open", {}, gBrowser.selectedBrowser);
> +  await open;
> +  is(browserAction.tabManager.hasActiveTabPermission(gBrowser.selectedTab), true, "Active tab was granted permission");

I think it might be worth testing the full circle of active tab permissions instead of this.  For example, test calling some `tabs` method from the popup that requires the `activeTab` permission?
Attachment #8845285 - Flags: review?(tomica) → review+
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review173044

> I don't see a good reason why we shouldn't return the window?  Chrome does so, and both our schema and mdn already document it.

I don't see a good reason why we should return the window... This isn't a public Chrome API, so what they do shouldn't really have any bearing.
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review173044

> no longer "unsupported", and shouldn't this include "requiresUserInput"?

I updated the tree, must have lost the changes in browser_action.json.  Next patch will fix that.

> Please remove this if we decide not to return the `window`.

again, lost changes in schema

> I think we should keep the schemas consistent.

again, lost changes in schema

> I don't actually understand what ensures that we throw here, since the schema doesn't require user input?

again, lost changes in schema
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review173044

That confusion is due to the changes lost in browser_action.json
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review173044

> I don't see a good reason why we should return the window... This isn't a public Chrome API, so what they do shouldn't really have any bearing.

I also don't think we should bother returning window here.

> toggle or open?  Since sidebars can be opened per window, it might be hard for devs to keep track of the state (we don't even have the close event).
> 
> Also, agree that *actions should open attached to the focused window, but for sidebars, it might make sense to allow opening in other windows too?

For the first item...I'm going to think about that a bit.

As far as opening in other windows, the way it works for all sidebars is that only the current window is affected.  If you open a new window from a window with a sidebar, that sidebar is also open in the new window.
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review173044

> I think it might be worth testing the full circle of active tab permissions instead of this.  For example, test calling some `tabs` method from the popup that requires the `activeTab` permission?

This is how activeTab is tested in other tests.  I think it's sufficient.
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review173176

I'm a simple man, I see less code, I r+ ;)

::: browser/components/extensions/ext-sidebarAction.js:385
(Diff revision 4)
>  
>            let panel = sidebarAction.getProperty(nativeTab, "panel");
>            return Promise.resolve(panel);
>          },
> +
> +        toggleSidebar: function() {

nit: `toggleSidebar() {`

::: browser/components/extensions/test/browser/browser_ext_openPanel.js:27
(Diff revision 4)
> +        browser.sidebarAction.toggleSidebar(),
> +        "sidebarAction.toggleSidebar may only be called from a user input handler",
> +        "The error is informative.");
> +
> +      browser.runtime.onMessage.addListener(async (msg) => {
> +        if (msg === "done") {

this is not used

::: browser/components/extensions/test/browser/browser_ext_openPanel.js:98
(Diff revision 4)
> +  }
> +
> +  function testActiveTab(extension, expected) {
> +    const {GlobalManager, Management: {global: {browserActionFor}}} = Cu.import("resource://gre/modules/Extension.jsm", {});
> +    let ext = GlobalManager.extensionMap.get(extension.id);
> +    let browserAction = browserActionFor(ext);

I think `tabManager` is available directly from the `ext` here.
Attachment #8845285 - Flags: review+
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review173044

> For the first item...I'm going to think about that a bit.
> 
> As far as opening in other windows, the way it works for all sidebars is that only the current window is affected.  If you open a new window from a window with a sidebar, that sidebar is also open in the new window.

Toggle works as expected.  If no sidebar open, it opens the extensions sidebar.  If sidebar is open to something else, switches to extensions sidebar.  If extensions sidebar is open, closes sidebar.
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review173044

> Toggle works as expected.  If no sidebar open, it opens the extensions sidebar.  If sidebar is open to something else, switches to extensions sidebar.  If extensions sidebar is open, closes sidebar.

I don't doubt it works as expected, but I'm expecting that the most common use case would be "unconditionally show my sidebar", and doing that with `toggle` would require too much work to track state.
I've moved to sidebarAction.open/close with the last patch.
Comment on attachment 8845285 [details]
Bug 1341126 implement open for browser/page/sidebar actions,

https://reviewboard.mozilla.org/r/118454/#review173734

::: browser/components/extensions/ext-sidebarAction.js:316
(Diff revisions 5 - 6)
> -   * Triggers this sidebar action for the given window, with the same effects as
> +   * Opens this sidebar action for the given window.
> -   * if it were toggled via menu or toolbarbutton by a user.
>     *
>     * @param {ChromeWindow} window
>     */
> -  triggerAction(window) {
> +  open(window) {

err, `triggerAction` this is called from ext-commands and ext-menus.
Attachment #8845285 - Flags: review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5f476ca58313 -d eb60d0337b16: rebasing 413833:5f476ca58313 "Bug 1341126 implement open for browser/page/sidebar actions, r=kmag,zombie" (tip)
merging browser/base/content/browser-sidebar.js
warning: conflicts while merging browser/base/content/browser-sidebar.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc12e0c71046
implement open for browser/page/sidebar actions, r=kmag,zombie
https://hg.mozilla.org/mozilla-central/rev/cc12e0c71046
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
See Also: → 1278180
Will any of that work on Android?
Blocks: 1391289
(In reply to Richard Z. from comment #43)
> Will any of that work on Android?

Not yet.  Bug 1391289 is for that.
Duplicate of this bug: 1376156
Blocks: 1392624
Either I can't find it or this still needs documentation?
(In reply to Croydon from comment #46)
> Either I can't find it or this still needs documentation?

that's why there's a dev-doc-needed keyword.  It will be got to (or feel free to contribute on MDN), lots of docs to write I'm sure.
Sorry, I was blind :)
lgtm
Flags: needinfo?(mixedpuppy)
Is manual testing required on this bug? If yes, please provide some STR and the proper webextension(if required) or set the “qe-verify-“ flag.
Flags: needinfo?(mixedpuppy)
This has automated testing.
Flags: needinfo?(mixedpuppy)
Flags: qe-verify-
@wbamberg: description should mention that it does not work on android yet. Also, would it work to call openPopup() from an onclick() or similar handler? Asking because my usecase for this feature would be to avoid https://bugzilla.mozilla.org/show_bug.cgi?id=1287590
(In reply to Richard Z. from comment #53)
> @wbamberg: description should mention that it does not work on android yet.

It does: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserAction/openPopup#Browser_compatibility

> Also, would it work to call openPopup() from an onclick() or similar
> handler?

Not in a normal web page, because that doesn't have access to the browserAction API. From an extension page (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/user_interface/Bundled_web_pages), yes, but the docs note that already.
Clicking a menu item created in a background script with code like this:

    browser.menus.create({
        title   : 'Example',
        onclick : () => {
            browser.browserAction.openPopup();
        }
    });

results in the following error:

> Error: browserAction.openPopup may only be called from a user input handler

Is this a bug given that clicking a menu item is certainly a user action? Thanks.
Similarly, calling openPopup from a browser command callback results in the same error:


browser.commands.onCommand.addListener(command => {
    if (command === "open-the-popup") {
        browser.browserAction.openPopup();
    }
});

> Error: browserAction.openPopup may only be called from a user input handler

even though the onCommand callback is definitely a user action.
Blocks: 1438465
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.