From ccca5d5c7d59fac6bad1a3191537febbbf80f03f Mon Sep 17 00:00:00 2001 From: Moritz Beier Date: Fri, 9 May 2025 08:41:37 +0000 Subject: [PATCH] Bug 1954705 - Confirm deletion of user and open search engines. r=search-reviewers,fluent-reviewers,urlbar-reviewers,bolsson,Standard8 Differential Revision: https://phabricator.services.mozilla.com/D245412 --- browser/components/preferences/search.js | 74 ++++++++++-- .../tests/browser_search_engineList.js | 107 +++++++++++++++--- .../en-US/browser/preferences/preferences.ftl | 4 + 3 files changed, 159 insertions(+), 26 deletions(-) diff --git a/browser/components/preferences/search.js b/browser/components/preferences/search.js index 5550694f4a43..153c5100dbb8 100644 --- a/browser/components/preferences/search.js +++ b/browser/components/preferences/search.js @@ -8,6 +8,7 @@ const lazy = {}; ChromeUtils.defineESModuleGetters(lazy, { + AddonSearchEngine: "resource://gre/modules/AddonSearchEngine.sys.mjs", CustomizableUI: "resource:///modules/CustomizableUI.sys.mjs", SearchUIUtils: "moz-src:///browser/components/search/SearchUIUtils.sys.mjs", SearchUtils: "resource://gre/modules/SearchUtils.sys.mjs", @@ -608,9 +609,11 @@ class EngineStore { var clonedObj = { iconURL: null, }; - for (let i of ["id", "name", "alias", "hidden"]) { + for (let i of ["id", "name", "alias", "hidden", "isAppProvided"]) { clonedObj[i] = aEngine[i]; } + clonedObj.isAddonEngine = + aEngine.wrappedJSObject instanceof lazy.AddonSearchEngine; clonedObj.isUserEngine = aEngine.wrappedJSObject instanceof lazy.UserSearchEngine; clonedObj.originalEngine = aEngine; @@ -676,8 +679,8 @@ class EngineStore { throw new Error("Cannot remove last engine!"); } - let engineName = aEngine.name; - let index = this.engines.findIndex(element => element.name == engineName); + let engineId = aEngine.id; + let index = this.engines.findIndex(element => element.id == engineId); if (index == -1) { throw new Error("invalid engine?"); @@ -785,10 +788,13 @@ class EngineStore { * Manages the view of the Search Shortcuts tree on the search pane of preferences. */ class EngineView { - _engineStore = null; + _engineStore; _engineList = null; tree = null; + /** + * @param {EngineStore} aEngineStore + */ constructor(aEngineStore) { this._engineStore = aEngineStore; this._engineList = document.getElementById("engineList"); @@ -939,6 +945,55 @@ class EngineView { ); } + /** + * Removes a search engine from the search service. + * + * Application provided engines are removed without confirmation since they + * can easily be restored. Addon engines are not removed (see comment). + * For other engine types, the user is prompted for confirmation. + * + * @param {object} engine + * The search engine object from EngineStore to remove. + */ + async promptAndRemoveEngine(engine) { + if (engine.isAppProvided) { + Services.search.removeEngine(this.selectedEngine.originalEngine); + return; + } + + if (engine.isAddonEngine) { + // Addon engines will re-appear after restarting, see Bug 1546652. + // This should ideally prompt the user if they want to remove the addon. + let msg = await document.l10n.formatValue("remove-addon-engine-alert"); + alert(msg); + return; + } + + let [body, removeLabel] = await document.l10n.formatValues([ + "remove-engine-confirmation", + "remove-engine-remove", + ]); + + let button = Services.prompt.confirmExBC( + window.browsingContext, + Services.prompt.MODAL_TYPE_CONTENT, + null, + body, + (Services.prompt.BUTTON_TITLE_IS_STRING * Services.prompt.BUTTON_POS_0) | + (Services.prompt.BUTTON_TITLE_CANCEL * Services.prompt.BUTTON_POS_1), + removeLabel, + null, + null, + null, + {} + ); + + // Button 0 is the remove button. + if (button == 0) { + Services.search.removeEngine(this.selectedEngine.originalEngine); + } + } + /** * Returns the local shortcut corresponding to a tree row, or null if the row * is not a local shortcut. @@ -1010,7 +1065,9 @@ class EngineView { this.#onRestoreDefaults(); break; case "removeEngineButton": - Services.search.removeEngine(this.selectedEngine.originalEngine); + if (this.isEngineSelectedAndRemovable()) { + this.promptAndRemoveEngine(this.selectedEngine); + } break; case "editEngineButton": gSubDialog.open( @@ -1110,11 +1167,12 @@ class EngineView { aEvent.keyCode == KeyEvent.DOM_VK_DELETE || (isMac && aEvent.shiftKey && - aEvent.keyCode == KeyEvent.DOM_VK_BACK_SPACE && - this.isEngineSelectedAndRemovable()) + aEvent.keyCode == KeyEvent.DOM_VK_BACK_SPACE) ) { // Delete and Shift+Backspace (Mac) removes selected engine. - Services.search.removeEngine(this.selectedEngine.originalEngine); + if (this.isEngineSelectedAndRemovable()) { + this.promptAndRemoveEngine(this.selectedEngine); + } } } } diff --git a/browser/components/preferences/tests/browser_search_engineList.js b/browser/components/preferences/tests/browser_search_engineList.js index 55e1cf960b58..7cfe08e5b9d3 100644 --- a/browser/components/preferences/tests/browser_search_engineList.js +++ b/browser/components/preferences/tests/browser_search_engineList.js @@ -1,3 +1,6 @@ +const { PromptTestUtils } = ChromeUtils.importESModule( + "resource://testing-common/PromptTestUtils.sys.mjs" +); const { SearchTestUtils } = ChromeUtils.importESModule( "resource://testing-common/SearchTestUtils.sys.mjs" ); @@ -45,6 +48,24 @@ async function engine_list_test(fn) { Object.defineProperty(task, "name", { value: fn.name }); add_task(task); } +async function selectEngine(tree, index) { + let rect = tree.getCoordsForCellItem( + index, + tree.columns.getNamedColumn("engineName"), + "text" + ); + let x = rect.x + rect.width / 2; + let y = rect.y + rect.height / 2; + let promise = BrowserTestUtils.waitForEvent(tree, "click"); + EventUtils.synthesizeMouse( + tree.body, + x, + y, + { clickCount: 1 }, + tree.ownerGlobal + ); + return promise; +} add_setup(async function () { installedEngines = await Services.search.getAppProvidedEngines(); @@ -64,11 +85,7 @@ add_setup(async function () { alias: "u", }); installedEngines.push(userEngine); - - registerCleanupFunction(async () => { - await Services.search.removeEngine(userEngine); - // Extension engine is cleaned up by SearchTestUtils. - }); + // The added engines are removed in the last test. }); engine_list_test(async function test_engine_list(tree) { @@ -201,25 +218,13 @@ engine_list_test(async function test_rename_engines(tree) { engine_list_test(async function test_remove_button_disabled_state(tree, doc) { let appProvidedEngines = await Services.search.getAppProvidedEngines(); - let win = tree.ownerGlobal; for (let i = 0; i < appProvidedEngines.length; i++) { let engine = appProvidedEngines[i]; let isDefaultSearchEngine = engine.id == Services.search.defaultEngine.id || engine.id == Services.search.defaultPrivateEngine.id; - let rect = tree.getCoordsForCellItem( - i, - tree.columns.getNamedColumn("engineName"), - "text" - ); - let x = rect.x + rect.width / 2; - let y = rect.y + rect.height / 2; - - let promise = BrowserTestUtils.waitForEvent(tree, "click"); - EventUtils.synthesizeMouse(tree.body, x, y, { clickCount: 1 }, win); - await promise; - + await selectEngine(tree, i); Assert.equal( doc.querySelector("#removeEngineButton").disabled, isDefaultSearchEngine, @@ -227,3 +232,69 @@ engine_list_test(async function test_remove_button_disabled_state(tree, doc) { ); } }); + +engine_list_test(async function test_remove_button(tree, doc) { + let win = tree.ownerGlobal; + let alertSpy = sinon.stub(win, "alert"); + + info("Removing user engine."); + let userEngineIndex = installedEngines.findIndex(e => e.id == userEngine.id); + await selectEngine(tree, userEngineIndex); + + let promptPromise = PromptTestUtils.handleNextPrompt( + gBrowser.selectedBrowser, + { modalType: Services.prompt.MODAL_TYPE_CONTENT }, + { buttonNumClick: 0 } // 0 = cancel, 1 = remove + ); + let removedPromise = SearchTestUtils.promiseSearchNotification( + SearchUtils.MODIFIED_TYPE.REMOVED, + SearchUtils.TOPIC_ENGINE_MODIFIED + ); + + doc.querySelector("#removeEngineButton").click(); + await promptPromise; + let removedEngine = await removedPromise; + Assert.equal( + removedEngine.id, + userEngine.id, + "User engine was removed after a prompt." + ); + + // Re-fetch the engines since removing the user engine changed it. + installedEngines = await Services.search.getEngines(); + + info("Removing extension engine."); + let extensionEngineIndex = installedEngines.findIndex( + e => e.id == extensionEngine.id + ); + await selectEngine(tree, extensionEngineIndex); + + doc.querySelector("#removeEngineButton").click(); + await TestUtils.waitForCondition(() => alertSpy.calledOnce); + Assert.ok(true, "Alert is shown when attempting to remove extension engine."); + + info("Removing (last) app provided engine."); + let appProvidedEngines = await Services.search.getAppProvidedEngines(); + let lastAppEngine = appProvidedEngines[appProvidedEngines.length - 1]; + let lastAppEngineIndex = installedEngines.findIndex( + e => e.id == lastAppEngine.id + ); + await selectEngine(tree, lastAppEngineIndex); + + doc.querySelector("#removeEngineButton").click(); + removedEngine = await SearchTestUtils.promiseSearchNotification( + SearchUtils.MODIFIED_TYPE.REMOVED, + SearchUtils.TOPIC_ENGINE_MODIFIED + ); + Assert.equal( + removedEngine.id, + lastAppEngine.id, + "Last app provided engine was removed without a prompt." + ); + + // Cleanup. + alertSpy.restore(); + Services.search.resetToAppDefaultEngine(); + // The user engine is purposefully not re-added. + // The extension engine is removed automatically on cleanup. +}); diff --git a/browser/locales/en-US/browser/preferences/preferences.ftl b/browser/locales/en-US/browser/preferences/preferences.ftl index a7a8cdf73d0e..a690cbebead8 100644 --- a/browser/locales/en-US/browser/preferences/preferences.ftl +++ b/browser/locales/en-US/browser/preferences/preferences.ftl @@ -820,6 +820,10 @@ search-keyword-warning-title = Duplicate Keyword search-keyword-warning-engine = You have chosen a keyword that is currently in use by “{ $name }”. Please select another. search-keyword-warning-bookmark = You have chosen a keyword that is currently in use by a bookmark. Please select another. +remove-engine-confirmation = Are you sure you want to remove this search engine? +remove-engine-remove = Remove +remove-addon-engine-alert = To remove this search engine, remove the associated add-on. + ## Containers Section containers-back-button2 =