Evgueni Naverniouk Reporter
	Description • 11 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:22.0) Gecko/20130415 Firefox/22.0
Build ID: 20130415004014

Steps to reproduce:

The current Aurora 22.0a2 build of the HTML5 Desktop Notifications appears to be auto-closing the notifications after a period of time. Although, I agree that by default this is a good thing, there are a few issues with that:

- The W3C spec does not state that auto-closing is a default behavior. In fact, they state that to auto-close a notification you should use setTimeout in the onshow function and then call cancel() after a desired timeout. (http://www.w3.org/TR/notifications)
- It's inconsistent with Webkit's current implementation (which references the W3C spec)
- There is no way exposed to the developer to stop the auto-close from firing. In some cases, an application may want to keep the notification up and wait for the user to manually dismiss it.

Ideally, we should encourage W3C to add a "closeAfter" attribute for notifications to make it really easy to developers to specify a time after which notifications should close (or null, to ever close). The current setTimeout method is ugly and messy to me. But in the meantime, can we update Firefox to keep behavior consistent with the spec?




Actual results:

--


Expected results:

--

	Jeremie Patonnier :Jeremie
	Comment 3 • 11 years ago
dev-doc-complete

Add some notes about this behavior to the documentation here:
* https://developer.mozilla.org/en-US/docs/WebAPI/Using_Web_Notifications
* https://developer.mozilla.org/en-US/docs/Web/API/Notification

	Moritz Naumann
	Comment 4 • 11 years ago

I concur, notifications should never vanish by default. 

Additionally, there should be a way for the web developer to change it so that it will close after a given time (this is currently possible through the rather ugly setTimeout() method, hopefully the spec will be improved there). 

And finally users should be able to override both the default and web developer intended behavior by configuring an integer (via about:config) option which, if a positive value, defines a fixed interval, or, if 0, keeps the notification open no matter what (unless the user manually closes it).

If DoS situations by infinitively spawned notifications are a concern - even though the user needs to opt in to notifications per domain - the implementation could be done so that a microsecond delay is enforced between notification showing up.

On a side note, the wiki Jeremie points to is outdated in that it states the permission request is not implemented in Chrome (I'm assuming Chrome = Chromium here). It does work fine for me on Chromium 28.0.1500.71 on Linux x86_64. It also seems to be no longer correct that "this technology's specification has not stabilized" and that there are mandatory "prefixes to use in various browsers".

	Jeremie Patonnier :Jeremie
	Comment 5 • 11 years ago

(In reply to Moritz Naumann from comment #4)
> On a side note, the wiki Jeremie points to is outdated in that it states the
> permission request is not implemented in Chrome (I'm assuming Chrome =
> Chromium here). It does work fine for me on Chromium 28.0.1500.71 on Linux
> x86_64.

Actually, it's not outdated. The current Chrome stable 30.0.1599.69 does not support the Notification.permission property. However, Chrome canary 32.0.1668.3 does, so it seams that it should be soon fixed. When it will be we will update the doc.

> It also seems to be no longer correct that "this technology's
> specification has not stabilized" and that there are mandatory "prefixes to
> use in various browsers".

I agree that it is arguable. The current spec status at W3C is LCWD which means it's seams stable enough. However, there are still serious implementation inconsistency. Therefor, the documentation team prefer to warn the readers explicitly.

	Jean-Yves Perrier [:teoli]
	Comment 8 • 10 years ago

Resetting the dev-doc flag for when this behaviour will be changed ;-)

And just a factual info: this problem doesn't happen on Firefox + Mac 10.9 as the notifications are sent to the Notification Centre.

	Matthew N. [:MattN]
	Comment 11 • 9 years ago

This bug actually wants the even newer requireInteraction flag.

	Martin Thomson [:mt:]
	Comment 14 • 9 years ago

I don't understand why I was n-i here.  This looks straightforward.  Of course, it requires support for notifications in service workers, but we have that coming.

(Bill, you should probably change your bugzilla display name to include your IRC nick.)

	Martin Thomson [:mt:]
	Comment 15 • 9 years ago

Decision from the UX meeting today was to provide requireInteraction behavior by default on platforms that don't provide a notification tray.

We will separately decide whether the site is able to specify requireInteraction to override this.  That might be implemented on a per-platform basis.

	Chris Karlof [:ckarlof]
	Comment 16 • 9 years ago

(In reply to Moritz Naumann from comment #4)
 
> And finally users should be able to override both the default and web
> developer intended behavior by configuring an integer (via about:config)
> option which, if a positive value, defines a fixed interval, or, if 0, keeps
> the notification open no matter what (unless the user manually closes it).

I agree. Bill, thoughts? Would this be part of 44 or later? At the least, we should have a bug to track it, because it requires some UI.

	Bill Maggs (bmaggs)
	Comment 17 • 9 years ago

We agreed this in a group today, so I am on baord with it. We should aim for first iteration of 44. It needs to be triaged, I guess, by Andrew Overholt.

	Chris Karlof [:ckarlof]
	Comment 18 • 9 years ago

(In reply to Bill Maggs from comment #17)
> We agreed this in a group today, so I am on baord with it. We should aim for
> first iteration of 44. It needs to be triaged, I guess, by Andrew Overholt.

I don't recall discussing today a user affordance to override the default and web developer requested closing behavior.

I created a bug for it: Bug 1205491

	Andrew Overholt [:overholt]
	Comment 19 • 9 years ago

mt, can you or someone else file a spec issue about comment 15?  https://github.com/whatwg/notifications/issues/new?title=&body=https%3A%2F%2Fnotifications.spec.whatwg.org%2F%23subtitle%0A%0A

William, please keep this potential change to notifications behaviour in mind as we may need some implementation help here in the near future :)

	Martin Thomson [:mt:]
	Comment 20 • 9 years ago

Andrew, my assessment is that we didn't actually want to change the spec for this.  What we actually want is a notification center of some form, at which point we can return to being spec compliant.

n-i Anne so that he is aware of the problem.  Maybe he can think of a better solution for us too.

	Andrew Overholt [:overholt]
	Comment 21 • 9 years ago

(In reply to Martin Thomson [:mt:] from comment #20)
> Andrew, my assessment is that we didn't actually want to change the spec for
> this.  What we actually want is a notification center of some form, at which
> point we can return to being spec compliant.

Oops, I totally missed "We will separately decide whether the site is able to specify requireInteraction to override this" in comment 15.

	Anne (:annevk)
	Comment 22 • 9 years ago

As filed per comment 0 this is invalid. When you create a notification using `new Notification()` it's non-persistent. Only with a service worker can you create a persistent notification.

Per comment 11 it seems this bug morphed into a feature request for requireInteraction and then comment 15 says this is now a UX bug?

This is all very confusing. I recommend first sorting out what the goal of this bug is.

	Matthew N. [:MattN]
	Comment 23 • 9 years ago

(In reply to Anne (:annevk) from comment #22)
> As filed per comment 0 this is invalid. When you create a notification using
> `new Notification()` it's non-persistent. Only with a service worker can you
> create a persistent notification.

Comment 0 also talks about "There is no way exposed to the developer to stop the auto-close from firing." which is what this bug is about now.

> Per comment 11 it seems this bug morphed into a feature request for
> requireInteraction and then comment 15 says this is now a UX bug?

requireInteraction achieves what I quoted from comment 0.

> This is all very confusing. I recommend first sorting out what the goal of
> this bug is.

This bug is about allowing developers to control whether a notification persists which I believe is the same as the requireInteraction property.

	Andrew Overholt [:overholt]
	Comment 24 • 9 years ago

William said he could take a look here.

	Bill Maggs (bmaggs)
	Comment 25 • 9 years ago

This bug should also be used to implement a key requirement for Push Notifications: that the default behavior of XUL-based notifications (currently used on Windows and pre 10.8 OSX) should be to Require Interaction. Of course, this default behavior for notifications can be overridden by developer choice. The default behavior to require interaction would require a change from the current default, which makes messages disappear after 4 seconds.  This has been discussed with William Chen (too many Williams!), who is helping out on the bug.

	Matthew N. [:MattN]
	Comment 26 • 9 years ago

William, I want to confirm that you will be implementing the alert service changes for all of the desktop alert service backends in 44:
* XUL => skip the close timer
* OS X =>  _style, _persistent and/or _dismissAfterDuration private APIs should achieve this
* libnotify => use `notify_notification_set_timeout`

It doesn't have to all be in this bug, I just need to know that you will handle it.

	William Chen [:wchen] Assignee
	Comment 27 • 9 years ago

Adds requireInteraction to web notifications and passes the value to platform notification implementations.

	William Chen [:wchen] Assignee
	Comment 29 • 9 years ago

(In reply to Matthew N. [:MattN] (behind on mail) from comment #26)
> William, I want to confirm that you will be implementing the alert service
> changes for all of the desktop alert service backends in 44:
> * XUL => skip the close timer
> * OS X =>  _style, _persistent and/or _dismissAfterDuration private APIs
> should achieve this
> * libnotify => use `notify_notification_set_timeout`
> 
> It doesn't have to all be in this bug, I just need to know that you will
> handle it.

I don't think that the requireInteraction flag requires that notifications will timeout/auto-close if set to false.

	Andrea Marchesini [:baku]
	Comment 30 • 9 years ago

Comment on attachment 8668184 [details] [diff] [review]
Part 1: Add requireInteraction property to web notifications.

Review of attachment 8668184 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/alerts/nsXULAlerts.cpp
@@ +43,5 @@
>  nsXULAlerts::ShowAlertNotification(const nsAString& aImageUrl, const nsAString& aAlertTitle,
>                                     const nsAString& aAlertText, bool aAlertTextClickable,
>                                     const nsAString& aAlertCookie, nsIObserver* aAlertListener,
>                                     const nsAString& aAlertName, const nsAString& aBidi,
> +                                   const nsAString& aLang, bool aRequireInteraction,

are you going to use this aRequireInteraction in the next patches?

	Matthew N. [:MattN]
	Comment 31 • 9 years ago

(In reply to William Chen [:wchen] from comment #29)
> (In reply to Matthew N. [:MattN] (behind on mail) from comment #26)
> > William, I want to confirm that you will be implementing the alert service
> > changes for all of the desktop alert service backends in 44:
> > * XUL => skip the close timer
> > * OS X =>  _style, _persistent and/or _dismissAfterDuration private APIs
> > should achieve this
> > * libnotify => use `notify_notification_set_timeout`
> > 
> > It doesn't have to all be in this bug, I just need to know that you will
> > handle it.
> 
> I don't think that the requireInteraction flag requires that notifications
> will timeout/auto-close if set to false.

Maybe not but that's the behaviour we still want on all platforms as that's the norm for notifications on all platforms.

	Matthew N. [:MattN]
	Comment 32 • 9 years ago

Are you planing on adding support outside of XUL notifications in another bug? I think that supporting this only for XUL notifications is going to confuse web developers. If we only support this for XUL notifications then the better solution would have been to just change the behaviour of XUL notifications and not implement this API.

	Bill Maggs (bmaggs)
	Comment 33 • 9 years ago

We need to implement consistency for all Push-based Notifications, not just XUL Notifications. I believe that means that the default we set (which can be overridden by developer choice and, in the case of OSX (and linux, for the most part, the user) is RequireInteraction. I think websites will be pretty pissed at us if we are not consistent in how we treat them.

	William Chen [:wchen] Assignee
	Comment 34 • 9 years ago

It doesn't make sense to have requireInteraction == false mean auto-close because that behavior can already be achieved using existing APIs (setTimeout to call close() on the notification). What the spec says is that notifications should be "readily available" until activated or dismissed by the user. I could see this being interpreted as keeping it on the screen or placing it in a tray/notification center but I don't see an interpretation that automatically closes the notification if the flag is not set.

Given the requirements in the last few comments of this bug, it's probably better to take Matt's suggestion to make all XUL notifications not auto-close by default (although I'm not sure if we should modify behavior of other parts of Firefox that use them such as the download manager). If that's what we want to do then we should do so in a different bug and hold off on landing this bug since this bug is about the "require interaction" flag as described in the spec.

	Bill Maggs (bmaggs)
	Comment 35 • 9 years ago

We can follow the spec by allowing the default to requireInteraction == false. In a separate bug we will set that default auto-close timeout to be of sufficient length that unless the developer says otherwise, the notifications will be viewable long enough to notice and read them, expecially on platforms without a Notification Center.

	Edwin Wong [:edwong]
	Comment 36 • 9 years ago

I logged that related bug 1211721

	Matthew N. [:MattN]
	Comment 37 • 9 years ago

(In reply to Bill Maggs (bmaggs) from comment #35)
> We can follow the spec by allowing the default to requireInteraction ==
> false. In a separate bug we will set that default auto-close timeout to be
> of sufficient length that unless the developer says otherwise, the
> notifications will be viewable long enough to notice and read them,
> expecially on platforms without a Notification Center.

Are there really good use cases to warrant the inconsistency that users will experience when some sites use requireInteraction and others don't? I'm not really convinced of that TBH.

	Edwin Wong [:edwong]
	Comment 38 • 9 years ago

There's a lively discussion around this - wmaggs to drive a decision with the team. Stand by.

	Edwin Wong [:edwong]
	Comment 39 • 9 years ago

Win/Mac Chrome Beta supports requireInteraction value.

	Bill Maggs (bmaggs)
	Comment 40 • 9 years ago

After much discussion about balancing the requirements of users and developers on this issue, for Firefox 44 and the initial launch of Push we've decided that giving all developers the ability to setrequireInteraction=true is potentially not a good initial experience for our users, and so we are not going to support it for now. We want instead to set the auto-close default at 12 seconds, subject to change as we do user testing and get more data form actual users.

	Ben Kennish
	Comment 42 • 9 years ago

(In reply to Bill Maggs (bmaggs) from comment #40)
> After much discussion about balancing the requirements of users and
> developers on this issue, for Firefox 44 and the initial launch of Push
> we've decided that giving all developers the ability to
> setrequireInteraction=true is potentially not a good initial experience for
> our users, and so we are not going to support it for now. We want instead to
> set the auto-close default at 12 seconds, subject to change as we do user
> testing and get more data form actual users.

Pleeeeeease at least make it possible to set the default behavior for notifications to "stay open" within about:config so that the more technically savvy of us can choose this if we wish.

	Moritz Naumann
	Comment 43 • 9 years ago

I could hardly agree more. I understand that there are a lot of policies and processes involved in decision making in Mozilla. Which can be a good thing. But the net result of how this feature request ended up being handled is, from my outside perspective, disappointing.

	Alex Gibson [:agibson]
	Comment 44 • 8 years ago

Just to some real world info for thought here, I maintain this library that adds as a wrapper for the Web Notifications API:

https://github.com/alexgibson/notify.js

I just had a contributor submit a PR to add support for the requireInteraction property. Because Chrome now supports this option, but Firefox does not, he added a hack that UA sniffed Firefox and repeatedly fired a notification over and over every 4 seconds if the option was passed, as a "work around". Obviously, I turned this down as the UX was pretty horrible. He did say however that he's already deployed this Firefox "fix" in their production app some time ago, because their client base needs some form of persistent notification capability. It seems to me like there is a genuine need for something here more than we currently offer.

	Edwin Wong [:edwong]
	Comment 47 • 8 years ago

removing tracking bug

	Lina Butler [:lina]
	Comment 48 • 8 years ago

Chatting with :benbangert on IRC...

One possibility is to persist notifications with `requireInteraction: true` by default, and add an "Automatically dismiss notifications from site.com" option to the action menu. If the user selects that option, we'd ignore `requireInteraction` and auto-close notifications from that site. We could extend this to existing web notifications, too: auto-close by default, with an option to make them persistent. Users could toggle between the two at any time.

I agree the current behavior isn't good for developers or users, and that any solution needs to put users in control. While adding a notification center is our long-term goal, I'd like for us to discuss some things we can do in the meantime to unblock this.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 49 • 8 years ago

Setting "Automatically dismiss notifications from example.com" to true makes sense for users who want them to dismiss on their own. This would make all notifications from that domain auto-dismiss.

However, if the user later changes the preference to false for that domain, should all notifications persist now?

Maybe it should be worded as "Limit example.com from persisting notifications" or some word that is easier to translate and understand than "persisting"? Maybe "Limit example.com from keeping notifications on-screen?"

	:Harald Kirschner :digitarald
	Comment 50 • 8 years ago

Jeff for product decision.

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 51 • 8 years ago

I talked to Philipp about this this morning. We came to the conclusion that, given the spec status requireInteraction we should probably get around to shipping it however we're concerned about the abuse / annoyance factor.

We want to block this on notification stacking - Philipp is checking into the state of stacking.

	Matthew N. [:MattN]
	Comment 52 • 8 years ago

Don't we also want user control/override too?

	Martin Thomson [:mt:]
	Comment 53 • 8 years ago

MattN, are you suggesting something in about:preferences?  I would have thought that the right approach here is to add a pref to disable { requireInteraction: true } and leave it to an addon to disable stickier notifications.  Then, if that proves to be popular, or the complaints get too loud, we consider adding UI for it.  Note that we probably want to look at { sticky: true } in the same patch, since they are effectively the same thing.  If you want annoying, sticky will be that.

	Anne (:annevk)
	Comment 54 • 8 years ago

Once about:preferences covers per-domain/origin settings so you can configure cookies/storage, geolocation, push, notifications, etc. as you'd expect, it would make sense to put this there.

If we do less platform integration for notifications (not sure what the status on this is) we could also offer options in the first notification of a given site, to give the user some control at that point.

Note that sticky only makes sense for persistent notifications (those that survive closing the window and are associated with a service worker).

	Matthew N. [:MattN]
	Comment 55 • 8 years ago

(In reply to Martin Thomson [:mt:] from comment #53)
I'm talking about per-domain control. As the user agent, we should let the user control whether a notification will continue to take up screen real estate.

(In reply to Anne (:annevk) from comment #54)
> If we do less platform integration for notifications (not sure what the
> status on this is) we could also offer options in the first notification of
> a given site, to give the user some control at that point.

That doesn't depend on less platform integration… we already have control for revoking the permission within the OS X ones and we can do the same for Linux (it wasn't prioritized) as they provide APIs for buttons.

	Anne (:annevk)
	Comment 56 • 8 years ago

Matthew, you could, but then you can't implement the actions feature of the latest version of the standard.

	Martin Thomson [:mt:]
	Comment 57 • 8 years ago

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #55)
> I'm talking about per-domain control. As the user agent, we should let the
> user control whether a notification will continue to take up screen real
> estate.

Oh, then requireInteraction is easy: users that don't want a notification showing can close that notification; if it happens too often, then revoke permissions.

Sticky notifications are different, but I agree that we would want some sort of control.  Though I don't see why that can't be something like: a) only one sticky per origin, and b) disable notifications for that origin if you don't like their stickies.  That is based on the notion that once a site has permission to show a notification, they can, if they choose, spam notifications to get the effect of a sticky notification.

	Bryan Clark (DevTools PM) [:clarkbw]
	Comment 58 • 8 years ago

Adding Ash who I believe has some designs for this already, or at least has the link to the designs. :)

	Ash Grigas
	Comment 59 • 8 years ago

(In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #58)
> Adding Ash who I believe has some designs for this already, or at least has
> the link to the designs. :)

The design spec for the new permissions notification design is here:
https://docs.google.com/a/mozilla.com/document/d/143nEfWfIvFZD2-pFqE8GD85inFjFIhE2J8QeegQufc0/edit?usp=sharing

There is an invision prototype here:
https://invis.io/J77RIHDK9#/142999157_Home-Phase3

Please not in the past week the implementation has started and therefore some mocks may not be completely updated in terms of the url bar area as we had to make some changes to reduce scope but the general framework is final.

	William Chen [:wchen] Assignee
	Comment 61 • 8 years ago

Since we don't have a stacking UI yet, this patch limits the number of requireInteraction notifications that we show at once and queues the rest. This is similar to what Chrome does notifications.

	Andrea Marchesini [:baku]
	Comment 63 • 8 years ago

Comment on attachment 8799668 [details] [diff] [review]
Part 4: Add preference for number of XUL alerts to show with requireInteraction set to true.

Review of attachment 8799668 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/alerts/nsXULAlerts.cpp
@@ +79,5 @@
>  }
>  
> +void
> +nsXULAlerts::PersistentAlertFinished()
> +{

MOZ_ASSERT(mPersistentAlertCount);

@@ +83,5 @@
> +{
> +  mPersistentAlertCount--;
> +
> +  // Show next pending persistent alert if any.
> +  if (mPendingPersistentAlerts.Length()) {

if (!mPendingPersistentAlerts.IsEmpty()) {

@@ +130,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // If there is a pending alert with the same name in the list of
> +  // pending alerts, replace it.
> +  if (mPendingPersistentAlerts.Length() > 0) {

if (!mPendingPersistentAlerts.IsEmpty()) {

@@ +147,5 @@
> +          rv = mPendingPersistentAlerts[i]->mListener->Observe(nullptr, "alertfinished", cookie.get());
> +          NS_ENSURE_SUCCESS(rv, rv);
> +        }
> +
> +        mPendingPersistentAlerts.ReplaceElementAt(i, new PendingAlert(aAlert, aAlertListener));

mPendingPersistentAlerts[i].Init(aAlert, aAlertListener);

@@ +160,5 @@
> +
> +  if (requireInteraction &&
> +      !mNamedWindows.Contains(name) &&
> +      mPersistentAlertCount >= Preferences::GetInt("dom.webnotifications.requireinteraction.count", 0)) {
> +    mPendingPersistentAlerts.AppendElement(new PendingAlert(aAlert, aAlertListener));

PendingAlert* pa = mPendingPersistentAlerts.AppendElement();
pa.mAlert = aAlert;
pa.mListener = aAlertListener;

or: pa.Init(aAlert, aAlerListener);

::: toolkit/components/alerts/nsXULAlerts.h
@@ +8,5 @@
>  
>  #include "nsCycleCollectionParticipant.h"
>  #include "nsHashKeys.h"
>  #include "nsInterfaceHashtable.h"
> +#include "nsDataHashtable.h"

alphabetic order. Move it after sCycleCollectionParticipant.h

@@ +45,4 @@
>  
>    nsInterfaceHashtable<nsStringHashKey, mozIDOMWindowProxy> mNamedWindows;
> +  uint32_t mPersistentAlertCount = 0;
> +  nsTArray<nsAutoPtr<PendingAlert>> mPendingPersistentAlerts;

nsTArray<PendingAlert> mPendingPersistentAlerts; Why do you use nsAutoPtr?

	Andrea Marchesini [:baku]
	Comment 64 • 8 years ago

Comment on attachment 8799669 [details] [diff] [review]
Part 5: Tests for web notifications requireInteraction.

Review of attachment 8799669 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/alerts/test/test_alerts_requireinteraction.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for alerts with requireInteraction</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> 

extra space.

	Pulsebot
	Comment 65 • 8 years ago

Pushed by wchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a705399d8e6
Part 1: Add requireInteraction property to web notifications. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1a94303232
Part 2: Update XUL web notifications to persist if require interaction flag is set. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1919859fd56
Part 3: Add preference for web notification requireInteraction flag. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a67414d0312
Part 4: Add preference for number of XUL alerts to show with requireInteraction set to true. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/a24574db096b
Part 5: Tests for web notifications requireInteraction. r=baku

	Carsten Book [:Tomcat]
	Comment 66 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0a705399d8e6
https://hg.mozilla.org/mozilla-central/rev/0f1a94303232
https://hg.mozilla.org/mozilla-central/rev/f1919859fd56
https://hg.mozilla.org/mozilla-central/rev/9a67414d0312
https://hg.mozilla.org/mozilla-central/rev/a24574db096b

	Jorg K (CEST = GMT+2)
	Comment 67 • 8 years ago

This busted Thunderbird.

Is this the right way to fix it or do I need to call

+  bool requireInteraction = mRequireInteraction;
+  if (!Preferences::GetBool("dom.webnotifications.requireinteraction.enabled", false)) {
+    requireInteraction = false;
+  }

or

+  bool requireInteraction;
+  rv = aAlert->GetRequireInteraction(&requireInteraction);
+  NS_ENSURE_SUCCESS(rv, rv);

	Jorg K (CEST = GMT+2)
	Comment 68 • 8 years ago

Looks like the previous comment was pretty much silly. I only found this where you added 'false' as last parameter:
https://hg.mozilla.org/mozilla-central/rev/0a705399d8e6#l11.13

I'm landing this now, if it's wrong, we can always adjust it later ;-)
https://hg.mozilla.org/comm-central/rev/68d61dfbe0ffa6dfd7a63dbbb92ef47498f3f56f

	William Chen [:wchen] Assignee
	Comment 69 • 8 years ago

Comment on attachment 8801680 [details] [diff] [review]
Thunderbird patch.

Review of attachment 8801680 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, that's the right way to fix it if you want to preserve the existing behavior.

	Jorg K (CEST = GMT+2)
	Comment 70 • 8 years ago

Comment on attachment 8801680 [details] [diff] [review]
Thunderbird patch.

Thanks, already landed ;-)

	Chris Mills [:cmills]
	Comment 71 • 7 years ago

I've documented the new property here:
https://developer.mozilla.org/en-US/docs/Web/API/notification
https://developer.mozilla.org/en-US/docs/Web/API/notification/requireInteraction
https://developer.mozilla.org/en-US/docs/Web/API/notification/Notification

And added a note to the Fx 52 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/52#DOM_HTML_DOM

I had one question — the spec no longer mentions the rather messy process of using a setTimeout to run close() inside the onshow handler to consistently close notifications. Should we just get rid of any mention of that technique now - do browsers now all close notifications by default? My testing suggests so, but I just wanted to check.

	zmandel
	Comment 72 • 7 years ago

Hello. requireInteraction does not seem to be working on the 52 release 52.0 (32-bit, Windows 10). Same code works on Chrome.
//From the app
            Notification.requestPermission(function (status) {
                if (status != "granted")
                    return;
                navigator.serviceWorker.ready.then(function (registration) {
                    if (!registration.showNotification || !registration.getNotifications)
                        return;
                    registration.showNotification('test', {
                        body: 'test',
                        icon: '../img/icon192.png',
                        tag: "myId",
                        silent: true,
                        requireInteraction: true,
                        data: { action: "test" }
                    });
                });


//From the service worker, I have:
self.addEventListener('notificationclick', function (event) {

The notification shows but it still auto-closes.
The service worker receives the clicks, however I have verified that event.notification.requireInteraction is not set in the event.notification object from the click above. In Chrome, it is set to true.