Modes; more changes...

- Better comments.
- Strip unnecessary handlers for leaving post-find mode.
- Simplify passKeys.
- focusInput now re-bubbles its triggering keydown event.

6 files changed, 74 insertions, 74 deletions

diff --git a/content_scripts/mode.coffee b/content_scripts/mode.coffee
index b6cb5fae..37f3a8c2 100644
--- a/content_scripts/mode.coffee
+++ b/content_scripts/mode.coffee
@@ -45,11 +45,12 @@ class Mode
   # If this is true, then we generate a trace of modes being activated and deactivated on the console.
   @debug = true
 
-  # Constants; readable shortcuts for event-handler return values.
+  # Constants; short, readable names for handlerStack event-handler return values.
   continueBubbling: true
   suppressEvent: false
   stopBubblingAndTrue: handlerStack.stopBubblingAndTrue
   stopBubblingAndFalse: handlerStack.stopBubblingAndFalse
+  restartBubbling: handlerStack.restartBubbling
 
   constructor: (@options={}) ->
     @handlers = []
diff --git a/content_scripts/mode_find.coffee b/content_scripts/mode_find.coffee
index 3b9f951e..d63b3319 100644
--- a/content_scripts/mode_find.coffee
+++ b/content_scripts/mode_find.coffee
@@ -2,11 +2,11 @@
 
 # When we use find mode, the selection/focus can end up in a focusable/editable element.  In this situation,
 # special considerations apply.  We implement three special cases:
-#   1. Be an InsertModeBlocker.  This prevents keyboard events from dropping us unintentionally into insert
-#      mode. This is achieved by inheriting from InsertModeBlocker.
+#   1. Prevent keyboard events from dropping us unintentionally into insert mode. This is achieved by
+#      inheriting from InsertModeBlocker.
 #   2. Prevent all keyboard events on the active element from propagating.  This is achieved by setting the
 #      trapAllKeyboardEvents option.  There's some controversy as to whether this is the right thing to do.
-#      See discussion in #1415. This implements option 2 from there, although option 3 would be a reasonable
+#      See discussion in #1415. This implements Option 2 from there, although Option 3 would be a reasonable
 #      alternative.
 #   3. If the very-next keystroke is Escape, then drop immediately into insert mode.
 #
@@ -16,9 +16,10 @@ class PostFindMode extends InsertModeBlocker
 
     super
       name: "post-find"
-      # Be a singleton.  That way, we don't have to keep track of any currently-active instance.  Such  an
-      # instance is automatically deactivated when a new instance is created.
+      # Be a singleton.  That way, we don't have to keep track of any currently-active instance.  Any active
+      # instance is automatically deactivated when a new instance is activated.
       singleton: PostFindMode
+      exitOnBlur: element
       trapAllKeyboardEvents: element
 
     return @exit() unless element and findModeAnchorNode
@@ -42,11 +43,5 @@ class PostFindMode extends InsertModeBlocker
         @remove()
         true
 
-    # Various ways in which we can leave PostFindMode.
-    @push
-      focus: (event) => @alwaysContinueBubbling => @exit()
-      blur: (event) => @alwaysContinueBubbling => @exit()
-      keydown: (event) => @alwaysContinueBubbling => @exit() if document.activeElement != element
-
 root = exports ? window
 root.PostFindMode = PostFindMode
diff --git a/content_scripts/mode_insert.coffee b/content_scripts/mode_insert.coffee
index 9a2d5ce1..144b0be6 100644
--- a/content_scripts/mode_insert.coffee
+++ b/content_scripts/mode_insert.coffee
@@ -1,5 +1,5 @@
 
-# This mode is installed when insert mode is active.
+# This mode is installed only when insert mode is active.
 class InsertMode extends Mode
   constructor: (options = {}) ->
     defaults =
@@ -11,10 +11,10 @@ class InsertMode extends Mode
       keyup: (event) => @stopBubblingAndTrue
       exitOnEscape: true
       blurOnExit: true
+      targetElement: null
 
-    options = extend defaults, options
-    options.exitOnBlur = options.targetElement || null
-    super options
+    options.exitOnBlur ||= options.targetElement
+    super extend defaults, options
     triggerSuppressor.suppress()
 
   exit: (event = null) ->
@@ -34,8 +34,8 @@ class InsertMode extends Mode
 #   - On a keydown event in a contentEditable element.
 #   - When a focusable element receives the focus.
 #
-# The trigger can be suppressed via triggerSuppressor; see InsertModeBlocker, below.
-# This mode is permanently installed fairly low down on the handler stack.
+# The trigger can be suppressed via triggerSuppressor; see InsertModeBlocker, below.  This mode is permanently
+# installed (just above normal mode and passkeys mode) on the handler stack.
 class InsertModeTrigger extends Mode
   constructor: ->
     super
@@ -44,7 +44,7 @@ class InsertModeTrigger extends Mode
         triggerSuppressor.unlessSuppressed =>
           # Some sites (e.g. inbox.google.com) change the contentEditable attribute on the fly (see #1245);
           # and unfortunately, the focus event happens *before* the change is made.  Therefore, we need to
-          # check again whether the active element is contentEditable.
+          # check (on every keydown) whether the active element is contentEditable.
           return @continueBubbling unless document.activeElement?.isContentEditable
           new InsertMode
             targetElement: document.activeElement
@@ -58,7 +58,7 @@ class InsertModeTrigger extends Mode
               new InsertMode
                 targetElement: event.target
 
-    # We may already have focussed an input, so check.
+    # We may have already focussed an input element, so check.
     if document.activeElement and DomUtils.isEditable document.activeElement
       new InsertMode
         targetElement: document.activeElement
@@ -66,12 +66,13 @@ class InsertModeTrigger extends Mode
 # Used by InsertModeBlocker to suppress InsertModeTrigger; see below.
 triggerSuppressor = new Utils.Suppressor true # Note: true == @continueBubbling
 
-# Suppresses InsertModeTrigger.  This is used by various modes (usually by inheritance) to prevent
+# Suppresses InsertModeTrigger.  This is used by various modes (usually via inheritance) to prevent
 # unintentionally dropping into insert mode on focusable elements.
 class InsertModeBlocker extends Mode
   constructor: (options = {}) ->
     triggerSuppressor.suppress()
     options.name ||= "insert-blocker"
+    # See "click" handler below for an explanation of options.onClickMode.
     options.onClickMode ||= InsertMode
     super options
     @onExit -> triggerSuppressor.unsuppress()
@@ -79,27 +80,29 @@ class InsertModeBlocker extends Mode
     @push
       "click": (event) =>
         @alwaysContinueBubbling =>
-          # The user knows best; so, if the user clicks on something, we get out of the way.
+          # The user knows best; so, if the user clicks on something, the insert-mode blocker gets out of the
+          # way.
           @exit event
-          # However, there's a corner case.  If the active element is focusable, then we would have been in
-          # insert mode had we not been blocking the trigger.  Now, clicking on the element will not generate
-          # a new focus event, so the insert-mode trigger will not fire.  We have to handle this case
-          # specially.  @options.onClickMode is the mode to use.
+          # However, there's a corner case.  If the active element is focusable, then, had we not been
+          # blocking the trigger, we would already have been in insert mode.  Now, a click on that element
+          # will not generate a new focus event, so the insert-mode trigger will not fire.  We have to handle
+          # this case specially.  @options.onClickMode specifies the mode to use (by default, insert mode).
           if document.activeElement and
-              event.target == document.activeElement and DomUtils.isEditable document.activeElement
+             event.target == document.activeElement and DomUtils.isEditable document.activeElement
             new @options.onClickMode
               targetElement: document.activeElement
 
 # There's some unfortunate feature interaction with chrome's content editable handling.  If the selection is
 # content editable and a descendant of the active element, then chrome focuses it on any unsuppressed keyboard
-# events.  This has the unfortunate effect of dropping us unintentally into insert mode.  See #1415.
-# This mode sits near the bottom of the handler stack and suppresses keyboard events if:
+# event.  This has the unfortunate effect of dropping us unintentally into insert mode.  See #1415.
+# A single instance of this mode sits near the bottom of the handler stack and suppresses keyboard events if:
 #   - they haven't been handled by any other mode (so not by normal mode, passkeys mode, insert mode, and so
-#     on), and
+#     on),
 #   - the selection is content editable, and
 #   - the selection is a descendant of the active element.
 # This should rarely fire, typically only on fudged keypresses in normal mode.  And, even then, only in the
-# circumstances outlined above.  So it shouldn't normally block other extensions or the page itself from
+# circumstances outlined above.  So, we shouldn't usually be blocking keyboard events for other extensions or
+# the page itself.
 # handling keyboard events.
 new class ContentEditableTrap extends Mode
   constructor: ->
@@ -109,17 +112,15 @@ new class ContentEditableTrap extends Mode
       keypress: (event) => @handle => @suppressEvent
       keyup: (event) => @handle => @suppressEvent
 
-  # True if the selection is content editable and a descendant of the active element.  In this situation,
-  # chrome unilaterally focuses the element containing the anchor, dropping us into insert mode.
+  handle: (func) -> if @isContentEditableFocused() then func() else @continueBubbling
+
+  # True if the selection is content editable and a descendant of the active element.
   isContentEditableFocused: ->
     element = document.getSelection()?.anchorNode?.parentElement
-    return element?.isContentEditable? and
-             document.activeElement? and
+    return element?.isContentEditable and
+             document.activeElement and
              DomUtils.isDOMDescendant document.activeElement, element
 
-  handle: (func) ->
-    if @isContentEditableFocused() then func() else @continueBubbling
-
 root = exports ? window
 root.InsertMode = InsertMode
 root.InsertModeTrigger = InsertModeTrigger
diff --git a/content_scripts/mode_passkeys.coffee b/content_scripts/mode_passkeys.coffee
index 972dcad7..112e14ed 100644
--- a/content_scripts/mode_passkeys.coffee
+++ b/content_scripts/mode_passkeys.coffee
@@ -3,24 +3,23 @@ class PassKeysMode extends Mode
   constructor: ->
     super
       name: "passkeys"
-      keydown: (event) => @handlePassKeyEvent event
-      keypress: (event) => @handlePassKeyEvent event
       trackState: true
+      keydown: (event) => @handleKeyChar KeyboardUtils.getKeyChar event
+      keypress: (event) => @handleKeyChar String.fromCharCode event.charCode
+      keyup: (event) => @handleKeyChar String.fromCharCode event.charCode
 
   # Decide whether this event should be passed to the underlying page.  Keystrokes are *never* considered
   # passKeys if the keyQueue is not empty.  So, for example, if 't' is a passKey, then 'gt' and '99t' will
   # neverthless be handled by vimium.
-  handlePassKeyEvent: (event) ->
-    for keyChar in [KeyboardUtils.getKeyChar(event), String.fromCharCode(event.charCode)]
-      return @stopBubblingAndTrue if keyChar and not @keyQueue and 0 <= @passKeys.indexOf(keyChar)
+  handleKeyChar: (keyChar) ->
+    return @stopBubblingAndTrue if keyChar and not @keyQueue and 0 <= @passKeys.indexOf keyChar
     @continueBubbling
 
   configure: (request) ->
     @keyQueue = request.keyQueue if request.keyQueue?
 
   chooseBadge: (badge) ->
-    @badge = if @passKeys and not @keyQueue then "P" else ""
-    super badge
+    badge.badge ||= "P" if @passKeys and not @keyQueue
 
 root = exports ? window
 root.PassKeysMode = PassKeysMode
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 97fbc56f..a9bf30a3 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -384,8 +384,8 @@ extend window,
           # shouldn't happen anyway.  However, it does no harm to enforce it.
           singleton: FocusSelector
           targetMode: targetMode
-          # For the InsertModeBlocker super-class (we'll always choose InsertMode on click).  See comment in
-          # InsertModeBlocker for an explanation of why this is needed.
+          # Set the target mode for when/if the active element is clicked.  Usually, the target is insert
+          # mode.  See comment in InsertModeBlocker for an explanation of why this is needed.
           onClickMode: targetMode
           keydown: (event) =>
             if event.keyCode == KeyboardUtils.keyCodes.tab
@@ -397,22 +397,22 @@ extend window,
               @suppressEvent
             else unless event.keyCode == KeyboardUtils.keyCodes.shiftKey
               @exit event
-              @continueBubbling
+              # In @exit(), we just pushed a new mode (usually insert mode).  Restart bubbling, so that the
+              # new mode can now see the event too.
+              @restartBubbling
 
         visibleInputs[selectedInputIndex].element.focus()
-        if visibleInputs.length == 1
-          @exit()
-        else
-          hints[selectedInputIndex].classList.add 'internalVimiumSelectedInputHint'
+        return @exit() if visibleInputs.length == 1
+
+        hints[selectedInputIndex].classList.add 'internalVimiumSelectedInputHint'
 
       exit: ->
         super()
         DomUtils.removeElement hintContainingDiv
         if document.activeElement == visibleInputs[selectedInputIndex].element
-          # The InsertModeBlocker super-class  handles the "click" case.
+          # The InsertModeBlocker super-class  handles "click" events, so we should skip it here.
           unless event?.type == "click"
-            # In the legacy (and probably common) case, we're entering insert mode here.  However, it could be
-            # some other mode.
+            # In most cases, we're entering insert mode here.  However, it could be some other mode.
             new @options.targetMode
               targetElement: document.activeElement
 
@@ -455,7 +455,7 @@ KeydownEvents =
 # Note that some keys will only register keydown events and not keystroke events, e.g. ESC.
 #
 
-onKeypress = (event, extra) ->
+onKeypress = (event) ->
   keyChar = ""
 
   # Ignore modifier keys by themselves.
@@ -484,7 +484,7 @@ onKeypress = (event, extra) ->
 
   return true
 
-onKeydown = (event, extra) ->
+onKeydown = (event) ->
   keyChar = ""
 
   # handle special keys, and normal input keys with modifiers being pressed. don't handle shiftKey alone (to
@@ -789,6 +789,7 @@ class FindMode extends InsertModeBlocker
     super()
     handleEscapeForFindMode() if event?.type == "keydown" and KeyboardUtils.isEscape event
     handleEscapeForFindMode() if event?.type == "click"
+    # If event?.type == "click", then the InsertModeBlocker super-class will be dropping us into insert mode.
     new PostFindMode findModeAnchorNode unless event?.type == "click"
 
 performFindInPlace = ->
diff --git a/lib/handler_stack.coffee b/lib/handler_stack.coffee
index 97e189c5..44c7538b 100644
--- a/lib/handler_stack.coffee
+++ b/lib/handler_stack.coffee
@@ -14,6 +14,11 @@ class HandlerStack
     # processing should take place.
     @stopBubblingAndFalse = 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.  See `focusInput` for an
+    # example.
+    @restartBubbling = new Object()
+
   # 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) ->
@@ -30,30 +35,26 @@ class HandlerStack
   # event's propagation by returning a falsy value, or stop bubbling by returning @stopBubblingAndFalse or
   # @stopBubblingAndTrue.
   bubbleEvent: (type, event) ->
-    # extra is passed to each handler.  This allows handlers to pass information down the stack.
-    extra = {}
-    # We take a copy of the array, here, in order to avoid interference from concurrent removes (for example,
-    # to avoid calling the same handler twice).
+    # 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).
-      if handler and handler.id
+      # A handler may have been removed (handler.id == null), so check.
+      if handler?.id and handler[type]
         @currentId = handler.id
-        # A handler can register a handler for type "all", which will be invoked on all events.  Such an "all"
-        # handler will be invoked first.
-        for func in [ handler.all, handler[type] ]
-          if func
-            passThrough = func.call @, event, extra
-            if not passThrough
-              DomUtils.suppressEvent(event) if @isChromeEvent event
-              return false
-            return true if passThrough == @stopBubblingAndTrue
-            return false if passThrough == @stopBubblingAndFalse
+        result = handler[type].call @, event
+        if not result
+          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
     true
 
   remove: (id = @currentId) ->
     for i in [(@stack.length - 1)..0] by -1
       handler = @stack[i]
       if handler.id == id
+        # Mark the handler as removed.
         handler.id = null
         @stack.splice(i, 1)
         break
@@ -63,7 +64,9 @@ class HandlerStack
   isChromeEvent: (event) ->
     event?.preventDefault? or event?.stopImmediatePropagation?
 
-  # Convenience wrappers.
+  # 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