Force our key event handlers to have the highest possible priority

* The `window` object receives key events before the `document` object,
and so any event listeners on `window` get priority. This commit
switches from binding `keydown`, `keypress`, `keyup` on `document` to
on `window`.
* We were using `event.stopPropagation()` to prevent other event
listeners from firing if we had handled an event. This stopped the event
from propagating to other elements/objects and triggering their event
listeners, but didn't block event listeners registered on the same
object as ours. Switching to `event.stopImmediatePropagation()` ensures
that our event listener is the last to run for the event.

Fixing these issues allows Vimium to regain control over key events in
Google Groups (eg. the [vimium-dev
group](https://groups.google.com/forum/vimium-dev)).

5 files changed, 31 insertions, 18 deletions

diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 4f7becba..93d0bcd2 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -135,32 +135,45 @@ initializePreDomReady = ->
     # Ensure the sendResponse callback is freed.
     false
 
+    installEventListeners()
+
 # Wrapper to install event listeners.  Syntactic sugar.
-installListener = (event, callback) -> document.addEventListener(event, callback, true)
+installListener = (element, event, callback) ->
+  element.addEventListener(event, ->
+    if isEnabledForUrl then callback.apply(this, arguments) else true
+  , true)
 
 #
 # This is called once the background page has told us that Vimium should be enabled for the current URL.
-# We enable/disable Vimium by toggling isEnabledForUrl.  The alternative, installing or uninstalling
-# listeners, is error prone.  It's more difficult to keep track of the state.
+# We enable/disable Vimium by toggling isEnabledForUrl.
 #
-installedListeners = false
 initializeWhenEnabled = (newPassKeys) ->
   isEnabledForUrl = true
   passKeys = newPassKeys
+
+#
+# Installing or uninstalling listeners is error prone. Instead we elect to check isEnabledForUrl each time so
+# we know whether the listener should run or not.
+# Run this as early as possible, so the page can't register any event handlers before us.
+#
+installedListeners = false
+installEventListeners = ->
   if (!installedListeners)
-    installListener "keydown", (event) -> if isEnabledForUrl then onKeydown(event) else true
-    installListener "keypress", (event) -> if isEnabledForUrl then onKeypress(event) else true
-    installListener "keyup", (event) -> if isEnabledForUrl then onKeyup(event) else true
-    installListener "focus", (event) -> if isEnabledForUrl then onFocusCapturePhase(event) else true
-    installListener "blur", (event) -> if isEnabledForUrl then onBlurCapturePhase(event)
-    installListener "DOMActivate", (event) -> if isEnabledForUrl then onDOMActivate(event)
+    # Key event handlers fire on window before they do on document. Prefer window for key events so the page
+    # can't set handlers to grab the keys before us.
+    installListener window, "keydown", onKeydown
+    installListener window, "keypress", onKeypress
+    installListener window, "keyup", onKeyup
+    installListener document, "focus", onFocusCapturePhase
+    installListener document, "blur", onBlurCapturePhase
+    installListener document, "DOMActivate", onDOMActivate
     enterInsertModeIfElementIsFocused()
     installedListeners = true
 
 setState = (request) ->
   isEnabledForUrl = request.enabled
   passKeys = request.passKeys
-  initializeWhenEnabled(passKeys) if isEnabledForUrl and !installedListeners
+  initializeWhenEnabled(passKeys) if isEnabledForUrl
 
 #
 # The backend needs to know which frame has focus.
@@ -425,7 +438,7 @@ onKeydown = (event) ->
       handledKeydownEvents.push event
 
     else if (!modifiers)
-      event.stopPropagation()
+      event.stopImmediatePropagation()
       handledKeydownEvents.push event
 
   else if (isShowingHelpDialog && KeyboardUtils.isEscape(event))
@@ -457,7 +470,7 @@ onKeydown = (event) ->
   if (keyChar == "" && !isInsertMode() &&
      (currentCompletionKeys.indexOf(KeyboardUtils.getKeyChar(event)) != -1 ||
       isValidFirstKey(KeyboardUtils.getKeyChar(event))))
-    event.stopPropagation()
+    event.stopImmediatePropagation()
     handledKeydownEvents.push event
 
 onKeyup = (event) ->
@@ -473,7 +486,7 @@ onKeyup = (event) ->
        event.keyCode == keydown.keyCode
 
       handledKeydownEvents.splice i, 1
-      event.stopPropagation()
+      event.stopImmediatePropagation()
       break
 
 checkIfEnabledForUrl = ->
diff --git a/content_scripts/vomnibar.coffee b/content_scripts/vomnibar.coffee
index 22b9ed64..8c6b2059 100644
--- a/content_scripts/vomnibar.coffee
+++ b/content_scripts/vomnibar.coffee
@@ -139,7 +139,7 @@ class VomnibarUI
           @hide()
 
     # It seems like we have to manually suppress the event here and still return true.
-    event.stopPropagation()
+    event.stopImmediatePropagation()
     event.preventDefault()
     true
 
diff --git a/lib/dom_utils.coffee b/lib/dom_utils.coffee
index dcdd5518..8c0445c5 100644
--- a/lib/dom_utils.coffee
+++ b/lib/dom_utils.coffee
@@ -129,7 +129,7 @@ DomUtils =
 
   suppressEvent: (event) ->
     event.preventDefault()
-    event.stopPropagation()
+    event.stopImmediatePropagation()
 
 root = exports ? window
 root.DomUtils = DomUtils
diff --git a/tests/dom_tests/dom_tests.coffee b/tests/dom_tests/dom_tests.coffee
index ac3f9ebe..4a61877c 100644
--- a/tests/dom_tests/dom_tests.coffee
+++ b/tests/dom_tests/dom_tests.coffee
@@ -8,7 +8,7 @@ mockKeyboardEvent = (keyChar) ->
   event.charCode = (if keyCodes[keyChar] isnt undefined then keyCodes[keyChar] else keyChar.charCodeAt(0))
   event.keyIdentifier = "U+00" + event.charCode.toString(16)
   event.keyCode = event.charCode
-  event.stopPropagation = ->
+  event.stopImmediatePropagation = ->
   event.preventDefault = ->
   event
 
diff --git a/tests/dom_tests/vomnibar_test.coffee b/tests/dom_tests/vomnibar_test.coffee
index dc2a849f..b414fdfb 100644
--- a/tests/dom_tests/vomnibar_test.coffee
+++ b/tests/dom_tests/vomnibar_test.coffee
@@ -52,7 +52,7 @@ context "Keep selection within bounds",
 
     eventMock =
       preventDefault: ->
-      stopPropagation: ->
+      stopImmediatePropagation: ->
 
     @completions = [{html:'foo',type:'tab',url:'http://example.com'}]
     ui.update(true)