Bug 1402721 - Add/edit bookmark panel should open anchored on the page action (ellipsis) button instead of on the identity block (left side of the address bar) when the bookmarks star action is not pinned/visible. r?Gijs draft
authorDrew Willcoxon <adw@mozilla.com>
Mon, 25 Sep 2017 13:30:08 -0700
changeset 670096 eee8ec20dd30365846a0491fd28a2937974447c5
parent 669742 5f3f19824efa14cc6db546baf59c54a0fc15ddc9
child 733128 2c31bdff5ba51994321c8cba706df5d54dfba154
push id81512
push userdwillcoxon@mozilla.com
push dateMon, 25 Sep 2017 20:30:24 +0000
reviewersGijs
bugs1402721
milestone58.0a1
Bug 1402721 - Add/edit bookmark panel should open anchored on the page action (ellipsis) button instead of on the identity block (left side of the address bar) when the bookmarks star action is not pinned/visible. r?Gijs MozReview-Commit-ID: JMviXx5ov7F
browser/base/content/browser-pageActions.js
browser/base/content/browser-places.js
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -257,38 +257,48 @@ var BrowserPageActions = {
 
     if (iframeNode) {
       action.onIframeShown(iframeNode, panelNode);
     }
 
     return panelNode;
   },
 
+  /**
+   * Returns the node in the urlbar to which popups for the given action should
+   * be anchored.  If the action is null, a sensible anchor is returned.
+   *
+   * @param  action (PageActions.Action, optional)
+   *         The action you want to anchor.
+   * @return (DOM node, nonnull) The node to which the action should be
+   *         anchored.
+   */
   panelAnchorNodeForAction(action) {
     // Try each of the following nodes in order, using the first that's visible.
     let potentialAnchorNodeIDs = [
-      action.anchorIDOverride || null,
-      this._urlbarButtonNodeIDForActionID(action.id),
+      action && action.anchorIDOverride,
+      action && this._urlbarButtonNodeIDForActionID(action.id),
       this.mainButtonNode.id,
       "identity-icon",
     ];
     let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
                     .getInterface(Ci.nsIDOMWindowUtils);
     for (let id of potentialAnchorNodeIDs) {
       if (id) {
         let node = document.getElementById(id);
         if (node && !node.hidden) {
           let bounds = dwu.getBoundsWithoutFlushing(node);
           if (bounds.height > 0 && bounds.width > 0) {
             return node;
           }
         }
       }
     }
-    throw new Error(`PageActions: No anchor node for '${action.id}'`);
+    let id = action ? action.id : "<no action>";
+    throw new Error(`PageActions: No anchor node for ${id}`);
   },
 
   get activatedActionPanelNode() {
     return document.getElementById(this._activatedActionPanelID);
   },
 
   get _activatedActionPanelID() {
     return "pageActionActivatedActionPanel";
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -297,31 +297,23 @@ var StarUI = {
     let forms = gNavigatorBundle.getString("editBookmark.removeBookmarks.label");
     let bookmarksCount = this._itemGuids.length;
     let label = PluralForm.get(bookmarksCount, forms)
                           .replace("#1", bookmarksCount);
     this._element("editBookmarkPanelRemoveButton").label = label;
 
     this.beginBatch();
 
-    if (aAnchorElement) {
-      // Set the open=true attribute if the anchor is a
-      // descendent of a toolbarbutton.
-      let parent = aAnchorElement.parentNode;
-      while (parent) {
-        if (parent.localName == "toolbarbutton") {
-          break;
-        }
-        parent = parent.parentNode;
-      }
-      if (parent) {
-        this._anchorToolbarButton = parent;
-        parent.setAttribute("open", "true");
-      }
+    if (aAnchorElement && aAnchorElement.closest("#urlbar")) {
+      this._anchorToolbarButton = aAnchorElement;
+      aAnchorElement.setAttribute("open", "true");
+    } else {
+      this._anchorToolbarButton = null;
     }
+
     let onPanelReady = fn => {
       let target = this.panel;
       if (target.parentNode) {
         // By targeting the panel's parent and using a capturing listener, we
         // can have our listener called before others waiting for the panel to
         // be shown (which probably expect the panel to be fully initialized)
         target = target.parentNode;
       }
@@ -329,23 +321,16 @@ var StarUI = {
         fn();
       }, {"capture": true, "once": true});
     };
     gEditItemOverlay.initPanel({ node: aNode,
                                  onPanelReady,
                                  hiddenRows: ["description", "location",
                                               "loadInSidebar", "keyword"],
                                  focusedElement: "preferred"});
-
-    if (aAnchorElement && aAnchorElement.id == BookmarkingUI.STAR_BOX_ID) {
-      aAnchorElement.setAttribute("open", "true");
-      this.panel.addEventListener("popuphiding", () => {
-        aAnchorElement.removeAttribute("open");
-      });
-    }
     this.panel.openPopup(aAnchorElement, aPosition);
   },
 
   panelShown:
   function SU_panelShown(aEvent) {
     if (aEvent.target == this.panel) {
       if (this._element("editBookmarkPanelContent").hidden) {
         // Note this isn't actually used anymore, we should remove this
@@ -1366,28 +1351,18 @@ var BookmarkingUI = {
   },
 
   get starBox() {
     delete this.starBox;
     return this.starBox = document.getElementById(this.STAR_BOX_ID);
   },
 
   get anchor() {
-    // Try to anchor the panel to:
-    // 1. The bookmarks star box (using the star itself is trickier because it
-    //    can be hidden while the star animation is visible)
-    // 2. The identity icon
-    if (this.starBox && isVisible(this.starBox)) {
-      return this.starBox;
-    }
-    let identityIcon = document.getElementById("identity-icon");
-    if (identityIcon && isVisible(identityIcon)) {
-      return identityIcon;
-    }
-    return null;
+    let action = PageActions.actionForID(PageActions.ACTION_ID_BOOKMARK);
+    return BrowserPageActions.panelAnchorNodeForAction(action);
   },
 
   get notifier() {
     delete this.notifier;
     return this.notifier = document.getElementById("bookmarked-notification-anchor");
   },
 
   get dropmarkerNotifier() {