Bug 1545742 - Quantumbar: Remove the proper nsIController on uninit and customize start so that left/right arrow and home/end keys work after exiting customize mode. r=mak
authorDrew Willcoxon <adw@mozilla.com>
Fri, 03 May 2019 16:03:00 +0000
changeset 531331 1963ed5da9820e633eae778edd690ed5c529ee31
parent 531330 85a694fb6093e78954c92029dc0152f75358d1f1
child 531332 42a61320b49b43123e2c71770d9b32cdb15fc6a0
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1545742
milestone68.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 1545742 - Quantumbar: Remove the proper nsIController on uninit and customize start so that left/right arrow and home/end keys work after exiting customize mode. r=mak When UrlbarInput.uninit is called after customize mode ends, uninit calls this.inputField.controllers.removeControllerAt(0), which is supposed to remove the input's CopyCutController inserted in the constructor. But the controller at index 0 at that point is not the CopyCutController. Instead it's some built-in controller that supports these commands (at least these): cmd_charPrevious, cmd_charPrevious, cmd_beginLine, cmd_endLine. (Verified by adding logging to nsXULControllers::GetControllerForCommand.) That's why arrow left/right and home/end don't work after ending customize mode. The problem is that this.inputField.controllers in the constructor and this.inputField.controllers in uninit (when customize mode ends) are not the same. I wasn't able to track down why, but I'm guessing that the textbox or something in its state is being reset or cloned when customized mode ends or maybe right after it starts. The CopyCutController isn't in the controllers array at all on uninit. (Verified by adding support for cmd_adw and iterating through the controllers array, looking for a controller supporting cmd_adw.) Note that urlbarBindings.xml has a try-catch around removeController(), I'm guessing for what turns out to be this reason: https://searchfox.org/mozilla-central/rev/7944190ad1668a94223b950a19f1fffe8662d6b8/browser/base/content/urlbarBindings.xml#190 However, CopyCutController *is* in the controllers array when customize mode starts. So I added a new gURLBarHandler.customizeStart method that calls a new UrlbarInput.removeCopyCutController method. Other things I tried or thought of doing: Call gURLBarHandler._reset on customize start instead of end. Problem with that is that the UrlbarInput ends up getting immediately recreated because some other parts of the browser access gURLBar at that time. (Of course I replaced the `gURLBar = this.urlbar` assignment in _reset with another lazy getter definition.) Just don't worry about removing CopyCutController at all. That seems bad because then we'd leak it, unless the controller is removed or the controllers array is emptied at some point by XUL, and I'm not at all certain about that. (Although I guess this is effectively what awesomebar does, given the link above!) Differential Revision: https://phabricator.services.mozilla.com/D29613
browser/base/content/browser-customization.js
browser/base/content/browser.js
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/tests/browser/browser.ini
browser/components/urlbar/tests/browser/browser_customizeMode.js
--- a/browser/base/content/browser-customization.js
+++ b/browser/base/content/browser-customization.js
@@ -34,16 +34,18 @@ var CustomizationHandler = {
       childNode.setAttribute("disabled", true);
 
     let cmd = document.getElementById("cmd_CustomizeToolbars");
     cmd.setAttribute("disabled", "true");
 
     UpdateUrlbarSearchSplitterState();
 
     PlacesToolbarHelper.customizeStart();
+
+    gURLBarHandler.customizeStart();
   },
 
   _customizationEnding(aDetails) {
     // Update global UI elements that may have been added or removed
     if (aDetails.changed &&
         AppConstants.platform != "macosx") {
       updateEditUIVisibility();
     }
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -235,16 +235,25 @@ var gURLBarHandler = {
    * Invoked when the quantumbar pref changes.
    */
   handlePrefChange() {
     this._updateBinding();
     this._reset();
   },
 
   /**
+   * Invoked by CustomizationHandler when a customization starts.
+   */
+  customizeStart() {
+    if (this._urlbar && this._urlbar.constructor.name == "UrlbarInput") {
+      this._urlbar.removeCopyCutController();
+    }
+  },
+
+  /**
    * Invoked by CustomizationHandler when a customization ends.
    */
   customizeEnd() {
     this._reset();
   },
 
   /**
    * Rebuilds the textbox binding by detaching the element when necessary.
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -161,17 +161,19 @@ class UrlbarInput {
     for (let name of this._inputFieldEvents) {
       this.inputField.addEventListener(name, this);
     }
 
     this.addEventListener("mousedown", this);
     this.view.panel.addEventListener("popupshowing", this);
     this.view.panel.addEventListener("popuphidden", this);
 
-    this.inputField.controllers.insertControllerAt(0, new CopyCutController(this));
+    this._copyCutController = new CopyCutController(this);
+    this.inputField.controllers.insertControllerAt(0, this._copyCutController);
+
     this._initPasteAndGo();
 
     // Tracks IME composition.
     this._compositionState = UrlbarUtils.COMPOSITION.NONE;
     this._compositionClosedPopup = false;
   }
 
   /**
@@ -182,33 +184,57 @@ class UrlbarInput {
       this.inputField.removeEventListener(name, this);
     }
     this.removeEventListener("mousedown", this);
 
     this.editor.removeEditActionListener(this);
 
     this.view.panel.remove();
 
-    this.inputField.controllers.removeControllerAt(0);
+    // When uninit is called due to exiting the browser's customize mode,
+    // this.inputField.controllers is not the original list of controllers, and
+    // it doesn't contain CopyCutController.  That's why removeCopyCutController
+    // must be called when entering customize mode.  If uninit ends up getting
+    // called by something else though, try to remove the controller now.
+    try {
+      // If removeCopyCutController throws, then the controller isn't in the
+      // list of the input's controllers, and the consumer should have called
+      // removeCopyCutController at some earlier point, e.g., when customize
+      // mode was entered.
+      this.removeCopyCutController();
+    } catch (ex) {
+      Cu.reportError("Leaking UrlbarInput._copyCutController! You should have called removeCopyCutController!");
+    }
 
     if (Object.getOwnPropertyDescriptor(this, "valueFormatter").get) {
       this.valueFormatter.uninit();
     }
 
     delete this.document;
     delete this.window;
     delete this.eventBufferer;
     delete this.valueFormatter;
     delete this.panel;
     delete this.view;
     delete this.controller;
     delete this.textbox;
   }
 
   /**
+   * Removes the CopyCutController from the input's controllers list.  This must
+   * be called when the browser's customize mode is entered.
+   */
+  removeCopyCutController() {
+    if (this._copyCutController) {
+      this.inputField.controllers.removeController(this._copyCutController);
+      delete this._copyCutController;
+    }
+  }
+
+  /**
    * Shortens the given value, usually by removing http:// and trailing slashes,
    * such that calling nsIURIFixup::createFixupURI with the result will produce
    * the same URI.
    *
    * @param {string} val
    *   The string to be trimmed if it appears to be URI
    * @returns {string}
    *   The trimmed string
--- a/browser/components/urlbar/tests/browser/browser.ini
+++ b/browser/components/urlbar/tests/browser/browser.ini
@@ -33,16 +33,17 @@ skip-if = os != "mac" # Mac only feature
 [browser_autoFill_trimURLs.js]
 [browser_autoFill_typed.js]
 [browser_autoFill_undo.js]
 [browser_canonizeURL.js]
 support-files =
   searchSuggestionEngine.xml
   searchSuggestionEngine.sjs
 [browser_caret_navigation.js]
+[browser_customizeMode.js]
 [browser_deleteAllText.js]
 [browser_downArrowKeySearch.js]
 [browser_dragdropURL.js]
 [browser_dropmarker.js]
 [browser_ime_composition.js]
 [browser_keepStateAcrossTabSwitches.js]
 [browser_keywordBookmarklets.js]
 [browser_keyword_override.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/urlbar/tests/browser/browser_customizeMode.js
@@ -0,0 +1,69 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// This test checks that the left/right arrow keys and home/end keys work in
+// the urlbar after customize mode starts and ends.
+
+"use strict";
+
+add_task(async function test() {
+  let win = await BrowserTestUtils.openNewBrowserWindow();
+  await startCustomizing(win);
+  await endCustomizing(win);
+
+  let urlbar = win.gURLBar;
+
+  let value = "example";
+  urlbar.value = value;
+  urlbar.focus();
+  urlbar.selectionEnd = value.length;
+  urlbar.selectionStart = value.length;
+
+  // left
+  EventUtils.synthesizeKey("KEY_ArrowLeft", {}, win);
+  Assert.equal(urlbar.selectionStart, value.length - 1);
+  Assert.equal(urlbar.selectionEnd, value.length - 1);
+
+  // home
+  if (AppConstants.platform == "macosx") {
+    EventUtils.synthesizeKey("KEY_ArrowLeft", { metaKey: true }, win);
+  } else {
+    EventUtils.synthesizeKey("KEY_Home", {}, win);
+  }
+  Assert.equal(urlbar.selectionStart, 0);
+  Assert.equal(urlbar.selectionEnd, 0);
+
+  // right
+  EventUtils.synthesizeKey("KEY_ArrowRight", {}, win);
+  Assert.equal(urlbar.selectionStart, 1);
+  Assert.equal(urlbar.selectionEnd, 1);
+
+  // end
+  if (AppConstants.platform == "macosx") {
+    EventUtils.synthesizeKey("KEY_ArrowRight", { metaKey: true }, win);
+  } else {
+    EventUtils.synthesizeKey("KEY_End", {}, win);
+  }
+  Assert.equal(urlbar.selectionStart, value.length);
+  Assert.equal(urlbar.selectionEnd, value.length);
+
+  await BrowserTestUtils.closeWindow(win);
+});
+
+async function startCustomizing(win = window) {
+  if (win.document.documentElement.getAttribute("customizing") != "true") {
+    let eventPromise =
+      BrowserTestUtils.waitForEvent(win.gNavToolbox, "customizationready");
+    win.gCustomizeMode.enter();
+    await eventPromise;
+  }
+}
+
+async function endCustomizing(win = window) {
+  if (win.document.documentElement.getAttribute("customizing") == "true") {
+    let eventPromise =
+      BrowserTestUtils.waitForEvent(win.gNavToolbox, "aftercustomization");
+    win.gCustomizeMode.exit();
+    await eventPromise;
+  }
+}