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

Conversation

anitawoodruff
Copy link

@anitawoodruff anitawoodruff commented May 22, 2018

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

anitawoodruff pushed a commit to anitawoodruff/web-platform-tests that referenced this pull request May 23, 2018
- 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.
@anitawoodruff
Copy link
Author

TAG review now requested at: w3ctag/design-reviews#284

Copy link
Member

@annevk annevk left a 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.
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?

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
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

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).
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

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).
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.

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.
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

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>:
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

notifications.bs Outdated

<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

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.
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

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
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

{{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.

Copy link
Author

@anitawoodruff anitawoodruff left a 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
Copy link
Author

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).
Copy link
Author

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).
Copy link
Author

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.
Copy link
Author

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>:
Copy link
Author

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.
Copy link
Author

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.
Copy link
Author

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
Copy link
Author

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>.
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

@annevk annevk left a 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
Copy link
Member

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.
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 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".

@anitawoodruff
Copy link
Author

Go for it, any time is fine - from my perspective the changes are done aside from your comments :)

@annevk
Copy link
Member

annevk commented May 28, 2018

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:

Copy link
Member

@beverloo beverloo left a 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.
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.

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.
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!)

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.
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?

{{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.

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 }

@anitawoodruff
Copy link
Author

(Chrome bug is at https://crbug.com/599859 )

Copy link

@lknik lknik left a 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".

@anitawoodruff
Copy link
Author

@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.
@annevk
Copy link
Member

annevk commented Aug 2, 2018

@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?

@beverloo
Copy link
Member

beverloo commented Aug 2, 2018

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.

@annevk
Copy link
Member

annevk commented Aug 9, 2018

It was more about feature detection I think, but it seems that should be doable with "reply" in NotificationEvent.prototype or some such.

@annevk
Copy link
Member

annevk commented Aug 9, 2018

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.

@sorin-davidoi
Copy link

Maybe it would be useful to expand the API to allow the reply to be prefilled with some value?

Use-case:

  • User A sends a message to user B: @b how are you?
  • User B wants to reply to the message.

It is common in some platforms (such as Mastodon and Twitter) for B's reply to start with @a, and when replying to such a message the input field is prepopulated with @a.

This is not possible to achieve with the current API.

@karlhorky
Copy link

karlhorky commented Aug 31, 2019

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!

@karlhorky
Copy link

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).

  1. https://twitter.com/karlhorky/status/1167843843238219777
  2. https://twitter.com/karlhorky/status/1167848030718046208
  3. https://bugs.chromium.org/p/chromium/issues/detail?id=999838

cc @beverloo

Base automatically changed from master to main January 15, 2021 07:46
@xfalcox
Copy link

xfalcox commented Dec 2, 2022

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 type attribute in the NotificationAction class ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants