Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

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

Merged
merged 4 commits into from
Jan 8, 2015
Merged

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
236b326
Allow more common key names in key bindings.
RaymondLim Dec 20, 2014
dcc6ec2
Fix the failing KeyBindingManager unit test.
RaymondLim Jan 1, 2015
8d29e49
Fix the issue of not converting key names to the correct case if the …
RaymondLim Jan 3, 2015
ff30488
Use simpler string methods only for case conversion.
RaymondLim Jan 5, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/command/KeyBindingManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ define(function (require, exports, module) {
Commands.EDIT_CUT, Commands.EDIT_COPY, Commands.EDIT_PASTE],
_reservedShortcuts = ["Ctrl-Z", "Ctrl-Y", "Ctrl-A", "Ctrl-X", "Ctrl-C", "Ctrl-V"],
_macReservedShortcuts = ["Cmd-,", "Cmd-H", "Cmd-Alt-H", "Cmd-M", "Cmd-Shift-Z", "Cmd-Q"],
_keyNames = ["Up", "Down", "Left", "Right", "Backspace", "Enter", "Space", "Tab"];
_keyNames = ["Up", "Down", "Left", "Right", "Backspace", "Enter", "Space", "Tab",
"PageUp", "PageDown", "Home", "End", "Insert", "Delete"];

/**
* @private
Expand Down Expand Up @@ -410,6 +411,13 @@ define(function (require, exports, module) {
});
}

// Also make sure that the second word of PageUp/PageDown has the first letter in upper case.
if (/^Page/.test(key)) {
key = key.replace(/(up|down)$/, function (match, p1) {
return p1.charAt(0).toUpperCase() + p1.substr(1);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).


// No restriction on single character key yet, but other key names are restricted to either
// Function keys or those listed in _keyNames array.
if (key.length > 1 && !/F\d+/.test(key) &&
Expand Down Expand Up @@ -504,6 +512,10 @@ define(function (require, exports, module) {
key = "Space";
} else if (key === "\b") {
key = "Backspace";
} else if (key === "Help") {
key = "Insert";
} else if (event.keyCode === KeyEvent.DOM_VK_DELETE) {
key = "Delete";
} else {
key = _mapKeycodeToKey(event.keyCode, key);
}
Expand Down Expand Up @@ -1084,7 +1096,7 @@ define(function (require, exports, module) {
function _getDisplayKey(key) {
var displayKey = "",
match = key ? key.match(/(Up|Down|Left|Right|\-)$/i) : null;
if (match) {
if (match && !/Page(Up|Down)/.test(key)) {
displayKey = key.substr(0, match.index) + _displayKeyMap[match[0].toLowerCase()];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think changing the regex above to match start of text is a simpler way to detect this.

key.match(/^(Up|Down|Left|Right|\-)$/i)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we can't match the start of text since key string may have some modifier keys as in Cmd-Up.

}
return displayKey;
Expand Down