Modes; reinstate key blockers:

- when the selection is contentEditable
- in PostFindMode

Restricted to printable characters.

4 files changed, 47 insertions, 13 deletions

diff --git a/content_scripts/mode.coffee b/content_scripts/mode.coffee
index a33197b0..5c6201e0 100644
--- a/content_scripts/mode.coffee
+++ b/content_scripts/mode.coffee
@@ -111,23 +111,21 @@ class Mode
               @passKeys = passKeys
               @registerStateChange?()
 
-    # If @options.trapAllKeyboardEvents is truthy, then it should be an element.  All keyboard events on that
-    # element are suppressed *after* bubbling the event down the handler stack.  This prevents such events
-    # from propagating to other extensions or the host page.
-    if @options.trapAllKeyboardEvents
+    # If @options.suppressPrintableEvents is truthy, then it should be an element.  All printable keyboard
+    # events on that element are suppressed, if necessary (that is, *after* bubbling down the handler stack).
+    # We only suppress keypress events.  This is used by PostFindMode to protect active, editable elements.
+    # Note: We use unshift here, not push, so the handler is installed at the bottom of the stack.
+    if @options.suppressPrintableEvents
       @unshift
-        _name: "mode-#{@id}/trapAllKeyboardEvents"
-        keydown: (event) =>
-          if event.srcElement == @options.trapAllKeyboardEvents then @suppressEvent else @continueBubbling
+        _name: "mode-#{@id}/suppressPrintableEvents"
         keypress: (event) =>
-          if event.srcElement == @options.trapAllKeyboardEvents then @suppressEvent else @continueBubbling
-        keyup: (event) =>
-          if event.srcElement == @options.trapAllKeyboardEvents then @suppressEvent else @continueBubbling
+          if DomUtils.isPrintable(event) and
+            event.srcElement == @options.suppressPrintableEvents then @suppressEvent else @continueBubbling
 
     Mode.updateBadge() if @badge
     Mode.modes.push @
     @log() if @debug
-    handlerStack.debugOn()
+    # handlerStack.debugOn()
     # End of Mode constructor.
 
   push: (handlers) ->
diff --git a/content_scripts/mode_find.coffee b/content_scripts/mode_find.coffee
index 740358d5..91ae4507 100644
--- a/content_scripts/mode_find.coffee
+++ b/content_scripts/mode_find.coffee
@@ -1,10 +1,14 @@
 # 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,
-# special considerations apply.  We implement two special cases:
+# special considerations apply.  We implement three special cases:
 #   1. Prevent keyboard events from dropping us unintentionally into insert mode. This is achieved by
 #      inheriting from InsertModeBlocker.
-#   2. If the very-next keystroke is Escape, then drop immediately into insert mode.
+#   2. Prevent all printable keyboard events on the active element from propagating.  This is achieved by setting the
+#      suppressPrintableEvents option.  There's some controversy as to whether this is the right thing to do.
+#      See discussion in #1415. This implements Option 2 from there, although Option 3 would be a reasonable
+#      alternative.
+#   3. If the very-next keystroke is Escape, then drop immediately into insert mode.
 #
 class PostFindMode extends InsertModeBlocker
   constructor: (findModeAnchorNode) ->
@@ -16,6 +20,7 @@ class PostFindMode extends InsertModeBlocker
       # instance is automatically deactivated when a new instance is activated.
       singleton: PostFindMode
       exitOnBlur: element
+      suppressPrintableEvents: element
 
     return @exit() unless element and findModeAnchorNode
 
diff --git a/content_scripts/mode_insert.coffee b/content_scripts/mode_insert.coffee
index 5720c901..b907f22e 100644
--- a/content_scripts/mode_insert.coffee
+++ b/content_scripts/mode_insert.coffee
@@ -96,6 +96,34 @@ class InsertModeBlocker extends Mode
             new @options.onClickMode
               targetElement: document.activeElement
 
+# There's an unfortunate feature interaction between chrome's contentEditable handling and our insert mode.
+# If the selection is contentEditable and a descendant of the active element, then chrome focuses it on any
+# unsuppressed printable keypress.  This drops us unintentally into insert mode.  See #1415.  A single
+# instance of this mode sits near the bottom of the handler stack and suppresses each keypress event if:
+#   - it hasn't been handled by any other mode (so not by normal mode, passkeys, insert, ...),
+#   - it represents a printable character,
+#   - the selection is content editable, and
+#   - the selection is a descendant of the active element.
+# This should rarely fire, typically only on fudged keypresses in normal mode.  And, even then, only in the
+# circumstances outlined above.  So, we shouldn't usually be blocking keyboard events for other extensions or
+# the page itself.
+# There's some controversy as to whether this is the right thing to do.  See discussion in #1415. This
+# implements Option 2 from there.
+new class ContentEditableTrap extends Mode
+  constructor: ->
+    super
+      name: "content-editable-trap"
+      keypress: (event) =>
+        if @wouldTriggerInsert event then @suppressEvent else @continueBubbling
+
+  # True if the selection is content editable and a descendant of the active element.
+  wouldTriggerInsert: (event) ->
+    element = document.getSelection()?.anchorNode?.parentElement
+    return element?.isContentEditable and
+             document.activeElement and
+             DomUtils. isPrintable event and
+             DomUtils.isDOMDescendant document.activeElement, element
+
 root = exports ? window
 root.InsertMode = InsertMode
 root.InsertModeTrigger = InsertModeTrigger
diff --git a/lib/dom_utils.coffee b/lib/dom_utils.coffee
index 322188b3..fd2427c4 100644
--- a/lib/dom_utils.coffee
+++ b/lib/dom_utils.coffee
@@ -212,5 +212,8 @@ DomUtils =
         @remove()
         false
 
+  isPrintable: (event) ->
+    not (event.metaKey or event.ctrlKey or event.altKey)
+
 root = exports ? window
 root.DomUtils = DomUtils