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.

3 files changed, 20 insertions, 35 deletions

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 = ""