verify set* functionality across page/browser/sidebar actions on desktop are consistent

RESOLVED FIXED in Firefox 59

Status

enhancement
P2
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: mixedpuppy, Assigned: Oriol)

Tracking

(Blocks 1 bug, {dev-doc-complete})

58 Branch
mozilla59
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

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

Comment 2

2 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
Comment hidden (mozreview-request)
Assignee

Comment 4

a year 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.
Assignee: nobody → oriol-bugzilla
Priority: -- → P2

Comment 5

a year 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?
Attachment #8936296 - Flags: review?(aswan)
Attachment #8936296 - Flags: review?(mixedpuppy)
Reporter

Updated

a year ago
Blocks: 1426484
Reporter

Updated

a year ago
Summary: verify set* functionality across page/browser/sidebar actions on desktop and mobile. → verify set* functionality across page/browser/sidebar actions on desktop are consistent
Reporter

Comment 6

a year 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?
Reporter

Comment 7

a year 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.
Attachment #8936296 - Flags: review?(mixedpuppy)
Assignee

Comment 8

a year 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.
Reporter

Comment 9

a year 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.
Flags: needinfo?(oriol-bugzilla)
Reporter

Comment 10

a year 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.
Assignee

Comment 11

a year ago
Posted image Empty panel.png
(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.
Flags: needinfo?(oriol-bugzilla)
Reporter

Comment 12

a year 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.
Assignee

Comment 13

a year 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.
Reporter

Comment 14

a year 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.
Assignee

Comment 15

a year 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.
Comment hidden (mozreview-request)
Assignee

Comment 17

a year 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
Comment hidden (mozreview-request)
Reporter

Comment 19

a year 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.
Comment hidden (mozreview-request)
Assignee

Comment 21

a year ago
Rebased patch.
Comment hidden (mozreview-request)
Reporter

Comment 23

a year 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.
Attachment #8936296 - Flags: review?(mixedpuppy)
Assignee

Comment 24

a year ago
But note sendMessage is

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

So `await sendMessage` works.
Reporter

Comment 25

a year 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.
Reporter

Updated

a year ago
Flags: needinfo?(oriol-bugzilla)
Assignee

Comment 26

a year 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
Flags: needinfo?(oriol-bugzilla)
Reporter

Comment 27

a year 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.
Reporter

Comment 28

a year 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
Attachment #8936296 - 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 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)
Flags: needinfo?(oriol-bugzilla)
Keywords: checkin-needed
Comment hidden (mozreview-request)
Assignee

Comment 31

a year ago
Rebased patch
Flags: needinfo?(oriol-bugzilla)
Keywords: checkin-needed

Comment 32

a year 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
Keywords: checkin-needed

Comment 33

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/43b467005ab2
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 34

a year 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.
Flags: needinfo?(oriol-bugzilla)
Assignee

Comment 35

a year ago
I guess the automated tests are enough.
Flags: needinfo?(oriol-bugzilla) → qe-verify-
Reporter

Comment 36

a year ago
The other day I noticed a blank browserAction icon on a test addon.  I need to look at that and see why.
Flags: needinfo?(mixedpuppy)
Assignee

Comment 38

a year 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 ?
Flags: needinfo?(oriol-bugzilla)
> 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.
Reporter

Updated

a year ago
See Also: → 1431943
Reporter

Updated

a year ago
Flags: needinfo?(mixedpuppy)

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.