Bug 1363188 - Add an add/remove context menu to page actions in the urlbar. r?Gijs draft
authorDrew Willcoxon <adw@mozilla.com>
Wed, 02 Aug 2017 09:31:07 -0700
changeset 619798 b052639e3dc130396c15e56d7d2b5d9b1902601c
parent 619442 fec8d72590053c3ad72cd3492d389213dfabc2ff
child 641000 f581371357a3972acdbfc3a71cb4cd006b08d45a
push id71813
push userdwillcoxon@mozilla.com
push dateWed, 02 Aug 2017 16:31:27 +0000
reviewersGijs
bugs1363188
milestone56.0a1
Bug 1363188 - Add an add/remove context menu to page actions in the urlbar. r?Gijs MozReview-Commit-ID: 5rJWvID5OPd
browser/base/content/browser-pageActions.js
browser/base/content/browser.xul
browser/base/content/test/urlbar/browser_page_action_menu.js
browser/base/content/test/urlbar/head.js
browser/modules/test/browser/browser_PageActions.js
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -294,16 +294,20 @@ var BrowserPageActions = {
     let buttonNode = document.createElement("image");
     buttonNode.classList.add("urlbar-icon");
     if (action.tooltip) {
       buttonNode.setAttribute("tooltiptext", action.tooltip);
     }
     if (action.iconURL) {
       buttonNode.style.listStyleImage = `url('${action.iconURL}')`;
     }
+    buttonNode.setAttribute("context", "pageActionPanelContextMenu");
+    buttonNode.addEventListener("contextmenu", event => {
+      BrowserPageActions.onContextMenu(event);
+    });
     if (action.nodeAttributes) {
       for (let name in action.nodeAttributes) {
         buttonNode.setAttribute(name, action.nodeAttributes[name]);
       }
     }
     buttonNode.addEventListener("click", event => {
       if (event.button != 0) {
         return;
@@ -415,17 +419,32 @@ var BrowserPageActions = {
    *         or the urlbar.
    * @return (PageAction.Action) The node's related action, or null if none.
    */
   actionForNode(node) {
     if (!node) {
       return null;
     }
     let actionID = this._actionIDForNodeID(node.id);
-    return PageActions.actionForID(actionID);
+    let action = PageActions.actionForID(actionID);
+    if (!action) {
+      // The given node may be an ancestor of a node corresponding to an action,
+      // like how #star-button is contained in #star-button-box, the latter
+      // being the bookmark action's node.  Look up the ancestor chain.
+      for (let n = node.parentNode; n && !action; n = n.parentNode) {
+        if (n.id == "urlbar-icons" || n.localName == "panelview") {
+          // We reached the urlbar icons container or the panelview container.
+          // Stop looking; no acton was found.
+          break;
+        }
+        actionID = this._actionIDForNodeID(n.id);
+        action = PageActions.actionForID(actionID);
+      }
+    }
+    return action;
   },
 
   // The ID of the given action's top-level button in the panel.
   _panelButtonNodeIDForActionID(actionID) {
     return `pageAction-panel-${actionID}`;
   },
 
   // The ID of the given action's button in the urlbar.
@@ -451,17 +470,26 @@ var BrowserPageActions = {
 
   // The ID of the action corresponding to the given top-level button in the
   // panel or button in the urlbar.
   _actionIDForNodeID(nodeID) {
     if (!nodeID) {
       return null;
     }
     let match = nodeID.match(/^pageAction-(?:panel|urlbar)-(.+)$/);
-    return match ? match[1] : null;
+    if (match) {
+      return match[1];
+    }
+    // Check all the urlbar ID overrides.
+    for (let action of PageActions.actions) {
+      if (action.urlbarIDOverride && action.urlbarIDOverride == nodeID) {
+        return action.id;
+      }
+    }
+    return null;
   },
 
   /**
    * Call this when the main page action button in the urlbar is activated.
    *
    * @param  event (DOM event, required)
    *         The click or whatever event.
    */
@@ -587,16 +615,23 @@ BrowserPageActions.bookmark = {
     // Update the button label via the bookmark observer.
     BookmarkingUI.updateBookmarkPageMenuItem();
   },
 
   onCommand(event, buttonNode) {
     BrowserPageActions.panelNode.hidePopup();
     BookmarkingUI.onStarCommand(event);
   },
+
+  onUrlbarNodeClicked(event) {
+    if (event.type == "click" && event.button != 0) {
+      return;
+    }
+    BookmarkingUI.onStarCommand(event);
+  },
 };
 
 // copy URL
 BrowserPageActions.copyURL = {
   onPlacedInPanel(buttonNode) {
     BrowserPageActions.takeNodeAttributeFromPanel(buttonNode, "title");
   },
 
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -896,26 +896,27 @@
                        onclick="FullZoom.reset();"
                        tooltip="dynamic-shortcut-tooltip"
                        hidden="true"/>
 #ifdef MOZ_PHOTON_THEME
                 <image id="pageActionButton"
                        class="urlbar-icon"
                        tooltiptext="&pageActionButton.tooltip;"
                        onclick="BrowserPageActions.mainButtonClicked(event);"/>
-                <hbox id="star-button-box" hidden="true">
-                  <image id="star-button"
-                         class="urlbar-icon"
-                         onclick="BookmarkingUI.onStarCommand(event);">
+                <hbox id="star-button-box"
+                      hidden="true"
+                      context="pageActionPanelContextMenu"
+                      oncontextmenu="BrowserPageActions.onContextMenu(event);"
+                      onclick="BrowserPageActions.bookmark.onUrlbarNodeClicked(event);">
+                  <image id="star-button" class="urlbar-icon">
                     <observes element="bookmarkThisPageBroadcaster" attribute="starred"/>
                     <observes element="bookmarkThisPageBroadcaster" attribute="tooltiptext"/>
                   </image>
                   <hbox id="star-button-animatable-box">
-                    <image id="star-button-animatable-image"
-                           onclick="BookmarkingUI.onStarCommand(event);"/>
+                    <image id="star-button-animatable-image"/>
                   </hbox>
                 </hbox>
 #endif
               </hbox>
               <hbox id="userContext-icons" hidden="true">
                 <label id="userContext-label"/>
                 <image id="userContext-indicator"/>
               </hbox>
--- a/browser/base/content/test/urlbar/browser_page_action_menu.js
+++ b/browser/base/content/test/urlbar/browser_page_action_menu.js
@@ -77,17 +77,17 @@ add_task(async function bookmark() {
     await promisePageActionPanelOpen();
 
     // The bookmark button should read "Bookmark This Page" and not be starred.
     Assert.equal(bookmarkButton.label, "Bookmark This Page");
     Assert.ok(!bookmarkButton.hasAttribute("starred"));
 
     // Done.
     hiddenPromise = promisePageActionPanelHidden();
-    gPageActionPanel.hidePopup();
+    BrowserPageActions.panelNode.hidePopup();
     await hiddenPromise;
   });
 });
 
 add_task(async function emailLink() {
   // Open an actionable page so that the main page action button appears.  (It
   // does not appear on about:blank for example.)
   let url = "http://example.com/";
@@ -120,17 +120,17 @@ add_task(async function sendToDevice_non
   await BrowserTestUtils.withNewTab("about:home", async () => {
     await promiseSyncReady();
     // Open the panel.  Send to Device should be disabled.
     await promisePageActionPanelOpen();
     let sendToDeviceButton =
       document.getElementById("pageAction-panel-sendToDevice");
     Assert.ok(sendToDeviceButton.disabled);
     let hiddenPromise = promisePageActionPanelHidden();
-    gPageActionPanel.hidePopup();
+    BrowserPageActions.panelNode.hidePopup();
     await hiddenPromise;
   });
 });
 
 add_task(async function sendToDevice_syncNotReady_other_states() {
   // Open a tab that's sendable.
   await BrowserTestUtils.withNewTab("http://example.com/", async () => {
     await promiseSyncReady();
@@ -175,17 +175,17 @@ add_task(async function sendToDevice_syn
           label: "Verify Your Account...",
         },
       }
     ];
     checkSendToDeviceItems(expectedItems);
 
     // Done, hide the panel.
     let hiddenPromise = promisePageActionPanelHidden();
-    gPageActionPanel.hidePopup();
+    BrowserPageActions.panelNode.hidePopup();
     await hiddenPromise;
 
     cleanUp();
   });
 });
 
 add_task(async function sendToDevice_syncNotReady_configured() {
   // Open a tab that's sendable.
@@ -265,17 +265,17 @@ add_task(async function sendToDevice_syn
         checkSendToDeviceItems(expectedItems);
       } else {
         ok(false, "This should never happen");
       }
     }
 
     // Done, hide the panel.
     let hiddenPromise = promisePageActionPanelHidden();
-    gPageActionPanel.hidePopup();
+    BrowserPageActions.panelNode.hidePopup();
     await hiddenPromise;
     cleanUp();
   });
 });
 
 add_task(async function sendToDevice_notSignedIn() {
   // Open a tab that's sendable.
   await BrowserTestUtils.withNewTab("http://example.com/", async () => {
@@ -311,17 +311,17 @@ add_task(async function sendToDevice_not
           label: "Learn About Sending Tabs..."
         },
       }
     ];
     checkSendToDeviceItems(expectedItems);
 
     // Done, hide the panel.
     let hiddenPromise = promisePageActionPanelHidden();
-    gPageActionPanel.hidePopup();
+    BrowserPageActions.panelNode.hidePopup();
     await hiddenPromise;
   });
 });
 
 add_task(async function sendToDevice_noDevices() {
   // Open a tab that's sendable.
   await BrowserTestUtils.withNewTab("http://example.com/", async () => {
     await promiseSyncReady();
@@ -367,17 +367,17 @@ add_task(async function sendToDevice_noD
           label: "Learn About Sending Tabs..."
         }
       }
     ];
     checkSendToDeviceItems(expectedItems);
 
     // Done, hide the panel.
     let hiddenPromise = promisePageActionPanelHidden();
-    gPageActionPanel.hidePopup();
+    BrowserPageActions.panelNode.hidePopup();
     await hiddenPromise;
 
     cleanUp();
 
     await UIState.reset();
   });
 });
 
@@ -433,23 +433,128 @@ add_task(async function sendToDevice_dev
           label: "Send to All Devices"
         }
       }
     );
     checkSendToDeviceItems(expectedItems);
 
     // Done, hide the panel.
     let hiddenPromise = promisePageActionPanelHidden();
-    gPageActionPanel.hidePopup();
+    BrowserPageActions.panelNode.hidePopup();
     await hiddenPromise;
 
     cleanUp();
   });
 });
 
+add_task(async function contextMenu() {
+  // Open an actionable page so that the main page action button appears.
+  let url = "http://example.com/";
+  await BrowserTestUtils.withNewTab(url, async () => {
+    // Open the panel and then open the context menu on the bookmark button.
+    await promisePageActionPanelOpen();
+    let bookmarkButton = document.getElementById("pageAction-panel-bookmark");
+    let contextMenuPromise = promisePanelShown("pageActionPanelContextMenu");
+    EventUtils.synthesizeMouseAtCenter(bookmarkButton, {
+      type: "contextmenu",
+      button: 2,
+    });
+    await contextMenuPromise;
+
+    // The context menu should show "Remove from Address Bar".  Click it.
+    let contextMenuNode = document.getElementById("pageActionPanelContextMenu");
+    Assert.equal(contextMenuNode.childNodes.length, 1,
+                 "Context menu has one child");
+    Assert.equal(contextMenuNode.childNodes[0].label, "Remove from Address Bar",
+                 "Context menu is in the 'remove' state");
+    contextMenuPromise = promisePanelHidden("pageActionPanelContextMenu");
+    EventUtils.synthesizeMouseAtCenter(contextMenuNode.childNodes[0], {});
+    await contextMenuPromise;
+
+    // The action should be removed from the urlbar.  In this case, the bookmark
+    // star, the node in the urlbar should be hidden.
+    let starButtonBox = document.getElementById("star-button-box");
+    await BrowserTestUtils.waitForCondition(() => {
+      return starButtonBox.hidden;
+    }, "Waiting for star button to become hidden");
+
+    // Open the context menu again on the bookmark button.  (The page action
+    // panel remains open.)
+    contextMenuPromise = promisePanelShown("pageActionPanelContextMenu");
+    EventUtils.synthesizeMouseAtCenter(bookmarkButton, {
+      type: "contextmenu",
+      button: 2,
+    });
+    await contextMenuPromise;
+
+    // The context menu should show "Add to Address Bar".  Click it.
+    Assert.equal(contextMenuNode.childNodes.length, 1,
+                 "Context menu has one child");
+    Assert.equal(contextMenuNode.childNodes[0].label, "Add to Address Bar",
+                 "Context menu is in the 'add' state");
+    contextMenuPromise = promisePanelHidden("pageActionPanelContextMenu");
+    EventUtils.synthesizeMouseAtCenter(contextMenuNode.childNodes[0], {});
+    await contextMenuPromise;
+
+    // The action should be added to the urlbar.
+    await BrowserTestUtils.waitForCondition(() => {
+      return !starButtonBox.hidden;
+    }, "Waiting for star button to become unhidden");
+
+    // Open the context menu on the bookmark star in the urlbar.
+    contextMenuPromise = promisePanelShown("pageActionPanelContextMenu");
+    EventUtils.synthesizeMouseAtCenter(starButtonBox, {
+      type: "contextmenu",
+      button: 2,
+    });
+    await contextMenuPromise;
+
+    // The context menu should show "Remove from Address Bar".  Click it.
+    Assert.equal(contextMenuNode.childNodes.length, 1,
+                 "Context menu has one child");
+    Assert.equal(contextMenuNode.childNodes[0].label, "Remove from Address Bar",
+                 "Context menu is in the 'remove' state");
+    contextMenuPromise = promisePanelHidden("pageActionPanelContextMenu");
+    EventUtils.synthesizeMouseAtCenter(contextMenuNode.childNodes[0], {});
+    await contextMenuPromise;
+
+    // The action should be removed from the urlbar.
+    await BrowserTestUtils.waitForCondition(() => {
+      return starButtonBox.hidden;
+    }, "Waiting for star button to become hidden");
+
+    // Finally, add the bookmark star back to the urlbar so that other tests
+    // that rely on it are OK.
+    await promisePageActionPanelOpen();
+    contextMenuPromise = promisePanelShown("pageActionPanelContextMenu");
+    EventUtils.synthesizeMouseAtCenter(bookmarkButton, {
+      type: "contextmenu",
+      button: 2,
+    });
+    await contextMenuPromise;
+    Assert.equal(contextMenuNode.childNodes.length, 1,
+                 "Context menu has one child");
+    Assert.equal(contextMenuNode.childNodes[0].label, "Add to Address Bar",
+                 "Context menu is in the 'add' state");
+    contextMenuPromise = promisePanelHidden("pageActionPanelContextMenu");
+    EventUtils.synthesizeMouseAtCenter(contextMenuNode.childNodes[0], {});
+    await contextMenuPromise;
+    await BrowserTestUtils.waitForCondition(() => {
+      return !starButtonBox.hidden;
+    }, "Waiting for star button to become unhidden");
+  });
+
+  // urlbar tests that run after this one can break if the mouse is left over
+  // the area where the urlbar popup appears, which seems to happen due to the
+  // above synthesized mouse events.  Move it over the urlbar.
+  EventUtils.synthesizeMouseAtCenter(gURLBar, { type: "mousemove" });
+  gURLBar.focus();
+});
+
+
 function promiseSyncReady() {
   let service = Cc["@mozilla.org/weave/service;1"]
                   .getService(Components.interfaces.nsISupports)
                   .wrappedJSObject;
   return service.whenLoaded().then(() => {
     UIState.isReady();
     return UIState.refresh();
   });
--- a/browser/base/content/test/urlbar/head.js
+++ b/browser/base/content/test/urlbar/head.js
@@ -195,44 +195,60 @@ function promiseNewSearchEngine(basename
       onError(errCode) {
         Assert.ok(false, "addEngine failed with error code " + errCode);
         reject();
       },
     });
   });
 }
 
-let gPageActionPanel = document.getElementById("pageActionPanel");
-
 function promisePageActionPanelOpen() {
-  let button = document.getElementById("pageActionButton");
+  if (BrowserPageActions.panelNode.state == "open") {
+    return Promise.resolve();
+  }
   let shownPromise = promisePageActionPanelShown();
-  EventUtils.synthesizeMouseAtCenter(button, {});
+  EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
   return shownPromise;
 }
 
 function promisePageActionPanelShown() {
-  return promisePageActionPanelEvent("popupshown");
+  return promisePanelShown(BrowserPageActions.panelNode);
 }
 
 function promisePageActionPanelHidden() {
-  return promisePageActionPanelEvent("popuphidden");
+  return promisePanelHidden(BrowserPageActions.panelNode);
+}
+
+function promisePanelShown(panelIDOrNode) {
+  return promisePanelEvent(panelIDOrNode, "popupshown");
+}
+
+function promisePanelHidden(panelIDOrNode) {
+  return promisePanelEvent(panelIDOrNode, "popuphidden");
 }
 
-function promisePageActionPanelEvent(name) {
+function promisePanelEvent(panelIDOrNode, eventType) {
   return new Promise(resolve => {
-    gPageActionPanel.addEventListener(name, () => {
+    let panel = typeof(panelIDOrNode) != "string" ? panelIDOrNode :
+                document.getElementById(panelIDOrNode);
+    if (!panel ||
+        (eventType == "popupshown" && panel.state == "open") ||
+        (eventType == "popuphidden" && panel.state == "closed")) {
+      executeSoon(resolve);
+      return;
+    }
+    panel.addEventListener(eventType, () => {
       executeSoon(resolve);
     }, { once: true });
   });
 }
 
 function promisePageActionViewShown() {
   return new Promise(resolve => {
-    gPageActionPanel.addEventListener("ViewShown", (event) => {
+    BrowserPageActions.panelNode.addEventListener("ViewShown", (event) => {
       let target = event.originalTarget;
       window.setTimeout(() => {
         resolve(target);
       }, 5000);
     }, { once: true });
   });
 }
 
--- a/browser/modules/test/browser/browser_PageActions.js
+++ b/browser/modules/test/browser/browser_PageActions.js
@@ -675,17 +675,17 @@ function promisePanelHidden(panelIDOrNod
   return promisePanelEvent(panelIDOrNode, "popuphidden");
 }
 
 function promisePanelEvent(panelIDOrNode, eventType) {
   return new Promise(resolve => {
     let panel = typeof(panelIDOrNode) != "string" ? panelIDOrNode :
                 document.getElementById(panelIDOrNode);
     if (!panel ||
-        (eventType == "popupshowing" && panel.state == "open") ||
+        (eventType == "popupshown" && panel.state == "open") ||
         (eventType == "popuphidden" && panel.state == "closed")) {
       executeSoon(resolve);
       return;
     }
     panel.addEventListener(eventType, () => {
       executeSoon(resolve);
     }, { once: true });
   });