Bug 854740: PopupNotifications doesn't handle showing a dismissed notification after showing a normal one, r=MattN
authorGavin Sharp <gavin@gavinsharp.com>
Thu, 28 Mar 2013 17:27:57 -0400
changeset 127470 4a5b662231b8e0ade07a81f5ccce89894d73973d
parent 127469 538ec52a91565157e869131f8850e612543bc1e9
child 127471 248bfb2dcc994c2bf3b6df4f340f491acb89c1c4
push id24506
push userryanvm@gmail.com
push dateWed, 03 Apr 2013 22:14:39 +0000
treeherdermozilla-central@475dc5f51bdb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs854740
milestone23.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 854740: PopupNotifications doesn't handle showing a dismissed notification after showing a normal one, r=MattN
browser/base/content/test/browser_popupNotification.js
toolkit/content/PopupNotifications.jsm
--- a/browser/base/content/test/browser_popupNotification.js
+++ b/browser/base/content/test/browser_popupNotification.js
@@ -695,17 +695,17 @@ var tests = [
       executeSoon(function delayedDismissal() {
         dismissNotification(popup);
       });
 
     },
     onHidden: function (popup) {
       ok(!this.notifyObj.mainActionClicked, "mainAction was not clicked because it was too soon");
       ok(this.notifyObj.dismissalCallbackTriggered, "dismissal callback was triggered");
-    },
+    }
   },
   { // Test #24  - test security delay - after delay
     run: function () {
       // Set the security delay to 10ms
       PopupNotifications.buttonDelay = 10;
 
       this.notifyObj = new basicNotification();
       showNotification(this.notifyObj);
@@ -718,17 +718,17 @@ var tests = [
         triggerMainCommand(popup);
       }, 500);
 
     },
     onHidden: function (popup) {
       ok(this.notifyObj.mainActionClicked, "mainAction was clicked after the delay");
       ok(!this.notifyObj.dismissalCallbackTriggered, "dismissal callback was not triggered");
       PopupNotifications.buttonDelay = PREF_SECURITY_DELAY_INITIAL;
-    },
+    }
   },
   { // Test #25 - reload removes notification
     run: function () {
       loadURI("http://example.com/", function() {
         let notifyObj = new basicNotification();
         notifyObj.options.eventCallback = function (eventName) {
           if (eventName == "removed") {
             ok(true, "Notification removed in background tab after reloading");
@@ -822,16 +822,47 @@ var tests = [
         };
         showNotification(notifyObj);
         executeSoon(function () {
           content.document.getElementById("iframe")
                           .setAttribute("src", "http://example.org/");
         });
       });
     }
+  },
+  { // Test #29 -  Existing popup notification shouldn't disappear when adding a dismissed notification
+    run: function () {
+      this.notifyObj1 = new basicNotification();
+      this.notifyObj1.id += "_1";
+      this.notifyObj1.anchorID = "default-notification-icon";
+      this.notification1 = showNotification(this.notifyObj1);
+    },
+    onShown: function (popup) {
+      // Now show a dismissed notification, and check that it doesn't clobber
+      // the showing one.
+      this.notifyObj2 = new basicNotification();
+      this.notifyObj2.id += "_2";
+      this.notifyObj2.anchorID = "geo-notification-icon";
+      this.notifyObj2.options.dismissed = true;
+      this.notification2 = showNotification(this.notifyObj2);
+
+      checkPopup(popup, this.notifyObj1);
+
+      // check that both anchor icons are showing
+      is(document.getElementById("default-notification-icon").getAttribute("showing"), "true",
+         "notification1 anchor should be visible");
+      is(document.getElementById("geo-notification-icon").getAttribute("showing"), "true",
+         "notification2 anchor should be visible");
+
+      dismissNotification(popup);
+    },
+    onHidden: function(popup) {
+      this.notification1.remove();
+      this.notification2.remove();
+    }
   }
 ];
 
 function showNotification(notifyObj) {
   return PopupNotifications.show(notifyObj.browser,
                                  notifyObj.id,
                                  notifyObj.message,
                                  notifyObj.anchorID,
@@ -841,18 +872,20 @@ function showNotification(notifyObj) {
 }
 
 function checkPopup(popup, notificationObj) {
   info("[Test #" + gTestIndex + "] checking popup");
 
   ok(notificationObj.shownCallbackTriggered, "shown callback was triggered");
 
   let notifications = popup.childNodes;
-  is(notifications.length, 1, "only one notification displayed");
+  is(notifications.length, 1, "one notification displayed");
   let notification = notifications[0];
+  if (!notification)
+    return;
   let icon = document.getAnonymousElementByAttribute(notification, "class", "popup-notification-icon");
   if (notificationObj.id == "geolocation") {
     isnot(icon.boxObject.width, 0, "icon for geo displayed");
     is(popup.anchorNode.className, "notification-anchor-icon", "notification anchored to icon");
   }
   is(notification.getAttribute("label"), notificationObj.message, "message matches");
   is(notification.id, notificationObj.id + "-notification", "id matches");
   if (notificationObj.mainAction) {
--- a/toolkit/content/PopupNotifications.jsm
+++ b/toolkit/content/PopupNotifications.jsm
@@ -250,17 +250,17 @@ PopupNotifications.prototype = {
       this._remove(existingNotification);
 
     let notifications = this._getNotificationsForBrowser(browser);
     notifications.push(notification);
 
     let fm = Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager);
     if (browser == this.tabbrowser.selectedBrowser && fm.activeWindow == this.window) {
       // show panel now
-      this._update(notification.anchorElement);
+      this._update(notification.anchorElement, true);
     } else {
       // Otherwise, update() will display the notification the next time the
       // relevant tab/window is selected.
 
       // Notify observers that we're not showing the popup (useful for testing)
       this._notify("backgroundShow");
     }
 
@@ -545,18 +545,24 @@ PopupNotifications.prototype = {
       n.timeShown = Date.now();
       this._fireCallback(n, NOTIFICATION_EVENT_SHOWN);
     }, this);
   },
 
   /**
    * Updates the notification state in response to window activation or tab
    * selection changes.
+   *
+   * @param anchor is a XUL element reprensenting the anchor whose notifications
+   *               should be shown.
+   * @param dismissShowing if true, dismiss any currently visible notifications
+   *                       if there are no notifications to show. Otherwise,
+   *                       currently displayed notifications will be left alone.
    */
-  _update: function PopupNotifications_update(anchor) {
+  _update: function PopupNotifications_update(anchor, dismissShowing = false) {
     if (this.iconBox) {
       // hide icons of the previous tab.
       this._hideIcons();
     }
 
     let anchorElement, notificationsToShow = [];
     let currentNotifications = this._currentNotifications;
     let haveNotifications = currentNotifications.length > 0;
@@ -579,19 +585,22 @@ PopupNotifications.prototype = {
     }
 
     if (notificationsToShow.length > 0) {
       this._showPanel(notificationsToShow, anchorElement);
     } else {
       // Notify observers that we're not showing the popup (useful for testing)
       this._notify("updateNotShowing");
 
-      // Dismiss the panel if needed. _onPopupHidden will ensure we never call
-      // a dismissal handler on a notification that's been removed.
-      this._dismiss();
+      // Close the panel if there are no notifications to show.
+      // When called from PopupNotifications.show() we should never close the
+      // panel, however. It may just be adding a dismissed notification, in
+      // which case we want to continue showing any existing notifications.
+      if (!dismissShowing)
+        this._dismiss();
 
       // Only hide the iconBox if we actually have no notifications (as opposed
       // to not having any showable notifications)
       if (this.iconBox && !haveNotifications)
         this.iconBox.hidden = true;
     }
   },