Modes; yet more tweaks...

- Mainly comments.
- Rename chooseBadge to updateBadge (for consistency).
- No badge for passkeys; also fix tests.

5 files changed, 28 insertions, 27 deletions

diff --git a/content_scripts/mode.coffee b/content_scripts/mode.coffee
index 6bd09af2..2ff71ca8 100644
--- a/content_scripts/mode.coffee
+++ b/content_scripts/mode.coffee
@@ -85,8 +85,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 which is intended to
-    # be unique.  New instances deactivate existing instances.
+    # 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
       do =>
         singletons = Mode.singletons ||= {}
@@ -94,8 +94,6 @@ class Mode
         @onExit => delete singletons[key] if singletons[key] == @
         if singletons[key]
           @log "singleton:", "deactivating #{singletons[key].id}"
-          # 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 can prevent badge flicker.
           singletons[key].exit()
         singletons[key] = @
 
diff --git a/content_scripts/mode_find.coffee b/content_scripts/mode_find.coffee
index 6b4f6bb1..35352277 100644
--- a/content_scripts/mode_find.coffee
+++ b/content_scripts/mode_find.coffee
@@ -1,6 +1,7 @@
-# NOTE(smblott).  Ultimately, all of the FindMode-related code should be moved to this file.
+# NOTE(smblott).  Ultimately, all of the FindMode-related code should be moved here.
 
-# This prevents printable characters from being passed through to underlying page; see #1415.
+# This prevents unmapped printable characters from being passed through to underlying page; see #1415.  Only
+# used by PostFindMode, below.
 class SuppressPrintable extends Mode
   constructor: (options) ->
     super options
@@ -9,20 +10,20 @@ class SuppressPrintable extends Mode
 
     # We use unshift here, so we see events after normal mode, so we only see unmapped keys.
     @unshift
-      _name: "mode-#{@id}/suppressPrintableEvents"
+      _name: "mode-#{@id}/suppress-printable"
       keydown: handler
       keypress: handler
       keyup: (event) =>
-        # If the selection types has changed (usually, no longer "Range"), then the user is interacting with
+        # If the selection type has changed (usually, no longer "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 != type then @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:
+# When we use find, the selection/focus can land in a focusable/editable element.  In this situation, special
+# considerations apply.  We implement three special cases:
 #   1. Disable insert mode, because the user hasn't asked to enter insert mode.  We do this by using
 #      InsertMode.suppressEvent.
-#   2. Prevent printable keyboard events from propagating to the page; see #1415.  We do this by inheriting
-#      from SuppressPrintable.
+#   2. Prevent unmapped printable keyboard events from propagating to the page; see #1415.  We do this by
+#      inheriting from SuppressPrintable.
 #   3. If the very-next keystroke is Escape, then drop immediately into insert mode.
 #
 class PostFindMode extends SuppressPrintable
@@ -39,7 +40,8 @@ class PostFindMode extends SuppressPrintable
       keypress: (event) -> InsertMode.suppressEvent event
       keyup: (event) -> InsertMode.suppressEvent event
 
-    # If the very-next keydown is Esc, drop immediately into insert mode.
+    # 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"
@@ -53,7 +55,7 @@ class PostFindMode extends SuppressPrintable
           true # Continue bubbling.
 
   # If PostFindMode is active, then we suppress the "I" badge from insert mode.
-  updateBadge: (badge) -> InsertMode.suppressEvent badge
+  updateBadge: (badge) -> 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 ef7223ad..123c72be 100644
--- a/content_scripts/mode_insert.coffee
+++ b/content_scripts/mode_insert.coffee
@@ -1,14 +1,14 @@
 
 class InsertMode extends Mode
-  # There is one permanently-installed instance of InsertMode.  It watches for focus changes and
-  # activates/deactivates itself accordingly.
+  # There is one permanently-installed instance of InsertMode.  It tracks focus changes and
+  # activates/deactivates itself (by setting @insertModeLock) accordingly.
   @permanentInstance: null
 
   constructor: (options = {}) ->
     InsertMode.permanentInstance ||= @
     @permanent = (@ == InsertMode.permanentInstance)
 
-    # If truthy, then options.global indicates that we were activated by the user (with "i").
+    # If truthy, then we were activated by the user (with "i").
     @global = options.global
 
     defaults =
@@ -21,7 +21,7 @@ class InsertMode extends Mode
 
     @insertModeLock =
       if document.activeElement and DomUtils.isEditable document.activeElement
-        #  An input element is already active, so use it.
+        # An input element is already active, so use it.
         document.activeElement
       else
         null
@@ -30,9 +30,9 @@ class InsertMode extends Mode
       "blur": (event) => @alwaysContinueBubbling =>
         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".  We track the active element in @insertModeLock, and
+        # 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 and DomUtils.isFocusable target
+        @exit event, target if target == @insertModeLock
       "focus": (event) => @alwaysContinueBubbling =>
         if @insertModeLock != event.target and DomUtils.isFocusable event.target
           @insertModeLock = event.target
@@ -44,7 +44,7 @@ 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 document.activeElement?.isContentEditable and @insertModeLock != document.activeElement
+    if @insertModeLock != document.activeElement and document.activeElement?.isContentEditable
       @insertModeLock = document.activeElement
       Mode.updateBadge()
     @insertModeLock != null
@@ -75,7 +75,7 @@ class InsertMode extends Mode
   updateBadge: (badge) ->
     badge.badge ||= "I" if @isActive badge
 
-  # Static stuff. This allows PostFindMode to suppress insert mode.
+  # Static stuff. This allows PostFindMode to suppress the permanently-installed InsertMode instance.
   @suppressedEvent: null
   @suppressEvent: (event) -> @suppressedEvent = event
 
diff --git a/content_scripts/mode_passkeys.coffee b/content_scripts/mode_passkeys.coffee
index a40fe7a6..94a7c7ec 100644
--- a/content_scripts/mode_passkeys.coffee
+++ b/content_scripts/mode_passkeys.coffee
@@ -16,8 +16,9 @@ class PassKeysMode extends Mode
     else
       @continueBubbling
 
-  updateBadge: (badge) ->
-    badge.badge ||= "P" if @passKeys and not @keyQueue
+  # Disabled, pending experimentation with how/whether to use badges (smblott, 2015/01/17).
+  # updateBadge: (badge) ->
+  #   badge.badge ||= "P" if @passKeys and not @keyQueue
 
 root = exports ? window
 root.PassKeysMode = PassKeysMode
diff --git a/tests/dom_tests/dom_tests.coffee b/tests/dom_tests/dom_tests.coffee
index dae69dd8..c73e0885 100644
--- a/tests/dom_tests/dom_tests.coffee
+++ b/tests/dom_tests/dom_tests.coffee
@@ -669,7 +669,7 @@ context "Mode badges",
   tearDown ->
     restoreStackState()
 
-  should "have an N badge without passKeys", ->
+  should "have no badge without passKeys", ->
     handlerStack.bubbleEvent "registerStateChange",
       enabled: true
       passKeys: ""
@@ -677,13 +677,13 @@ context "Mode badges",
     handlerStack.bubbleEvent "updateBadge", badge = { badge: "" }
     assert.isTrue badge.badge == ""
 
-  should "have an P badge with passKeys", ->
+  should "have no badge with passKeys", ->
     handlerStack.bubbleEvent "registerStateChange",
       enabled: true
       passKeys: "p"
 
     handlerStack.bubbleEvent "updateBadge", badge = { badge: "" }
-    assert.isTrue badge.badge == "P"
+    assert.isTrue badge.badge == ""
 
   should "have no badge when disabled", ->
     handlerStack.bubbleEvent "registerStateChange",