Merge pull request #2079 from smblott-github/rename-handlerStack-constants-v3

Rename handler stack constants, and rework logic for greater clarity

6 files changed, 67 insertions, 63 deletions

diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee
index 59649e5d..254d9811 100644
--- a/content_scripts/link_hints.coffee
+++ b/content_scripts/link_hints.coffee
@@ -750,7 +750,6 @@ class WaitForEnter extends Mode
         else if KeyboardUtils.isEscape event
           @exit()
           callback false # false -> isSuccess.
-        DomUtils.suppressEvent event
 
 root = exports ? window
 root.LinkHints = LinkHints
diff --git a/content_scripts/mode.coffee b/content_scripts/mode.coffee
index 317fbc86..37321660 100644
--- a/content_scripts/mode.coffee
+++ b/content_scripts/mode.coffee
@@ -31,12 +31,15 @@ class Mode
   @modes: []
 
   # Constants; short, readable names for the return values expected by handlerStack.bubbleEvent.
-  continueBubbling: true
-  suppressEvent: false
-  stopBubblingAndTrue: handlerStack.stopBubblingAndTrue
-  stopBubblingAndFalse: handlerStack.stopBubblingAndFalse
+  continueBubbling: handlerStack.continueBubbling
+  suppressEvent: handlerStack.suppressEvent
+  passEventToPage: handlerStack.passEventToPage
+  suppressPropagation: handlerStack.suppressPropagation
   restartBubbling: handlerStack.restartBubbling
 
+  alwaysContinueBubbling: handlerStack.alwaysContinueBubbling
+  alwaysSuppressPropagation: handlerStack.alwaysSuppressPropagation
+
   constructor: (@options = {}) ->
     @handlers = []
     @exitHandlers = []
@@ -53,7 +56,8 @@ class Mode
     # or 2) to worry about event suppression and event-handler return values.
     if @options.suppressAllKeyboardEvents
       for type in [ "keydown", "keypress", "keyup" ]
-        @options[type] = @alwaysSuppressEvent @options[type]
+        do (handler = @options[type]) =>
+          @options[type] = (event) => @alwaysSuppressPropagation => handler? event
 
     @push
       keydown: @options.keydown || null
@@ -67,7 +71,7 @@ class Mode
         if @options.indicator?
           if HUD?.isReady()
             if @options.indicator then HUD.show @options.indicator else HUD.hide true, false
-          @stopBubblingAndTrue
+          @passEventToPage
         else @continueBubbling
 
     # If @options.exitOnEscape is truthy, then the mode will exit when the escape key is pressed.
@@ -126,7 +130,7 @@ class Mode
         _name: "mode-#{@id}/passInitialKeyupEvents"
         keydown: => @alwaysContinueBubbling -> handlerStack.remove()
         keyup: (event) =>
-          if KeyboardUtils.isPrintable event then @stopBubblingAndFalse else @stopBubblingAndTrue
+          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
@@ -136,16 +140,16 @@ class Mode
       @onExit ->
         handler = (event) ->
           if event.repeat
-            false # Suppress event.
+            handlerStack.suppressEvent
           else
             keyEventSuppressor.exit()
-            true # Do not suppress event.
+            handlerStack.continueBubbling
 
         keyEventSuppressor = new Mode
           name: "suppress-trailing-key-events"
           keydown: handler
           keypress: handler
-          keyup: -> handlerStack.stopBubblingAndFalse
+          keyup: -> handlerStack.suppressPropagation
 
     Mode.modes.push this
     @setIndicator()
@@ -181,18 +185,6 @@ class Mode
       @modeIsActive = false
       @setIndicator()
 
-  # Shorthand for an otherwise long name.  This wraps a handler with an arbitrary return value, and always
-  # yields @continueBubbling instead.  This simplifies handlers if they always continue bubbling (a common
-  # case), because they do not need to be concerned with the value they yield.
-  alwaysContinueBubbling: handlerStack.alwaysContinueBubbling
-
-  # Shorthand for an event handler which always suppresses event propagation.
-  alwaysSuppressEvent: (handler = null) ->
-    (event) =>
-      handler? event
-      DomUtils.suppressPropagation event
-      @stopBubblingAndFalse
-
   # Debugging routines.
   logModes: ->
     if Mode.debug
diff --git a/content_scripts/mode_insert.coffee b/content_scripts/mode_insert.coffee
index f86038d6..4cb0a39e 100644
--- a/content_scripts/mode_insert.coffee
+++ b/content_scripts/mode_insert.coffee
@@ -14,9 +14,9 @@ class InsertMode extends Mode
       # Check for a pass-next-key key.
       if KeyboardUtils.getKeyCharString(event) in Settings.get "passNextKeyKeys"
         new PassNextKeyMode
-        return false
+        return @suppressEvent
 
-      return @stopBubblingAndTrue unless event.type == 'keydown' and KeyboardUtils.isEscape event
+      return @passEventToPage unless event.type == 'keydown' and KeyboardUtils.isEscape event
       DomUtils.suppressKeyupAfterEscape handlerStack
       target = event.srcElement
       if target and DomUtils.isFocusable target
@@ -115,19 +115,19 @@ class PassNextKeyMode extends Mode
       # We exit on blur because, once we lose the focus, we can no longer track key events.
       exitOnBlur: window
       keypress: =>
-        @stopBubblingAndTrue
+        @passEventToPage
 
       keydown: =>
         seenKeyDown = true
         keyDownCount += 1
-        @stopBubblingAndTrue
+        @passEventToPage
 
       keyup: =>
         if seenKeyDown
           unless 0 < --keyDownCount
             unless 0 < --count
               @exit()
-        @stopBubblingAndTrue
+        @passEventToPage
 
 root = exports ? window
 root.InsertMode = InsertMode
diff --git a/content_scripts/mode_key_handler.coffee b/content_scripts/mode_key_handler.coffee
index 96792306..08222d98 100644
--- a/content_scripts/mode_key_handler.coffee
+++ b/content_scripts/mode_key_handler.coffee
@@ -40,12 +40,12 @@ class KeyHandlerMode extends Mode
     if isEscape and (@countPrefix != 0 or @keyState.length != 1)
       @keydownEvents[event.keyCode] = true
       @reset()
-      false # Suppress event.
+      @suppressEvent
     # If the help dialog loses the focus, then Escape should hide it; see point 2 in #2045.
     else if isEscape and HelpDialog?.showing
       @keydownEvents[event.keyCode] = true
       HelpDialog.hide()
-      false # Suppress event.
+      @suppressEvent
     else if isEscape
       @continueBubbling
     else if @isMappedKey keyChar
@@ -56,8 +56,7 @@ class KeyHandlerMode extends Mode
       # We will possibly be handling a subsequent keypress event, so suppress propagation of this event to
       # prevent triggering page event listeners (e.g. Google instant Search).
       @keydownEvents[event.keyCode] = true
-      DomUtils.suppressPropagation event
-      @stopBubblingAndTrue
+      @suppressPropagation
     else
       @continueBubbling
 
@@ -68,7 +67,7 @@ class KeyHandlerMode extends Mode
     else if @isCountKey keyChar
       digit = parseInt keyChar
       @reset if @keyState.length == 1 then @countPrefix * 10 + digit else digit
-      false # Suppress event.
+      @suppressEvent
     else
       @reset()
       @continueBubbling
@@ -76,8 +75,7 @@ class KeyHandlerMode extends Mode
   onKeyup: (event) ->
     return @continueBubbling unless event.keyCode of @keydownEvents
     delete @keydownEvents[event.keyCode]
-    DomUtils.suppressPropagation event
-    @stopBubblingAndTrue
+    @suppressPropagation
 
   # This tests whether there is a mapping of keyChar in the current key state (and accounts for pass keys).
   isMappedKey: (keyChar) ->
@@ -104,7 +102,7 @@ class KeyHandlerMode extends Mode
       bgLog "Call #{command.command}[#{count}] (#{@name})"
       @reset()
       @commandHandler {command, count}
-    false # Suppress event.
+    @suppressEvent
 
 root = exports ? window
 root.KeyHandlerMode = KeyHandlerMode
diff --git a/lib/handler_stack.coffee b/lib/handler_stack.coffee
index 2a44d26b..c17be24f 100644
--- a/lib/handler_stack.coffee
+++ b/lib/handler_stack.coffee
@@ -9,16 +9,22 @@ class HandlerStack
 
     # A handler should return this value to immediately discontinue bubbling and pass the event on to the
     # underlying page.
-    @stopBubblingAndTrue = new Object()
+    @passEventToPage = new Object()
 
     # A handler should return this value to indicate that the event has been consumed, and no further
     # processing should take place.  The event does not propagate to the underlying page.
-    @stopBubblingAndFalse = new Object()
+    @suppressPropagation = new Object()
 
     # A handler should return this value to indicate that bubbling should be restarted.  Typically, this is
     # used when, while bubbling an event, a new mode is pushed onto the stack.
     @restartBubbling = new Object()
 
+    # A handler should return this value to continue bubbling the event.
+    @continueBubbling = true
+
+    # A handler should return this value to suppress an event.
+    @suppressEvent = false
+
   # Adds a handler to the top of the stack. Returns a unique ID for that handler that can be used to remove it
   # later.
   push: (handler) ->
@@ -34,27 +40,35 @@ class HandlerStack
     handler.id = ++@counter
 
   # Called whenever we receive a key or other event. Each individual handler has the option to stop the
-  # event's propagation by returning a falsy value, or stop bubbling by returning @stopBubblingAndFalse or
-  # @stopBubblingAndTrue.
+  # event's propagation by returning a falsy value, or stop bubbling by returning @suppressPropagation or
+  # @passEventToPage.
   bubbleEvent: (type, event) ->
     @eventNumber += 1
     eventNumber = @eventNumber
-    # We take a copy of the array in order to avoid interference from concurrent removes (for example, to
-    # avoid calling the same handler twice, because elements have been spliced out of the array by remove).
     for handler in @stack[..].reverse()
-      # A handler may have been removed (handler.id == null), so check.
-      if handler?.id and handler[type]
+      # A handler might have been removed (handler.id == null), so check; or there might just be no handler
+      # for this type of event.
+      unless handler?.id and handler[type]
+        @logResult eventNumber, type, event, handler, "skip [#{handler[type]?}]" if @debug
+      else
         @currentId = handler.id
         result = handler[type].call this, event
         @logResult eventNumber, type, event, handler, result if @debug
-        if not result
+        if result == @passEventToPage
+          return true
+        else if result == @suppressPropagation
+          DomUtils.suppressPropagation event
+          return false
+        else if result == @restartBubbling
+          return @bubbleEvent type, event
+        else if result == @continueBubbling or (result and result != @suppressEvent)
+          true # Do nothing, but continue bubbling.
+        else
+          # result is @suppressEvent or falsy.
           DomUtils.suppressEvent event if @isChromeEvent event
           return false
-        return true if result == @stopBubblingAndTrue
-        return false if result == @stopBubblingAndFalse
-        return @bubbleEvent type, event if result == @restartBubbling
-      else
-        @logResult eventNumber, type, event, handler, "skip" if @debug
+
+    # None of our handlers care about this event, so pass it to the page.
     true
 
   remove: (id = @currentId) ->
@@ -74,21 +88,21 @@ class HandlerStack
   # Convenience wrappers.  Handlers must return an approriate value.  These are wrappers which handlers can
   # use to always return the same value.  This then means that the handler itself can be implemented without
   # regard to its return value.
-  alwaysContinueBubbling: (handler) ->
-    handler()
-    true
+  alwaysContinueBubbling: (handler = null) ->
+    handler?()
+    @continueBubbling
 
-  neverContinueBubbling: (handler) ->
-    handler()
-    false
+  alwaysSuppressPropagation: (handler = null) ->
+    handler?()
+    @suppressPropagation
 
   # Debugging.
   logResult: (eventNumber, type, event, handler, result) ->
     label =
       switch result
-        when @stopBubblingAndTrue then "stop/true"
-        when @stopBubblingAndFalse then "stop/false"
-        when @restartBubbling then "rebubble"
+        when @passEventToPage then "passEventToPage"
+        when @suppressPropagation then "suppressPropagation"
+        when @restartBubbling then "restartBubbling"
         when "skip" then "skip"
         when true then "continue"
     label ||= if result then "continue/truthy" else "suppress"
diff --git a/tests/unit_tests/handler_stack_test.coffee b/tests/unit_tests/handler_stack_test.coffee
index 629fc3ed..7b62af07 100644
--- a/tests/unit_tests/handler_stack_test.coffee
+++ b/tests/unit_tests/handler_stack_test.coffee
@@ -5,6 +5,7 @@ context "handlerStack",
   setup ->
     stub global, "DomUtils", {}
     stub DomUtils, "suppressEvent", ->
+    stub DomUtils, "suppressPropagation", ->
     @handlerStack = new HandlerStack
     @handler1Called = false
     @handler2Called = false
@@ -23,16 +24,16 @@ context "handlerStack",
     assert.isTrue @handler2Called
     assert.isFalse @handler1Called
 
-  should "terminate bubbling on stopBubblingAndTrue, and be true", ->
+  should "terminate bubbling on passEventToPage, and be true", ->
     @handlerStack.push { keydown: => @handler1Called = true }
-    @handlerStack.push { keydown: => @handler2Called = true; @handlerStack.stopBubblingAndTrue  }
+    @handlerStack.push { keydown: => @handler2Called = true; @handlerStack.passEventToPage }
     assert.isTrue @handlerStack.bubbleEvent 'keydown', {}
     assert.isTrue @handler2Called
     assert.isFalse @handler1Called
 
-  should "terminate bubbling on stopBubblingAndTrue, and be false", ->
+  should "terminate bubbling on passEventToPage, and be false", ->
     @handlerStack.push { keydown: => @handler1Called = true }
-    @handlerStack.push { keydown: => @handler2Called = true; @handlerStack.stopBubblingAndFalse  }
+    @handlerStack.push { keydown: => @handler2Called = true; @handlerStack.suppressPropagation }
     assert.isFalse @handlerStack.bubbleEvent 'keydown', {}
     assert.isTrue @handler2Called
     assert.isFalse @handler1Called