Allow WebExtensions to disable Web API notifications

RESOLVED FIXED in Firefox 58

Status

enhancement
P2
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

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

unspecified
mozilla58
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: triaged[browserSettings])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
This will use browserSetting [1] to manage which extension has control over the pref. This does not really belong in privacy so will go into the browserSettings API.

According to the notes in [2], this would make use of the `dom.webnotifications.enabled` although I am not sure about that. Toggling that setting does not affect notifications generated via the WebExtensions notifications API, but perhaps those are not expected to be affected. Perhaps this is only for Web API notifications.

I noticed that there is another pref called `notification.feature.enabled`, but I am not sure what it does and it defaults to false. I tested to see if toggling it affects WebExtensions API notifications, and it does not.

More investigation is required, but both of those prefs are booleans, so when we do figure out the implementation it can be a boolean called browser.browserSettings.desktopNotificationsEnabled.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy/BrowserSetting
[2] https://docs.google.com/document/d/1HZiI-dqFATzLpmTtN1Bg6tD8y4N0Ne3D74LEWWgttOk/edit#
(Assignee)

Comment 1

2 years ago
I did some more testing and verified that setting `dom.webnotifications.enabled` to false does disable Web API notifications but not WebExtensions API notifications. If that is sufficient to satisfy this requirement then we can go ahead with that. Rob, can you confirm that?
Flags: needinfo?(rob)

Comment 2

2 years ago
(In reply to Bob Silverberg [:bsilverberg] from comment #1)
> I did some more testing and verified that setting
> `dom.webnotifications.enabled` to false does disable Web API notifications
> but not WebExtensions API notifications. If that is sufficient to satisfy
> this requirement then we can go ahead with that. Rob, can you confirm that?

I haven't tested, but what you've described sounds good.
Flags: needinfo?(rob)
(Assignee)

Comment 3

2 years ago
Because this only controls Web API notifications, I am going to name the setting `webAPINotificationsEnabled`.
(Assignee)

Comment 4

2 years ago
Kit, could you please confirm whether flipping the `dom.webnotifications.enabled` is an acceptable approach for an API that would allow a WebExtension to disable and enable Web API notifications?
Flags: needinfo?(kit)
Summary: Allow WebExtensions to disable desktop notifications → Allow WebExtensions to disable Web API notifications
TL;DR: For now, probably.

You'll probably also want to disable "dom.webnotifications.serviceworker.enabled", but be careful. Flipping the pref will hide the `Notification` constructor and `serviceWorkerRegistration.showNotification()` function, so scripts that don't feature detect will throw a `ReferenceError` or a `TypeError`. Is that OK for now?

We'll probably want to suppress delivering `push` events to service workers, too. Otherwise, we'll fire the event, the worker won't be able to show a notification, and we'll eventually revoke the site's push subscription because it sent too many silent pushes...even though it tried to do the right thing! (I think "do not disturb" has the same problem). There's an undocumented "dom.push.testing.notifyWorkers" pref that you can set to false to suppress this; we should probably make it public, if we go this route.

I think the correct long-term approach is to let you set permissions globally, though. All Web API notifications are controlled by the "desktop-notification" permission, so disabling it for all sites will do the right thing, without needing to flip a matrix of prefs. That's bug 1275599.
Flags: needinfo?(kit)
(Assignee)

Comment 6

2 years ago
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #5)

Thanks for the detailed response, Kit. 

> TL;DR: For now, probably.
> 
> You'll probably also want to disable
> "dom.webnotifications.serviceworker.enabled", but be careful. Flipping the
> pref will hide the `Notification` constructor and
> `serviceWorkerRegistration.showNotification()` function, so scripts that
> don't feature detect will throw a `ReferenceError` or a `TypeError`. Is that
> OK for now?
> 

I noticed in my manual testing that, even with only `dom.webnotifications.enabled` set to "false" the `Notification` constructor is no longer available and I was getting errors when trying to create a notification. I'm not sure if you're suggesting that would only happen if I also set `dom.webnotifications.serviceworker.enabled` to false, or if I am reading that incorrectly. In any case that is a potential issue.

Rob: do you think that is an acceptable result if developers use this API to disable notifications? 

> We'll probably want to suppress delivering `push` events to service workers,
> too. Otherwise, we'll fire the event, the worker won't be able to show a
> notification, and we'll eventually revoke the site's push subscription
> because it sent too many silent pushes...even though it tried to do the
> right thing! (I think "do not disturb" has the same problem). There's an
> undocumented "dom.push.testing.notifyWorkers" pref that you can set to false
> to suppress this; we should probably make it public, if we go this route.
> 

So is the only option right now to suppress delivering `push` events to service workers setting the "dom.push.testing.notifyWorkers" pref to false? I'm not sure what an undocumented pref is vs. a public one? Can I still use that pref in our API code regardless of whether it is public or not, or does something need to be done to make it public?

> I think the correct long-term approach is to let you set permissions
> globally, though. All Web API notifications are controlled by the
> "desktop-notification" permission, so disabling it for all sites will do the
> right thing, without needing to flip a matrix of prefs. That's bug 1275599.

That does sound like a good approach. Bug 1275599 looks like it's about creating a UI to allow users to better control permissions on a site-by-site level, so is it also about controlling permissions globally? That is what this bug would need. Is that something we can do now, without a UI, or is it expected that the ability to control permissions globally is something that will be included in bug 1275599?
Flags: needinfo?(rob)
Flags: needinfo?(kit)
(In reply to Bob Silverberg [:bsilverberg] from comment #6)
> I noticed in my manual testing that, even with only
> `dom.webnotifications.enabled` set to "false" the `Notification` constructor
> is no longer available and I was getting errors when trying to create a
> notification. I'm not sure if you're suggesting that would only happen if I
> also set `dom.webnotifications.serviceworker.enabled` to false...

Yes. Service workers don't use the `Notification` constructor; instead, they have `serviceWorkerRegistration.showNotification()`. Disabling `dom.webnotifications.serviceworker.enabled` will hide that method from service workers. `dom.webnotifications.enabled` only controls the `Notification` constructor, not `showNotification`.

> So is the only option right now to suppress delivering `push` events to
> service workers setting the "dom.push.testing.notifyWorkers" pref to false?

Yes. You can definitely use the pref in your API code; I only meant undocumented in the sense that it doesn't have a default value, and it's only used in the push tests.

> Is that something we can do now, without a UI, or is it
> expected that the ability to control permissions globally is something that
> will be included in bug 1275599?

You're right, I think it only covers the UI part. :MattN suggested extending the permission manager to do this in bug 1313939, comment 5, but I'm not sure if that went anywhere. We can always add another pref to suppress notifications and `push` events (but keep `new Notification()` and `.showNotification()` exposed) for now, and remove it if and when we can deny permissions globally.
Flags: needinfo?(kit)
(Assignee)

Comment 8

2 years ago
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #7)
> 
> Yes. Service workers don't use the `Notification` constructor; instead, they
> have `serviceWorkerRegistration.showNotification()`. Disabling
> `dom.webnotifications.serviceworker.enabled` will hide that method from
> service workers. `dom.webnotifications.enabled` only controls the
> `Notification` constructor, not `showNotification`.
>

Thanks for the clarification. So I could set these prefs, but we'll still have the problem of this potentially breaking web sites.
 
> > So is the only option right now to suppress delivering `push` events to
> > service workers setting the "dom.push.testing.notifyWorkers" pref to false?
> 
> Yes. You can definitely use the pref in your API code; I only meant
> undocumented in the sense that it doesn't have a default value, and it's
> only used in the push tests.
> 

Ok, that sounds like a workable solution for the push case.

> > Is that something we can do now, without a UI, or is it
> > expected that the ability to control permissions globally is something that
> > will be included in bug 1275599?
> 
> You're right, I think it only covers the UI part. :MattN suggested extending
> the permission manager to do this in bug 1313939, comment 5, but I'm not
> sure if that went anywhere. We can always add another pref to suppress
> notifications and `push` events (but keep `new Notification()` and
> `.showNotification()` exposed) for now, and remove it if and when we can
> deny permissions globally.

It does sound like having a pref to disable notifications without removing the exposed functions is something we'd need in order to not break web sites. How difficult would that be to do?

It also sounds like the best long term solution is to deal with this via a global permission, rather than prefs. I gather from your last sentence that we have no way, via code, to currently disable a permission globally - is that correct? From a WebExtensions API perspective we don't really need there to be a UI, we could control all of this through code. Although that raises the issue of how a user would re-enable notifications themselves if an extension disabled them. Currently that would work if either the extension provided a way of re-enabling them or it would happen automatically if the extension was disabled or uninstalled.

This isn't particularly urgent, so maybe it's better to wait to address it with permissions, rather than add a new pref as discussed above. What do you think? Are either of those things likely to happen before 57?

Comment 9

2 years ago
(In reply to Bob Silverberg [:bsilverberg] from comment #6)
> (In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #5)
> 
> Thanks for the detailed response, Kit. 
> 
> > TL;DR: For now, probably.
> > 
> > You'll probably also want to disable
> > "dom.webnotifications.serviceworker.enabled", but be careful. Flipping the
> > pref will hide the `Notification` constructor and
> > `serviceWorkerRegistration.showNotification()` function, so scripts that
> > don't feature detect will throw a `ReferenceError` or a `TypeError`. Is that
> > OK for now?
> > 
> 
> I noticed in my manual testing that, even with only
> `dom.webnotifications.enabled` set to "false" the `Notification` constructor
> is no longer available and I was getting errors when trying to create a
> notification. I'm not sure if you're suggesting that would only happen if I
> also set `dom.webnotifications.serviceworker.enabled` to false, or if I am
> reading that incorrectly. In any case that is a potential issue.
> 
> Rob: do you think that is an acceptable result if developers use this API to
> disable notifications?

I think that it is better to have a preference that causes Notification.requestPermission to fail, opposed to hiding the API.
If that is not feasible, then hiding the related APIs is acceptable.
Flags: needinfo?(rob)
(Assignee)

Comment 10

2 years ago
Kit, I neglected to needinfo you on comment #8, which includes some followup questions for you. I would appreciate it if you could take a look and reply when you have the time.
Flags: needinfo?(kit)
Ack, I'm sorry I missed your comment, Bob. After thinking some more about this, I'm not sure that adding another pref to disable notifications globally is the way to go.

Matt brought up some good points in bug 1313939, comment 5, which apply to this approach as well. If the disable notifications pref is set, but the permission manager has `ALLOW` for that domain, which takes precedence? If you navigate to the site, open the control center, and change the notification permission, what happens? Does the pref still override the per-site setting, or do we allow exceptions?

Reading https://developer.chrome.com/extensions/contentSettings, it looks like `chrome.contentSettings.notifications` also takes a pattern...so ISTM we'd need to use the pref if `<all_urls>` is given, and the permission manager otherwise. However, the permission manager doesn't support wildcards, either, so "http://*.example.com/*" wouldn't work at all, and a global Boolean pref wouldn't help here.

We could turn the pref into a blacklist, but now we're adding a complicated hack around the permission manager, and we still don't have good answers to Matt's questions.

I think it really would be best to wait until we have a way to set permissions globally. Unfortunately, per bug 1275599, comment 6, it looks like that's cut from the MVP, because of the risk that users will accidentally break their sites. But maybe we can still implement support for global blocks (and wildcards) in the permission manager, without exposing them in the UI.

Johann, what do you think?
Flags: needinfo?(kit) → needinfo?(jhofmann)
(Assignee)

Comment 12

2 years ago
Thanks for the reply, Kit. I believe Johann and I (and some others) are going to meet today to discuss this whole permissions thing.
Thanks for alerting me, I proposed a solution in bug 1313939 and made it block the meta-bug of this. Supporting wildcards in the permission manager sounds hard right now, but a global toggle might be possible, depending on how that bug goes.
Flags: needinfo?(jhofmann)
(Assignee)

Comment 14

2 years ago
Thanks Johann. As per our conversation I'm going to mark this as blocked by 1313939.
Depends on: 1313939
Depends on: 1379560
No longer depends on: 1313939
Comment hidden (mozreview-request)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8919920 [details]
Bug 1364942 - Allow WebExtensions to disable Web API notifications,

https://reviewboard.mozilla.org/r/190852/#review196064

I'd like to know for sure that this doesn't unexpectedly break on android since the pref is defaulted only for firefox.  Otherwise I'd r+.  r? me again when you got an answer or fix.

::: toolkit/components/extensions/ext-browserSettings.js:88
(Diff revision 1)
>    setCallback(value) {
>      return {[this.prefNames[0]]: value};
>    },
>  });
>  
> +ExtensionPreferencesManager.addSetting("webAPINotificationsEnabled", {

how about just webNotificationsEnabled?

::: toolkit/components/extensions/ext-browserSettings.js:133
(Diff revision 1)
>              return aboutNewTabService.newTabURL;
>            }, URL_STORE_TYPE, true),
> +        webAPINotificationsEnabled: getSettingsAPI(extension,
> +          "webAPINotificationsEnabled",
> +          () => {
> +            let prefValue = Services.prefs.getIntPref("permissions.default.desktop-notification");

this pref seems to only be defaulted for browser, not toolkit.  Does getSettingsAPI protect against prefs throwing?
Attachment #8919920 - Flags: review?(mixedpuppy)
(Assignee)

Comment 17

2 years ago
Thanks for the review, Shane. I was thinking about this a bit and now I'm wondering if we should only allow an extension to make the default "block", which is what the bug originally asked. If we allow extensions to also set the default to "allow" that would mean that notifications would be allowed on all sites, and the user would never get prompted, which seems like a bad idea. What do you think?
Flags: needinfo?(mixedpuppy)
Yeah that seems like a bad idea. :)
To say a little more, we are not intending to expose the "Allow Always" option in the UI. The current mockup shows just a single checkbox saying "Never prompt for notification permission" or something like that. It's not checked by default, but if the user checks it we would flip the pref that you're modifying to 2/BLOCK. This work is tracked in bug 1368744.

I would love if we could collaborate to add an indicator to preferences that the setting is being controlled by an extension once these two bugs have landed.

We could also consider to add more indicators if we notice users being confused, e.g. in page info, the identity popup or even the identity block.
(In reply to Bob Silverberg [:bsilverberg] from comment #17)

Yeah, I think the addon shouldn't set allow_always.
Flags: needinfo?(mixedpuppy)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

2 years ago
mozreview-review-reply
Comment on attachment 8919920 [details]
Bug 1364942 - Allow WebExtensions to disable Web API notifications,

https://reviewboard.mozilla.org/r/190852/#review196064

I've also changed the behaviour so it can only be used to disable notifications, not to enable notifications.
Sorry, now I wonder if an extension disables it, shouldn't it be able to re-enable (but not override user selection)?  I can see not allowing to just randomly turn it on.
Flags: needinfo?(bob.silverberg)
ie. if the pref is not user set, and it is enabled, and the extension disables it, it seems like it should be able to undo that setting.
(Assignee)

Updated

2 years ago
Blocks: 1411700
Sorry for the noise, after irc and looking it's fine.
Flags: needinfo?(bob.silverberg)

Comment 26

2 years ago
mozreview-review
Comment on attachment 8919920 [details]
Bug 1364942 - Allow WebExtensions to disable Web API notifications,

https://reviewboard.mozilla.org/r/190852/#review198302

Lets run try/android
Attachment #8919920 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)

Comment 28

2 years ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ac09a374b69
Allow WebExtensions to disable Web API notifications, r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/2ac09a374b69
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Updated

2 years ago
Keywords: dev-doc-needed
Docs page: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/webNotificationsDisabled

I haven't updated the compat data yet. I had a question: although the JSON file suggests this is supported on Android, some of the comments above suggest that it isn't. Is there some nuance we should capture in the compat data for Android?
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 31

a year ago
Thanks Will, the docs look good. I'm not sure if this will work on Android or not. Johann, what do you think?
Flags: needinfo?(bob.silverberg) → needinfo?(jhofmann)
We just don't have a default pref on Android right now (we could change that), but setting the pref should still work. :)
Flags: needinfo?(jhofmann)

Comment 33

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?(bob.silverberg)
(Assignee)

Updated

a year ago
Flags: needinfo?(bob.silverberg) → qe-verify-
Duplicate of this bug: 1421040

Updated

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