Merge pull request #1950 from smblott-github/filtered-hints--wait-for-enter

For filtered hints, wait for <Enter> before activating link

5 files changed, 89 insertions, 24 deletions

diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee
index db8fe300..699c911a 100644
--- a/content_scripts/link_hints.coffee
+++ b/content_scripts/link_hints.coffee
@@ -367,7 +367,7 @@ class LinkHintsMode
     if linksMatched.length == 0
       @deactivateMode()
     else if linksMatched.length == 1
-      @activateLink linksMatched[0], keyResult.delay ? 0
+      @activateLink linksMatched[0], keyResult.delay ? 0, keyResult.waitForEnter and Settings.get "waitForEnterForFilteredHints"
     else
       @hideMarker marker for marker in hintMarkers
       @showMarker matched, @markerMatcher.hintKeystrokeQueue.length for matched in linksMatched
@@ -375,8 +375,8 @@ class LinkHintsMode
   #
   # When only one link hint remains, this function activates it in the appropriate way.
   #
-  activateLink: (matchedLink, delay = 0) ->
-    clickEl = matchedLink.clickableItem
+  activateLink: (@matchedLink, delay = 0, waitForEnter = false) ->
+    clickEl = @matchedLink.clickableItem
     if (DomUtils.isSelectable(clickEl))
       DomUtils.simulateSelect(clickEl)
       @deactivateMode delay
@@ -384,12 +384,18 @@ class LinkHintsMode
       # TODO figure out which other input elements should not receive focus
       if (clickEl.nodeName.toLowerCase() == "input" and clickEl.type not in ["button", "submit"])
         clickEl.focus()
-      DomUtils.flashRect(matchedLink.rect)
-      @linkActivator(clickEl)
-      if @mode is OPEN_WITH_QUEUE
-        @deactivateMode delay, -> LinkHints.activateModeWithQueue()
-      else
-        @deactivateMode delay
+
+      linkActivator = =>
+        @linkActivator(clickEl)
+        LinkHints.activateModeWithQueue() if @mode is OPEN_WITH_QUEUE
+
+      delay = 0 if waitForEnter
+      @deactivateMode delay, =>
+        if waitForEnter
+          new WaitForEnter @matchedLink.rect, linkActivator
+        else
+          DomUtils.flashRect @matchedLink.rect
+          linkActivator()
 
   #
   # Shows the marker, highlighting matchingCharCount characters.
@@ -419,7 +425,7 @@ class LinkHintsMode
     if delay
       # Install a mode to block keyboard events if the user is still typing.  The intention is to prevent the
       # user from inadvertently launching Vimium commands when typing the link text.
-      new TypingProtector delay, ->
+      new TypingProtector delay, @matchedLink?.rect, ->
         deactivate()
         callback?()
     else
@@ -571,7 +577,7 @@ class FilterHints
     @activeHintMarker = linksMatched[tabCount]
     @activeHintMarker?.classList.add "vimiumActiveHintMarker"
 
-    { linksMatched: linksMatched, delay: delay }
+    { linksMatched: linksMatched, delay: delay, waitForEnter: 0 < delay }
 
   pushKeyChar: (keyChar, keydownKeyChar) ->
     # For filtered hints, we *always* use the keyChar value from keypress, because there is no obvious and
@@ -644,12 +650,12 @@ spanWrap = (hintString) ->
 
 # Suppress all keyboard events until the user stops typing for sufficiently long.
 class TypingProtector extends Mode
-  constructor: (delay, callback) ->
+  constructor: (delay, rect, callback) ->
     @timer = Utils.setTimeout delay, => @exit()
 
     handler = (event) =>
       clearTimeout @timer
-      @timer = Utils.setTimeout 150, => @exit()
+      @timer = Utils.setTimeout delay, => @exit()
 
     super
       name: "hint/typing-protector"
@@ -657,7 +663,34 @@ class TypingProtector extends Mode
       keydown: handler
       keypress: handler
 
+    if rect
+      # We keep a "flash" overlay active while the user is typing; this provides visual feeback that something
+      # has been selected.
+      flashEl = DomUtils.addFlashRect rect
+      @onExit -> DomUtils.removeElement flashEl
+
     @onExit callback
 
+
+class WaitForEnter extends Mode
+  constructor: (rect, callback) ->
+    super
+      name: "hint/wait-for-enter"
+      suppressAllKeyboardEvents: true
+      exitOnEscape: true
+      indicator: "Hit <Enter> to proceed..."
+
+    @push
+      keydown: (event) =>
+        if event.keyCode == keyCodes.enter
+          @exit()
+          callback()
+          DomUtils.suppressEvent event
+        else
+          true
+
+    flashEl = DomUtils.addFlashRect rect
+    @onExit -> DomUtils.removeElement flashEl
+
 root = exports ? window
 root.LinkHints = LinkHints
diff --git a/lib/dom_utils.coffee b/lib/dom_utils.coffee
index 7473df17..db90c43a 100644
--- a/lib/dom_utils.coffee
+++ b/lib/dom_utils.coffee
@@ -262,8 +262,7 @@ DomUtils =
       # but Webkit will. Dispatching a click on an input box does not seem to focus it; we do that separately
       element.dispatchEvent(mouseEvent)
 
-  # momentarily flash a rectangular border to give user some visual feedback
-  flashRect: (rect) ->
+  addFlashRect: (rect) ->
     flashEl = @createElement "div"
     flashEl.id = "vimiumFlash"
     flashEl.className = "vimiumReset"
@@ -271,7 +270,12 @@ DomUtils =
     flashEl.style.top = rect.top  + window.scrollY  + "px"
     flashEl.style.width = rect.width + "px"
     flashEl.style.height = rect.height + "px"
-    document.documentElement.appendChild(flashEl)
+    document.documentElement.appendChild flashEl
+    flashEl
+
+  # momentarily flash a rectangular border to give user some visual feedback
+  flashRect: (rect) ->
+    flashEl = @addFlashRect rect
     setTimeout((-> DomUtils.removeElement flashEl), 400)
 
   suppressPropagation: (event) ->
diff --git a/lib/settings.coffee b/lib/settings.coffee
index 79ee04a9..da45eb61 100644
--- a/lib/settings.coffee
+++ b/lib/settings.coffee
@@ -170,6 +170,7 @@ Settings =
     newTabUrl: "chrome://newtab"
     grabBackFocus: false
     regexFindMode: false
+    waitForEnterForFilteredHints: false # Note: this defaults to true for new users; see below.
 
     settingsVersion: Utils.getCurrentVersion()
     helpDialog_showAdvancedCommands: false
@@ -181,6 +182,15 @@ Settings.init()
 # Perform migration from old settings versions, if this is the background page.
 if Utils.isBackgroundPage()
 
+  if not Settings.get "settingsVersion"
+    # This is a new install.  For some settings, we retain a legacy default behaviour for existing users but
+    # use a non-default behaviour for new users.
+
+    # For waitForEnterForFilteredHints, we (smblott) think that "true" gives a better UX; see #1950.  However,
+    # forcing the change on existing users would be unnecessarily disruptive.  So, only new users default to
+    # "true".
+    Settings.set "waitForEnterForFilteredHints", true
+
   # We use settingsVersion to coordinate any necessary schema changes.
   Settings.set("settingsVersion", Utils.getCurrentVersion())
 
diff --git a/pages/options.coffee b/pages/options.coffee
index f0de5342..51400740 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -184,6 +184,7 @@ class ExclusionRulesOnPopupOption extends ExclusionRulesOption
 Options =
   exclusionRules: ExclusionRulesOption
   filterLinkHints: CheckBoxOption
+  waitForEnterForFilteredHints: CheckBoxOption
   hideHud: CheckBoxOption
   keyMappings: TextOption
   linkHintCharacters: NonEmptyTextOption
@@ -206,14 +207,16 @@ initOptionsPage = ->
 
   # Display either "linkHintNumbers" or "linkHintCharacters", depending upon "filterLinkHints".
   maintainLinkHintsView = ->
-    hide = (el) -> el.parentNode.parentNode.style.display = "none"
-    show = (el) -> el.parentNode.parentNode.style.display = "table-row"
+    hide = (el) -> el.style.display = "none"
+    show = (el) -> el.style.display = "table-row"
     if $("filterLinkHints").checked
-      hide $("linkHintCharacters")
-      show $("linkHintNumbers")
+      hide $("linkHintCharactersContainer")
+      show $("linkHintNumbersContainer")
+      show $("waitForEnterForFilteredHintsContainer")
     else
-      show $("linkHintCharacters")
-      hide $("linkHintNumbers")
+      show $("linkHintCharactersContainer")
+      hide $("linkHintNumbersContainer")
+      hide $("waitForEnterForFilteredHintsContainer")
 
   maintainAdvancedOptions = ->
     if bgSettings.get "optionsPage_showAdvancedOptions"
diff --git a/pages/options.html b/pages/options.html
index b5aa5936..ac515db9 100644
--- a/pages/options.html
+++ b/pages/options.html
@@ -82,7 +82,7 @@ b: http://b.com/?q=%s description
               <input id="scrollStepSize" type="number" />px
             </td>
           </tr>
-          <tr>
+          <tr id="linkHintCharactersContainer">
             <td class="caption">Characters used<br/> for link hints</td>
             <td verticalAlign="top">
                 <div class="help">
@@ -94,7 +94,7 @@ b: http://b.com/?q=%s description
                 <div class="nonEmptyTextOption">
             </td>
           </tr>
-          <tr>
+          <tr id="linkHintNumbersContainer">
             <td class="caption">Numbers used<br/> for link hints</td>
             <td verticalAlign="top">
                 <div class="help">
@@ -129,6 +129,21 @@ b: http://b.com/?q=%s description
               </label>
             </td>
           </tr>
+          <tr id="waitForEnterForFilteredHintsContainer">
+            <td class="caption"></td>
+            <td verticalAlign="top" class="booleanOption">
+              <div class="help">
+                <div class="example">
+                  You activate the link with <tt>Enter</tt>, <em>always</em>; so you never accidentally type Vimium
+                  commands.
+                </div>
+              </div>
+              <label>
+                <input id="waitForEnterForFilteredHints" type="checkbox"/>
+                Require <tt>Enter</tt> when filtering hints
+              </label>
+            </td>
+          </tr>
           <tr>
             <td class="caption"></td>
             <td verticalAlign="top" class="booleanOption">