A user can click away from a popup notification ("meh") and that's great, but some web APIs need to get a "not now" response from that action, maybe after a timeout. IndexedDB, for example, needs a way to notify the web page that the user is not interested in offline storage so that the page can take appropriate steps. What if we add two other options, "autoDismiss" (a boolean) and "dismissAction" (a callback function). Those can be passed to the show() method just like the rest.

dismissalHandler is bug 590794, and my patch in bug 587587 adds a "removeOnDismissal" option.

(In reply to comment #1) > dismissalHandler is bug 590794, and my patch in bug 587587 adds a > "removeOnDismissal" option. Ok. So maybe this bug should be morphed to just handle the timeout part? Basically with those other bugs fixed all we'd need is some timer that says "remove the notification after so many seconds of 'meh' or inactivity".

It's currently possible for the PopupNotification caller to do that manually, FWIW. Just keep a reference to the notification object (returned from show()), and call remove() on it when you want it removed. Do you think it's necessary to have a built-in way to do this?

That's probably enough IMHO. Though if we end up using setTimeout to call remove() in a lot of places, we might want to add timeout as a built-in feature. But for now that sounds like an ok solution to me.

Given the progress in bug 587587 I'm going to morph this bug a bit. This bug will be tasked to making the IndexedDB notifications auto-dismiss when the user doesn't respond after a certain time.

The PopupNotifications patch was suggested by gavin and works great! The timeout values have been approved by sicking.

Shouldn't the firstTimeoutDuration timeout be set from the showCallback? With that patch, it looks like for the common case (notification for currently active tab) the firstTimeoutDuration timeout will be immediately canceled.

(In reply to comment #7) > Shouldn't the firstTimeoutDuration timeout be set from the showCallback? With > that patch, it looks like for the common case (notification for currently > active tab) the firstTimeoutDuration timeout will be immediately canceled. No, setting the timer after calling show() prevents that. The callback comes from inside show(), so the first timer persists until you dismiss the first time.

Changes in bug 597244 require a new patch.

So with this patch, if a background tab notifies and the user doesn't switch to it before the 30 seconds elapse, they will have had no chance to react at all, right? Seems like we shouldn't trigger the timeout until the first show (can set a property on the notification object to distinguish first vs. subsequent shown events, I guess). (It also seems like you could just assign the result of show() to a local variable and close over it rather than calling getNotification in the callback - remove() on an already-removed notification doesn't throw, so no need to worry about that.)

(In reply to comment #10) > So with this patch, if a background tab notifies and the user doesn't switch to > it before the 30 seconds elapse, they will have had no chance to react at all, > right? No, the first timeout is set for 2 minutes, and we want it to autodismiss if the user doesn't switch to it or notice it. Dismissing the popup clears that timeout and sets a 30 second timeout. Reactivating the popup disables the timeout entirely (popup will remain forever), unless it is again dismissed (which reactivates the timeout). > (It also seems like you could just assign the result of show() to a local > variable and close over it rather than calling getNotification in the callback > - remove() on an already-removed notification doesn't throw, so no need to > worry about that.) Ok, will update the patch in a sec.

Uses a closure variable for the notification. Also added some more comments to make it clear how all this works.

This seems like the kind of change faaborg probably ought to weigh in on. Notifications on a self-destruct timer seem like they could be bad UI. How important is this for IndexDB, from a technical pov? Seems like web authors may already be unhappy with the 2-minute timeout, so I wonder if this ends up being a case of there not even being a happy middle ground for a timeout short enough for pages to be happy, yet long enough to not be surprising UI.

The main problem here is the quota prompt. The problem with that is that it will happen at a basically random time from the web authors point of view. So I don't think we can expect that the author will be dealing properly with it and add timeouts with fallbacks etc. So we need to figure out a way to avoid letting the page "hang" indefinitely until the user stumbles upon a background tab. Especially since background tabs can be blocking a tab that the user *is* interacting with. IMHO an ideal solution would be to pop up the notification on all pages that is currently using a given database. However that requires also dismissing all the popups if the user makes a choice in one of them. This is currently not possible with the current popup APIs.

>This seems like the kind of change faaborg probably ought to weigh in on. >Notifications on a self-destruct timer seem like they could be bad UI. Without really getting the full context, my generic advice is that timers are bad in that they leave users feeling like they aren't in control (especially if you were about to hit something and then it disappears right out from under you). So anything we can do to avoid a timer would be useful.

(In reply to comment #14) > The main problem here is the quota prompt. How about showing the UI when the DB hits, say, 80% of the quota. Then the user has an arbitrary period of time to deal with the dialog, and as far as the web page is concerned exceeding the quota is just an instant hard-failure.

I was thinking about this too. Popping up a dialog at 80% would be a good idea. But I suspect that we don't want to just make it a hard failure if 100% is reached before the user confirms that more space can be used. Instead we'd want to block the page until the user confirms or dismisses the popup. So this wouldn't really remove the problem, but it would probably significantly help since we wouldn't be in as big of a hurry to "time out" the dialog.

Gavin, review ping :)

I landed this by accident here: http://hg.mozilla.org/mozilla-central/rev/b1d573743e35 And then removed the unreviewed JS changes here: http://hg.mozilla.org/mozilla-central/rev/23a389589086 Sorry about that.

Comment on attachment 477205 [details] [diff] [review] Patch, v2.1 >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+ var timeoutId; >+ >+ function cleanup() { >+ if (timeoutId) { >+ clearTimeout(timeoutId); >+ delete timeoutId; This "delete" doesn't actually have any effect - you can't delete a local var... >+ var options = { >+ eventCallback: function(state) { >+ // Don't do anything if the timeout has not been set or if a selection >+ // has already been made (either by user action or by timing out). >+ if (!timeoutId) { >+ return; >+ } ... so this condition should never be hit. I imagine that's not an issue because this handler is never called with state={dismissed|shown} in either of the cases described (user action or timeout has occurred). So I think you can address this by just removing cleanup() in favor of direct calls to clearTimeout(), and remove that check. Otherwise I think this looks fine. It would be really good to have some tests for this (timeouts make it a bit tricky, but I think it's still doable). And I suppose the general idea of timeouts should be run by faaborg if it hasn't been already (I forget whether this specifically was discussed over email). I apologize for the unreasonable review delay here - I know it sucks to have to page back in all of this code's context and such.

Regarding the autodismissing behavior. I certainly agree that it's not ideal. What I'd prefer is that each window which has databases in that origin open show a door-hanger. However do we really have anyone which has time to implement that for this release?

(In reply to comment #20) > ... so this condition should never be hit. I imagine that's not an issue > because this handler is never called with state={dismissed|shown} in either of > the cases described (user action or timeout has occurred). I think maybe my comment confused you. Here's the issue. Reading the code, PopupNotifications.show() calls _update(), which calls _showPanel(), which then calls _fireCallback with "shown". If I don't check that timeoutId is non-null then I will clear the timeout unintentionally. I guess I thought that we would eventually get a "dismissed" callback when the user clicked a button on the panel but it looks like we only get "removed" so I think the comment is just wrong there. So, given that, I think I actually need to leave the cleanup() function and simply s/delete timeoutId/timeoutId = undefined/. And I'll fix the comment. Does all that make sense?

clearTimeout never throws, so you can generally just clearTimeout(foo) without worrying about whether foo is null or a valid timeout ID, which should allow you to avoid the need for cleanup().

(In reply to comment #23) > clearTimeout never throws Ok. Is that the only change you'd like to see?

This includes changes from bug 596776 (sorry, I botched the backout and accidentally merged the two patches). But this removes the cleanup() function and fixes the comments.

(In reply to comment #24) > Ok. Is that the only change you'd like to see? I think we still need to sort out how the doorhanger should work. Maybe we can just grab faaborg sometime this week and nail it down?

>I think we still need to sort out how the doorhanger should work. I'm around and happy to answer any questions. General guidance is to avoid timers, and don't allow the user to ever say a definitive no to any question. I realize most specs don't want a UI that is that flexible (geolocation, etc), and would rather we treat the user like a function that is called through the use of a modal dialog box.

Just talked to dolske and jonas, turns out we are considering a long term timer (5 min, maybe 10 min) which is totally fine. I was just worried about a short term timer (15 seconds).

It's currently set for 2 minutes, but we could of course tweak that.

Comment on attachment 497079 [details] [diff] [review] Patch, v2.2 r=me code wise, but I think we still need to get consensus that this is the right behavior (ui-review?).

Comment on attachment 497079 [details] [diff] [review] Patch, v2.2 Alex, can you weigh in here officially?

Comment on attachment 497079 [details] [diff] [review] Patch, v2.2 let's set the timeout to 5 minutes.

Ok, here's 5 minutes. Thanks!