1274559 - Type error for parameter options (Unexpected property "buttons") for notifications.create

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Julian]

Attachment: screenshot.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160519004038

Steps to reproduce:

My previously worked web extension:
https://addons.mozilla.org/de/firefox/addon/news-feed-for-github/
has stopped working. The plugin creates notifications, for example:

```
chrome.notifications.create(notifyPostID, {
	// ...
	"buttons": [{
		"title": `View ${author}`,
		"iconUrl": blobURL
	}]
});
```

Related to the docs
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Notifications/NotificationOptions
"buttons" should be optional and should not break notifications.




Actual results:

An error will be thrown (see subject for the message).


Expected results:

The notification should appear.

Comment 1

[u554753]

Hi julnot, 

Can you try your addon on the latest version of Firefox Nightly and see if you can reproduce this issue?

Comment 2

[Bob Silverberg [:bsilverberg]]

This looks to me like a valid bug. Comparing the docs for Chrome [1] with the schema for notifications [2] shows that our schema is missing an entry for the `buttons` property. The only thing that would make it invalid is if our schema parsing code silently ignores unwanted properties. Even if that is the case, this looks like something that should probably be fixed.

[1] https://developer.chrome.com/extensions/notifications#type-NotificationOptions
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/notifications.json#42

Comment 3

[Julian]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #1)
> Can you try your addon on the latest version of Firefox Nightly and see if
> you can reproduce this issue?

I've just tested it with the latest Nightly build and yes, I can reproduce it!

Comment 4

[Kris Maglione [:kmag]]

If we add a "buttons" property to the schema, it should just be a stub marked as unsupported. I don't think that silently ignoring the property is the right solution.

Comment 5

[Julian]

(In reply to Kris Maglione [:kmag] from comment #4)
> If we add a "buttons" property to the schema, it should just be a stub
> marked as unsupported. I don't think that silently ignoring the property is
> the right solution.

In fact this was the previous behavior. Firefox never supported the buttons property of notifications.create, instead silently skipped it. Whatever changed in the last days should either be undone or the feature should be implemented completely.

Comment 6

[Kris Maglione [:kmag]]

We only recently added argument type checking for this API.

We are planning to implement it completely, but in the mean time I don't think that silently creating broken notifications is the correct solution.

Comment 7

[Julian]

Well, the confusing thing here is not that fact that the notification is breaking. The confusing thing is that it is breaking, even if there is a documentation and it is valid. From my point of view it is a wrong behavior to just check for those things that are implemented and not for those things that are left to do but are available in the documentation.

In the meantime I think the correct way would be to also add the necessary checks for the buttons property and implement the feature itself afterwards. Just letting the siuation as it is now is definitely a problem.

Comment 8

[Julian]

Any update on this?

Comment 9

[Andy McKay]

(In reply to julmot from comment #8)
> Any update on this?

If there's any updates, they will be in the bug. People are busy, asking after two days won't help things.

Comment 10

[Bob Silverberg [:bsilverberg]]

I propose that we add the `buttons` property to the schema

Comment 11

[Bob Silverberg [:bsilverberg]]

Oops, I didn't mean to add that comment, only change the various statuses. My thought was not complete. What I meant to say was:

I propose we add the `buttons` property to the schema and mark it as unsupported. This way an exception will still be thrown when a user specifies the `buttons` property, but the message will be a bit more specific. The message will say:

`Type error for parameter options (Property "buttons" is unsupported by Firefox) for notifications.create`

I agree with Kris that this is more appropriate than simply ignoring the fact that `buttons` was provided because they are unsupported, and an extension that wishes to create buttons will also likely expect to be able to interact with those buttons (e.g., by listening for a click), but that isn't currently possible in Firefox.

I am going to attach a patch to the bug that implements this behaviour.

Comment 12

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1274559 - Type error for parameter options (Unexpected property "buttons") for notifications.create, r?kmag

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

Comment 13

[Julian]

> and an
> extension that wishes to create buttons will also likely expect to be able
> to interact with those buttons (e.g., by listening for a click), but that
> isn't currently possible in Firefox.

1. The property `buttons` is documentated and should work
2. The property `buttons` hasn't thrown an exception previously
3. Nothing will stop work when not throwing an exception, as the callback will always only be called when the user clicks on the button. As this is not guaranteed, no one expects this function to be called.

I'm sorry, but I'm not on your side. I don't expect anything from getting called, but I expect that the notification will appear without an error. There might be a warning to indicate that this isn't (yet) possible with Firefox, but not an error. There might be further developers implementing the Notifications API like me and you will break the code from working.

Also, I'd like to suggest adding such important information to the documentation.

Comment 14

[Bob Silverberg [:bsilverberg]]

(In reply to julmot from comment #13)
> > and an
> > extension that wishes to create buttons will also likely expect to be able
> > to interact with those buttons (e.g., by listening for a click), but that
> > isn't currently possible in Firefox.
> 
> 1. The property `buttons` is documentated and should work

There is a footnote in the documentation [1] that indicates that "Firefox only supports: type, iconUrl, title, message." Perhaps it would be better to call out each option that is not currently supported in the section for that option, rather than relying on a footnote.

> 2. The property `buttons` hasn't thrown an exception previously

This is because the input checking was not being done at all before. That was an oversight which has been fixed.

> 3. Nothing will stop work when not throwing an exception, as the callback
> will always only be called when the user clicks on the button. As this is
> not guaranteed, no one expects this function to be called.

This may be the case for your extension, but consider an extension that displays a notification which includes two buttons, "yes" and "no", and waits for a user to click on one of those buttons to proceed. That extension could be considered to be broken if no buttons are displayed to the user as the extension will wait forever for a user to click on one of the two buttons.

> 
> I'm sorry, but I'm not on your side. I don't expect anything from getting
> called, but I expect that the notification will appear without an error.
> There might be a warning to indicate that this isn't (yet) possible with
> Firefox, but not an error. There might be further developers implementing
> the Notifications API like me and you will break the code from working.
> 
> Also, I'd like to suggest adding such important information to the
> documentation.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Notifications/NotificationOptions#null-Firefox

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8758014 [details]
MozReview Request: Bug 1274559 - Type error for parameter options (Unexpected property "buttons") for notifications.create, r?kmag

https://reviewboard.mozilla.org/r/56374/#review53728

::: toolkit/components/extensions/test/mochitest/test_ext_notifications.html:201
(Diff revision 1)
> +    };
> +
> +    let exception = {};
> +    try {
> +      browser.notifications.create(opts).then(() => {
> +        browser.test.assertTrue(false, "notifications.create with buttons option threw an exception");

`browser.test.fail(...)`

However, this won't actually resolve until it's too late to have any effect, so you may as well remove it.

::: toolkit/components/extensions/test/mochitest/test_ext_notifications.html:208
(Diff revision 1)
> +    } catch (e) {
> +      exception = e;
> +    }
> +
> +    browser.test.assertTrue(
> +      exception.toString().includes('Property "buttons" is unsupported by Firefox'),

`String(exception)...`

Please use double quotes.

::: toolkit/components/extensions/test/mochitest/test_ext_notifications.html:221
(Diff revision 1)
> +      permissions: ["notifications"],
> +    },
> +    background: `(${backgroundScript})()`,
> +  });
> +  yield extension.startup();
> +  yield extension.awaitFinish();

Please pass the same string to `awaitFinish` as you pass to `notifyPass`.

Comment 16

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8758014 [details]
MozReview Request: Bug 1274559 - Type error for parameter options (Unexpected property "buttons") for notifications.create, r?kmag

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

Comment 17

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/56374/#review53728

> `String(exception)...`
> 
> Please use double quotes.

I used single quotes because the string I was testing against included double quotes and I thought it was ugly to escape them. But now I am escaping them.

Comment 18

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9881464e23db

Comment 19

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/56374/#review53728

> I used single quotes because the string I was testing against included double quotes and I thought it was ugly to escape them. But now I am escaping them.

Sorry, you're right, single quotes are preferred in that case.

Comment 20

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8758014 [details]
MozReview Request: Bug 1274559 - Type error for parameter options (Unexpected property "buttons") for notifications.create, r?kmag

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

Comment 21

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5432b4e159ab

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79031b5fc3ca
Type error for parameter options (Unexpected property "buttons") for notifications.create. r=kmag

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79031b5fc3ca

Comment 24

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/ebfe1808a42d152c40b2ad7bb2198bdf706e0686
Bug 1274559: Follow-up: Remove spurious CR characters. DONTBUILD

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ebfe1808a42d

Comment 26

[sankar88.aj]

Hello,
My extension also hit by this issue.
I'm unable to infer the result of the bug closure. Can you please see the latest attachment and let me know, if i'm using the API wrong. I've read documentation and followed the same. Yet I hit the same issue.

Comment 27

[sankar88.aj]

Attachment: Button property usage.png

Attachment to review the usage.

Comment 28

[sankar88.aj]

[Tracking Requested - why for this release]:

Because bug is reproducible in FF version 52. Refer the attached extension to reproduce the issue, which is sample implementation mentioned in documentation. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/notifications/create

Comment 29

[sankar88.aj]

Attachment: extension to reproduce bug

Comment 30

[Kris Maglione [:kmag]]

If you can still reproduce the issue, please open a new bug. This is not what tracking flags are for.

Comment 31

[sankar88.aj]

Ok. I'll open a new bug

Comment 32

[Bob Silverberg [:bsilverberg]]

I'm not sure if a new bug was opened for this or not, but in either case this is not a bug. Based on your screenshot the code is behaving as expected based on the discussions in this bug. If you specify a "buttons" property you should get an error stating that `Property "buttons" is unsupported by Firefox`, which is exactly what is happening.