Modes; pre-merge clean up.

8 files changed, 72 insertions, 67 deletions

diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index 83a6b3f8..c1c8dfc8 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -347,7 +347,7 @@ chrome.browserAction.setBadgeBackgroundColor
   color: [82, 156, 206, 255]
 
 setBadge = do ->
-  current = ""
+  current = null
   timer = null
   updateBadge = (badge) -> -> chrome.browserAction.setBadgeText text: badge
   (request) ->
@@ -355,13 +355,8 @@ setBadge = do ->
     if badge? and badge != current
       current = badge
       clearTimeout timer if timer
-      if badge == ""
-        # We set an empty badge immediately. This is the common case when changing tabs.
-        updateBadge(badge)()
-      else
-        # We wait a few milliseconds before setting any other badge.  This avoids badge flicker when there are
-        # rapid changes (e.g. InsertMode is activated by find, followed almost immediately by PostFindMode).
-        timer = setTimeout updateBadge(badge), 50
+      # We wait a few moments. This avoids badge flicker when there are rapid changes.
+      timer = setTimeout updateBadge(badge), 50
 
 # Updates the browserAction icon to indicate whether Vimium is enabled or disabled on the current page.
 # Also propagates new enabled/disabled/passkeys state to active window, if necessary.
diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee
index 3c8240c0..5e41cbbb 100644
--- a/content_scripts/link_hints.coffee
+++ b/content_scripts/link_hints.coffee
@@ -10,7 +10,6 @@
 #
 # The "name" property below is a short-form name to appear in the link-hints mode name.  Debugging only.  The
 # key appears in the mode's badge.
-# NOTE(smblott) The use of keys in badges is experimental. It may prove too noisy.
 #
 OPEN_IN_CURRENT_TAB = { name: "curr-tab", key: "" }
 OPEN_IN_NEW_BG_TAB = { name: "bg-tab", key: "B" }
diff --git a/content_scripts/mode.coffee b/content_scripts/mode.coffee
index 2ff71ca8..a74acfed 100644
--- a/content_scripts/mode.coffee
+++ b/content_scripts/mode.coffee
@@ -24,8 +24,10 @@
 # responds to "focus" events, then push an additional handler:
 #   @push
 #     "focus": (event) => ....
-# Any such handlers are removed when the mode is deactivated.
+# Such handlers are removed when the mode is deactivated.
 #
+# The following events can be handled:
+#   keydown, keypress, keyup, click, focus and blur
 
 # Debug only.
 count = 0
@@ -85,8 +87,8 @@ class Mode
         "click": (event) => @alwaysContinueBubbling => @exit event
 
     # 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 the which is
-    # intended to be unique.  New instances deactivate existing instances with the same key.
+    # if @options.singleton is truthy.  The value of @options.singleton should be the key which is intended to
+    # be unique.  New instances deactivate existing instances with the same key.
     if @options.singleton
       do =>
         singletons = Mode.singletons ||= {}
@@ -99,7 +101,7 @@ class Mode
 
     # If @options.trackState is truthy, then the mode mainatins the current state in @enabled and @passKeys,
     # and calls @registerStateChange() (if defined) whenever the state changes. The mode also tracks the
-    # keyQueue in @keyQueue.
+    # current keyQueue in @keyQueue.
     if @options.trackState
       @enabled = false
       @passKeys = ""
@@ -115,7 +117,7 @@ class Mode
 
     Mode.modes.push @
     Mode.updateBadge()
-    @logStack()
+    @logModes()
     # End of Mode constructor.
 
   push: (handlers) ->
@@ -124,7 +126,6 @@ class Mode
 
   unshift: (handlers) ->
     handlers._name ||= "mode-#{@id}"
-    handlers._name += "/unshifted"
     @handlers.push handlerStack.unshift handlers
 
   onExit: (handler) ->
@@ -160,7 +161,7 @@ class Mode
         badge: badge.badge
 
   # Debugging routines.
-  logStack: ->
+  logModes: ->
     if @debug
       @log "active modes (top to bottom):"
       @log " ", mode.id for mode in Mode.modes[..].reverse()
diff --git a/content_scripts/mode_find.coffee b/content_scripts/mode_find.coffee
index 35352277..dff63949 100644
--- a/content_scripts/mode_find.coffee
+++ b/content_scripts/mode_find.coffee
@@ -33,6 +33,8 @@ class PostFindMode extends SuppressPrintable
 
     super
       name: "post-find"
+      # We show a "?" badge, but only while an Escape activates insert mode.
+      badge: "?"
       singleton: PostFindMode
       exitOnBlur: element
       exitOnClick: true
@@ -42,20 +44,23 @@ class PostFindMode extends SuppressPrintable
 
     # If the very-next keydown is Escape, then exit immediately, thereby passing subsequent keys to the
     # underlying insert-mode instance.
-    self = @
     @push
       _name: "mode-#{@id}/handle-escape"
-      keydown: (event) ->
+      keydown: (event) =>
         if KeyboardUtils.isEscape event
           DomUtils.suppressKeyupAfterEscape handlerStack
-          self.exit()
-          false # Suppress event.
+          @exit()
+          @suppressEvent
         else
-          @remove()
-          true # Continue bubbling.
+          handlerStack.remove()
+          @badge = ""
+          Mode.updateBadge()
+          @continueBubbling
 
-  # If PostFindMode is active, then we suppress the "I" badge from insert mode.
-  updateBadge: (badge) -> InsertMode.suppressEvent badge # Always truthy.
+  updateBadge: (badge) ->
+    badge.badge ||= @badge
+    # Suppress the "I" badge from insert mode.
+    InsertMode.suppressEvent badge # Always truthy.
 
 root = exports ? window
 root.PostFindMode = PostFindMode
diff --git a/content_scripts/mode_insert.coffee b/content_scripts/mode_insert.coffee
index 123c72be..196f910b 100644
--- a/content_scripts/mode_insert.coffee
+++ b/content_scripts/mode_insert.coffee
@@ -11,11 +11,18 @@ class InsertMode extends Mode
     # If truthy, then we were activated by the user (with "i").
     @global = options.global
 
+    handleKeyEvent = (event) =>
+      return @continueBubbling unless @isActive event
+      return @stopBubblingAndTrue unless event.type == 'keydown' and KeyboardUtils.isEscape event
+      DomUtils.suppressKeyupAfterEscape handlerStack
+      @exit event, event.srcElement
+      @suppressEvent
+
     defaults =
       name: "insert"
-      keydown: (event) => @handleKeydownEvent event
-      keypress: (event) => @handleKeyEvent event
-      keyup: (event) => @handleKeyEvent event
+      keypress: handleKeyEvent
+      keyup: handleKeyEvent
+      keydown: handleKeyEvent
 
     super extend defaults, options
 
@@ -32,11 +39,10 @@ class InsertMode extends Mode
         # We can't rely on focus and blur events arriving in the expected order.  When the active element
         # changes, we might get "focus" before "blur".  We track the active element in @insertModeLock, and
         # exit only when that element blurs.
-        @exit event, target if target == @insertModeLock
+        @exit event, target if @insertModeLock and target == @insertModeLock
       "focus": (event) => @alwaysContinueBubbling =>
         if @insertModeLock != event.target and DomUtils.isFocusable event.target
-          @insertModeLock = event.target
-          Mode.updateBadge()
+          @activateOnElement event.target
 
   isActive: (event) ->
     return false if event == InsertMode.suppressedEvent
@@ -44,24 +50,18 @@ class InsertMode extends Mode
     # Some sites (e.g. inbox.google.com) change the contentEditable property on the fly (see #1245); and
     # unfortunately, the focus event fires *before* the change.  Therefore, we need to re-check whether the
     # active element is contentEditable.
-    if @insertModeLock != document.activeElement and document.activeElement?.isContentEditable
-      @insertModeLock = document.activeElement
-      Mode.updateBadge()
+    @activateOnElement document.activeElement if document.activeElement?.isContentEditable
     @insertModeLock != null
 
-  handleKeydownEvent: (event) ->
-    return @continueBubbling unless @isActive event
-    return @stopBubblingAndTrue unless KeyboardUtils.isEscape event
-    DomUtils.suppressKeyupAfterEscape handlerStack
-    @exit event, event.srcElement
-    @suppressEvent
-
-  # Handles keypress and keyup events.
-  handleKeyEvent: (event) ->
-    if @isActive event then @stopBubblingAndTrue else @continueBubbling
+  activateOnElement: (element) ->
+    @log "#{@id}: activating (permanent)" if @debug and @permanent
+    @insertModeLock = element
+    Mode.updateBadge()
 
   exit: (_, target)  ->
+    # Note: target == undefined, here, is required only for tests.
     if (target and target == @insertModeLock) or @global or target == undefined
+      @log "#{@id}: deactivating (permanent)" if @debug and @permanent and @insertModeLock
       @insertModeLock = null
       if target and DomUtils.isFocusable target
         # Remove the focus, so the user can't just get back into insert mode by typing in the same input box.
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 0a034e28..6a26a133 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -117,10 +117,11 @@ initializePreDomReady = ->
         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 are active.
+      Scroller.init settings
+
+  # Install the permanent modes.  The permanently-installed insert mode tracks focus/blur events, and
+  # activates/deactivates itself accordingly.
   new NormalMode
-  Scroller.init settings
   new PassKeysMode
   new InsertMode
 
@@ -242,8 +243,6 @@ enterInsertModeIfElementIsFocused = ->
     enterInsertModeWithoutShowingIndicator(document.activeElement)
 
 onDOMActivate = (event) -> handlerStack.bubbleEvent 'DOMActivate', event
-onFocus = (event) -> handlerStack.bubbleEvent 'focus', event
-onBlur = (event) -> handlerStack.bubbleEvent 'blur', event
 
 executePageCommand = (request) ->
   return unless frameId == request.frameId
@@ -326,8 +325,7 @@ extend window,
     HUD.showForDuration("Yanked URL", 1000)
 
   enterInsertMode: ->
-    new InsertMode
-      global: true
+    new InsertMode global: true
 
   enterVisualMode: =>
     new VisualMode()
@@ -804,17 +802,16 @@ executeFind = (query, options) ->
     -> document.addEventListener("selectionchange", restoreDefaultSelectionHighlight, true)
     0)
 
+  # We are either in normal mode ("n"), or find mode ("/").  We are not in insert mode.  Nevertheless, if a
+  # previous find landed in an editable element, then that element may still be activated.  In this case, we
+  # don't want to leave it behind (see #1412).
+  if document.activeElement and DomUtils.isEditable document.activeElement
+    if not DomUtils.isSelected document.activeElement
+      document.activeElement.blur()
+
   # we need to save the anchor node here because <esc> seems to nullify it, regardless of whether we do
   # preventDefault()
   findModeAnchorNode = document.getSelection().anchorNode
-
-  # TODO(smblott). Disabled. This is the wrong test.  Should be reinstated when we have the right test, which
-  # looks like it should be "isSelected" from #1431.
-  # # If the anchor node not a descendent of the active element, then blur the active element.  We don't want to
-  # # leave behind an inappropriate active element. This fixes #1412.
-  # if document.activeElement and not DomUtils.isDOMDescendant document.activeElement, findModeAnchorNode
-  #   document.activeElement.blur()
-
   result
 
 restoreDefaultSelectionHighlight = -> document.body.classList.remove("vimiumFindMode")
diff --git a/lib/dom_utils.coffee b/lib/dom_utils.coffee
index 322188b3..e1f1f442 100644
--- a/lib/dom_utils.coffee
+++ b/lib/dom_utils.coffee
@@ -166,6 +166,14 @@ DomUtils =
       node = node.parentNode
     false
 
+  # True if element contains the active selection range.
+  isSelected: (element) ->
+    if element.isContentEditable
+      node = document.getSelection()?.anchorNode
+      node and @isDOMDescendant element, node
+    else
+      element.selectionStart? and element.selectionEnd? and element.selectionStart != element.selectionEnd
+
   simulateSelect: (element) ->
     element.focus()
     # When focusing a textbox, put the selection caret at the end of the textbox's contents.
diff --git a/lib/handler_stack.coffee b/lib/handler_stack.coffee
index fddc45db..76d835b7 100644
--- a/lib/handler_stack.coffee
+++ b/lib/handler_stack.coffee
@@ -13,25 +13,24 @@ class HandlerStack
     @stopBubblingAndTrue = new Object()
 
     # A handler should return this value to indicate that the event has been consumed, and no further
-    # processing should take place.
+    # processing should take place.  The event does not propagate to the underlying page.
     @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.
+    # used when, while bubbling an event, a new mode is pushed onto the stack.
     @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) ->
-    handler.id = ++@counter
     handler._name ||= "anon-#{@counter}"
     @stack.push handler
-    handler.id
+    handler.id = ++@counter
 
-  # Adds a handler to the bottom of the stack. Returns a unique ID for that handler that can be used to remove
-  # it later.
+  # As above, except the new handler is added to the bottom of the stack.
   unshift: (handler) ->
+    handler._name ||= "anon-#{@counter}"
+    handler._name += "/unshift"
     @stack.unshift handler
     handler.id = ++@counter
 
@@ -84,8 +83,9 @@ class HandlerStack
   # Debugging.
   logResult: (type, event, handler, result) ->
     # FIXME(smblott).  Badge updating is too noisy, so we filter it out.  However, we do need to look at how
-    # many badge update events are happening.  It seems to be more than necessary.
-    return if type == "updateBadge"
+    # many badge update events are happening.  It seems to be more than necessary. We also filter out
+    # registerKeyQueue as unnecessarily noisy and not particularly helpful.
+    return if type in [ "updateBadge", "registerKeyQueue" ]
     label =
       switch result
         when @stopBubblingAndTrue then "stop/true"