Visual/edit modes: code cleanup.

- convert getCaretCoordinates from JS to CS
- handle x axis in scrollIntoView
- better comments throughout.

3 files changed, 99 insertions, 132 deletions

diff --git a/content_scripts/mode_visual_edit.coffee b/content_scripts/mode_visual_edit.coffee
index ba0bc307..e11c29ec 100644
--- a/content_scripts/mode_visual_edit.coffee
+++ b/content_scripts/mode_visual_edit.coffee
@@ -26,7 +26,7 @@
 # - ..., EditMode, VisualLineMode
 #
 
-# This prevents printable characters from being passed through to underlying modes or to the underlying page.
+# This prevents printable characters from being passed through to underlying modes or the underlying page.
 class SuppressPrintable extends Mode
   constructor: (options = {}) ->
     handler = (event) =>
@@ -44,7 +44,8 @@ class SuppressPrintable extends Mode
 class CountPrefix extends SuppressPrintable
   constructor: (options) ->
     @countPrefix = ""
-    # This allows us to implement both "d3w" and "3dw". Also, "3d2w" deletes six words.
+    # This is an initial multiplier for the first count.  It allows edit mode to implement both "d3w" and
+    # "3dw". Also, "3d2w" deletes six words.
     @countPrefixFactor = options.initialCountPrefix || 1
     super options
 
@@ -61,9 +62,8 @@ class CountPrefix extends SuppressPrintable
                 ""
 
   getCountPrefix: ->
-    count = @countPrefixFactor * if 0 < @countPrefix?.length then parseInt @countPrefix else 1
-    @countPrefix = ""
-    @countPrefixFactor = 1
+    count = @countPrefixFactor * (if 0 < @countPrefix.length then parseInt @countPrefix else 1)
+    @countPrefix = ""; @countPrefixFactor = 1
     count
 
 # Symbolic names for some common strings.
@@ -81,9 +81,11 @@ lineboundary= "lineboundary"
 class Movement extends CountPrefix
   opposite: forward: backward, backward: forward
 
+  # Paste from clipboard.
   paste: (callback) ->
     chrome.runtime.sendMessage handler: "pasteFromClipboard", (response) -> callback response
 
+  # Copy to clipboard.
   copy: (text, isFinalUserCopy = false) ->
     chrome.runtime.sendMessage handler: "copyToClipboard", data: text
     # If isFinalUserCopy is set, then we're copying the final text selected by the user (and exiting).
@@ -105,7 +107,8 @@ class Movement extends CountPrefix
         @paste (text) =>
           func(); @copy text; locked = false
 
-  # Replace the current mode with another.  For example, replace visual mode with visual-line mode.
+  # Replace the current mode with another. For example, replace caret mode with visual mode, or replace visual
+  # mode with visual-line mode.
   changeMode: (mode, options = {}) ->
     @exit()
     if @options.parentMode
@@ -114,7 +117,7 @@ class Movement extends CountPrefix
       new mode options
 
   # Return the character following (to the right of) the focus, and leave the selection unchanged.  Returns
-  # undefined if there is no such character.
+  # undefined if no such character exists.
   getNextForwardCharacter: ->
     beforeText = @selection.toString()
     if beforeText.length == 0 or @getDirection() == forward
@@ -124,7 +127,7 @@ class Movement extends CountPrefix
         @selection.modify "extend", backward, character
         afterText[afterText.length - 1]
     else
-      beforeText[0]
+      beforeText[0] # Existing range selection is backwards.
 
   # As above, but backwards.
   getNextBackwardCharacter: ->
@@ -136,9 +139,9 @@ class Movement extends CountPrefix
         @selection.modify "extend", forward, character
         afterText[0]
     else
-      beforeText[beforeText.length - 1]
+      beforeText[beforeText.length - 1] # Existing range selection is forwards.
 
-  # Test whether the character following the focus is a word character.  Leave the selection unchanged.
+  # Test whether the character following the focus is a word character (and leave the selection unchanged).
   nextCharacterIsWordCharacter: do ->
     regexp = /[A-Za-z0-9_]/; -> regexp.test @getNextForwardCharacter()
 
@@ -148,8 +151,8 @@ class Movement extends CountPrefix
   #   @runMovement [ "forward", "word" ]
   #   @runMovement "forward", "word"
   #
-  # The granularities are word, "line", "lineboundary", "sentence" and "paragraph".  In addition, we implement
-  # the pseudo granularity "vimword", which implements vim-like word movement (for "w").
+  # The granularities are word, "character", "line", "lineboundary", "sentence" and "paragraph".  In addition,
+  # we implement the pseudo granularity "vimword", which implements vim-like word movement (for "w").
   #
   runMovement: (args...) ->
     # Normalize the various argument forms.
@@ -217,8 +220,8 @@ class Movement extends CountPrefix
       which = if direction == forward then "start" else "end"
       @selection.extend original["#{which}Container"], original["#{which}Offset"]
 
-  # Try to extend the selection one character in direction.  Return 1, -1 or 0, indicating whether the
-  # selection got bigger, or smaller, or is unchanged.
+  # Try to extend the selection one character in direction.  Return positive, negative or 0, indicating
+  # whether the selection got bigger, or smaller, or is unchanged.
   extendByOneCharacter: (direction) ->
     length = @selection.toString().length
     @selection.modify "extend", direction, character
@@ -226,8 +229,8 @@ class Movement extends CountPrefix
 
   # Get the direction of the selection.  The selection is "forward" if the focus is at or after the anchor,
   # and "backward" otherwise.
-  # NOTE(smblott). This could be better, see: https://dom.spec.whatwg.org/#interface-range (haowever, that probably
-  # wouldn't work for text inputs).
+  # NOTE(smblott). This could be better, see: https://dom.spec.whatwg.org/#interface-range (however, that
+  # probably wouldn't work for text inputs).
   getDirection: ->
     # Try to move the selection forward or backward, check whether it got bigger or smaller (then restore it).
     for direction in [ forward, backward ]
@@ -248,8 +251,8 @@ class Movement extends CountPrefix
     @selection.removeAllRanges()
     @selection.addRange range
 
-  # A movement can be a string (which will be passed to @runMovement count times), or a function (which will
-  # be called once with count as its argument).
+  # A movement can be either a string (which will be passed to @runMovement count times), or a function (which
+  # will be called once with count as its argument).
   movements:
     "l": "forward character"
     "h": "backward character"
@@ -309,7 +312,8 @@ class Movement extends CountPrefix
       @runMovementKeyChar @options.immediateMovement, @getCountPrefix()
       return
 
-    # This is the main keyboard-event handler for movements and commands.
+    # This is the main keyboard-event handler for movements and commands for all user modes (visual,
+    # visual-line, caret and edit).
     @push
       _name: "#{@id}/keypress"
       keypress: (event) =>
@@ -357,24 +361,24 @@ class Movement extends CountPrefix
         @movements.n = (count) -> executeFind count, false
         @movements.N = (count) -> executeFind count, true
         @movements["/"] = ->
-          @findMode = enterFindMode()
+          @findMode = window.enterFindMode()
           @findMode.onExit => @changeMode VisualMode
     #
     # End of Movement constructor.
 
-  # Yank the selection; always exits; either deletes the selection or collapses it; set @yankedText and
-  # returns it.
+  # Yank the selection; always exits; either deletes the selection or removes it; set @yankedText and return
+  # it.
   yank: (args = {}) ->
     @yankedText = @selection.toString()
     @selection.deleteFromDocument() if @options.deleteFromDocument or args.deleteFromDocument
-    @selection.removeAllRanges()
+    @selection.removeAllRanges() unless @options.parentMode
 
     message = @yankedText.replace /\s+/g, " "
     message = message[...12] + "..." if 15 < @yankedText.length
     plural = if @yankedText.length == 1 then "" else "s"
     HUD.showForDuration "Yanked #{@yankedText.length} character#{plural}: \"#{message}\".", 2500
 
-    @options.onYank.call @, @yankedText if @options.onYank
+    @options.onYank?.call @, @yankedText
     @exit()
     @yankedText
 
@@ -434,15 +438,14 @@ class Movement extends CountPrefix
           char = @getNextForwardCharacter()
         @runMovement forward, character
 
-  # Try to scroll the focus into view.
+  # Scroll the focus into view.
   scrollIntoView: ->
     @protectClipboard =>
       if @element and DomUtils.isEditable @element
         if @element.clientHeight < @element.scrollHeight
           if @element.isContentEditable
-            # WIP...
+            # WIP (edit mode only)...
             elementWithFocus = DomUtils.getElementWithFocus @selection, @getDirection() == backward
-            console.log elementWithFocus.innerHTML
             # position = @element.getClientRects()[0].top - elementWithFocus.getClientRects()[0].top
             # console.log "top", position
             # Scroller.scrollToPosition @element, position, 0
@@ -547,6 +550,7 @@ class VisualMode extends Movement
         console.log "yank:", @yankedText if @debug
         @copy @yankedText, true
 
+  # Call sub-class; then yank, if we've only been created for a single movement.
   handleMovementKeyChar: (args...) ->
     super args...
     @yank() if @options.oneMovementOnly or @options.immediateMovement
diff --git a/content_scripts/scroller.coffee b/content_scripts/scroller.coffee
index 6d224814..08cc0779 100644
--- a/content_scripts/scroller.coffee
+++ b/content_scripts/scroller.coffee
@@ -209,7 +209,7 @@ CoreScroller =
     # Launch animator.
     requestAnimationFrame animate
 
-# Scroller contains the two main scroll functions (scrollBy and scrollTo) which are exported to clients.
+# Scroller contains the two main scroll functions which are used by clients.
 Scroller =
   init: (frontendSettings) ->
     handlerStack.push
@@ -246,31 +246,43 @@ Scroller =
     amount = getDimension(element,direction,pos) - element[scrollProperties[direction].axisName]
     CoreScroller.scroll element, direction, amount
 
-  # FIXME(smblott). We should also scroll in the "x" dimension.
+  # Scroll the top, bottom, left and right of element into view.  The is used by visual mode to ensure the
+  # focus remains visible.
   scrollIntoView: (element) ->
     activatedElement ||= document.body and firstScrollableElement()
-    rect = element.getBoundingClientRect()
-    direction = "y"
-    if rect.top < 0
-      amount = rect.top - 10
-      element = findScrollableElement element, direction, amount, 1
-      CoreScroller.scroll element, direction, amount, false
-    else if window.innerHeight < rect.bottom
-      amount = rect.bottom - window.innerHeight + 10
-      element = findScrollableElement element, direction, amount, 1
-      CoreScroller.scroll element, direction, amount, false
-
+    rect = element. getClientRects()?[0]
+    if rect?
+      # Scroll y axis.
+      if rect.top < 0
+        amount = rect.top - 10
+        element = findScrollableElement element, "y", amount, 1
+        CoreScroller.scroll element, "y", amount, false
+      else if window.innerHeight < rect.bottom
+        amount = rect.bottom - window.innerHeight + 10
+        element = findScrollableElement element, "y", amount, 1
+        CoreScroller.scroll element, "y", amount, false
+
+      # Scroll x axis.
+      if rect.left < 0
+        amount = rect.left - 10
+        element = findScrollableElement element, "x", amount, 1
+        CoreScroller.scroll element, "x", amount, false
+      else if window.innerWidth < rect.right
+        amount = rect.right - window.innerWidth + 10
+        element = findScrollableElement element, "x", amount, 1
+        CoreScroller.scroll element, "x", amount, false
+
+  # Scroll element to position top, left.  This is used by edit mode to ensure that the caret remains visible
+  # in text inputs (not contentEditable).
   scrollToPosition: (element, top, left) ->
     activatedElement ||= document.body and firstScrollableElement()
 
     # Scroll down, "y".
     amount = top + 20 - (element.clientHeight + element.scrollTop)
-    console.log "y down", amount, 0 < amount
     CoreScroller.scroll element, "y", amount, false if 0 < amount
 
     # Scroll up, "y".
     amount = top - (element.scrollTop) - 5
-    console.log "y up", amount, amount < 0
     CoreScroller.scroll element, "y", amount, false if amount < 0
 
     # Scroll down, "x".
diff --git a/lib/dom_utils.coffee b/lib/dom_utils.coffee
index 9360bb95..2ae9412e 100644
--- a/lib/dom_utils.coffee
+++ b/lib/dom_utils.coffee
@@ -250,99 +250,50 @@ DomUtils =
     t = o || t?.parentNode
     t
 
-extend DomUtils,
+  # This calculates the caret coordinates within an input element.  It is used by edit mode to calculate the
+  # caret position for scrolling.  It creates a hidden div contain a mirror of element, and all of the text
+  # from element up to position, then calculates the scroll position.
   # From: https://github.com/component/textarea-caret-position/blob/master/index.js
   getCaretCoordinates: do ->
-    # The properties that we copy into a mirrored div.
-    # Note that some browsers, such as Firefox,
-    # do not concatenate properties, i.e. padding-top, bottom etc. -> padding,
-    # so we have to do every single property specifically.
+    # The properties that we copy to the mirrored div.
     properties = [
-      'direction',  # RTL support
-      'boxSizing',
-      'width',  # on Chrome and IE, exclude the scrollbar, so the mirror div wraps exactly as the textarea does
-      'height',
-      'overflowX',
-      'overflowY',  # copy the scrollbar for IE
-
-      'borderTopWidth',
-      'borderRightWidth',
-      'borderBottomWidth',
-      'borderLeftWidth',
-
-      'paddingTop',
-      'paddingRight',
-      'paddingBottom',
-      'paddingLeft',
-
-      # https://developer.mozilla.org/en-US/docs/Web/CSS/font
-      'fontStyle',
-      'fontVariant',
-      'fontWeight',
-      'fontStretch',
-      'fontSize',
-      'fontSizeAdjust',
-      'lineHeight',
-      'fontFamily',
-
-      'textAlign',
-      'textTransform',
-      'textIndent',
-      'textDecoration',  # might not make a difference, but better be safe
-
-      'letterSpacing',
-      'wordSpacing'
-    ]
-
-    `function (element, position, recalculate) {
-      // mirrored div
-      var div = document.createElement('div');
-      div.id = 'input-textarea-caret-position-mirror-div';
-      document.body.appendChild(div);
-
-      var style = div.style;
-      var computed = window.getComputedStyle? getComputedStyle(element) : element.currentStyle;  // currentStyle for IE < 9
-
-      // default textarea styles
-      style.whiteSpace = 'pre-wrap';
-      if (element.nodeName !== 'INPUT')
-        style.wordWrap = 'break-word';  // only for textarea-s
-
-      // position off-screen
-      style.position = 'absolute';  // required to return coordinates properly
-      style.visibility = 'hidden';  // not 'display: none' because we want rendering
-
-      // transfer the element's properties to the div
-      properties.forEach(function (prop) {
-        style[prop] = computed[prop];
-      });
-
-      style.overflow = 'hidden';  // for Chrome to not render a scrollbar; IE keeps overflowY = 'scroll'
-
-      div.textContent = element.value.substring(0, position);
-      // the second special handling for input type="text" vs textarea: spaces need to be replaced with non-breaking spaces - http://stackoverflow.com/a/13402035/1269037
-      if (element.nodeName === 'INPUT')
-        div.textContent = div.textContent.replace(/\s/g, "\u00a0");
-
-      var span = document.createElement('span');
-      // Wrapping must be replicated *exactly*, including when a long word gets
-      // onto the next line, with whitespace at the end of the line before (#7).
-      // The  *only* reliable way to do that is to copy the *entire* rest of the
-      // textarea's content into the <span> created at the caret position.
-      // for inputs, just '.' would be enough, but why bother?
-      span.textContent = element.value.substring(position) || '.';  // || because a completely empty faux span doesn't render at all
-      div.appendChild(span);
-
-      var coordinates = {
-        top: span.offsetTop + parseInt(computed['borderTopWidth']),
-        left: span.offsetLeft + parseInt(computed['borderLeftWidth'])
-      };
-
-      document.body.removeChild(div);
-
-      return coordinates;
-    }
-    `
+      'direction', 'boxSizing', 'width', 'height', 'overflowX', 'overflowY',
+      'borderTopWidth', 'borderRightWidth', 'borderBottomWidth', 'borderLeftWidth',
+      'paddingTop', 'paddingRight', 'paddingBottom', 'paddingLeft',
+      'fontStyle', 'fontVariant', 'fontWeight', 'fontStretch', 'fontSize', 'fontSizeAdjust',
+      'lineHeight', 'fontFamily',
+      'textAlign', 'textTransform', 'textIndent', 'textDecoration',
+      'letterSpacing', 'wordSpacing' ]
+
+    (element, position) ->
+      div = document.createElement "div"
+      div.id = "vimium-input-textarea-caret-position-mirror-div"
+      document.body.appendChild div
+
+      style = div.style
+      computed = getComputedStyle element
+
+      style.whiteSpace = "pre-wrap"
+      style.wordWrap = "break-word" if element.nodeName.toLowerCase() != "input"
+      style.position = "absolute"
+      style.visibility = "hidden"
+      style[prop] = computed[prop] for prop in properties
+      style.overflow = "hidden"
+
+      div.textContent = element.value.substring 0, position
+      if element.nodeName.toLowerCase() == "input"
+        div.textContent = div.textContent.replace /\s/g, "\u00a0"
+
+      span = document.createElement "span"
+      span.textContent = element.value.substring(position) || "."
+      div.appendChild span
+
+      coordinates =
+        top: span.offsetTop + parseInt computed["borderTopWidth"]
+        left: span.offsetLeft + parseInt computed["borderLeftWidth"]
+
+      document.body.removeChild div
+      coordinates
 
 root = exports ? window
 root.DomUtils = DomUtils