Bug 1449947 - The "Add Search Engine" page action button doesn't respond to clicks when it's in the urlbar. r?Gijs draft
authorDrew Willcoxon <adw@mozilla.com>
Fri, 30 Mar 2018 09:34:47 -0700
changeset 775217 133d4500769a7904df47d9b2af607858537788f1
parent 774711 8c71359d60e21055074ae8bc3dcb796d20f0cbaf
push id104658
push userbmo:adw@mozilla.com
push dateFri, 30 Mar 2018 16:35:19 +0000
reviewersGijs
bugs1449947
milestone61.0a1
Bug 1449947 - The "Add Search Engine" page action button doesn't respond to clicks when it's in the urlbar. r?Gijs MozReview-Commit-ID: 5H9XfRXq8eO
browser/base/content/browser-pageActions.js
browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js
browser/base/content/test/urlbar/head.js
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -1201,39 +1201,42 @@ BrowserPageActions.addSearchEngine = {
     }
     for (let engine of this.engines) {
       let button = document.createElement("toolbarbutton");
       button.classList.add("subviewbutton", "subviewbutton-iconic");
       button.setAttribute("label", engine.title);
       button.setAttribute("image", engine.icon);
       button.setAttribute("uri", engine.uri);
       button.addEventListener("command", event => {
-        PanelMultiView.hidePopup(BrowserPageActions.panelNode);
-        this._handleClickOnEngineButton(button);
+        let panelNode = panelViewNode.closest("panel");
+        PanelMultiView.hidePopup(panelNode);
+        this._installEngine(button.getAttribute("uri"),
+                            button.getAttribute("image"));
       });
       body.appendChild(button);
     }
   },
 
   onCommand(event, buttonNode) {
     if (!buttonNode.closest("panel")) {
       // The urlbar button was clicked.  It should have a subview if there are
       // many engines.
       let manyEngines = this.engines.length > 1;
       this.action.setWantsSubview(manyEngines, window);
       if (manyEngines) {
         return;
       }
     }
-    this._handleClickOnEngineButton(buttonNode);
-  },
-
-  _handleClickOnEngineButton(button) {
-    this._installEngine(button.getAttribute("uri"),
-                        button.getAttribute("image"));
+    // Either the panel button or urlbar button was clicked -- not a button in
+    // the subview -- but in either case, there's only one search engine.
+    // (Because this method isn't called when the panel button is clicked and it
+    // shows a subview, and the many-engines case for the urlbar returned early
+    // above.)
+    let engine = this.engines[0];
+    this._installEngine(engine.uri, engine.icon);
   },
 
   _installEngine(uri, image) {
     Services.search.addEngine(uri, null, image, false, {
       onSuccess: engine => {
         BrowserPageActionFeedback.show(this.action, {
           text: this.strings.GetStringFromName("searchAddedFoundEngine"),
         });
--- a/browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js
+++ b/browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js
@@ -1,11 +1,11 @@
 "use strict";
 
-// Checks a page that doesn't offer any engines.
+// Checks the panel button with a page that doesn't offer any engines.
 add_task(async function none() {
   let url = "http://mochi.test:8888/";
   await BrowserTestUtils.withNewTab(url, async () => {
     // Open the panel.
     await promisePageActionPanelOpen();
     EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
     await promisePageActionPanelHidden();
 
@@ -15,17 +15,17 @@ add_task(async function none() {
               "Action should not be present in panel");
     let button =
       BrowserPageActions.panelButtonNodeForActionID("addSearchEngine");
     Assert.ok(!button, "Action button should not be in panel");
   });
 });
 
 
-// Checks a page that offers one engine.
+// Checks the panel button with a page that offers one engine.
 add_task(async function one() {
   let url = getRootDirectory(gTestPath) + "page_action_menu_add_search_engine_one.html";
   await BrowserTestUtils.withNewTab(url, async () => {
     // Open the panel.
     await promisePageActionPanelOpen();
 
     // The action should be present.
     let actions = PageActions.actionsInPanel(window);
@@ -40,17 +40,17 @@ add_task(async function one() {
     Assert.equal(button.label, expectedTitle, "Button label");
     Assert.equal(button.classList.contains("subviewbutton-nav"), false,
                  "Button should not expand into a subview");
 
     // Click the action's button.
     let enginePromise =
       promiseEngine("engine-added", "page_action_menu_add_search_engine_0");
     let hiddenPromise = promisePageActionPanelHidden();
-    let feedbackPromise = promiseFeedbackPanelHidden();
+    let feedbackPromise = promiseFeedbackPanelShownAndHidden();
     EventUtils.synthesizeMouseAtCenter(button, {});
     await hiddenPromise;
     let engine = await enginePromise;
     let feedbackText = await feedbackPromise;
     Assert.equal(feedbackText, "Added to Search Dropdown");
 
     // Open the panel again.
     await promisePageActionPanelOpen();
@@ -84,17 +84,17 @@ add_task(async function one() {
     Assert.ok(button, "Action button should be in panel");
     Assert.equal(button.label, expectedTitle, "Button label");
     Assert.equal(button.classList.contains("subviewbutton-nav"), false,
                  "Button should not expand into a subview");
   });
 });
 
 
-// Checks a page that offers many engines.
+// Checks the panel button with a page that offers many engines.
 add_task(async function many() {
   let url = getRootDirectory(gTestPath) + "page_action_menu_add_search_engine_many.html";
   await BrowserTestUtils.withNewTab(url, async () => {
     // Open the panel.
     await promisePageActionPanelOpen();
 
     // The action should be present.
     let actions = PageActions.actionsInPanel(window);
@@ -127,17 +127,17 @@ add_task(async function many() {
       ],
       "Subview children"
     );
 
     // Click the first engine to install it.
     let enginePromise =
       promiseEngine("engine-added", "page_action_menu_add_search_engine_0");
     let hiddenPromise = promisePageActionPanelHidden();
-    let feedbackPromise = promiseFeedbackPanelHidden();
+    let feedbackPromise = promiseFeedbackPanelShownAndHidden();
     EventUtils.synthesizeMouseAtCenter(body.childNodes[0], {});
     await hiddenPromise;
     let engines = [];
     let engine = await enginePromise;
     engines.push(engine);
     let feedbackText = await feedbackPromise;
     Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
 
@@ -155,17 +155,17 @@ add_task(async function many() {
       ],
       "Subview children"
     );
 
     // Click the next engine to install it.
     enginePromise =
       promiseEngine("engine-added", "page_action_menu_add_search_engine_1");
     hiddenPromise = promisePageActionPanelHidden();
-    feedbackPromise = promiseFeedbackPanelHidden();
+    feedbackPromise = promiseFeedbackPanelShownAndHidden();
     EventUtils.synthesizeMouseAtCenter(body.childNodes[0], {});
     await hiddenPromise;
     engine = await enginePromise;
     engines.push(engine);
     feedbackText = await feedbackPromise;
     Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
 
     // Open the panel again.  This time the action button should show the one
@@ -182,17 +182,17 @@ add_task(async function many() {
     Assert.equal(button.label, expectedTitle, "Button label");
     Assert.equal(button.classList.contains("subviewbutton-nav"), false,
                  "Button should not expand into a subview");
 
     // Click the button.
     enginePromise =
       promiseEngine("engine-added", "page_action_menu_add_search_engine_2");
     hiddenPromise = promisePageActionPanelHidden();
-    feedbackPromise = promiseFeedbackPanelHidden();
+    feedbackPromise = promiseFeedbackPanelShownAndHidden();
     EventUtils.synthesizeMouseAtCenter(button, {});
     await hiddenPromise;
     engine = await enginePromise;
     engines.push(engine);
     feedbackText = await feedbackPromise;
     Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
 
     // All engines are installed at this point.  Open the panel and make sure
@@ -231,17 +231,17 @@ add_task(async function many() {
 
     // Remove the second engine.
     enginePromise =
       promiseEngine("engine-removed", "page_action_menu_add_search_engine_1");
     Services.search.removeEngine(engines.shift());
     await enginePromise;
 
     // Open the panel again and check the subview.  The subview should be
-    // present now that there are two offerred engines again.
+    // present now that there are two offered engines again.
     await promisePageActionPanelOpen();
     actions = PageActions.actionsInPanel(window);
     action = actions.find(a => a.id == "addSearchEngine");
     Assert.ok(action, "Action should be present in panel");
     expectedTitle = "Add One-Click Search Engine";
     Assert.equal(action.getTitle(window), expectedTitle, "Action title");
     button = BrowserPageActions.panelButtonNodeForActionID("addSearchEngine");
     Assert.ok(button, "Button should be in panel");
@@ -284,22 +284,263 @@ add_task(async function many() {
       "Subview children"
     );
     EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
     await promisePageActionPanelHidden();
   });
 });
 
 
+// Checks the urlbar button with a page that offers one engine.
+add_task(async function urlbarOne() {
+  let url = getRootDirectory(gTestPath) + "page_action_menu_add_search_engine_one.html";
+  await BrowserTestUtils.withNewTab(url, async () => {
+    await promiseNodeVisible(BrowserPageActions.mainButtonNode);
+
+    // Pin the action to the urlbar.
+    let placedPromise = promisePlacedInUrlbar();
+    PageActions.actionForID("addSearchEngine").pinnedToUrlbar = true;
+
+    // It should be placed.
+    let button = await placedPromise;
+    let actions = PageActions.actionsInUrlbar(window);
+    let action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(action, "Action should be present in urlbar");
+    Assert.ok(button, "Action button should be in urlbar");
+
+    // Click the action's button.
+    let enginePromise =
+      promiseEngine("engine-added", "page_action_menu_add_search_engine_0");
+    let feedbackPromise = promiseFeedbackPanelShownAndHidden();
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    let engine = await enginePromise;
+    let feedbackText = await feedbackPromise;
+    Assert.equal(feedbackText, "Added to Search Dropdown");
+
+    // The action should be gone.
+    actions = PageActions.actionsInUrlbar(window);
+    action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(!action, "Action should not be present in urlbar");
+    button = BrowserPageActions.urlbarButtonNodeForActionID("addSearchEngine");
+    Assert.ok(!button, "Action button should not be in urlbar");
+
+    // Remove the engine.
+    enginePromise =
+      promiseEngine("engine-removed", "page_action_menu_add_search_engine_0");
+    placedPromise = promisePlacedInUrlbar();
+    Services.search.removeEngine(engine);
+    await enginePromise;
+
+    // The action should be present again.
+    button = await placedPromise;
+    actions = PageActions.actionsInUrlbar(window);
+    action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(action, "Action should be present in urlbar");
+    Assert.ok(button, "Action button should be in urlbar");
+
+    // Clean up.
+    PageActions.actionForID("addSearchEngine").pinnedToUrlbar = false;
+    await BrowserTestUtils.waitForCondition(() => {
+      return !BrowserPageActions.urlbarButtonNodeForActionID("addSearchEngine");
+    });
+  });
+});
+
+
+// Checks the urlbar button with a page that offers many engines.
+add_task(async function urlbarMany() {
+  let url = getRootDirectory(gTestPath) + "page_action_menu_add_search_engine_many.html";
+  await BrowserTestUtils.withNewTab(url, async () => {
+    await promiseNodeVisible(BrowserPageActions.mainButtonNode);
+
+    // Pin the action to the urlbar.
+    let placedPromise = promisePlacedInUrlbar();
+    PageActions.actionForID("addSearchEngine").pinnedToUrlbar = true;
+
+    // It should be placed.
+    let button = await placedPromise;
+    let actions = PageActions.actionsInUrlbar(window);
+    let action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(action, "Action should be present in urlbar");
+    Assert.ok(button, "Action button should be in urlbar");
+
+    // Click the action's button.  The activated-action panel should open, and
+    // it should contain the addSearchEngine subview.
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    let view = await waitForActivatedActionPanel();
+    let viewID =
+       BrowserPageActions._panelViewNodeIDForActionID("addSearchEngine", true);
+    Assert.equal(view.id, viewID, "View ID");
+    let body = view.firstChild;
+    Assert.deepEqual(
+      Array.map(body.childNodes, n => n.label),
+      [
+        "page_action_menu_add_search_engine_0",
+        "page_action_menu_add_search_engine_1",
+        "page_action_menu_add_search_engine_2",
+      ],
+      "Subview children"
+    );
+
+    // Click the first engine to install it.
+    let enginePromise =
+      promiseEngine("engine-added", "page_action_menu_add_search_engine_0");
+    let hiddenPromise =
+      promisePanelHidden(BrowserPageActions.activatedActionPanelNode);
+    let feedbackPromise = promiseFeedbackPanelShownAndHidden();
+    EventUtils.synthesizeMouseAtCenter(body.childNodes[0], {});
+    await hiddenPromise;
+    let engines = [];
+    let engine = await enginePromise;
+    engines.push(engine);
+    let feedbackText = await feedbackPromise;
+    Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
+
+    // Open the panel again.  The installed engine should be gone.
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    view = await waitForActivatedActionPanel();
+    body = view.firstChild;
+    Assert.deepEqual(
+      Array.map(body.childNodes, n => n.label),
+      [
+        "page_action_menu_add_search_engine_1",
+        "page_action_menu_add_search_engine_2",
+      ],
+      "Subview children"
+    );
+
+    // Click the next engine to install it.
+    enginePromise =
+      promiseEngine("engine-added", "page_action_menu_add_search_engine_1");
+    hiddenPromise =
+      promisePanelHidden(BrowserPageActions.activatedActionPanelNode);
+    feedbackPromise = promiseFeedbackPanelShownAndHidden();
+    EventUtils.synthesizeMouseAtCenter(body.childNodes[0], {});
+    await hiddenPromise;
+    engine = await enginePromise;
+    engines.push(engine);
+    feedbackText = await feedbackPromise;
+    Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
+
+    // Now there's only one engine left, so clicking the button should simply
+    // install it instead of opening the activated-action panel.
+    enginePromise =
+      promiseEngine("engine-added", "page_action_menu_add_search_engine_2");
+    feedbackPromise = promiseFeedbackPanelShownAndHidden();
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    engine = await enginePromise;
+    engines.push(engine);
+    feedbackText = await feedbackPromise;
+    Assert.equal(feedbackText, "Added to Search Dropdown", "Feedback text");
+
+    // All engines are installed at this point.  The action should be gone.
+    actions = PageActions.actionsInUrlbar(window);
+    action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(!action, "Action should be gone");
+    button = BrowserPageActions.urlbarButtonNodeForActionID("addSearchEngine");
+    Assert.ok(!button, "Button should not be in urlbar");
+
+    // Remove the first engine.
+    enginePromise =
+      promiseEngine("engine-removed", "page_action_menu_add_search_engine_0");
+    placedPromise = promisePlacedInUrlbar();
+    Services.search.removeEngine(engines.shift());
+    await enginePromise;
+
+    // The action should be placed again.
+    button = await placedPromise;
+    actions = PageActions.actionsInUrlbar(window);
+    action = actions.find(a => a.id == "addSearchEngine");
+    Assert.ok(action, "Action should be present in urlbar");
+    Assert.ok(button, "Button should be in urlbar");
+
+    // Remove the second engine.
+    enginePromise =
+      promiseEngine("engine-removed", "page_action_menu_add_search_engine_1");
+    Services.search.removeEngine(engines.shift());
+    await enginePromise;
+
+    // Open the panel again and check the subview.  The subview should be
+    // present now that there are two offered engines again.
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    view = await waitForActivatedActionPanel();
+    body = view.firstChild;
+    Assert.deepEqual(
+      Array.map(body.childNodes, n => n.label),
+      [
+        "page_action_menu_add_search_engine_0",
+        "page_action_menu_add_search_engine_1",
+      ],
+      "Subview children"
+    );
+
+    // Hide the panel.
+    hiddenPromise =
+      promisePanelHidden(BrowserPageActions.activatedActionPanelNode);
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    await hiddenPromise;
+
+    // Remove the third engine.
+    enginePromise =
+      promiseEngine("engine-removed", "page_action_menu_add_search_engine_2");
+    Services.search.removeEngine(engines.shift());
+    await enginePromise;
+
+    // Open the panel again and check the subview.
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    view = await waitForActivatedActionPanel();
+    body = view.firstChild;
+    Assert.deepEqual(
+      Array.map(body.childNodes, n => n.label),
+      [
+        "page_action_menu_add_search_engine_0",
+        "page_action_menu_add_search_engine_1",
+        "page_action_menu_add_search_engine_2",
+      ],
+      "Subview children"
+    );
+
+    // Hide the panel.
+    hiddenPromise =
+      promisePanelHidden(BrowserPageActions.activatedActionPanelNode);
+    EventUtils.synthesizeMouseAtCenter(button, {});
+    await hiddenPromise;
+
+    // Clean up.
+    PageActions.actionForID("addSearchEngine").pinnedToUrlbar = false;
+    await BrowserTestUtils.waitForCondition(() => {
+      return !BrowserPageActions.urlbarButtonNodeForActionID("addSearchEngine");
+    });
+  });
+});
+
+
 function promiseEngine(expectedData, expectedEngineName) {
+  info(`Waiting for engine ${expectedData}`);
   return TestUtils.topicObserved("browser-search-engine-modified", (engine, data) => {
+    info(`Got engine ${engine.wrappedJSObject.name} ${data}`);
     return expectedData == data &&
            expectedEngineName == engine.wrappedJSObject.name;
   }).then(([engine, data]) => engine);
 }
 
-function promiseFeedbackPanelHidden() {
-  return new Promise(resolve => {
-    BrowserPageActionFeedback.panelNode.addEventListener("popuphidden", event => {
-      resolve(BrowserPageActionFeedback.feedbackLabel.textContent);
-    }, {once: true});
+function promiseFeedbackPanelShownAndHidden() {
+  info("Waiting for feedback panel popupshown");
+  return BrowserTestUtils.waitForEvent(BrowserPageActionFeedback.panelNode, "popupshown").then(() => {
+    info("Got feedback panel popupshown. Now waiting for popuphidden");
+    return BrowserTestUtils.waitForEvent(BrowserPageActionFeedback.panelNode, "popuphidden")
+          .then(() => BrowserPageActionFeedback.feedbackLabel.textContent);
   });
 }
+
+function promisePlacedInUrlbar() {
+  let action = PageActions.actionForID("addSearchEngine");
+  return new Promise(resolve => {
+    let onPlaced = action._onPlacedInUrlbar;
+    action._onPlacedInUrlbar = button => {
+      action._onPlacedInUrlbar = onPlaced;
+      if (action._onPlacedInUrlbar) {
+        action._onPlacedInUrlbar(button);
+      }
+      promiseNodeVisible(button).then(() => resolve(button));
+    };
+  });
+}
--- a/browser/base/content/test/urlbar/head.js
+++ b/browser/base/content/test/urlbar/head.js
@@ -242,16 +242,48 @@ function promisePageActionPanelOpen() {
     EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
     return shownPromise;
   }).then(() => {
     // Wait for items in the panel to become visible.
     return promisePageActionViewChildrenVisible(BrowserPageActions.mainViewNode);
   });
 }
 
+async function waitForActivatedActionPanel() {
+  if (!BrowserPageActions.activatedActionPanelNode) {
+    info("Waiting for activated-action panel to be added to mainPopupSet");
+    await new Promise(resolve => {
+      let observer = new MutationObserver(mutations => {
+        if (BrowserPageActions.activatedActionPanelNode) {
+          observer.disconnect();
+          resolve();
+        }
+      });
+      let popupSet = document.getElementById("mainPopupSet");
+      observer.observe(popupSet, { childList: true });
+    });
+    info("Activated-action panel added to mainPopupSet");
+  }
+  if (!BrowserPageActions.activatedActionPanelNode.state == "open") {
+    info("Waiting for activated-action panel popupshown");
+    await promisePanelShown(BrowserPageActions.activatedActionPanelNode);
+    info("Got activated-action panel popupshown");
+  }
+  let panelView =
+    BrowserPageActions.activatedActionPanelNode.querySelector("panelview");
+  if (panelView) {
+    await BrowserTestUtils.waitForEvent(
+      BrowserPageActions.activatedActionPanelNode,
+      "ViewShown"
+    );
+    await promisePageActionViewChildrenVisible(panelView);
+  }
+  return panelView;
+}
+
 function promisePageActionPanelShown() {
   return promisePanelShown(BrowserPageActions.panelNode);
 }
 
 function promisePageActionPanelHidden() {
   return promisePanelHidden(BrowserPageActions.panelNode);
 }
 
@@ -288,26 +320,28 @@ function promisePageActionViewShown() {
   return BrowserTestUtils.waitForEvent(BrowserPageActions.panelNode, "ViewShown").then(async event => {
     let panelViewNode = event.originalTarget;
     await promisePageActionViewChildrenVisible(panelViewNode);
     return panelViewNode;
   });
 }
 
 function promisePageActionViewChildrenVisible(panelViewNode) {
-  info("promisePageActionViewChildrenVisible waiting for a child node to be visible");
+  return promiseNodeVisible(panelViewNode.firstChild.firstChild);
+}
+
+function promiseNodeVisible(node) {
+  info(`promiseNodeVisible waiting, node.id=${node.id} node.localeName=${node.localName}\n`);
   let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
                   .getInterface(Ci.nsIDOMWindowUtils);
   return BrowserTestUtils.waitForCondition(() => {
-    let bodyNode = panelViewNode.firstChild;
-    for (let childNode of bodyNode.childNodes) {
-      let bounds = dwu.getBoundsWithoutFlushing(childNode);
-      if (bounds.width > 0 && bounds.height > 0) {
-        return true;
-      }
+    let bounds = dwu.getBoundsWithoutFlushing(node);
+    if (bounds.width > 0 && bounds.height > 0) {
+      info(`promiseNodeVisible OK, node.id=${node.id} node.localeName=${node.localName}\n`);
+      return true;
     }
     return false;
   });
 }
 
 function promiseSpeculativeConnection(httpserver) {
   return BrowserTestUtils.waitForCondition(() => {
     if (httpserver) {