bug fixes: Clean up hotkey mappings.

We simplify hotkey mappings by using different hashes for
keydown and keypress events.  There are browser bugs (iOS, for
example) where keypress events have the wrong keyCode values.
This led us, under iOS, to interpret "!" as "page up."

This fix also helps us disinguish escape from shift-escape.

Brock Whittaker helped on figuring out the keypress/keydown
issues that are addressed in this commit.

Fixes #4019

frontend_tests/node_tests/hotkey.js

@@ -27,15 +27,51 @@ function stubbing(func_name_to_stub, test_function) {
     });
 }
 
+(function test_mappings() {
+    function map_press(which, shiftKey) {
+        return hotkey.get_keypress_hotkey({
+            which: which,
+            shiftKey: shiftKey,
+        });
+    }
+
+    function map_down(which, shiftKey) {
+        return hotkey.get_keydown_hotkey({
+            which: which,
+            shiftKey: shiftKey,
+        });
+    }
+
+    // The next assertion protects against an iOS bug where we
+    // treat "!" as a hotkey, because iOS sends the wrong code.
+    assert.equal(map_press(33), undefined);
+
+    // Test page-up does work.
+    assert.equal(map_down(33).name, 'page_up');
+
+    // Test other mappings.
+    assert.equal(map_down(9).name, 'tab');
+    assert.equal(map_down(9, true).name, 'shift_tab');
+    assert.equal(map_down(27).name, 'escape');
+    assert.equal(map_down(37).name, 'left_arrow');
+    assert.equal(map_down(13).name, 'enter');
+    assert.equal(map_down(13, true).name, 'enter');
+
+    assert.equal(map_press(47).name, 'search'); // slash
+    assert.equal(map_press(106).name, 'vim_down'); // j
+
+    // More negative tests.
+    assert.equal(map_down(47), undefined);
+    assert.equal(map_press(27), undefined);
+    assert.equal(map_down(27, true), undefined);
+}());
+
 (function test_basic_chars() {
     function process(s) {
-        // Simulating keyboard events is a huge pain.
-        var shifted_keys = '~!@#$%^*()_+{}:"<>?';
         var e = {
             which: s.charCodeAt(0),
-            shiftKey: (shifted_keys.indexOf(s) >= 0),
         };
-        return hotkey.process_hotkey(e);
+        return hotkey.process_keypress(e);
     }
 
     function assert_mapping(c, func_name, shiftKey) {

static/js/hotkey.js

@@ -31,14 +31,16 @@ var actions_dropdown_hotkeys = [
 // we'll do in cases where they have the exact same semantics.
 // DON'T FORGET: update keyboard_shortcuts.html
 
-var hotkeys_shift = {
+var keydown_shift_mappings = {
     // these can be triggered by shift + key only
     9: {name: 'shift_tab', message_view_only: false}, // tab
     32: {name: 'shift_spacebar', message_view_only: true},  // space bar
 };
-var hotkeys_no_modifiers = {
+
+var keydown_unshift_mappings = {
     // these can be triggered by key only (without shift)
     9: {name: 'tab', message_view_only: false}, // tab
+    27: {name: 'escape', message_view_only: false}, // escape
     32: {name: 'spacebar', message_view_only: true}, // space bar
     33: {name: 'page_up', message_view_only: true}, // page up
     34: {name: 'page_down', message_view_only: true}, // page down
@@ -48,12 +50,24 @@ var hotkeys_no_modifiers = {
     38: {name: 'up_arrow', message_view_only: true}, // up arrow
     40: {name: 'down_arrow', message_view_only: true}, // down arrow
 };
-var hotkeys_shift_insensitive = {
+
+var keydown_either_mappings = {
     // these can be triggered by key or shift + key
     // Note that codes for letters are still case sensitive!
+    //
+    // We may want to revisit both of these.  For backspace, we don't
+    // have any specific mapping behavior; we are just trying to disable
+    // the normal browser features for certain OSes when we are in the
+    // compose box, and the little bit of backspace-related code here is
+    // dubious, but may apply to shift-backspace.
+    // For enter, there is some possibly that shift-enter is intended to
+    // have special behavior for folks that are used to shift-enter behavior
+    // in other apps, but that's also slightly dubious.
     8: {name: 'backspace', message_view_only: true}, // backspace
     13: {name: 'enter', message_view_only: false}, // enter
-    27: {name: 'escape', message_view_only: false}, // escape
+};
+
+var keypress_mappings = {
     47: {name: 'search', message_view_only: false}, // '/'
     63: {name: 'show_shortcuts', message_view_only: false}, // '?'
     64: {name: 'compose_reply_with_mention', message_view_only: true}, // '@'
@@ -94,25 +108,36 @@ var tab_up_down = (function () {
     };
 }());
 
-function get_hotkey_from_event(e) {
+exports.get_keydown_hotkey = function (e) {
-
-    // We're in the middle of a combo; stop processing because
-    // we want the browser to handle it (to avoid breaking
-    // things like Ctrl-C or Command-C for copy).
     if (e.metaKey || e.ctrlKey || e.altKey) {
-        return {name: 'ignore', message_view_only: false};
+        return;
     }
 
-    if (e.shiftKey && hotkeys_shift[e.which] !== undefined) {
+    var hotkey;
-        return hotkeys_shift[e.which];
+    if (e.shiftKey) {
-    } else if (!e.shiftKey && hotkeys_no_modifiers[e.which] !== undefined) {
+        hotkey = keydown_shift_mappings[e.which];
-        return hotkeys_no_modifiers[e.which];
+        if (hotkey) {
-    } else if (hotkeys_shift_insensitive[e.which] !== undefined) {
+            return hotkey;
-        return hotkeys_shift_insensitive[e.which];
+        }
     }
 
-    return {name: 'ignore', message_view_only: false};
+    if (!e.shiftKey) {
-}
+        hotkey = keydown_unshift_mappings[e.which];
+        if (hotkey) {
+            return hotkey;
+        }
+    }
+
+    return keydown_either_mappings[e.which];
+};
+
+exports.get_keypress_hotkey = function (e) {
+    if (e.metaKey || e.ctrlKey || e.altKey) {
+        return;
+    }
+
+    return keypress_mappings[e.which];
+};
 
 exports.processing_text = function () {
     var selector = 'input:focus,select:focus,textarea:focus,#compose-send-button:focus';
@@ -286,18 +311,12 @@ exports.process_enter_key = function (e) {
 // Process a keydown or keypress event.
 //
 // Returns true if we handled it, false if the browser should.
-exports.process_hotkey = function (e) {
+exports.process_hotkey = function (e, hotkey) {
     var alert_words_content;
     var focused_message_edit_content;
     var focused_message_edit_save;
     var message_edit_form;
-    var hotkey = get_hotkey_from_event(e);
     var event_name = hotkey.name;
-    activity.new_user_input = true;
-
-    if (event_name === 'ignore') {
-        return false;
-    }
 
     if (event_name === 'escape') {
         return exports.process_escape_key(e);
@@ -554,28 +573,33 @@ exports.process_hotkey = function (e) {
    so we bail in .keydown if the event is a letter or number and
    instead just let keypress go for it. */
 
+exports.process_keydown = function (e) {
+    activity.new_user_input = true;
+    var hotkey = exports.get_keydown_hotkey(e);
+    if (!hotkey) {
+        return false;
+    }
+    return exports.process_hotkey(e, hotkey);
+};
+
 $(document).keydown(function (e) {
-    // Restrict to non-alphanumeric keys
+    if (exports.process_keydown(e)) {
-    // check if 27 (esc) because it doesn't register under .keypress()
+        e.preventDefault();
-    if (e.which < 48 || e.which > 90 || e.which === 27) {
-        if (exports.process_hotkey(e)) {
-            e.preventDefault();
-        }
     }
     resize.resize_bottom_whitespace();
 });
 
+exports.process_keypress = function (e) {
+    var hotkey = exports.get_keypress_hotkey(e);
+    if (!hotkey) {
+        return false;
+    }
+    return exports.process_hotkey(e, hotkey);
+};
+
 $(document).keypress(function (e) {
-    // What exactly triggers .keypress may vary by browser.
+    if (exports.process_keypress(e)) {
-    // Welcome to compatability hell.
+        e.preventDefault();
-    //
-    // In particular, when you press tab in Firefox, it fires a
-    // keypress event with keycode 0 after processing the original
-    // event.
-    if (e.which !== 0 && e.charCode !== 0) {
-        if (exports.process_hotkey(e)) {
-            e.preventDefault();
-        }
     }
 });