Bug 1641287: Focus on browser after keyup. r=mak
authorDaisuke Akatsuka <daisuke@birchill.co.jp>
Mon, 26 Oct 2020 05:30:44 +0000
changeset 554358 be354ee427d01400963bf0086058b63b9dfadc09
parent 554357 9cf73428357c3679bf6dfff0e1e0c1bc24d5c0e9
child 554359 b1a74943bc51bd3e62ea52242ec5e403ea3760bb
push id37893
push userbtara@mozilla.com
push dateMon, 26 Oct 2020 09:28:34 +0000
treeherdermozilla-central@b1a74943bc51 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1641287
milestone84.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 1641287: Focus on browser after keyup. r=mak Differential Revision: https://phabricator.services.mozilla.com/D92561
browser/base/content/utilityOverlay.js
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/tests/browser/browser_enter.js
--- a/browser/base/content/utilityOverlay.js
+++ b/browser/base/content/utilityOverlay.js
@@ -693,17 +693,21 @@ function openLinkIn(url, where, params) 
             },
           },
           "webNavigation-createdNavigationTarget"
         );
       }
       break;
   }
 
-  if (!focusUrlBar && targetBrowser == w.gBrowser.selectedBrowser) {
+  if (
+    !params.avoidBrowserFocus &&
+    !focusUrlBar &&
+    targetBrowser == w.gBrowser.selectedBrowser
+  ) {
     // Focus the content, but only if the browser used for the load is selected.
     targetBrowser.focus();
   }
 }
 
 // Used as an onclick handler for UI elements with link-like behavior.
 // e.g. onclick="checkForMiddleClick(this, event);"
 function checkForMiddleClick(node, event) {
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -12,16 +12,17 @@ const { XPCOMUtils } = ChromeUtils.impor
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   AppConstants: "resource://gre/modules/AppConstants.jsm",
   BrowserUsageTelemetry: "resource:///modules/BrowserUsageTelemetry.jsm",
   BrowserUtils: "resource://gre/modules/BrowserUtils.jsm",
   ExtensionSearchHandler: "resource://gre/modules/ExtensionSearchHandler.jsm",
   ObjectUtils: "resource://gre/modules/ObjectUtils.jsm",
   PrivateBrowsingUtils: "resource://gre/modules/PrivateBrowsingUtils.jsm",
+  PromiseUtils: "resource://gre/modules/PromiseUtils.jsm",
   ReaderMode: "resource://gre/modules/ReaderMode.jsm",
   PartnerLinkAttribution: "resource:///modules/PartnerLinkAttribution.jsm",
   SearchUtils: "resource://gre/modules/SearchUtils.jsm",
   Services: "resource://gre/modules/Services.jsm",
   UrlbarController: "resource:///modules/UrlbarController.jsm",
   UrlbarEventBufferer: "resource:///modules/UrlbarEventBufferer.jsm",
   UrlbarPrefs: "resource:///modules/UrlbarPrefs.jsm",
   UrlbarProvidersManager: "resource:///modules/UrlbarProvidersManager.jsm",
@@ -552,17 +553,17 @@ class UrlbarInput {
     });
 
     let isValidUrl = false;
     try {
       new URL(url);
       isValidUrl = true;
     } catch (ex) {}
     if (isValidUrl) {
-      this._loadURL(url, where, openParams);
+      this._loadURL(url, event, where, openParams);
       return;
     }
 
     // This is not a URL and there's no selected element, because likely the
     // view is closed, or paste&go was used.
     // We must act consistently here, having or not an open view should not
     // make a difference if the search string is the same.
 
@@ -608,17 +609,17 @@ class UrlbarInput {
             preferredURI: uri,
             postData,
           } = Services.uriFixup.getFixupURIInfo(url, flags);
           if (
             where != "current" ||
             browser.lastLocationChange == lastLocationChange
           ) {
             openParams.postData = postData;
-            this._loadURL(uri.spec, where, openParams, null, browser);
+            this._loadURL(uri.spec, event, where, openParams, null, browser);
           }
         }
       });
     // Don't add further handling here, the catch above is our last resort.
   }
 
   handleRevert() {
     this.window.gBrowser.userTypedValue = null;
@@ -691,17 +692,17 @@ class UrlbarInput {
 
     if (isCanonized) {
       this.controller.engagementEvent.record(event, {
         searchString: this._lastSearchString,
         selIndex,
         selType: "canonized",
         provider: result.providerName,
       });
-      this._loadURL(this.value, where, openParams, browser);
+      this._loadURL(this.value, event, where, openParams, browser);
       return;
     }
 
     let { url, postData } = UrlbarUtils.getUrlFromResult(result);
     openParams.postData = postData;
 
     switch (result.type) {
       case UrlbarUtils.RESULT_TYPE.URL: {
@@ -945,16 +946,17 @@ class UrlbarInput {
       searchString: this._lastSearchString,
       selIndex,
       selType: this.controller.engagementEvent.typeFromElement(element),
       provider: result.providerName,
     });
 
     this._loadURL(
       url,
+      event,
       where,
       openParams,
       {
         source: result.source,
         type: result.type,
       },
       browser
     );
@@ -2261,16 +2263,18 @@ class UrlbarInput {
     this._autofillPlaceholder = value;
   }
 
   /**
    * Loads the url in the appropriate place.
    *
    * @param {string} url
    *   The URL to open.
+   * @param {Event} event
+   *   The event that triggered to load the url.
    * @param {string} openUILinkWhere
    *   Where we expect the result to be opened.
    * @param {object} params
    *   The parameters related to how and where the result will be opened.
    *   Further supported paramters are listed in utilityOverlay.js#openUILinkIn.
    * @param {object} params.triggeringPrincipal
    *   The principal that the action was triggered from.
    * @param {nsIInputStream} [params.postData]
@@ -2282,16 +2286,17 @@ class UrlbarInput {
    * @param {UrlbarUtils.RESULT_TYPE} [result.type]
    *   Details of the result type, if any.
    * @param {UrlbarUtils.RESULT_SOURCE} [result.source]
    *   Details of the result source, if any.
    * @param {object} browser [optional] the browser to use for the load.
    */
   _loadURL(
     url,
+    event,
     openUILinkWhere,
     params,
     resultDetails = null,
     browser = this.window.gBrowser.selectedBrowser
   ) {
     // No point in setting these because we'll handleRevert() a few rows below.
     if (openUILinkWhere == "current") {
       this.value = url;
@@ -2340,20 +2345,29 @@ class UrlbarInput {
       params.targetBrowser = browser;
       params.indicateErrorPageLoad = true;
       params.allowPinnedTabHostChange = true;
       params.allowPopups = url.startsWith("javascript:");
     } else {
       params.initiatingDoc = this.window.document;
     }
 
+    if (event?.keyCode === KeyEvent.DOM_VK_RETURN) {
+      if (openUILinkWhere === "current") {
+        params.avoidBrowserFocus = true;
+        this._keyDownEnterDeferred?.resolve(browser);
+      }
+    }
+
     // Focus the content area before triggering loads, since if the load
     // occurs in a new tab, we want focus to be restored to the content
     // area when the current tab is re-selected.
-    browser.focus();
+    if (!params.avoidBrowserFocus) {
+      browser.focus();
+    }
 
     if (openUILinkWhere != "current") {
       this.handleRevert();
     }
 
     // Notify about the start of navigation.
     this._notifyStartNavigation(resultDetails);
 
@@ -2699,16 +2713,23 @@ class UrlbarInput {
     // We may have hidden popup notifications, show them again if necessary.
     if (
       this.getAttribute("pageproxystate") != "valid" &&
       this.window.UpdatePopupNotificationsVisibility
     ) {
       this.window.UpdatePopupNotificationsVisibility();
     }
 
+    // If user move the focus to another component while pressing Enter key,
+    // then keyup at that component, as we can't get the event, clear the promise.
+    if (this._keyDownEnterDeferred) {
+      this._keyDownEnterDeferred.resolve();
+      this._keyDownEnterDeferred = null;
+    }
+
     Services.obs.notifyObservers(null, "urlbar-blur");
   }
 
   _on_click(event) {
     if (
       event.target == this.inputField ||
       event.target == this._inputContainer ||
       event.target.id == SEARCH_BUTTON_ID
@@ -3025,33 +3046,58 @@ class UrlbarInput {
   }
 
   _on_TabSelect(event) {
     this._gotTabSelect = true;
     this._afterTabSelectAndFocusChange();
   }
 
   _on_keydown(event) {
+    if (event.keyCode === KeyEvent.DOM_VK_RETURN) {
+      if (this._keyDownEnterDeferred) {
+        this._keyDownEnterDeferred.reject();
+      }
+      this._keyDownEnterDeferred = PromiseUtils.defer();
+    }
+
     // Due to event deferring, it's possible preventDefault() won't be invoked
     // soon enough to actually prevent some of the default behaviors, thus we
     // have to handle the event "twice". This first immediate call passes false
     // as second argument so that handleKeyNavigation will only simulate the
     // event handling, without actually executing actions.
     // TODO (Bug 1541806): improve this handling, maybe by delaying actions
     // instead of events.
     if (this.eventBufferer.shouldDeferEvent(event)) {
       this.controller.handleKeyNavigation(event, false);
     }
     this._toggleActionOverride(event);
     this.eventBufferer.maybeDeferEvent(event, () => {
       this.controller.handleKeyNavigation(event);
     });
   }
 
-  _on_keyup(event) {
+  async _on_keyup(event) {
+    if (
+      event.keyCode === KeyEvent.DOM_VK_RETURN &&
+      this._keyDownEnterDeferred
+    ) {
+      try {
+        const loadingBrowser = await this._keyDownEnterDeferred.promise;
+        // Ensure the selected browser didn't change in the meanwhile.
+        if (this.window.gBrowser.selectedBrowser === loadingBrowser) {
+          loadingBrowser.focus();
+        }
+      } catch (ex) {
+        // Not all the Enter actions in the urlbar will cause a navigation, then it
+        // is normal for this to be rejected.
+      }
+      this._keyDownEnterDeferred = null;
+      return;
+    }
+
     this._toggleActionOverride(event);
   }
 
   _on_compositionstart(event) {
     if (this._compositionState == UrlbarUtils.COMPOSITION.COMPOSING) {
       throw new Error("Trying to start a nested composition?");
     }
     this._compositionState = UrlbarUtils.COMPOSITION.COMPOSING;
--- a/browser/components/urlbar/tests/browser/browser_enter.js
+++ b/browser/components/urlbar/tests/browser/browser_enter.js
@@ -1,16 +1,29 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 const TEST_VALUE = "example.com/\xF7?\xF7";
 const START_VALUE = "example.com/%C3%B7?%C3%B7";
 
+add_task(async function setup() {
+  const engine = await SearchTestUtils.promiseNewSearchEngine(
+    getRootDirectory(gTestPath) + "searchSuggestionEngine.xml"
+  );
+
+  const defaultEngine = Services.search.defaultEngine;
+  Services.search.defaultEngine = engine;
+
+  registerCleanupFunction(async function() {
+    Services.search.defaultEngine = defaultEngine;
+  });
+});
+
 add_task(async function returnKeypress() {
   info("Simple return keypress");
   let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, START_VALUE);
 
   gURLBar.focus();
   EventUtils.synthesizeKey("KEY_Enter");
   await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
 
@@ -77,33 +90,25 @@ add_task(async function altGrReturnKeypr
 
   // Cleanup.
   BrowserTestUtils.removeTab(tab);
   BrowserTestUtils.removeTab(gBrowser.selectedTab);
 });
 
 add_task(async function searchOnEnterNoPick() {
   info("Search on Enter without picking a urlbar result");
-  let engine = await SearchTestUtils.promiseNewSearchEngine(
-    getRootDirectory(gTestPath) + "searchSuggestionEngine.xml"
-  );
-  let defaultEngine = Services.search.defaultEngine;
-  Services.search.defaultEngine = engine;
+
   // Why is BrowserTestUtils.openNewForegroundTab not causing the bug?
   let promiseTabOpened = BrowserTestUtils.waitForEvent(
     gBrowser.tabContainer,
     "TabOpen"
   );
   EventUtils.synthesizeMouseAtCenter(gBrowser.tabContainer.newTabButton, {});
   let openEvent = await promiseTabOpened;
   let tab = openEvent.target;
-  registerCleanupFunction(async function() {
-    Services.search.defaultEngine = defaultEngine;
-    BrowserTestUtils.removeTab(tab);
-  });
 
   let loadPromise = BrowserTestUtils.browserLoaded(
     gBrowser.selectedBrowser,
     false,
     null,
     true
   );
   gURLBar.focus();
@@ -115,9 +120,82 @@ add_task(async function searchOnEnterNoP
     gBrowser.selectedBrowser.currentURI.spec.endsWith("test+test"),
     "Should have loaded the correct page"
   );
   Assert.equal(
     gBrowser.selectedBrowser.currentURI.spec,
     gURLBar.untrimmedValue,
     "The location should have changed"
   );
+
+  // Cleanup.
+  BrowserTestUtils.removeTab(tab);
 });
+
+add_task(async function searchOnEnterSoon() {
+  info("Search on Enter as soon as typing a char");
+  const tab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    START_VALUE
+  );
+
+  const onLoad = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
+  const onBeforeUnload = SpecialPowers.spawn(
+    gBrowser.selectedBrowser,
+    [],
+    () => {
+      return new Promise(resolve => {
+        content.window.addEventListener("beforeunload", () => {
+          resolve();
+        });
+      });
+    }
+  );
+  const onResult = SpecialPowers.spawn(gBrowser.selectedBrowser, [], () => {
+    return new Promise(resolve => {
+      content.window.addEventListener("keyup", () => {
+        resolve("keyup");
+      });
+      content.window.addEventListener("unload", () => {
+        resolve("unload");
+      });
+    });
+  });
+
+  // Focus on the input field in urlbar.
+  EventUtils.synthesizeMouseAtCenter(gURLBar.inputField, {});
+  const ownerDocument = gBrowser.selectedBrowser.ownerDocument;
+  is(
+    ownerDocument.activeElement,
+    gURLBar.inputField,
+    "The input field in urlbar has focus"
+  );
+
+  info("Keydown a char and Enter");
+  EventUtils.synthesizeKey("x", { type: "keydown" });
+  EventUtils.synthesizeKey("KEY_Enter", { type: "keydown" });
+
+  // Wait for beforeUnload event in the content.
+  await onBeforeUnload;
+  is(
+    ownerDocument.activeElement,
+    gURLBar.inputField,
+    "The input field in urlbar still has focus"
+  );
+
+  // Keyup both key as soon as beforeUnload event happens.
+  EventUtils.synthesizeKey("x", { type: "keyup" });
+  EventUtils.synthesizeKey("KEY_Enter", { type: "keyup" });
+
+  // Wait for moving the focus.
+  await TestUtils.waitForCondition(
+    () => ownerDocument.activeElement === gBrowser.selectedBrowser
+  );
+  info("The focus is moved to the browser");
+
+  // Check whether keyup event is not captured before unload event happens.
+  const result = await onResult;
+  is(result, "unload", "Keyup event is not captured.");
+
+  // Cleanup.
+  await onLoad;
+  BrowserTestUtils.removeTab(tab);
+});