1427431 - Add methods to check if browser/page/sidebar actions are enabled/shown/open

Status: VERIFIED FIXED

(WebExtensions :: Frontend, enhancement, P5)

Description

[Oriol Brufau [:Oriol]]

browser.browserAction has various get* methods to retrieve the title, badge, bgcolor, etc. But there is no way to know if the browserAction is enabled or not. So I propose a new method:

  browser.browserAction.isEnabled({tabId})

isEnabled would return a promise to be fulfilled with a boolean. tabId would be an optional parameter to check if the browserAction is specifically enabled in a tab. Omitting it would yield the global value.

Similarly for pageAction:

  browser.pageAction.isShown({tabId})

but here tabId wouldn't be optional because pageAction is always tab-specific.

And for sidebarAction:

  browser.sidebarAction.isOpen()

no parameter here because sidebarAction can only be opened or closed globally.

Comment 1

[Caitlin Neiman [:caitmuenster]]

Hi Oriol, this has been added to the agenda for the January 9, 2018 WebExtensions APIs triage meeting. Would you e able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/15JYw3L1490dKbr6yTLz1uirH8rfhcSiLJCubSWDlnfs/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

Comment 2

[Oriol Brufau [:Oriol]]

I think I will be able to join.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Do you expect sidebarAction.isOpen() to return true if the sidebar is open no matter what is in it, or only if the extensions sidebar is shown?

I'd also probably call that one isShown.

Comment 4

[Oriol Brufau [:Oriol]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> Do you expect sidebarAction.isOpen() to return true if the sidebar is open
> no matter what is in it, or only if the extensions sidebar is shown?

Not sure, I guess both might be useful. Maybe allow to specify it in a parameter?

> I'd also probably call that one isShown.

But note the method to open it is called `open`, not `show`.

Comment 5

[Croydon]

As the author of Vertical Tabs Reloaded I definitely need a function sidebarAction.isOpen() which returns true when the extension's own sidebar is open.

I'm getting regulary reports of people, who are assuming that VTR is completely broken, because they closed the sidebar (or it didn't open when installing the add-on) and now wondering where their vertical tabs are gone. Since automatic opening of sidebars got denied (for reasons I still can't accept, but that's another topic) I will not be able to fully do something against this anyway.

However, I offer a hotkey, a browserAction icon and users can also go the way over the generic sidebar icon and select Vertical Tabs Reloaded to display the vertical tabs. The hotkey and the browserAction icon is supposed to **toggle** the VTR sidebar. Toggling requires to know the current state and a function is missing for this. As a temporary workaround I'm just keeping track of hotkey and browserAction icon clicking/triggering and I'm trying to asume the inital state. This has many flaws, as anything else (like Firefox's generic sidebar icon) can't be tracked. So users have sometimes to click/press the keys twice before something happens (-> even more "add-on is broken" reports).

Alexander225 suggest some workaround (https://github.com/Croydon/vertical-tabs-reloaded/issues/134) but in the end of the day this should be simply a build-in function.

Comment 6

[Shane Caraveo (:mixedpuppy)]

We'll have to figure out some particular issues around the sidebar portion of this.  Windows can have different sidebars open (or none), and extensions sidebar may be open in one or more, but not all.  I'm also inclined towards only allowing an extension to know whether its sidebar is open or not rather than the general visibility of the sidebar ui.

Comment 8

[Oriol Brufau [:Oriol]]

Attachment: Bug 1427431 - Add methods to check if browser/page/sidebar actions are enabled/shown/open

Review commit: https://reviewboard.mozilla.org/r/212754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212754/

Comment 9

[Oriol Brufau [:Oriol]]

sidebarAction.isOpen() returns true if the sidebar is open and it is showing the panel specified by the extension in question, i.e.

    SidebarUI.isOpen && this.id == SidebarUI.currentID

This is the same condition checked by sidebarAction.close()

Does this sound good?

Comment 10

[Oriol Brufau [:Oriol]]

Did you discuss the sidebarAction method name in the meeting? I think isShown/show/hide might be better, but since we already have open/close, I used isOpen for consistency.

Comment 11

[Rémy Hubscher (:natim)]

> sidebarAction.isOpen() returns true if the sidebar is open and it is showing the panel specified by the extension in question

In the current windows right?

Comment 12

[Oriol Brufau [:Oriol]]

(In reply to Rémy Hubscher (:natim) from comment #11)
> In the current windows right?

In the window returned by Services.wm.getMostRecentWindow("navigator:browser"). This should be the latest focused normal window  (i.e excluding things like the browser console).

https://searchfox.org/mozilla-central/rev/137f1b2f434346a0c3756ebfcbdbee4069e15dc8/toolkit/components/extensions/ext-tabs-base.js#1285-1287

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8942462 [details]
Bug 1427431 - Add methods to check if browser/page/sidebar actions are enabled/shown/open

https://reviewboard.mozilla.org/r/212754/#review218902

looking goot, just a couple things to change.

::: browser/components/extensions/ext-browserAction.js:616
(Diff revision 1)
>  
> +        isEnabled: function(details) {
> +          let tab = getTab(details.tabId);
> +
> +          let enabled = browserAction.getProperty(tab, "enabled");
> +          return Promise.resolve(enabled);

returned results are wrapped internally, you shouldn't need the promise.resolve.

::: browser/components/extensions/ext-pageAction.js:279
(Diff revision 1)
>  
> +        isShown(details) {
> +          let tab = tabTracker.getTab(details.tabId);
> +
> +          let show = pageAction.getProperty(tab, "show");
> +          return Promise.resolve(show);

just "return pageAction.getProperty()" should work

::: browser/components/extensions/ext-sidebarAction.js:441
(Diff revision 1)
>          close() {
>            let window = windowTracker.topWindow;
>            sidebarAction.close(window);
>          },
> +
> +        isOpen() {

I think it is reasonable to allow an extension to provide an optional window argument here, default would be topWindow.

::: browser/components/extensions/ext-sidebarAction.js:443
(Diff revision 1)
>            sidebarAction.close(window);
>          },
> +
> +        isOpen() {
> +          let window = windowTracker.topWindow;
> +          return Promise.resolve(sidebarAction.isOpen(window));

You should be able to do this without promise.resolve

::: browser/components/extensions/schemas/browser_action.json:424
(Diff revision 1)
>        },
>        {
> +        "name": "isEnabled",
> +        "type": "function",
> +        "description": "Checks whether the browser action is enabled.",
> +        "async": "callback",

For new APIs we're not using callback, just use async: true and drop the callback param below.

::: browser/components/extensions/schemas/page_action.json:103
(Diff revision 1)
>        },
>        {
> +        "name": "isShown",
> +        "type": "function",
> +        "description": "Checks whether the page action is shown.",
> +        "async": "callback",

dito.

Comment 14

[Oriol Brufau [:Oriol]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> returned results are wrapped internally, you shouldn't need the
> promise.resolve.

OK, but all the existing get* methods use Promise.resolve

Comment 15

[Oriol Brufau [:Oriol]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> I think it is reasonable to allow an extension to provide an optional window
> argument here, default would be topWindow.

Seems reasonable, but then open() and close() methods should probably accept this parameter too. Seems useless to know whether the sidebar is open in another window if you can't open nor close it. But all of this is becoming a bit off-topic for this bug, so maybe do it somewhere else like bug 1390464 or a new one.

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to Oriol Brufau [:Oriol] from comment #15)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> > I think it is reasonable to allow an extension to provide an optional window
> > argument here, default would be topWindow.
> 
> Seems reasonable, but then open() and close() methods should probably accept
> this parameter too. Seems useless to know whether the sidebar is open in
> another window if you can't open nor close it. But all of this is becoming a
> bit off-topic for this bug, so maybe do it somewhere else like bug 1390464
> or a new one.

I'm much less inclined to do that.  IIRC open at least requires user input.  The intent is to match ui changes to user actions.  OTOH knowing if your sidebars are open is fine (and you could do that without this api anyway by sending msgs from the sidebar).

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to Oriol Brufau [:Oriol] from comment #14)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> > returned results are wrapped internally, you shouldn't need the
> > promise.resolve.
> 
> OK, but all the existing get* methods use Promise.resolve

They were probably created prior to the auto wrapping.

Comment 18

[Oriol Brufau [:Oriol]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> They were probably created prior to the auto wrapping.

OK, should I remove the existing Promise.resolve too?

Comment 19

[Oriol Brufau [:Oriol]]

Comment on attachment 8942462 [details]
Bug 1427431 - Add methods to check if browser/page/sidebar actions are enabled/shown/open

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212754/diff/1-2/

Comment 20

[Oriol Brufau [:Oriol]]

Comment on attachment 8942462 [details]
Bug 1427431 - Add methods to check if browser/page/sidebar actions are enabled/shown/open

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212754/diff/2-3/

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8942462 [details]
Bug 1427431 - Add methods to check if browser/page/sidebar actions are enabled/shown/open

https://reviewboard.mozilla.org/r/212754/#review219016

Assuming a clean try r+!  Thanks!

Comment 22

[Oriol Brufau [:Oriol]]

Only some unrelated intermittent failures, it seems.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7be74a94e766
Add methods to check if browser/page/sidebar actions are enabled/shown/open r=mixedpuppy

Comment 24

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7be74a94e766

Comment 25

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 26

[Oriol Brufau [:Oriol]]

Attachment: test.xpi

(In reply to marius.santa from comment #25)
> Is manual testing required on this bug? If Yes, please provide some STR and
> the proper webextension(if required), if No set the “qe-verify-“ flag.

Install this extension in about:debugging and follow the steps in the sidebar.

Comment 27

[marius.santa]

Attachment: action working.gif

I was able to reproduce this bug in Firefox 59.0a1(20171230220352).
Retested and verified the bug in Firefox 60.0a1(20180202102708) on Windows 10 64Bit and Mac OS  X 10.13.2.

Comment 28

[Will Bamberg [:wbamberg]]

Added:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction/isOpen
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/isShown
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserAction/isEnabled

Please let me know if that covers it.

Comment 29

[Oriol Brufau [:Oriol]]

Yes, I already added compat data and added links in browserAction, pageAction and sidebarAction pages, so that's all. Or maybe a link in https://developer.mozilla.org/en-US/Firefox/Releases/59#WebExtensions

For completeness I added a link to windows.WINDOW_ID_CURRENT in the isOpen page.