Add requiredInteraction support #47
Conversation
…ecessary to implement the requiredInteraction field This PR: 1) passes along the requiredInteraction field that Chrome respects 2) adds a field to close the notification on click when a requiredInteraction is used (bug in chrome?) 3) adds a hack to Firefox to continually call notifications when the requiredInteraction field is used until the user clicks it (ugly, but effective)
|
To be clear, I'm happy passing through this new property as it's in the spec, but we should acknowledge that other browsers have yet to implement it and degrade gracefully, without resorting to horrible hacks. |
|
Completely up to you, but this hack was to fulfill a business requirement. Our application is for mechanics working on vehicles. Often they are away from the browser that supplies them notifications about parts and new work and such. Having the notification go away is unacceptable. If Firefox implemented the spec correctly instead of trying to decide for us what's correct, none of this would be necessary. It's possible that they will in Firefox 44 but I haven't seen any confirmation that it's coming. All this is attempting to do is provide functionality as close to the intent of the requiredInteraction flag as possible given firefox's limitations. It will never reopen them (until clicked) unless the requiredInteraction flag is on. If you wanted to toss another optional flag in there to turn the hack on I could see that. If you wanted to yank it, I could see that as well. I just thought that since I ran into the situation where it was necessary that someone else might as well. We had customers complain, and they complain less this way :) |
|
Whoops, I accidentally deleted my first comment, sorry :/ For the record - before requiredInteraction was added to the spec, Firefox used a default timeout of 4 seconds, and Chrome 20 seconds. This was a gray area of the spec. I agree the default timeout in Firefox is not enough and should be changed. I think it's a bit unfair to blame Firefox seeing as this new requiredInteraction property is only recent. Let's give them a chance to implement it before accusing Mozilla of not following the spec, or forcing those users to suffer poor UX like this. I don't think many sites will have a hard-dependency on persistent notifications seeing as they are so recent and cross browser support it still in the works. |
|
I may actually show this hack as an example for why Firefox should reconsider their default timeout and implement this new property… seeing people write code like this in the wild when only one browser supports a new property shows we really need better parity here. |
|
Believe me. I 100% agree that it would be nice if all browsers implemented this parameter correctly :) Before Chrome 47s removal of the notification center, our users always had a way to get at those notifications. Once some users upgraded and the (mostly unused per google) notification center went away our phones lit up. I had actually written the original hack for FF early this year to try to be more consistent w/ the way that Chrome worked. So it's not like FF is moving quickly one way or the other. I was happy to see that there was actually still a way to get chrome to keep notifications around. My boss probably would have had me make the same hack for Chrome that I did for FF if they hadn't... I get that we're an edge case. But it's a legitimate one. Ok, short of re-writing all of our notifications to use some custom javascript solution where the notification is in the browser or something. Using the built in notification saves a ton of time though, even if I'm now asking something of Firefox that they'd prefer I didn't do. If nothing else, I'm glad this has brought up a good conversation about it. I get that you might prefer to not have it in the standard code base. I can remove it from the pr if you'd like. I could leave it in there commented out as well in case someone else did want it. Up to you. Appreciate your lib. I'll modify my PR as you see fit. |
|
Thanks for your thoughts and conversation :) Let's get rid of the browser hacks and just add support for the new property. I'd also like to understand why you need to force close a notification when clicked in Chrome if this new property is in use. I can't see anything in the spec about this. |
|
the closing just plain didn't work. You can try that out yourself. I don't know if that's a bug on chromes part or if they expected an explicit close call when it was interacted with. |
|
I suppose I should say that the close didn't work when the requiredInteraction was true. I updated the comment to reflect that, removed the FF hack and have repushed |
| @@ -127,12 +128,14 @@ | |||
| }; | |||
|
|
|||
|
|
|||
| Notify.prototype.show = function() { | |||
| Notify.prototype.show = function () { | |||
There was a problem hiding this comment.
Please remove the unnecessary white space changes here.
| timeout: null | ||
| timeout: null, | ||
| requireInteraction: false, | ||
| closeOnClick: false // used with the requireInteraction to close the notification after it's been clicked |
There was a problem hiding this comment.
Testing shows that Chrome does not close notifications on click by default, irrespective of requireInteraction. I'm fine to leave closeOnClick as an option, but the comment here is not strictly accurate. I think we can remove it.
|
It would be great to add a JS test to ensure that |
|
It would also be good to add a test to ensure that the notification is closed when |
added a check to make sure the timeout doesn't get set if requireInteraction is on updated the readme to explain the existence of closeOnClick
|
Thanks for the updated tests. I squashed your commits and merged in 7fcae8a. Thanks again! |
Our clients make heavy use of notifications and Chrome 47s removal of their notification center broke some things. This was my attempt to fix it. I thought I'd share if you'd like to use it.
This PR: