bug 441794 — User configurable keyboard shortcuts: review comments from mfinkle

mobile/chrome/content/browser-ui.js

@@ -378,6 +378,19 @@ var BrowserUI = {
       sidebar.left = newLeft + tabbarW + browserW;
       panelUI.left = newLeft + tabbarW + browserW + sidebarW;
       panelUI.width = browserW;
+
+      if (aMode == UIMODE_NONE)
+        Shortcuts.deinit();
+      else
+      {
+        let addons = document.getElementById("addons-container");
+        if (addons.getAttribute("src") == "")
+          addons.setAttribute("src", "chrome://mozapps/content/extensions/extensions.xul");
+        let dloads = document.getElementById("downloads-container");
+        if (dloads.getAttribute("src") == "")
+          dloads.setAttribute("src", "chrome://mozapps/content/downloads/downloads.xul");
+        Shortcuts.init();
+      }
   },
 
   _sizeControls : function(aEvent) {
@@ -604,7 +617,6 @@ var BrowserUI = {
       urllist.hidden = true;
 
       this._showPanel(UIMODE_NONE);
-      Shortcuts.dismiss();
     }
     else if (aMode == UIMODE_URLEDIT) {
       this._showToolbar(true);
@@ -614,7 +626,6 @@ var BrowserUI = {
       urllist.hidden = true;
 
       this._showPanel(UIMODE_NONE);
-      Shortcuts.dismiss();
     }
     else if (aMode == UIMODE_BOOKMARK) {
       this._showToolbar(true);
@@ -625,7 +636,6 @@ var BrowserUI = {
       bookmark.width = container.boxObject.width;
 
       this._showPanel(UIMODE_NONE);
-      Shortcuts.dismiss();
     }
     else if (aMode == UIMODE_BOOKMARKLIST) {
       this._showToolbar(false);
@@ -639,7 +649,6 @@ var BrowserUI = {
       urllist.height = container.boxObject.height;
 
       this._showPanel(UIMODE_NONE);
-      Shortcuts.dismiss();
     }
     else if (aMode == UIMODE_PANEL) {
       this._showToolbar(true);
@@ -647,14 +656,6 @@ var BrowserUI = {
 
       bookmark.hidden = true;
 
-      let addons = document.getElementById("addons-container");
-      if (addons.getAttribute("src") == "")
-        addons.setAttribute("src", "chrome://mozapps/content/extensions/extensions.xul");
-      let dloads = document.getElementById("downloads-container");
-      if (dloads.getAttribute("src") == "")
-        dloads.setAttribute("src", "chrome://mozapps/content/downloads/downloads.xul");
-      Shortcuts.edit();
-
       this._showPanel(aMode);
     }
     else if (aMode == UIMODE_TABS || aMode == UIMODE_CONTROLS) {
@@ -666,7 +667,6 @@ var BrowserUI = {
       urllist.hidden = true;
       bookmark.hidden = true;
       this._showPanel(aMode);
-      Shortcuts.dismiss();
     }
   },
 
@@ -800,7 +800,6 @@ var BrowserUI = {
       case "cmd_go":
       case "cmd_star":
       case "cmd_bookmarks":
-      case "cmd_shortcuts":
       case "cmd_menu":
       case "cmd_newTab":
       case "cmd_closeTab":
@@ -869,10 +868,6 @@ var BrowserUI = {
       case "cmd_bookmarks":
         this.showBookmarks();
         break;
-      case "cmd_shortcuts":
-        this.show(PANELMODE_NONE);
-        Shortcuts.edit();
-        break;
       case "cmd_menu":
         break;
       case "cmd_newTab":

mobile/chrome/content/shortcuts.js

@@ -191,7 +191,7 @@ function ShortcutEditor()
             return 0;
 
         var result;
-        for each (m in modifiers.split(" "))
+        for each (m in modifiers.split(/,\s*|\s+/))
             result |= modifierFlags[m];
         return result;
     }
@@ -408,9 +408,11 @@ function ShortcutEditor()
         }
     }
 
-    // show the window
-    this.edit = function()
+    this.init = function()
     {
+        if (tree)
+            return; // we've already been called
+
         tree = document.getElementById("shortcuts");
 
         var textbox = document.getAnonymousElementByAttribute(tree, "anonid", "input");
@@ -431,16 +433,16 @@ function ShortcutEditor()
         keyCache = undefined;
     }
 
-    this.dismiss = function()
+    this.deinit = function()
     {
         if (!tree)
-            return;
+            return; // got called early, just return
 
         hack();
         var textbox = document.getAnonymousElementByAttribute(tree, "anonid", "input");
         textbox.removeEventListener("keypress", keyListener, true);
         textbox.removeEventListener("reset", resetListener, true);
-        document.getElementById("shortcuts-container").hidden = true;
+        tree.removeEventListener("DOMAttrModified", modificationListener, true);
     };
 
     // also, updating the UI is helpful
@@ -547,10 +549,10 @@ function ShortcutEditor()
          [["control", "a"],                               {exists: true,  modifiers: 2,  key: "a",   keycode: false},      "Ctrl+A"],
          [["meta", "a"],                                  {exists: true,  modifiers: 4,  key: "a",   keycode: false},      "Meta+A"],
          [["shift", "a"],                                 {exists: true,  modifiers: 8,  key: "a",   keycode: false},      "Shift+A"],
-         [["control alt", "a"],                           {exists: true,  modifiers: 3,  key: "a",   keycode: false},      "Ctrl+Alt+A"],
-         [["alt shift", "a"],                             {exists: true,  modifiers: 9,  key: "a",   keycode: false},      "Alt+Shift+A"],
-         [["shift meta", "a"],                            {exists: true,  modifiers: 12, key: "a",   keycode: false},      "Meta+Shift+A"],
-         [["control alt shift", "a"],                     {exists: true,  modifiers: 11, key: "a",   keycode: false},      "Ctrl+Alt+Shift+A"],
+         [["control,alt", "a"],                           {exists: true,  modifiers: 3,  key: "a",   keycode: false},      "Ctrl+Alt+A"],
+         [["alt, shift", "a"],                            {exists: true,  modifiers: 9,  key: "a",   keycode: false},      "Alt+Shift+A"],
+         [["shift ,meta", "a"],                           {exists: true,  modifiers: 12, key: "a",   keycode: false},      "Meta+Shift+A"],
+         [["control , alt shift", "a"],                   {exists: true,  modifiers: 11, key: "a",   keycode: false},      "Ctrl+Alt+Shift+A"],
          [["alt shift meta", "a"],                        {exists: true,  modifiers: 13, key: "a",   keycode: false},      "Alt+Meta+Shift+A"],
          [[undefined, undefined, "VK_BACK"],              {exists: true,  modifiers: 0,  key: false, keycode: "VK_BACK"},  "Backspace"],
          [["control", undefined, "VK_BACK"],              {exists: true,  modifiers: 2,  key: false, keycode: "VK_BACK"},  "Ctrl+Backspace"],

mobile/chrome/locale/en-US/shortcuts.properties

@@ -11,8 +11,6 @@ cmd_findAgain.name=Find again
 cmd_findPrevious.name=Find previous
 cmd_menu.name=Show menu
 cmd_fullscreen.name=Use full screen
-cmd_addons.name=Show addons list
-cmd_downloads.name=Show downloads list
 cmd_scrollPageUp.name=Page up
 cmd_scrollToBeginning.name=Scroll to beginning
 cmd_scrollPageDown.name=Page down
@@ -23,7 +21,6 @@ cmd_copylink.name=Copy link URL
 cmd_paste.name=Paste
 cmd_delete.name=Delete
 cmd_selectAll.name=Select All
-cmd_shortcuts.name=Edit shortcuts
 cmd_newTab.name=Open new tab
 cmd_closeTab.name=Close current tab
 cmd_actions.name=Open Actions list