Re: Feedback from Safari on Web Notifications

On Mar 7, 2012, at 10:15 AM, John Gregg <johnnyg@google.com> wrote:

> 2. Use static functions on the notification constructor for permissions checks.
> 
> Notification.permissionLevel() --> String
> 
> The name aligns with similar functionality in the DAP Feature Permissions spec [FEAT], but scoped specifically to Web Notifications. The method returns one of three strings: "granted", "denied", and "default". This follows the current best practice from the WebApps WG where strings are being used instead of enums.
> 
> The Feature Permissions spec allows for 4 enums, two of which denote default behavior if the user hasn't explicitly determined a policy. We think 3 are only necessary, and any UA that wishes to grant permission by default should just return "granted".
> 
> From a flow perspective, it makes sense for a UA to deny by default, for any web site to detect that it's denied, and then request the UA for permission. Assuming the UA's default policy is the opposite, the flow doesn't fit. That is, assume a UA allows all notifications by default, and the web site detects a default-granted policy. We don't think any website would then ask the user to deny itself the privilege of posting notifications. In this case, the UA should prompt the user for denial directly, and not assume that the web site will handle the default-granted case. Therefore, we think UA's that want to give permission by default should just return "granted".
> 
> Notification.requestPermission(callback)
> 
> The callback specified includes one argument which represents the permission, similar to the return values in Notification.permissionLevel(). If the user had already made a policy decision, then the callback is invoked with the decision included as the parameter (with either "granted" or "denied"). Otherwise the UA may ask the user for permission, and the callback is invoked when the user makes a decision. Cancelling the request does not invoke the callback. Any notifications posted in the meantime will have onerror() called. Multiple calls to this function should just result in one visible request.
> 
> Although there is already a way to query for the permission, it seems weird from a programming flow perspective that the callback would not contain information about the user's decision as an argument.
> 
> I think the biggest question that the group needs to address here is whether the Notifications spec should continue depending on the Feature Permissions for its permissions behavior (and perhaps forward these proposals there to be applied in the general sense), or should we remove that dependency and return to writing our own permissions behavior scoped to notifications?
I believe we should not at this time depend on that spec for permissions behavior. From a general perspective, it opens the door for bad user experience, and I think this kind of API should only be available when necessary. Using the feature permissions spec makes it too easy to add new features that don't need it, and which could be handled through better API design, like in geolocation. UAs could either pummel the user with permission requests, or force the user to allow all features at once.

> 3. Replace setReplaceId() with a property id.
> 
> It isn't clear in the spec that the reason for this property is to avoid posting duplicate notifications by multiple frames, so adding some description about that would be useful. Platforms may differ in how they handle notifications with the same ID, so we think it is appropriate to remove the "replace" prefix. It looks like retrieving the ID is missing, and would be useful for the web site, so we should make it an assignable attribute.
> 
> I can add more explanations for this to the spec, although this is not the exclusive use case of replaceId -- there are also single-frame use cases (keeping a chat notification updated with the latest message, for example).
Yes! Explaining foreseen use cases really would help this part of the spec. My original thought was to remove this from the spec, until the use cases were explained to me.

> In those cases I think "id" alone suggests uniqueness that doesn't actually exist, whereas replaceId suggests the real relationship.  The term is already used by a notification platform [1] (as "replaces_id") for the same behavior.
Then maybe it's the term "id" that is misleading.

The word "replace" sounds like I'm replacing some ID, but I don't know where in the API I put the original one. "replacementID" would be slightly better, but either way I feel this implies an API that works more like the following:

firstNotif = new Notification();
firstNotif.id = "blah";

secondNotif = new Notification();
secondNotif.id = "blah2";
secondNotif.replaceID = "blah";

thirdNotif = new Notification();
thirdNotif.id = "blah3";
thirdNotif.replaceID = "blah2";

Instead maybe the term should be something that implies that a set of notifications are related. How about "tag"?

> As far as dealing with platform differences, I think the goal of the spec should be to abstract away those differences as much as possible so that web authors can expect consistency.  In the replaceID case we can do this by specifying that either the notification is replaced, if possible; or if not, the old one closed and the new one opened, which is not exactly the same but very close.  It would be much worse to specify that there is an ID property and the underlying platform will do whatever it chooses with it.
Agreed.

> 
> Also, is it clear that this id should be ignored if the string is null or empty?
> 
> I'll change the spec so that this is clear.
Great!

> 4. Improve show() behavior.
> 
> We think show() can only be used once per notification. Subsequent invocations should call onerror().
> 
> Agreed that show() is only meant to be used once.  Ian Hickson has made the related suggestion that show() should not exist as a method, but rather that show() be implicit in the constructor.  I don't prefer that because it requires that all possible optional parameters of the notification be provided in the constructor, which makes the interface harder to use.  I prefer using onerror to deal with multiple attempts to show, but this should be resolved by the WG.
I tend to agree with that. I also like the balance of saying n.show(), n.close().

> 5. Rename cancel() to close().
> 
> From a naming perspective, the asymmetry between cancel() and close() seems odd.
> 
> Did you mean to point out an asymmetry between cancel() and *show*() ?  The reason "cancel" makes sense rather than "close" is that the behavior depends on the state of the notification.  A notification which has been shown will be closed, but a notification that is queued pending available space on the display will be canceled before being shown.  IMHO "cancel" covers both situations better than "close", but if the group prefers close I can make that change to the spec. 
The asymmetry between cancel() and onclose().

My first time familiarizing with this spec, I thought cancel was an odd word choice, although now I understand what it is trying to imply. In the case where the notification actually does get shown, I don't think one would cancel it, because you're not backing out of the intended action (think "OK"/"Cancel"). But "close" does imply that something is visible. How about "remove", and "onremove"?

Jon