Modes; yet more teaks and fiddles.

6 files changed, 108 insertions, 102 deletions

diff --git a/content_scripts/mode.coffee b/content_scripts/mode.coffee
index d14778a8..9f820469 100644
--- a/content_scripts/mode.coffee
+++ b/content_scripts/mode.coffee
@@ -1,14 +1,14 @@
 #
-# A mode implements a number of keyboard event handlers which are pushed onto the handler stack when the mode
-# is activated, and popped off when it is deactivated.  The Mode class constructor takes a single argument,
-# options, which can define (amongst other things):
+# A mode implements a number of keyboard (and possibly other) event handlers which are pushed onto the handler
+# stack when the mode is activated, and popped off when it is deactivated.  The Mode class constructor takes a
+# single argument "options" which can define (amongst other things):
 #
 # name:
 #   A name for this mode.
 #
 # badge:
 #   A badge (to appear on the browser popup).
-#   Optional.  Define a badge if the badge is constant; for example, in insert mode the badge is always "I".
+#   Optional.  Define a badge if the badge is constant; for example, in find mode the badge is always "/".
 #   Otherwise, do not define a badge, but instead override the chooseBadge method; for example, in passkeys
 #   mode, the badge may be "P" or "", depending on the configuration state.  Or, if the mode *never* shows a
 #   badge, then do neither.
@@ -26,35 +26,23 @@
 #     "focus": (event) => ....
 # Any such handlers are removed when the mode is deactivated.
 #
-# To activate a mode, use:
-#   myMode = new MyMode()
-#
-# Or (usually better) just:
-#   new MyMode()
-# It is usually not necessary to retain a reference to the mode object.
-#
-# To deactivate a mode, use:
-#   @exit()       # internally triggered (more common).
-#   myMode.exit() # externally triggered.
-#
 
-# For debug only.
+# Debug only.
 count = 0
 
 class Mode
-  # If Mode.debug is true, then we generate a trace of modes being activated and deactivated on the console, along
-  # with a list of the currently active modes.
+  # If Mode.debug is true, then we generate a trace of modes being activated and deactivated on the console.
   debug: true
   @modes: []
 
-  # Constants; short, readable names for handlerStack event-handler return values.
+  # Constants; short, readable names for the return values expected by handlerStack.bubbleEvent.
   continueBubbling: true
   suppressEvent: false
   stopBubblingAndTrue: handlerStack.stopBubblingAndTrue
   stopBubblingAndFalse: handlerStack.stopBubblingAndFalse
   restartBubbling: handlerStack.restartBubbling
 
-  constructor: (@options={}) ->
+  constructor: (@options = {}) ->
     @handlers = []
     @exitHandlers = []
     @modeIsActive = true
@@ -71,14 +59,12 @@ class Mode
       keyup: @options.keyup || null
       updateBadge: (badge) => @alwaysContinueBubbling => @chooseBadge badge
 
-    # Some modes are singletons: there may be at most one instance active at any one time.  A mode is a
-    # singleton if @options.singleton is truthy.  The value of @options.singleton should be the key which is
-    # required to be unique.  See PostFindMode for an example.
-    # New instances deactivate existing instances as they themselves are activated.
+    # Some modes are singletons: there may be at most one instance active at any time.  A mode is a singleton
+    # if @options.singleton is truthy.  The value of @options.singleton should be the key which is required to
+    # be unique.  New instances deactivate existing instances.
     @registerSingleton @options.singleton if @options.singleton
 
-    # If @options.exitOnEscape is truthy, then the mode will exit when the escape key is pressed.  The
-    # triggering keyboard event will be passed to the mode's @exit() method.
+    # If @options.exitOnEscape is truthy, then the mode will exit when the escape key is pressed.
     if @options.exitOnEscape
       # Note. This handler ends up above the mode's own key handlers on the handler stack, so it takes
       # priority.
@@ -110,12 +96,11 @@ class Mode
       @passKeys = ""
       @push
         _name: "mode-#{@id}/registerStateChange"
-        "registerStateChange": ({ enabled: enabled, passKeys: passKeys }) =>
-          @alwaysContinueBubbling =>
-            if enabled != @enabled or passKeys != @passKeys
-              @enabled = enabled
-              @passKeys = passKeys
-              @registerStateChange?()
+        "registerStateChange": ({ enabled: enabled, passKeys: passKeys }) => @alwaysContinueBubbling =>
+          if enabled != @enabled or passKeys != @passKeys
+            @enabled = enabled
+            @passKeys = passKeys
+            @registerStateChange?()
 
     Mode.modes.push @
     Mode.updateBadge()
@@ -150,11 +135,10 @@ class Mode
 
   # 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 their return value (which helps keep code concise and
-  # clear).
+  # case), because they do not need to be concerned with the value they yield.
   alwaysContinueBubbling: handlerStack.alwaysContinueBubbling
 
-  # User for sometimes suppressing badge updates.
+  # Used for sometimes suppressing badge updates.
   @badgeSuppressor: new Utils.Suppressor()
 
   # Static method.  Used externally and internally to initiate bubbling of an updateBadge event and to send
@@ -171,11 +155,11 @@ class Mode
   registerSingleton: do ->
     singletons = {} # Static.
     (key) ->
-      # We're currently installing a new mode. So we'll be updating the badge shortly.  Therefore, we can
-      # suppress badge updates while exiting any existing active singleton.  This prevents the badge from
-      # flickering in some cases.
       if singletons[key]
         @log "singleton:", "deactivating #{singletons[key].id}" if @debug
+        # We're currently installing a new mode. So we'll be updating the badge shortly.  Therefore, we can
+        # suppress badge updates while deactivating the existing singleton.  This prevents the badge from
+        # flickering in some cases.
         Mode.badgeSuppressor.runSuppresed -> singletons[key].exit()
       singletons[key] = @
 
@@ -184,28 +168,26 @@ class Mode
   # Debugging routines.
   logStack: ->
     @log "active modes (top to bottom):"
-    for mode in Mode.modes[..].reverse()
-      @log " ",  mode.id
+    @log " ", mode.id for mode in Mode.modes[..].reverse()
 
   log: (args...) ->
     console.log args...
 
-  # Return the name of the must-recently activated mode.
+  # Return the must-recently activated mode (only used in tests).
   @top: ->
     @modes[@modes.length-1]
 
-# UIMode is a mode for Vimium UI components.  They share a common singleton, so new UI components displace
-# previously-active UI components.  For example, the FocusSelector mode displaces PostFindMode.
-class UIMode extends Mode
+# InputController is a super-class for modes which control insert mode: PostFindMode and FocusSelector.  It's
+# a singleton, so no two instances may be active at the same time.
+class InputController extends Mode
   constructor: (options) ->
     defaults =
-      singleton: UIMode
+      singleton: InputController
     super extend defaults, options
 
 # BadgeMode is a pseudo mode for triggering badge updates on focus changes and state updates. It sits at the
 # bottom of the handler stack, and so it receives state changes *after* all other modes, and can override the
-# badge choice of the other active modes.
-# Note.  We create the the one-and-only instance here.
+# badge choice of the other modes.  We create the the one-and-only instance here.
 new class BadgeMode extends Mode
   constructor: () ->
     super
@@ -213,14 +195,14 @@ new class BadgeMode extends Mode
       trackState: true
 
     # FIXME(smblott) BadgeMode is currently triggering an updateBadge event on every focus event.  That's a
-    # lot, considerably more than is necessary.  Really, it only needs to trigger when we change frame, or
-    # when we change tab.
+    # lot, considerably more than necessary.  Really, it only needs to trigger when we change frame, or when
+    # we change tab.
     @push
       _name: "mode-#{@id}/focus"
       "focus": => @alwaysContinueBubbling -> Mode.updateBadge()
 
   chooseBadge: (badge) ->
-    # If we're not enabled, then post an empty badge.  BadgeMode is last, so this takes priority.
+    # If we're not enabled, then post an empty badge.
     badge.badge = "" unless @enabled
 
   # When the registerStateChange event bubbles to the bottom of the stack, all modes have been notified.  So
@@ -230,4 +212,4 @@ new class BadgeMode extends Mode
 
 root = exports ? window
 root.Mode = Mode
-root.UIMode = UIMode
+root.InputController = InputController
diff --git a/content_scripts/mode_find.coffee b/content_scripts/mode_find.coffee
index e79bc0dd..f151b8cd 100644
--- a/content_scripts/mode_find.coffee
+++ b/content_scripts/mode_find.coffee
@@ -1,32 +1,33 @@
 # NOTE(smblott).  Ultimately, all of the FindMode-related code should be moved to this file.
 
-# When we use find mode, the selection/focus can end up in a focusable/editable element.  In this situation,
+# When we use find mode, the selection/focus can land in a focusable/editable element.  In this situation,
 # special considerations apply.  We implement three special cases:
-#   1. Prevent keyboard events from dropping us unintentionally into insert mode.
-#   2. Prevent all printable keypress events on the active element from propagating beyond normal mode.  See
-#   #1415.
+#   1. Disable keyboard events in insert mode, because the user hasn't asked to enter insert mode.
+#   2. Prevent printable keyboard events from propagating to the page; see #1415.
 #   3. If the very-next keystroke is Escape, then drop immediately into insert mode.
 #
-class PostFindMode extends UIMode
+class PostFindMode extends InputController
   constructor: (findModeAnchorNode) ->
-    # Locate the element we need to protect and focus it, if necessary.  Usually, we can just rely on insert
-    # mode to have picked it up (when it received the focus).
-    element = InsertMode.permanentInstance.insertModeLock
-    unless element?
-      # For contentEditable elements, chrome does not leave them focused, so insert mode does not pick them
-      # up. We start at findModeAnchorNode and walk up the DOM, stopping at the last node encountered which is
-      # contentEditable.
-      element = findModeAnchorNode
-      element = element.parentElement while element?.parentElement?.isContentEditable
-      return unless element?.isContentEditable
-      # The element might be disabled (and therefore unable to receive focus), we use the approximate
-      # heuristic of checking that element is an ancestor of the active element.
-      return unless document.activeElement and DomUtils.isDOMDescendant document.activeElement, element
-      element.focus()
+    # Locate the element we need to protect.  In most cases, it's just the active element.
+    element =
+      if document.activeElement and DomUtils.isEditable document.activeElement
+        document.activeElement
+      else
+        # For contentEditable elements, chrome does not focus them, although they are activated by keystrokes.
+        # We need to find the element ourselves.
+        element = findModeAnchorNode
+        element = element.parentElement while element.parentElement?.isContentEditable
+        if element.isContentEditable
+          if DomUtils.isDOMDescendant element, findModeAnchorNode
+            # TODO(smblott).  We shouldn't really need to focus the element, here.  Need to look into why this
+            # is necessary.
+            element.focus()
+            element
+
+    return unless element
 
     super
       name: "post-find"
-      badge: "N" # Pretend to be normal mode (because we don't want the insert-mode badge).
       exitOnBlur: element
       exitOnClick: true
       keydown: (event) -> InsertMode.suppressEvent event # Truthy.
@@ -35,7 +36,7 @@ class PostFindMode extends UIMode
         @alwaysContinueBubbling =>
           if document.getSelection().type != "Range"
             # If the selection is no longer a range, then the user is interacting with the element, so get out
-            # of the way and stop suppressing insert mode.  See discussion of Option 5c from #1415.
+            # of the way.  See Option 5c from #1415.
             @exit()
           else
             InsertMode.suppressEvent event
@@ -45,7 +46,7 @@ class PostFindMode extends UIMode
     @push
       _name: "mode-#{@id}/handle-escape"
       keydown: (event) ->
-        if document.activeElement == element and KeyboardUtils.isEscape event
+        if KeyboardUtils.isEscape event
           DomUtils.suppressKeyupAfterEscape handlerStack
           self.exit()
           false # Suppress event.
@@ -53,7 +54,7 @@ class PostFindMode extends UIMode
           @remove()
           true # Continue bubbling.
 
-    # Prevent printable keyboard events from propagating to to the page; see #1415.
+    # Prevent printable keyboard events from propagating to the page; see #1415.
     do =>
       handler = (event) =>
         if event.srcElement == element and KeyboardUtils.isPrintable event
@@ -69,10 +70,9 @@ class PostFindMode extends UIMode
         keypress: handler
         keyup: handler
 
-# NOTE.  There's a problem with this approach when a find/search lands in a contentEditable element.  Chrome
-# generates a focus event triggering insert mode (good), then immediately generates a "blur" event, disabling
-# insert mode again.  Nevertheless, unmapped keys *do* result in the element being focused again.
-# So, asking insert mode whether it's active is giving us the wrong answer.
+  chooseBadge: (badge) ->
+    # If PostFindMode is active, then we don't want the "I" badge from insert mode.
+    InsertMode.suppressEvent badge
 
 root = exports ? window
 root.PostFindMode = PostFindMode
diff --git a/content_scripts/mode_insert.coffee b/content_scripts/mode_insert.coffee
index f815090a..9be520c7 100644
--- a/content_scripts/mode_insert.coffee
+++ b/content_scripts/mode_insert.coffee
@@ -62,6 +62,7 @@ class InsertMode extends Mode
       super() unless @ == InsertMode.permanentInstance
 
   chooseBadge: (badge) ->
+    return if badge == InsertMode.suppressedEvent
     badge.badge ||= "I" if @isActive()
 
   # Static stuff to allow PostFindMode to suppress insert mode.
diff --git a/content_scripts/mode_passkeys.coffee b/content_scripts/mode_passkeys.coffee
index dde91c13..a6cd7d2d 100644
--- a/content_scripts/mode_passkeys.coffee
+++ b/content_scripts/mode_passkeys.coffee
@@ -8,6 +8,10 @@ class PassKeysMode extends Mode
       keypress: (event) => @handleKeyChar String.fromCharCode event.charCode
       keyup: (event) => @handleKeyChar String.fromCharCode event.charCode
 
+    @keyQueue = ""
+    @push
+      registerKeyQueue: ({ keyQueue: keyQueue }) => @alwaysContinueBubbling => @keyQueue = keyQueue
+
   # 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.
@@ -17,9 +21,6 @@ class PassKeysMode extends Mode
     else
       @continueBubbling
 
-  configure: (request) ->
-    @keyQueue = request.keyQueue if request.keyQueue?
-
   chooseBadge: (badge) ->
     badge.badge ||= "P" if @passKeys and not @keyQueue
 
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index e536ebbc..7d24e714 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -5,7 +5,6 @@
 # "domReady".
 #
 
-passKeysMode = null
 targetElement = null
 findMode = false
 findModeQuery = { rawQuery: "", matchCount: 0 }
@@ -103,19 +102,6 @@ frameId = Math.floor(Math.random()*999999999)
 
 hasModifiersRegex = /^<([amc]-)+.>/
 
-class NormalMode extends Mode
-  constructor: ->
-    super
-      name: "normal"
-      badge: "N"
-      keydown: (event) => onKeydown.call @, event
-      keypress: (event) => onKeypress.call @, event
-      keyup: (event) => onKeyup.call @, event
-
-  chooseBadge: (badge) ->
-    super badge
-    badge.badge = "" unless isEnabledForUrl
-
 #
 # Complete initialization work that sould be done prior to DOMReady.
 #
@@ -123,11 +109,20 @@ initializePreDomReady = ->
   settings.addEventListener("load", LinkHints.init.bind(LinkHints))
   settings.load()
 
-  # Install permanent modes and handlers.
-  new NormalMode()
+  class NormalMode extends Mode
+    constructor: ->
+      super
+        name: "normal"
+        keydown: (event) => onKeydown.call @, event
+        keypress: (event) => onKeypress.call @, event
+        keyup: (event) => onKeyup.call @, event
+
+  # Install the permanent modes and handlers.  The permanent insert mode operates only when focusable/editable
+  # elements have the focus.
+  new NormalMode
   Scroller.init settings
-  passKeysMode = new PassKeysMode()
-  new InsertMode()
+  new PassKeysMode
+  new InsertMode
 
   checkIfEnabledForUrl()
 
@@ -157,7 +152,7 @@ initializePreDomReady = ->
     setState: setState
     currentKeyQueue: (request) ->
       keyQueue = request.keyQueue
-      passKeysMode.configure request
+      handlerStack.bubbleEvent "registerKeyQueue", { keyQueue: keyQueue }
 
   chrome.runtime.onMessage.addListener (request, sender, sendResponse) ->
     # In the options page, we will receive requests from both content and background scripts. ignore those
@@ -369,7 +364,7 @@ extend window,
       id: "vimiumInputMarkerContainer"
       className: "vimiumReset"
 
-    new class FocusSelector extends UIMode
+    new class FocusSelector extends InputController
       constructor: ->
         super
           name: "focus-selector"
diff --git a/tests/dom_tests/dom_tests.coffee b/tests/dom_tests/dom_tests.coffee
index 9918b12d..0b09df27 100644
--- a/tests/dom_tests/dom_tests.coffee
+++ b/tests/dom_tests/dom_tests.coffee
@@ -307,12 +307,18 @@ context "Passkeys mode",
       enabled: true
       passKeys: ""
 
+    handlerStack.bubbleEvent "registerKeyQueue",
+      keyQueue: ""
+
   tearDown ->
     restoreStackState()
     handlerStack.bubbleEvent "registerStateChange",
       enabled: true
       passKeys: ""
 
+    handlerStack.bubbleEvent "registerKeyQueue",
+      keyQueue: ""
+
   should "not suppress passKeys", ->
     # First check normal-mode key (just to verify the framework).
     for k in [ "m", "p" ]
@@ -332,12 +338,33 @@ context "Passkeys mode",
       handlerStack.bubbleEvent event, key
       assert.isFalse key.suppressed
 
-    # And re-verify mapped key.
+    # And re-verify a mapped key.
     for event in [ "keydown", "keypress", "keyup" ]
       key = mockKeyboardEvent "m"
       handlerStack.bubbleEvent event, key
       assert.isTrue key.suppressed
 
+  should "suppress passKeys with a non-empty keyQueue", ->
+    # Install passKey.
+    handlerStack.bubbleEvent "registerStateChange",
+      enabled: true
+      passKeys: "p"
+
+    # First check the key is indeed not suppressed.
+    for event in [ "keydown", "keypress", "keyup" ]
+      key = mockKeyboardEvent "p"
+      handlerStack.bubbleEvent event, key
+      assert.isFalse key.suppressed
+
+    handlerStack.bubbleEvent "registerKeyQueue",
+      keyQueue: "1"
+
+    # Now verify that the key is suppressed.
+    for event in [ "keydown", "keypress", "keyup" ]
+      key = mockKeyboardEvent "p"
+      handlerStack.bubbleEvent event, key
+      assert.isTrue key.suppressed
+
 context "Insert mode",
   setup ->
     document.activeElement?.blur()