Closed Bug 968424 Opened 8 years ago Closed 7 years ago

Change keyboard shortcut to move selected lines in source editor to Alt-Up/Down


(DevTools :: Source Editor, defect)

Windows 8
Not set


(firefox29 verified, firefox30 verified)

Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified


(Reporter: gl, Assigned: gl)



(1 file, 1 obsolete file)

Continuing the discussion from
> So the shortcut before Code Mirror was Alt + UP/DOWN on all browsers except
> Mac, where it was Cmd + Opt + UP/DOWN.
> Now, its Ctrl + Alt + UP/DOWN on windows too. Which does not work out for
> most Windows with Intel chips as Intel graphics driver (at least on Windows)
> map Ctrl + Alt + ARROW keys to screen rotation.
> Also, right now, the documentation is incorrect too. [1]
> Can we have it back to Alt + UP/DOWN ?

Objective of this bug:
1. Change the command key for moving lines up and down to Alt - Up/Down across all platforms.
2. Update MDN documentations.

I will try to have a patch by Friday evening.
Assignee: nobody → gabriel.luong
OS: Mac OS X → Windows 8
Attached patch 968424.patch (obsolete) — Splinter Review
I wasn't sure what was the best way to ensure that accel wasn't appended to every command. This patch ensure that alt+up/down is the shortkey for moving lines up/down across all platforms (mac and windows included).
Attachment #8372126 - Flags: review?(anton)
Comment on attachment 8372126 [details] [diff] [review]

Review of attachment 8372126 [details] [diff] [review]:

::: browser/devtools/sourceeditor/editor.js
@@ +831,3 @@
>   */
> +Editor.keyFor = function (cmd, accel=true) {
> +  return accel? Editor.accel(L10N.GetStringFromName(cmd + ".commandkey")) :

Nit: put a space after accel.

@@ +831,4 @@
>   */
> +Editor.keyFor = function (cmd, accel=true) {
> +  return accel? Editor.accel(L10N.GetStringFromName(cmd + ".commandkey")) :
> +         L10N.GetStringFromName(cmd + ".commandkey");

You should assign the value of L10N.GetStringFromName call to a variable:

  var key = L10N.GetStringFromName(cmd + ".commandkey");
  return accel ? Editor.accel(key) : key;

One thing that bugs me about this method, though, is that accel=true by default. This makes Editor.keyFor("X") and Editor.keyFor("X", false) behave differently which is not very JavaScript-ey. Also it introduces a boolean trap [1]. I think this methods signature should be changed to:

  Editor.keyFor = function (cmd, opts={ noaccel: false }) { ... };

[1] —

::: browser/locales/en-US/chrome/browser/devtools/
@@ +58,5 @@
>  # conjunction with accel (Command on Mac or Ctrl on other platforms) to either
>  # comment or uncomment selected lines in the editor.
>  toggleComment.commandkey=/
> +# LOCALIZATION NOTE  (toolboxPrevTool.commandkey): This is the key to use in

What are these changes? They don't seem to change anything.
Attachment #8372126 - Flags: review?(anton) → review-
Attached patch 968424.patchSplinter Review
Thanks for the review Anton! The hall of api shame was especially insightful.

This patch fixes the following:
-Added noaccel option to keyFor signature and adjusted the method calls for moveLineUp and moveLineDown
-In, I updated the localization note for moveLineUp and moveLineDown to remove the note about accel conjuction with the commandkey.
Attachment #8372126 - Attachment is obsolete: true
Attachment #8375204 - Flags: review?(anton)
Comment on attachment 8375204 [details] [diff] [review]

Review of attachment 8375204 [details] [diff] [review]:

Looks good, thanks for the patch!
Attachment #8375204 - Flags: review?(anton) → review+
Whiteboard: [checkin-needed]
Thanks Anton! I will update the MDN docs once it lands.
Whiteboard: [checkin-needed] → [fixed-in-fx-team]
Can we have this uplifted too ? at least till aurora, if not beta.
Updated MDN with the new keys
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Keywords: verifyme
Please ask for aurora approval. This shortcut will be broken in Windows for 2 releases otherwise.
Flags: needinfo?(gabriel.luong)
Can this get uplift to Aurora? Otherwise, the previous command (cmd+alt+up/down) would be rotating Window user's displays. Thanks!
Flags: needinfo?(gabriel.luong) → needinfo?(rcampbell)
Comment on attachment 8375204 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 924996
User impact if declined: The previous command for moving lines up and down in the source editor (cmd+alt+up/down) would rotate Window user's displays
Testing completed (on m-c, etc.): on m-c since 2-13-2014
Risk to taking this patch (and alternatives if risky): Low risk - command key changes to DevTools
String or IDL/UUID changes made by this patch:
Attachment #8375204 - Flags: approval-mozilla-aurora?
Figured out how to ask for aurora uplift!
Flags: needinfo?(rcampbell)
Attachment #8375204 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Gabriel L (:gluong) from comment #0)
> Objective of this bug:
> 1. Change the command key for moving lines up and down to Alt - Up/Down
> across all platforms.
> 2. Update MDN documentations.
Verified fixed FF 29.0a2(2014-03-11), 30.0a1(2014-03-11) Win 7, Ubuntu 13.04, Mac OS X 10.8.5
Keywords: verifyme
I'm updating devtools docs for Firefox 30, and just wanted to check with you that the documentation for these shortcuts is correct now. I reorganised the keyboard shortcut docs after you updated them, but before I saw this bug, so I just wanted to check I didn't break the docs again after you fixed them:

Flags: needinfo?(gabriel.luong)
Looks good still!
Flags: needinfo?(gabriel.luong)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.