Bug 1600953 - Ignore clicks within tip results outside the buttons. r=harry

A couple of problems: * `pickElement` gets the tip's URL (from `UrlbarUtils.getUrlFromResult`). As long as the picked element isn't the help button, we go ahead and use that URL even if the element is not the main button. * Even after fixing the previous problem, `_on_mousedown` will "select" the tip row itself (not its buttons). If the tip button is already selected (because it's the heuristic for example), the selection goes away at that point. So we need to ignore tip elements that aren't the buttons. Differential Revision: https://phabricator.services.mozilla.com/D55758 --HG-- extra : moz-landing-system : lando
2019-12-04 18:55:51 +00:00 · 2019-12-04 18:55:51 +00:00 · 5475985abc
--- a/browser/components/urlbar/UrlbarView.jsm
+++ b/browser/components/urlbar/UrlbarView.jsm
@ -20,13 +20,6 @@ XPCOMUtils.defineLazyModuleGetters(this, {
 // by setting UrlbarView.removeStaleRowsTimeout.
 const DEFAULT_REMOVE_STALE_ROWS_TIMEOUT = 400;

-// The classNames of view elements that can be selected.
-const SELECTABLE_ELEMENTS = [
-  "urlbarView-row",
-  "urlbarView-tip-button",
-  "urlbarView-tip-help",
-];
-
 const getBoundsWithoutFlushing = element =>
  element.ownerGlobal.windowUtils.getBoundsWithoutFlushing(element);

@ -290,11 +283,13 @@ class UrlbarView {
  }

  /**
+   * Returns the result of the row containing the given element, or the result
+   * of the element if it itself is a row.
+   *
   * @param {Element} element
   *   An element in the view.
   * @returns {UrlbarResult}
-   *   The result attached to parameter `element`, if `element` is a row or a
-   *   decendant of a row.
+   *   The result of the element's row.
   */
  getResultFromElement(element) {
    if (!this.isOpen) {
@ -310,6 +305,31 @@ class UrlbarView {
    return row.result;
  }

+  /**
+   * Returns the element closest to the given element that can be
+   * selected/picked.  If the element itself can be selected, it's returned.  If
+   * there is no such element, null is returned.
+   *
+   * @param {Element} element
+   *   An element in the view.
+   * @returns {Element}
+   *   The closest element that can be picked including the element itself, or
+   *   null if there is no such element.
+   */
+  getClosestSelectableElement(element) {
+    let result = this.getResultFromElement(element);
+    if (result && result.type == UrlbarUtils.RESULT_TYPE.TIP) {
+      if (
+        element.classList.contains("urlbarView-tip-button") ||
+        element.classList.contains("urlbarView-tip-help")
+      ) {
+        return element;
+      }
+      return null;
+    }
+    return element.closest(".urlbarView-row");
+  }
+
  /**
   * Moves the view selection forward or backward.
   *
@ -1436,11 +1456,12 @@ class UrlbarView {
      // Ignore right clicks.
      return;
    }
-    let target = event.target;
-    while (!SELECTABLE_ELEMENTS.includes(target.className)) {
-      target = target.parentNode;
+    let element = this.getClosestSelectableElement(event.target);
+    if (!element) {
+      // Ignore clicks on elements that can't be selected/picked.
+      return;
    }
-    this._selectElement(target, { updateInput: false });
+    this._selectElement(element, { updateInput: false });
    this.controller.speculativeConnect(
      this.selectedResult,
      this._queryContext,
@ -1453,7 +1474,12 @@ class UrlbarView {
      // Ignore right clicks.
      return;
    }
-    this.input.pickElement(event.target, event);
+    let element = this.getClosestSelectableElement(event.target);
+    if (!element) {
+      // Ignore clicks on elements that can't be selected/picked.
+      return;
+    }
+    this.input.pickElement(element, event);
  }

  _on_overflow(event) {
--- a/browser/components/urlbar/tests/browser/browser_tip_selection.js
+++ b/browser/components/urlbar/tests/browser/browser_tip_selection.js
@ -1,14 +1,14 @@
 /* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

+// Tests keyboard selection within and clicks on UrlbarUtils.RESULT_TYPE.TIP
+// results.
+
 "use strict";

-const MEGABAR_PREF = "browser.urlbar.megabar";
 const HELP_URL = "about:mozilla";
 const TIP_URL = "about:about";

-// Tests keyboard selection within UrlbarUtils.RESULT_TYPE.TIP results.
-
 /**
 * A test provider.
 */
@ -67,7 +67,7 @@ add_task(async function tipIsSecondResult() {
  await UrlbarTestUtils.promiseAutocompleteResultPopup({
    value: "test",
    window,
-    waitForFocus: SimpleTest.waitForFocus,
+    waitForFocus,
  });

  Assert.equal(
@ -120,14 +120,22 @@ add_task(async function tipIsSecondResult() {
    "The third element should be selected."
  );

-  EventUtils.synthesizeKey("KEY_ArrowDown");
-  await BrowserTestUtils.waitForCondition(() => {
-    info("Waiting for one-off to become selected.");
-    let oneOff = document.querySelector(
-      ".urlbarView .search-panel-one-offs .searchbar-engine-one-off-item:first-child"
+  // If this test is running alone, the one-offs will rebuild themselves when
+  // the view is opened above, and they may not be visible yet.  Wait for the
+  // first one to become visible before trying to select it.
+  await TestUtils.waitForCondition(() => {
+    return (
+      gURLBar.view.oneOffSearchButtons.buttons.firstElementChild &&
+      BrowserTestUtils.is_visible(
+        gURLBar.view.oneOffSearchButtons.buttons.firstElementChild
+      )
    );
-    return oneOff.hasAttribute("selected");
-  });
+  }, "Waiting for first one-off to become visible.");
+
+  EventUtils.synthesizeKey("KEY_ArrowDown");
+  await TestUtils.waitForCondition(() => {
+    return gURLBar.view.oneOffSearchButtons.selectedButton;
+  }, "Waiting for one-off to become selected.");
  Assert.equal(
    UrlbarTestUtils.getSelectedElementIndex(window),
    -1,
@ -168,7 +176,7 @@ add_task(async function tipIsOnlyResult() {
  await UrlbarTestUtils.promiseAutocompleteResultPopup({
    value: "test",
    window,
-    waitForFocus: SimpleTest.waitForFocus,
+    waitForFocus,
  });

  Assert.equal(
@ -253,7 +261,7 @@ add_task(async function tipHasNoHelpButton() {
  await UrlbarTestUtils.promiseAutocompleteResultPopup({
    value: "test",
    window,
-    waitForFocus: SimpleTest.waitForFocus,
+    waitForFocus,
  });

  Assert.equal(
@ -289,13 +297,9 @@ add_task(async function tipHasNoHelpButton() {
  );

  EventUtils.synthesizeKey("KEY_ArrowDown");
-  await BrowserTestUtils.waitForCondition(() => {
-    info("Waiting for one-off to become selected.");
-    let oneOff = document.querySelector(
-      ".urlbarView .search-panel-one-offs .searchbar-engine-one-off-item:first-child"
-    );
-    return oneOff.hasAttribute("selected");
-  });
+  await TestUtils.waitForCondition(() => {
+    return gURLBar.view.oneOffSearchButtons.selectedButton;
+  }, "Waiting for one-off to become selected.");
  Assert.equal(
    UrlbarTestUtils.getSelectedElementIndex(window),
    -1,
@ -338,31 +342,79 @@ add_task(async function mouseSelection() {
  let provider = new TipTestProvider(matches);
  UrlbarProvidersManager.registerProvider(provider);

+  // Click the help button.
  await UrlbarTestUtils.promiseAutocompleteResultPopup({
    value: "test",
    window,
-    waitForFocus: SimpleTest.waitForFocus,
+    waitForFocus,
  });
-  let element = document.querySelector(".urlbarView-row .urlbarView-tip-help");
+  let row = await UrlbarTestUtils.waitForAutocompleteResultAt(window, 0);
+  let helpButton = row._elements.get("helpButton");
  let loadPromise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
-  EventUtils.synthesizeMouseAtCenter(element, {}, element.ownerGlobal);
-  await loadPromise;
+  await Promise.all([
+    loadPromise,
+    UrlbarTestUtils.promisePopupClose(window, () => {
+      EventUtils.synthesizeMouseAtCenter(helpButton, {});
+    }),
+  ]);
  Assert.equal(
    gURLBar.value,
    HELP_URL,
    "Should have navigated to the tip's help page."
  );

+  // Click the main button.
  await UrlbarTestUtils.promiseAutocompleteResultPopup({
    value: "test",
    window,
-    waitForFocus: SimpleTest.waitForFocus,
+    waitForFocus,
  });
-  element = document.querySelector(".urlbarView-row .urlbarView-tip-button");
+  row = await UrlbarTestUtils.waitForAutocompleteResultAt(window, 0);
+  let mainButton = row._elements.get("tipButton");
  loadPromise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
-  EventUtils.synthesizeMouseAtCenter(element, {}, element.ownerGlobal);
-  await loadPromise;
+  await Promise.all([
+    loadPromise,
+    UrlbarTestUtils.promisePopupClose(window, () => {
+      EventUtils.synthesizeMouseAtCenter(mainButton, {});
+    }),
+  ]);
  Assert.equal(gURLBar.value, TIP_URL, "Should have navigated to the tip URL.");

+  // Click inside the tip but outside the buttons.  Nothing should happen.  Make
+  // the result the heuristic to check that the selection on the main button
+  // isn't lost.
+  matches[0].heuristic = true;
+  await UrlbarTestUtils.promiseAutocompleteResultPopup({
+    value: "test",
+    window,
+    waitForFocus,
+  });
+  row = await UrlbarTestUtils.waitForAutocompleteResultAt(window, 0);
+  Assert.equal(
+    UrlbarTestUtils.getSelectedElementIndex(window),
+    0,
+    "The main button's index should be selected initially"
+  );
+  Assert.equal(
+    UrlbarTestUtils.getSelectedElement(window),
+    row._elements.get("tipButton"),
+    "The main button element should be selected initially"
+  );
+  EventUtils.synthesizeMouseAtCenter(row, {});
+  // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+  await new Promise(r => setTimeout(r, 500));
+  Assert.ok(gURLBar.view.isOpen, "The view should remain open");
+  Assert.equal(
+    UrlbarTestUtils.getSelectedElementIndex(window),
+    0,
+    "The main button's index should remain selected"
+  );
+  Assert.equal(
+    UrlbarTestUtils.getSelectedElement(window),
+    row._elements.get("tipButton"),
+    "The main button element should remain selected"
+  );
+  await UrlbarTestUtils.promisePopupClose(window);
+
  UrlbarProvidersManager.unregisterProvider(provider);
 });