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 822841 dec6f13a08bda5ecb170fbde6fce8627fdff7725
parent 822828 7ba07ef0e4532b644b812942aa38af4510dbc74f
push id117484
push userbmo:cpearce@mozilla.com
push dateThu, 26 Jul 2018 02:56:54 +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_keyboard.js
browser/modules/PermissionUI.jsm
toolkit/modules/PopupNotifications.jsm
--- a/browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js
+++ b/browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js
@@ -7,21 +7,30 @@ function test() {
 
   ok(PopupNotifications, "PopupNotifications object exists");
   ok(PopupNotifications.panel, "PopupNotifications panel exists");
 
   // Force tabfocus for all elements on OSX.
   SpecialPowers.pushPrefEnv({"set": [["accessibility.tabfocus", 7]]}).then(setup);
 }
 
+function promiseElementVisible(element) {
+  // HTMLElement.offsetParent is null when the element is not visible
+  // (or if the element has |position: fixed|). See:
+  // https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetParent
+  return BrowserTestUtils.waitForCondition(() => element.offsetParent !== null,
+                                          "Waiting for element to be visible");
+}
+
 var tests = [
   // Test that for persistent notifications,
   // the secondary action is triggered by pressing the escape key.
   { id: "Test#1",
-    run() {
+    async run() {
+      await waitForWindowReadyForPopupNotifications(window);
       this.notifyObj = new BasicNotification(this.id);
       this.notifyObj.options.persistent = true;
       showNotification(this.notifyObj);
     },
     onShown(popup) {
       checkPopup(popup, this.notifyObj);
       EventUtils.synthesizeKey("KEY_Escape");
     },
@@ -48,17 +57,18 @@ var tests = [
       ok(!this.notifyObj.secondaryActionClicked, "secondaryAction was not clicked");
       ok(this.notifyObj.dismissalCallbackTriggered, "dismissal callback triggered");
       ok(!this.notifyObj.removedCallbackTriggered, "removed callback was not triggered");
       this.notification.remove();
     }
   },
   // Test that the space key on an anchor element focuses an active notification
   { id: "Test#3",
-    run() {
+    async run() {
+      await waitForWindowReadyForPopupNotifications(window);
       this.notifyObj = new BasicNotification(this.id);
       this.notifyObj.anchorID = "geo-notification-icon";
       this.notifyObj.addOptions({
         persistent: true,
       });
       this.notification = showNotification(this.notifyObj);
     },
     onShown(popup) {
@@ -71,16 +81,17 @@ var tests = [
       this.notification.remove();
     },
     onHidden(popup) { }
   },
   // Test that you can switch between active notifications with the space key
   // and that the notification is focused on selection.
   { id: "Test#4",
     async run() {
+      await waitForWindowReadyForPopupNotifications(window);
       let notifyObj1 = new BasicNotification(this.id);
       notifyObj1.id += "_1";
       notifyObj1.anchorID = "default-notification-icon";
       notifyObj1.addOptions({
         hideClose: true,
         checkbox: {
           label: "Test that elements inside the panel can be focused",
         },
@@ -127,17 +138,18 @@ var tests = [
 
       notification1.remove();
       notification2.remove();
       goNext();
     },
   },
   // Test that passing the autofocus option will focus an opened notification.
   { id: "Test#5",
-    run() {
+    async run() {
+      await waitForWindowReadyForPopupNotifications(window);
       this.notifyObj = new BasicNotification(this.id);
       this.notifyObj.anchorID = "geo-notification-icon";
       this.notifyObj.addOptions({
         autofocus: true,
       });
       this.notification = showNotification(this.notifyObj);
     },
     onShown(popup) {
@@ -170,16 +182,17 @@ var tests = [
 
       this.notification.remove();
       notification.remove();
     }
   },
   // Test that focus is not moved out of a content element if autofocus is not set.
   { id: "Test#6",
     async run() {
+      await waitForWindowReadyForPopupNotifications(window);
       let id = this.id;
       await BrowserTestUtils.withNewTab("data:text/html,<input id='test-input'/>", async function(browser) {
         let notifyObj = new BasicNotification(id);
         await ContentTask.spawn(browser, {}, function() {
           content.document.getElementById("test-input").focus();
         });
 
         let opened = waitForNotificationPanel();
@@ -200,9 +213,50 @@ var tests = [
           is(content.document.activeElement, content.document.getElementById("test-input"));
         });
 
         notification.remove();
       });
       goNext();
     },
   },
+  // Test that when the escape key is pressed the secondary action is
+  // called with a source of "esc-press".
+  { id: "Test#7",
+    async run() {
+      await waitForWindowReadyForPopupNotifications(window);
+      this.checkboxChecked = null;
+      this.source = null;
+      this.notifyObj = new BasicNotification(this.id);
+      this.notifyObj.options.persistent = true;
+      // this.notifyObj.options.overrideCheckboxOnEscPress = true;
+      // this.notifyObj.options.checkbox = {
+      //   label: "This is a checkbox",
+      //   checked: true,
+      // };
+      this.notifyObj.secondaryActions = [{
+        label: "Test Secondary",
+        accessKey: "T",
+        callback: ({source}) => {
+          this.notifyObj.secondaryActionClicked = true;
+          // this.checkboxChecked = checkboxChecked;
+          this.source = source;
+        },
+      }];
+      this.notification = showNotification(this.notifyObj);
+    },
+    async onShown(popup) {
+      checkPopup(popup, this.notifyObj);
+      // let notification = popup.childNodes[0];
+      // let checkbox = notification.checkbox;
+      // ok(checkbox.checked, "Checkbox should be checked");
+      // await promiseElementVisible(checkbox);
+      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 not triggered");
+      ok(this.notifyObj.removedCallbackTriggered, "removed callback was triggered");
+      is(this.source, "esc-press", "Checkbox should have been reported as unchecked in the secondary callback");
+    }
+  },
 ];
--- a/browser/modules/PermissionUI.jsm
+++ b/browser/modules/PermissionUI.jsm
@@ -307,38 +307,39 @@ 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) {
-              let scope = SitePermissions.SCOPE_PERSISTENT;
-              // Only remember permission for session if in PB mode.
-              if (PrivateBrowsingUtils.isBrowserPrivate(this.browser)) {
-                scope = SitePermissions.SCOPE_SESSION;
+            if (state.source !== "esc-press") {
+              if ((state && state.checkboxChecked) ||
+                        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,
+                                    scope);
+              } else if (promptAction.action == SitePermissions.BLOCK) {
+                // Temporarily store BLOCK permissions only.
+                // SitePermissions does not consider subframes when storing temporary
+                // permissions on a tab, thus storing ALLOW could be exploited.
+                SitePermissions.set(this.principal.URI,
+                                    this.permissionKey,
+                                    promptAction.action,
+                                    SitePermissions.SCOPE_TEMPORARY,
+                                    this.browser);
               }
-              SitePermissions.set(this.principal.URI,
-                                  this.permissionKey,
-                                  promptAction.action,
-                                  scope);
-            } else if (promptAction.action == SitePermissions.BLOCK) {
-              // Temporarily store BLOCK permissions only.
-              // SitePermissions does not consider subframes when storing temporary
-              // permissions on a tab, thus storing ALLOW could be exploited.
-              SitePermissions.set(this.principal.URI,
-                                  this.permissionKey,
-                                  promptAction.action,
-                                  SitePermissions.SCOPE_TEMPORARY,
-                                  this.browser);
             }
 
             // Grant permission if action is ALLOW.
             if (promptAction.action == SitePermissions.ALLOW) {
               this.allow();
             } else {
               this.cancel();
             }
--- 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,20 @@ 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.
    *          - [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 +808,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', 'button');");
       } 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 +874,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 +1476,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 +1540,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;