Merge pull request #156 from atom/io-modifier-keyups

Improve reliability of catching modifier key-up events

spec/helpers-spec.coffee

@@ -1,4 +1,4 @@
-{normalizeKeystrokes, keystrokesMatch} = require '../src/helpers'
+{normalizeKeystrokes, keystrokesMatch, isModifierKeyup} = require '../src/helpers'
 
 describe ".normalizeKeystrokes(keystrokes)", ->
   it "parses and normalizes the keystrokes", ->
@@ -32,32 +32,19 @@ describe ".normalizeKeystrokes(keystrokes)", ->
     assert.equal(normalizeKeystrokes('- '), false)
     assert.equal(normalizeKeystrokes('a '), false)
 
-describe ".keystrokesMatch(bindingKeystrokes, userKeystrokes)", ->
-  it "returns 'exact' for exact matches", ->
-    assert.equal(keystrokesMatch(['ctrl-tab', '^tab', '^ctrl'], ['ctrl-tab', '^tab', '^ctrl']), 'exact')
-    assert.equal(keystrokesMatch(['ctrl-tab', '^ctrl'], ['ctrl-tab', '^tab', '^ctrl']), 'exact')
-    assert.equal(keystrokesMatch(['a', 'b', 'c'], ['a', '^a', 'b', '^b', 'c']), 'exact')
-    assert.equal(keystrokesMatch(['a', 'b', '^b', 'c'], ['a', '^a', 'b', '^b', 'c']), 'exact')
-
-  it "returns false for non-matches", ->
-    assert.equal(keystrokesMatch(['ctrl-tab', '^tab'], ['ctrl-tab', '^tab', '^ctrl']), false)
-    assert.equal(keystrokesMatch(['a', 'b', 'c'], ['a', '^a', 'b', '^b', 'c', '^c']), false)
-    assert.equal(keystrokesMatch(['a', 'b', '^b', 'c'], ['a', '^a', 'b', '^b', 'c', '^c']), false)
-
-    assert.equal(keystrokesMatch(['a'], ['a', '^a', 'b', '^b', 'c', '^c']), false)
-    assert.equal(keystrokesMatch(['a'], ['a', '^a']), false)
-    assert.equal(keystrokesMatch(['a', 'c'], ['a', '^a', 'b', '^b', 'c', '^c']), false)
-    assert.equal(keystrokesMatch(['a', 'b', '^d'], ['a', '^a', 'b', '^b', 'c', '^c']), false)
-    assert.equal(keystrokesMatch(['a', 'd', '^d'], ['a', '^a', 'b', '^b', 'c', '^c']), false)
-    assert.equal(keystrokesMatch(['a', 'd', '^d'], ['^c']), false)
-
-  it "returns 'partial' for partial matches", ->
-    assert.equal(keystrokesMatch(['a', 'b', '^b'], ['a']), 'partial')
-    assert.equal(keystrokesMatch(['a', 'b', 'c'], ['a']), 'partial')
-    assert.equal(keystrokesMatch(['a', 'b', 'c'], ['a', '^a']), 'partial')
-    assert.equal(keystrokesMatch(['a', 'b', 'c'], ['a', '^a', 'b']), 'partial')
-    assert.equal(keystrokesMatch(['a', 'b', 'c'], ['a', '^a', 'b', '^b']), 'partial')
-    assert.equal(keystrokesMatch(['a', 'b', 'c'], ['a', '^a', 'd', '^d']), false)
-
-  it "returns 'keydownExact' for bindings that match and contain a remainder of only keyup events", ->
-    assert.equal(keystrokesMatch(['a', 'b', '^b'], ['a', 'b']), 'keydownExact')
+describe ".isModifierKeyup(keystroke)", ->
+  it "returns true for single modifier keyups", ->
+    assert.isTrue(isModifierKeyup('^ctrl'))
+    assert.isTrue(isModifierKeyup('^shift'))
+    assert.isTrue(isModifierKeyup('^alt'))
+    assert.isTrue(isModifierKeyup('^cmd'))
+    assert.isTrue(isModifierKeyup('^ctrl-shift'))
+    assert.isTrue(isModifierKeyup('^alt-cmd'))
+
+  it "returns false for modifier keydowns", ->
+    assert.isFalse(isModifierKeyup('ctrl-x'))
+    assert.isFalse(isModifierKeyup('shift-x'))
+    assert.isFalse(isModifierKeyup('alt-x'))
+    assert.isFalse(isModifierKeyup('cmd-x'))
+    assert.isFalse(isModifierKeyup('ctrl-shift-x'))
+    assert.isFalse(isModifierKeyup('alt-cmd-x'))

spec/key-binding-spec.coffee

@@ -0,0 +1,35 @@
+{KeyBinding, MATCH_TYPES} = require '../src/key-binding'
+
+describe "KeyBinding", ->
+  describe ".matchesKeystrokes(userKeystrokes)", ->
+    it "returns 'exact' for exact matches", ->
+      assert.equal(keyBindingArgHelper('ctrl-tab ^tab ^ctrl').matchesKeystrokes(['ctrl-tab', '^tab', '^ctrl']), 'exact')
+      assert.equal(keyBindingArgHelper('ctrl-tab ^ctrl').matchesKeystrokes(['ctrl-tab', '^tab', '^ctrl']), 'exact')
+      assert.equal(keyBindingArgHelper('a b c').matchesKeystrokes(['a', '^a', 'b', '^b', 'c']), 'exact')
+      assert.equal(keyBindingArgHelper('a b ^b c').matchesKeystrokes(['a', '^a', 'b', '^b', 'c']), 'exact')
+
+    it "returns false for non-matches", ->
+      assert.equal(keyBindingArgHelper('ctrl-tab ^tab').matchesKeystrokes(['ctrl-tab', '^tab', '^ctrl']), false)
+      assert.equal(keyBindingArgHelper('a b c').matchesKeystrokes(['a', '^a', 'b', '^b', 'c', '^c']), false)
+      assert.equal(keyBindingArgHelper('a b ^b c').matchesKeystrokes(['a', '^a', 'b', '^b', 'c', '^c']), false)
+
+      assert.equal(keyBindingArgHelper('a').matchesKeystrokes(['a', '^a', 'b', '^b', 'c', '^c']), false)
+      assert.equal(keyBindingArgHelper('a').matchesKeystrokes(['a', '^a']), false)
+      assert.equal(keyBindingArgHelper('a c').matchesKeystrokes(['a', '^a', 'b', '^b', 'c', '^c']), false)
+      assert.equal(keyBindingArgHelper('a b ^d').matchesKeystrokes(['a', '^a', 'b', '^b', 'c', '^c']), false)
+      assert.equal(keyBindingArgHelper('a d ^d').matchesKeystrokes(['a', '^a', 'b', '^b', 'c', '^c']), false)
+      assert.equal(keyBindingArgHelper('a d ^d').matchesKeystrokes(['^c']), false)
+
+    it "returns 'partial' for partial matches", ->
+      assert.equal(keyBindingArgHelper('a b ^b').matchesKeystrokes(['a']), 'partial')
+      assert.equal(keyBindingArgHelper('a b c').matchesKeystrokes(['a']), 'partial')
+      assert.equal(keyBindingArgHelper('a b c').matchesKeystrokes(['a', '^a']), 'partial')
+      assert.equal(keyBindingArgHelper('a b c').matchesKeystrokes(['a', '^a', 'b']), 'partial')
+      assert.equal(keyBindingArgHelper('a b c').matchesKeystrokes(['a', '^a', 'b', '^b']), 'partial')
+      assert.equal(keyBindingArgHelper('a b c').matchesKeystrokes(['a', '^a', 'd', '^d']), false)
+
+    it "returns MATCH_TYPES.PENDING_KEYUP for bindings that match and contain a remainder of only keyup events", ->
+      assert.equal(keyBindingArgHelper('a b ^b').matchesKeystrokes(['a', 'b']), MATCH_TYPES.PENDING_KEYUP)
+
+keyBindingArgHelper = (binding) ->
+  return new KeyBinding('test', 'test', binding, 'body', 0)

spec/keymap-manager-spec.coffee

@@ -403,6 +403,7 @@ describe "KeymapManager", ->
         elementA.addEventListener 'x-command-ctrl-up', (e) -> events.push('x-ctrl-keyup')
         elementA.addEventListener 'y-command-y-up-ctrl-up', (e) -> events.push('y-up-ctrl-keyup')
         elementA.addEventListener 'abc-secret-code-command', (e) -> events.push('abc-secret-code')
+        elementA.addEventListener 'z-command-d-e-f', (e) -> events.push('z-keydown-d-e-f')
 
         keymapManager.add "test",
           ".a":
@@ -411,6 +412,7 @@ describe "KeymapManager", ->
             "ctrl-x ^ctrl": "x-command-ctrl-up"
             "ctrl-y ^y ^ctrl": "y-command-y-up-ctrl-up"
             "a b c ^b ^a ^c": "abc-secret-code-command"
+            "ctrl-z d e f": "z-command-d-e-f"
 
       it "dispatches the command for a binding containing only keydown events immediately even when there is a corresponding multi-stroke binding that contains only other keyup events", ->
         keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'y', ctrlKey: true, target: elementA))
@@ -425,6 +427,15 @@ describe "KeymapManager", ->
         getFakeClock().tick(keymapManager.getPartialMatchTimeout())
         assert.deepEqual(events, ['y-keydown', 'y-up-ctrl-keyup'])
 
+      it "dispatches the command when the keyup comes after the partial match timeout", ->
+        keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'y', ctrlKey: true, target: elementA))
+        assert.deepEqual(events, ['y-keydown'])
+        keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'y', ctrlKey: true, cmd: true, shift: true, alt: true, target: elementA))
+        assert.deepEqual(events, ['y-keydown'])
+        getFakeClock().tick(keymapManager.getPartialMatchTimeout())
+        keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'ctrl', target: elementA))
+        assert.deepEqual(events, ['y-keydown', 'y-up-ctrl-keyup'])
+
       it "dispatches the command multiple times when multiple keydown events for the binding come in before the binding with a keyup handler", ->
         keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'y', ctrlKey: true, target: elementA))
         assert.deepEqual(events, ['y-keydown'])
@@ -450,14 +461,24 @@ describe "KeymapManager", ->
         getFakeClock().tick(keymapManager.getPartialMatchTimeout())
         assert.deepEqual(events, ['x-ctrl-keyup'])
 
-      it "does _not_ dispatch the command when extra user-generated keydown events are not specified in the binding", ->
+      it "dispatches the command when extra user-generated keydown events not specified in the binding occur between keydown and keyup", ->
         keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'y', ctrlKey: true, target: elementA))
         assert.deepEqual(events, ['y-keydown'])
-        keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'z', ctrlKey: true, target: elementA)) # not specified in binding
+        keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'j', ctrlKey: true, target: elementA)) # not specified in binding
         assert.deepEqual(events, ['y-keydown'])
         keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'Control', target: elementA))
         getFakeClock().tick(keymapManager.getPartialMatchTimeout())
-        assert.deepEqual(events, ['y-keydown'])
+        assert.deepEqual(events, ['y-keydown', 'y-ctrl-keyup'])
+
+      it "does _not_ dispatch the command when extra user-generated keydown events not specified in the binding occur between keydowns", ->
+        keymapManager.handleKeyboardEvent(buildKeydownEvent('z', ctrl: true, target: elementA))
+        keymapManager.handleKeyboardEvent(buildKeyupEvent('ctrl', target: elementA))
+        keymapManager.handleKeyboardEvent(buildKeyupEvent('ctrl', target: elementA))
+        keymapManager.handleKeyboardEvent(buildKeydownEvent('z', target: elementA)) # not specified in binding
+        keymapManager.handleKeyboardEvent(buildKeydownEvent('d', target: elementA))
+        keymapManager.handleKeyboardEvent(buildKeydownEvent('e', target: elementA))
+        keymapManager.handleKeyboardEvent(buildKeydownEvent('f', target: elementA))
+        assert.deepEqual(events, [])
 
       it "dispatches the command when multiple keyup keystrokes are specified", ->
         keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'a', target: elementA))

spec/partial-keyup-matcher-spec.js

@@ -0,0 +1,53 @@
+/** @babel */
+/* eslint-env mocha */
+/* global assert */
+
+const PartialKeyupMatcher = require('../src/partial-keyup-matcher.js')
+import {KeyBinding} from '../src/key-binding'
+
+describe('PartialKeyupMatcher', () => {
+  it('returns a simple single-modifier-keyup match', () => {
+    const matcher = new PartialKeyupMatcher()
+    const kb = keyBindingArgHelper('ctrl-tab ^ctrl')
+    matcher.addPendingMatch(kb)
+    const matches = matcher.getMatches('^ctrl')
+    assert.equal(matches.length, 1)
+    assert.equal(matches[0], kb)
+    it('removes match returned', () => {
+      const matches = matcher.getMatches('^ctrl')
+      assert.equal(matches.length, 0)
+    })
+  })
+
+  it('does not match multiple keyup binding on single keyup events', () => {
+    const matcher = new PartialKeyupMatcher()
+    const kb = keyBindingArgHelper('ctrl-shift-tab ^ctrl-shift')
+    matcher.addPendingMatch(kb)
+    let matches = matcher.getMatches('^ctrl')
+    assert.equal(matches.length, 0)
+    matches = matcher.getMatches('^shift')
+    assert.equal(matches.length, 0)
+  })
+
+  it('for multi-keystroke bindings, matches only when all keyups are received', () => {
+    const matcher = new PartialKeyupMatcher()
+    const kb = keyBindingArgHelper('ctrl-shift-tab ^ctrl ^shift')
+    matcher.addPendingMatch(kb)
+    matches = matcher.getMatches('^shift') // no-op should return no match
+    assert.equal(matches.length, 0)
+     // should return no match but set state to match on next ^ctrl
+    let matches = matcher.getMatches('^ctrl')
+    assert.equal(matches.length, 0)
+    matches = matcher.getMatches('^shift')
+    assert.equal(matches.length, 1)
+    assert.equal(matches[0], kb)
+    it('removes match returned', () => {
+      const matches = matcher.getMatches('^ctrl')
+      assert.equal(matches.length, 0)
+    })
+  })
+})
+
+function keyBindingArgHelper (binding) {
+  return new KeyBinding('test', 'test', binding, 'body', 0)
+}

src/helpers.coffee

@@ -16,11 +16,6 @@ NON_CHARACTER_KEY_NAMES_BY_KEYBOARD_EVENT_KEY = {
   'ArrowLeft': 'left',
   'ArrowRight': 'right'
 }
-MATCH_TYPES = {
-  EXACT: 'exact'
-  KEYDOWN_EXACT: 'keydownExact'
-  PARTIAL: 'partial'
-}
 
 isASCIICharacter = (character) ->
   character? and character.length is 1 and character.charCodeAt(0) <= 127
@@ -49,7 +44,7 @@ exports.normalizeKeystrokes = (keystrokes) ->
   normalizedKeystrokes.join(' ')
 
 normalizeKeystroke = (keystroke) ->
-  if isKeyup = keystroke.startsWith('^')
+  if keyup = isKeyup(keystroke)
     keystroke = keystroke.slice(1)
   keys = parseKeystroke(keystroke)
   return false unless keys
@@ -67,22 +62,22 @@ normalizeKeystroke = (keystroke) ->
       else
         return false
 
-  if isKeyup
+  if keyup
     primaryKey = primaryKey.toLowerCase() if primaryKey?
   else
     modifiers.add('shift') if isUpperCaseCharacter(primaryKey)
     if modifiers.has('shift') and isLowerCaseCharacter(primaryKey)
       primaryKey = primaryKey.toUpperCase()
 
   keystroke = []
-  if not isKeyup or (isKeyup and not primaryKey?)
+  if not keyup or (keyup and not primaryKey?)
     keystroke.push('ctrl') if modifiers.has('ctrl')
     keystroke.push('alt') if modifiers.has('alt')
     keystroke.push('shift') if modifiers.has('shift')
     keystroke.push('cmd') if modifiers.has('cmd')
   keystroke.push(primaryKey) if primaryKey?
   keystroke = keystroke.join('-')
-  keystroke = "^#{keystroke}" if isKeyup
+  keystroke = "^#{keystroke}" if keyup
   keystroke
 
 parseKeystroke = (keystroke) ->
@@ -191,18 +186,18 @@ exports.keystrokeForKeyboardEvent = (event, customKeystrokeResolvers) ->
         key = characters.unmodified
 
   keystroke = ''
-  if key is 'ctrl' or ctrlKey
+  if key is 'ctrl' or (ctrlKey and event.type isnt 'keyup')
     keystroke += 'ctrl'
 
-  if key is 'alt' or altKey
+  if key is 'alt' or (altKey and event.type isnt 'keyup')
     keystroke += '-' if keystroke.length > 0
     keystroke += 'alt'
 
-  if key is 'shift' or (shiftKey and (isNonCharacterKey or (isLatinCharacter(key) and isUpperCaseCharacter(key))))
+  if key is 'shift' or (shiftKey and event.type isnt 'keyup' and (isNonCharacterKey or (isLatinCharacter(key) and isUpperCaseCharacter(key))))
     keystroke += '-' if keystroke
     keystroke += 'shift'
 
-  if key is 'cmd' or metaKey
+  if key is 'cmd' or (metaKey and event.type isnt 'keyup')
     keystroke += '-' if keystroke
     keystroke += 'cmd'
 
@@ -231,19 +226,33 @@ nonAltModifiedKeyForKeyboardEvent = (event) ->
     else
       characters.unmodified
 
+exports.MODIFIERS = MODIFIERS
+
 exports.characterForKeyboardEvent = (event) ->
   event.key if event.key.length is 1 and not (event.ctrlKey or event.metaKey)
 
 exports.calculateSpecificity = calculateSpecificity
 
 exports.isBareModifier = (keystroke) -> ENDS_IN_MODIFIER_REGEX.test(keystroke)
 
+exports.isModifierKeyup = (keystroke) -> isKeyup(keystroke) and ENDS_IN_MODIFIER_REGEX.test(keystroke)
+
+exports.isKeyup = isKeyup = (keystroke) -> keystroke.startsWith('^')
+
 exports.keydownEvent = (key, options) ->
   return buildKeyboardEvent(key, 'keydown', options)
 
 exports.keyupEvent = (key, options) ->
   return buildKeyboardEvent(key, 'keyup', options)
 
+exports.getModifierKeys = (keystroke) ->
+  keys = keystroke.split('-')
+  mod_keys = []
+  for key in keys when MODIFIERS.has(key)
+    mod_keys.push(key)
+  mod_keys
+
+
 buildKeyboardEvent = (key, eventType, {ctrl, shift, alt, cmd, keyCode, target, location}={}) ->
   ctrlKey = ctrl ? false
   altKey = alt ? false
@@ -260,51 +269,3 @@ buildKeyboardEvent = (key, eventType, {ctrl, shift, alt, cmd, keyCode, target, l
     Object.defineProperty(event, 'target', get: -> target)
     Object.defineProperty(event, 'path', get: -> [target])
   event
-
-# bindingKeystrokes and userKeystrokes are arrays of keystrokes
-# e.g. ['ctrl-y', 'ctrl-x', '^x']
-exports.keystrokesMatch = (bindingKeystrokes, userKeystrokes) ->
-  userKeystrokeIndex = -1
-  userKeystrokesHasKeydownEvent = false
-  matchesNextUserKeystroke = (bindingKeystroke) ->
-    while userKeystrokeIndex < userKeystrokes.length - 1
-      userKeystrokeIndex += 1
-      userKeystroke = userKeystrokes[userKeystrokeIndex]
-      isKeydownEvent = not userKeystroke.startsWith('^')
-      userKeystrokesHasKeydownEvent = true if isKeydownEvent
-      if bindingKeystroke is userKeystroke
-        return true
-      else if isKeydownEvent
-        return false
-    null
-
-  isPartialMatch = false
-  bindingRemainderContainsOnlyKeyups = true
-  bindingKeystrokeIndex = 0
-  for bindingKeystroke in bindingKeystrokes
-    unless isPartialMatch
-      doesMatch = matchesNextUserKeystroke(bindingKeystroke)
-      if doesMatch is false
-        return false
-      else if doesMatch is null
-        # Make sure userKeystrokes with only keyup events doesn't match everything
-        if userKeystrokesHasKeydownEvent
-          isPartialMatch = true
-        else
-          return false
-
-    if isPartialMatch
-      bindingRemainderContainsOnlyKeyups = false unless bindingKeystroke.startsWith('^')
-
-  # Bindings that match the beginning of the user's keystrokes are not a match.
-  # e.g. This is not a match. It would have been a match on the previous keystroke:
-  # bindingKeystrokes = ['ctrl-tab', '^tab']
-  # userKeystrokes    = ['ctrl-tab', '^tab', '^ctrl']
-  return false if userKeystrokeIndex < userKeystrokes.length - 1
-
-  if isPartialMatch and bindingRemainderContainsOnlyKeyups
-    MATCH_TYPES.KEYDOWN_EXACT
-  else if isPartialMatch
-    MATCH_TYPES.PARTIAL
-  else
-    MATCH_TYPES.EXACT