441794 - User configurable keyboard shortcuts

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P3)

Description

[Christian Sejersen]

Allow the user to change the keyboard shortcuts to their liking.

Comment 1

[Daniel Brooks [:db48x]]

http://db48x.net/hg/moz/441794/

Comment 2

[Daniel Brooks [:db48x]]

Ok, this is close. It needs bug 437640 to be checked in, but otherwise it should function correctly. There are only a few things missing: there is no way to clear a shortcut, there is no support for multiple shortcuts for the same command, and I need to add a button or something to the prefs that opens this "window" (currently there's a button on the side toolbar.)

Comment 3

[Daniel Brooks [:db48x]]

Attachment: 441794-1.hg

hg bundle of changes

Comment 4

[Daniel Brooks [:db48x]]

Attachment: 441794-2.hg

Ok, this is essentially done. We don't yet have a preferences window to add it to, so for now it's just on the secondary toolbar. You can just pull from my repository at http://db48x.net/hg/hgwebdir.cgi/moz/441794/ if you don't prefer to use the bundle.

Comment 5

[Daniel Brooks [:db48x]]

Just so that you know, the code in my repository is more up to date than the one in the bundle.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Next time, use real patches. Reviewing and commenting is much harder from a repository or bundle.

-- shortcut.js --

The overall coding structure used in shortcut.js is not typically of the rest of the application. Not saying it is better or worse, but it could make maintenance a problem for other programmers. Consider this next time.

85      function findCommandForKey(keySpec)

This function does not appear to return a <command>. It appears to return a <key>. It also fails to return anything if the condition in the for loop is not met. Explicitly returning null at the bottom of the function might be safer.

522     this.test = function()

Probably want this to move to a separate unit test before landing

-- browser.xul -- 
  67     <stringbundle id="bundle_browser" src="chrome://browser/locale/browser.properties"/>
  68     <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
+ 69     <stringbundle id="bundle-keys" src="chrome://global/locale/keys.properties"/>
+ 70     <stringbundle id="bundle-platformKeys" src="chrome://global-platform/locale/platformKeys.properties"/>

please use the same id strucutre as the existing bundles. I know we are not consistent in this file, but this is too close to let go.

73   <stringbundleset id="shortcut-bundles">
74     <stringbundle src="chrome://browser/locale/shortcuts.properties"/>
75   </stringbundleset>
76 
77   <script type="application/x-javascript;version=1.8" src="chrome://browser/content/shortcuts.js"/>

Why make a new bundle? Why isn't the script with the other scripts?

291       <hbox pack="end">
292         <button label="Dismiss" oncommand="Shortcuts.dismiss();"/>
293       </hbox>

String should be in browser.dtd and we are using "Close" and close() for other helpers. I notice that the UI will save as the user edits. The bookmark editor doesn't work this way. We should get UX (madhava) to figure out the method we want to use so it's consistent (or at least expected).

-- browser-ui.js --

154     Shortcuts.test();

Remove before landing

577       case "cmd_shortcuts":
578         this.show(PANELMODE_NONE);
579         Shortcuts.edit();
580         break;

I'd like a PANEL_MODE for shortcuts, so it is consistent with the rest of the code. We can refactor the entire panel system later, but I want it to be consistent.


Those are the only files I checked. If changes were made to other files, please let me know - or better yet, add a real patch file.

Comment 7

[Daniel Brooks [:db48x]]

(In reply to comment #6)
> Next time, use real patches. Reviewing and commenting is much harder from a
> repository or bundle.

Indeed

> 85      function findCommandForKey(keySpec)
> 
> This function does not appear to return a <command>. It appears to return a
> <key>. It also fails to return anything if the condition in the for loop is not
> met. Explicitly returning null at the bottom of the function might be safer.

The name is just wrong. I'll add an explicit null to the other return though.

> 522     this.test = function()
> 
> Probably want this to move to a separate unit test before landing

Well, it needs to access functions that aren't "public", so I'll leave it here. We won't want to call it on startup, of course. However, as it's not currently possible to write mochitests for the Fennec chrome it'll have to wait.

> -- browser.xul -- 
>   67     <stringbundle id="bundle_browser"
> src="chrome://browser/locale/browser.properties"/>
>   68     <stringbundle id="bundle_brand"
> src="chrome://branding/locale/brand.properties"/>
> + 69     <stringbundle id="bundle-keys"
> src="chrome://global/locale/keys.properties"/>
> + 70     <stringbundle id="bundle-platformKeys"
> src="chrome://global-platform/locale/platformKeys.properties"/>
> 
> please use the same id strucutre as the existing bundles. I know we are not
> consistent in this file, but this is too close to let go.

Sure. I think the style of the previous entries actually changed while I was working on this code :)

> 73   <stringbundleset id="shortcut-bundles">
> 74     <stringbundle src="chrome://browser/locale/shortcuts.properties"/>
> 75   </stringbundleset>
> 76 
> 77   <script type="application/x-javascript;version=1.8"
> src="chrome://browser/content/shortcuts.js"/>
> 
> Why make a new bundle? Why isn't the script with the other scripts?

Why did I add a stringbundleset, you mean? My intention is to allow commands (and keys) added by extensions to show up in the editor as well. If the extension author overlays a new bundle into this stringbundleset, then the editor will automatically check it when it looks up the command names.

The script is here because it relies on having the platformKeys bundle loaded when it runs. 

> 
> 291       <hbox pack="end">
> 292         <button label="Dismiss" oncommand="Shortcuts.dismiss();"/>
> 293       </hbox>
> 
> String should be in browser.dtd and we are using "Close" and close() for other
> helpers. I notice that the UI will save as the user edits. The bookmark editor
> doesn't work this way. We should get UX (madhava) to figure out the method we
> want to use so it's consistent (or at least expected).

Ah, I'd forgotten about this. The closest I see in browser.dtd is "Close tab", but I'll look at the other code and see what it uses. I'll talk to madhava about consistency.

> 
> -- browser-ui.js --
> 
> 154     Shortcuts.test();
> 
> Remove before landing

done.

> 
> 577       case "cmd_shortcuts":
> 578         this.show(PANELMODE_NONE);
> 579         Shortcuts.edit();
> 580         break;
> 
> I'd like a PANEL_MODE for shortcuts, so it is consistent with the rest of the
> code. We can refactor the entire panel system later, but I want it to be
> consistent.

Sure.

> 
> 
> Those are the only files I checked. If changes were made to other files, please
> let me know - or better yet, add a real patch file.
> 

There's only the properties file for the string bundle I added.

Comment 8

[Daniel Brooks [:db48x]]

due to the change in design, it's not really dependent on the prefs pane.

Comment 9

[Daniel Brooks [:db48x]]

Attachment: 441794-3.diff

Updates it to work correctly with the panel UI.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 340547 [details] [diff] [review]
441794-3.diff

>diff -r 666d3fa016b8 -r b6754eb8bdf9 chrome/content/browser-ui.js

>     window.addEventListener("resize", this, false);
>+    Shortcuts.restore();
>+    Shortcuts.test();

Remove the test line, right?

>       this._showPanel(UIMODE_NONE);
>+      Shortcuts.dismiss();

Can we move these into _showPanel() ?

Also, do you need to be adding and removing the event listeners everytime the panel is shown/hidden? It's not being destroyed.

>     else if (aMode == UIMODE_PANEL) {
>       this._showToolbar(true);
>@@ -647,6 +653,7 @@
>       let dloads = document.getElementById("downloads-container");
>       if (dloads.getAttribute("src") == "")
>         dloads.setAttribute("src", "chrome://mozapps/content/downloads/downloads.xul");

I should have tried moving the above lines into _showPanel too.

> 
>@@ -792,6 +800,7 @@
>       case "cmd_go":
>       case "cmd_star":
>       case "cmd_bookmarks":
>+      case "cmd_shortcuts":

Remove. You don't need it anymore

>       case "cmd_bookmarks":
>         this.showBookmarks();
>+        break;
>+      case "cmd_shortcuts":
>+        this.show(PANELMODE_NONE);
>+        Shortcuts.edit();

Remove. You don't need it anymore. It's handled by the panel UI and the shortcut button.

>diff -r 666d3fa016b8 -r b6754eb8bdf9 chrome/content/browser.xul
>     <!-- scrolling -->
>     <command id="cmd_scrollPageUp" oncommand="CommandUpdater.doCommand(this.id);"/>
>+    <command id="cmd_scrollToBeginning" oncommand="CommandUpdater.doCommand(this.id);"/>
>     <command id="cmd_scrollPageDown" oncommand="CommandUpdater.doCommand(this.id);"/>
>+    <command id="cmd_scrollToEnd" oncommand="CommandUpdater.doCommand(this.id);"/>

What are these?

>diff -r 666d3fa016b8 -r b6754eb8bdf9 chrome/content/shortcuts.js

your TABs are set to 4 spaces in this file. We use 2 spaces.

>+    var modifierFlags = { alt: 1, control: 2, meta: 4, shift: 8 };
>+    function getFlagsForModifiers(modifiers)
>+    {
>+        if (!modifiers)
>+            return 0;
>+
>+        var result;
>+        for each (m in modifiers.split(" "))

modifiers could be separated by "," too, right? So we either need to handle that here or be ever vigilant that we don't add a comma sep'd modifier to our commands. I noticed you "fixed" one of the modifiers in browser.xul

I'd rather be robust here
>+    var keyCodeMap = { };
>+    var nsIDOMKeyEvent = Components.interfaces.nsIDOMKeyEvent;
>+    keyCodeMap[nsIDOMKeyEvent.DOM_VK_CANCEL] = "VK_CANCEL";
>+    ...
>+    keyCodeMap[nsIDOMKeyEvent.DOM_VK_META] = "VK_META";

Didn't you and Mook come up with a regex for this?

>+    // show the window
>+    this.edit = function()
>+    {
>+        tree = document.getElementById("shortcuts");
>+
>+        var textbox = document.getAnonymousElementByAttribute(tree, "anonid", "input");
>+        textbox.addEventListener("keypress", keyListener, true);
>+        textbox.addEventListener("reset", resetListener, true);
>+        tree.addEventListener("DOMAttrModified", modificationListener, true);
>+
>+        fillShortcutList();
>+    };

Couldn't this be "init" and only called once?

>+    this.dismiss = function()

Same with this?

>+    {
>+        if (!tree)
>+            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;

No need to hide the panel. Deck handles it.

>diff -r 666d3fa016b8 -r b6754eb8bdf9 chrome/locale/en-US/shortcuts.properties

>+cmd_addons.name=Show addons list
>+cmd_downloads.name=Show downloads list

I think these are gone too

>+cmd_shortcuts.name=Edit shortcuts

Remove

Comment 11

[Daniel Brooks [:db48x]]

(In reply to comment #10)
> >       this._showPanel(UIMODE_NONE);
> >+      Shortcuts.dismiss();
> 
> Can we move these into _showPanel() ?

sure.

> 
> Also, do you need to be adding and removing the event listeners everytime the
> panel is shown/hidden? It's not being destroyed.

Well, one of them is a mutation listener, which will slow down all dom manipulation as long as it's attached.

> >     else if (aMode == UIMODE_PANEL) {
> >       this._showToolbar(true);
> >@@ -647,6 +653,7 @@
> >       let dloads = document.getElementById("downloads-container");
> >       if (dloads.getAttribute("src") == "")
> >         dloads.setAttribute("src", "chrome://mozapps/content/downloads/downloads.xul");
> 
> I should have tried moving the above lines into _showPanel too.

easily done.

> >@@ -792,6 +800,7 @@
> >       case "cmd_go":
> >       case "cmd_star":
> >       case "cmd_bookmarks":
> >+      case "cmd_shortcuts":
> 
> Remove. You don't need it anymore
> 
> >       case "cmd_bookmarks":
> >         this.showBookmarks();
> >+        break;
> >+      case "cmd_shortcuts":
> >+        this.show(PANELMODE_NONE);
> >+        Shortcuts.edit();
> 
> Remove. You don't need it anymore. It's handled by the panel UI and the
> shortcut button.

oops, my bad

> 
> >diff -r 666d3fa016b8 -r b6754eb8bdf9 chrome/content/browser.xul
> >     <!-- scrolling -->
> >     <command id="cmd_scrollPageUp" oncommand="CommandUpdater.doCommand(this.id);"/>
> >+    <command id="cmd_scrollToBeginning" oncommand="CommandUpdater.doCommand(this.id);"/>
> >     <command id="cmd_scrollPageDown" oncommand="CommandUpdater.doCommand(this.id);"/>
> >+    <command id="cmd_scrollToEnd" oncommand="CommandUpdater.doCommand(this.id);"/>
> 
> What are these?

That's just me starting on one bug, then moving to another while forgetting to use a new tree :)

> >diff -r 666d3fa016b8 -r b6754eb8bdf9 chrome/content/shortcuts.js
> 
> your TABs are set to 4 spaces in this file. We use 2 spaces.

meh. is it important?

> >+    var modifierFlags = { alt: 1, control: 2, meta: 4, shift: 8 };
> >+    function getFlagsForModifiers(modifiers)
> >+    {
> >+        if (!modifiers)
> >+            return 0;
> >+
> >+        var result;
> >+        for each (m in modifiers.split(" "))
> 
> modifiers could be separated by "," too, right? So we either need to handle
> that here or be ever vigilant that we don't add a comma sep'd modifier to our
> commands. I noticed you "fixed" one of the modifiers in browser.xul
> 
> I'd rather be robust here

Yes, thanks for catching that. I've added a regex to catch commas, spaces, and commas plus spaces

> >+    var keyCodeMap = { };
> >+    var nsIDOMKeyEvent = Components.interfaces.nsIDOMKeyEvent;
> >+    keyCodeMap[nsIDOMKeyEvent.DOM_VK_CANCEL] = "VK_CANCEL";
> >+    ...
> >+    keyCodeMap[nsIDOMKeyEvent.DOM_VK_META] = "VK_META";
> 
> Didn't you and Mook come up with a regex for this?

Mook did, I merely corrected a small bug in it. I'll see if I can find it.

> >+cmd_addons.name=Show addons list
> >+cmd_downloads.name=Show downloads list
> 
> I think these are gone too
> 
> >+cmd_shortcuts.name=Edit shortcuts
> 
> Remove

Good point.

Comment 12

[Daniel Brooks [:db48x]]

Attachment: 441794-4.diff

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 340643 [details]
441794-4.diff

>diff -r 666d3fa016b8 -r be5b2202f1e9 chrome/content/browser-ui.js

>+
>+      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();
>+      }

Change:
* Remove this "if" and add a case for UIMODE_PANEL to the "switch" and move this code into the right cases
 * UIMODE_NONE
 * UIMODE_PANEL  <- that's right, it's not a fall through case

>   },
> 
>   _sizeControls : function(aEvent) {
>@@ -429,6 +442,8 @@
>     Browser.content.addEventListener("mousemove", this, true);
> 
>     window.addEventListener("resize", this, false);
>+    Shortcuts.restore();
>+    Shortcuts.test();

We really don't want this test in the code do we?

>diff -r 666d3fa016b8 -r be5b2202f1e9 chrome/content/browser.xul

>     <!-- scrolling -->
>     <command id="cmd_scrollPageUp" oncommand="CommandUpdater.doCommand(this.id);"/>
>+    <command id="cmd_scrollToBeginning" oncommand="CommandUpdater.doCommand(this.id);"/>
>     <command id="cmd_scrollPageDown" oncommand="CommandUpdater.doCommand(this.id);"/>
>+    <command id="cmd_scrollToEnd" oncommand="CommandUpdater.doCommand(this.id);"/>

You didn't want this in the patch did you?

* I can live with the 4 space indents for now
* The regex would be nice but not a "must have"

r- just so I see the switch change and the scrolling cmd removal

Comment 14

[Daniel Brooks [:db48x]]

Attachment: 441794-5.diff

(In reply to comment #13)
> (From update of attachment 340643 [details])
> >     window.addEventListener("resize", this, false);
> >+    Shortcuts.restore();
> >+    Shortcuts.test();
> 
> We really don't want this test in the code do we?

No, I won't push it to the repo with it there, but since I changed the behavior and the tests, I figured I'd hang on to it a little longer.

> >diff -r 666d3fa016b8 -r be5b2202f1e9 chrome/content/browser.xul
> 
> >     <!-- scrolling -->
> >     <command id="cmd_scrollPageUp" oncommand="CommandUpdater.doCommand(this.id);"/>
> >+    <command id="cmd_scrollToBeginning" oncommand="CommandUpdater.doCommand(this.id);"/>
> >     <command id="cmd_scrollPageDown" oncommand="CommandUpdater.doCommand(this.id);"/>
> >+    <command id="cmd_scrollToEnd" oncommand="CommandUpdater.doCommand(this.id);"/>
> 
> You didn't want this in the patch did you?

Ah, thought I might have forgotten something.

> * The regex would be nice but not a "must have"

Good, because I had already added it :)

Comment 15

[Daniel Brooks [:db48x]]

Thanks :)

pushed.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

how can I verify this?

Comment 17

[Anna (Waverley)]

Does anyone know how to verify this bug? :|

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

We removed this feature