Modes; tweaks.

4 files changed, 45 insertions, 34 deletions

diff --git a/content_scripts/mode_find.coffee b/content_scripts/mode_find.coffee
index cfcd18b5..21638a34 100644
--- a/content_scripts/mode_find.coffee
+++ b/content_scripts/mode_find.coffee
@@ -1,20 +1,24 @@
 # NOTE(smblott).  Ultimately, all of the FindMode-related code should be moved to this file.
 
-# This is Used as a sub-class to PostFindMode.  It prevents printable characters from being passed through to
-# underlying input element; see #1415.  Note, also, that the "Range" condition in the keyup handler
-# implmements option 5c from #1415.
+# This prevents printable characters from being passed through to underlying page; see #1415.
 class SuppressPrintable extends Mode
   constructor: (options) ->
     super options
     handler = (event) => if KeyboardUtils.isPrintable event then @suppressEvent else @continueBubbling
 
-    # Note: we use unshift here. We see events *after* normal mode, so we only see unmapped keys.
+    # We use unshift here, so we see events after normal mode, so we only see unmapped keys.
     @unshift
       _name: "mode-#{@id}/suppressPrintableEvents"
       keydown: handler
       keypress: handler
       keyup: (event) =>
-        if document.getSelection().type.toLowerCase() != "range" then @exit() else handler event
+        # If the selection is no longer a 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 != "Range"
+          console.log "aaa", @options.targetElement
+          @exit()
+        else
+          handler event
 
 # 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:
@@ -25,9 +29,9 @@ class SuppressPrintable extends Mode
 #   3. If the very-next keystroke is Escape, then drop immediately into insert mode.
 #
 class PostFindMode extends SuppressPrintable
-  constructor: (findModeAnchorNode) ->
+  constructor: ->
+    return unless document.activeElement and DomUtils.isEditable document.activeElement
     element = document.activeElement
-    return unless element and DomUtils.isEditable element
 
     super
       name: "post-find"
diff --git a/content_scripts/mode_insert.coffee b/content_scripts/mode_insert.coffee
index 9be520c7..204c629d 100644
--- a/content_scripts/mode_insert.coffee
+++ b/content_scripts/mode_insert.coffee
@@ -1,7 +1,6 @@
 
 class InsertMode extends Mode
-  # There is one permanently-installed instance of InsertMode.  This allows PostFindMode to query the active
-  # element.
+  # There is one permanently-installed instance of InsertMode.
   @permanentInstance: null
 
   constructor: (options = {}) ->
@@ -15,25 +14,34 @@ class InsertMode extends Mode
       keyup: (event) => @handleKeyEvent event
 
     super extend defaults, options
-    @insertModeLock = if options.targetElement? then options.targetElement else null
+
+    @insertModeLock =
+      if document.activeElement and DomUtils.isEditable document.activeElement
+        # We have already focused an input element, so use it.
+        document.activeElement
+      else
+        null
 
     @push
       "blur": (event) => @alwaysContinueBubbling =>
-        if DomUtils.isFocusable event.target
-          @exit event, event.target
-          Mode.updateBadge()
+        target = event.target
+        # We can't rely on focus and blur events arriving in the expected order.  When the active element
+        # changes, we might get "blur" before "focus".  The approach we take is to track the active element in
+        # @insertModeLock, and exit only when the that element blurs.
+        @exit event, target if target == @insertModeLock and DomUtils.isFocusable target
       "focus": (event) => @alwaysContinueBubbling =>
-        @insertModeLock = event.target if DomUtils.isFocusable event.target
-
-    # We may already have focused an input element, so check.
-    @insertModeLock = document.activeElement if document.activeElement and DomUtils.isEditable document.activeElement
+        if @insertModeLock != event.target and DomUtils.isFocusable event.target
+          @insertModeLock = event.target
+          Mode.updateBadge()
 
   isActive: ->
-    return true if @insertModeLock != null or @global
+    return true if @insertModeLock or @global
     # 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.
-    @insertModeLock = document.activeElement if document.activeElement?.isContentEditable
+    if document.activeElement?.isContentEditable and @insertModeLock != document.activeElement
+      @insertModeLock = document.activeElement
+      Mode.updateBadge()
     @insertModeLock != null
 
   handleKeydownEvent: (event) ->
@@ -41,7 +49,6 @@ class InsertMode extends Mode
     return @stopBubblingAndTrue unless KeyboardUtils.isEscape event
     DomUtils.suppressKeyupAfterEscape handlerStack
     @exit event, event.srcElement
-    Mode.updateBadge()
     @suppressEvent
 
   # Handles keypress and keyup events.
@@ -49,17 +56,17 @@ class InsertMode extends Mode
     if @isActive() and event != InsertMode.suppressedEvent then @stopBubblingAndTrue else @continueBubbling
 
   exit: (_, target)  ->
-    if target and (target == @insertModeLock or @global) and DomUtils.isFocusable target
-      # Remove focus so the user can't just get himself back into insert mode by typing in the same input
-      # box.
-      # NOTE(smblott, 2014/12/22) Including embeds for .blur() etc. here is experimental.  It appears to be
-      # the right thing to do for most common use cases.  However, it could also cripple flash-based sites and
-      # games.  See discussion in #1211 and #1194.
-      target.blur()
-    if target == undefined or target == @insertModeLock or @global
+    if (target and target == @insertModeLock) or @global or target == undefined
       @insertModeLock = null
-      # Now really exit, unless this is the permanently-installed instance.
-      super() unless @ == InsertMode.permanentInstance
+      if target and DomUtils.isFocusable target
+        # Remove the focus, so the user can't just get himself back into insert mode by typing in the same input
+        # box.
+        # NOTE(smblott, 2014/12/22) Including embeds for .blur() etc. here is experimental.  It appears to be
+        # the right thing to do for most common use cases.  However, it could also cripple flash-based sites and
+        # games.  See discussion in #1211 and #1194.
+        target.blur()
+      # Really exit, but only if this isn't the permanently-installed instance.
+      if @ == InsertMode.permanentInstance then Mode.updateBadge() else super()
 
   chooseBadge: (badge) ->
     return if badge == InsertMode.suppressedEvent
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 3049784e..b2c591fd 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -770,7 +770,7 @@ class FindMode extends Mode
     handleEscapeForFindMode() if event?.type == "keydown" and KeyboardUtils.isEscape event
     handleEscapeForFindMode() if event?.type == "click"
     if findModeQueryHasResults and event?.type != "click"
-      new PostFindMode findModeAnchorNode
+      new PostFindMode
 
 performFindInPlace = ->
   cachedScrollX = window.scrollX
@@ -864,7 +864,7 @@ findAndFocus = (backwards) ->
 
   if findModeQueryHasResults
     focusFoundLink()
-    new PostFindMode findModeAnchorNode if findModeQueryHasResults
+    new PostFindMode() if findModeQueryHasResults
   else
     HUD.showForDuration("No matches for '" + findModeQuery.rawQuery + "'", 1000)
 
diff --git a/tests/dom_tests/dom_tests.coffee b/tests/dom_tests/dom_tests.coffee
index 9e65d954..dae69dd8 100644
--- a/tests/dom_tests/dom_tests.coffee
+++ b/tests/dom_tests/dom_tests.coffee
@@ -202,7 +202,7 @@ context "Input focus",
   # This is the same as above, but also verifies that focusInput activates insert mode.
   should "activate insert mode", ->
     focusInput 1
-    handlerStack.bubbleEvent 'focus', { target: document. activeElement }
+    handlerStack.bubbleEvent 'focus', { target: document.activeElement }
     assert.isTrue InsertMode.permanentInstance.isActive()
 
     focusInput 100
@@ -384,7 +384,7 @@ context "Insert mode",
 
     # Install insert mode.
     insertMode = new InsertMode
-      targetElement: document.body
+      global: true
 
     # Then verify insert mode.
     for event in [ "keydown", "keypress", "keyup" ]