Bug 1505751 - Wait for menulist close for browser language handlers r=Gijs
authorMark Striemer <mstriemer@mozilla.com>
Wed, 21 Nov 2018 15:36:52 +0000
changeset 504052 c18244a415389d76f0d786313d0687ab1c7ebeeb
parent 504051 9b6242ab169a1ba89acedac1bd978661d97decce
child 504053 8d14b467d422dbea411f638e2740ae55527a174f
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1505751
milestone65.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 1505751 - Wait for menulist close for browser language handlers r=Gijs The search handler was being called when focusing the menuitem with the keyboard on Windows. This didn't provide a good experience and left the popup open once the search started. Ensure the popup is always shown when using the keyboard and don't trigger the search until the popup is closed. Differential Revision: https://phabricator.services.mozilla.com/D12324
browser/components/preferences/browserLanguages.js
browser/components/preferences/in-content/main.js
browser/components/preferences/in-content/main.xul
browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js
browser/modules/SelectionChangedMenulist.jsm
browser/modules/moz.build
--- a/browser/components/preferences/browserLanguages.js
+++ b/browser/components/preferences/browserLanguages.js
@@ -7,16 +7,18 @@
 ChromeUtils.import("resource://gre/modules/Services.jsm");
 
 ChromeUtils.defineModuleGetter(this, "AddonManager",
                                "resource://gre/modules/AddonManager.jsm");
 ChromeUtils.defineModuleGetter(this, "AddonRepository",
                                "resource://gre/modules/addons/AddonRepository.jsm");
 ChromeUtils.defineModuleGetter(this, "RemoteSettings",
                                "resource://services-settings/remote-settings.js");
+ChromeUtils.defineModuleGetter(this, "SelectionChangedMenulist",
+                               "resource:///modules/SelectionChangedMenulist.jsm");
 
 async function installFromUrl(url, hash) {
   let install = await AddonManager.getInstallForURL(
     url, "application/x-xpinstall", hash);
   return install.install();
 }
 
 async function dictionaryIdsForLocale(locale) {
@@ -155,17 +157,18 @@ class OrderedListBox {
 class SortedItemSelectList {
   constructor({menulist, button, onSelect, onChange, compareFn}) {
     this.menulist = menulist;
     this.popup = menulist.firstElementChild;
     this.button = button;
     this.compareFn = compareFn;
     this.items = [];
 
-    menulist.addEventListener("command", () => {
+    // This will register the "command" listener.
+    new SelectionChangedMenulist(this.menulist, () => {
       button.disabled = !menulist.selectedItem;
       if (menulist.selectedItem) {
         onChange(this.items[menulist.selectedIndex]);
       }
     });
     button.addEventListener("command", () => {
       if (!menulist.selectedItem) return;
 
--- a/browser/components/preferences/in-content/main.js
+++ b/browser/components/preferences/in-content/main.js
@@ -13,16 +13,18 @@ ChromeUtils.import("resource://gre/modul
 ChromeUtils.import("resource:///modules/ShellService.jsm");
 ChromeUtils.import("resource:///modules/TransientPrefs.jsm");
 ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
 ChromeUtils.import("resource://gre/modules/DownloadUtils.jsm");
 ChromeUtils.import("resource://gre/modules/L10nRegistry.jsm");
 ChromeUtils.import("resource://gre/modules/Localization.jsm");
 ChromeUtils.defineModuleGetter(this, "CloudStorage",
   "resource://gre/modules/CloudStorage.jsm");
+ChromeUtils.defineModuleGetter(this, "SelectionChangedMenulist",
+                               "resource:///modules/SelectionChangedMenulist.jsm");
 
 XPCOMUtils.defineLazyServiceGetters(this, {
   gAUS: ["@mozilla.org/updates/update-service;1", "nsIApplicationUpdateService"],
   gHandlerService: ["@mozilla.org/uriloader/handler-service;1", "nsIHandlerService"],
   gMIMEService: ["@mozilla.org/mime;1", "nsIMIMEService"],
 });
 
 // Constants & Enumeration Values
@@ -768,28 +770,30 @@ var gMainPane = {
 
     // Add an option to search for more languages if downloading is supported.
     if (Services.prefs.getBoolPref("intl.multilingual.downloadEnabled")) {
       let menuitem = document.createXULElement("menuitem");
       menuitem.id = "defaultBrowserLanguageSearch";
       menuitem.setAttribute(
         "label", await document.l10n.formatValue("browser-languages-search"));
       menuitem.setAttribute("value", "search");
-      menuitem.addEventListener("command", () => {
-        gMainPane.showBrowserLanguages({search: true});
-      });
       fragment.appendChild(menuitem);
     }
 
     let menulist = document.getElementById("defaultBrowserLanguage");
     let menupopup = menulist.querySelector("menupopup");
     menupopup.textContent = "";
     menupopup.appendChild(fragment);
     menulist.value = requesting;
 
+    // This will register the "command" listener.
+    new SelectionChangedMenulist(menulist, event => {
+      gMainPane.onBrowserLanguageChange(event);
+    });
+
     document.getElementById("browserLanguagesBox").hidden = false;
   },
 
   /* Show the confirmation message bar to allow a restart into the new locales. */
   async showConfirmLanguageChangeMessageBar(locales) {
     let messageBar = document.getElementById("confirmBrowserLanguage");
 
     // Get the bundle for the new locale.
@@ -860,16 +864,17 @@ var gMainPane = {
     }
   },
 
   /* Show or hide the confirm change message bar based on the new locale. */
   onBrowserLanguageChange(event) {
     let locale = event.target.value;
 
     if (locale == "search") {
+      gMainPane.showBrowserLanguages({search: true});
       return;
     } else if (locale == Services.locale.requestedLocale) {
       this.hideConfirmLanguageChangeMessageBar();
       return;
     }
 
     let locales = Array.from(new Set([
       locale,
--- a/browser/components/preferences/in-content/main.xul
+++ b/browser/components/preferences/in-content/main.xul
@@ -280,17 +280,17 @@
 
 <!-- Languages -->
 <groupbox id="languagesGroup" data-category="paneGeneral" hidden="true">
   <label><html:h2 data-l10n-id="language-header"/></label>
 
   <vbox id="browserLanguagesBox" align="start" hidden="true">
     <description flex="1" controls="chooseBrowserLanguage" data-l10n-id="choose-browser-language-description"/>
     <hbox>
-      <menulist id="defaultBrowserLanguage" oncommand="gMainPane.onBrowserLanguageChange(event)">
+      <menulist id="defaultBrowserLanguage">
         <menupopup/>
       </menulist>
       <button id="manageBrowserLanguagesButton"
               class="accessory-button"
               data-l10n-id="manage-browser-languages-button"
               oncommand="gMainPane.showBrowserLanguages({search: false})"/>
     </hbox>
   </vbox>
--- a/browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js
+++ b/browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js
@@ -143,16 +143,17 @@ function requestLocale(localeCode, avail
   available.selectedItem = locale;
   dialogDoc.getElementById("add").doCommand();
 }
 
 async function openDialog(doc, search = false) {
   let dialogLoaded = promiseLoadSubDialog(BROWSER_LANGUAGES_URL);
   if (search) {
     doc.getElementById("defaultBrowserLanguageSearch").doCommand();
+    doc.getElementById("defaultBrowserLanguage").firstElementChild.hidePopup();
   } else {
     doc.getElementById("manageBrowserLanguagesButton").doCommand();
   }
   let dialogWin = await dialogLoaded;
   let dialogDoc = dialogWin.document;
   return {
     dialog: dialogDoc.getElementById("BrowserLanguagesDialog"),
     dialogDoc,
new file mode 100644
--- /dev/null
+++ b/browser/modules/SelectionChangedMenulist.jsm
@@ -0,0 +1,32 @@
+/* - This Source Code Form is subject to the terms of the Mozilla Public
+   - License, v. 2.0. If a copy of the MPL was not distributed with this file,
+   - You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+var EXPORTED_SYMBOLS = ["SelectionChangedMenulist"];
+
+class SelectionChangedMenulist {
+  // A menulist wrapper that will open the popup when navigating with the
+  // keyboard on Windows and trigger the provided handler when the popup
+  // is hiding. This matches the behaviour of MacOS and Linux more closely.
+
+  constructor(menulist, onCommand) {
+    let popup = menulist.firstElementChild;
+    let lastEvent;
+
+    menulist.addEventListener("command", event => {
+      lastEvent = event;
+      if (popup.state != "open" && popup.state != "showing") {
+        popup.openPopup();
+      }
+    });
+
+    popup.addEventListener("popuphiding", () => {
+      if (lastEvent) {
+        onCommand(lastEvent);
+        lastEvent = null;
+      }
+    });
+  }
+}
--- a/browser/modules/moz.build
+++ b/browser/modules/moz.build
@@ -77,16 +77,19 @@ with Files("ProcessHangMonitor.jsm"):
     BUG_COMPONENT = ("Core", "DOM: Content Processes")
 
 with Files("ReaderParent.jsm"):
     BUG_COMPONENT = ("Toolkit", "Reader Mode")
 
 with Files("Sanitizer.jsm"):
     BUG_COMPONENT = ("Firefox", "Preferences")
 
+with Files("SelectionChangedMenulist.jsm"):
+    BUG_COMPONENT = ("Firefox", "Preferences")
+
 with Files("SiteDataManager.jsm"):
     BUG_COMPONENT = ("Firefox", "Preferences")
 
 with Files("SitePermissions.jsm"):
     BUG_COMPONENT = ("Firefox", "Site Identity and Permission Panels")
 
 with Files("TabsList.jsm"):
     BUG_COMPONENT = ("Firefox", "Tabbed Browser")
@@ -141,16 +144,17 @@ EXTRA_JS_MODULES += [
     'PageActions.jsm',
     'PermissionUI.jsm',
     'PingCentre.jsm',
     'ProcessHangMonitor.jsm',
     'ReaderParent.jsm',
     'RemotePrompt.jsm',
     'Sanitizer.jsm',
     'SchedulePressure.jsm',
+    'SelectionChangedMenulist.jsm',
     'SiteDataManager.jsm',
     'SitePermissions.jsm',
     'TabsList.jsm',
     'ThemeVariableMap.jsm',
     'TransientPrefs.jsm',
     'webrtcUI.jsm',
     'ZoomUI.jsm',
 ]