Merge pull request #2753 from mrmr1993/always-consume-keyup

Always suppress keyups for keydowns that we handle, enforced in handlerStack

8 files changed, 20 insertions, 28 deletions

diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee
index 38ac3b28..33a876e0 100644
--- a/content_scripts/link_hints.coffee
+++ b/content_scripts/link_hints.coffee
@@ -166,7 +166,6 @@ class LinkHintsMode
       name: "hint/#{@mode.name}"
       indicator: false
       singleton: "link-hints-mode"
-      passInitialKeyupEvents: true
       suppressAllKeyboardEvents: true
       suppressTrailingKeyEvents: true
       exitOnEscape: true
@@ -294,7 +293,7 @@ class LinkHintsMode
           if keyChar.length == 1
             @markerMatcher.pushKeyChar keyChar
             @updateVisibleMarkers()
-          DomUtils.consumeKeyup event
+          handlerStack.suppressEvent
       return
 
     # We've handled the event, so suppress it and update the mode indicator.
diff --git a/content_scripts/marks.coffee b/content_scripts/marks.coffee
index 55f3137c..ac653a52 100644
--- a/content_scripts/marks.coffee
+++ b/content_scripts/marks.coffee
@@ -52,7 +52,7 @@ Marks =
             else
               localStorage[@getLocationKey keyChar] = @getMarkString()
               @showMessage "Created local mark", keyChar
-          DomUtils.consumeKeyup event
+          handlerStack.suppressEvent
 
   activateGotoMode: ->
     @mode = new Mode
@@ -82,7 +82,7 @@ Marks =
                 @showMessage "Jumped to local mark", keyChar
               else
                 @showMessage "Local mark not set", keyChar
-          DomUtils.consumeKeyup event
+          handlerStack.suppressEvent
 
 root = exports ? (window.root ?= {})
 root.Marks =  Marks
diff --git a/content_scripts/mode.coffee b/content_scripts/mode.coffee
index 2057d093..a4a91c1f 100644
--- a/content_scripts/mode.coffee
+++ b/content_scripts/mode.coffee
@@ -55,7 +55,7 @@ class Mode
     # the need for modes which suppress all keyboard events 1) to provide handlers for all of those events,
     # or 2) to worry about event suppression and event-handler return values.
     if @options.suppressAllKeyboardEvents
-      for type in [ "keydown", "keypress", "keyup" ]
+      for type in [ "keydown", "keypress" ]
         do (handler = @options[type]) =>
           @options[type] = (event) => @alwaysSuppressPropagation => handler? event
 
@@ -82,7 +82,7 @@ class Mode
         "keydown": (event) =>
           return @continueBubbling unless KeyboardUtils.isEscape event
           @exit event, event.target
-          DomUtils.consumeKeyup event
+          @suppressEvent
 
     # If @options.exitOnBlur is truthy, then it should be an element.  The mode will exit when that element
     # loses the focus.
@@ -120,16 +120,6 @@ class Mode
       singletons[key]?.exit()
       singletons[key] = this
 
-    # If @options.passInitialKeyupEvents is set, then we pass initial non-printable keyup events to the page
-    # or to other extensions (because the corresponding keydown events were passed).  This is used when
-    # activating link hints, see #1522.
-    if @options.passInitialKeyupEvents
-      @push
-        _name: "mode-#{@id}/passInitialKeyupEvents"
-        keydown: => @alwaysContinueBubbling -> handlerStack.remove()
-        keyup: (event) =>
-          if KeyboardUtils.isPrintable event then @suppressPropagation else @passEventToPage
-
     # if @options.suppressTrailingKeyEvents is set, then  -- on exit -- we suppress all key events until a
     # subsquent (non-repeat) keydown or keypress.  In particular, the intention is to catch keyup events for
     # keys which we have handled, but which otherwise might trigger page actions (if the page is listening for
@@ -147,7 +137,6 @@ class Mode
           name: "suppress-trailing-key-events"
           keydown: handler
           keypress: handler
-          keyup: -> handlerStack.suppressPropagation
 
     Mode.modes.push this
     @setIndicator()
diff --git a/content_scripts/mode_find.coffee b/content_scripts/mode_find.coffee
index b6c80cec..0fc147a3 100644
--- a/content_scripts/mode_find.coffee
+++ b/content_scripts/mode_find.coffee
@@ -16,7 +16,7 @@ class SuppressPrintable extends Mode
       keyup: (event) =>
         # If the selection type has changed (usually, no longer "Range"), then the user is interacting with
         # the input element, so we get out of the way.  See discussion of option 5c from #1415.
-        if document.getSelection().type != type then @exit() else handler event
+        @exit() if document.getSelection().type != type
 
 # When we use find, the selection/focus can land in a focusable/editable element.  In this situation, special
 # considerations apply.  We implement three special cases:
@@ -48,7 +48,7 @@ class PostFindMode extends SuppressPrintable
       keydown: (event) =>
         if KeyboardUtils.isEscape event
           @exit()
-          DomUtils.consumeKeyup event
+          @suppressEvent
         else
           handlerStack.remove()
           @continueBubbling
diff --git a/content_scripts/mode_insert.coffee b/content_scripts/mode_insert.coffee
index a43a129f..d2a33091 100644
--- a/content_scripts/mode_insert.coffee
+++ b/content_scripts/mode_insert.coffee
@@ -26,13 +26,12 @@ class InsertMode extends Mode
         # An editable element in a shadow DOM is focused; blur it.
         @insertModeLock.blur()
       @exit event, event.target
-      DomUtils.consumeKeyup event
+      @suppressEvent
 
     defaults =
       name: "insert"
       indicator: if not @permanent and not Settings.get "hideHud"  then "Insert mode"
       keypress: handleKeyEvent
-      keyup: handleKeyEvent
       keydown: handleKeyEvent
 
     super extend defaults, options
diff --git a/content_scripts/mode_key_handler.coffee b/content_scripts/mode_key_handler.coffee
index 0e84b9fb..cca6b77a 100644
--- a/content_scripts/mode_key_handler.coffee
+++ b/content_scripts/mode_key_handler.coffee
@@ -27,8 +27,6 @@ class KeyHandlerMode extends Mode
 
     super extend options,
       keydown: @onKeydown.bind this
-      # We cannot track keyup events if we lose the focus.
-      blur: (event) => @alwaysContinueBubbling => @keydownEvents = {} if event.target == window
 
     if options.exitOnEscape
       # If we're part way through a command's key sequence, then a first Escape should reset the key state,
@@ -38,7 +36,7 @@ class KeyHandlerMode extends Mode
         keydown: (event) =>
           if KeyboardUtils.isEscape(event) and not @isInResetState()
             @reset()
-            DomUtils.consumeKeyup event
+            @suppressEvent
           else
             @continueBubbling
 
@@ -49,11 +47,13 @@ class KeyHandlerMode extends Mode
       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()
-      DomUtils.consumeKeyup event, -> HelpDialog.toggle()
+      HelpDialog.toggle()
+      @suppressEvent
     else if isEscape
       @continueBubbling
     else if @isMappedKey keyChar
-      DomUtils.consumeKeyup event, => @handleKeyChar keyChar
+      @handleKeyChar keyChar
+      @suppressEvent
     else if @isCountKey keyChar
       digit = parseInt keyChar
       @reset if @keyState.length == 1 then @countPrefix * 10 + digit else digit
diff --git a/lib/dom_utils.coffee b/lib/dom_utils.coffee
index e771d436..b44f5f51 100644
--- a/lib/dom_utils.coffee
+++ b/lib/dom_utils.coffee
@@ -353,7 +353,8 @@ DomUtils =
           keyup: (event) ->
             return handlerStack.continueBubbling unless event.code == code
             @remove()
-            handlerStack.suppressEvent
+            DomUtils.suppressEvent event
+            handlerStack.continueBubbling
           # We cannot track keyup events if we lose the focus.
           blur: (event) ->
             @remove() if event.target == window
diff --git a/lib/handler_stack.coffee b/lib/handler_stack.coffee
index 2439f55c..646ddfbd 100644
--- a/lib/handler_stack.coffee
+++ b/lib/handler_stack.coffee
@@ -65,7 +65,11 @@ class HandlerStack
           true # Do nothing, but continue bubbling.
         else
           # result is @suppressEvent or falsy.
-          DomUtils.suppressEvent event if @isChromeEvent event
+          if @isChromeEvent event
+            if type == "keydown"
+              DomUtils.consumeKeyup event
+            else
+              DomUtils.suppressEvent event
           return false
 
     # None of our handlers care about this event, so pass it to the page.