Merge pull request #2470 from smblott-github/rework-key-handling-to-keydown

Rework key handling to keydown

8 files changed, 49 insertions, 94 deletions

diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee
index 741d42cf..eb138caa 100644
--- a/content_scripts/link_hints.coffee
+++ b/content_scripts/link_hints.coffee
@@ -172,11 +172,10 @@ class LinkHintsMode
       exitOnEscape: true
       exitOnClick: true
       keydown: @onKeyDownInMode.bind this
-      keypress: @onKeyPressInMode.bind this
 
     @hintMode.onExit (event) =>
       if event?.type == "click" or (event?.type == "keydown" and
-        (KeyboardUtils.isEscape(event) or event.keyCode in [keyCodes.backspace, keyCodes.deleteKey]))
+        (KeyboardUtils.isEscape(event) or KeyboardUtils.isBackspace event))
           HintCoordinator.sendMessage "exit", isSuccess: false
 
     # Note(philc): Append these markers as top level children instead of as child nodes to the link itself,
@@ -230,10 +229,9 @@ class LinkHintsMode
       linkText: desc.linkText
       stableSortCount: ++@stableSortCount
 
-  # Handles <Shift> and <Ctrl>.
+  # Handles all keyboard events.
   onKeyDownInMode: (event) ->
     return if event.repeat
-    @keydownKeyChar = KeyboardUtils.getKeyChar(event).toLowerCase()
 
     previousTabCount = @tabCount
     @tabCount = 0
@@ -241,25 +239,25 @@ class LinkHintsMode
     # NOTE(smblott) As of 1.54, the Ctrl modifier doesn't work for filtered link hints; therefore we only
     # offer the control modifier for alphabet hints.  It is not clear whether we should fix this.  As of
     # 16-03-28, nobody has complained.
-    modifiers = [keyCodes.shiftKey]
-    modifiers.push keyCodes.ctrlKey unless Settings.get "filterLinkHints"
+    modifiers = ["Shift"]
+    modifiers.push "Control" unless Settings.get "filterLinkHints"
 
-    if event.keyCode in modifiers and
+    if event.key in modifiers and
       @mode in [ OPEN_IN_CURRENT_TAB, OPEN_WITH_QUEUE, OPEN_IN_NEW_BG_TAB, OPEN_IN_NEW_FG_TAB ]
         @tabCount = previousTabCount
         # Toggle whether to open the link in a new or current tab.
         previousMode = @mode
-        keyCode = event.keyCode
+        key = event.key
 
-        switch keyCode
-          when keyCodes.shiftKey
+        switch key
+          when "Shift"
             @setOpenLinkMode(if @mode is OPEN_IN_CURRENT_TAB then OPEN_IN_NEW_BG_TAB else OPEN_IN_CURRENT_TAB)
-          when keyCodes.ctrlKey
+          when "Control"
             @setOpenLinkMode(if @mode is OPEN_IN_NEW_FG_TAB then OPEN_IN_NEW_BG_TAB else OPEN_IN_NEW_FG_TAB)
 
         handlerId = handlerStack.push
           keyup: (event) =>
-            if event.keyCode == keyCode
+            if event.key == key
               handlerStack.remove()
               @setOpenLinkMode previousMode
             true # Continue bubbling the event.
@@ -268,7 +266,7 @@ class LinkHintsMode
         # Therefore, we ensure that it's always removed when hint mode exits.  See #1911 and #1926.
         @hintMode.onExit -> handlerStack.remove handlerId
 
-    else if event.keyCode in [ keyCodes.backspace, keyCodes.deleteKey ]
+    else if KeyboardUtils.isBackspace event
       if @markerMatcher.popKeyChar()
         @updateVisibleMarkers()
       else
@@ -276,36 +274,27 @@ class LinkHintsMode
         # knows not to restart hints mode.
         @hintMode.exit event
 
-    else if event.keyCode == keyCodes.enter
+    else if event.key == "Enter"
       # Activate the active hint, if there is one.  Only FilterHints uses an active hint.
       HintCoordinator.sendMessage "activateActiveHintMarker" if @markerMatcher.activeHintMarker
 
-    else if event.keyCode == keyCodes.tab
+    else if event.key == "Tab"
       @tabCount = previousTabCount + (if event.shiftKey then -1 else 1)
       @updateVisibleMarkers @tabCount
 
-    else if event.keyCode == keyCodes.space and @markerMatcher.shouldRotateHints event
+    else if event.key == " " and @markerMatcher.shouldRotateHints event
       @tabCount = previousTabCount
       HintCoordinator.sendMessage "rotateHints"
 
     else
       @tabCount = previousTabCount if event.ctrlKey or event.metaKey or event.altKey
-      return
-
-    # We've handled the event, so suppress it and update the mode indicator.
-    DomUtils.suppressEvent event
-
-  # Handles normal input.
-  onKeyPressInMode: (event) ->
-    return if event.repeat
-
-    keyChar = String.fromCharCode(event.charCode).toLowerCase()
-    if keyChar
-      @markerMatcher.pushKeyChar keyChar, @keydownKeyChar
-      @updateVisibleMarkers()
-
-    # We've handled the event, so suppress it.
-    DomUtils.suppressEvent event
+      unless event.repeat
+        if keyChar = KeyboardUtils.getKeyChar event
+          keyChar = " " if keyChar == "space"
+          if keyChar.length == 1
+            @markerMatcher.pushKeyChar keyChar
+            @updateVisibleMarkers()
+          DomUtils.consumeKeyup event
 
   updateVisibleMarkers: (tabCount = 0) ->
     {hintKeystrokeQueue, linkTextKeystrokeQueue} = @markerMatcher
@@ -445,11 +434,6 @@ class LinkHintsMode
 class AlphabetHints
   constructor: ->
     @linkHintCharacters = Settings.get("linkHintCharacters").toLowerCase()
-    # We use the keyChar from keydown if the link-hint characters are all "a-z0-9".  This is the default
-    # settings value, and preserves the legacy behavior (which always used keydown) for users which are
-    # familiar with that behavior.  Otherwise, we use keyChar from keypress, which admits non-Latin
-    # characters. See #1722.
-    @useKeydown = /^[a-z0-9]*$/.test @linkHintCharacters
     @hintKeystrokeQueue = []
 
   fillInMarkers: (hintMarkers) ->
@@ -478,8 +462,8 @@ class AlphabetHints
     matchString = @hintKeystrokeQueue.join ""
     linksMatched: hintMarkers.filter (linkMarker) -> linkMarker.hintString.startsWith matchString
 
-  pushKeyChar: (keyChar, keydownKeyChar) ->
-    @hintKeystrokeQueue.push (if @useKeydown then keydownKeyChar else keyChar)
+  pushKeyChar: (keyChar) ->
+    @hintKeystrokeQueue.push keyChar
   popKeyChar: -> @hintKeystrokeQueue.pop()
 
   # For alphabet hints, <Space> always rotates the hints, regardless of modifiers.
@@ -535,10 +519,7 @@ class FilterHints
     linksMatched: linksMatched
     userMightOverType: @hintKeystrokeQueue.length == 0 and 0 < @linkTextKeystrokeQueue.length
 
-  pushKeyChar: (keyChar, keydownKeyChar) ->
-    # For filtered hints, we *always* use the keyChar value from keypress, because there is no obvious and
-    # easy-to-understand meaning for choosing one of keyChar or keydownKeyChar (as there is for alphabet
-    # hints).
+  pushKeyChar: (keyChar) ->
     if 0 <= @linkHintNumbers.indexOf keyChar
       @hintKeystrokeQueue.push keyChar
     # We only accept <Space> and characters which are not used for splitting (e.g. "a", "b", etc., but not "-").
@@ -901,7 +882,7 @@ class WaitForEnter extends Mode
 
     @push
       keydown: (event) =>
-        if event.keyCode == keyCodes.enter
+        if event.key == "Enter"
           @exit()
           callback true # true -> isSuccess.
         else if KeyboardUtils.isEscape event
diff --git a/content_scripts/marks.coffee b/content_scripts/marks.coffee
index 37b062ba..4a2a8203 100644
--- a/content_scripts/marks.coffee
+++ b/content_scripts/marks.coffee
@@ -35,8 +35,8 @@ Marks =
       indicator: "Create mark..."
       exitOnEscape: true
       suppressAllKeyboardEvents: true
-      keypress: (event) =>
-        keyChar = String.fromCharCode event.charCode
+      keydown: (event) =>
+        keyChar = KeyboardUtils.getKeyChar event
         @exit =>
           if @isGlobalMark event, keyChar
             # We record the current scroll position, but only if this is the top frame within the tab.
@@ -58,27 +58,27 @@ Marks =
       indicator: "Go to mark..."
       exitOnEscape: true
       suppressAllKeyboardEvents: true
-      keypress: (event) =>
+      keydown: (event) =>
         @exit =>
-          markName = String.fromCharCode event.charCode
-          if @isGlobalMark event, markName
+          keyChar = KeyboardUtils.getKeyChar event
+          if @isGlobalMark event, keyChar
             # This key must match @getLocationKey() in the back end.
-            key = "vimiumGlobalMark|#{markName}"
+            key = "vimiumGlobalMark|#{keyChar}"
             Settings.storage.get key, (items) ->
               if key of items
-                chrome.runtime.sendMessage handler: 'gotoMark', markName: markName
-                HUD.showForDuration "Jumped to global mark '#{markName}'", 1000
+                chrome.runtime.sendMessage handler: 'gotoMark', markName: keyChar
+                HUD.showForDuration "Jumped to global mark '#{keyChar}'", 1000
               else
-                HUD.showForDuration "Global mark not set '#{markName}'", 1000
+                HUD.showForDuration "Global mark not set '#{keyChar}'", 1000
           else
-            markString = @localRegisters[markName] ? localStorage[@getLocationKey markName]
+            markString = @localRegisters[keyChar] ? localStorage[@getLocationKey keyChar]
             if markString?
               @setPreviousPosition()
               position = JSON.parse markString
               window.scrollTo position.scrollX, position.scrollY
-              @showMessage "Jumped to local mark", markName
+              @showMessage "Jumped to local mark", keyChar
             else
-              @showMessage "Local mark not set", markName
+              @showMessage "Local mark not set", keyChar
 
 root = exports ? window
 root.Marks =  Marks
diff --git a/content_scripts/mode.coffee b/content_scripts/mode.coffee
index 2d8cc9cc..85187b2c 100644
--- a/content_scripts/mode.coffee
+++ b/content_scripts/mode.coffee
@@ -82,7 +82,7 @@ class Mode
         "keydown": (event) =>
           return @continueBubbling unless KeyboardUtils.isEscape event
           @exit event, event.target
-          DomUtils.suppressKeyupAfterEscape handlerStack
+          DomUtils.consumeKeyup event
 
     # If @options.exitOnBlur is truthy, then it should be an element.  The mode will exit when that element
     # loses the focus.
diff --git a/content_scripts/mode_find.coffee b/content_scripts/mode_find.coffee
index 63825600..77d3762d 100644
--- a/content_scripts/mode_find.coffee
+++ b/content_scripts/mode_find.coffee
@@ -48,7 +48,7 @@ class PostFindMode extends SuppressPrintable
       keydown: (event) =>
         if KeyboardUtils.isEscape event
           @exit()
-          DomUtils.suppressKeyupAfterEscape handlerStack
+          DomUtils.consumeKeyup event
         else
           handlerStack.remove()
           @continueBubbling
diff --git a/content_scripts/mode_insert.coffee b/content_scripts/mode_insert.coffee
index 73a24112..a4f1836d 100644
--- a/content_scripts/mode_insert.coffee
+++ b/content_scripts/mode_insert.coffee
@@ -26,7 +26,7 @@ class InsertMode extends Mode
         # An editable element in a shadow DOM is focused; blur it.
         @insertModeLock.blur()
       @exit event, event.target
-      DomUtils.suppressKeyupAfterEscape handlerStack
+      DomUtils.consumeKeyup event
 
     defaults =
       name: "insert"
diff --git a/content_scripts/mode_key_handler.coffee b/content_scripts/mode_key_handler.coffee
index e206dbc6..1b3b21e7 100644
--- a/content_scripts/mode_key_handler.coffee
+++ b/content_scripts/mode_key_handler.coffee
@@ -12,7 +12,6 @@
 # consists of a (non-empty) list of such mappings.
 
 class KeyHandlerMode extends Mode
-  keydownEvents: {}
   setKeyMapping: (@keyMapping) -> @reset()
   setPassKeys: (@passKeys) -> @reset()
   # Only for tests.
@@ -28,8 +27,6 @@ class KeyHandlerMode extends Mode
 
     super extend options,
       keydown: @onKeydown.bind this
-      keypress: @onKeypress.bind this
-      keyup: @onKeyup.bind this
       # We cannot track keyup events if we lose the focus.
       blur: (event) => @alwaysContinueBubbling => @keydownEvents = {} if event.target == window
 
@@ -41,7 +38,7 @@ class KeyHandlerMode extends Mode
         keydown: (event) =>
           if KeyboardUtils.isEscape(event) and not @isInResetState()
             @reset()
-            DomUtils.suppressKeyupAfterEscape handlerStack
+            DomUtils.consumeKeyup event
           else
             @continueBubbling
 
@@ -49,45 +46,22 @@ class KeyHandlerMode extends Mode
     keyChar = KeyboardUtils.getKeyCharString event
     isEscape = KeyboardUtils.isEscape event
     if isEscape and (@countPrefix != 0 or @keyState.length != 1)
-      @keydownEvents[event.keyCode] = true
-      @reset()
-      @suppressEvent
+      DomUtils.consumeKeyup event, => @reset()
     # If the help dialog loses the focus, then Escape should hide it; see point 2 in #2045.
     else if isEscape and HelpDialog?.isShowing()
-      @keydownEvents[event.keyCode] = true
-      HelpDialog.toggle()
-      @suppressEvent
+      DomUtils.consumeKeyup event, -> HelpDialog.toggle()
     else if isEscape
       @continueBubbling
     else if @isMappedKey keyChar
-      @keydownEvents[event.keyCode] = true
-      @handleKeyChar keyChar
-    else if not keyChar and (keyChar = KeyboardUtils.getKeyChar event) and
-        (@isMappedKey(keyChar) or @isCountKey keyChar)
-      # We will possibly be handling a subsequent keypress event, so suppress propagation of this event to
-      # prevent triggering page event listeners (e.g. Google instant Search).
-      @keydownEvents[event.keyCode] = true
-      @suppressPropagation
-    else
-      @continueBubbling
-
-  onKeypress: (event) ->
-    keyChar = KeyboardUtils.getKeyCharString event
-    if @isMappedKey keyChar
-      @handleKeyChar keyChar
+      DomUtils.consumeKeyup event, => @handleKeyChar keyChar
     else if @isCountKey keyChar
       digit = parseInt keyChar
       @reset if @keyState.length == 1 then @countPrefix * 10 + digit else digit
       @suppressEvent
     else
-      @reset()
+      @reset() if keyChar
       @continueBubbling
 
-  onKeyup: (event) ->
-    return @continueBubbling unless event.keyCode of @keydownEvents
-    delete @keydownEvents[event.keyCode]
-    @suppressPropagation
-
   # This tests whether there is a mapping of keyChar in the current key state (and accounts for pass keys).
   isMappedKey: (keyChar) ->
     (mapping for mapping in @keyState when keyChar of mapping)[0]? and not @isPassKey keyChar
diff --git a/content_scripts/mode_visual.coffee b/content_scripts/mode_visual.coffee
index e4e4f541..cc1baf34 100644
--- a/content_scripts/mode_visual.coffee
+++ b/content_scripts/mode_visual.coffee
@@ -258,7 +258,7 @@ class VisualMode extends KeyHandlerMode
       _name: "#{@id}/enter/click"
       # Yank on <Enter>.
       keypress: (event) =>
-        if event.keyCode == keyCodes.enter
+        if event.key == "Enter"
           unless event.metaKey or event.ctrlKey or event.altKey or event.shiftKey
             @yank()
             return @suppressEvent
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 2331a8cf..cdb23352 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -451,14 +451,14 @@ extend window,
             name: "focus-selector"
             exitOnClick: true
             keydown: (event) =>
-              if event.keyCode == KeyboardUtils.keyCodes.tab
+              if event.key == "Tab"
                 hints[selectedInputIndex].classList.remove 'internalVimiumSelectedInputHint'
                 selectedInputIndex += hints.length + (if event.shiftKey then -1 else 1)
                 selectedInputIndex %= hints.length
                 hints[selectedInputIndex].classList.add 'internalVimiumSelectedInputHint'
                 DomUtils.simulateSelect visibleInputs[selectedInputIndex].element
                 @suppressEvent
-              else unless event.keyCode == KeyboardUtils.keyCodes.shiftKey
+              else unless event.key == "Shift"
                 @exit()
                 # Give the new mode the opportunity to handle the event.
                 @restartBubbling
@@ -687,4 +687,4 @@ root.bgLog = bgLog
 extend root, {handleEscapeForFindMode, handleEnterForFindMode, performFind, performBackwardsFind,
   enterFindMode, focusThisFrame}
 # These are exported only for the tests.
-extend root, {installModes, installListeners}
+extend root, {installModes}