Closed Bug 1427431 Opened 2 years ago Closed 2 years ago

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

Categories

(WebExtensions :: Frontend, enhancement, P5)

enhancement

Tracking

(firefox59 verified)

VERIFIED FIXED
mozilla59
Tracking Status
firefox59 --- verified

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

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

Attachments

(3 files)

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.
Priority: -- → P5
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
I think I will be able to join.
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.
Flags: needinfo?(oriol-bugzilla)
(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`.
Flags: needinfo?(oriol-bugzilla)
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.
Whiteboard: [design-decision-needed] → [design-decision-approved]
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.
Duplicate of this bug: 1429054
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?
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
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.
> 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?
(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 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.
Attachment #8942462 - Flags: review?(mixedpuppy) → review-
(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
(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.
(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).
(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.
(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 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!
Attachment #8942462 - Flags: review?(mixedpuppy) → review+
Only some unrelated intermittent failures, it seems.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7be74a94e766
Add methods to check if browser/page/sidebar actions are enabled/shown/open r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7be74a94e766
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Keywords: dev-doc-needed
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.
Flags: needinfo?(oriol-bugzilla)
Attached file 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.
Flags: needinfo?(oriol-bugzilla)
Attached image 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.
Status: RESOLVED → VERIFIED
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.
Flags: needinfo?(oriol-bugzilla)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.