Refactor handlerStack. Closes #657.

Previously, handlerStack was designed only for removal of the handler
right at the top of the stack. However, some handlers sought to remove
themselves when they were not at the top of the stack, creating
confusion. The new handlerStack ensures that such removal can always be
done safely.

11 files changed, 119 insertions, 38 deletions

diff --git a/.gitignore b/.gitignore
index e5702ba4..633d6cc9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,3 +3,4 @@
 *.swp
 *.crx
 *.js
+node_modules/*
diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee
index a47f9263..7faa1a65 100644
--- a/content_scripts/link_hints.coffee
+++ b/content_scripts/link_hints.coffee
@@ -63,7 +63,7 @@ LinkHints =
       { id: "vimiumHintMarkerContainer", className: "vimiumReset" })
 
     # handlerStack is declared by vimiumFrontend.js
-    handlerStack.push({
+    @handlerId = handlerStack.push({
       keydown: @onKeyDownInMode.bind(this, hintMarkers),
       # trap all key events
       keypress: -> false
@@ -163,7 +163,7 @@ LinkHints =
         keyup: (event) ->
           return if (event.keyCode != keyCodes.shiftKey)
           LinkHints.setOpenLinkMode(!LinkHints.shouldOpenInNewTab, LinkHints.shouldOpenWithQueue, false)
-          handlerStack.pop()
+          @remove()
       })
 
     # TODO(philc): Ignore keys that have modifiers.
@@ -231,7 +231,7 @@ LinkHints =
       if (LinkHints.hintMarkerContainingDiv)
         DomUtils.removeElement LinkHints.hintMarkerContainingDiv
       LinkHints.hintMarkerContainingDiv = null
-      handlerStack.pop()
+      handlerStack.remove @handlerId
       HUD.hide()
       @isActive = false
 
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 29161ec6..5e5b4958 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -10,7 +10,7 @@ findModeQuery = { rawQuery: "" }
 findModeQueryHasResults = false
 findModeAnchorNode = null
 isShowingHelpDialog = false
-handlerStack = []
+handlerStack = new HandlerStack
 keyPort = null
 # Users can disable Vimium on URL patterns via the settings page.
 isEnabledForUrl = true
@@ -355,7 +355,9 @@ extend window,
         visibleInputs[selectedInputIndex].element.focus()
       else unless event.keyCode == KeyboardUtils.keyCodes.shiftKey
         DomUtils.removeElement hintContainingDiv
-        handlerStack.pop()
+        @remove()
+
+      false
 
 #
 # Sends everything except i & ESC to the handler in background_page. i & ESC are special because they control
@@ -365,7 +367,7 @@ extend window,
 # Note that some keys will only register keydown events and not keystroke events, e.g. ESC.
 #
 onKeypress = (event) ->
-  return unless bubbleEvent('keypress', event)
+  return unless handlerStack.bubbleEvent('keypress', event)
 
   keyChar = ""
 
@@ -381,32 +383,15 @@ onKeypress = (event) ->
     if (keyChar)
       if (findMode)
         handleKeyCharForFindMode(keyChar)
-        suppressEvent(event)
+        DomUtils.suppressEvent(event)
       else if (!isInsertMode() && !findMode)
         if (currentCompletionKeys.indexOf(keyChar) != -1)
-          suppressEvent(event)
+          DomUtils.suppressEvent(event)
 
         keyPort.postMessage({ keyChar:keyChar, frameId:frameId })
 
-#
-# Called whenever we receive a key event.  Each individual handler has the option to stop the event's
-# propagation by returning a falsy value.
-#
-bubbleEvent = (type, event) ->
-  for i in [(handlerStack.length - 1)..0]
-    # We need to check for existence of handler because the last function call may have caused the release of
-    # more than one handler.
-    if (handlerStack[i] && handlerStack[i][type] && !handlerStack[i][type](event))
-      suppressEvent(event)
-      return false
-  true
-
-suppressEvent = (event) ->
-  event.preventDefault()
-  event.stopPropagation()
-
 onKeydown = (event) ->
-  return unless bubbleEvent('keydown', event)
+  return unless handlerStack.bubbleEvent('keydown', event)
 
   keyChar = ""
 
@@ -442,20 +427,20 @@ onKeydown = (event) ->
       if (isEditable(event.srcElement))
         event.srcElement.blur()
       exitInsertMode()
-      suppressEvent(event)
+      DomUtils.suppressEvent(event)
 
   else if (findMode)
     if (KeyboardUtils.isEscape(event))
       handleEscapeForFindMode()
-      suppressEvent(event)
+      DomUtils.suppressEvent(event)
 
     else if (event.keyCode == keyCodes.backspace || event.keyCode == keyCodes.deleteKey)
       handleDeleteForFindMode()
-      suppressEvent(event)
+      DomUtils.suppressEvent(event)
 
     else if (event.keyCode == keyCodes.enter)
       handleEnterForFindMode()
-      suppressEvent(event)
+      DomUtils.suppressEvent(event)
 
     else if (!modifiers)
       event.stopPropagation()
@@ -466,7 +451,7 @@ onKeydown = (event) ->
   else if (!isInsertMode() && !findMode)
     if (keyChar)
       if (currentCompletionKeys.indexOf(keyChar) != -1)
-        suppressEvent(event)
+        DomUtils.suppressEvent(event)
 
       keyPort.postMessage({ keyChar:keyChar, frameId:frameId })
 
@@ -485,7 +470,7 @@ onKeydown = (event) ->
       isValidFirstKey(KeyboardUtils.getKeyChar(event))))
     event.stopPropagation()
 
-onKeyup = () -> return unless bubbleEvent('keyup', event)
+onKeyup = (event) -> return unless handlerStack.bubbleEvent('keyup', event)
 
 checkIfEnabledForUrl = ->
   url = window.location.toString()
@@ -750,7 +735,7 @@ findAndFocus = (backwards) ->
   if (elementCanTakeInput)
     handlerStack.push({
       keydown: (event) ->
-        handlerStack.pop()
+        @remove()
         if (KeyboardUtils.isEscape(event))
           DomUtils.simulateSelect(document.activeElement)
           enterInsertModeWithoutShowingIndicator(document.activeElement)
diff --git a/content_scripts/vomnibar.coffee b/content_scripts/vomnibar.coffee
index 32d5dbac..724f4874 100644
--- a/content_scripts/vomnibar.coffee
+++ b/content_scripts/vomnibar.coffee
@@ -52,13 +52,13 @@ class VomnibarUI
   show: ->
     @box.style.display = "block"
     @input.focus()
-    handlerStack.push({ keydown: @onKeydown.bind(this) })
+    @handlerId = handlerStack.push keydown: @onKeydown.bind @
 
   hide: ->
     @box.style.display = "none"
     @completionList.style.display = "none"
     @input.blur()
-    handlerStack.pop()
+    handlerStack.remove @handlerId
 
   reset: ->
     @input.value = ""
diff --git a/lib/dom_utils.coffee b/lib/dom_utils.coffee
index 30530e0d..501e43a5 100644
--- a/lib/dom_utils.coffee
+++ b/lib/dom_utils.coffee
@@ -131,5 +131,9 @@ DomUtils =
     document.documentElement.appendChild(flashEl)
     setTimeout((-> DomUtils.removeElement flashEl), 400)
 
+  suppressEvent: (event) ->
+    event.preventDefault()
+    event.stopPropagation()
+
 root = exports ? window
 root.DomUtils = DomUtils
diff --git a/lib/handler_stack.coffee b/lib/handler_stack.coffee
new file mode 100644
index 00000000..858f2ec9
--- /dev/null
+++ b/lib/handler_stack.coffee
@@ -0,0 +1,37 @@
+root = exports ? window
+
+class root.HandlerStack
+
+  constructor: ->
+    @stack = []
+    @counter = 0
+
+  genId: -> @counter = ++@counter & 0xffff
+
+  # Adds a handler to the stack. Returns a unique ID for that handler that can be used to remove it later.
+  push: (handler) ->
+    handler.id = @genId()
+    @stack.push handler
+    handler.id
+
+  # Called whenever we receive a key event. Each individual handler has the option to stop the event's
+  # propagation by returning a falsy value.
+  bubbleEvent: (type, event) ->
+    for i in [(@stack.length - 1)..0] by -1
+      handler = @stack[i]
+      # We need to check for existence of handler because the last function call may have caused the release
+      # of more than one handler.
+      if handler && handler[type]
+        @currentId = handler.id
+        passThrough = handler[type].call(@, event)
+        if not passThrough
+          DomUtils.suppressEvent(event)
+          return false
+    true
+
+  remove: (id = @currentId) ->
+    for i in [(@stack.length - 1)..0] by -1
+      handler = @stack[i]
+      if handler.id == id
+        @stack.splice(i, 1)
+        break
diff --git a/manifest.json b/manifest.json
index 8ec9d400..bfccb920 100644
--- a/manifest.json
+++ b/manifest.json
@@ -30,6 +30,7 @@
       "js": ["lib/utils.js",
              "lib/keyboard_utils.js",
              "lib/dom_utils.js",
+             "lib/handler_stack.js",
              "lib/clipboard.js",
              "content_scripts/link_hints.js",
              "content_scripts/vomnibar.js",
diff --git a/test_harnesses/vomnibar.html b/test_harnesses/vomnibar.html
index af1d0a19..f9528548 100644
--- a/test_harnesses/vomnibar.html
+++ b/test_harnesses/vomnibar.html
@@ -15,7 +15,7 @@
   <link rel="stylesheet" type="text/css" href="../vimium.css" />
   <script>
     function setup() {
-      window.handlerStack = [];
+      window.handlerStack = new HandlerStack();
       // This itemHtml was obtained just by copying and pasting what was generated when using it in practice.
       var itemHtml = '<span class="source">history</span> http://<span class="fuzzyMatch">n</span><span class="fuzzyMatch">i</span><span class="fuzzyMatch">n</span><span class="fuzzyMatch">j</span><span class="fuzzyMatch">a</span>words.com/info/about <span class="title">Ninjawords - a really fast dictionary</span>';
       var results = [{ html: itemHtml }, { html: itemHtml }];
diff --git a/tests/dom_tests/dom_tests.coffee b/tests/dom_tests/dom_tests.coffee
index 3d981ee5..a0254acf 100644
--- a/tests/dom_tests/dom_tests.coffee
+++ b/tests/dom_tests/dom_tests.coffee
@@ -177,11 +177,11 @@ context "Input focus",
     focusInput 1
     assert.equal "first", document.activeElement.id
     # deactivate the tabbing mode and its overlays
-    handlerStack[handlerStack.length - 1].keydown mockKeyboardEvent("A")
+    handlerStack.bubbleEvent 'keydown', mockKeyboardEvent("A")
 
     focusInput 100
     assert.equal "third", document.activeElement.id
-    handlerStack[handlerStack.length - 1].keydown mockKeyboardEvent("A")
+    handlerStack.bubbleEvent 'keydown', mockKeyboardEvent("A")
 
 Tests.outputMethod = (args...) ->
   newOutput = args.join "\n"
diff --git a/tests/dom_tests/dom_tests.html b/tests/dom_tests/dom_tests.html
index 1dc45782..c658235b 100644
--- a/tests/dom_tests/dom_tests.html
+++ b/tests/dom_tests/dom_tests.html
@@ -32,6 +32,7 @@
     <script type="text/javascript" src="../../lib/utils.js"></script>
     <script type="text/javascript" src="../../lib/keyboard_utils.js"></script>
     <script type="text/javascript" src="../../lib/dom_utils.js"></script>
+    <script type="text/javascript" src="../../lib/handler_stack.js"></script>
     <script type="text/javascript" src="../../lib/clipboard.js"></script>
     <script type="text/javascript" src="../../content_scripts/link_hints.js"></script>
     <script type="text/javascript" src="../../content_scripts/vomnibar.js"></script>
diff --git a/tests/unit_tests/handler_stack_test.coffee b/tests/unit_tests/handler_stack_test.coffee
new file mode 100644
index 00000000..0ed8f4c0
--- /dev/null
+++ b/tests/unit_tests/handler_stack_test.coffee
@@ -0,0 +1,52 @@
+require "./test_helper.js"
+extend(global, require "../../lib/handler_stack.js")
+
+context "handlerStack",
+  setup ->
+    stub global, "DomUtils", {}
+    stub DomUtils, "suppressEvent", ->
+    @handlerStack = new HandlerStack
+    @handler1Called = false
+    @handler2Called = false
+
+  should "bubble events", ->
+    @handlerStack.push { keydown: => @handler1Called = true }
+    @handlerStack.push { keydown: => @handler2Called = true }
+    @handlerStack.bubbleEvent 'keydown', {}
+    assert.isTrue @handler2Called
+    assert.isTrue @handler1Called
+
+  should "terminate bubbling on falsy return value", ->
+    @handlerStack.push { keydown: => @handler1Called = true }
+    @handlerStack.push { keydown: => @handler2Called = true; false }
+    @handlerStack.bubbleEvent 'keydown', {}
+    assert.isTrue @handler2Called
+    assert.isFalse @handler1Called
+
+  should "remove handlers correctly", ->
+    @handlerStack.push { keydown: => @handler1Called = true }
+    handlerId = @handlerStack.push { keydown: => @handler2Called = true }
+    @handlerStack.remove handlerId
+    @handlerStack.bubbleEvent 'keydown', {}
+    assert.isFalse @handler2Called
+    assert.isTrue @handler1Called
+
+  should "remove handlers correctly", ->
+    handlerId = @handlerStack.push { keydown: => @handler1Called = true }
+    @handlerStack.push { keydown: => @handler2Called = true }
+    @handlerStack.remove handlerId
+    @handlerStack.bubbleEvent 'keydown', {}
+    assert.isTrue @handler2Called
+    assert.isFalse @handler1Called
+
+  should "handle self-removing handlers correctly", ->
+    ctx = @
+    @handlerStack.push { keydown: => @handler1Called = true }
+    @handlerStack.push { keydown: ->
+      ctx.handler2Called = true
+      @remove()
+    }
+    @handlerStack.bubbleEvent 'keydown', {}
+    assert.isTrue @handler2Called
+    assert.isTrue @handler1Called
+    assert.equal @handlerStack.stack.length, 1