Bug 1703889 - Ensure hidden extension pageAction are not visible in the urlbar overflow menu on Proton. r=mixedpuppy,mak
authorLuca Greco <lgreco@mozilla.com>
Mon, 12 Apr 2021 12:06:59 +0000
changeset 575455 f54c679ded81eeebe05fa63d48be29a556118ac5
parent 575454 e3a7a6111dfe2de8fcf5a3cbedf5ea3e4834ead4
child 575456 2e13cdc47fb91949c48627656604bbce3b632847
push id140727
push userluca.greco@alcacoop.it
push dateMon, 12 Apr 2021 13:44:50 +0000
treeherderautoland@2e13cdc47fb9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmixedpuppy, mak
bugs1703889
milestone89.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1703889 - Ensure hidden extension pageAction are not visible in the urlbar overflow menu on Proton. r=mixedpuppy,mak Differential Revision: https://phabricator.services.mozilla.com/D111423
browser/base/content/browser-pageActions.js
browser/base/content/test/pageActions-proton/browser_PageActions_overflow.js
browser/components/extensions/test/browser/browser_ext_menus.js
browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js
browser/modules/PageActions.jsm
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -137,16 +137,28 @@ var BrowserPageActions = {
    *
    * @param  action (PageActions.Action, required)
    *         The action to place.
    */
   placeActionInPanel(action) {
     if (this._panelNode && this.panelNode.state != "closed") {
       this._placeActionInPanelNow(action);
     } else {
+      // This method may be called for the same action more than once
+      // (e.g. when an extension does call pageAction.show/hidden to
+      // enable or disable its own pageAction and we will have to
+      // update the urlbar overflow panel accordingly).
+      //
+      // Ensure we don't add the same actions more than once (otherwise we will
+      // not remove all the entries in _removeActionFromPanel).
+      if (
+        this._actionsToLazilyPlaceInPanel.findIndex(a => a.id == action.id) >= 0
+      ) {
+        return;
+      }
       // Lazily place the action in the panel the next time it opens.
       this._actionsToLazilyPlaceInPanel.push(action);
     }
   },
 
   _placeActionInPanelNow(action) {
     if (action.shouldShowInPanel(window)) {
       this._addActionToPanel(action);
@@ -611,17 +623,27 @@ var BrowserPageActions = {
   },
 
   _updateActionDisabled(
     action,
     panelNode,
     urlbarNode,
     disabled = action.getDisabled(window)
   ) {
-    if (action.__transient) {
+    // When Proton is enabled, the extension page actions should behave similarly
+    // to a transient action, and be hidden from the urlbar overflow menu if they
+    // are disabled (as in the urlbar when the overflow menu isn't available)
+    //
+    // TODO(Bug 1704139): as a follow up we may look into just set on all
+    // extension pageActions `_transient: true`, at least once we sunset
+    // the proton preference and we don't need the pre-Proton behavior anymore,
+    // and remove this special case.
+    const isProtonExtensionAction = action.extensionID && gProton;
+
+    if (action.__transient || isProtonExtensionAction) {
       this.placeActionInPanel(action);
     } else {
       this._updateActionDisabledInPanel(action, panelNode, disabled);
     }
     this.placeActionInUrlbar(action);
   },
 
   _updateActionDisabledInPanel(
--- a/browser/base/content/test/pageActions-proton/browser_PageActions_overflow.js
+++ b/browser/base/content/test/pageActions-proton/browser_PageActions_overflow.js
@@ -161,8 +161,96 @@ add_task(async function bookmark() {
   info("Remove the extension");
   let promiseUninstalled = promiseAddonUninstalled(extension.id);
   await extension.unload();
   await promiseUninstalled;
 
   await promiseStableResize(originalOuterWidth, win);
   await BrowserTestUtils.closeWindow(win);
 });
+
+add_task(async function test_disabledPageAction_hidden_in_protonOverflowMenu() {
+  // Make sure that proton is enabled for this test (indipendently from its
+  // default value set on the current build).
+  await SpecialPowers.pushPrefEnv({
+    set: [["browser.proton.enabled", true]],
+  });
+  // Make sure the overflow menu urlbar button is visible (indipendently from
+  // the current size of the Firefox window).
+  BrowserPageActions.mainButtonNode.style.visibility = "visible";
+  registerCleanupFunction(() => {
+    BrowserPageActions.mainButtonNode.style.removeProperty("visibility");
+  });
+
+  const extension = ExtensionTestUtils.loadExtension({
+    manifest: { page_action: {} },
+    async background() {
+      const { browser } = this;
+      const [tab] = await browser.tabs.query({
+        active: true,
+        currentWindow: true,
+      });
+      browser.test.assertTrue(tab, "Got an active tab as expected");
+      browser.test.onMessage.addListener(async msg => {
+        switch (msg) {
+          case "show-pageAction":
+            await browser.pageAction.show(tab.id);
+            break;
+          case "hide-pageAction":
+            await browser.pageAction.hide(tab.id);
+            break;
+          default:
+            browser.test.fail(`Unexpected message received: ${msg}`);
+        }
+        browser.test.sendMessage(`${msg}:done`);
+      });
+    },
+  });
+
+  await BrowserTestUtils.withNewTab("http://example.com", async browser => {
+    const win = browser.ownerGlobal;
+    const promisePageActionPanelClosed = async () => {
+      let popupHiddenPromise = promisePageActionPanelHidden(win);
+      win.BrowserPageActions.panelNode.hidePopup();
+      await popupHiddenPromise;
+    };
+
+    await extension.startup();
+    const widgetId = ExtensionCommon.makeWidgetId(extension.id);
+
+    info(
+      "Show pageAction and verify it is visible in the urlbar overflow menu"
+    );
+    extension.sendMessage("show-pageAction");
+    await extension.awaitMessage("show-pageAction:done");
+    await promisePageActionPanelOpen(win);
+    let pageActionNode = win.BrowserPageActions.panelButtonNodeForActionID(
+      widgetId
+    );
+    ok(
+      pageActionNode && BrowserTestUtils.is_visible(pageActionNode),
+      "enabled pageAction should be visible in the urlbar overflow menu"
+    );
+
+    info("Hide pageAction and verify it is hidden in the urlbar overflow menu");
+    extension.sendMessage("hide-pageAction");
+    await extension.awaitMessage("hide-pageAction:done");
+
+    await BrowserTestUtils.waitForCondition(
+      () => !win.BrowserPageActions.panelButtonNodeForActionID(widgetId),
+      "Wait for the disabled pageAction to be removed from the urlbar overflow menu"
+    );
+
+    await promisePageActionPanelClosed();
+
+    info("Reopen the urlbar overflow menu");
+    await promisePageActionPanelOpen(win);
+    ok(
+      !win.BrowserPageActions.panelButtonNodeForActionID(widgetId),
+      "Disabled pageAction is still removed as expected"
+    );
+
+    await promisePageActionPanelClosed();
+    await extension.unload();
+  });
+
+  await SpecialPowers.popPrefEnv();
+});
--- a/browser/components/extensions/test/browser/browser_ext_menus.js
+++ b/browser/components/extensions/test/browser/browser_ext_menus.js
@@ -168,21 +168,23 @@ add_task(async function test_actionConte
     is(tab.id, tabId, "Click event tab ID is correct");
   }
 
   BrowserTestUtils.removeTab(tab);
   await extension.unload();
 });
 
 add_task(async function test_hiddenPageActionContextMenu() {
-  // In Proton there's no page actions menu by default, there is only an
-  // overflow menu when the window is smaller than 680px. While currently
-  // disabled page actions are shown in that overflow menu, bug 1703889 will
-  // change the behavior, hiding them. Then this test won't be necessary anymore
-  // since the user won't be able to open the context menu on disabled actions.
+  // In Proton the disabled pageAction are hidden in the urlbar
+  // and in the overflow menu, and so the pageAction context menu
+  // cannot be triggered on a disabled pageACtion.
+  //
+  // When we will sunset the proton about:config pref, this test
+  // won't be necessary anymore since the user won't be able to
+  // open the context menu on disabled actions.
   await SpecialPowers.pushPrefEnv({
     set: [["browser.proton.enabled", false]],
   });
   const manifest = {
     page_action: {},
     permissions: ["menus"],
   };
 
--- a/browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js
+++ b/browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js
@@ -1,15 +1,24 @@
 /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* vim: set sts=2 sw=2 et tw=80: */
 "use strict";
 
 add_task(async function setup() {
   // The page action button is hidden by default for proton.
   // This tests the use of pageAction when the button is visible.
+  //
+  // TODO(Bug 1704171): this should technically be removed in a follow up
+  // and the tests in this file adapted to keep into account that:
+  // - in Proton the pageAction is pinned on the urlbar by default
+  //   when shown, and hidden when is not available (same for the
+  //   overflow menu when enabled)
+  // - with Proton disabled, the pageAction is always part of the overflow
+  //   panel (either if the pageAction is enabled or disabled) and on the urlbar
+  //   only if explicitly pinned.
   if (gProton) {
     BrowserPageActions.mainButtonNode.style.visibility = "visible";
     registerCleanupFunction(() => {
       BrowserPageActions.mainButtonNode.style.removeProperty("visibility");
     });
   }
 });
 
@@ -154,16 +163,27 @@ add_task(async function test_clickData_r
 
   await triggerPageActionWithKeyboard(extension);
   assertInfoReset(await extension.awaitMessage("onClick"));
 
   await extension.unload();
 });
 
 add_task(async function test_click_disabled() {
+  // In Proton the disabled pageAction are hidden in the urlbar
+  // and in the overflow menu, and so the pageAction context menu
+  // cannot be triggered on a disabled pageACtion.
+  //
+  // When we will sunset the proton about:config pref, this test
+  // won't be necessary anymore since the user won't be able to
+  // open the context menu on disabled actions.
+  await SpecialPowers.pushPrefEnv({
+    set: [["browser.proton.enabled", false]],
+  });
+
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       page_action: {},
     },
 
     background() {
       let expectClick = false;
       function onClicked(tab, info) {
@@ -213,10 +233,12 @@ add_task(async function test_click_disab
   extension.sendMessage("show");
   await extension.awaitMessage("visibilitySet");
 
   await clickPageActionInPanel(extension, window, { button: 0 });
   await extension.awaitMessage("onClick");
   await clickPageActionInPanel(extension, window, { button: 1 });
   await extension.awaitMessage("onClick");
 
+  // Undo the Proton pref change.
+  await SpecialPowers.popPrefEnv();
   await extension.unload();
 });
--- a/browser/modules/PageActions.jsm
+++ b/browser/modules/PageActions.jsm
@@ -1181,18 +1181,29 @@ Action.prototype = {
    *
    * @param  browserWindow (DOM window, required)
    *         The window.
    * @return True if the action should be shown and false otherwise.  Actions
    *         are always shown in the panel unless they're both transient and
    *         disabled.
    */
   shouldShowInPanel(browserWindow) {
+    // When Proton is enabled, the extension page actions should behave similarly
+    // to a transient action, and be hidden from the urlbar overflow menu if they
+    // are disabled (as in the urlbar when the overflow menu isn't available)
+    //
+    // TODO(Bug 1704139): as a follow up we may look into just set on all
+    // extensions pageActions `_transient: true`, at least once we sunset
+    // the proton preference and we don't need the pre-Proton behavior anymore,
+    // and remove this special case.
+    const isProtonExtensionAction = this.extensionID && protonEnabled;
+
     return (
-      (!this.__transient || !this.getDisabled(browserWindow)) &&
+      (!(this.__transient || isProtonExtensionAction) ||
+        !this.getDisabled(browserWindow)) &&
       this.canShowInWindow(browserWindow)
     );
   },
 
   /**
    * Returns whether the action should be shown in a given window's urlbar.
    *
    * @param  browserWindow (DOM window, required)