Fix false positive in Notifications test for Chrome on Android (fixes #1660) #1813

Merged
merged 1 commit into from Jan 2, 2016

Projects

None yet

3 participants

@alexgibson
Contributor

Please see #1660 for details. Note I don't actually have Chrome for Android to test on. I'm basing the change on this article: https://developers.google.com/web/updates/2015/05/Notifying-you-of-notificiation-changes

@ryanseddon ryanseddon commented on an outdated diff Dec 22, 2015
feature-detects/notification.js
@@ -18,5 +24,22 @@
Detects support for the Notifications API
*/
define(['Modernizr'], function(Modernizr) {
- Modernizr.addTest('notification', 'Notification' in window && 'permission' in window.Notification && 'requestPermission' in window.Notification);
+ Modernizr.addTest('notification', function() {
+ if (!window.Notification || !window.Notification.requestPermission) {
+ return false;
+ }
+ // if permission is already granted, assume support
+ if (window.Notification.permission === 'granted') {
+ return true;
+ }
+
+ try {
+ new window.Notification('');
+ } catch (e) {
+ if (e.name === 'TypeError') {
+ return false;
@ryanseddon
ryanseddon Dec 22, 2015 Member

two space indent

@ryanseddon ryanseddon commented on an outdated diff Dec 22, 2015
feature-detects/notification.js
+ if (!window.Notification || !window.Notification.requestPermission) {
+ return false;
+ }
+ // if permission is already granted, assume support
+ if (window.Notification.permission === 'granted') {
+ return true;
+ }
+
+ try {
+ new window.Notification('');
+ } catch (e) {
+ if (e.name === 'TypeError') {
+ return false;
+ }
+ }
+ return true;
@ryanseddon
ryanseddon Dec 22, 2015 Member

New line above

@ryanseddon
Member

Just tested on a nexus 6 with chrome 45 and it returns no support for notifications with your updated test. Is this correct?

Test case: http://output.jsbin.com/memunosofe/quiet

realdroid_google_nexus_6

@alexgibson
Contributor

Thanks @ryanseddon - yes this is correct, it should return false as only push notifications are now supported in Chrome on Android. Will update these nits shortly.

@alexgibson
Contributor

@ryanseddon updated thanks

@patrickkettner
Member

@alexgibson could you fix the style issue?

@alexgibson alexgibson Fix false positive in Notifications test for Chrome on Android (fixes #…
b1b95b2
@alexgibson
Contributor

@patrickkettner updated

@alexgibson
Contributor

Note: The AppVeyor failure for Node v0.10 looks unrelated to this change (looks like it couldn't install phantomjs for some reason).

@ryanseddon
Member

Yep appveyor seems to fail sometimes for other unrelated reasons.

@ryanseddon ryanseddon merged commit 4666d44 into Modernizr:master Jan 2, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexgibson alexgibson deleted the unknown repository branch Jan 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment