Shane Caraveo (:mixedpuppy) Reporter
	Description • 7 years ago

Bug 1419940 made some changes to desktop browserAction.  we should probably make sure these api's remain consistent.

	Oriol Brufau [:Oriol] Assignee
	Comment 2 • 7 years ago

This was my attempt, but the tests time out, and I don't have any android device to debug.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a36c6dc127a6ac5d45acaea299191300b70a813e

	Oriol Brufau [:Oriol] Assignee
	Comment 4 • 7 years ago

This makes pageAction and sidebarAction consistent with browserAction in desktop. But note that pageAction methods require a tabId, I didn't change this. And as I said I can't do it for mobile.

	Andrew Swan [:aswan]
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8936296 [details]
Bug 1424538 - Allow pageAction and sidebarAction set* methods to accept a null value (desktop).

https://reviewboard.mozilla.org/r/207032/#review214558

Sorry it took so long to get to this, since I don't know the sidebar action code and tests at all, I'm redireting to Shane.
Also, the Android question needs to be addressed, if Shane is okay landing this without Android support then we need another bug for Android.
You don't actually need an Android device to run tests on Fennec, you can use the Android emulator...

::: browser/components/extensions/test/browser/browser_ext_pageAction_context.js:66
(Diff revision 2)
>           "popup": browser.runtime.getURL("default.html"),
>           "title": "Default T\u00edtulo \u263a"},
>          {"icon": browser.runtime.getURL("2.png"),
>           "popup": browser.runtime.getURL("2.html"),
>           "title": "Title 2"},
> -        {"icon": browser.runtime.getURL("2.png"),
> +        {"icon": contextUri,

I'm not very familiar with these tests but this doesn't look right, why is the icon `generated_background_page` here?

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8936296 [details]
Bug 1424538 - Allow pageAction and sidebarAction set* methods to accept a null value (desktop).

https://reviewboard.mozilla.org/r/207032/#review214580

This all looks ok, just the question on using the background page url and setting an empty panel or popup shouldn't be possible but it looks like that test is removed.

::: browser/components/extensions/test/browser/browser_ext_pageAction_context.js:66
(Diff revision 2)
>           "popup": browser.runtime.getURL("default.html"),
>           "title": "Default T\u00edtulo \u263a"},
>          {"icon": browser.runtime.getURL("2.png"),
>           "popup": browser.runtime.getURL("2.html"),
>           "title": "Title 2"},
> -        {"icon": browser.runtime.getURL("2.png"),
> +        {"icon": contextUri,

I also don't understand this one...

::: browser/components/extensions/test/browser/browser_ext_sidebarAction.js:37
(Diff revision 2)
>    },
>  
>    background: function() {
> -    browser.test.onMessage.addListener(msg => {
> +    browser.test.onMessage.addListener(async msg => {
>        if (msg === "set-panel") {
> -        browser.sidebarAction.setPanel({panel: ""}).then(() => {
> +        await browser.sidebarAction.setPanel({panel: null});

We still should not be able to set panel:"".  I'm fine if it reverts to default same as using null.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js:342
(Diff revision 2)
> -         "icon": browser.runtime.getURL("icon.png")},
>        ];
>  
>        return [
>          async expect => {
> -          browser.test.log("Initial state. Expect extension title as default title.");
> +          browser.test.log("Initial state. Expect extension title as global title.");

I dont think default->global changes here were necessary, but ok.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js:426
(Diff revision 2)
> +         "panel": browser.runtime.getURL("p1.html"),
> +         "title": "t1"},
> +        {"icon": browser.runtime.getURL("i2.png"),
> +         "panel": browser.runtime.getURL("p2.html"),
> +         "title": "t2"},
> +        {"icon": contextUri,

contextUri again, why are we using the generated background page url?

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8936296 [details]
Bug 1424538 - Allow pageAction and sidebarAction set* methods to accept a null value (desktop).

https://reviewboard.mozilla.org/r/207032/#review214588

clearing flag so I'll see the next request.

	Oriol Brufau [:Oriol] Assignee
	Comment 8 • 7 years ago

The generated_background_page thing is because "" is no longer treated in a special way. Then it's just a relative uri that points to the current script, which is generated_background_page.
This is consistent with what I did in bug 1419940, but maybe returning "" would be better?

> We still should not be able to set panel: "".

Why not? It didn't make sense to me to allow "" in a tab but not globally.

> I'm fine if it reverts to default same as using null.

I think it should be possible to have a default_panel in the manifest and then in some circumstance set the global panel to "" in order to remove all panels except in the tabs that have a specific one.

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 9 • 7 years ago

(In reply to Oriol Brufau [:Oriol] from comment #8)
> The generated_background_page thing is because "" is no longer treated in a
> special way. Then it's just a relative uri that points to the current
> script, which is generated_background_page.
> This is consistent with what I did in bug 1419940, but maybe returning ""
> would be better?
> 
> > We still should not be able to set panel: "".
> 
> Why not? It didn't make sense to me to allow "" in a tab but not globally.
> 
> > I'm fine if it reverts to default same as using null.
> 
> I think it should be possible to have a default_panel in the manifest and
> then in some circumstance set the global panel to "" in order to remove all
> panels except in the tabs that have a specific one.

What is the behavior when the panel is ""?  Does the button disappear, or does it try to load "moz-extension://uuid/"?  I don't think we want either of those behaviors.

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 10 • 7 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> (In reply to Oriol Brufau [:Oriol] from comment #8)

> What is the behavior when the panel is ""?  Does the button disappear, or
> does it try to load "moz-extension://uuid/"?  I don't think we want either
> of those behaviors.

Button above would be in reference to page/browser actions.  It feels less clear what this means when it comes to the sidebar.

	Oriol Brufau [:Oriol] Assignee
	Comment 11 • 7 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> What is the behavior when the panel is ""?  Does the button disappear, or
> does it try to load "moz-extension://uuid/"?  I don't think we want either
> of those behaviors.

Currently, browser.sidebarAction.setPanel({tabId, panel: ""}) removes the tab-specific panel so that the global value is used instead. With my patch this behavior can be achieved with browser.sidebarAction.setPanel({tabId, panel: null}), and browser.sidebarAction.setPanel({tabId, panel: ""}) just shows a blank panel (see attached screenshot).

browser.browserAction.setPopup({tabId, popup: ""}) disables the pop-up in that tab, clicking the browserAction does nothing. This behavior precedes bug 1419940.

Similarly, browser.pageAction.setPopup({tabId, popup: ""}) disables the pop-up in that tab, clicking the pageAction does nothing.

Note that default_popup in the manifest is optional both in browser_action and page_action. I think default_panel should also be optional, default to "", and allow to use browser.sidebarAction.setPanel with panel:"" both globally or in a tab.

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 12 • 7 years ago

This is a quick brain dump of how I see the apis working.

A sidebar without a panel makes no sense and could potentially lead to users getting blank sidebars.  So, default_panel should not be optional and should never be an empty string.  setPanel allows to either use a different panel per tab or revert to the default.  In this case, empty string and null should behave the same.

page/browser actions, if no default_popup is specified, browser/pageAction.onClicked listeners will work.  If default_popup is specified, onClicked will never work (desired behavior), so default_panel cannot default to an empty string.  It must either not be set or must have a relative url.

page/browser actions should never resolve to an empty panel.

For all purposes with popup/panel, I believe that empty string and null should work the same, which is to revert to the default set in the manifest.  Right now it appears that should be the case.

Optionally, and we would need to think about the use case, null could set browser/page actions to a state where onClicked handlers could work, while empty string would revert to defaults from the manifest (or visa-verse), but we need to ensure compatibility with chrome here. e.g. if chrome accepts empty string and/or null to revert to default we must match that behavior, compatibility on this item is important.  If one or the other is not accepted by chrome at all, we can document a behavior difference in that case.

	Oriol Brufau [:Oriol] Assignee
	Comment 13 • 7 years ago

On Chrome, an empty string popup just disables it instead of reverting to default. So the value that reverts to default should be null indeed (null throws on Chrome).

I don't see why webextensions should not be allowed to create blank sidebars, but I can't think any use-case, so OK. But I think it will be a bit inconsistent if the empty string reverts some properties to default but not others.

But if you don't want blank sidebars I suspect you won't like blank browser or page icons. So what should setIcon({path: ""}) do?

 - Be treated as a relative URI that resolves to the current script. This will be an invalid image, so no icon. This is what I did in bug 1419940.
 - Keep it as "", then CSS will resolve it to the URL of the stylesheet. This will be an invalid image, so no icon.
 - Treat it as null, i.e. revert a tab-specific icon to the global one, or the global one to the default one.
 - Use the fallback icon provided by Firefox.

Note that there is an use-case for hiding the icon and just showing a badge: https://addons.mozilla.org/en-US/firefox/addon/tab-counter/

Chrome warns "Could not load action icon ''." and maintains the old icon, but Firefox always replaces the old icon, even if the new one can't be loaded (not just for the empty string). So if setIcon({path: "there-is-no-such-file"}) hides the icon on Firefox, I think setIcon({path: ""}) should too.

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 14 • 7 years ago

Unfortunately we're out until January at this point, so my responses will be short and possibly not well thought out, but I do want to be sure we're clear on the functionality we want the API to have.

When you say "disables it", what does that actually mean?  Is the button grey'd out and unclickable, or is it clickable but no panel appears?  If the later, does the onClicked listener work in that case?  If the listener does work I'm ok with that behavior, it provides a way an extension can change from using a panel to using the onClicked handler.

Regarding blank panels.  I don't know we need to promise a path exists, but unless there is a solid use case for a blank panel, I don't think it's good ux and likely appears as a bug to the user.  So I'd generally be against the api providing an obvious way to get a blank panel (in any of the three).

Regarding blank icons.  I also don't think we should end up in a situation where there is no icon.  Putting aside the potential use case of a badge but no icon, a blank button again looks like a bug rather than a feature.  If the extension chooses to use no icon, we should by default show the default firefox extension icon (puzzle piece) or the extensions default icon.

	Oriol Brufau [:Oriol] Assignee
	Comment 15 • 7 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> When you say "disables it", what does that actually mean?  Is the button
> grey'd out and unclickable, or is it clickable but no panel appears?  If the
> later, does the onClicked listener work in that case?  If the listener does
> work I'm ok with that behavior, it provides a way an extension can change
> from using a panel to using the onClicked handler.

Exactly, empty string popup shows no popup but the icon is clickable and the onClicked listener works.

> Regarding blank panels.  I don't know we need to promise a path exists, but
> unless there is a solid use case for a blank panel, I don't think it's good
> ux and likely appears as a bug to the user.  So I'd generally be against the
> api providing an obvious way to get a blank panel (in any of the three).

I think the behavior when a non-existing URL is specified appears much more buggy, but OK.

> Regarding blank icons.  I also don't think we should end up in a situation
> where there is no icon.  Putting aside the potential use case of a badge but
> no icon, a blank button again looks like a bug rather than a feature.  If
> the extension chooses to use no icon, we should by default show the default
> firefox extension icon (puzzle piece) or the extensions default icon.

OK, I will do this for empty string paths, but the icon will still be blank if a non-existing path is used.

	Oriol Brufau [:Oriol] Assignee
	Comment 17 • 7 years ago

So now an empty string panel will behave like null, i.e. revert a tab-specific one to the global one, or revert the global one to the default one.

And an empty string icon path will show chrome://browser/content/extension.svg

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 19 • 7 years ago

(In reply to Oriol Brufau [:Oriol] from comment #15)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> > When you say "disables it", what does that actually mean?  Is the button
> > grey'd out and unclickable, or is it clickable but no panel appears?  If the
> > later, does the onClicked listener work in that case?  If the listener does
> > work I'm ok with that behavior, it provides a way an extension can change
> > from using a panel to using the onClicked handler.
> 
> Exactly, empty string popup shows no popup but the icon is clickable and the
> onClicked listener works.

Cool.

> > Regarding blank panels.  I don't know we need to promise a path exists, but

> I think the behavior when a non-existing URL is specified appears much more
> buggy, but OK.

> > Regarding blank icons.  I also don't think we should end up in a situation

> OK, I will do this for empty string paths, but the icon will still be blank
> if a non-existing path is used.

I think it's fine that non-existing paths break, that is an obvious bug and not something intentional.

	Oriol Brufau [:Oriol] Assignee
	Comment 21 • 7 years ago

Rebased patch.

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8936296 [details]
Bug 1424538 - Allow pageAction and sidebarAction set* methods to accept a null value (desktop).

https://reviewboard.mozilla.org/r/207032/#review219766

::: browser/components/extensions/test/browser/browser_ext_sidebarAction.js
(Diff revisions 5 - 6)
>    await extension.startup();
>    // Test sidebar is opened on install
>    await extension.awaitMessage("sidebar");
>    ok(!document.getElementById("sidebar-box").hidden, "sidebar box is visible in first window");
>    await sendMessage(extension, "set-panel");
> -  await extension.awaitFinish();

Shouldn't need await for sendMessage. We still need to wait for something here.  I'd use extension.awaitFinish("sidebar-ok") where you removed the awaitFinish, and add a browser.test.notifyPass("sidebar-ok") to the "set-panel" section of the background script after the assert.

	Oriol Brufau [:Oriol] Assignee
	Comment 24 • 7 years ago

But note sendMessage is

  async function sendMessage(ext, msg, data = undefined) {
    ext.sendMessage({msg, data});
    await ext.awaitMessage("done");
  }

So `await sendMessage` works.

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 25 • 7 years ago

(In reply to Oriol Brufau [:Oriol] from comment #24)
> But note sendMessage is
> 
>   async function sendMessage(ext, msg, data = undefined) {
>     ext.sendMessage({msg, data});
>     await ext.awaitMessage("done");
>   }
> 
> So `await sendMessage` works.

Ahh, yes, sorry about that.  Looking over the entire patch again since it's been a while, but I think this is good.  The behavior change is buried in our conversation above, could you verify my understanding?  For page/browser actions, chrome compatibility is important so I want to think about any differences before giving a final r+.

browser/pageAction

- setIcon
  chrome: "" results in warning, leaving prior icon, null throws
  firefox old behavior: "" unsets tab-specific icon, reverting to manifest icon.
  firefox new behavior: "" unsets tab-specific icon, reverting to manifest icon, null revert to global (if tab-specific set) or default in manifest.
- setPopup
  on chrome: "" disables, null throws
  firefox old behavior: "" reverts to default in manifest.
  firefox new behavior: "" unsets popup and allows onClicked to be used, null reverts to global or default in manifest, depending on whether tab-specific was set prior.

sidebar

- setIcon
  chrome: "" results in warning, leaving prior icon, null throws
  firefox old behavior: "" unsets tab-specific icon, reverting to manifest icon.
  firefox new behavior: "" unsets tab-specific icon, reverting to manifest icon, null revert to global (if tab-specific set) or default in manifest.
- setPopup
  on chrome: "" disables, null throws
  firefox old behavior: "" reverts to default in manifest.
  firefox new behavior: "" and null revert to global (if tab-specific set) or default in manifest.

	Oriol Brufau [:Oriol] Assignee
	Comment 26 • 7 years ago

browser/pageAction

- setIcon
  - chrome:
    - "" results in warning, leaving prior icon, the callback is not called.
    - Same for paths that don't contain an image.
    - null throws
  - firefox before bug 1419940
    - "" shows chrome://browser/content/extension.svg
    - Paths that don't contain an image hide the icon
    - null shows shows chrome://browser/content/extension.svg
  - firefox current behavior
    - "" hides the icon
    - Paths that don't contain an image hide the icon
    - For browserAction, null reverts a tab-specific icon to the global one, or a global one to the manifest one
      For pageAction, null shows chrome://browser/content/extension.svg
  - firefox new behavior
    - "" shows chrome://browser/content/extension.svg
    - Paths that don't contain an image hide the icon
    - null reverts a tab-specific icon to the global one, or a global one to the manifest one

- setPopup
  - chrome:
    - "" disables popup and allows onClicked
    - null throws
  - firefox browserAction before bug 1419940, pageAction current behavior
    - "" disables popup and allows onClicked
    - null throws
  - firefox browserAction current behavior, pageAction new behavior
    - "" disables popup and allows onClicked
    - null reverts a tab-specific popup to the global one, or a global one to the manifest one

sidebarAction

- setIcon
  - chrome: not supported
  - firefox before bug 1419940
    - "" shows chrome://browser/content/extension.svg
    - Paths that don't contain an image hide the icon
    - null shows chrome://browser/content/extension.svg
  - firefox current behavior
    - "" hides the icon
    - Paths that don't contain an image hide the icon
    - null shows chrome://browser/content/extension.svg
  - firefox new behavior
    - "" shows chrome://browser/content/extension.svg
    - Paths that don't contain an image hide the icon
    - null reverts a tab-specific icon to the global one, or a global one to the manifest one

- setPanel
  - chrome: not supported
  - firefox current behavior
    - Global "" rejects the promise, leaving prior panel
      Tab-specific "" reverts it to global panel
    - null throws
  - firefox new behavior
    - "" reverts a tab-specific panel to the global one, or a global one to the manifest one
    - Same for null

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 27 • 7 years ago
mozreview-review-reply

Comment on attachment 8936296 [details]
Bug 1424538 - Allow pageAction and sidebarAction set* methods to accept a null value (desktop).

https://reviewboard.mozilla.org/r/207032/#review219766

> Shouldn't need await for sendMessage. We still need to wait for something here.  I'd use extension.awaitFinish("sidebar-ok") where you removed the awaitFinish, and add a browser.test.notifyPass("sidebar-ok") to the "set-panel" section of the background script after the assert.

my mistake.

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8936296 [details]
Bug 1424538 - Allow pageAction and sidebarAction set* methods to accept a null value (desktop).

https://reviewboard.mozilla.org/r/207032/#review219868

	Mozilla Autoland
	Comment 29 • 7 years ago

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 4081a5a3fd7bc471fdb9ccd5aec6edb4337f8f08 -d 1578425fbe9f: rebasing 443215:4081a5a3fd7b "Bug 1424538 - Allow pageAction and sidebarAction set* methods to accept a null value (desktop). r=mixedpuppy" (tip)
merging browser/components/extensions/test/browser/browser_ext_browserAction_context.js
warning: conflicts while merging browser/components/extensions/test/browser/browser_ext_browserAction_context.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Oriol Brufau [:Oriol] Assignee
	Comment 31 • 7 years ago

Rebased patch

	Pulsebot
	Comment 32 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/43b467005ab2
Allow pageAction and sidebarAction set* methods to accept a null value (desktop). r=mixedpuppy

	Raul Gurzau (:RaulG)
	Comment 33 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/43b467005ab2

	marius.santa
	Comment 34 • 7 years ago

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.

	Oriol Brufau [:Oriol] Assignee
	Comment 35 • 7 years ago

I guess the automated tests are enough.

	Shane Caraveo (:mixedpuppy) Reporter
	Comment 36 • 7 years ago

The other day I noticed a blank browserAction icon on a test addon.  I need to look at that and see why.

	Will Bamberg [:wbamberg]
	Comment 37 • 7 years ago

I've updated some pages, please let me know if this covers it:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/setIcon
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/setPopup
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction/setPanel
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction/setIcon
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction/setTitle

tbh I struggled a bit with how to describe it all clearly.

	Oriol Brufau [:Oriol] Assignee
	Comment 38 • 7 years ago

Yes, I think that's all, thanks. I just added some "or null" to the lists of parameter types.
Should this appear in https://developer.mozilla.org/en-US/Firefox/Releases/59#WebExtensions ?

	Will Bamberg [:wbamberg]
	Comment 39 • 7 years ago

> Should this appear in https://developer.mozilla.org/en-US/Firefox/Releases/59#WebExtensions ?

Yes, it should! Usually I update this page last when I've finished all the docs.