Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never". r=johannh draft
authorNihanth Subramanya <nhnt11@gmail.com>
Fri, 24 Feb 2017 18:42:36 -0800
changeset 501207 cd0188b641c858a8a631b1112be593dc4085aa8c
parent 500857 23a4b7430dd7e83a2809bf3dc41471f154301eda
child 549800 275505965d28eba5c5d320aab15911a5a57ca629
push id49896
push usernhnt11@gmail.com
push dateSat, 18 Mar 2017 20:39:45 +0000
reviewersjohannh
bugs1341742
milestone55.0a1
Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never". r=johannh MozReview-Commit-ID: DTkUuWabNjH
browser/locales/en-US/chrome/browser/browser.properties
browser/modules/PermissionUI.jsm
browser/modules/test/browser/browser_PermissionUI_prompts.js
browser/modules/test/browser/head.js
dom/notification/test/browser/browser_permission_dismiss.js
toolkit/content/widgets/notification.xml
--- a/browser/locales/en-US/chrome/browser/browser.properties
+++ b/browser/locales/en-US/chrome/browser/browser.properties
@@ -486,22 +486,23 @@ puAlertText=Click here for details
 geolocation.allowLocation=Allow Location Access
 geolocation.allowLocation.accesskey=A
 geolocation.dontAllowLocation=Don’t Allow
 geolocation.dontAllowLocation.accesskey=n
 geolocation.shareWithSite3=Will you allow %S to access your location?
 geolocation.shareWithFile3=Will you allow this local file to access your location?
 geolocation.remember=Remember this decision
 
-webNotifications.remember=Remember this decision
-webNotifications.rememberForSession=Remember decision for this session
 webNotifications.allow=Allow Notifications
 webNotifications.allow.accesskey=A
-webNotifications.dontAllow=Don’t Allow
-webNotifications.dontAllow.accesskey=n
+webNotifications.notNow=Not Now
+webNotifications.notNow.accesskey=n
+webNotifications.never=Never Allow
+webNotifications.neverForSession=Never For This Session
+webNotifications.never.accesskey=v
 webNotifications.receiveFromSite2=Will you allow %S to send notifications?
 # LOCALIZATION NOTE (webNotifications.upgradeTitle): When using native notifications on OS X, the title may be truncated around 32 characters.
 webNotifications.upgradeTitle=Upgraded notifications
 # LOCALIZATION NOTE (webNotifications.upgradeBody): When using native notifications on OS X, the body may be truncated around 100 characters in some views.
 webNotifications.upgradeBody=You can now receive notifications from sites that are not currently loaded. Click to learn more.
 
 # Phishing/Malware Notification Bar.
 # LOCALIZATION NOTE (notADeceptiveSite, notAnAttack)
--- a/browser/modules/PermissionUI.jsm
+++ b/browser/modules/PermissionUI.jsm
@@ -296,17 +296,18 @@ this.PermissionPromptPrototype = {
         callback: state => {
           if (promptAction.callback) {
             promptAction.callback();
           }
 
           if (this.permissionKey) {
 
             // Permanently store permission.
-            if (state && state.checkboxChecked) {
+            if ((state && state.checkboxChecked) ||
+                promptAction.scope == SitePermissions.SCOPE_PERSISTENT) {
               let scope = SitePermissions.SCOPE_PERSISTENT;
               // Only remember permission for session if in PB mode.
               if (PrivateBrowsingUtils.isBrowserPrivate(this.browser)) {
                 scope = SitePermissions.SCOPE_SESSION;
               }
               SitePermissions.set(this.principal.URI,
                                   this.permissionKey,
                                   promptAction.action,
@@ -534,32 +535,18 @@ DesktopNotificationPermissionPrompt.prot
   get permissionKey() {
     return "desktop-notification";
   },
 
   get popupOptions() {
     let learnMoreURL =
       Services.urlFormatter.formatURLPref("app.support.baseURL") + "push";
 
-    let checkbox = {
-      show: true,
-      checked: true,
-      label: gBrowserBundle.GetStringFromName("webNotifications.remember")
-    };
-
-    // In PB mode, the "always remember" checkbox should only remember for the
-    // session.
-    if (PrivateBrowsingUtils.isWindowPrivate(this.browser.ownerGlobal)) {
-      checkbox.label =
-        gBrowserBundle.GetStringFromName("webNotifications.rememberForSession");
-    }
-
     return {
       learnMoreURL,
-      checkbox,
       displayURI: false
     };
   },
 
   get notificationID() {
     return "web-notifications";
   },
 
@@ -578,21 +565,31 @@ DesktopNotificationPermissionPrompt.prot
 
   get promptActions() {
     return [
       {
         label: gBrowserBundle.GetStringFromName("webNotifications.allow"),
         accessKey:
           gBrowserBundle.GetStringFromName("webNotifications.allow.accesskey"),
         action: SitePermissions.ALLOW,
+        scope: SitePermissions.SCOPE_PERSISTENT,
       },
       {
-        label: gBrowserBundle.GetStringFromName("webNotifications.dontAllow"),
+        label: gBrowserBundle.GetStringFromName("webNotifications.notNow"),
         accessKey:
-          gBrowserBundle.GetStringFromName("webNotifications.dontAllow.accesskey"),
+          gBrowserBundle.GetStringFromName("webNotifications.notNow.accesskey"),
         action: SitePermissions.BLOCK,
       },
+      {
+        label: PrivateBrowsingUtils.isBrowserPrivate(this.browser) ?
+          gBrowserBundle.GetStringFromName("webNotifications.neverForSession") :
+          gBrowserBundle.GetStringFromName("webNotifications.never"),
+        accessKey:
+          gBrowserBundle.GetStringFromName("webNotifications.never.accesskey"),
+        action: SitePermissions.BLOCK,
+        scope: SitePermissions.SCOPE_PERSISTENT,
+      },
     ];
   },
 };
 
 PermissionUI.DesktopNotificationPermissionPrompt =
   DesktopNotificationPermissionPrompt;
--- a/browser/modules/test/browser/browser_PermissionUI_prompts.js
+++ b/browser/modules/test/browser/browser_PermissionUI_prompts.js
@@ -45,18 +45,21 @@ function* testPrompt(Prompt) {
     let curPerm = SitePermissions.get(principal.URI, permissionKey, browser);
     Assert.equal(curPerm.state, SitePermissions.UNKNOWN,
                  "Should be no permission set to begin with.");
 
     // First test denying the permission request without the checkbox checked.
     let popupNotification = getPopupNotificationNode();
     popupNotification.checkbox.checked = false;
 
-    Assert.equal(notification.secondaryActions.length, 1,
-                 "There should only be 1 secondary action");
+    let isNotificationPrompt = Prompt == PermissionUI.DesktopNotificationPermissionPrompt;
+
+    let expectedSecondaryActionsCount = isNotificationPrompt ? 2 : 1;
+    Assert.equal(notification.secondaryActions.length, expectedSecondaryActionsCount,
+                 "There should only be " + expectedSecondaryActionsCount + " secondary action(s)");
     yield clickSecondaryAction();
     curPerm = SitePermissions.get(principal.URI, permissionKey, browser);
     Assert.deepEqual(curPerm, {
                        state: SitePermissions.BLOCK,
                        scope: SitePermissions.SCOPE_TEMPORARY,
                      }, "Should have denied the action temporarily");
 
     Assert.ok(mockRequest._cancelled,
@@ -68,23 +71,29 @@ function* testPrompt(Prompt) {
     mockRequest._cancelled = false;
 
     // Bring the PopupNotification back up now...
     shownPromise =
       BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popupshown");
     TestPrompt.prompt();
     yield shownPromise;
 
-    // Test denying the permission request with the checkbox checked.
+    // Test denying the permission request with the checkbox checked (for geolocation)
+    // or by clicking the "never" option from the dropdown (for notifications).
     popupNotification = getPopupNotificationNode();
-    popupNotification.checkbox.checked = true;
+    let secondaryActionToClickIndex = 0;
+    if (isNotificationPrompt) {
+      secondaryActionToClickIndex = 1;
+    } else {
+      popupNotification.checkbox.checked = true;
+    }
 
-    Assert.equal(notification.secondaryActions.length, 1,
-                 "There should only be 1 secondary action");
-    yield clickSecondaryAction();
+    Assert.equal(notification.secondaryActions.length, expectedSecondaryActionsCount,
+                 "There should only be " + expectedSecondaryActionsCount + " secondary action(s)");
+    yield clickSecondaryAction(secondaryActionToClickIndex);
     curPerm = SitePermissions.get(principal.URI, permissionKey);
     Assert.deepEqual(curPerm, {
                        state: SitePermissions.BLOCK,
                        scope: SitePermissions.SCOPE_PERSISTENT
                      }, "Should have denied the action permanently");
     Assert.ok(mockRequest._cancelled,
               "The request should have been cancelled");
     Assert.ok(!mockRequest._allowed,
--- a/browser/modules/test/browser/head.js
+++ b/browser/modules/test/browser/head.js
@@ -173,26 +173,49 @@ function clickMainAction() {
   popupNotification.button.click();
   return removePromise;
 }
 
 /**
  * For an opened PopupNotification, clicks on the secondary action,
  * and waits for the panel to fully close.
  *
+ * @param actionIndex (Number)
+ *        The index of the secondary action to be clicked. The default
+ *        secondary action (the button shown directly in the panel) is
+ *        treated as having index 0.
+ *
  * @return {Promise}
  *         Resolves once the panel has fired the "popuphidden"
  *         event.
  */
-function clickSecondaryAction() {
+function clickSecondaryAction(actionIndex) {
   let removePromise =
     BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popuphidden");
   let popupNotification = getPopupNotificationNode();
-  popupNotification.secondaryButton.click();
-  return removePromise;
+  if (!actionIndex) {
+    popupNotification.secondaryButton.click();
+    return removePromise;
+  }
+
+  return Task.spawn(function* () {
+    // Click the dropmarker arrow and wait for the menu to show up.
+    let dropdownPromise =
+      BrowserTestUtils.waitForEvent(popupNotification.menupopup, "popupshown");
+    yield EventUtils.synthesizeMouseAtCenter(popupNotification.menubutton, {});
+    yield dropdownPromise;
+
+    // The menuitems in the dropdown are accessible as direct children of the panel,
+    // because they are injected into a <children> node in the XBL binding.
+    // The target action is the menuitem at index actionIndex - 1, because the first
+    // secondary action (index 0) is the button shown directly in the panel.
+    let actionMenuItem = popupNotification.querySelectorAll("menuitem")[actionIndex - 1];
+    yield EventUtils.synthesizeMouseAtCenter(actionMenuItem, {});
+    yield removePromise;
+  });
 }
 
 /**
  * Makes sure that 1 (and only 1) <xul:popupnotification> is being displayed
  * by PopupNotification, and then returns that <xul:popupnotification>.
  *
  * @return {<xul:popupnotification>}
  */
--- a/dom/notification/test/browser/browser_permission_dismiss.js
+++ b/dom/notification/test/browser/browser_permission_dismiss.js
@@ -1,14 +1,15 @@
 "use strict";
 
 const ORIGIN_URI = Services.io.newURI("http://mochi.test:8888");
 const PERMISSION_NAME = "desktop-notification";
 const PROMPT_ALLOW_BUTTON = -1;
-const PROMPT_BLOCK_BUTTON = 0;
+const PROMPT_NOT_NOW_BUTTON = 0;
+const PROMPT_NEVER_BUTTON = 1;
 const TEST_URL = "http://mochi.test:8888/browser/dom/notification/test/browser/notification.html";
 
 /**
  * Clicks the specified web-notifications prompt button.
  *
  * @param {Number} aButtonIndex Number indicating which button to click.
  *                              See the constants in this file.
  * @note modified from toolkit/components/passwordmgr/test/browser/head.js
@@ -16,20 +17,26 @@ const TEST_URL = "http://mochi.test:8888
 function clickDoorhangerButton(aButtonIndex) {
   let popup = PopupNotifications.getNotification("web-notifications");
   let notifications = popup.owner.panel.childNodes;
   ok(notifications.length > 0, "at least one notification displayed");
   ok(true, notifications.length + " notification(s)");
   let notification = notifications[0];
 
   if (aButtonIndex == PROMPT_ALLOW_BUTTON) {
-    ok(true, "Triggering main action");
+    ok(true, "Triggering main action (allow the permission)");
     notification.button.doCommand();
+  } else if (aButtonIndex == PROMPT_NEVER_BUTTON) {
+    ok(true, "Triggering secondary action (deny the permission permanently)");
+    // The menuitems in the dropdown are accessible as direct children of the panel,
+    // because they are injected into a <children> node in the XBL binding.
+    // The "never" button is the first menuitem in the dropdown.
+    notification.querySelector("menuitem").doCommand();
   } else {
-    ok(true, "Triggering secondary action");
+    ok(true, "Triggering secondary action (deny the permission temporarily)");
     notification.secondaryButton.doCommand();
   }
 }
 
 /**
  * Opens a tab which calls `Notification.requestPermission()` with a callback
  * argument, calls the `task` function while the permission prompt is open,
  * and verifies that the expected permission is set.
@@ -79,19 +86,32 @@ add_task(function* test_requestPermissio
   ok(!PopupNotifications.getNotification("web-notifications"),
      "Should remove the doorhanger notification icon if granted");
 
   is(Services.perms.testPermission(ORIGIN_URI, PERMISSION_NAME),
      Services.perms.ALLOW_ACTION,
      "Check permission in perm. manager");
 });
 
-add_task(function* test_requestPermission_denied() {
+add_task(function* test_requestPermission_denied_temporarily() {
   yield tabWithRequest(function() {
-    clickDoorhangerButton(PROMPT_BLOCK_BUTTON);
+    clickDoorhangerButton(PROMPT_NOT_NOW_BUTTON);
+  }, "default");
+
+  ok(!PopupNotifications.getNotification("web-notifications"),
+     "Should remove the doorhanger notification icon if denied");
+
+  is(Services.perms.testPermission(ORIGIN_URI, PERMISSION_NAME),
+     Services.perms.UNKNOWN_ACTION,
+     "Check permission in perm. manager");
+});
+
+add_task(function* test_requestPermission_denied_permanently() {
+  yield tabWithRequest(function*() {
+    yield clickDoorhangerButton(PROMPT_NEVER_BUTTON);
   }, "denied");
 
   ok(!PopupNotifications.getNotification("web-notifications"),
      "Should remove the doorhanger notification icon if denied");
 
   is(Services.perms.testPermission(ORIGIN_URI, PERMISSION_NAME),
      Services.perms.DENY_ACTION,
      "Check permission in perm. manager");
--- a/toolkit/content/widgets/notification.xml
+++ b/toolkit/content/widgets/notification.xml
@@ -510,17 +510,18 @@
         </xul:vbox>
       </xul:hbox>
       <xul:hbox class="popup-notification-button-container">
         <children includes="button"/>
         <xul:button anonid="secondarybutton"
                     class="popup-notification-button"
                     xbl:inherits="oncommand=secondarybuttoncommand,label=secondarybuttonlabel,accesskey=secondarybuttonaccesskey,hidden=secondarybuttonhidden"/>
         <xul:toolbarseparator xbl:inherits="hidden=dropmarkerhidden"/>
-        <xul:button type="menu"
+        <xul:button anonid="menubutton"
+                    type="menu"
                     class="popup-notification-button popup-notification-dropmarker"
                     xbl:inherits="onpopupshown=dropmarkerpopupshown,hidden=dropmarkerhidden">
           <xul:menupopup anonid="menupopup"
                          position="after_end"
                          xbl:inherits="oncommand=menucommand">
             <children/>
           </xul:menupopup>
         </xul:button>
@@ -543,14 +544,17 @@
         document.getAnonymousElementByAttribute(this, "anonid", "closebutton");
       </field>
       <field name="button" readonly="true">
         document.getAnonymousElementByAttribute(this, "anonid", "button");
       </field>
       <field name="secondaryButton" readonly="true">
         document.getAnonymousElementByAttribute(this, "anonid", "secondarybutton");
       </field>
+      <field name="menubutton" readonly="true">
+        document.getAnonymousElementByAttribute(this, "anonid", "menubutton");
+      </field>
       <field name="menupopup" readonly="true">
         document.getAnonymousElementByAttribute(this, "anonid", "menupopup");
       </field>
     </implementation>
   </binding>
 </bindings>