Closed Bug 912645 Opened 11 years ago Closed 10 years ago

Notifications: ability to indicate notification modes

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:-)

RESOLVED FIXED
2.1 S5 (26sep)
blocking-b2g -

People

(Reporter: jrburke, Assigned: robertbindar)

References

Details

(Whiteboard: [systemsfe], ux-most-wanted-nov2014)

Attachments

(1 file, 4 obsolete files)

Some notifications from an app do not warrant all notification modes. For instance, email notifications do not need to wake the screen or trigger a vibration or sound, or a popup that breaks the user's existing flow.

While there is a story to tell around an end user being able to control the notification modes on a per-app basis via a system wide preferences UI, we should also allow applications to opt in to less obtrusive notifications if they so choose. We should encourage a pathway that allows apps to be good citizens instead of making the end user do all the work.

This would also immediately benefit the email notification work for 1.2. Without it, the email notifications could be too obnoxious to be useful.

This is just a proposal to get the conversation started, nothing set in stone:

Notifications should support a "modes" option in the Notification constructor. It is an Array. If this option is present, it means that only the modes in the "modes" array are activated, all others are defaulted to "off". Example:

var notification = new Notification('Hello World', {
  body: 'Welcome to the world',
  modes: ['status', 'list']
});

Means that the notification would only show up for 'status' and 'list', and all other modes below would not be activated for this notification.

Possible values for the modes array:

* status: include in any aggregate count of notifications, and in any top level status indicator of notifications (like the top status area on the phone).

* list: include an entry in any list of notifications.

* display: show a popup display of the notification.

* wake: wake up the device for the notification (like turning on the screen).

* vibration: vibration is OK to use. If 'wake' is not listed, then only vibrate if device is already awake.

* sound: sound is OK to use. If 'wake' is not listed, then only play sound if device is already awake.

Any user-indicated preferences would take priority over these mode indicators. So if the user has turned off sound, that takes priority. If the device allows the user to set a notification policy on the notifications for an app, that policy overrides these indicators in places where the policy and the indicator overlap.

It could be that "modes" should be moz-prefixed to "mozModes" to start. Originally started with using "behavior" for the name, but that has a "behavior" "behaviour" discussion that should be avoided.
Component: Gaia → Gaia::System
i wonder if there is 2 bugs here since implementing notifications preference in settings does not involve adding more arguments, isn't it? If so maybe adding something in settings would be good enough for 1.2 ? (it still seems a bit too short though).
I agree that the overall, long term solution is two parts, the other part being a UI that the user can set for overrides. That should be tracked as a separate ticket, as it implies quite a lot more work. It may be even be a larger tracking bug for smaller pieces of work like figuring out what kinds of apps do notifications and therefore should be listed in the UI.

The hope for this bug was more around data and plumbing. I could see the UI pathway leveraging the work from this ticket. The UI that allowed overrides could introduce another piece in the notification pipeline  that would overlay the UI mode preferences over the set the notification initiates.

So this ticket could be seen as a starting point along that path, to help define the discrete notification modes/behaviors that could later be customized or overridden by a user preference UI. It also seemed to be a smaller bit of work, but work that would be immediately useful for apps too.

What also came up in UX review was the desire for the email notification sound to be subtler than the regular sound, if one is triggered. So I can see the "sound" mode also allowing the app to specify an audio file URL for the notification. That would also be something that could be customized in a settings UI, but it is another example of a notification mode preference that the app itself may want to indicate.
Whiteboard: [systemfe]
Whiteboard: [systemfe] → [systemsfe]
Not a blocking feature - we're working around this in the 1.2 timeframe. This is something we'll look into a future release though.
blocking-b2g: --- → -
Francis, as discussed, this could use your UX input on which aspects of the notification are critical to control.
Flags: needinfo?(fdjabri)
Hi Rob, passing this to you.
Flags: needinfo?(fdjabri) → needinfo?(rmacdonald)
Blocks: 1042361
Bug 1042361 indicates another mode that may be useful, something that expresses "just update any existing notification, do not disrupt the user with sounds or flashes of notification UI". Perhaps that just means the email app needs to see if there is an existing notification and decide to set the modes appropriately, but just calling out something related to this feature.
(In reply to James Burke [:jrburke] from comment #6)
> Bug 1042361 indicates another mode that may be useful, something that
> expresses "just update any existing notification, do not disrupt the user
> with sounds or flashes of notification UI". Perhaps that just means the
> email app needs to see if there is an existing notification and decide to
> set the modes appropriately, but just calling out something related to this
> feature.

Yeah, my thought would be that this is the app's responsibility. Maybe some apps want all their notifications to be disruptive.
This is a great thread and I'd like to see a lot of these features on the device. I know it's not planned for 2.1 but it'd be great if we could work on this in the 2.2 timeframe. If there's anything we can do sooner let me know and we can discuss.
Flags: needinfo?(rmacdonald)
Assignee: nobody → robertbindar
Talking with Robert Bindar offline, there was some confusion about the "status" and "list" descriptions above:

* "status": I meant that it is included in the status bar visible at the top of the phone screen, where battery/wifi is shown, where it shows the total number of notifications that are unaddressed.

* "list": I meant showing it in the list of notifications in what I believe is called the "notification tray" -- the list that is shown when you swipe down from the top of the phone screen.

I was just trying to enumerate all possible display states for the notification, but just for illustration purposes for describing the feature. However discussion with others and the people doing the implementation might lead to a better classification of the modes.
Attached patch custom_behavior.patch (obsolete) — Splinter Review
We discussed a lot about the first options we should support and we decided on:
  * "nolights"
  * "noclear"
  * "vibrationPattern"
  * "soundFile"

Here are some more details about the meaning of these behaviors.
http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Aug/0065.html
I also filed a spec issue related to bug 1042361, Jonas told me the expected behavior is not to retrigger the actions when replacing, so that might be just a b2g bug, we'll see where this is going.
https://github.com/whatwg/notifications/issues/24

I'll upload the gaia support patch as soon as I'll finish updating the failing tests.
Comment on attachment 8485238 [details] [diff] [review]
custom_behavior.patch

The try is green
https://tbpl.mozilla.org/?tree=Try&rev=3e49b112c7a9
Attachment #8485238 - Flags: review?(amarchesini)
Attached file system app support (obsolete) —
The tests are green modulo notification_test.js which depends on the gecko support.
Comment on attachment 8485238 [details] [diff] [review]
custom_behavior.patch

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

lgtm, but can I see this patch again with these comments applied?

::: dom/notification/ChromeNotifications.js
@@ +49,5 @@
>  
>      notifications.forEach(function(notification) {
> +      let behavior = undefined;
> +      if (notification.behavior) {
> +        behavior = JSON.parse(notification.behavior);

JSON.parse() can throw an exception if the input is not a JSON object. Use try/catch.

::: dom/notification/Notification.cpp
@@ +473,2 @@
>  
>    nsString dataString;

nsAutoString

@@ +478,5 @@
>      scContainer->GetDataAsBase64(dataString);
>    }
>  
> +  NotificationBehavior bDict;
> +  bDict = aOptions.mBehavior;

why do you need this bDict? Can you just do aOptions.mBehavior.ToJSON(..) ?

@@ +479,5 @@
>    }
>  
> +  NotificationBehavior bDict;
> +  bDict = aOptions.mBehavior;
> +  nsString behavior;

nsAutoString

@@ +480,5 @@
>  
> +  NotificationBehavior bDict;
> +  bDict = aOptions.mBehavior;
> +  nsString behavior;
> +  bDict.ToJSON(behavior);

This can fail.

::: dom/webidl/AppNotificationServiceOptions.webidl
@@ +19,2 @@
>  };
> +

remove this extra line.
Attachment #8485238 - Flags: review?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #14)
> Comment on attachment 8485238 [details] [diff] [review]
> custom_behavior.patch
> 
> Review of attachment 8485238 [details] [diff] [review]:
> -----------------------------------------------------------------
>  
> @@ +478,5 @@
> >      scContainer->GetDataAsBase64(dataString);
> >    }
> >  
> > +  NotificationBehavior bDict;
> > +  bDict = aOptions.mBehavior;
> 
> why do you need this bDict? Can you just do aOptions.mBehavior.ToJSON(..) ?
>

aOptions.mBehavior is 'const NotificationBehavior', so I cannot call a non-const method on a const object.

> @@ +480,5 @@
> >  
> > +  NotificationBehavior bDict;
> > +  bDict = aOptions.mBehavior;
> > +  nsString behavior;
> > +  bDict.ToJSON(behavior);
> 
> This can fail.
> 
Are you sure that can fail? NotificationBehavior is intended to be JSON serializable and as long as its content is trusted it shouldn't fail. Do you mean the stringifying callbacks could fail anyway?
Flags: needinfo?(amarchesini)
Depends on: 1065025
> aOptions.mBehavior is 'const NotificationBehavior', so I cannot call a
> non-const method on a const object.

Yes. and I don't see any reason why ToJSON should not be const. I filed bug 1065025.

> > This can fail.
> > 
> Are you sure that can fail? NotificationBehavior is intended to be JSON
> serializable and as long as its content is trusted it shouldn't fail. Do you
> mean the stringifying callbacks could fail anyway?

I meant that ToJSON returns a boolean and I think it's important to check the return value.
Flags: needinfo?(amarchesini)
Attached patch custom_behavior.patch v2 (obsolete) — Splinter Review
Thanks for reviewing this Andrea.
I fixed the nits and I rebased the patch after bug 1065025.
Attachment #8485238 - Attachment is obsolete: true
Attachment #8486578 - Flags: review?(amarchesini)
Comment on attachment 8486578 [details] [diff] [review]
custom_behavior.patch v2

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

lgtm. Just throw an error when the ToJSON bit fails.

::: dom/notification/Notification.cpp
@@ +463,5 @@
>      scContainer->GetDataAsBase64(dataString);
>    }
>  
> +  nsAutoString behavior;
> +  if (!aOptions.mBehavior.ToJSON(behavior)) {

aRv.Throw(NS_ERROR_FAILURE);
Attachment #8486578 - Flags: review?(amarchesini) → review+
Attached patch custom_behavior.patch v3 (obsolete) — Splinter Review
Attachment #8486578 - Attachment is obsolete: true
Keywords: checkin-needed
Blocks: 1066385
Keywords: checkin-needed
Attachment #8486746 - Flags: superreview?(jonas)
Comment on attachment 8486746 [details] [diff] [review]
custom_behavior.patch v3

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

I'm not really crazy about passing data around as a JSON encoded string. But I'll leave that up to the reviewer.

The actual options seem great to me. So sr=me on that.

::: dom/webidl/Notification.webidl
@@ +61,5 @@
>    DOMString body = "";
>    DOMString tag = "";
>    DOMString icon = "";
>    any data = null;
> +  NotificationBehavior behavior = null;

Rename this property to 'mozbehavior' since it's FirefoxOS specific.

Actually, I might even simply call this property "mozilla" since I think that we'll end up with a bunch of behavior parameters in the outer dictionary over time. There's not really much point in nesting dictionaries here other than as a way to indicate which parameters are non-standard and FirefoxOS specific.

@@ +69,5 @@
>    DOMString tag;
>  };
>  
> +dictionary NotificationBehavior {
> +  boolean nolights = false;

Looks like this means "don't turn on screen". I think something like "noscreen" might be a more understandable name.

@@ +71,5 @@
>  
> +dictionary NotificationBehavior {
> +  boolean nolights = false;
> +  boolean noclear = false;
> +  DOMString soundFile = "";

You need to resolve the URL here at the callsite. I.e. in the child process before we pass the information to the parent process. You should resolve it the same way that we resolve the icon URL.

@@ +72,5 @@
> +dictionary NotificationBehavior {
> +  boolean nolights = false;
> +  boolean noclear = false;
> +  DOMString soundFile = "";
> +  sequence<unsigned long> vibrationPattern;

We should treat passing an empty pattern here as "use default vibration". I would even say that we should treat an initial value of 0 as "use default vibration".

I'd rather not enable people to use this as a way to turn off vibration.
Attachment #8486746 - Flags: superreview?(jonas) → superreview+
(In reply to Jonas Sicking (:sicking) from comment #20)
> Comment on attachment 8486746 [details] [diff] [review]
> custom_behavior.patch v3
> 
> Review of attachment 8486746 [details] [diff] [review]:
> 
> ::: dom/webidl/Notification.webidl
> @@ +61,5 @@
> >    DOMString body = "";
> >    DOMString tag = "";
> >    DOMString icon = "";
> >    any data = null;
> > +  NotificationBehavior behavior = null;
> 
> Rename this property to 'mozbehavior' since it's FirefoxOS specific.
> 
> Actually, I might even simply call this property "mozilla" since I think
> that we'll end up with a bunch of behavior parameters in the outer
> dictionary over time. There's not really much point in nesting dictionaries
> here other than as a way to indicate which parameters are non-standard and
> FirefoxOS specific.

The goal is to make this work for Firefox OS and other platforms, even desktop. It seems like WHATWG is on board with the general idea [1][2], and I can see this API being useful for Android and iOS web apps (ie. they each have similar native API's). Is the reason for prefixing because it is not on the WHATWG spec yet? I though we were trying to avoid Mozilla prefixes when possible.

1.) https://github.com/whatwg/notifications/issues/22
2.) http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Aug/0065.html
Flags: needinfo?(jonas)
Sadly there hasn't been consensus for adding these properties yet. I think that means that we can not expose these properties on the desktop implementation for notifications.

I agree that we want to avoid prefixes. But if we want to ship this before there's consensus (which is likely to take quite a while still), then I think it's better to use prefixes than not to.

So I think we have the following options:

1. Only expose this to certified apps. Then we don't have to worry about prefixes since it's easy to
   change apps and the API together in the future when we align with the spec.
2. Expose this to privileged apps using a permission. In this case it's likely better to use prefixes
   in order to make a future update to the spec simpler as the specced API and the prefixed API can live
   side-by-side for a few releases.
Flags: needinfo?(jonas)
I am not working on this feature, just using it, but I prefer not leaning on "certified". The more we put in that "certified" bucket, the harder it is to get to out of band network updates for gaia apps. It also makes it harder to get broader app testing in the field outside the very restrictive "certified" set.
Ok, prefix it is. I prefer the name 'mozbehavior' for now, since eventually we hope to call this 'behavior', and it's more descriptive than 'mozilla'.
Alternatively we could simply do:


dictionary NotificationOptions {
  NotificationDirection dir = "auto";
  DOMString lang = "";
  DOMString body = "";
  DOMString tag = "";
  DOMString icon = "";
  boolean moznolights = false;
  boolean moznoclear = false;
  DOMString mozsoundFile = "";
  sequence<unsigned long> mozvibrationPattern;
  any data = null;
};

I.e. I don't think the extra nested dictionary really improves the API.

But I really don't feel strongly.
 
> @@ +71,5 @@
> >  
> > +dictionary NotificationBehavior {
> > +  boolean nolights = false;
> > +  boolean noclear = false;
> > +  DOMString soundFile = "";
> 
> You need to resolve the URL here at the callsite. I.e. in the child process
> before we pass the information to the parent process. You should resolve it
> the same way that we resolve the icon URL.
> 

I think we needed to resolve the icon URL for firefox desktop because we load the image in a XUL image tag, but for b2g we don't do that, we just pass it as it comes from content, it is resolved by loading it in an html image.src attribute (which apparently resolves the url).
Flags: needinfo?(jonas)
Yes, b2g will eventually stick the icon URL into an <img>.src and the sound URL into an <audio>.src, which will resolve the url. However it will be resolved against the base URL of the web page in the system app, rather than the base URL of the page that created the notification.

I.e. if a webpage located on "http://example.com/awesomeapp/index.html" does:

new Notification({
 ...
 icon: "picture.jpg",
 soundFile: "coffeepot.mp3"   //(should we rename this to just "sound" btw?)
}

then that should cause us to use "http://example.com/awesomeapp/picture.jpg" and "http://example.com/awesomeapp/coffeepot.mp3" as URLs for icon and sound respectively.

But if gaia simply creates an <img src="picture.jpg"> <audio src="coffeepot.mp3">, then we'll try to load "app://system.gaiamobile.org/picture.jpg" and "app://system.gaiamobile.org/coffeepot.mp3"
Flags: needinfo?(jonas)
I see your point, I just tried to be consistent with the way we resolve the icon URL. Should I fix the code to resolve the icon URL the way you described it?
If we really aren't resolving icon URLs correctly right now (though that sounds surprising to me), then we should definitely fix that too.
https://tbpl.mozilla.org/?tree=Try&rev=f11558368950
Attachment #8486208 - Attachment is obsolete: true
Attachment #8486746 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9712d6370c3f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
Blocks: 1098041
Blocks: 994991
Whiteboard: [systemsfe] → [systemsfe], ux-most-wanted-nov2014
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: