Allow more common key names in key bindings (#10233)

[RaymondLim]

This fixes issue #10233.

[redmunds]

Do you really need this? Seems like a case-sensitive search of _keyNames list will catch this.

[RaymondLim]

Yes, we do need this since we don't want the case-sensitive search of _keyNames list to catch a case-insensitive-matching key and reject it by returning null. We also need the exactly matching case for the shell code to correctly assign the corresponding accelerator.

[redmunds]

It looks like this changes Pageup to be PageUp and Pagedown to be PageDown, but pageup or pagedown are not changed. Is that correct?

[RaymondLim]

The if block above this already takes care of the first letter of all key names and here we're only taking care of the two key names that have the second word.

[redmunds]

I tried creating shortcuts with PAGEUP and PAGEDOWN and they fail. Seems weird that case doesn't need to be exact, but only some case are accepted. Is that the intention?

[RaymondLim]

@redmunds Good catch! This if statement is assuming that the other if block above it has been executed and all letters except the first one are already in lower case. So the culprit is in /^[a-z]/.test(key) where we should be testing with i flag. That is, we've already had a bug that has nothing to do with this case conversion (as in UP not converted to Up).

[redmunds]

This KeyBindingManager unit test needs to be fixed:

"should show an error when parsing invalid shortcuts"

Error: Expected 'These shortcuts are invalid: <ul class='dialog-list'><li>command-2</li><li>Option-Cmd-Backspace</li><li>no_modifier_key</li><li>ctrl-kk</li><li>cmd-Delete</li></ul>' to match 'Home'.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:114:32)
    at null.toMatch (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1235:29)
    at Object.<anonymous> (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/spec/KeyBindingManager-test.js:633:41)
    at Object.spyObj (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:413:24)
    at file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/src/command/KeyBindingManager.js:1017:21
    at Object.context.execCb (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/src/thirdparty/requirejs/require.js:1658:33)
    at Object.Module.check (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/src/thirdparty/requirejs/require.js:874:51)
    at Object.Module.enable (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/src/thirdparty/requirejs/require.js:1151:22)
    at Object.Module.init (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/src/thirdparty/requirejs/require.js:782:26)
    at file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/src/thirdparty/requirejs/require.js:1424:36

[redmunds]

Done with review.

[RaymondLim]

@redmunds Thanks for reviewing and catching the unit test failure. I just pushed the unit test fix and this is ready for another review.

[marcelgerber]

Why don't you just do it like this?

key = key.charAt(0).toUpperCase() + key.substr(1).toLowerCase();

This probably also resolves problems with umlauts like ä.

[RaymondLim]

Because I don't want to convert non-English alphabet like ä to uppercase. Also to exclude other punctuation and numeric characters.

[marcelgerber]

.toUpperCase() will no-op on non-letters.

And if you really need to check for valid English letters, you can still do it like this:

if (/^[a-z]/i.test(key)) {
    key = key.charAt(0).toUpperCase() + key.substr(1).toLowerCase();
}

[redmunds]

Looks good. Merging.