Modes; rework Singletons, InsertModeBlocker and HandlerStack.

This begins work on addressing @philc's comments in #1413.  That work is
nevertheless not yet complete.

5 files changed, 61 insertions, 36 deletions

diff --git a/content_scripts/mode.coffee b/content_scripts/mode.coffee
index ff75460f..e9a4a621 100644
--- a/content_scripts/mode.coffee
+++ b/content_scripts/mode.coffee
@@ -75,6 +75,7 @@ class Mode
       keyup: @keyup
       updateBadge: (badge) => @alwaysContinueBubbling => @chooseBadge badge
 
+    @registerSingleton options.singleton if options.singleton
     Mode.updateBadge() if @badge
 
   push: (handlers) ->
@@ -116,6 +117,17 @@ class Mode
     func()
     mode.exit()
 
+  # 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.
+  @singletons: {}
+  registerSingleton: (singleton) ->
+    singletons = Mode.singletons
+    singletons[singleton].exit() if singletons[singleton]
+    singletons[singleton] = @
+    @onExit =>
+      delete singletons[singleton] if singletons[singleton] == @
+
 # A SingletonMode is a Mode of which there may be at most one instance (of @singleton) active at any one time.
 # New instances cancel previously-active instances on startup.
 class SingletonMode extends Mode
diff --git a/content_scripts/mode_find.coffee b/content_scripts/mode_find.coffee
index 44d50608..18cb7b71 100644
--- a/content_scripts/mode_find.coffee
+++ b/content_scripts/mode_find.coffee
@@ -10,8 +10,9 @@ class PostFindMode extends InsertModeBlocker
   constructor: (findModeAnchorNode) ->
     element = document.activeElement
 
-    super element,
+    super
       name: "post-find"
+      singleton: PostFindMode
 
     return @exit() unless element and findModeAnchorNode
 
diff --git a/content_scripts/mode_insert.coffee b/content_scripts/mode_insert.coffee
index 960b42f8..83d85fa7 100644
--- a/content_scripts/mode_insert.coffee
+++ b/content_scripts/mode_insert.coffee
@@ -55,21 +55,23 @@ class InsertModeTrigger extends Mode
     super
       name: "insert-trigger"
       keydown: (event, extra) =>
-        @alwaysContinueBubbling =>
-          unless InsertModeBlocker.isActive()
-            # 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.
-            new InsertMode document.activeElement if document.activeElement?.isContentEditable
+        return @continueBubbling if InsertModeBlocker.isActive extra
+        # 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.
+        return @continueBubbling unless document.activeElement?.isContentEditable
+        new InsertMode document.activeElement
+        @stopBubblingAndTrue
 
     @push
       focus: (event, extra) =>
         @alwaysContinueBubbling =>
-          unless InsertMode.isActive() or InsertModeBlocker.isActive()
+          unless InsertMode.isActive() or InsertModeBlocker.isActive extra
             new InsertMode event.target if isFocusable event.target
 
       click: (event, extra) =>
         @alwaysContinueBubbling =>
+          # Do not check InsertModeBlocker.isActive() here.  A user click overrides the blocker.
           unless InsertMode.isActive()
             if document.activeElement == event.target and isEditable event.target
               new InsertMode event.target
@@ -79,16 +81,16 @@ class InsertModeTrigger extends Mode
 
 # Disables InsertModeTrigger.  Used by find mode and findFocus to prevent unintentionally dropping into insert
 # mode on focusable elements.
-class InsertModeBlocker extends SingletonMode
-  constructor: (element, options={}) ->
+class InsertModeBlocker extends Mode
+  constructor: (options={}) ->
     options.name ||= "insert-blocker"
-    super InsertModeBlocker, options
+    super options
 
     @push
-      "blur": (event) => @alwaysContinueBubbling => @exit() if element? and event.srcElement == element
+      "all": (event, extra) => @alwaysContinueBubbling => extra.isInsertModeBlockerActive = true
 
-  # Static method. Return whether the insert-mode blocker is currently active or not.
-  @isActive: (singleton) -> SingletonMode.isActive InsertModeBlocker
+  # Static method. Return whether an insert-mode blocker is currently active or not.
+  @isActive: (extra) -> extra?.isInsertModeBlockerActive
 
 root = exports ? window
 root.InsertMode = InsertMode
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 24cc25c3..193a1592 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -378,9 +378,10 @@ extend window,
 
     new class FocusSelector extends InsertModeBlocker
       constructor: ->
-        super null,
+        super
           name: "focus-selector"
           badge: "?"
+          singleton: FocusSelector
           keydown: (event) =>
             if event.keyCode == KeyboardUtils.keyCodes.tab
               hints[selectedInputIndex].classList.remove 'internalVimiumSelectedInputHint'
@@ -388,11 +389,14 @@ extend window,
               selectedInputIndex %= hints.length
               hints[selectedInputIndex].classList.add 'internalVimiumSelectedInputHint'
               visibleInputs[selectedInputIndex].element.focus()
-              false
+              @suppressEvent
             else unless event.keyCode == KeyboardUtils.keyCodes.shiftKey
               @exit()
               @continueBubbling
 
+        # TODO. InsertModeBlocker is no longer a singleton.  Need to make this a singleton.  Fix once class
+        # hierarchy is removed.
+
         visibleInputs[selectedInputIndex].element.focus()
         @exit() if visibleInputs.length == 1
 
@@ -441,7 +445,7 @@ KeydownEvents =
 # Note that some keys will only register keydown events and not keystroke events, e.g. ESC.
 #
 
-onKeypress = (event) ->
+onKeypress = (event, extra) ->
   keyChar = ""
 
   # Ignore modifier keys by themselves.
@@ -465,14 +469,15 @@ onKeypress = (event) ->
 
         keyPort.postMessage({ keyChar:keyChar, frameId:frameId })
 
-  if InsertModeBlocker.isActive()
+  if InsertModeBlocker.isActive extra
     # If PostFindMode is active, then we're blocking vimium's keystrokes from going into an input
-    # element.  So we should also block other keystrokes (otherwise, it's weird).
+    # element.  So we should also block other keystrokes (otherwise, it's weird).  There's some controversy as
+    # to whether this is the right thing to do.  See discussion in #1415.
     DomUtils.suppressEvent(event)
 
   return true
 
-onKeydown = (event) ->
+onKeydown = (event, extra) ->
   keyChar = ""
 
   # handle special keys, and normal input keys with modifiers being pressed. don't handle shiftKey alone (to
@@ -563,9 +568,10 @@ onKeydown = (event) ->
       isValidFirstKey(KeyboardUtils.getKeyChar(event))))
     DomUtils.suppressPropagation(event)
     KeydownEvents.push event
-  else if InsertModeBlocker.isActive()
+  else if InsertModeBlocker.isActive extra
     # If PostFindMode is active, then we're blocking vimium's keystrokes from going into an input
-    # element.  So we should also block other keystrokes (otherwise, it's weird).
+    # element.  So we should also block other keystrokes (otherwise, it's weird).  There's some controversy as
+    # to whether this is the right thing to do.  See discussion in #1415.
     DomUtils.suppressPropagation(event)
     KeydownEvents.push event
 
diff --git a/lib/handler_stack.coffee b/lib/handler_stack.coffee
index 0a34087f..9da0bc33 100644
--- a/lib/handler_stack.coffee
+++ b/lib/handler_stack.coffee
@@ -15,8 +15,10 @@ class HandlerStack
     @stopBubblingAndFalse = new Object()
 
   # Adds a handler to the stack. Returns a unique ID for that handler that can be used to remove it later.
+  # We use unshift (which is more expensive than push) so that bubbleEvent can just iterate over the stack in
+  # the normal order.
   push: (handler) ->
-    @stack.push handler
+    @stack.unshift handler
     handler.id = ++@counter
 
   # Called whenever we receive a key or other event. Each individual handler has the option to stop the
@@ -25,26 +27,28 @@ class HandlerStack
   bubbleEvent: (type, event) ->
     # extra is passed to each handler.  This allows handlers to pass information down the stack.
     extra = {}
-    for i in [(@stack.length - 1)..0] by -1
-      handler = @stack[i]
-      # We need to check for existence of handler because the last function call may have caused the release
-      # of more than one handler.
-      if handler and handler.id and handler[type]
+    for handler in @stack[..] # Take a copy of @stack, so that concurrent removes do not interfere.
+      # We need to check whether the handler has been removed (handler.id == null).
+      if handler and handler.id
         @currentId = handler.id
-        passThrough = handler[type].call @, event, extra
-        if not passThrough
-          DomUtils.suppressEvent(event) if @isChromeEvent event
-          return false
-        return true if passThrough == @stopBubblingAndTrue
-        return false if passThrough == @stopBubblingAndFalse
+        # 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
     true
 
   remove: (id = @currentId) ->
     # This is more expense than splicing @stack, but better because splicing can interfere with concurrent
     # bubbleEvents.
     @stack = @stack.filter (handler) ->
-      # Mark this handler as removed (to notify any concurrent bubbleEvent call).
-      if handler.id == id then handler.id = null
+      # Mark this handler as removed (so concurrent bubbleEvents will know not to invoke it).
+      handler.id = null if handler.id == id
       handler?.id?
 
   # The handler stack handles chrome events (which may need to be suppressed) and internal (fake) events.