Intent to Ship : Notification Options "vibrate" flag.

[Sanghyun Park]

Contact emails

sh919...@samsung.com

Spec

https://notifications.spec.whatwg.org/#api

"If options's vibrate is present, validate and normalize it and set notification’s vibration pattern to the return value."

Summary

The "vibrate" flag as part of Notification options, which allows authors to supply a vibration pattern similar to "navigator.vibrate()", which is to be applied when a Notification gets shown.

For example:

serviceWorkerRegistration.showNotification('Title', {

    body: 'message',

    vibrate: [100, 200, 300]

});

Introducing the "vibrate" attribute is blocked on the following bug. 

https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682

But we would much prefer to have the attribute introduced to the specification prior to us shipping this.

Motivation

By supporting Notification.vibrate, developers can control vibrate pattern, when a Notification gets shown such as native notification.

And developers can support a variety of vibration patterns to the user through this attribute.

Link to "Intent to Implement" blink-dev discussion

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pFC0Qj6tvA8/discussion

Compatibility Risk

Low, but we are the first to implement this.

Ongoing technical constraints

None.

Will this feature be supported on all five Blink platforms (Windows, Mac, Linux, Chrome OS and Android)?

Vibration is dependent on device capability.

This feature will be supported only android, for now.

OWP launch tracking bug?

https://crbug.com/442132

Link to entry on the feature dashboard

None

[Peter Beverloo]

Hi Sanghyun Park,

Thank you for implementing this! It'll make a great addition for Web Notifications on Android, as developers not rarely use a vibration pattern to allow users to quickly, audibly identify the source of the notification.

On Fri, May 22, 2015 at 2:08 PM, Sanghyun Park <sh919...@samsung.com> wrote:

Contact emails

sh919...@samsung.com

Spec

https://notifications.spec.whatwg.org/#api

"If options's vibrate is present, validate and normalize it and set notification’s vibration pattern to the return value."

Summary

The "vibrate" flag as part of Notification options, which allows authors to supply a vibration pattern similar to "navigator.vibrate()", which is to be applied when a Notification gets shown.

Not just similar, but following exactly the same syntax, sanitation and limitations.

For example:

serviceWorkerRegistration.showNotification('Title', {

    body: 'message',

    vibrate: [100, 200, 300]

});

Introducing the "vibrate" attribute is blocked on the following bug. 

https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682

But we would much prefer to have the attribute introduced to the specification prior to us shipping this.

To clarify, are you requesting permission to ship the dictionary option but *not* the attribute, since we implement both?

It would be great if you could file an issue against the Notification spec (capturing what the specification already mentions) for introducing the attribute. I'd prefer to ship both at the same time if we can :-)

https://github.com/whatwg/notifications/issues

Thanks,

Peter

[Sanghyun Park]

Hi Peter.

Thanks for your comment. :)

2015년 5월 22일 금요일 오후 10시 17분 27초 UTC+9, Peter Beverloo 님의 말:

Hi Sanghyun Park,

Thank you for implementing this! It'll make a great addition for Web Notifications on Android, as developers not rarely use a vibration pattern to allow users to quickly, audibly identify the source of the notification.

On Fri, May 22, 2015 at 2:08 PM, Sanghyun Park <sh919...@samsung.com> wrote:

Contact emails

sh919...@samsung.com

Spec

https://notifications.spec.whatwg.org/#api

"If options's vibrate is present, validate and normalize it and set notification’s vibration pattern to the return value."

Summary

The "vibrate" flag as part of Notification options, which allows authors to supply a vibration pattern similar to "navigator.vibrate()", which is to be applied when a Notification gets shown.

Not just similar, but following exactly the same syntax, sanitation and limitations.

You're right. 

For example:

serviceWorkerRegistration.showNotification('Title', {

    body: 'message',

    vibrate: [100, 200, 300]

});

Introducing the "vibrate" attribute is blocked on the following bug. 

https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682

But we would much prefer to have the attribute introduced to the specification prior to us shipping this.

To clarify, are you requesting permission to ship the dictionary option but *not* the attribute, since we implement both?

It would be great if you could file an issue against the Notification spec (capturing what the specification already mentions) for introducing the attribute. I'd prefer to ship both at the same time if we can :-)

Okay, I'll raise an issue  in notification spec issue page.

And your intention is to ship vibrate flag in options and vibrate attribute at same time after exposing the vibrate attribute in spec.

is it right? 

I also want to ship these at same time, if it is possible.

I'll try to this.

Thanks

Sanghyun Park

[Domenic Denicola]

[+annevk, heycam] I don’t think fixing this will be as easy as you might hope. Good old bug 23682 has a long history, and nobody’s buckled down to solve it yet.

 

We could try to get the notifications spec patched with a one-off fix, but that doesn’t seem great. And even if we did, implementing it would require custom bindings---the current implementation in Blink returns a new array every time, which is exactly what we want to avoid by solving bug 23682. It’d be better to solve bug 23682 at the spec level, which will result in new Web IDL syntax which we then implement bindings for, which would make the actual implementation of the vibrate attribute much simpler.

 

So I don’t think it’s a great idea to block shipping vibrate functionality on exposing the vibrate attribute.

 

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Peter Beverloo]

On Fri, May 22, 2015 at 4:52 PM, Domenic Denicola <d...@domenic.me> wrote:

[+annevk, heycam] I don’t think fixing this will be as easy as you might hope. Good old bug 23682 has a long history, and nobody’s buckled down to solve it yet.

Sorry, I didn't mean to oversimplify the problem; I'm aware of 23682, but the last time I looked at it was well over a year ago. I perhaps naively assumed we'd have some progress there.

 

We could try to get the notifications spec patched with a one-off fix, but that doesn’t seem great. And even if we did, implementing it would require custom bindings---the current implementation in Blink returns a new array every time, which is exactly what we want to avoid by solving bug 23682. It’d be better to solve bug 23682 at the spec level, which will result in new Web IDL syntax which we then implement bindings for, which would make the actual implementation of the vibrate attribute much simpler.

We shouldn't introduce any further one-off fixes. While it does have my preference to ship both at the same time, there's absolutely no rush here :-).

 

So I don’t think it’s a great idea to block shipping vibrate functionality on exposing the vibrate attribute.

Shipping the option but not the attribute is fine with me.

The primary consequences are not being able to feature detect and not being able to clone previously shown notifications. People won't depend their use of notifications on this, and it's easy enough to work around the clone case with some application logic.

Thanks,

Peter

[Philip Jägenstedt]

Trying to figure out if bug 23682 should block this...

The WebIDL syntax used is "readonly attribute sequence<unsigned long>?
vibrate". While we already ship sequence<T> as a return value for
Document.elementsFromPoint() and other methods, the only other
attribute is BeforeInstallPromptEvent.platforms which is being shipped
in M44.

Anne, can you summarize what kind of mess we should expect if we get
this wrong? Is it the question of whether the same object should be
returned every time? The current implementation of
Notificiation.vibrate would take a copy of the object from the
constructor and return a new copy every time:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3530 (logs
"100", false, false)

Philip

[Anne van Kesteren]

On Monday, May 25, 2015, Philip Jägenstedt <phi...@opera.com> wrote:

The WebIDL syntax used is "readonly attribute sequence<unsigned long>?

It should not be nullable. That syntax is also not stable.

 

Is it the question of whether the same object should be returned every time? 

It should be the same object. One array per Notification object that is. Depending on the outcome of the IDL bug it might need to be a frozen array.

--
https://annevankesteren.nl/

[Philip Jägenstedt]

Should it also be the same object as is passed to new Notification('',
{ vibrate: [100] })? If it's frozen, should that object be frozen by
the constructor?

Cameron, do you know what needs to be done to move bug 23682 along for
the specific case of a readonly attribute sequence<T>?

Philip

[Philip Jägenstedt]

On Tue, May 26, 2015 at 9:53 AM, Cameron McCormack <c...@mcc.id.au> wrote:
> Anne van Kesteren:

>> > It should be the same object. One array per Notification object that is.
>> > Depending on the outcome of the IDL bug it might need to be a frozen array.
>

> Philip Jägenstedt:

>> Should it also be the same object as is passed to new Notification('',
>> { vibrate: [100] })? If it's frozen, should that object be frozen by
>> the constructor?
>

> No, I was planning on making it a separate, newly created and frozen
> array. You’d convert the [100] JS value as if you were passing it
> something expecting a sequence<T>, and then you’d make a new Array
> object with the right values, and then freeze it.

>
>> Cameron, do you know what needs to be done to move bug 23682 along for
>> the specific case of a readonly attribute sequence<T>?
>

> The syntax I’ve got in my branch is FrozenArray<T>. So you’d still be
> prohibited from writing |readonly attribute sequence<T>|.
>
> I think we exhausted the discussion around the different ways to handle
> properties with array-ish values. The next step is for me to unbitrot
> that branch and merge it in. (It’s the “arrays” branch of
> https://github.com/heycam/webidl/ if you want to have a look before I do
> that.)

Thanks, I've taken a look and it all looks pretty straightforward:
convert to an Array and freeze it. Whether or not that's the right
model for Notification.vibrate I think is mostly a question for Anne
and Peter, but it sounds OK to me.

If that spec change is made in WebIDL and the Notification spec,
shipping Notification.vibrate would be blocked on implementing
FrozenArray<T>.

Philip

[Peter Beverloo]

On Tue, May 26, 2015 at 9:57 AM, Philip Jägenstedt <phi...@opera.com> wrote:

On Tue, May 26, 2015 at 9:53 AM, Cameron McCormack <c...@mcc.id.au> wrote:
> Anne van Kesteren:
>> > It should be the same object. One array per Notification object that is.
>> > Depending on the outcome of the IDL bug it might need to be a frozen array.
>
> Philip Jägenstedt:
>> Should it also be the same object as is passed to new Notification('',
>> { vibrate: [100] })? If it's frozen, should that object be frozen by
>> the constructor?
>
> No, I was planning on making it a separate, newly created and frozen
> array.  You’d convert the [100] JS value as if you were passing it
> something expecting a sequence<T>, and then you’d make a new Array
> object with the right values, and then freeze it.

Additionally, specific to this case, the vibration pattern will be normalized before it's set on the Notification object, so passing [100, 10] in the NotificationOptions dictionary would result in Notification.vibrate to be [100].

>> Cameron, do you know what needs to be done to move bug 23682 along for
>> the specific case of a readonly attribute sequence<T>?
>
> The syntax I’ve got in my branch is FrozenArray<T>.  So you’d still be
> prohibited from writing |readonly attribute sequence<T>|.
>
> I think we exhausted the discussion around the different ways to handle
> properties with array-ish values.  The next step is for me to unbitrot
> that branch and merge it in.  (It’s the “arrays” branch of
> https://github.com/heycam/webidl/ if you want to have a look before I do
> that.)

Thanks, I've taken a look and it all looks pretty straightforward:
convert to an Array and freeze it. Whether or not that's the right
model for Notification.vibrate I think is mostly a question for Anne
and Peter, but it sounds OK to me.

The Notification object, aside from the event listeners, is already immutable so that seems fine to me. I'll leave the IDL and array specifics to Anne and Cameron.

If that spec change is made in WebIDL and the Notification spec,
shipping Notification.vibrate would be blocked on implementing
FrozenArray<T>.

That raises the question whether we should rescope this Intent to Ship to shipping NotificationOptions.vibrate and not Notification.vibrate, or block shipping the feature on the FrozenArray<T> work.

I'm on the fence - this is not a high priority feature and we can hold off without much harm, but it does provide nice UX benefits when used on Android and it would be nice to get it out there. Perhaps we should postpone the decision to when we also support author-defined sounds and the renotify property, in order to provide a more holistic package in regards to gaining the user's attention?

I'd also like to hear from Sanghyun Park what his opinion is, since he implemented it.

Thanks,

Peter

[Sanghyun Park]

2015년 5월 26일 화요일 오후 11시 44분 14초 UTC+9, Peter Beverloo 님의 말:

On Tue, May 26, 2015 at 9:57 AM, Philip Jägenstedt <phi...@opera.com> wrote:

On Tue, May 26, 2015 at 9:53 AM, Cameron McCormack <c...@mcc.id.au> wrote:
> Anne van Kesteren:
>> > It should be the same object. One array per Notification object that is.
>> > Depending on the outcome of the IDL bug it might need to be a frozen array.
>
> Philip Jägenstedt:
>> Should it also be the same object as is passed to new Notification('',
>> { vibrate: [100] })? If it's frozen, should that object be frozen by
>> the constructor?
>
> No, I was planning on making it a separate, newly created and frozen
> array.  You’d convert the [100] JS value as if you were passing it
> something expecting a sequence<T>, and then you’d make a new Array
> object with the right values, and then freeze it.

Additionally, specific to this case, the vibration pattern will be normalized before it's set on the Notification object, so passing [100, 10] in the NotificationOptions dictionary would result in Notification.vibrate to be [100].

Gritty further described, vibrate Pattern can be normalized according to Vibration API spec[1].

[1] http://www.w3.org/TR/vibration/#dfn-validate-and-normalize

And I think that we doesn't need to keep developer's wrong vibrate pattern.

The returning the normalized vibration pattern will be even better for developers, since they can know actual execution results of vibration.

Anne, could you tell me why we should use "Sameobject" about this?

>> Cameron, do you know what needs to be done to move bug 23682 along for
>> the specific case of a readonly attribute sequence<T>?
>
> The syntax I’ve got in my branch is FrozenArray<T>.  So you’d still be
> prohibited from writing |readonly attribute sequence<T>|.
>

> I think we exhausted the discussion around the different ways to handle
> properties with array-ish values.  The next step is for me to unbitrot
> that branch and merge it in.  (It’s the “arrays” branch of
> https://github.com/heycam/webidl/ if you want to have a look before I do
> that.)

Thanks, I've taken a look and it all looks pretty straightforward:
convert to an Array and freeze it. Whether or not that's the right
model for Notification.vibrate I think is mostly a question for Anne
and Peter, but it sounds OK to me.

The Notification object, aside from the event listeners, is already immutable so that seems fine to me. I'll leave the IDL and array specifics to Anne and Cameron.

If that spec change is made in WebIDL and the Notification spec,
shipping Notification.vibrate would be blocked on implementing
FrozenArray<T>.

That raises the question whether we should rescope this Intent to Ship to shipping NotificationOptions.vibrate and not Notification.vibrate, or block shipping the feature on the FrozenArray<T> work.

I'm on the fence - this is not a high priority feature and we can hold off without much harm, but it does provide nice UX benefits when used on Android and it would be nice to get it out there. Perhaps we should postpone the decision to when we also support author-defined sounds and the renotify property, in order to provide a more holistic package in regards to gaining the user's attention?

I'd also like to hear from Sanghyun Park what his opinion is, since he implemented it.

 

Thanks,

Peter

 

I think that vibrate attribute may be changed due to "FrozenArray", and vibrate attribute is not exposed in current notification spec. 

So, I'd like to support only NotificationOptions.vibrate  like this topic's title for now.

This have no problems.

I will modify vibrate attribute in accordance with the spec, after FrozenArray is supported.

Thanks.

Sanghyun Park.

 

[Anne van Kesteren]

On Wednesday, May 27, 2015, Sanghyun Park <sh919...@samsung.com> wrote:

Anne, could you tell me why we should use "Sameobject" about this?

It should not be the same object that is passed in. That one is normalized per IDL sequence semantics. However, for properties we want to preserve obj.property === obj.property. Returning fresh objects from the getter would destroy that invariant.

I have no strong opinions on frozen. I think I would rather use an actual immutable array, but that doesn't exist.

--
https://annevankesteren.nl/

[Sanghyun Park]

2015년 5월 27일 수요일 오전 9시 4분 7초 UTC+9, Anne van Kesteren 님의 말:

On Wednesday, May 27, 2015, Sanghyun Park <sh919...@samsung.com> wrote:

Anne, could you tell me why we should use "Sameobject" about this?

It should not be the same object that is passed in. That one is normalized per IDL sequence semantics. However, for properties we want to preserve obj.property === obj.property. Returning fresh objects from the getter would destroy that invariant.

Thanks for your reply.  I agree with your opinion.

Currently,  V8bindings always make new array object, when sequence is returned. so "notification.vibrate === notification.vibrate" returns false.

I also think that this is a problem. I'll find a way to resolve this.

 

I have no strong opinions on frozen. I think I would rather use an actual immutable array, but that doesn't exist.

--
https://annevankesteren.nl/

As you said, I think that FrozenArray may be an alternative, though we cannot  fully satisfy.

And I am pleased by the news that FrozenArray is in progress.

If FrozenArray(or immutable) is supported and vibrate attribute is exposed again in spec, I'll try to make progress notification.vbirate attribute again.

I'd like to support only NotificationOptions.vibrate for now. 

Thanks

Sanghyun Park

[Alex Russell]

I'm pretty upset that this intent has been derailed by what, at base, is a judgement call from the perspective of the API author. It's fine here to return a new, normalized array each time.

[Domenic Denicola]

From: Alex Russell [mailto:sligh...@google.com]

> I'm pretty upset that this intent has been derailed by what, at base, is a judgement call from the perspective of the API author. It's fine here to return a new, normalized array each time.

I think we may have gotten a little lost here. I emphatically don't think we should implement (or specify) an API such that `n.vibrate !== n.vibrate`. However I also think we should get this new capability---vibrating notifications---into the hands of developers as soon as possible.

Fortunately, these things are not in conflict! We can ship the vibrate option to the constructor now, and ship the vibrate getter latter. The use cases for the vibrate getter are small---reflection on Notification objects is nice, but not fundamental.

It sounds like we have a clear path forward on exposing `n.vibrate`, too:

1. heycam un-bitrots and merges the patch to add FrozenArray<T> to WebIDL
2. annevk uses it in the Notifications spec
3. Blink team writes bindings support for it
4. We use it for `n.vibrate`.

If we think it's urgent to get `n.vibrate` working sooner rather than later, there are variations: for example specifying the behavior in prose and using a custom binding, so that we aren't blocked on 1. But I think that it's totally fine to ship vibration support without `n.vibrate`, and it makes for a saner platform.

[Philip Jägenstedt]

LGTM to ship just NotificationOptions.vibrate, but if we can ship Notification.vibrate with custom bindings at the same time that would be even better.

[Chris Harrelson]

LGTM2 to what Philip said.

[Philip Rogers]

LGTM3

[Sanghyun Park]

Thanks all!

I'll only enable the NotificationOptions.vibrate.

Sorry for all the confusion regarding the vibrate attribute.

And I'll try to solve issues about the vibrate attribute.

Thanks
Sanghyun Park

[Peter Beverloo]

Non-OWNERS LGTM as well, thanks!

Let's not rush Notification.vibrate through, and ship that when we're ready :).

Thanks,

Peter

[ebi...@gmail.com]

Can we please add this feature to chrometstatus.com?

[Philip Jägenstedt]

I created https://www.chromestatus.com/features/5727996321202176

Sanghyun, I assigned it to you, so please tweak it if I got something wrong.

[Sanghyun Park]

Philip, 

Thanks so much. I was looking for way to upload in chromestatus.

This is good for me.

Thanks

Sanghyun Park

2015년 6월 10일 수요일 오후 6시 20분 26초 UTC+9, Philip Jägenstedt 님의 말:

[Eric Bidelman]

Thanks Philip!