Bug 1315236 - Handle popup notifications with no actions. r=paolo draft
authorJohann Hofmann <jhofmann@mozilla.com>
Wed, 23 Nov 2016 12:16:50 +0100
changeset 443448 b122d6a11d77d4c8e4d99eb01d7bc72295fe611e
parent 443220 34fce7c12173bdd6dda54c2ebf6d344252f1ac48
child 538054 62b17d82c8e8328a21d60f06dde8ee9b04da81f7
push id36996
push userbmo:jhofmann@mozilla.com
push dateThu, 24 Nov 2016 13:55:52 +0000
reviewerspaolo
bugs1315236
milestone53.0a1
Bug 1315236 - Handle popup notifications with no actions. r=paolo MozReview-Commit-ID: F3rkoEovBYc
browser/base/content/browser-addons.js
browser/base/content/test/general/browser_bug553455.js
browser/base/content/test/popupNotifications/browser_popupNotification.js
toolkit/content/widgets/notification.xml
toolkit/locales/en-US/chrome/global/notification.dtd
toolkit/modules/PopupNotifications.jsm
toolkit/themes/shared/popupnotification.inc.css
--- a/browser/base/content/browser-addons.js
+++ b/browser/base/content/browser-addons.js
@@ -231,16 +231,17 @@ const gXPInstallObserver = {
     var messageString, action;
     var brandShortName = brandBundle.getString("brandShortName");
 
     var notificationID = aTopic;
     // Make notifications persistent
     var options = {
       displayURI: installInfo.originatingURI,
       persistent: true,
+      hideClose: true,
       timeout: Date.now() + 30000,
     };
 
     switch (aTopic) {
     case "addon-install-disabled": {
       notificationID = "xpinstall-disabled";
 
       if (gPrefService.prefIsLocked("xpinstall.enabled")) {
@@ -444,16 +445,17 @@ const gXPInstallObserver = {
       messageString = messageString.replace("#1", installInfo.installs[0].name);
       messageString = messageString.replace("#2", installInfo.installs.length);
       messageString = messageString.replace("#3", brandShortName);
 
       // Remove notificaion on dismissal, since it's possible to cancel the
       // install through the addons manager UI, making the "restart" prompt
       // irrelevant.
       options.removeOnDismissal = true;
+      options.persistent = false;
 
       PopupNotifications.show(browser, notificationID, messageString, anchorID,
                               action, null, options);
       break; }
     }
   },
   _removeProgressNotification(aBrowser) {
     let notification = PopupNotifications.getNotification("addon-progress", aBrowser);
--- a/browser/base/content/test/general/browser_bug553455.js
+++ b/browser/base/content/test/general/browser_bug553455.js
@@ -820,17 +820,17 @@ function test_urlBar() {
     gBrowser.selectedTab = gBrowser.addTab("about:blank");
     yield BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
     gURLBar.value = TESTROOT + "amosigned.xpi";
     gURLBar.focus();
     EventUtils.synthesizeKey("VK_RETURN", {});
     let panel = yield notificationPromise;
 
     let notification = panel.childNodes[0];
-    is(notification.button.label, "", "Button to allow install should be hidden.");
+    ok(!notification.hasAttribute("buttonlabel"), "Button to allow install should be hidden.");
     yield removeTab();
   });
 },
 
 function test_wrongHost() {
   return Task.spawn(function* () {
     let requestedUrl = TESTROOT2 + "enabled.html";
     gBrowser.selectedTab = gBrowser.addTab();
--- a/browser/base/content/test/popupNotifications/browser_popupNotification.js
+++ b/browser/base/content/test/popupNotifications/browser_popupNotification.js
@@ -198,33 +198,56 @@ var tests = [
       ok(!this.testNotif1.secondaryActionClicked, "secondary action #1 wasn't clicked");
       ok(!this.testNotif1.dismissalCallbackTriggered, "dismissal callback #1 wasn't called");
 
       ok(!this.testNotif2.mainActionClicked, "main action #2 wasn't clicked");
       ok(this.testNotif2.secondaryActionClicked, "secondary action #2 was clicked");
       ok(!this.testNotif2.dismissalCallbackTriggered, "dismissal callback #2 wasn't called");
     }
   },
-  // Test notification without mainAction
+  // Test notification without mainAction or secondaryActions, it should fall back
+  // to a default button that dismisses the notification in place of the main action.
   { id: "Test#9",
     run: function() {
       this.notifyObj = new BasicNotification(this.id);
       this.notifyObj.mainAction = null;
+      this.notifyObj.secondaryActions = null;
+      this.notification = showNotification(this.notifyObj);
+    },
+    onShown: function(popup) {
+      triggerMainCommand(popup);
+    },
+    onHidden: function(popup) {
+      ok(!this.notifyObj.mainActionClicked, "mainAction was not clicked");
+      ok(!this.notifyObj.dismissalCallbackTriggered, "dismissal callback wasn't triggered");
+      ok(this.notifyObj.removedCallbackTriggered, "removed callback triggered");
+    }
+  },
+  // Test notification without mainAction but with secondaryActions, it should fall back
+  // to a default button that dismisses the notification in place of the main action
+  // and ignore the passed secondaryActions.
+  { id: "Test#10",
+    run: function() {
+      this.notifyObj = new BasicNotification(this.id);
+      this.notifyObj.mainAction = null;
       this.notification = showNotification(this.notifyObj);
     },
     onShown: function(popup) {
-      checkPopup(popup, this.notifyObj);
-      dismissNotification(popup);
+      let notification = popup.childNodes[0];
+      is(notification.getAttribute("secondarybuttonhidden"), "true", "secondary button is hidden");
+      triggerMainCommand(popup);
     },
     onHidden: function(popup) {
-      this.notification.remove();
+      ok(!this.notifyObj.mainActionClicked, "mainAction was not clicked");
+      ok(!this.notifyObj.dismissalCallbackTriggered, "dismissal callback wasn't triggered");
+      ok(this.notifyObj.removedCallbackTriggered, "removed callback triggered");
     }
   },
   // Test two notifications with different anchors
-  { id: "Test#10",
+  { id: "Test#11",
     run: function() {
       this.notifyObj = new BasicNotification(this.id);
       this.firstNotification = showNotification(this.notifyObj);
       this.notifyObj2 = new BasicNotification(this.id);
       this.notifyObj2.id += "-2";
       this.notifyObj2.anchorID = "addons-notification-icon";
       // Second showNotification() overrides the first
       this.secondNotification = showNotification(this.notifyObj2);
--- a/toolkit/content/widgets/notification.xml
+++ b/toolkit/content/widgets/notification.xml
@@ -526,16 +526,18 @@
                          position="after_end"
                          xbl:inherits="oncommand=menucommand">
             <children/>
           </xul:menupopup>
         </xul:button>
         <xul:button anonid="button"
                     class="popup-notification-button"
                     default="true"
+                    label="&defaultButton.label;"
+                    accesskey="&defaultButton.accesskey;"
                     xbl:inherits="oncommand=buttoncommand,label=buttonlabel,accesskey=buttonaccesskey,disabled=mainactiondisabled"/>
       </xul:hbox>
     </content>
     <resources>
       <stylesheet src="chrome://global/skin/notification.css"/>
     </resources>
     <implementation>
       <field name="checkbox" readonly="true">
--- a/toolkit/locales/en-US/chrome/global/notification.dtd
+++ b/toolkit/locales/en-US/chrome/global/notification.dtd
@@ -2,8 +2,11 @@
    - License, v. 2.0. If a copy of the MPL was not distributed with this
    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
 
 <!ENTITY closeNotification.tooltip "Close this message">
 
 <!ENTITY checkForUpdates "Check for updates…">
 
 <!ENTITY learnMore "Learn more…">
+
+<!ENTITY defaultButton.label "OK!">
+<!ENTITY defaultButton.accesskey "O">
--- a/toolkit/modules/PopupNotifications.jsm
+++ b/toolkit/modules/PopupNotifications.jsm
@@ -286,18 +286,18 @@ PopupNotifications.prototype = {
    *        action. If present, it must have the following properties:
    *          - label (string): the button's label.
    *          - accessKey (string): the button's accessKey.
    *          - callback (function): a callback to be invoked when the button is
    *            pressed, is passed an object that contains the following fields:
    *              - checkboxChecked: (boolean) If the optional checkbox is checked.
    *          - [optional] dismiss (boolean): If this is true, the notification
    *            will be dismissed instead of removed after running the callback.
-   *        If null, the notification will not have a button, and
-   *        secondaryActions will be ignored.
+   *        If null, the notification will have a default "OK" action button
+   *        that can be used to dismiss the popup and secondaryActions will be ignored.
    * @param secondaryActions
    *        An optional JavaScript array describing the notification's alternate
    *        actions. The array should contain objects with the same properties
    *        as mainAction. These are used to populate the notification button's
    *        dropdown menu.
    * @param options
    *        An options JavaScript object holding additional properties for the
    *        notification. The following properties are currently supported:
@@ -677,19 +677,20 @@ PopupNotifications.prototype = {
       if (n.mainAction) {
         popupnotification.setAttribute("buttonlabel", n.mainAction.label);
         popupnotification.setAttribute("buttonaccesskey", n.mainAction.accessKey);
         popupnotification.setAttribute("buttoncommand", "PopupNotifications._onButtonEvent(event, 'buttoncommand');");
         popupnotification.setAttribute("dropmarkerpopupshown", "PopupNotifications._onButtonEvent(event, 'dropmarkerpopupshown');");
         popupnotification.setAttribute("learnmoreclick", "PopupNotifications._onButtonEvent(event, 'learnmoreclick');");
         popupnotification.setAttribute("menucommand", "PopupNotifications._onMenuCommand(event);");
       } else {
+        // Enable the default button to let the user close the popup if the close button is hidden
+        popupnotification.setAttribute("buttoncommand", "PopupNotifications._onButtonEvent(event, 'buttoncommand');");
         popupnotification.removeAttribute("buttonlabel");
         popupnotification.removeAttribute("buttonaccesskey");
-        popupnotification.removeAttribute("buttoncommand");
         popupnotification.removeAttribute("dropmarkerpopupshown");
         popupnotification.removeAttribute("learnmoreclick");
         popupnotification.removeAttribute("menucommand");
       }
 
       if (n.options.popupIconClass) {
         let classes = "popup-notification-icon " + n.options.popupIconClass;
         popupnotification.setAttribute("iconclass", classes);
@@ -718,17 +719,17 @@ PopupNotifications.prototype = {
       } else
         popupnotification.removeAttribute("origin");
 
       if (n.options.hideClose)
         popupnotification.setAttribute("closebuttonhidden", "true");
 
       popupnotification.notification = n;
 
-      if (n.secondaryActions && n.secondaryActions.length > 0) {
+      if (n.mainAction && n.secondaryActions && n.secondaryActions.length > 0) {
         let telemetryStatId = TELEMETRY_STAT_ACTION_2;
 
         let secondaryAction = n.secondaryActions[0];
         popupnotification.setAttribute("secondarybuttonlabel", secondaryAction.label);
         popupnotification.setAttribute("secondarybuttonaccesskey", secondaryAction.accessKey);
         popupnotification.setAttribute("secondarybuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand');");
 
         for (let i = 1; i < n.secondaryActions.length; i++) {
@@ -1313,27 +1314,29 @@ PopupNotifications.prototype = {
 
     if (type == "secondarybuttoncommand") {
       action = notification.secondaryActions[0];
       telemetryStatId = TELEMETRY_STAT_ACTION_2;
     }
 
     notification._recordTelemetryStat(telemetryStatId);
 
-    try {
-      action.callback.call(undefined, {
-        checkboxChecked: notificationEl.checkbox.checked
-      });
-    } catch (error) {
-      Cu.reportError(error);
-    }
+    if (action) {
+      try {
+        action.callback.call(undefined, {
+          checkboxChecked: notificationEl.checkbox.checked
+        });
+      } catch (error) {
+        Cu.reportError(error);
+      }
 
-    if (action.dismiss) {
-      this._dismiss();
-      return;
+      if (action.dismiss) {
+        this._dismiss();
+        return;
+      }
     }
 
     this._remove(notification);
     this._update();
   },
 
   _onMenuCommand: function PopupNotifications_onMenuCommand(event) {
     let target = event.originalTarget;
--- a/toolkit/themes/shared/popupnotification.inc.css
+++ b/toolkit/themes/shared/popupnotification.inc.css
@@ -39,17 +39,16 @@
 .popup-notification-learnmore-link {
   margin-top: .5em !important;
 }
 
 .popup-notification-button-container {
   background-color: var(--arrowpanel-dimmed);
   border-top: 1px solid var(--panel-separator-color);
   display: flex;
-  justify-content: flex-end;
 }
 
 .popup-notification-button-container > toolbarseparator {
   -moz-appearance: none;
   border: 0;
   border-left: 1px solid var(--panel-separator-color);
   margin: 7px 0 7px;
   min-width: 0;
@@ -99,16 +98,20 @@
 .popup-notification-button[default]:hover:not([disabled]) {
   background-color: #0675d3;
 }
 
 .popup-notification-button[default]:hover:active:not([disabled]) {
   background-color: #0568ba;
 }
 
+.popup-notification-button[anonid="secondarybutton"][hidden="true"] ~ .popup-notification-button[default] {
+  flex: 1;
+}
+
 .popup-notification-button > .button-box {
   padding: 0;
   margin: 0;
   /* prevent double border on windows when focused */
   border: none;
 }
 
 .popup-notification-dropmarker {