Bug 1636583 - Split pickResult out of pickElement. r=adw
☠☠ backed out by 5d62e7f9ed7b ☠ ☠
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 21 May 2020 23:11:38 +0000
changeset 531606 9fd14281791905a085a85cf79de8a5f1c64bce4b
parent 531605 76186c5df6b85b0baa82e37a491fbe3253f105ec
child 531607 660b7de8921523970782ef9ae793293d960bde30
push id37441
push userapavel@mozilla.com
push dateFri, 22 May 2020 21:38:53 +0000
treeherdermozilla-central@d6abd35b54ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1636583
milestone78.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 1636583 - Split pickResult out of pickElement. r=adw This allows to pick a result without necessarily having a view element. Differential Revision: https://phabricator.services.mozilla.com/D75909
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/UrlbarView.jsm
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -471,68 +471,93 @@ class UrlbarInput {
     url = this._maybeCanonizeURL(event, url) || url.trim();
 
     this.controller.engagementEvent.record(event, {
       numChars,
       selIndex: this.view.selectedRowIndex,
       selType,
     });
 
+    let isValidUrl = false;
     try {
       new URL(url);
-    } catch (ex) {
-      // This is not a URL, so it must be a search or a keyword.
-
-      // TODO (Bug 1604927): If the urlbar results are restricted to a specific
-      // engine, here we must search with that specific engine; indeed the
-      // docshell wouldn't know about our engine restriction.
-      // Also remember to invoke this._recordSearch, after replacing url with
-      // the appropriate engine submission url.
+      isValidUrl = true;
+    } catch (ex) {}
+    if (isValidUrl) {
+      this._loadURL(url, where, openParams);
+      return;
+    }
 
-      let browser = this.window.gBrowser.selectedBrowser;
-      let lastLocationChange = browser.lastLocationChange;
-      UrlbarUtils.getShortcutOrURIAndPostData(url).then(data => {
-        // Because this happens asynchronously, we must verify that the browser
-        // location did not change in the meanwhile.
-        if (
-          where != "current" ||
-          browser.lastLocationChange == lastLocationChange
-        ) {
-          openParams.postData = data.postData;
-          openParams.allowInheritPrincipal = data.mayInheritPrincipal;
-          this._loadURL(data.url, where, openParams, null, browser);
-        }
-      });
-      // Bail out, because we will handle the _loadURL call asynchronously.
+    // This is not a URL and there's no selected element, because likely the
+    // view is closed, or paste&go was used.
+
+    // If we have a result for the current value, just use it.
+    if (this._resultForCurrentValue) {
+      this.pickResult(this._resultForCurrentValue, event);
       return;
     }
 
-    this._loadURL(url, where, openParams);
+    // Otherwise, fall back to an imperfect clone of what the heuristic result
+    // would do.
+    // TODO: Run a search to get just the heuristic result here, and then use
+    // pickResult.
+
+    // TODO (Bug 1604927): If the urlbar results are restricted to a specific
+    // engine, here we must search with that specific engine; indeed the
+    // docshell wouldn't know about our engine restriction.
+    // Also remember to invoke this._recordSearch, after replacing url with
+    // the appropriate engine submission url.
+
+    let browser = this.window.gBrowser.selectedBrowser;
+    let lastLocationChange = browser.lastLocationChange;
+    UrlbarUtils.getShortcutOrURIAndPostData(url).then(data => {
+      // Because this happens asynchronously, we must verify that the browser
+      // location did not change in the meanwhile.
+      if (
+        where != "current" ||
+        browser.lastLocationChange == lastLocationChange
+      ) {
+        openParams.postData = data.postData;
+        openParams.allowInheritPrincipal = data.mayInheritPrincipal;
+        this._loadURL(data.url, where, openParams, null, browser);
+      }
+    });
   }
 
   handleRevert() {
     this.window.gBrowser.userTypedValue = null;
     this.setURI(null, true);
     if (this.value && this.focused) {
       this.select();
     }
   }
 
   /**
-   * Called by the view when an element is picked.
+   * Called when an element of the view is picked.
    *
    * @param {Element} element The element that was picked.
    * @param {Event} event The event that picked the element.
    */
   pickElement(element, event) {
-    let originalUntrimmedValue = this.untrimmedValue;
     let result = this.view.getResultFromElement(element);
     if (!result) {
       return;
     }
+    this.pickResult(result, event, element);
+  }
+
+  /**
+   * Called when a result is picked.
+   *
+   * @param {UrlbarResult} result The result that was picked.
+   * @param {Event} event The event that picked the result.
+   * @param {DOMElement} element the picked view element, if available.
+   */
+  pickResult(result, event, element = null) {
+    let originalUntrimmedValue = this.untrimmedValue;
     let isCanonized = this.setValueFromResult(result, event);
     let where = this._whereToOpen(event);
     let openParams = {
       allowInheritPrincipal: false,
     };
 
     let selIndex = result.rowIndex;
     if (!result.payload.keywordOffer) {
@@ -876,31 +901,35 @@ class UrlbarInput {
       return;
     }
 
     this.setValueFromResult(result);
   }
 
   /**
    * Invoked by the view when the first result is received.
-   * To prevent selection flickering, we apply autofill on input through a
-   * placeholder, without waiting for results.
-   * But, if the first result is not an autofill one, the autofill prediction
-   * was wrong and we should restore the original user typed string.
    * @param {UrlbarResult} firstResult The first result received.
    */
-  maybeClearAutofillPlaceholder(firstResult) {
+  onFirstResult(firstResult) {
+    // To prevent selection flickering, we apply autofill on input through a
+    // placeholder, without waiting for results. But, if the first result is
+    // not an autofill one, the autofill prediction was wrong and we should
+    // restore the original user typed string.
     if (
       this._autofillPlaceholder &&
       !firstResult.autofill &&
       // Avoid clobbering added spaces (for token aliases, for example).
       !this.value.endsWith(" ")
     ) {
       this._setValue(this.window.gBrowser.userTypedValue, false);
     }
+
+    if (firstResult.heuristic) {
+      this._resultForCurrentValue = firstResult;
+    }
   }
 
   /**
    * Starts a query based on the current input value.
    *
    * @param {boolean} [options.allowAutofill]
    *   Whether or not to allow providers to include autofill results.
    * @param {boolean} [options.autofillIgnoresSelection]
--- a/browser/components/urlbar/UrlbarView.jsm
+++ b/browser/components/urlbar/UrlbarView.jsm
@@ -505,19 +505,18 @@ class UrlbarView {
       let trimmedValue = queryContext.searchString.trim();
       this._enableOrDisableOneOffSearches(
         trimmedValue &&
           trimmedValue[0] != "@" &&
           (trimmedValue[0] != UrlbarTokenizer.RESTRICT.SEARCH ||
             trimmedValue.length != 1)
       );
 
-      // The input field applies autofill on input, without waiting for results.
-      // Once we get results, we can ask it to correct wrong predictions.
-      this.input.maybeClearAutofillPlaceholder(firstResult);
+      // Notify the input, so it can make adjustments based on the first result.
+      this.input.onFirstResult(firstResult);
     }
 
     if (
       firstResult.heuristic &&
       !this.selectedElement &&
       !this.oneOffSearchButtons.selectedButton
     ) {
       // Select the heuristic result.  The heuristic may not be the first result