Closed
Bug 912645
Opened 11 years ago
Closed 10 years ago
Notifications: ability to indicate notification modes
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(blocking-b2g:-)
People
(Reporter: jrburke, Assigned: robertbindar)
References
Details
(Whiteboard: [systemsfe], ux-most-wanted-nov2014)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Blocks: b2g-notifications
Updated•11 years ago
|
Component: Gaia → Gaia::System
Comment 1•11 years ago
|
||
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).
Reporter | ||
Comment 2•11 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: [systemfe]
Updated•11 years ago
|
Whiteboard: [systemfe] → [systemsfe]
Comment 3•11 years ago
|
||
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: --- → -
Comment 4•11 years ago
|
||
Francis, as discussed, this could use your UX input on which aspects of the notification are critical to control.
Flags: needinfo?(fdjabri)
Comment 5•11 years ago
|
||
Hi Rob, passing this to you.
Flags: needinfo?(fdjabri) → needinfo?(rmacdonald)
Reporter | ||
Comment 6•10 years ago
|
||
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.
Comment 7•10 years ago
|
||
(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.
Comment 8•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → robertbindar
Reporter | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
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
Assignee | ||
Comment 11•10 years ago
|
||
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.
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
The tests are green modulo notification_test.js which depends on the gecko support.
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
(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)
Comment 16•10 years ago
|
||
> 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)
Assignee | ||
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
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+
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8486578 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
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+
Comment 21•10 years ago
|
||
(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)
Reporter | ||
Comment 23•10 years ago
|
||
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.
Comment 24•10 years ago
|
||
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.
Assignee | ||
Comment 26•10 years ago
|
||
> @@ +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)
Assignee | ||
Comment 28•10 years ago
|
||
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.
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8486208 -
Attachment is obsolete: true
Attachment #8486746 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 31•10 years ago
|
||
Keywords: checkin-needed
Comment 32•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
You need to log in
before you can comment on or make changes to this bug.
Description
•