When an install is pending restart, a doorhanger notification is shown prompting to restart. However, if that install is cancelled (see bug 562300), the doorhanger still shows - it should disappear.

Requesting blocking on final - its pretty noticeable with the fix for bug 562300.

How is this possible? Clicking outside the doorhanger is meant to dismiss it so surely clicking to cancel the install would do the same?

Oh, I was using keyboard shortcuts to switch tabs back to the addons manager - which hides the doorhanger (but doesn't dismiss it). But when switching back to the original tab, the doorhanger is shown again. However, if I switch tabs using the mouse, the doorhanger is dismissed. Is this a bug with the doorhanger code? I don't know what behaviour is expected there.

Yeah my understanding of what doorhangers are meant to be would suggest that they should dismiss with keyboard use, gavin?

I guess they probably should. I left it this way intentionally because it seemed kind of useful :) Now that dismissal isn't permanent it's less of an issue I guess. I guess that would just involve removing _ignoreDismissal (and _hidePanel).

Ok to take this gavin? If not pass it along

Actually, that doesn't really improve the situation much. It "dismisses" the notification, but it still persists and can be re-shown from the icon. It seems to me like the addon notifications should perhaps not persist at all - we could add a flag to show's "options" param for that, I suppose.

That said, Vlad may be in this range too

(In reply to comment #8) > That said, Vlad may be in this range too This isn't that other bug, I realise that now. (In reply to comment #7) > Actually, that doesn't really improve the situation much. It "dismisses" the > notification, but it still persists and can be re-shown from the icon. It seems > to me like the addon notifications should perhaps not persist at all - we could > add a flag to show's "options" param for that, I suppose. Yeah I guess there isn't a lot of value in these ones persisting. At least not worth some of the strange UI cases I can think of.

Also adds a missing comma in browser.js

No action for a couple of months. Gavin, are you still working on this, or can someone else take it over?

Gavin, does this just need a test? I can do that if you're too busy.

Yeah, I think it just needs a test (and maybe some minor unbitrotting).

I updated the patch to address the problems that came up with getting rid of _hidePanel(). I also added a test.

Comment on attachment 509500 [details] [diff] [review] patch v2 >Bug 587587: always dismiss notifications when they are hidden Checkin comment should probably mention the three changes: - always dismiss notifications when they are hidden (e.g. due to tab switch, scroll, etc.) - add support for "removeOnDismissal" option to show() that causes dismissed notifications to be removed - make use of removeOnDismissal for addons manager notifications Perhaps the third is best split off to a separate bug/changeset. >diff --git a/browser/base/content/test/browser_popupNotification.js b/browser/base/content/test/browser_popupNotification.js > onHidden: function (popup) { > // actually remove the notification to prevent it from reappearing >- ok(!wrongBrowserNotificationObject.dismissalCallbackTriggered, "dismissal callback wasn't called"); I guess you could just reverse this test (change comment to "dismissal callback was called due to tab switch"). We really need to audit this test to make sure we have coverage of all the possible combinations of event handler callbacks, maybe file a followup bug? >diff --git a/toolkit/content/PopupNotifications.jsm b/toolkit/content/PopupNotifications.jsm >+ * removeOnDismissal: >+ * Notifications with this parameter set to true will be >+ * removed when they are dismissed for any reason (i.e. >+ * any time the popup is closed due to user interaction). This comment is slightly misleading... Perhaps this should just say "removed when they would have otherwise been dismissed", and then elsewhere list what causes a notification to be "dismissed" exactly. Something like: A notification is "dismissed" anytime it is hidden but *not* removed, for example: - due to a tab/window switch - due to a user clicking elsewhere on the screen Note: - if the hide is triggered by a command (mainAction or secondaryAction), no dismissal occurs (only removal) - if the notification is being overridden by another, no dismissal occurs (the notification will be re-displayed once the overriding one is dismissed/removed) - if the notification is replaced by another notification with the same ID, no dismissal occurs (only removal) I think we need a test for those last two cases. Need to update https://developer.mozilla.org/en/JavaScript_code_modules/PopupNotifications.jsm#Notification_options too. > _onPopupHidden: function PopupNotifications_onPopupHidden(event) { >+ if (notificationObj.options.removeOnDismissal) >+ this._remove(notificationObj); >+ else >+ notificationObj.dismissed = true; > this._fireCallback(notificationObj, "dismissed"); Hmm, this ordering seems wrong - "dismissed" should probably be called before "removed". Though actually now I'm wondering whether we should fire "dismissed" at all for the removal case... We don't fire it in the other cases where removal occurs, and I'm not sure that this case in particular is useful to distinguish from the other removal cases. I think we might be able to avoid adding the parameter to _update by just ensuring that we never call a dismissal handler on a notification that's been removed, since I think all of the cases where we want to ignore the dismissal handlers are cases where we're updating after a removal. We can't use currentNotifications here because this might be called after a tab switch (so _currentNotifications would point to the old set of notifications), so you'd need to do something like: let browser = this.panel.firstChild && this.panel.firstChild.notification.browser; if (!browser) return; let notifications = this._getNotificationsForBrowser(browser); Array.forEach(this.panel.childNodes, function (nEl) { let notificationObj = nEl.notification; if (notifications.indexOf(notificationObj) == -1) return; // dismiss, fire callback, etc. });

I added some more dismissal/removed checks to browser_popupNotification.js to cover the points in your review comments, but I filed bug 632510 for a more thorough inspection of our test coverage. I'll update MDC once we're sure this is what we want and this patch lands.

Comment on attachment 511142 [details] [diff] [review] patch v3 >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > var notificationID = aTopic; >- // Make notifications persist a minimum of 30 seconds >+ // Make notifications persist a minimum of 30 seconds, but remove themselves >+ // on dismissal > var options = { >+ removeOnDismissal: true, > timeout: Date.now() + 30000 > }; I'm not sure we want removeOnDismissal for all of these (e.g. the progress notification). I'd also like to land this change in a separate changeset, so maybe worth pushing to another bug? >diff --git a/toolkit/content/PopupNotifications.jsm b/toolkit/content/PopupNotifications.jsm > _onPopupHidden: function PopupNotifications_onPopupHidden(event) { >+ let browser = this.panel.firstChild && >+ this.panel.firstChild.notification.browser; >+ if (!browser) >+ return; >+ >+ let notifications = this._getNotificationsForBrowser(browser); Thinking about this a bit further, it's kind of fragile to rely on getting the browser from the children like this (someone adding non-notification children would bust us). That's already true of the existing code in this function, though, so perhaps I should just file a followup about this. The added test coverage is excellent! r=me.

Can this land now?

No, I need to address Gavin's first comment to push that change to a new bug. I'm planning on landing this tomorrow morning.

I'm changing the bug summary to reflect what we actually did here. I filed bug 633218 as a follow-up about the addons notifications themselves. http://hg.mozilla.org/mozilla-central/rev/18b846f059d1