Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for inline replies #132

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
102 changes: 82 additions & 20 deletions notifications.bs
Original file line number Diff line number Diff line change
Expand Up @@ -108,19 +108,42 @@ the <a>notification</a>, but then it should have less visual priority than the
is not otherwise accessible to the end user, especially since notification platforms that do not
support these features might ignore them.

<p>A <a>notification</a> has an associated list of
zero or more <dfn for=notification lt=action id=actions>actions</dfn>. Each
<a for=notification>action</a> has an associated
<dfn for=action id=action-title>title</dfn> and <dfn for=action id=action-name>name</dfn> and
<em>can</em> have an associated <dfn for=action>icon URL</dfn> and
<dfn for=action>icon resource</dfn>. Users may activate actions, as alternatives to
activating the notification itself. The user agent must determine the
<dfn>maximum number of actions</dfn> supported, within the constraints of the
notification platform.
<p>A <a>notification</a> has <dfn for=notification lt=action id=actions>actions</dfn>,
a <a for=/>list</a> of <a>notification actions</a>, initially empty.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this to something like "A notification has an action list (a list of actions). It is initially empty." And then use the <dfn> instead of the the <dfn> for "notification action" below since "action" always meant to refer to the individual item and not the associated list.

Copy link
Author

@anitawoodruff anitawoodruff May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have changed the wording but I'm a little confused about where to put the <dfn> tags from your comment, can you clarify?


<p>A <dfn id=notification-action>notification action</dfn> defines a way the user can interact with
a notification, as an alternative to activing the notification itself.

<div dfn-for="notification action">
<p>A <a>notification action</a> has a <dfn id=notification-action-type>type</dfn>, that is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use one space for indentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, no need to set an ID for new <dfn>s.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"<code>button</code>" or "<code>text</code>".

<p class=note>Actions of type "<code>button</code>" can only be activated, whereas actions of type
"<code>text</code>" allow the user to input text during activation.

<p>A <a>notification action</a> has a <dfn id=notification-action-title>title</dfn> (a DOMString).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't change the ID. That'll affect folks linking to this.

Thanks for adding a type, but let's make that "a string".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


<p>A <a>notification action</a> has a <dfn id=notification-action-name>name</dfn> (a DOMString).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as above and they apply below too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


<p>A <a>notification action</a> has an <dfn id=notification-action-icon-url>icon URL</dfn> (null
or a <a>URL</a>). Unless stated otherwise, it is null.

<p>A <a>notification action</a> has an <dfn id=notification-action-icon-resource>icon resource</dfn>
(null or an image). Unless stated otherwise, it is null.

<p>A <a>notification action</a> has a <dfn id=notification-action-placeholder>placeholder</dfn>
(a DOMString). Unless stated otherwise, it is an empty string.
</div>

<p>The user agent must determine the <dfn>maximum number of actions</dfn> supported, within the
constraints of the notification platform.

<p class=note>Since display of actions is platform-dependent, developers are
encouraged to make sure that any action a user can invoke from a notification is
also available within the web application.
also available within the web application. The ability to reply inline to
notifications during activation is also platform-dependent, so developers are encouraged to
handle the case where a text action was activated but the reply was null (e.g.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow e.g. with a comma.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

by focusing a chat window).

<p class="note no-backref">Some platforms might modify an <a for=action>icon resource</a> to better
match the platform's visual style before displaying it to the user, for example by rounding the
Expand Down Expand Up @@ -227,14 +250,22 @@ these steps:
<li><p>If <var>options</var>'s <code>requireInteraction</code> is true, set
<var>notification</var>'s <a for=notification>require interaction preference flag</a>.

<li><p>Set <var>notification</var>'s list of <a>actions</a> to an empty list,
then for each <var>entry</var> in <var>options</var>'s <code>actions</code>,
up to the <a>maximum number of actions</a> supported (skip any excess
entries), perform the following steps:
<li>For each <var>entry</var> in <var>options</var>'s <code>actions</code>:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<li><p> throughout.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


<ol>
<li>If the user agent does not support additional actions, break.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then break*

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


<li><p>Let <var>action</var> be a new <a lt="actions">action</a>.

<li><p>Set <var>action</var>'s <a for=action>type</a> to the <var>entry</var>'s
<code>type</code> property.

<li>If the user agent does not support additional actions of this
<a for="notification action">type</a>, continue.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then continue*

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


<p class=note>For instance, some operating systems don't support buttons if a text action has
been provided, or vice-versa.

<li><p>Set <var>action</var>'s <a for=action>name</a> to the <var>entry</var>'s
<code>action</code>.

Expand All @@ -244,7 +275,11 @@ these steps:
<li><p>If <var>entry</var>'s <code>icon</code> is present,
<a lt="url parser">parse</a> it using <var>baseURL</var>, and if that does
not return failure, set <var>action</var>'s <a for=action>icon URL</a> to the
return value. (Otherwise <a for=action>icon URL</a> is not set.)
return value.

<li><p>If <var>entry</var>'s <code>placeholder</code> is present, set
action's <a for=action>placeholder</a> to the <var>entry</var>'s
<code>placeholder</code>.

<li><p>Append <var>action</var> to <var>notification</var>'s list of
<a>actions</a>.
Expand Down Expand Up @@ -517,11 +552,18 @@ specified) run these steps:
<p>If <var>notification</var> is a <a>persistent notification</a>, then:

<ol>
<li><p>Let <var>action</var> be the empty string.
<li><p>Let <var>action</var> be an empty string.

<li><p>Let <var>reply</var> be null.

<li><p>If one of <var>notification</var>'s <a for=notification>actions</a> was activated by the
user, then set <var>action</var> to that <a for=notification>action</a>'s <a for=action>name</a>.

<li><p>If one of <var>notification</var>'s <a for=notification>actions</a>
with type <i>'text'</i> was activated by the user, and the opportunity to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be "<code>text</code>"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

input a reply was provided, then set <var>reply</var> to the text entered by
the user during activation.

<li><p>Let <var>callback</var> be an algorithm that when invoked with a <var>global</var>,
<a lt="fire a service worker notification event named e">fires a service worker notification event</a>
named <code>notificationclick</code> given <var>notification</var> and <var>action</var> on
Expand Down Expand Up @@ -666,9 +708,16 @@ enum NotificationDirection {
};

dictionary NotificationAction {
NotificationActionType type = "button";
required DOMString action;
required DOMString title;
USVString icon;
DOMString? placeholder = "";
};

enum NotificationActionType {
"button",
"text"
};

callback NotificationPermissionCallback = void (NotificationPermission permission);
Expand Down Expand Up @@ -863,12 +912,14 @@ result of the following steps:
<ol>
<li><p>Let <var>frozenActions</var> be an empty list of type {{NotificationAction}}.

<li><p>For each <var>entry</var> in the <a>notification</a>'s list of
<a for=notification>actions</a>, perform the following steps:
<li><p>For each <var>entry</var> in <a>notification</a>'s <a for=notification>actions</a>:

<ol>
<li><p>Let <var>action</var> be a new {{NotificationAction}}.

<li><p>Set <var>action</var>'s {{NotificationAction/type}} to
<var>entry</var>'s <a for=action>type</a>.

<li><p>Set <var>action</var>'s {{NotificationAction/action}} to
<var>entry</var>'s <a for=action>name</a>.

Expand All @@ -878,6 +929,9 @@ result of the following steps:
<li><p>Set <var>action</var>'s {{NotificationAction/icon}} to
<var>entry</var>'s <a for=action>icon URL</a>.

<li><p>Set <var>action</var>'s {{NotificationAction/placeholder}} to
<var>entry</var>'s <a for=action>placeholder</a>.

<!-- XXX IDL dictionaries are usually returned by value, so don't need to be
immutable. But FrozenArray reifies the dictionaries to mutable JS
objects accessed by reference, so we explicitly freeze them.
Expand Down Expand Up @@ -1000,11 +1054,13 @@ partial interface ServiceWorkerRegistration {
interface NotificationEvent : ExtendableEvent {
readonly attribute Notification notification;
readonly attribute DOMString action;
readonly attribute DOMString? reply;
};

dictionary NotificationEventInit : ExtendableEventInit {
required Notification notification;
DOMString action = "";
DOMString? reply = null;
};

partial interface ServiceWorkerGlobalScope {
Expand Down Expand Up @@ -1087,8 +1143,10 @@ the same underlying <a>notification</a> of {{Notification}} objects already in e
given <var>notification</var> and <var>action</var>,
<a>fire an event</a> named <var>e</var>, using {{NotificationEvent}}, with the
{{NotificationEvent/notification}} attribute initialized to a new
{{Notification}} object representing <var>notification</var> and the
{{NotificationEvent/action}} attribute initialized to <var>action</var>.
{{Notification}} object representing <var>notification</var>, the
{{NotificationEvent/action}} attribute initialized to <var>action</var>, and the
{{NotificationEvent/reply}} attribute initialized to <var>reply</var>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the reply variable come from?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point, guess I can just change the beginning of this sentence to 'given notification, action and reply' which should cover it, yes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you'll need to update the callers of this algorithm too, then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, that makes sense.

Have updated Step 1.5 of Section 2.7 'Activating a Notification' to 'given notification, action and reply' also. I think this now covers it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's two callers of this algorithm, so I don't think that quite covers it. Also, if both are going to set it to null, does it need to be a variable for this algorithm?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 1.4 of Section 2.7 ('Activating a notification') sets 'reply' to the text entered by the user during activation, so it shouldn't be null in that case.

I assume you are talking about Section 2.8 ('Closing a notification') as the other caller of this algorithm. That caller does not pass anything for 'action' either right now, so I guess this needs fixing too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah :/ I guess action needs to be the empty string there; @beverloo can confirm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it should be. We could consider changing the event hierarchy a bit to avoid including the action and reply fields with close events - it'd add a bit of boilerplate, but seems cleaner.

NotificationEvent { .notification }
NotificationClickEvent : NotificationEvent { .action, .reply }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah that would involve adding a whole new interface to the ServiceWorker API, no?

I'm up for doing this but won't this involve quite a lot of work updating idls and tests etc? Could it be done as a followup in a separate change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with beverloo@ offline that changing the event hierarchy is out of scope for this change.

Have now updated the 'Closing a notification' algorithm as suggested to pass the empty string & null.


<!-- XXX need to define at what the event is fired -->

<p>The {{NotificationEvent/notification}} attribute's getter must return the
Expand All @@ -1097,6 +1155,9 @@ value it was initialized to.
<p>The {{NotificationEvent/action}} attribute's getter must return the value it
was initialized to.

<p>The {{NotificationEvent/reply}} attribute's getter must return the value it
was initialized to.

<p>The following is the <a>event handler</a> (and its corresponding
<a>event handler event type</a>) that must be supported as attribute by the
{{ServiceWorkerGlobalScope}} object:
Expand All @@ -1122,6 +1183,7 @@ was initialized to.
Addison Phillips,
Aharon (Vladimir) Lanin,
Alex Russell,
Anita Woodruff,
Anssi Kostiainen,
Arkadiusz Michalski,
Boris Zbarsky,
Expand Down