Bug 1473671 - Don't store persistent block permission if ESC pressed while showing permission doorhanger. r?florian draft
authorChris Pearce <cpearce@mozilla.com>
Tue, 17 Jul 2018 14:45:17 +1200
changeset 823376 20c109e72554d1120127e4c80b3fc2bdc6db7407
parent 822828 7ba07ef0e4532b644b812942aa38af4510dbc74f
push id117651
push userbmo:cpearce@mozilla.com
push dateFri, 27 Jul 2018 04:24:34 +0000
reviewersflorian
bugs1473671
milestone63.0a1
Bug 1473671 - Don't store persistent block permission if ESC pressed while showing permission doorhanger. r?florian While showing a doorhanger permisison prompt, if the user presses the ESC key we call the secondary action callback, passing in whether any checkbox on the popup notification was checked. In the case of an autoplay-media permission prompt, we have a "remember" checkbox, which is checked by default. So pressing ESC means the user will remember a "block" result for the current site. We believe that users don't expect pressing ESC to result in a remembered decision, they expect pressing ESC to avoid making a decision. So we want to ignore the checkbox when ESC is pressed for autoplay-media. So this patch adds a new PopupNotification behaviour which reports the source of event which caused the action callback to be called. This enables the ESC key press to ignore storing a permission. Note: the change here to not store a permission on ESC press applies to all permissions, not just autoplay-media. MozReview-Commit-ID: IUWFN8LG9VF
browser/base/content/test/popupNotifications/browser_popupNotification.js
browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js
browser/base/content/test/popupNotifications/head.js
browser/modules/PermissionUI.jsm
dom/quota/test/browser_permissionsPromptDeny.js
toolkit/modules/PopupNotifications.jsm
--- a/browser/base/content/test/popupNotifications/browser_popupNotification.js
+++ b/browser/base/content/test/popupNotifications/browser_popupNotification.js
@@ -24,31 +24,35 @@ var tests = [
     onShown(popup) {
       checkPopup(popup, this.notifyObj);
       triggerMainCommand(popup);
     },
     onHidden(popup) {
       ok(this.notifyObj.mainActionClicked, "mainAction was clicked");
       ok(!this.notifyObj.dismissalCallbackTriggered, "dismissal callback wasn't triggered");
       ok(this.notifyObj.removedCallbackTriggered, "removed callback triggered");
+      is(this.notifyObj.mainActionSource, "button", "main action should have been triggered by button.");
+      is(this.notifyObj.secondaryActionSource, undefined, "shouldn't have a secondary action source.");
     }
   },
   { id: "Test#2",
     run() {
       this.notifyObj = new BasicNotification(this.id);
       showNotification(this.notifyObj);
     },
     onShown(popup) {
       checkPopup(popup, this.notifyObj);
       triggerSecondaryCommand(popup, 0);
     },
     onHidden(popup) {
       ok(this.notifyObj.secondaryActionClicked, "secondaryAction was clicked");
       ok(!this.notifyObj.dismissalCallbackTriggered, "dismissal callback wasn't triggered");
       ok(this.notifyObj.removedCallbackTriggered, "removed callback triggered");
+      is(this.notifyObj.mainActionSource, undefined, "shouldn't have a main action source.");
+      is(this.notifyObj.secondaryActionSource, "button", "secondary action should have been triggered by button.");
     }
   },
   { id: "Test#2b",
     run() {
       this.notifyObj = new BasicNotification(this.id);
       this.notifyObj.secondaryActions.push({
         label: "Extra Secondary Action",
         accessKey: "E",
--- a/browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js
+++ b/browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js
@@ -25,16 +25,18 @@ var tests = [
       checkPopup(popup, this.notifyObj);
       EventUtils.synthesizeKey("KEY_Escape");
     },
     onHidden(popup) {
       ok(!this.notifyObj.mainActionClicked, "mainAction was not clicked");
       ok(this.notifyObj.secondaryActionClicked, "secondaryAction was clicked");
       ok(!this.notifyObj.dismissalCallbackTriggered, "dismissal callback wasn't triggered");
       ok(this.notifyObj.removedCallbackTriggered, "removed callback triggered");
+      is(this.notifyObj.mainActionSource, undefined, "shouldn't have a main action source.");
+      is(this.notifyObj.secondaryActionSource, "esc-press", "secondary action should be from ESC key press");
     }
   },
   // Test that for non-persistent notifications, the escape key dismisses the notification.
   { id: "Test#2",
     async run() {
       await waitForWindowReadyForPopupNotifications(window);
       this.notifyObj = new BasicNotification(this.id);
       this.notification = showNotification(this.notifyObj);
@@ -43,16 +45,18 @@ var tests = [
       checkPopup(popup, this.notifyObj);
       EventUtils.synthesizeKey("KEY_Escape");
     },
     onHidden(popup) {
       ok(!this.notifyObj.mainActionClicked, "mainAction was not clicked");
       ok(!this.notifyObj.secondaryActionClicked, "secondaryAction was not clicked");
       ok(this.notifyObj.dismissalCallbackTriggered, "dismissal callback triggered");
       ok(!this.notifyObj.removedCallbackTriggered, "removed callback was not triggered");
+      is(this.notifyObj.mainActionSource, undefined, "shouldn't have a main action source.");
+      is(this.notifyObj.secondaryActionSource, undefined, "shouldn't have a secondary action source.");
       this.notification.remove();
     }
   },
   // Test that the space key on an anchor element focuses an active notification
   { id: "Test#3",
     run() {
       this.notifyObj = new BasicNotification(this.id);
       this.notifyObj.anchorID = "geo-notification-icon";
--- a/browser/base/content/test/popupNotifications/head.js
+++ b/browser/base/content/test/popupNotifications/head.js
@@ -109,23 +109,29 @@ function dismissNotification(popup) {
 function BasicNotification(testId) {
   this.browser = gBrowser.selectedBrowser;
   this.id = "test-notification-" + testId;
   this.message = testId + ": Will you allow <> to perform this action?";
   this.anchorID = null;
   this.mainAction = {
     label: "Main Action",
     accessKey: "M",
-    callback: () => this.mainActionClicked = true
+    callback: ({source}) => {
+      this.mainActionClicked = true;
+      this.mainActionSource = source;
+    },
   };
   this.secondaryActions = [
     {
       label: "Secondary Action",
       accessKey: "S",
-      callback: () => this.secondaryActionClicked = true
+      callback: ({source}) => {
+        this.secondaryActionClicked = true;
+        this.secondaryActionSource = source;
+      },
     }
   ];
   this.options = {
     name: "http://example.com",
     eventCallback: eventName => {
       switch (eventName) {
         case "dismissed":
           this.dismissalCallbackTriggered = true;
--- a/browser/modules/PermissionUI.jsm
+++ b/browser/modules/PermissionUI.jsm
@@ -307,20 +307,19 @@ var PermissionPromptPrototype = {
         label: promptAction.label,
         accessKey: promptAction.accessKey,
         callback: state => {
           if (promptAction.callback) {
             promptAction.callback();
           }
 
           if (this.permissionKey) {
-
-            // Permanently store permission.
-            if ((state && state.checkboxChecked) ||
-                promptAction.scope == SitePermissions.SCOPE_PERSISTENT) {
+            if ((state && state.checkboxChecked && state.source != "esc-press") ||
+                 promptAction.scope == SitePermissions.SCOPE_PERSISTENT) {
+              // Permanently store permission.
               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,
--- a/dom/quota/test/browser_permissionsPromptDeny.js
+++ b/dom/quota/test/browser_permissionsPromptDeny.js
@@ -76,15 +76,25 @@ add_task(async function testPermissionDe
 
   info("Creating tab");
   gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
 
   info("Loading test page: " + testPageURL);
   gBrowser.selectedBrowser.loadURI(testPageURL);
   await waitForMessage(false, gBrowser);
 
+  // Pressing ESC results in a temporary block permission on the browser object.
+  // So the global permission for the URL should still be unknown, but the browser
+  // should have a block permission with a temporary scope.
   is(getPermission(testPageURL, "persistent-storage"),
-     Ci.nsIPermissionManager.DENY_ACTION,
+     Ci.nsIPermissionManager.UNKNOWN_ACTION,
      "Correct permission set");
+
+  let tempBlock = SitePermissions.getAllForBrowser(gBrowser.selectedBrowser)
+                                 .find(p => p.id == "persistent-storage" &&
+                                            p.state == SitePermissions.BLOCK &&
+                                            p.scope == SitePermissions.SCOPE_TEMPORARY);
+  ok(tempBlock, "Should have a temporary block permission on active browser");
+
   unregisterAllPopupEventHandlers();
   gBrowser.removeCurrentTab();
   removePermission(testPageURL, "persistent-storage");
 });
--- a/toolkit/modules/PopupNotifications.jsm
+++ b/toolkit/modules/PopupNotifications.jsm
@@ -248,17 +248,17 @@ function PopupNotifications(tabbrowser, 
 
     // If the chrome window has a focused element, let it handle the ESC key instead.
     if (!focusedElement ||
         focusedElement == doc.body ||
         focusedElement == this.tabbrowser.selectedBrowser ||
         // Ignore focused elements inside the notification.
         getNotificationFromElement(focusedElement) == notification ||
         notification.contains(focusedElement)) {
-      this._onButtonEvent(aEvent, "secondarybuttoncommand", notification);
+      this._onButtonEvent(aEvent, "secondarybuttoncommand", "esc-press", notification);
     }
   };
 
   let documentElement = this.window.document.documentElement;
   let locationBarHidden = documentElement.getAttribute("chromehidden").includes("location");
   let isFullscreen = !!this.window.document.fullscreenElement;
 
   this.panel.setAttribute("followanchor", !locationBarHidden && !isFullscreen);
@@ -340,16 +340,21 @@ PopupNotifications.prototype = {
    * @param mainAction
    *        A JavaScript object literal describing the notification button's
    *        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.
+   *              - source: (string): the source of the action that initiated the
+   *                callback, either:
+   *                - "button" if popup buttons were directly activated, or
+   *                - "esc-press" if the user pressed the escape key, or
+   *                - "menucommand" if a menu was activated.
    *          - [optional] dismiss (boolean): If this is true, the notification
    *            will be dismissed instead of removed after running the callback.
    *          - [optional] disableHighlight (boolean): If this is true, the button
    *            will not apply the default highlight style.
    *        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
@@ -804,31 +809,31 @@ PopupNotifications.prototype = {
       popupnotification.setAttribute("label", desc.start);
       popupnotification.setAttribute("name", desc.name);
       popupnotification.setAttribute("endlabel", desc.end);
 
       popupnotification.setAttribute("id", popupnotificationID);
       popupnotification.setAttribute("popupid", n.id);
       popupnotification.setAttribute("oncommand", "PopupNotifications._onCommand(event);");
       if (Services.prefs.getBoolPref("privacy.permissionPrompts.showCloseButton")) {
-        popupnotification.setAttribute("closebuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand');");
+        popupnotification.setAttribute("closebuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand', 'esc-press');");
       } else {
         popupnotification.setAttribute("closebuttoncommand", `PopupNotifications._dismiss(event, ${TELEMETRY_STAT_DISMISSAL_CLOSE_BUTTON});`);
       }
       if (n.mainAction) {
         popupnotification.setAttribute("buttonlabel", n.mainAction.label);
         popupnotification.setAttribute("buttonaccesskey", n.mainAction.accessKey);
         popupnotification.setAttribute("buttonhighlight", !n.mainAction.disableHighlight);
-        popupnotification.setAttribute("buttoncommand", "PopupNotifications._onButtonEvent(event, 'buttoncommand');");
+        popupnotification.setAttribute("buttoncommand", "PopupNotifications._onButtonEvent(event, 'buttoncommand', 'button');");
         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.setAttribute("buttoncommand", "PopupNotifications._onButtonEvent(event, 'buttoncommand', 'button');");
         popupnotification.setAttribute("buttonhighlight", "true");
         popupnotification.removeAttribute("buttonlabel");
         popupnotification.removeAttribute("buttonaccesskey");
         popupnotification.removeAttribute("dropmarkerpopupshown");
         popupnotification.removeAttribute("learnmoreclick");
         popupnotification.removeAttribute("menucommand");
       }
 
@@ -870,17 +875,17 @@ PopupNotifications.prototype = {
       popupnotification.notification = n;
 
       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');");
+        popupnotification.setAttribute("secondarybuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand', 'button');");
 
         for (let i = 1; i < n.secondaryActions.length; i++) {
           let action = n.secondaryActions[i];
           let item = doc.createElementNS(XUL_NS, "menuitem");
           item.setAttribute("label", action.label);
           item.setAttribute("accesskey", action.accessKey);
           item.notification = n;
           item.action = action;
@@ -1472,17 +1477,17 @@ PopupNotifications.prototype = {
         break;
       }
       notificationEl =
         notificationEl.ownerDocument.getBindingParent(notificationEl) || notificationEl.parentNode;
     }
     this._setNotificationUIState(notificationEl);
   },
 
-  _onButtonEvent(event, type, notificationEl = null) {
+  _onButtonEvent(event, type, source = "unspecified", notificationEl = null) {
     if (!notificationEl) {
       notificationEl = getNotificationFromElement(event.originalTarget);
     }
 
     if (!notificationEl)
       throw "PopupNotifications._onButtonEvent: couldn't find notification element";
 
     if (!notificationEl.notification)
@@ -1536,17 +1541,18 @@ PopupNotifications.prototype = {
       telemetryStatId = TELEMETRY_STAT_ACTION_2;
     }
 
     notification._recordTelemetryStat(telemetryStatId);
 
     if (action) {
       try {
         action.callback.call(undefined, {
-          checkboxChecked: notificationEl.checkbox.checked
+          checkboxChecked: notificationEl.checkbox.checked,
+          source,
         });
       } catch (error) {
         Cu.reportError(error);
       }
 
       if (action.dismiss) {
         this._dismiss();
         return;
@@ -1564,17 +1570,18 @@ PopupNotifications.prototype = {
 
     let notificationEl = target.parentElement;
     event.stopPropagation();
 
     target.notification._recordTelemetryStat(target.action.telemetryStatId);
 
     try {
       target.action.callback.call(undefined, {
-        checkboxChecked: notificationEl.checkbox.checked
+        checkboxChecked: notificationEl.checkbox.checked,
+        source: "menucommand",
       });
     } catch (error) {
       Cu.reportError(error);
     }
 
     if (target.action.dismiss) {
       this._dismiss();
       return;