929234 - Make CM extraKeys localizable

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect, P2)

Description

[Anton Kovalyov (:anton)]

CodeMirror expects extraKeys to have a certain format ("Ctrl-F", etc.) It will be more consistent with the rest of the codebase if we could move them into localizeable XUL and extract the info from there.

Comment 1

[Anton Kovalyov (:anton)]

Attachment: WIP 1

Changed the code to grab actual keys either from DTD (via DOM) or from .properties files. Also made Editor.ctrl static method so that we don't have multiple implementation of the same logic.

Try: https://tbpl.mozilla.org/?tree=Try&rev=dcf1c8f9642e

Comment 2

[Victor Porof [:vporof][:vp]]

Comment on attachment 822040 [details] [diff] [review]
WIP 1

Review of attachment 822040 [details] [diff] [review]:
-----------------------------------------------------------------

Let me know what you think about the comments below.

::: browser/devtools/debugger/debugger-view.js
@@ +190,2 @@
>  
>      // This needs to be more localizable: see bug 929234.

You can now remove this comment.

@@ +190,5 @@
>  
>      // This needs to be more localizable: see bug 929234.
>      let extraKeys = {};
> +    let searchKey = document.getElementById("tokenSearchKey").getAttribute("key");
> +    extraKeys[Editor.ctrl(searchKey)] = (cm) => {

Rename Editor.ctrl to Editor.accel. I know it handles cmd on OS X, but it's not immediately obvious the way the function is named right now.

::: browser/devtools/sourceeditor/editor.js
@@ +139,4 @@
>    Object.keys(config).forEach((k) => this.config[k] = config[k]);
>  
>    // Additional shortcuts.
> +  let jumpKey = L10N.GetStringFromName("jumpToLine.commandkey");

It'd be better if there was a way of opting out of the default bindings.

For example, I just noticed that in the debugger, Accel+J adds an in-editor "Jump to line" dialog, but we already have an Accel+L which focuses the searchbox and does exactly the same thing. So now there are two ways of jumping to a line in the debugger :)

I suggest allowing the editor to define both its own search-for-string and jump-to-line keyboard shortcuts with their own UI, but making them optional (on by default, but with a pref that allows consumers to disable them).

Where is the search-for-string key (Accel+F) defined and bound to its respective action? I think it should be here as well, along with jump-to-line. It should also be l10ed too if not already.

@@ +140,5 @@
>  
>    // Additional shortcuts.
> +  let jumpKey = L10N.GetStringFromName("jumpToLine.commandkey");
> +  this.config.extraKeys[Editor.ctrl(jumpKey)] = (cm) => this.jumpToLine();
> +  this.config.extraKeys[Editor.ctrl("/")] = "toggleComment";

Maybe these keys should be localized as well ("/", "[", "]"), since they're not always available (at least directly) on certain keyboard layouts.

Comment 3

[Victor Porof [:vporof][:vp]]

Any progress on this Anton? Can I help?

Comment 4

[Anton Kovalyov (:anton)]

I'll do this and push to Aurora early next week. This got postponed due to more important bug with shortcuts in Scratchpad/StyleEditor that I managed to figure out only on Friday.

Comment 5

[Anton Kovalyov (:anton)]

Attachment: WIP 2

Try: https://tbpl.mozilla.org/?tree=Try&rev=dbc80348dd71

The CodeMirror-y approach is to set extraKeys[key] to false if you want to disable the key. I updated the patch to make this easier. Added another static method Editor.forKey (which should be called forCommand actually; will change prior to landing) that takes a command as an argument and returns its localized key.

For example, to disable jumpToLine command you'd do:

var extraKeys = {};
extraKeys[Editor.forCommand("jumpToLine")] = false;

And then, as usual, pass extraKeys with config to the Editor function.

Now the default keymap we're using with CM is not localizable so I propose to make our own keymap: it will be a clone of the default keymap except that everything that needs to be localized will be taken from the .properties file. If you agree with this approach, I will file a bug to clone/localize default keymap.

(Default keymap is where the search shortcut is defined btw)

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 828345 [details] [diff] [review]
WIP 2

Review of attachment 828345 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/debugger/debugger-view.js
@@ +191,3 @@
>      let extraKeys = {};
> +    let searchKey = document.getElementById("tokenSearchKey").getAttribute("key");
> +    extraKeys[Editor.accel(searchKey)] = (cm) => {

I still don't fully understand why the search key can't stay defined only in XUL, and the jumpToLine key can. There's a symmetry breach here, which will not prevent this patch from landing, but which I'd love to understand before it does.

Simply put, why doesn't this work?
extraKeys[Editor.keyFor("whateverKeyForSearching")] = false;
extraKeys[Editor.keyFor("jumpToLine")] = false;

(I guess it may be because there's no key for searching yet in the properties file?).

::: browser/devtools/sourceeditor/editor.js
@@ +149,2 @@
>    // Overwrite default config with user-provided, if needed.
> +  Object.keys(config).forEach((k) => {

Nit: you could simply for..in this, but I don't really care.

Comment 7

[Victor Porof [:vporof][:vp]]

(In reply to Anton Kovalyov (:anton) from comment #5)
> 
> Now the default keymap we're using with CM is not localizable so I propose
> to make our own keymap: it will be a clone of the default keymap except that
> everything that needs to be localized will be taken from the .properties
> file. If you agree with this approach, I will file a bug to clone/localize
> default keymap.
> 

Sounds good!

Comment 8

[Anton Kovalyov (:anton)]

(In reply to Victor Porof [:vp] from comment #6)
> Comment on attachment 828345 [details] [diff] [review]
> WIP 2
> 
> Review of attachment 828345 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger-view.js
> @@ +191,3 @@
> >      let extraKeys = {};
> > +    let searchKey = document.getElementById("tokenSearchKey").getAttribute("key");
> > +    extraKeys[Editor.accel(searchKey)] = (cm) => {
> 
> I still don't fully understand why the search key can't stay defined only in
> XUL, and the jumpToLine key can. There's a symmetry breach here, which will
> not prevent this patch from landing, but which I'd love to understand before
> it does.
> 
> Simply put, why doesn't this work?
> extraKeys[Editor.keyFor("whateverKeyForSearching")] = false;
> extraKeys[Editor.keyFor("jumpToLine")] = false;
> 
> (I guess it may be because there's no key for searching yet in the
> properties file?).
> 

Yes exactly. Key maps are currently hard-coded within CodeMirror source code and SE properties file doesn't have shortcuts in them. So there's no way to get those aside from quering the DOM. Fortunately, CodeMirror allows us to use whatever key maps we want hence my proposal to make our own localizable key map.

Comment 9

[Anton Kovalyov (:anton)]

https://hg.mozilla.org/integration/fx-team/rev/4ee49067a5c6

Comment 10

[Anton Kovalyov (:anton)]

Comment on attachment 828345 [details] [diff] [review]
WIP 2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 919709
User impact if declined: Shortcuts Cmd/Ctrl-J, Cmd/Ctrl-], Cmd/Ctrl-[ and Cmd/Ctrl-/ will not be localizable. I don't think anyone actually translates these shortcuts but who knows?
Testing completed (on m-c, etc.): local, fx-team, m-c (when merged; won't uplift before that)
Risk to taking this patch (and alternatives if risky): low, patch is pretty straightforward and doesn't change any functionality.
String or IDL/UUID changes made by this patch: None changed, 4 added to the sourceeditor.properties file.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ee49067a5c6

Comment 12

[bhavana bajaj [:bajaj]]

I am needinfo'ing :pike here to help understand the l10n impact. :anton aurora is a string freeze period so no string changes are allowed, can you provide alternatives here or help understand the impact if this is delayed for a few weeks ?

Comment 13

[Axel Hecht [:Pike]]

I personally think this is fine to ride the trains

Comment 14

[Anton Kovalyov (:anton)]

Unless people around the world like to remap common editor shortcuts such as Ctrl-/ and Ctrl-J the impact is not horrible. But if they do, their remapping won't work so they'll default to Ctrl-/, Ctrl-J and so on. 

And with Ctrl-[ and Ctrl-], our l10n files tell explicitly not to translate these values. So I don't think anyone did.

Comment 15

[bhavana bajaj [:bajaj]]

Understanding the impact of taking this is low and limited to dev-tools debugger and offline conversation with :anton, lets let this ride the trains to avoid any l10n impact. Happy to take an uplift if there are any workarounds without string changes.

Comment 16

[Francesco Lodolo [:flod]]

Can someone explain the meaning of this localization comment? It doesn't make much sense honestly

> # DO NOT translate this key without proper synchronization with toolbox.dtd.

toolbox.dtd
<!ENTITY toolboxNextTool.key "]">
<!ENTITY toolboxPreviousTool.key "[">

sourceeditor.properties
indentLess.commandkey=[
indentMore.commandkey=]

Why would I need to keep them in Sync? They perform different commands.

Comment 17

[Anton Kovalyov (:anton)]

That's because these shortcuts conflicts with each other so in http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/editor.js#142 we disable them. If you modify only sourceeditor.properties entries Editor will not be disabling correct shortcut within CodeMirror.

Comment 18

[Francesco Lodolo [:flod]]

Thanks Anton, but I still don't get the localization comment. Basically, shortcuts in sourceeditor.properties are never used: if I'm reading editor.js right, it just disables both command keys, it's not a decision based on their values.

Comment 19

[Anton Kovalyov (:anton)]

Yes, it assumes that the values are the same as in toolbox.dtd.