-
Notifications
You must be signed in to change notification settings - Fork 49
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
base: main
Are you sure you want to change the base?
Conversation
- Migrated existing html test to .any.js test + .idl interface to allow testing the new properties from a service worker scope. - See whatwg/notifications#132 for the spec change this tests. - See https://github.com/anitawoodruff/inline-notification-replies for an Explainer.
TAG review now requested at: w3ctag/design-reviews#284 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch! Here's some initial feedback. Hope that helps.
notifications.bs
Outdated
<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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
notifications.bs
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
<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). |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
|
||
<p>A <a>notification action</a> has a <dfn id=notification-action-title>title</dfn> (a DOMString). | ||
|
||
<p>A <a>notification action</a> has a <dfn id=notification-action-name>name</dfn> (a DOMString). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
notifications.bs
Outdated
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
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>: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<li><p>
throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
|
||
<ol> | ||
<li>If the user agent does not support additional actions, break. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then break*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
<code>type</code> property. | ||
|
||
<li>If the user agent does not support additional actions of this | ||
<a for="notification action">type</a>, continue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then continue*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
|
||
<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 |
There was a problem hiding this comment.
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>
"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{{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>. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 }
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the notes!
notifications.bs
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
<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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
|
||
<p>A <a>notification action</a> has a <dfn id=notification-action-title>title</dfn> (a DOMString). | ||
|
||
<p>A <a>notification action</a> has a <dfn id=notification-action-name>name</dfn> (a DOMString). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
notifications.bs
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
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>: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
|
||
<ol> | ||
<li>If the user agent does not support additional actions, break. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
<code>type</code> property. | ||
|
||
<li>If the user agent does not support additional actions of this | ||
<a for="notification action">type</a>, continue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
notifications.bs
Outdated
|
||
<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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{{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>. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you're okay with me pushing a formatting fixup commit and when would be a good time to avoid interrupting your work on this PR.
notifications.bs
Outdated
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 action list (a <a for=/>list</a> of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"action list" needs to be a <dfn>
(and some places that currently refer to "actions" need to be updated accordingly)
notifications.bs
Outdated
<dfn>maximum number of actions</dfn> supported, within the constraints of the | ||
notification platform. | ||
<p>A <a>notification</a> has an action list (a <a for=/>list</a> of | ||
<dfn for=notification lt=action id=actions>actions</dfn>). It is initially empty. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should become a reference <a for=notification>action</a>
and then this <dfn>
can replace the <dfn>
below so we just have "action" and not "notification action".
Go for it, any time is fine - from my perspective the changes are done aside from your comments :) |
I addressed some of the formatting issues. There's still at least one outstanding issue though. Review from @beverloo would also be good. What we need beyond this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thanks Anita!
notifications.bs
Outdated
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
notifications.bs
Outdated
|
||
<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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
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!)
notifications.bs
Outdated
<code>action</code>. | ||
<li> | ||
<p>If the user agent does not support additional actions of this <a for=action>type</a>, then | ||
continue. |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
{{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>. |
There was a problem hiding this comment.
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 }
Filed the following implementation bugs against the other browsers: |
(Chrome bug is at https://crbug.com/599859 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to reword "encuraged" to "required".
@aliams, @hober, @annevk - Please can you comment on whether your respective browsers would be interested in implementing this feature. Since the native notification centers on Mac, Android, iOS and Windows now all provide this functionality, we think it would make a great addition to the spec. See the explainer for more details. |
Text actions should be treated as button actions for display purposes only - their type should not be updated.
In response to TAG Review, emphasize that user agents should make it clear that replies entered in a notification will be sent to the notification's origin.
@anitawoodruff the main input I got at mozilla/standards-positions#91 (comment) from @martinthomson was that it was unclear how to deploy this in a backwards compatible fashion. That is, how can a website use this feature in a way that doesn't make things weird in browsers that don't support it? |
Open a window with a view that allows them to send a response, as that's clearly the user's intention by activating that button? The website would want to do something similar if the user doesn't enter an inline reply, i.e. just clicks on the button. |
It was more about feature detection I think, but it seems that should be doable with |
FWIW, @jakearchibald offered to help with rebasing this. I think the main thing lacking here is explicit interest from one more browser. And perhaps some non-IDL tests. |
Maybe it would be useful to expand the API to allow the reply to be prefilled with some value? Use-case:
It is common in some platforms (such as Mastodon and Twitter) for This is not possible to achieve with the current API. |
Just as a note from us all the way over here in user-land, I was just wondering about this particular feature today (whether it was possible with the Notification API): https://twitter.com/karlhorky/status/1167778767542128643 Use Case: It came up as part of a discussion about whether a Slack PWA could hold its own against an Electron app: https://twitter.com/karlhorky/status/1167777816466866177 Thanks for listening! |
Ah, I guess this feature is already (partially) implemented in Chrome: https://twitter.com/beverloo/status/1167824727009832960 However, they are disabled on macOS due to deficiencies in the macOS notification system (extra buttons for "Site Settings" are not allowed): https://twitter.com/beverloo/status/1167847717361639425 This is a valid user concern, so I have proposed a few ways of dealing with that, including an extra permissions prompt (1) and requiring an installed PWA (2 and 3).
cc @beverloo |
So this been live in Chrome for Windows/ChromeOS/Android for over 6 years, but the update for the spec has been forgot on this PR? Can we merge this so it will trigger the necessary updates in affects specs like https://specifications.freedesktop.org/notification-spec/notification-spec-latest.html#protocol that still doesn't have support for the |
This change adds support for notifications that accept text input within their UI, aka 'inline replies'.
Explainer: https://github.com/anitawoodruff/inline-notification-replies
Requesting TAG review shortly, will link to it below.
Preview | Diff