Bug 1436462 - Use "Open in new private window" for bookmarks when in PBM. r=places-reviewers,mak

This makes the bookmark menu consistent with the "File" and context
menu when using private browsing mode.

We also share the same hide item logic for these "open" items in one
place in PlacesUIUtils so that they can be shared between the two
consumers (regular bookmarks and managed bookmarks). This ensures that
the "Open in container" item if hidden for managed bookmarks in a
private window.

Differential Revision: https://phabricator.services.mozilla.com/D220120

browser/components/places/PlacesUIUtils.sys.mjs

@@ -1514,6 +1514,40 @@ export var PlacesUIUtils = {
     }
   },
 
+  /**
+   * Determines whether the given "placesContext" menu item would open a link
+   * under some special conditions, but those special conditions cannot be met.
+   *
+   * @param {Element} item The menu or menu item to decide for.
+   *
+   * @returns {boolean} Whether the item is an "open" item that should be
+   *   hidden.
+   */
+  shouldHideOpenMenuItem(item) {
+    if (
+      item.hasAttribute("hide-if-disabled-private-browsing") &&
+      !lazy.PrivateBrowsingUtils.enabled
+    ) {
+      return true;
+    }
+
+    if (
+      item.hasAttribute("hide-if-private-browsing") &&
+      lazy.PrivateBrowsingUtils.isWindowPrivate(item.ownerGlobal)
+    ) {
+      return true;
+    }
+
+    if (
+      item.hasAttribute("hide-if-usercontext-disabled") &&
+      !Services.prefs.getBoolPref("privacy.userContext.enabled", false)
+    ) {
+      return true;
+    }
+
+    return false;
+  },
+
   async managedPlacesContextShowing(event) {
     let menupopup = event.target;
     let document = menupopup.ownerDocument;
@@ -1528,12 +1562,6 @@ export var PlacesUIUtils = {
         menupopup.triggerNode.menupopup
       );
     }
-    let linkItems = [
-      "placesContext_open:newtab",
-      "placesContext_open:newwindow",
-      "placesContext_openSeparator",
-      "placesContext_copy",
-    ];
     // Hide everything. We'll unhide the things we need.
     Array.from(menupopup.children).forEach(function (child) {
       child.hidden = true;
@@ -1555,12 +1583,18 @@ export var PlacesUIUtils = {
       openContainerInTabs_menuitem.disabled = !openContainerInTabs;
       openContainerInTabs_menuitem.hidden = false;
     } else {
-      linkItems.forEach(id => (document.getElementById(id).hidden = false));
-      document.getElementById("placesContext_open:newprivatewindow").hidden =
-        lazy.PrivateBrowsingUtils.isWindowPrivate(window) ||
-        !lazy.PrivateBrowsingUtils.enabled;
-      document.getElementById("placesContext_open:newcontainertab").hidden =
-        !Services.prefs.getBoolPref("privacy.userContext.enabled", false);
+      for (let id of [
+        "placesContext_open:newtab",
+        "placesContext_open:newcontainertab",
+        "placesContext_open:newwindow",
+        "placesContext_open:newprivatewindow",
+      ]) {
+        let item = document.getElementById(id);
+        item.hidden = this.shouldHideOpenMenuItem(item);
+      }
+      for (let id of ["placesContext_openSeparator", "placesContext_copy"]) {
+        document.getElementById(id).hidden = false;
+      }
     }
 
     event.target.ownerGlobal.updateCommands("places");

browser/components/places/content/controller.js

@@ -465,17 +465,7 @@ PlacesController.prototype = {
    *          and the item can be displayed, false otherwise.
    */
   _shouldShowMenuItem(aMenuItem, aMetaData) {
-    if (
-      aMenuItem.hasAttribute("hide-if-private-browsing") &&
-      !PrivateBrowsingUtils.enabled
-    ) {
-      return false;
-    }
-
-    if (
-      aMenuItem.hasAttribute("hide-if-usercontext-disabled") &&
-      !Services.prefs.getBoolPref("privacy.userContext.enabled", false)
-    ) {
+    if (PlacesUIUtils.shouldHideOpenMenuItem(aMenuItem)) {
       return false;
     }
 
@@ -566,8 +556,12 @@ PlacesController.prototype = {
    *     menuitem when there's no insertion point. An insertion point represents
    *     a point in the view where a new item can be inserted.
    *  9) The boolean `hide-if-private-browsing` attribute may be set to hide a
-   *     menuitem in private browsing mode
-   * 10) The boolean `hide-if-single-click-opens` attribute may be set to hide a
+   *     menuitem in private browsing mode.
+   * 10) The boolean `hide-if-disabled-private-browsing` attribute may be set to
+   *     hide a menuitem if private browsing is not enabled.
+   * 11) The boolean `hide-if-usercontext-disabled` attribute may be set to
+   *     hide a menuitem if containers are disabled.
+   * 12) The boolean `hide-if-single-click-opens` attribute may be set to hide a
    *     menuitem in views opening entries with a single click.
    *
    * @param {object} aPopup
@@ -593,9 +587,6 @@ PlacesController.prototype = {
           item.getAttribute("hide-if-no-insertion-point") == "true" &&
           noIp &&
           !(ip && ip.isTag && item.id == "placesContext_paste");
-        let hideIfPrivate =
-          item.getAttribute("hide-if-private-browsing") == "true" &&
-          PrivateBrowsingUtils.isWindowPrivate(window);
         // Hide `Open` if the primary action on click is opening.
         let hideIfSingleClickOpens =
           item.getAttribute("hide-if-single-click-opens") == "true" &&
@@ -610,7 +601,6 @@ PlacesController.prototype = {
 
         let shouldHideItem =
           hideIfNoIP ||
-          hideIfPrivate ||
           hideIfSingleClickOpens ||
           hideIfNotSearch ||
           !this._shouldShowMenuItem(item, metadata);

browser/components/places/content/placesContextMenu.inc.xhtml

@@ -52,13 +52,14 @@
             command="placesCmd_open:window"
             data-l10n-id="places-open-in-window"
             selection-type="single"
-            node-type="link"/>
+            node-type="link"
+            hide-if-private-browsing="true"/>
   <menuitem id="placesContext_open:newprivatewindow"
             command="placesCmd_open:privatewindow"
             data-l10n-id="places-open-in-private-window"
             selection-type="single"
             node-type="link"
-            hide-if-private-browsing="true"/>
+            hide-if-disabled-private-browsing="true"/>
   <menuitem id="placesContext_showInFolder"
             data-l10n-id="places-show-in-folder"
             command="placesCmd_showInFolder"

browser/components/places/tests/browser/browser_bookmark_context_menu_contents.js

@@ -814,3 +814,118 @@ add_task(async function test_library_noselection_contextmenu_contents() {
     );
   });
 });
+
+add_task(async function test_private_browsing_window() {
+  // Test the context menu when in a private browsing window.
+
+  let win = await BrowserTestUtils.openNewBrowserWindow({
+    private: true,
+  });
+
+  let optionItems = [
+    "placesContext_open:newtab",
+    // Hidden in private window "placesContext_open:newcontainertab"
+    // Hidden in private window "placesContext_open:newwindow"
+    "placesContext_open:newprivatewindow",
+    "placesContext_show_bookmark:info",
+    "placesContext_deleteBookmark",
+    "placesContext_cut",
+    "placesContext_copy",
+    "placesContext_paste_group",
+    "placesContext_new:bookmark",
+    "placesContext_new:folder",
+    "placesContext_new:separator",
+  ];
+
+  // Test toolbar.
+  await checkContextMenu(
+    async function () {
+      let toolbarBookmark = await PlacesUtils.bookmarks.insert({
+        parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+        title: "Bookmark Title",
+        url: TEST_URL,
+      });
+
+      let toolbarNode = getToolbarNodeForItemGuid(toolbarBookmark.guid, win);
+
+      let contextMenu = win.document.getElementById("placesContext");
+      let popupShownPromise = BrowserTestUtils.waitForEvent(
+        contextMenu,
+        "popupshown"
+      );
+
+      EventUtils.synthesizeMouseAtCenter(
+        toolbarNode,
+        { button: 2, type: "contextmenu" },
+        win
+      );
+      await popupShownPromise;
+      return contextMenu;
+    },
+    [
+      ...optionItems,
+      "placesContext_showAllBookmarks",
+      "toggle_PersonalToolbar",
+      "show-other-bookmarks_PersonalToolbar",
+    ],
+    win.document
+  );
+
+  // Test side bar.
+  await withSidebarTree(
+    "bookmarks",
+    async tree => {
+      await checkContextMenu(
+        async bookmark => {
+          tree.selectItems([bookmark.guid]);
+
+          let contextMenu =
+            win.SidebarController.browser.contentDocument.getElementById(
+              "placesContext"
+            );
+          let popupShownPromise = BrowserTestUtils.waitForEvent(
+            contextMenu,
+            "popupshown"
+          );
+          synthesizeClickOnSelectedTreeCell(tree, { type: "contextmenu" }, win);
+          await popupShownPromise;
+          return contextMenu;
+        },
+        optionItems,
+        win.SidebarController.browser.contentDocument
+      );
+    },
+    win
+  );
+
+  // Test library window opened when using private browsing window.
+  optionItems.splice(
+    optionItems.indexOf("placesContext_show_bookmark:info"),
+    1
+  );
+  optionItems.splice(0, 0, "placesContext_open");
+
+  await withLibraryWindow(
+    "BookmarksToolbar",
+    async ({ right }) => {
+      await checkContextMenu(
+        async bookmark => {
+          let contextMenu = right.ownerDocument.getElementById("placesContext");
+          let popupShownPromise = BrowserTestUtils.waitForEvent(
+            contextMenu,
+            "popupshown"
+          );
+          right.selectItems([bookmark.guid]);
+          synthesizeClickOnSelectedTreeCell(right, { type: "contextmenu" });
+          await popupShownPromise;
+          return contextMenu;
+        },
+        optionItems,
+        right.ownerDocument
+      );
+    },
+    win
+  );
+
+  await BrowserTestUtils.closeWindow(win);
+});

browser/components/places/tests/browser/head.js

@@ -6,8 +6,8 @@ ChromeUtils.defineLazyGetter(this, "gFluentStrings", function () {
   return new Localization(["branding/brand.ftl", "browser/browser.ftl"], true);
 });
 
-function openLibrary(callback, aLeftPaneRoot) {
-  let library = window.openDialog(
+function openLibrary(callback, aLeftPaneRoot, win = window) {
+  let library = win.openDialog(
     "chrome://browser/content/places/places.xhtml",
     "",
     "chrome,toolbar=yes,dialog=no,resizable",
@@ -27,10 +27,12 @@ function openLibrary(callback, aLeftPaneRoot) {
  *
  * @param {object} aLeftPaneRoot
  *        Hierarchy to open and select in the left pane.
+ * @param {Window} [win]
+ *        The window to use to open the library.
  * @returns {Promise}
  *          Resolves to the handle to the library window.
  */
-function promiseLibrary(aLeftPaneRoot) {
+function promiseLibrary(aLeftPaneRoot, win = window) {
   return new Promise(resolve => {
     let library = Services.wm.getMostRecentWindow("Places:Organizer");
     if (library && !library.closed) {
@@ -42,7 +44,7 @@ function promiseLibrary(aLeftPaneRoot) {
       checkLibraryPaneVisibility(library, aLeftPaneRoot);
       resolve(library);
     } else {
-      openLibrary(resolve, aLeftPaneRoot);
+      openLibrary(resolve, aLeftPaneRoot, win);
     }
   });
 }
@@ -380,9 +382,11 @@ function fillBookmarkTextField(id, text, win, blur = true) {
  * @param {Function} taskFn
  *        The task to execute once the sidebar is ready. Will get the Places
  *        tree view as input.
+ * @param {Window} [win]
+ *        The window to open the sidebar in, else uses the default window.
  */
-var withSidebarTree = async function (type, taskFn) {
-  let sidebar = document.getElementById("sidebar");
+var withSidebarTree = async function (type, taskFn, win = window) {
+  let sidebar = win.document.getElementById("sidebar");
   info("withSidebarTree: waiting sidebar load");
   let sidebarLoadedPromise = new Promise(resolve => {
     sidebar.addEventListener(
@@ -395,7 +399,7 @@ var withSidebarTree = async function (type, taskFn) {
   });
   let sidebarId =
     type == "bookmarks" ? "viewBookmarksSidebar" : "viewHistorySidebar";
-  SidebarController.show(sidebarId);
+  win.SidebarController.show(sidebarId);
   await sidebarLoadedPromise;
 
   let treeId = type == "bookmarks" ? "bookmarks-view" : "historyTree";
@@ -406,7 +410,7 @@ var withSidebarTree = async function (type, taskFn) {
   try {
     await taskFn(tree);
   } finally {
-    SidebarController.hide();
+    win.SidebarController.hide();
   }
 };
 
@@ -419,9 +423,11 @@ var withSidebarTree = async function (type, taskFn) {
  * @param {Function} taskFn
  *        The task to execute once the Library is ready.
  *        Will get { left, right } trees as argument.
+ * @param {Window} [win]
+ *        The window to use to open the library.
  */
-var withLibraryWindow = async function (hierarchy, taskFn) {
-  let library = await promiseLibrary(hierarchy);
+var withLibraryWindow = async function (hierarchy, taskFn, win = window) {
+  let library = await promiseLibrary(hierarchy, win);
   let left = library.document.getElementById("placesList");
   let right = library.document.getElementById("placeContent");
   info("withLibrary: executing the task");
@@ -477,8 +483,8 @@ function promisePopupHidden(popup) {
 }
 
 // Identify a bookmark node in the Bookmarks Toolbar by its guid.
-function getToolbarNodeForItemGuid(itemGuid) {
-  let children = document.getElementById("PlacesToolbarItems").childNodes;
+function getToolbarNodeForItemGuid(itemGuid, win = window) {
+  let children = win.document.getElementById("PlacesToolbarItems").childNodes;
   for (let child of children) {
     if (itemGuid === child._placesNode.bookmarkGuid) {
       return child;