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

Notification events not working with service workers #68

Open
luke-john opened this issue Aug 22, 2016 · 18 comments
Open

Notification events not working with service workers #68

luke-john opened this issue Aug 22, 2016 · 18 comments

Comments

@luke-john
Copy link

Currently on browsers with service workers push.js is not able to handle any events on the notification.

push.js/push.js

Line 189 in 93014bc

if (w.navigator) {

@Nickersoft
Copy link
Owner

Could you please elaborate?

@luke-john
Copy link
Author

Sure,

So when using a browser such as android chrome which does not have support for the Notification API, push.js uses a serviceworker instead.

Currently the logic push.js uses for serviceworker push notifications is to simply register the serviceworker and then call showNotification on it.

This means pretty much all the logic following this is unused

push.js/push.js

Lines 245 to 284 in 93014bc

id = addNotification(notification);
/* Wrapper used to get/close notification later on */
wrapper = {
get: function () {
return notification;
},
close: function () {
closeNotification(id);
}
};
/* Autoclose timeout */
if (options.timeout) {
setTimeout(function () {
wrapper.close();
}, options.timeout);
}
/* Notification callbacks */
if (isFunction(options.onShow))
notification.addEventListener('show', options.onShow);
if (isFunction(options.onError))
notification.addEventListener('error', options.onError);
if (isFunction(options.onClick))
notification.addEventListener('click', options.onClick);
onClose = function () {
/* A bit redundant, but covers the cases when close() isn't explicitly called */
removeNotification(id);
if (isFunction(options.onClose)) {
options.onClose.call(this);
}
}
notification.addEventListener('close', onClose);
notification.addEventListener('cancel', onClose);
// notification will be undefined

As a result the following push.js options do not work on android chrome

onClick: Callback to execute when the notification is clicked.
onError: Callback to execute when if the notification throws an error.

I've had a quick look at the service worker documentation and it doesn't look like it's trivial to wire up that logic to the serviceworker notification.

@Nickersoft
Copy link
Owner

Oh shucks, you're right. The notification variable is never being assigned. I'll start working on a fix as soon as I can. Shouldn't be too hard.

@Nickersoft
Copy link
Owner

Thanks for the explanation

@Nickersoft
Copy link
Owner

I apologize that I've been sort of MIA for these past few months, but I am currently working on merging in a change that will fix this issue.

@nickmask
Copy link

@Nickersoft Any update on when mobile notifications will start working again?

@Nickersoft
Copy link
Owner

@nickmask Mobile notifications never stopped working (unless there is some issue I am not aware of). This issue had to do with notification events not firing and throwing console errors on mobile. While this issue has been fixed in bc8932d, it has not been published to NPM yet, as there were some additional changes I was hoping to make before the next release.

@dmitry-korolev
Copy link

dmitry-korolev commented Nov 18, 2016

While this issue has been fixed in bc8932d

Was it? Did you check it? As far as i see, it wasn't (or even it can't be) fixed - there is no such event as "notificationclink" outside of service worker.

The ServiceWorkerGlobalScope.onnotificationclick property is an event handler called whenever the notificationclick event is dispatched on the ServiceWorkerGlobalScope object, that is when a user clicks on a displayed notification spawned by ServiceWorkerRegistration.showNotification().

Notifications created on the main thread or in workers which aren't service workers using the Notification() constructor will instead receive a click event on the Notification object itself.

Reference: https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerGlobalScope/onnotificationclick

My current solution is to add notificationclick event listener inside the service worker and then pass it to the parent window via postMessage. You can find an example here - https://github.com/dmitry-korolev/push-js

@Oupsla
Copy link

Oupsla commented Mar 20, 2017

Hello @Nickersoft ,

Same problem here, the notification is shown on mobile but methods (onClick, onShow, ...) don't trigger. Any news about this ?

Thanks !

@Glennmen
Copy link

@Nickersoft What is the status of this issue? How to get onClick to work on Android Chrome browser?

@Nickersoft
Copy link
Owner

Hi guys. I'm currently looking into an issue where the link option doesn't seem to be operational on the Push homepage. onClick is in the same block of code so it's most likely a related issue. It's weird though... could have sworn link was working. Also, @Oupsla, onClick is the only event that can be triggered on mobile notifications (I thought it said that in the documentation... I might have accidentally removed it). This is just due to how the notifications framework operates. The Notifications API (which mobile notifications don't use) is the only API that supports onShow, onError, etc. Sorry for the snag in the release of v1.0. I'll see if I can get it resolved within a few days.

@anthonyG78
Copy link

anthonyG78 commented Oct 11, 2017

Hello @Nickersoft ,
Any news about the onClick issue on Android Chrome browser?
I recently test Push.js 1.0.5, works fine on desktop but unfortunately, it only display the notification without click action on mobile (Chrome 61).
Thanks

Edit: with link: '/#' option, browser show up and load a new page of the website.

@afridi26
Copy link

HI @Nickersoft please let us know why the onclick is not working on Android chrome?

It is not fixed???

@Nickersoft
Copy link
Owner

To be completely honest, I forget what the outcome of my investigation was :/ I can try to reproduce the issue again soon. Looping in @theLufenk to see if he may be able to assist as well.

@theLufenk
Copy link
Collaborator

@Nickersoft
I was off the grid for a while. Sorry about that !

AFAIK, @dmitry-korolev is correct. The only possible solution IMO would be adding an Event Listener inside the service worker.

I don't think we want to force the folks to use a compulsory piece of code in their ServiceWorkers.
However, since an EventListener for notificationclick is anyhow required for onclick handler to work, we can reflect the same in the Documentation and also add it to the barebones ServiceWorker that has already been provided in the project.

There is another problem though. While Notification.onclick enables us to attach the click handler function to every Notification instance we create, the ServiceWorker logic for the same entails registering a common Event Handler for all notification generated.
But, we do have the 'tag' property to make out the different Notification instances from one another.

So in effect, we can have a message event listener inside Push.js that fires when the ServiceWorker send a message with postMessage. This event listener will need to figure out if it's a message relevant to Push.js (can be done by choosing a unique event string such as "PushJSNotificationClickEventMessage"). The "tag" property of the target notification would be a payload for the event. The "tag" can be used to find the notification instance from the notifications={} mapping we keep. This instance now has a onclick property which is the target function that needs to be invoked.

@Nickersoft @dmitry-korolev I would love to have your opinions on this approach and see if it is a possible solution ATM. And also, how we can refine the process in order not to cause a regression on the "Install and use with minimum hassles" aspect of the library.

Let me also try and send in a pull request. Doesn't seem like a lot of changes TBH.

@theLufenk
Copy link
Collaborator

nudge @Nickersoft @dmitry-korolev

@Nickersoft
Copy link
Owner

Hey @theLufenk @dmitry-korolev, sorry for the delay here – I think this approach sounds reasonable if you wanted to give it a shot.

@avnishalok
Copy link

avnishalok commented Feb 16, 2021

Add this event listner into your service worker For android chrome

self.addEventListener('notificationclick', function(event) { let url = "https://www.google.com"; event.notification.close(); // Android needs explicit close. event.waitUntil( clients.matchAll({type: 'window'}).then( windowClients => { // Check if there is already a window/tab open with the target URL for (var i = 0; i < windowClients.length; i++) { var client = windowClients[i]; // If so, just focus it. if (client.url === url && 'focus' in client) { return client.focus(); } } // If not, then open the target URL in a new window/tab. if (clients.openWindow) { return clients.openWindow(url); } }) ); });

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

No branches or pull requests

10 participants