From 795e24c040f63a2981d8e279bd7e70e34fe4d3a0 Mon Sep 17 00:00:00 2001 From: William Durand Date: Thu, 17 Nov 2022 09:45:27 +0000 Subject: [PATCH] Bug 1799846 - Filter out extensions that have a browser action in the non-CUI list in the unified extensions panel. r=rpl Differential Revision: https://phabricator.services.mozilla.com/D161686 --- browser/base/content/browser-addons.js | 58 ++++++++-- .../test/browser/browser_ext_activeScript.js | 45 +++++--- .../test/browser/browser_ext_originControls.js | 46 ++++---- .../test/browser/browser_unified_extensions.js | 58 ++++++++++ .../browser_unified_extensions_context_menu.js | 120 +-------------------- browser/modules/ExtensionsUI.jsm | 5 +- toolkit/components/extensions/Extension.jsm | 4 + 7 files changed, 170 insertions(+), 166 deletions(-) diff --git a/browser/base/content/browser-addons.js b/browser/base/content/browser-addons.js index e80563b303c0..65e12c190d02 100644 --- a/browser/base/content/browser-addons.js +++ b/browser/base/content/browser-addons.js @@ -1509,23 +1509,58 @@ var gUnifiedExtensions = { /** * Gets a list of active AddonWrapper instances of type "extension", sorted - * alphabetically based on add-on's names. + * alphabetically based on add-on's names. Optionally, filter out extensions + * with browser action. * - * @return {Array} An array of active extensions. + * @param {bool} all When set to true (the default), return the list of all + * active extensions, including the ones that have a + * browser action. Otherwise, extensions with browser + * action are filtered out. + * @returns {Array} An array of active extensions. */ - async getActiveExtensions() { + async getActiveExtensions(all = true) { // TODO: Bug 1778682 - Use a new method on `AddonManager` so that we get // the same list of extensions as the one in `about:addons`. - // We only want to display active and visible extensions, and we want to - // list them alphabetically. + // We only want to display active and visible extensions that do not have a + // browser action, and we want to list them alphabetically. let addons = await AddonManager.getAddonsByTypes(["extension"]); - addons = addons.filter(addon => !addon.hidden && addon.isActive); + addons = addons.filter( + addon => + !addon.hidden && + addon.isActive && + (all || + !WebExtensionPolicy.getByID(addon.id).extension.hasBrowserActionUI) + ); addons.sort((a1, a2) => a1.name.localeCompare(a2.name)); return addons; }, + /** + * Returns true when there are active extensions listed/shown in the unified + * extensions panel, and false otherwise (e.g. when extensions are pinned in + * the toolbar OR there are 0 active extensions). + * + * @returns {boolean} Whether there are extensions listed in the panel. + */ + async hasExtensionsInPanel() { + const extensions = await this.getActiveExtensions(); + + return !!extensions + .map(extension => { + const policy = WebExtensionPolicy.getByID(extension.id); + return this.browserActionFor(policy)?.widget; + }) + .filter(widget => { + return ( + !widget || + widget?.areaType !== CustomizableUI.TYPE_TOOLBAR || + widget?.forWindow(window).overflowed + ); + }).length; + }, + handleEvent(event) { switch (event.type) { case "ViewShowing": { @@ -1540,7 +1575,10 @@ var gUnifiedExtensions = { async onPanelViewShowing(panelview) { const list = panelview.querySelector(".unified-extensions-list"); - const extensions = await this.getActiveExtensions(); + // Only add extensions that do not have a browser action in this list since + // the extensions with browser action have CUI widgets and will appear in + // the panel (or toolbar) via the CUI mechanism. + const extensions = await this.getActiveExtensions(/* all */ false); for (const extension of extensions) { const item = document.createElement("unified-extensions-item"); @@ -1584,9 +1622,9 @@ var gUnifiedExtensions = { } let panel = this.panel; - // The button should directly open `about:addons` when there is no active - // extension to show in the panel. - if ((await this.getActiveExtensions()).length === 0) { + // The button should directly open `about:addons` when the user does not + // have any active extensions listed in the unified extensions panel. + if (!(await this.hasExtensionsInPanel())) { await BrowserOpenAddonsMgr("addons://discover/"); return; } diff --git a/browser/components/extensions/test/browser/browser_ext_activeScript.js b/browser/components/extensions/test/browser/browser_ext_activeScript.js index ef5c246db664..b20fbbd73f9f 100644 --- a/browser/components/extensions/test/browser/browser_ext_activeScript.js +++ b/browser/components/extensions/test/browser/browser_ext_activeScript.js @@ -27,6 +27,7 @@ async function makeExtension({ manifest_version = 3, granted = [], noStaticScript = false, + withUnifiedExtensionsPanel, }) { info(`Loading extension ` + JSON.stringify({ id, granted })); @@ -44,14 +45,18 @@ async function makeExtension({ ], }; - if (manifest_version === 3) { - manifest.action = { - default_area: "navbar", - }; - } else { - manifest.browser_action = { - default_area: "navbar", - }; + // When the unified extensions pref is enabled, we want to verify the behavior + // of the non-CUI widgets inside the panel. + if (!withUnifiedExtensionsPanel) { + if (manifest_version === 3) { + manifest.action = { + default_area: "navbar", + }; + } else { + manifest.browser_action = { + default_area: "navbar", + }; + } } let ext = ExtensionTestUtils.loadExtension({ @@ -215,26 +220,39 @@ const verifyActionActiveScript = async ({ id: "ext0@test", manifest_version: 2, granted: ["*://example.com/*"], + withUnifiedExtensionsPanel, }); - let ext1 = await makeExtension({ id: "ext1@test" }); + let ext1 = await makeExtension({ + id: "ext1@test", + withUnifiedExtensionsPanel, + }); let ext2 = await makeExtension({ id: "ext2@test", granted: ["*://example.com/*"], + withUnifiedExtensionsPanel, }); let ext3 = await makeExtension({ id: "ext3@test", granted: ["*://mochi.test/*"], + withUnifiedExtensionsPanel, }); // Test run_at script ordering. - let ext4 = await makeExtension({ id: "ext4@test" }); + let ext4 = await makeExtension({ + id: "ext4@test", + withUnifiedExtensionsPanel, + }); // Test without static scripts in the manifest, because they add optional // permissions, and we specifically want to test activeTab without them. - let ext5 = await makeExtension({ id: "ext5@test", noStaticScript: true }); + let ext5 = await makeExtension({ + id: "ext5@test", + noStaticScript: true, + withUnifiedExtensionsPanel, + }); await BrowserTestUtils.withNewTab("about:blank", async () => { info("No content scripts run on top level about:blank."); @@ -458,14 +476,9 @@ add_task(async function test_action_activeScript() { }); add_task(async function test_activeScript_with_unified_extensions_panel() { - await SpecialPowers.pushPrefEnv({ - set: [["extensions.unifiedExtensions.enabled", true]], - }); - const win = await promiseEnableUnifiedExtensions(); await verifyActionActiveScript({ win, withUnifiedExtensionsPanel: true }); await BrowserTestUtils.closeWindow(win); - await SpecialPowers.popPrefEnv(); }); diff --git a/browser/components/extensions/test/browser/browser_ext_originControls.js b/browser/components/extensions/test/browser/browser_ext_originControls.js index 40b1cffa7d71..2bdbc66c8762 100644 --- a/browser/components/extensions/test/browser/browser_ext_originControls.js +++ b/browser/components/extensions/test/browser/browser_ext_originControls.js @@ -6,10 +6,17 @@ const { ExtensionPermissions } = ChromeUtils.import( loadTestSubscript("head_unified_extensions.js"); +let win; + add_setup(async () => { await SpecialPowers.pushPrefEnv({ set: [["extensions.manifestV3.enabled", true]], }); + + win = await promiseEnableUnifiedExtensions(); + registerCleanupFunction(async () => { + await BrowserTestUtils.closeWindow(win); + }); }); async function makeExtension({ @@ -80,29 +87,21 @@ async function testOriginControls( let buttonOrWidget; let menu; - let manageExtensionClassName; - let visibleOriginItems; + let nextMenuItemClassName; switch (contextMenuId) { case "toolbar-context-menu": let target = `#${CSS.escape(makeWidgetId(extension.id))}-BAP`; buttonOrWidget = win.document.querySelector(target).parentElement; menu = await openChromeContextMenu(contextMenuId, target, win); - manageExtensionClassName = "customize-context-manageExtension"; - visibleOriginItems = menu.querySelectorAll( - ":is(menuitem, menuseparator):not([hidden])" - ); + nextMenuItemClassName = "customize-context-manageExtension"; break; case "unified-extensions-context-menu": await openExtensionsPanel(win); buttonOrWidget = getUnifiedExtensionsItem(win, extension.id); menu = await openUnifiedExtensionsContextMenu(win, extension.id); - manageExtensionClassName = - "unified-extensions-context-menu-manage-extension"; - visibleOriginItems = menu.querySelectorAll( - ".unified-extensions-context-menu-management-separator ~ :is(menuitem, menuseparator):not([hidden])" - ); + nextMenuItemClassName = "unified-extensions-context-menu-pin-to-toolbar"; break; default: @@ -110,6 +109,9 @@ async function testOriginControls( } let doc = menu.ownerDocument; + let visibleOriginItems = menu.querySelectorAll( + ":is(menuitem, menuseparator):not([hidden])" + ); info("Check expected menu items."); for (let i = 0; i < items.length; i++) { @@ -132,7 +134,7 @@ async function testOriginControls( ); is( visibleOriginItems[items.length + 1].className, - manageExtensionClassName, + nextMenuItemClassName, "All items accounted for." ); } @@ -220,16 +222,23 @@ const originControlsInContextMenu = async options => { if (options.contextMenuId === "unified-extensions-context-menu") { // Unified button should only show a notification indicator when extensions // asking for attention are not already visible in the toolbar. + moveWidget(ext1, options.win, false); moveWidget(ext2, options.win, false); moveWidget(ext3, options.win, false); + moveWidget(ext4, options.win, false); + moveWidget(ext5, options.win, false); unifiedButton = options.win.document.querySelector( "#unified-extensions-button" ); } else { // TestVerify runs this again in the same Firefox instance, so move the - // widgets back to the toolbar for testing outside the unified button. + // widgets back to the toolbar for testing outside the unified extensions + // panel. + moveWidget(ext1, options.win, true); moveWidget(ext2, options.win, true); moveWidget(ext3, options.win, true); + moveWidget(ext4, options.win, true); + moveWidget(ext5, options.win, true); } const NO_ACCESS = { id: "origin-controls-no-access", args: null }; @@ -385,23 +394,14 @@ const originControlsInContextMenu = async options => { add_task(async function originControls_in_browserAction_contextMenu() { await originControlsInContextMenu({ - win: window, + win, contextMenuId: "toolbar-context-menu", }); }); add_task(async function originControls_in_unifiedExtensions_contextMenu() { - await SpecialPowers.pushPrefEnv({ - set: [["extensions.unifiedExtensions.enabled", true]], - }); - - const win = await promiseEnableUnifiedExtensions(); - await originControlsInContextMenu({ win, contextMenuId: "unified-extensions-context-menu", }); - - await BrowserTestUtils.closeWindow(win); - await SpecialPowers.popPrefEnv(); }); diff --git a/browser/components/extensions/test/browser/browser_unified_extensions.js b/browser/components/extensions/test/browser/browser_unified_extensions.js index 18f9dd821fcb..1bc13eaa8b84 100644 --- a/browser/components/extensions/test/browser/browser_unified_extensions.js +++ b/browser/components/extensions/test/browser/browser_unified_extensions.js @@ -294,10 +294,30 @@ add_task(async function test_panel_has_a_manage_extensions_button() { }); add_task(async function test_list_active_extensions_only() { + await SpecialPowers.pushPrefEnv({ + set: [["extensions.manifestV3.enabled", true]], + }); + const arrayOfManifestData = [ { name: "hidden addon", hidden: true }, { name: "regular addon", hidden: false }, { name: "disabled addon", hidden: false }, + { + name: "regular addon with browser action", + hidden: false, + browser_action: {}, + }, + { + manifest_version: 3, + name: "regular mv3 addon with browser action", + hidden: false, + action: {}, + }, + { + name: "regular addon with page action", + hidden: false, + page_action: {}, + }, ]; const extensions = createExtensions(arrayOfManifestData, { // We have to use the mock provider below so we don't need to use the @@ -334,6 +354,27 @@ add_task(async function test_list_active_extensions_only() { // Mark this add-on as disabled. userDisabled: true, }, + { + id: extensions[3].id, + name: arrayOfManifestData[3].name, + type: "extension", + version: "1", + hidden: arrayOfManifestData[3].hidden, + }, + { + id: extensions[4].id, + name: arrayOfManifestData[4].name, + type: "extension", + version: "1", + hidden: arrayOfManifestData[4].hidden, + }, + { + id: extensions[5].id, + name: arrayOfManifestData[5].name, + type: "extension", + version: "1", + hidden: arrayOfManifestData[4].hidden, + }, ]); await openExtensionsPanel(win); @@ -351,6 +392,23 @@ add_task(async function test_list_active_extensions_only() { const disabledAddonItem = getUnifiedExtensionsItem(win, extensions[2].id); is(disabledAddonItem, null, `didn't expect an item for ${extensions[2].id}`); + const browserActionItem = getUnifiedExtensionsItem(win, extensions[3].id); + is(browserActionItem, null, `didn't expect an item for ${extensions[3].id}`); + + const mv3BrowserActionItem = getUnifiedExtensionsItem(win, extensions[4].id); + is( + mv3BrowserActionItem, + null, + `didn't expect an item for ${extensions[4].id}` + ); + + const pageActionItem = getUnifiedExtensionsItem(win, extensions[5].id); + is( + pageActionItem.querySelector(".unified-extensions-item-name").textContent, + "regular addon with page action", + "expected an item for a regular add-on with page action" + ); + await closeExtensionsPanel(win); await Promise.all(extensions.map(extension => extension.unload())); diff --git a/browser/components/extensions/test/browser/browser_unified_extensions_context_menu.js b/browser/components/extensions/test/browser/browser_unified_extensions_context_menu.js index 2aa0aac773b6..ef1cb8682dee 100644 --- a/browser/components/extensions/test/browser/browser_unified_extensions_context_menu.js +++ b/browser/components/extensions/test/browser/browser_unified_extensions_context_menu.js @@ -587,51 +587,7 @@ add_task(async function test_context_menu_without_browserActionFor_global() { cleanup(); }); -add_task(async function test_browser_action_context_menu() { - const extWithMenuBrowserAction = ExtensionTestUtils.loadExtension({ - manifest: { - browser_action: { - default_area: "navbar", - }, - permissions: ["contextMenus"], - }, - background() { - browser.contextMenus.create( - { - id: "some-menu-id", - title: "Click me!", - contexts: ["all"], - }, - () => browser.test.sendMessage("ready") - ); - }, - useAddonManager: "temporary", - }); - const extWithSubMenuBrowserAction = ExtensionTestUtils.loadExtension({ - manifest: { - browser_action: { - default_area: "navbar", - }, - permissions: ["contextMenus"], - }, - background() { - browser.contextMenus.create({ - id: "some-menu-id", - title: "Open sub-menu", - contexts: ["all"], - }); - browser.contextMenus.create( - { - id: "some-sub-menu-id", - parentId: "some-menu-id", - title: "Click me!", - contexts: ["all"], - }, - () => browser.test.sendMessage("ready") - ); - }, - useAddonManager: "temporary", - }); +add_task(async function test_page_action_context_menu() { const extWithMenuPageAction = ExtensionTestUtils.loadExtension({ manifest: { page_action: {}, @@ -644,7 +600,7 @@ add_task(async function test_browser_action_context_menu() { title: "Click me!", contexts: ["all"], }, - () => browser.test.sendMessage("ready") + () => browser.test.sendMessage("menu-created") ); }, useAddonManager: "temporary", @@ -655,59 +611,19 @@ add_task(async function test_browser_action_context_menu() { }, useAddonManager: "temporary", }); - const extWithoutMenu2 = ExtensionTestUtils.loadExtension({ - manifest: { - browser_action: { - default_area: "navbar", - }, - name: "extension with a browser action but no menu", - }, - useAddonManager: "temporary", - }); - const extensions = [ - extWithMenuBrowserAction, - extWithMenuPageAction, - extWithSubMenuBrowserAction, - extWithoutMenu1, - extWithoutMenu2, - ]; + const extensions = [extWithMenuPageAction, extWithoutMenu1]; await Promise.all(extensions.map(extension => extension.startup())); - await extWithMenuBrowserAction.awaitMessage("ready"); - await extWithMenuPageAction.awaitMessage("ready"); - await extWithSubMenuBrowserAction.awaitMessage("ready"); + await extWithMenuPageAction.awaitMessage("menu-created"); await openExtensionsPanel(win); - info("extension with browser action and a menu"); - // The context menu for the extension that declares a browser action menu - // should have the menu item created by the extension, a menu separator and - // the 3 default menu items. - let contextMenu = await openUnifiedExtensionsContextMenu( - win, - extWithMenuBrowserAction.id - ); - assertVisibleContextMenuItems(contextMenu, 5); - - const [item, separator] = contextMenu.children; - is( - item.getAttribute("label"), - "Click me!", - "expected menu item as first child" - ); - is( - separator.tagName, - "menuseparator", - "expected separator after last menu item created by the extension" - ); - await closeChromeContextMenu(contextMenu.id, null, win); - info("extension with page action and a menu"); // This extension declares a page action so its menu shouldn't be added to // the unified extensions context menu. - contextMenu = await openUnifiedExtensionsContextMenu( + let contextMenu = await openUnifiedExtensionsContextMenu( win, extWithMenuPageAction.id ); @@ -721,32 +637,6 @@ add_task(async function test_browser_action_context_menu() { assertVisibleContextMenuItems(contextMenu, 3); await closeChromeContextMenu(contextMenu.id, null, win); - info("extension with browser action and no menu"); - // There is no context menu created by this extension, so there should only - // be 3 menu items corresponding to the default manage/remove/report items. - contextMenu = await openUnifiedExtensionsContextMenu(win, extWithoutMenu2.id); - assertVisibleContextMenuItems(contextMenu, 3); - await closeChromeContextMenu(contextMenu.id, null, win); - - info("extension with browser action and menu + sub-menu"); - // Open a context menu that has a sub-menu and verify that we can get to the - // sub-menu item. - contextMenu = await openUnifiedExtensionsContextMenu( - win, - extWithSubMenuBrowserAction.id - ); - assertVisibleContextMenuItems(contextMenu, 5); - const popup = await openSubmenu(contextMenu.children[0]); - is(popup.children.length, 1, "expected 1 submenu item"); - is( - popup.children[0].getAttribute("label"), - "Click me!", - "expected menu item" - ); - // The number of items in the (main) context menu should remain the same. - assertVisibleContextMenuItems(contextMenu, 5); - await closeChromeContextMenu(contextMenu.id, null, win); - await closeExtensionsPanel(win); await Promise.all(extensions.map(extension => extension.unload())); diff --git a/browser/modules/ExtensionsUI.jsm b/browser/modules/ExtensionsUI.jsm index caad14142888..9f7af10d1c63 100644 --- a/browser/modules/ExtensionsUI.jsm +++ b/browser/modules/ExtensionsUI.jsm @@ -686,11 +686,12 @@ var ExtensionsUI = { ); } - // Insert all before Manage Extension, after any extension's menu items. + // Insert all before Pin to toolbar OR Manage Extension, after any + // extension's menu items. let items = [headerItem, whenClicked, alwaysOn, allDomains, separator]; let manageItem = popup.querySelector(".customize-context-manageExtension") || - popup.querySelector(".unified-extensions-context-menu-manage-extension"); + popup.querySelector(".unified-extensions-context-menu-pin-to-toolbar"); items.forEach(item => item && popup.insertBefore(item, manageItem)); let cleanup = () => items.forEach(item => item?.remove()); diff --git a/toolkit/components/extensions/Extension.jsm b/toolkit/components/extensions/Extension.jsm index 96c3656fcda9..16b95e2bfbae 100644 --- a/toolkit/components/extensions/Extension.jsm +++ b/toolkit/components/extensions/Extension.jsm @@ -3374,6 +3374,10 @@ class Extension extends ExtensionData { } return this._optionalOrigins; } + + get hasBrowserActionUI() { + return this.manifest.browser_action || this.manifest.action; + } } class Dictionary extends ExtensionData { -- 2.11.4.GIT