Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited. r=johannh, a=gchang
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Fri, 10 Feb 2017 13:16:02 +0000
changeset 376210 8dadad0f570d4e76beb0d9300a1ccb90926cf6d2
parent 376209 8103314b2cd43d908f44d600315fc0127b7c1e79
child 376211 4960fa87292a33ad253d7525b9a68cc670a7741e
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjohannh, gchang
bugs1328304
milestone53.0a2
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited. r=johannh, a=gchang MozReview-Commit-ID: 5GhCsA9Gi3f
browser/base/content/browser.js
browser/base/content/test/popupNotifications/browser_popupNotification_no_anchors.js
browser/base/content/test/popupNotifications/head.js
browser/base/content/urlbarBindings.xml
toolkit/modules/PopupNotifications.jsm
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -146,19 +146,26 @@ XPCOMUtils.defineLazyGetter(this, "PageM
   Cu.import("resource://gre/modules/PageMenu.jsm", tmp);
   return new tmp.PageMenuParent();
 });
 
 XPCOMUtils.defineLazyGetter(this, "PopupNotifications", function() {
   let tmp = {};
   Cu.import("resource://gre/modules/PopupNotifications.jsm", tmp);
   try {
+    // Hide all notifications while the URL is being edited and the address bar
+    // has focus, including the virtual focus in the results popup.
+    let shouldSuppress = () => {
+      return gURLBar.getAttribute("pageproxystate") != "valid" &&
+             gURLBar.focused;
+    };
     return new tmp.PopupNotifications(gBrowser,
                                       document.getElementById("notification-popup"),
-                                      document.getElementById("notification-popup-box"));
+                                      document.getElementById("notification-popup-box"),
+                                      { shouldSuppress });
   } catch (ex) {
     Cu.reportError(ex);
     return null;
   }
 });
 
 XPCOMUtils.defineLazyGetter(this, "Win7Features", function() {
   if (AppConstants.platform != "win")
@@ -2419,27 +2426,18 @@ function BrowserPageInfo(documentURL, in
                     "chrome,toolbar,dialog=no,resizable", args);
 }
 
 /**
  * Sets the URI to display in the location bar.
  *
  * @param aURI [optional]
  *        nsIURI to set. If this is unspecified, the current URI will be used.
- * @param aOptions [optional]
- *        An object with the following properties:
- *        {
- *          isForLocationChange:
- *            Set to true to indicate that the function was invoked to respond
- *            to a location change event, rather than to reset the current URI
- *            value. This is useful to avoid calling PopupNotifications.jsm
- *            multiple times.
- *        }
  */
-function URLBarSetURI(aURI, aOptions = {}) {
+function URLBarSetURI(aURI) {
   var value = gBrowser.userTypedValue;
   var valid = false;
 
   if (value == null) {
     let uri = aURI || gBrowser.currentURI;
     // Strip off "wyciwyg://" and passwords for the location bar
     try {
       uri = Services.uriFixup.createExposableURI(uri);
@@ -2462,17 +2460,17 @@ function URLBarSetURI(aURI, aOptions = {
       }
     }
 
     valid = !isBlankPageURL(uri.spec);
   }
 
   gURLBar.value = value;
   gURLBar.valueIsTyped = !valid;
-  SetPageProxyState(valid ? "valid" : "invalid", aOptions);
+  SetPageProxyState(valid ? "valid" : "invalid");
 }
 
 function losslessDecodeURI(aURI) {
   let scheme = aURI.scheme;
   if (scheme == "moz-action")
     throw new Error("losslessDecodeURI should never get a moz-action URI");
 
   var value = aURI.spec;
@@ -2569,48 +2567,56 @@ function UpdatePageProxyState() {
  * different than the loaded page, because it's being edited or because a search
  * result is currently selected and is displayed in the location bar.
  *
  * @param aState
  *        The string "valid" indicates that the security indicators and other
  *        related user interface elments should be shown because the URI in the
  *        location bar matches the loaded page. The string "invalid" indicates
  *        that the URI in the location bar is different than the loaded page.
- * @param aOptions [optional]
- *        An object with the following properties:
- *        {
- *          isForLocationChange:
- *            Set to true to indicate that the function was invoked to respond
- *            to a location change event. This is useful to avoid calling
- *            PopupNotifications.jsm multiple times.
- *        }
  */
-function SetPageProxyState(aState, aOptions = {}) {
+function SetPageProxyState(aState) {
   if (!gURLBar)
     return;
 
+  let oldPageProxyState = gURLBar.getAttribute("pageproxystate");
+  // The "browser_urlbar_stop_pending.js" test uses a MutationObserver to do
+  // some verifications at this point, and it breaks if we don't write the
+  // attribute, even if it hasn't changed (bug 1338115).
   gURLBar.setAttribute("pageproxystate", aState);
 
   // the page proxy state is set to valid via OnLocationChange, which
   // gets called when we switch tabs.
   if (aState == "valid") {
     gLastValidURLStr = gURLBar.value;
     gURLBar.addEventListener("input", UpdatePageProxyState);
   } else if (aState == "invalid") {
     gURLBar.removeEventListener("input", UpdatePageProxyState);
   }
 
-  // Only need to call anchorVisibilityChange if the PopupNotifications object
-  // for this window has already been initialized (i.e. its getter no
-  // longer exists). If this is the result of a locations change, then we will
-  // already invoke PopupNotifications.locationChange separately.
-  if (!Object.getOwnPropertyDescriptor(window, "PopupNotifications").get &&
-      !aOptions.isForLocationChange) {
-    PopupNotifications.anchorVisibilityChange();
-  }
+  // After we've ensured that we've applied the listeners and updated the value
+  // of gLastValidURLStr, return early if the actual state hasn't changed.
+  if (oldPageProxyState == aState) {
+    return;
+  }
+
+  UpdatePopupNotificationsVisibility();
+}
+
+function UpdatePopupNotificationsVisibility() {
+  // Only need to do something if the PopupNotifications object for this window
+  // has already been initialized (i.e. its getter no longer exists).
+  if (Object.getOwnPropertyDescriptor(window, "PopupNotifications").get) {
+    return;
+  }
+
+  // Notify PopupNotifications that the visible anchors may have changed. This
+  // also checks the suppression state according to the "shouldSuppress"
+  // function defined earlier in this file.
+  PopupNotifications.anchorVisibilityChange();
 }
 
 function PageProxyClickHandler(aEvent) {
   if (aEvent.button == 1 && gPrefService.getBoolPref("middlemouse.paste"))
     middleMousePaste(aEvent);
 }
 
 var gMenuButtonBadgeManager = {
@@ -4513,17 +4519,17 @@ var XULBrowserWindow = {
       if ((location == "about:blank" && checkEmptyPageOrigin()) ||
           location == "") {  // Second condition is for new tabs, otherwise
                              // reload function is enabled until tab is refreshed.
         this.reloadCommand.setAttribute("disabled", "true");
       } else {
         this.reloadCommand.removeAttribute("disabled");
       }
 
-      URLBarSetURI(aLocationURI, { isForLocationChange: true });
+      URLBarSetURI(aLocationURI);
 
       BookmarkingUI.onLocationChange();
 
       gIdentityHandler.onLocationChange();
 
       SocialUI.updateState();
 
       UITour.onLocationChange(location);
--- a/browser/base/content/test/popupNotifications/browser_popupNotification_no_anchors.js
+++ b/browser/base/content/test/popupNotifications/browser_popupNotification_no_anchors.js
@@ -97,57 +97,134 @@ var tests = [
       dismissNotification(popup);
     },
     onHidden(popup) {
       this.notification.remove();
       gBrowser.removeTab(gBrowser.selectedTab);
       gBrowser.selectedTab = this.oldSelectedTab;
     }
   },
-  // Test that popupnotifications are anchored to the identity icon while
-  // editing the URL in the location bar, and restored to their anchors when the
-  // URL is reverted.
+  // Test that popupnotifications are hidden while editing the URL in the
+  // location bar, anchored to the identity icon when the focus is moved away
+  // from the location bar, and restored when the URL is reverted.
   { id: "Test#4",
     *run() {
-      this.oldSelectedTab = gBrowser.selectedTab;
-      yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/");
+      for (let persistent of [false, true]) {
+        let shown = waitForNotificationPanel();
+        this.notifyObj = new BasicNotification(this.id);
+        this.notifyObj.anchorID = "geo-notification-icon";
+        this.notifyObj.addOptions({ persistent });
+        this.notification = showNotification(this.notifyObj);
+        yield shown;
+
+        checkPopup(PopupNotifications.panel, this.notifyObj);
+
+        // Typing in the location bar should hide the notification.
+        let hidden = waitForNotificationPanelHidden();
+        gURLBar.select();
+        EventUtils.synthesizeKey("*", {});
+        yield hidden;
+
+        is(document.getElementById("geo-notification-icon").boxObject.width, 0,
+           "geo anchor shouldn't be visible");
+
+        // Moving focus to the next control should show the notifications again,
+        // anchored to the identity icon. We clear the URL bar before moving the
+        // focus so that the awesomebar popup doesn't get in the way.
+        shown = waitForNotificationPanel();
+        EventUtils.synthesizeKey("VK_BACK_SPACE", {});
+        EventUtils.synthesizeKey("VK_TAB", {});
+        yield shown;
+
+        is(PopupNotifications.panel.anchorNode.id, "identity-icon",
+           "notification anchored to identity icon");
+
+        // Moving focus to the location bar should hide the notification again.
+        hidden = waitForNotificationPanelHidden();
+        EventUtils.synthesizeKey("VK_TAB", { shiftKey: true });
+        yield hidden;
+
+        // Reverting the URL should show the notification again.
+        shown = waitForNotificationPanel();
+        EventUtils.synthesizeKey("VK_ESCAPE", {});
+        yield shown;
+
+        checkPopup(PopupNotifications.panel, this.notifyObj);
 
-      let shownInitially = waitForNotificationPanel();
+        hidden = waitForNotificationPanelHidden();
+        this.notification.remove();
+        yield hidden;
+      }
+      goNext();
+    }
+  },
+  // Test that popupnotifications triggered while editing the URL in the
+  // location bar are only shown later when the URL is reverted.
+  { id: "Test#5",
+    *run() {
+      for (let persistent of [false, true]) {
+        // Start editing the URL, ensuring that the awesomebar popup is hidden.
+        gURLBar.select();
+        EventUtils.synthesizeKey("*", {});
+        EventUtils.synthesizeKey("VK_BACK_SPACE", {});
+
+        // Trying to show a notification should display nothing.
+        let notShowing = promiseTopicObserved("PopupNotifications-updateNotShowing");
+        this.notifyObj = new BasicNotification(this.id);
+        this.notifyObj.anchorID = "geo-notification-icon";
+        this.notifyObj.addOptions({ persistent });
+        this.notification = showNotification(this.notifyObj);
+        yield notShowing;
+
+        // Reverting the URL should show the notification.
+        let shown = waitForNotificationPanel();
+        EventUtils.synthesizeKey("VK_ESCAPE", {});
+        yield shown;
+
+        checkPopup(PopupNotifications.panel, this.notifyObj);
+
+        let hidden = waitForNotificationPanelHidden();
+        this.notification.remove();
+        yield hidden;
+      }
+
+      goNext();
+    }
+  },
+  // Test that persistent panels are still open after switching to another tab
+  // and back, even while editing the URL in the new tab.
+  { id: "Test#6",
+    *run() {
+      let shown = waitForNotificationPanel();
       this.notifyObj = new BasicNotification(this.id);
       this.notifyObj.anchorID = "geo-notification-icon";
       this.notifyObj.addOptions({
         persistent: true,
       });
       this.notification = showNotification(this.notifyObj);
-      yield shownInitially;
+      yield shown;
+
+      // Switching to a new tab should hide the notification.
+      let hidden = waitForNotificationPanelHidden();
+      this.oldSelectedTab = gBrowser.selectedTab;
+      yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/");
+      yield hidden;
+
+      // Start editing the URL.
+      gURLBar.select();
+      EventUtils.synthesizeKey("*", {});
+
+      // Switching to the old tab should show the notification again.
+      shown = waitForNotificationPanel();
+      gBrowser.removeTab(gBrowser.selectedTab);
+      gBrowser.selectedTab = this.oldSelectedTab;
+      yield shown;
 
       checkPopup(PopupNotifications.panel, this.notifyObj);
 
-      let shownAgain = waitForNotificationPanel();
-      // This will cause the popup to hide and show again.
-      gURLBar.select();
-      EventUtils.synthesizeKey("*", {});
-      // Keep the URL bar empty, so we don't show the awesomebar.
-      EventUtils.synthesizeKey("VK_BACK_SPACE", {});
-      yield shownAgain;
-
-      is(document.getElementById("geo-notification-icon").boxObject.width, 0,
-         "geo anchor shouldn't be visible");
-      is(PopupNotifications.panel.anchorNode.id, "identity-icon",
-         "notification anchored to identity icon");
-
-      let shownLastTime = waitForNotificationPanel();
-      // This will cause the popup to hide and show again.
-      EventUtils.synthesizeKey("VK_ESCAPE", {});
-      yield shownLastTime;
-
-      checkPopup(PopupNotifications.panel, this.notifyObj);
-
-      let hidden = new Promise(resolve => onPopupEvent("popuphidden", resolve));
+      hidden = waitForNotificationPanelHidden();
       this.notification.remove();
-      gBrowser.removeTab(gBrowser.selectedTab);
-      gBrowser.selectedTab = this.oldSelectedTab;
       yield hidden;
 
       goNext();
     }
   },
 ];
--- a/browser/base/content/test/popupNotifications/head.js
+++ b/browser/base/content/test/popupNotifications/head.js
@@ -259,16 +259,24 @@ function onPopupEvent(eventName, callbac
 function waitForNotificationPanel() {
   return new Promise(resolve => {
     onPopupEvent("popupshown", function() {
       resolve(this);
     });
   });
 }
 
+function waitForNotificationPanelHidden() {
+  return new Promise(resolve => {
+    onPopupEvent("popuphidden", function() {
+      resolve(this);
+    });
+  });
+}
+
 function triggerMainCommand(popup) {
   let notifications = popup.childNodes;
   ok(notifications.length > 0, "at least one notification displayed");
   let notification = notifications[0];
   info("Triggering main command for notification " + notification.id);
   EventUtils.synthesizeMouseAtCenter(notification.button, {});
 }
 
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -1222,23 +1222,29 @@ file, You can obtain one at http://mozil
             this._clearNoActions();
         }
       ]]></handler>
 
       <handler event="focus"><![CDATA[
         if (event.originalTarget == this.inputField) {
           this._hideURLTooltip();
           this.formatValue();
+          if (this.getAttribute("pageproxystate") != "valid") {
+            UpdatePopupNotificationsVisibility();
+          }
         }
       ]]></handler>
 
       <handler event="blur"><![CDATA[
         if (event.originalTarget == this.inputField) {
           this._clearNoActions();
           this.formatValue();
+          if (this.getAttribute("pageproxystate") != "valid") {
+            UpdatePopupNotificationsVisibility();
+          }
         }
         if (ExtensionSearchHandler.hasActiveInputSession()) {
           ExtensionSearchHandler.handleInputCancelled();
         }
       ]]></handler>
 
       <handler event="dragstart" phase="capturing"><![CDATA[
         // Drag only if the gesture starts from the input field.
--- a/toolkit/modules/PopupNotifications.jsm
+++ b/toolkit/modules/PopupNotifications.jsm
@@ -197,25 +197,37 @@ Notification.prototype = {
  *        populated with <popupnotification> children and displayed it as
  *        needed.
  * @param iconBox
  *        Reference to a container element that should be hidden or
  *        unhidden when notifications are hidden or shown. It should be the
  *        parent of anchor elements whose IDs are passed to show().
  *        It is used as a fallback popup anchor if notifications specify
  *        invalid or non-existent anchor IDs.
+ * @param options
+ *        An optional object with the following optional properties:
+ *        {
+ *          shouldSuppress:
+ *            If this function returns true, then all notifications are
+ *            suppressed for this window. This state is checked on construction
+ *            and when the "anchorVisibilityChange" method is called.
+ *        }
  */
-this.PopupNotifications = function PopupNotifications(tabbrowser, panel, iconBox) {
+this.PopupNotifications = function PopupNotifications(tabbrowser, panel,
+                                                      iconBox, options = {}) {
   if (!(tabbrowser instanceof Ci.nsIDOMXULElement))
     throw "Invalid tabbrowser";
   if (iconBox && !(iconBox instanceof Ci.nsIDOMXULElement))
     throw "Invalid iconBox";
   if (!(panel instanceof Ci.nsIDOMXULElement))
     throw "Invalid panel";
 
+  this._shouldSuppress = options.shouldSuppress || (() => false);
+  this._suppress = this._shouldSuppress();
+
   this.window = tabbrowser.ownerDocument.defaultView;
   this.panel = panel;
   this.tabbrowser = tabbrowser;
   this.iconBox = iconBox;
   this.buttonDelay = Services.prefs.getIntPref(PREF_SECURITY_DELAY);
 
   this.panel.addEventListener("popuphidden", this, true);
   this.panel.classList.add("popup-notification-panel");
@@ -519,35 +531,45 @@ PopupNotifications.prototype = {
 
       this._fireCallback(notification, NOTIFICATION_EVENT_REMOVED);
       return false;
     }, this);
 
     this._setNotificationsForBrowser(aBrowser, notifications);
 
     if (this._isActiveBrowser(aBrowser)) {
-      // get the anchor element if the browser has defined one so it will
-      // _update will handle both the tabs iconBox and non-tab permission
-      // anchors.
-      this._update(notifications, this._getAnchorsForNotifications(notifications,
-        getAnchorFromBrowser(aBrowser)));
+      this.anchorVisibilityChange();
     }
   },
 
   /**
    * Called by the consumer to indicate that the visibility of the notification
-   * anchors may have changed, but the location has not changed. This may result
-   * in the "showing" and "shown" events for visible notifications to be
-   * invoked even if the anchor has not changed.
+   * anchors may have changed, but the location has not changed. This also
+   * checks whether all notifications are suppressed for this window.
+   *
+   * Calling this method may result in the "showing" and "shown" events for
+   * visible notifications to be invoked even if the anchor has not changed.
    */
   anchorVisibilityChange() {
-    let notifications =
-      this._getNotificationsForBrowser(this.tabbrowser.selectedBrowser);
-    this._update(notifications, this._getAnchorsForNotifications(notifications,
-      getAnchorFromBrowser(this.tabbrowser.selectedBrowser)));
+    let suppress = this._shouldSuppress();
+    if (!suppress) {
+      // If notifications are not suppressed, always update the visibility.
+      this._suppress = false;
+      let notifications =
+        this._getNotificationsForBrowser(this.tabbrowser.selectedBrowser);
+      this._update(notifications, this._getAnchorsForNotifications(notifications,
+        getAnchorFromBrowser(this.tabbrowser.selectedBrowser)));
+      return;
+    }
+
+    // Notifications are suppressed, ensure that the panel is hidden.
+    if (!this._suppress) {
+      this._suppress = true;
+      this._hidePanel().catch(Cu.reportError);
+    }
   },
 
   /**
    * Removes a Notification.
    * @param notification
    *        The Notification object to remove.
    */
   remove: function PopupNotifications_remove(notification) {
@@ -1009,20 +1031,22 @@ PopupNotifications.prototype = {
         if (anchor.parentNode == this.iconBox)
           continue;
         useIconBox = false;
         break;
       }
     }
 
     // Filter out notifications that have been dismissed, unless they are
-    // persistent.
-    let notificationsToShow = notifications.filter(function(n) {
-      return (!n.dismissed || n.options.persistent) && !n.options.neverShow;
-    });
+    // persistent. Also check if we should not show any notification.
+    let notificationsToShow = [];
+    if (!this._suppress) {
+      notificationsToShow = notifications.filter(
+        n => (!n.dismissed || n.options.persistent) && !n.options.neverShow);
+    }
 
     if (useIconBox) {
       // Hide icons of the previous tab.
       this._hideIcons();
     }
 
     if (haveNotifications) {
       // Also filter out notifications that are for a different anchor.
@@ -1278,28 +1302,33 @@ PopupNotifications.prototype = {
         return n.options.eventCallback.call(n, event, ...args);
     } catch (error) {
       Cu.reportError(error);
     }
     return undefined;
   },
 
   _onPopupHidden: function PopupNotifications_onPopupHidden(event) {
-    if (event.target != this.panel || this._ignoreDismissal) {
-      if (this._ignoreDismissal) {
-        this._ignoreDismissal.resolve();
-        this._ignoreDismissal = null;
-      }
+    if (event.target != this.panel) {
       return;
     }
 
-    // Ensure that when the panel comes up without user interaction,
-    // we don't autofocus it.
+    // We may have removed the "noautofocus" attribute before showing the panel
+    // if it was opened with user interaction. When the panel is closed, we have
+    // to restore the attribute to its default value, so we don't autofocus it
+    // if it is subsequently opened from a different code path.
     this.panel.setAttribute("noautofocus", "true");
 
+    // Handle the case where the panel was closed programmatically.
+    if (this._ignoreDismissal) {
+      this._ignoreDismissal.resolve();
+      this._ignoreDismissal = null;
+      return;
+    }
+
     this._dismissOrRemoveCurrentNotifications();
 
     this._clearPanel();
 
     this._update();
   },
 
   _dismissOrRemoveCurrentNotifications() {