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

feat(service-worker): handle 'notificationclick' events #25860

Closed
wants to merge 10 commits into
base: master
from

Conversation

@joostme
Contributor

joostme commented Sep 7, 2018

PR Checklist

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

The current version of the service worker does not allow to handle clicks on the notification or be informed which notification or action was clicked.

Issue Number: #20956, #22311

What is the new behavior?

The new behaviour also listens for the 'notificationclick' event and provides an observable of all messages that were clicked in the SwPush service.

swPush.messagesClicked.pipe(
  ...
);

The emitted values will be of the following type:

{
  action: string,
  notification: NotificationObject
}

where NotificationObject is the following interface:

export interface NotificationObject extends NotificationOptions { title: string; }

If no custom actions are defined in the notification, action will be undefined.

Does this PR introduce a breaking change?

[ ] Yes
[x] No
@gkalpak

I think this is heading in the right direction. Thx, @joostme 👍
I left a few commets/questions.

(msg: {message: string}) => receivedMessages.push(msg.message));
sendMessage('NOTIFICATION_CLICK', 'this was a click');
sendMessage('NOT_OTIFICATION_CLICK', 'this was not a click');

This comment has been minimized.

@gkalpak

gkalpak Sep 17, 2018

Member

This should either be NOT_NOTIFICATION_CLICK or NOT_IFICATION_CLICK (for extra fun 😃).

@@ -83,6 +83,8 @@ interface PushEvent extends ExtendableEvent {
data: PushMessageData;
}
interface NotificationClickEvent extends NotificationEvent, ExtendableEvent {}

This comment has been minimized.

@gkalpak

gkalpak Sep 17, 2018

Member

AFAICT, there is no such thing as a NotificationClickEvent. It is actually the NotificationEvent , so maybe we should just append extends ExtendableEvent to the NotificationEvent interface above.

@@ -333,6 +342,9 @@ class MockPushEvent extends MockExtendableEvent {
json: () => this._data,
};
}
class MockNotificationClickEvent extends MockExtendableEvent {

This comment has been minimized.

@gkalpak

gkalpak Sep 17, 2018

Member

Nit: MockNotificationEvent is probably more accurate.

Show resolved Hide resolved packages/service-worker/worker/src/driver.ts
Show resolved Hide resolved packages/service-worker/worker/src/driver.ts
@joostme

This comment has been minimized.

Contributor

joostme commented Sep 21, 2018

@gkalpak I implemented your suggested changes 😊

Furthermore the client is now focused if a notification is clicked and the client allows focusing.
I tested all this locally and it also works with the manual serialization of the notification.

I think opening the browser window or navigating to a specific url should also be possible. Especially if a notification is clicked while no window is active or opened. But that is something for another PR in my opinion.

@JonnyBGod

This comment has been minimized.

JonnyBGod commented Sep 21, 2018

For your inspiration:

this.scope.addEventListener('notificationclick', (event) => {
            event.notification.close();
            event.waitUntil(clients.matchAll({
              type: "window"
            }).then(function(clientList) {
              const link =
                event.notification.data &&
                event.notification.data.fcmOptions &&
                event.notification.data.fcmOptions.link;

              for (var i = 0; i < clientList.length; i++) {
                var client = clientList[i];
                if (link && link === client.url) {
                  return client.focus();
                } else if ('focus' in client) {
                  if (link) {
                    client.navigate(link);
                  }
                  return client.focus();
                }
              }
              if (clients.openWindow) {
                return clients.openWindow(link || '/');
              }
            }));
          });

this is working for me.

@gkalpak

Getting there 👍
Let's keep this PR simple (and safe for future modification) and then discuss more advanced usecases in another issue/PR.

Show resolved Hide resolved packages/service-worker/test/comm_spec.ts Outdated
Show resolved Hide resolved packages/service-worker/test/comm_spec.ts Outdated
@@ -950,6 +974,18 @@ export class Driver implements Debuggable, UpdateSource {
}, Promise.resolve());
}
async broadcastAndFocus(msg: Object): Promise<void> {
const clients = await this.scope.clients.matchAll();

This comment has been minimized.

@gkalpak

gkalpak Sep 21, 2018

Member

It may make sense to use type: 'window' here.

@@ -286,6 +296,20 @@ export class Driver implements Debuggable, UpdateSource {
await this.scope.registration.showNotification(desc['title'] !, options);
}
private async handleClick(notification: any, action?: string): Promise<void> {

This comment has been minimized.

@gkalpak

gkalpak Sep 21, 2018

Member

Why not properly type notification as Notification?

private async handleClick(notification: any, action?: string): Promise<void> {
(notification as Notification).close();
const desc = notification as{[key: string]: string | undefined};

This comment has been minimized.

@gkalpak

gkalpak Sep 21, 2018

Member

Why is this necessary?

@@ -33,6 +33,10 @@ const NOTIFICATION_OPTION_NAMES = [
'actions', 'badge', 'body', 'dir', 'icon', 'lang', 'renotify', 'requireInteraction', 'tag',
'vibrate', 'data'
];
const NOTIFICATION_CLICK_OPTION_NAMES = [
'actions', 'badge', 'title', 'body', 'dir', 'icon', 'lang', 'renotify', 'requireInteraction',

This comment has been minimized.

@gkalpak

gkalpak Sep 21, 2018

Member

What about image and timestamp?

@@ -333,6 +342,10 @@ class MockPushEvent extends MockExtendableEvent {
json: () => this._data,
};
}
class MockNotificationEvent extends MockExtendableEvent {
constructor(private _notification: any, readonly action?: string) { super(); }
readonly notification = {...this._notification, close: () => { return; }};

This comment has been minimized.

@gkalpak

gkalpak Sep 21, 2018

Member

Nit: If you want a no-op method, you can also do close: () => undefined (or close() { return; }) 😉

clients.forEach((client: any) => {
if ('focus' in client) {
if (!client.focused) {
client.focus();

This comment has been minimized.

@gkalpak

gkalpak Sep 21, 2018

Member

According to w3c/ServiceWorker#602 (and my experiments) you can only call one method of clients.openWindow() or client.focus() (so you can't call .focus() on all clients). Why would you do that anyway?

We need to think this through. Let's keep it simple in this PR (and avoid having to introduce a breaking change in the future).
I suggest we only broadcast and let the apps deal with the message.

In a future PR, we can discuss other options. Off the top of my head:

  • If there are no open clients:

    1. Open a new window at ServiceWorkerRegistration#scope.
    2. Open a new window at a URL defined in Notification#data.url (or some other property we agree on).
    3. Have another convention to specify URLs per action in Notification#data.urlPerAction. Then we could look up the URL to open based on the action's name.
  • If there are clients open:

    1. Focus the last focused client.
    2. Navigate the last focused client to a specific URL (e.g. if specified in Notification#data.url or Notification#data.urlPerAction).
  • Alternatively, we could specify some operations per action according to some predefined convention. E.g.:

    notification: NotificationOptions = {
      actions: [
        {action: 'foo', title: 'Open new tab'},
        {action: 'bar', title: 'Navigate in last focused tab'},
        {action: 'baz', title: 'Focus last focused tab'},
        {action: 'qux', title: 'Focus last focused tab (if any) or open new tab'},
      ],
      data: {
        onActionClick: {
          foo: {operation: 'openWindow', url: 'http://example.com/foo'},
          bar: {operation: 'navigateLastFocused', url: 'http://example.com/bar'},
          baz: {operation: 'focusLastFocused'},
          qux: {operation: 'focusLastFocusedOrOpen', url: 'http://example.com/qux'},
        },
      },
    }
@@ -21,6 +21,7 @@ import {ERR_SW_NOT_SUPPORTED, NgswCommChannel} from './low_level';
@Injectable()
export class SwPush {
readonly messages: Observable<object>;
readonly messagesClicked: Observable<object>;

This comment has been minimized.

@gkalpak

gkalpak Sep 21, 2018

Member

We should have some API docs for this (see #23138).
We should mention which properties the payload will have (action and notification) and make it clear that the notification is not a Notification object (but more like a NotificationOptions obejct).

@joostme

This comment has been minimized.

Contributor

joostme commented Sep 28, 2018

@gkalpak I updated the pull request with according to your feedback. Again thank you for your great feedback 😊!

  • I removed the focus of the window for now to keep things simple in this PR. The click handler now uses the normal broadcast method.
  • I also properly typed the messagesClicked stream in the SwPsuh service and added JSDocs.
  • image and timestamp are now added to the Notification object in the messagesClicked stream.

The handleClick method copies the notification object and creates an options object because the broadcast method uses postMessage. This serializes the data but is unable to serialize the notification object as it contains functions. So I copy the necessary properties into the options object. The same solution is also used in the handlePush method above.

@gkalpak gkalpak added the aio: preview label Oct 1, 2018

@gkalpak

Thx for making the changes, @joostme 👍
Here's another round of feedback (since you seem to be enjoying it) 😛

action: string;
notification: NotificationObject
}
> ;

This comment has been minimized.

@gkalpak

gkalpak Oct 1, 2018

Member

This block's formatting seems odd. Did the formatter format it like this?
I would expected something like: readonly messagesClicked: Observable <{action: string; notification: NotificationObject;}>;

This comment has been minimized.

@joostme

joostme Oct 1, 2018

Contributor

Yeah the clang formatter formatted it like this 🤔. I know it looks kind of strange

* interacted with.
* If no action was used the action property will be an empty string `''`.
*
* Note that the `notification` property is __not__ a

This comment has been minimized.

@gkalpak

gkalpak Oct 1, 2018

Member

Use ** for bold (for consistency with the rest of the docs).

* If no action was used the action property will be an empty string `''`.
*
* Note that the `notification` property is __not__ a
* [Notification](https://developer.mozilla.org/en-US/docs/Web/API/Notification) but rather a

This comment has been minimized.

@gkalpak

gkalpak Oct 1, 2018

Member

Consider changing [Notification](...) to [Notification](...) object. (I think it reads better, because Notification is basically an interface.)

* Note that the `notification` property is __not__ a
* [Notification](https://developer.mozilla.org/en-US/docs/Web/API/Notification) but rather a
* [NotificationOptions](https://notifications.spec.whatwg.org/#dictdef-notificationoptions)
* object that also includes the notification `title`.

This comment has been minimized.

@gkalpak

gkalpak Oct 1, 2018

Member

What is the notification title? Where does it come from?

This comment has been minimized.

@joostme

joostme Oct 1, 2018

Contributor

The title comes from the Notification object. I will add this so it is more clear

@@ -45,6 +45,8 @@ interface StatusEvent {
error?: string;
}
export interface NotificationObject extends NotificationOptions { title: string; }

This comment has been minimized.

@gkalpak

gkalpak Oct 1, 2018

Member

Why not put this in push.ts instead?

That said, I think it is better to keep the public API changes as small as possible. If this interface is used as part of messagesClick (which is public API), then it also needs to be exported from @angular/service-worker.

I would suggest using NotificationOptions & {title: string} directly on messagesClicked's signature.

* [NotificationOptions](https://notifications.spec.whatwg.org/#dictdef-notificationoptions)
* object that also includes the notification `title`.
*/
readonly messagesClicked: Observable < {

This comment has been minimized.

@gkalpak

gkalpak Oct 1, 2018

Member

I just realized that messagesClicked is not a very intuitive name. It implies a 1-to-1 relationship with messages, but not all messages have corresponding notifications.
Why not use notificationClicks instead? 😁

@@ -33,6 +33,10 @@ const NOTIFICATION_OPTION_NAMES = [
'actions', 'badge', 'body', 'dir', 'icon', 'lang', 'renotify', 'requireInteraction', 'tag',
'vibrate', 'data'
];
const NOTIFICATION_CLICK_OPTION_NAMES = [
'actions', 'badge', 'title', 'body', 'dir', 'icon', 'image', 'lang', 'renotify',
'requireInteraction', 'tag', 'timestamp', 'vibrate', 'data'

This comment has been minimized.

@gkalpak

gkalpak Oct 1, 2018

Member

Can we have the properties in alphabetic order 🙏
Also, missing the silent option.

notification.close();
const desc = notification as any;
let options: {[key: string]: string | undefined} = {};

This comment has been minimized.

@gkalpak

gkalpak Oct 1, 2018

Member
  1. let --> const

  2. The | undefined part is redundant.

Useless fact of the day:
If you wanted to type options like a boss™️ (😛), you could use {-readonly [key in keyof Notification]?: Notification[key]}.
But that is totally unnecessary here (and I am not even sure it works for all TS versions that Angular supports atm 😁).

private async handleClick(notification: Notification, action?: string): Promise<void> {
notification.close();
const desc = notification as any;

This comment has been minimized.

@gkalpak

gkalpak Oct 1, 2018

Member

This shouldn't be necessary if you correctly type NOTIFICATION_CLICK_OPTION_NAMES as (keyof Notification)[].

This comment has been minimized.

@joostme

joostme Oct 1, 2018

Contributor

This will not work as Notification includes a lot more properties than those listed in NOTIFICATION_OPTION_NAMES like onclick, onclose etc.

This comment has been minimized.

@gkalpak

gkalpak Oct 2, 2018

Member

Which part won't work? And why?
(I know keyof Notification is a superset of the allowed values, but it is still more strict than the currently inferred string[] type.)

This comment has been minimized.

@joostme

joostme Oct 2, 2018

Contributor

I now actually got this working 👏. The NOTIFICATION_OPTION_NAMES is now of type (keyof Notification)[] and the options object of type {-readonly: [key in keyof Notification]?: Notification[key]}.

The problem I had before were the missing properties in the NOTIFICATION_OPTION_NAMES array. But the TS error I got was so misleading I did not catch that.

New version is on its way!

@@ -33,6 +33,10 @@ const NOTIFICATION_OPTION_NAMES = [
'actions', 'badge', 'body', 'dir', 'icon', 'lang', 'renotify', 'requireInteraction', 'tag',
'vibrate', 'data'
];
const NOTIFICATION_CLICK_OPTION_NAMES = [

This comment has been minimized.

@gkalpak

gkalpak Oct 1, 2018

Member

Actually, why have a different array and not re-use NOTIFICATION_OPTION_NAMES?
The only difference should be title.

We can either include title (since it won't matter) or omit it from NOTIFICATION_OPTION_NAMES and have a second list which adds title:

const NOTIFICATION_OPTION_NAMES = [...];
const NOTIFICATION_PROPERTY_NAMES = NOTIFICATION_OPTION_NAMES.concat('title');
@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 1, 2018

@joostme

This comment has been minimized.

Contributor

joostme commented Oct 1, 2018

I refactored it and did the following changes:

  • Rename messagesClicked to notificationClicks
  • Update JSDoc for notificationClicks to be more precise on where the title comes from
  • Add missing properties to the NOTIFICATION_OPTION_NAMES array and use this one instead of a new one just for the handleClick method
  • Use intersection type NotificationOptions & { title: string } in SwPush service

I was not able to remove the any cast in the handleClick method even with your super fancy typing for options. Typescript is still giving me warnings when not casting it to any because of a missing index signature on the Notification interface 😐

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 2, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 2, 2018

@gkalpak

Really close now 😃

Show resolved Hide resolved packages/service-worker/src/push.ts Outdated
Show resolved Hide resolved packages/service-worker/worker/src/driver.ts Outdated
Show resolved Hide resolved packages/service-worker/worker/src/driver.ts Outdated
@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 2, 2018

@joostme

This comment has been minimized.

Contributor

joostme commented Oct 2, 2018

I made the changes you suggested @gkalpak . 😊👏

  • The NOTIFICATION_OPTION_NAMES now has the proper (keyof Notification)[] typing
  • The options object now has the {-readonly: [key in keyof Notification]?: Notification[key]} typing to be absolutely correct.
  • I removed the NotificationObject interface in favor of the existing NotificationOptions interface
  • The filter in handleClick now uses name in notification instead of hasOwnProperty as hasOwnProperty actually always returns false because the properties only exist in the Notification prototype. I tested this locally and it works properly now.
  • I reverted the unwanted line-breaks in the comments. My tslint wanted to format those lines 😶
@gkalpak

Almost there 😃 Keep it up 😛

* Note that the `notification` property is **not** a
* [Notification](https://developer.mozilla.org/en-US/docs/Web/API/Notification) object but rather
* a
* [NotificationOptions](https://notifications.spec.whatwg.org/#dictdef-notificationoptions)

This comment has been minimized.

@gkalpak

gkalpak Oct 2, 2018

Member

I believe this line can be joined with the previous one, right?

This comment has been minimized.

@joostme

joostme Oct 2, 2018

Contributor

I formatted it properly and also refactored it so there is not duplicate link to the mozilla Notification API site

Show resolved Hide resolved packages/service-worker/worker/src/driver.ts
Show resolved Hide resolved packages/service-worker/worker/src/driver.ts
Show resolved Hide resolved packages/service-worker/worker/test/happy_spec.ts Outdated
Show resolved Hide resolved packages/service-worker/worker/src/driver.ts
@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 2, 2018

@gkalpak

gkalpak approved these changes Oct 2, 2018

A couple of comment formatting. Other than that LGTM 👌

* If no action was used the action property will be an empty string `''`.
*
* Note that the `notification` property is **not** a
* [Notification][Mozilla Notification] object but rather

This comment has been minimized.

@gkalpak

gkalpak Oct 2, 2018

Member
  • Can't the link go to the line above?
  • Double space before "object".
Show resolved Hide resolved packages/service-worker/src/push.ts
* [Notification][Mozilla Notification] object but rather
* a [NotificationOptions](https://notifications.spec.whatwg.org/#dictdef-notificationoptions)
* object that also includes the `title` of the
* [Notification][Mozilla Notification] object.

This comment has been minimized.

@gkalpak

gkalpak Oct 2, 2018

Member

Can you please format these lines to have as few of them as possible?

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 3, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Oct 3, 2018

@joostme

This comment has been minimized.

Contributor

joostme commented Oct 4, 2018

@gkalpak I changed the formatting of the JSDoc. Not everything could be optimized because of the clang formatter. But it's a bit cleaner now ☺
Thank you for taking so much time and providing great feedback 👍🏻

@gkalpak

gkalpak approved these changes Oct 5, 2018

LGTM 💯
Thank you for working on this and following through ❤️

Looking forward to the next PR (#25860 (comment) maybe?) 😉

@gkalpak gkalpak requested a review from IgorMinar Oct 5, 2018

joostme added some commits Sep 28, 2018

feat(service-worker): Add typing for messagesClicked in SwPush service
- Properly type messagesClicked Observable stream
- Add new NotificationObject interface
refactor(service-worker): Rework notification click handler
- Add missing image and timestamp properties
- Remove focus from click handler
refactor(service-worker): Format comments and add additional test
- Format JSDoc for notificationClicks
- Add comment on why handleClick does not use hasOwnProperty
- Add additional test that uses handleClick without action
@IgorMinar

This comment has been minimized.

Member

IgorMinar commented Nov 1, 2018

I've rebased this PR. hopefully the CI will go green. If not, we'll need to investigate.

@mary-poppins

This comment has been minimized.

mary-poppins commented Nov 1, 2018

@kara kara closed this in cf6ea28 Nov 1, 2018

kara added a commit that referenced this pull request Nov 1, 2018

fix(service-worker): add missing api typing (#25860)
Add missing messagesClicked typing for SwPush service

PR Close #25860

kara added a commit that referenced this pull request Nov 1, 2018

refactor(service-worker): update tests based of review feedback (#25860)
- Rename invalid click event name
- Make test expects more explicit
- Remove NotificationClickEvent in favor of NotificationEvent

PR Close #25860

kara added a commit that referenced this pull request Nov 1, 2018

feat(service-worker): close notifications and focus window on click (#…
…25860)

- Serialize notification object before using postMessage
- Close notification on click
- Focus browser if it is not already focused on click

PR Close #25860

kara added a commit that referenced this pull request Nov 1, 2018

refactor(service-worker): Code optimizations (#25860)
- Add missing new line in tests
- Add proper noop method in MockNotificationEvent

PR Close #25860

kara added a commit that referenced this pull request Nov 1, 2018

feat(service-worker): Add typing for messagesClicked in SwPush service (
#25860)

- Properly type messagesClicked Observable stream
- Add new NotificationObject interface

PR Close #25860

kara added a commit that referenced this pull request Nov 1, 2018

refactor(service-worker): Rework notification click handler (#25860)
- Add missing image and timestamp properties
- Remove focus from click handler

PR Close #25860

kara added a commit that referenced this pull request Nov 1, 2018

kara added a commit that referenced this pull request Nov 1, 2018

refactor(service-worker): Format comments and add additional test (#2…
…5860)

- Format JSDoc for notificationClicks
- Add comment on why handleClick does not use hasOwnProperty
- Add additional test that uses handleClick without action

PR Close #25860
@gkalpak

This comment has been minimized.

Member

gkalpak commented Nov 1, 2018

🎉 Nice job, @joostme 💯

(BTW, I opened #26907 for discussing possible future enhancements. Feel free to chime in 😉)

sculove added a commit to sculove/angular that referenced this pull request Nov 2, 2018

feat(service-worker): handle 'notificationclick' events (angular#25860)
The previous version did not support the 'notificationclick' event.
Add event handler for the event and provide an observable of
clicked notifications in the SwPush service.

Closes angular#20956, angular#22311

PR Close angular#25860

sculove added a commit to sculove/angular that referenced this pull request Nov 2, 2018

fix(service-worker): add missing api typing (angular#25860)
Add missing messagesClicked typing for SwPush service

PR Close angular#25860

sculove added a commit to sculove/angular that referenced this pull request Nov 2, 2018

refactor(service-worker): update tests based of review feedback (angu…
…lar#25860)

- Rename invalid click event name
- Make test expects more explicit
- Remove NotificationClickEvent in favor of NotificationEvent

PR Close angular#25860

sculove added a commit to sculove/angular that referenced this pull request Nov 2, 2018

feat(service-worker): close notifications and focus window on click (a…
…ngular#25860)

- Serialize notification object before using postMessage
- Close notification on click
- Focus browser if it is not already focused on click

PR Close angular#25860

sculove added a commit to sculove/angular that referenced this pull request Nov 2, 2018

refactor(service-worker): Code optimizations (angular#25860)
- Add missing new line in tests
- Add proper noop method in MockNotificationEvent

PR Close angular#25860

sculove added a commit to sculove/angular that referenced this pull request Nov 2, 2018

feat(service-worker): Add typing for messagesClicked in SwPush service (
angular#25860)

- Properly type messagesClicked Observable stream
- Add new NotificationObject interface

PR Close angular#25860

sculove added a commit to sculove/angular that referenced this pull request Nov 2, 2018

refactor(service-worker): Rework notification click handler (angular#…
…25860)

- Add missing image and timestamp properties
- Remove focus from click handler

PR Close angular#25860

sculove added a commit to sculove/angular that referenced this pull request Nov 2, 2018

sculove added a commit to sculove/angular that referenced this pull request Nov 2, 2018

sculove added a commit to sculove/angular that referenced this pull request Nov 2, 2018

refactor(service-worker): Format comments and add additional test (an…
…gular#25860)

- Format JSDoc for notificationClicks
- Add comment on why handleClick does not use hasOwnProperty
- Add additional test that uses handleClick without action

PR Close angular#25860
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment