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

Silence failures to trigger notifications when not available #732

Merged
merged 2 commits into from Nov 16, 2016

Conversation

astorije
Copy link
Member

@astorije astorije commented Nov 7, 2016

Fixes #731.

Recent Chrome versions are dropping out new Notification in favor of ServiceWorkerRegistration.showNotification.
This makes sure nothing bad happens until we have proper support for Service Workers.

See:

@astorije astorije added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Nov 7, 2016
@xPaw
Copy link
Member

xPaw commented Nov 7, 2016

I would add the same thing in settings, so if notifications can't be created, the option is disabled. Also the sounds can't be played on mobile without user interaction which also throws.

Recent Chrome versions are dropping out `new Notification` in favor of `ServiceWorkerRegistration.showNotification`.
This makes sure nothing bad happens until we have proper support for Service Workers.

See:
- https://stackoverflow.com/questions/29774836/failed-to-construct-notification-illegal-constructor
- https://stackoverflow.com/questions/31512504/html5-notification-not-working-in-mobile-chrome
@astorije
Copy link
Member Author

astorije commented Nov 8, 2016

Also the sounds can't be played on mobile without user interaction which also throws.

Added a try/catch for this too.

I would add the same thing in settings, so if notifications can't be created, the option is disabled.

This can only be detected when calling Notification.new so it can't be done in the settings. I'd need to fiddle with the options, localStorage, checkboxes and others to make something nice. For this I would also need to move around things to make sure they are declared in order and in the proper scope.

I'm not comfortable doing so as I'm pretty sure I'll break something at first try, and I don't have much time to give at the moment so I won't be able to do it myself.
I'd say let's fix the bugs reported in #731 for now and UX of that state can be improved later on. What do you think?

@astorije astorije added this to the 2.2.0 milestone Nov 16, 2016
@astorije astorije merged commit 49c473c into master Nov 16, 2016
@astorije astorije deleted the astorije/fix-notifications branch November 16, 2016 02:20
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
…tions

Silence failures to trigger notifications when not available
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants