Nickersoft / push.js

The world's most versatile desktop notifications framework :earth_americas:
https://pushjs.org
MIT License
8.77k stars 548 forks source link

Safari requestPermission() Promise not supported. #117

Closed ellisio closed 5 years ago

ellisio commented 7 years ago

Hello,

According to the MDN, Safari does not support the requestPermission().then() Promise version.

Reference: https://developer.mozilla.org/en-US/docs/Web/API/Notification/requestPermission#Browser_compatibility

So when Push.Permission.request() is called in Safari users get the following error:

undefined is not an object (evaluating 'this._win.Notification.requestPermission().then')

Due to this block: https://github.com/Nickersoft/push.js/blob/master/src/classes/Permission.js#L34-L39

Cheers, Andrew

ellisio commented 7 years ago

Hey @Nickersoft any update on this?

Nickersoft commented 7 years ago

Hi @ellisio! Sorry, I've been busy lately! You're totally right... I'll see if I can get a fix out by the end of the week. I think if the browser supports the promises API it throws a depreciation warning, so some extra logic will need to be added to prevent that. It's actually surprising, as Safari is one of the browsers testing this library on BrowserStack and all tests pass... maybe the permission tests need to be revisited.

n3il commented 7 years ago

This looks ... promising https://github.com/Nickersoft/push.js/issues/120

Edit: that issue doesn't seem to be an attempt to fix Push.js for Safari.

basiclines commented 7 years ago

Looks like the official Safari web documentation uses a callback for the requestPermission function.

https://developer.apple.com/library/content/documentation/NetworkingInternet/Conceptual/NotificationProgrammingGuideForWebsites/LocalNotifications/LocalNotifications.html#//apple_ref/doc/uid/TP40012932-SW1

n3il commented 6 years ago

Very helpful @basiclines.

I'm still testing, but I opened this PR https://github.com/Nickersoft/push.js/pull/124.

n3il commented 6 years ago

So this PR (https://github.com/Nickersoft/push.js/pull/124) works and is tested against Chrome, Safari, and Firefox.

The Fix: Checking for Safari first means that it uses the callback method instead of the promise-based method which errors out. In addition, the requestPermission method doesn't return the permission name in Safari. So we get it from window instead. Also a plus, assuming that Safari removes webkitNotifications entirely like Chrome did, the promise-based method will be used automatically on that Safari update.

Also worth writing is that Safari is currently listed as having "No support" for promise-based Notifications. https://developer.mozilla.org/en-US/docs/Web/API/Notification/requestPermission

n3il commented 6 years ago

@Nickersoft Thoughts?

You could even add a Push.js specific support table for browser notifications, using the Notifications API.

Nickersoft commented 6 years ago

@n3il I went ahead a merged your PR (sorry I've been so busy lately.. taken me awhile to get around to it). I like your idea of a support table. I'll see if I can put one together and post it on pushjs.org.

n3il commented 6 years ago

@Nickersoft Thanks. I think we can close this ticket out then. Looking forward to it 👍

Nickersoft commented 6 years ago

Cool beans :) Thanks for the contribution @n3il!

dsalzr commented 6 years ago

I'm also getting this error:

undefined is not an object (evaluating 'this._win.Notification.requestPermission().then')

Looks like you committed some code to fix it. Are you planning on releasing 1.0.5 soon?

Thanks!

lensisle commented 6 years ago

Hi !

I'm getting this error on safari too:

undefined is not an object (evaluating 'this._win.Notification.requestPermission().then')

any news about the status of this issue? I'll be glad to help if there's anything more to do to fix this problem.

Thanks!

joshbeckman commented 5 years ago

It seems the thinking here was that the webkitNotification API with callback-based permissions would be kept to support older Safari. The assumption was that Safari would not drop the deprecated webkitNotification API without also updating to the modern Notification/promise-based permissions API.

That didn't happen. :(

Safari (in Mojave, at least) dropped the webkit prefix but also still used the old callback-based permissions API.

andresbravog commented 5 years ago

this is still broken on last package version, can you release a new version?

mxshdev commented 5 years ago

Also not working in last version package.

SohrabZ commented 5 years ago

Any updates on this issue?

tuborgbeer commented 5 years ago

Safari is shit abandon this browser. Forward this issue to Apple they must fix one

RajeshMalviya14 commented 5 years ago

This is working for me if (Notification.permission === "granted") { console.log('allow', result); return true; } if (!Notification.requestPermission()) { return true; } Notification.requestPermission().then(function (result) { if (result === 'denied') { console.log('denied', result); return; } if (result === 'granted') { console.log('allow', result); } }