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.

Sign up for GitHub

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
Open

Add support for inline replies #132

Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
904bcad
Adding Inline Notification Replies to the spec
May 22, 2018
eac4ebe
Updating based on annevk's review
May 24, 2018
844b530
address some nits
annevk May 28, 2018
1dba647
s/A action/An action
May 29, 2018
e308162
Addressing (some of) Peter's comments
May 31, 2018
8467059
Update algorithm in 'Closing a notification' to pass action and reply
Jun 6, 2018
56bb9a1
Add a note encouraging attribution
Jun 22, 2018
b8426e9
Change note to mandate attribution
anitawoodruff Jul 10, 2018
301ccf8
Amend note on treating text actions as buttons
anitawoodruff Jul 13, 2018
713eafb
Add note on destination of reply data
anitawoodruff Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 116 additions & 56 deletions notifications.bs
View file
Open in desktop
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 an <dfn for=notification>action list</dfn> (a <a for=/>list</a> of
<a for=notification>actions</a>). It is initially empty.

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

<div dfn-for=action>
<p>An <a for=notification>action</a> has a <dfn>type</dfn>, that is "<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>An <a for=notification>action</a> has a <dfn id=action-title>title</dfn> (a string).

<p>An <a for=notification>action</a> has a <dfn id=action-name>name</dfn> (a string).

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

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

<p>An <a for=notification>action</a> has a <dfn>placeholder</dfn> (a string). Unless stated
otherwise, it is an empty string.
Copy link
Member

Choose a reason for hiding this comment

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

nit: "an empty string" -> "the empty string" for consistency with the rest of the standard. In Section 2.7. 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.

</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., 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 @@ -152,11 +175,11 @@ these steps:
<var>notification</var>'s <a for=notification>service worker registration</a> to
<var>serviceWorkerRegistration</var>.

<li><p>If a <var>serviceWorkerRegistration</var> was not provided and
<var>options</var>'s <code>actions</code> is not empty, <a>throw</a> a
<code>TypeError</code> exception.
<li>
<p>If a <var>serviceWorkerRegistration</var> was not provided and <var>options</var>'s
<code>actions</code> is not empty, then <a>throw</a> a {{TypeError}}.

<p class=note><a>Actions</a> are only currently supported for
<p class=note><a for=notification>Actions</a> are only currently supported for
<a>persistent notifications</a>.

<li><p>If <var>options</var>'s <code>silent</code> is true and <var>options</var>'s
Expand Down Expand Up @@ -227,27 +250,36 @@ 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>
<p>For each <var>entry</var> in <var>options</var>'s <code>actions</code>:

<ol>
<li><p>Let <var>action</var> be a new <a lt="actions">action</a>.
<li><p>If the user agent does not support additional actions, then break.
Copy link
Member

Choose a reason for hiding this comment

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

It would be worthwhile to continue to explicitly refer to maximum number of actions - it's not entirely clear to me what "additional actions" would refer to otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Copy link
Author

Choose a reason for hiding this comment

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

Done. (And sorry for not 'starting a review' for these comments!)


<li><p>Let <var>action</var> be a new <a for=notification>action</a>.

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

<li><p>Set <var>action</var>'s <a for=action>name</a> to the <var>entry</var>'s
<code>action</code>.
<li>
<p>If the user agent does not support additional actions of this <a for=action>type</a>, then
continue.
Copy link
Member

Choose a reason for hiding this comment

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

This means that we wouldn't append the action to the action list, so they'd be silently dropped. I worry a bit that this would be a footgun, E.g.,

registration.showNotification('title', {
  actions: [
    { type: 'text', action: 'reply', title: 'Reply' },
    { action: 'block', title: 'Block' }
  ]
});

addEventListener('notificationclick', event => {
  event.notification.actions[0] = ???;
});

It would be reply for user agents that support text actions, but block for user agents that do not. It's also inconsistent with the note about action buttons, which implies they'd still be presented. ("The ability to...a chat window.")

Instead, I think it would be more predictable if we'd add them to the list of actions anyway, and add a note in Section 2.6. explaining that action buttons of an unsupported type are expected to (gracefully) degrade to button?

Copy link
Author

Choose a reason for hiding this comment

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

ok; removed the stuff here and am adding the following to section 2.6 (Showing a notification), as a note:

"If the platform does not support entering text within a
notification, implementers are expected to treat text actions as regular buttons and update their
type to button accordingly."

How does that sound?


<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 <var>entry</var>'s <code>action</code>.

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

<li><p>Set <var>action</var>'s <a for=action>title</a> to the
<var>entry</var>'s <code>title</code>.
<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.

<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.)
<li><p>If <var>entry</var>'s <code>placeholder</code> is present, then 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>.
<li><p>Append <var>action</var> to <var>notification</var>'s <a for=notification>action list</a>.
</ol>

<li><p>Return <var>notification</var>.
Expand Down Expand Up @@ -418,8 +450,8 @@ interpreted as a language tag. Validity or well-formedness are not enforced. [[!

<li>
<p>If the notification platform supports actions and action icons, then for each <var>action</var>
in <var>notification</var>'s list of <a>actions</a> <a for=/>fetch</a> <var>action</var>'s
<a for=action>icon URL</a>, if <a for=action>icon URL</a> is set.
in <var>notification</var>'s <a for=notification>action list</a>: <a for=/>fetch</a>
<var>action</var>'s <a for=action>icon URL</a>, if <a for=action>icon URL</a> is set.

<p class=note>The intent is to fetch this resource similar to an
<a href="https://html.spec.whatwg.org/multipage/images.html#update-the-image-data"><code>&lt;img&gt;</code></a>,
Expand Down Expand Up @@ -517,15 +549,22 @@ 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 <code>text</code> was activated by the user, and the opportunity to
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
<var>global</var>.
named <code>notificationclick</code> given <var>notification</var>, <var>action</var> and
<var>reply</var> on <var>global</var>.

<li><p>Then run <a>Handle Functional Event</a> with <var>notification</var>'s
<a for=notification>service worker registration</a> and <var>callback</var>.
Expand Down Expand Up @@ -666,9 +705,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 @@ -861,35 +907,41 @@ return the <a>notification</a>'s <a for=notification>require interaction prefere
result of the following steps:

<ol>
<li><p>Let <var>frozenActions</var> be an empty list of type {{NotificationAction}}.
<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>action list</a>:

<ol>
<li><p>Let <var>action</var> be a new {{NotificationAction}}.
<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>.

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

<li><p>Set <var>action</var>'s {{NotificationAction/title}} to
<var>entry</var>'s <a for=action>title</a>.
<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/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.
It would be better to do this with IDL primitives instead of JS - see
https://www.w3.org/Bugs/Public/show_bug.cgi?id=29004 -->
<li><p>Call <a lt="freeze">Object.freeze</a> on <var>action</var>, to
prevent accidental mutation by scripts.
<!-- 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.
It would be better to do this with IDL primitives instead of JS - see
https://www.w3.org/Bugs/Public/show_bug.cgi?id=29004 -->
<li><p>Call <a lt="freeze">Object.freeze</a> on <var>action</var>, to prevent accidental mutation
by scripts.

<li><p>Append <var>action</var> to <var>frozenActions</var>.
<li><p>Append <var>action</var> to <var>frozenActions</var>.
</ol>

<li><p><a>Create a frozen array</a> from <var>frozenActions</var>.
<li><p><a>Create a frozen array</a> from <var>frozenActions</var>.
</ol>

<h3 id=examples>Examples</h3>
Expand Down Expand Up @@ -1000,11 +1052,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 @@ -1084,11 +1138,13 @@ the same underlying <a>notification</a> of {{Notification}} objects already in e
<hr>

<p>To <dfn>fire a service worker notification event named <var>e</var></dfn>
given <var>notification</var> and <var>action</var>,
given <var>notification</var>, <var>action</var> and <var>reply</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 +1153,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 +1181,7 @@ was initialized to.
Addison Phillips,
Aharon (Vladimir) Lanin,
Alex Russell,
Anita Woodruff,
Anssi Kostiainen,
Arkadiusz Michalski,
Boris Zbarsky,
Expand Down