Merge pull request #347 from bustlelabs/fix-selection-arrow-movement

Ensure using arrow keys when text is selected moves cursor correctly

src/js/editor/editor.js

@@ -37,6 +37,7 @@ import EditHistory from 'mobiledoc-kit/editor/edit-history';
 import EventManager from 'mobiledoc-kit/editor/event-manager';
 import EditState from 'mobiledoc-kit/editor/edit-state';
 import Logger from 'mobiledoc-kit/utils/logger';
+let log = Logger.for('editor'); /* jshint ignore:line */
 
 Logger.enableTypes([
   'mutation-handler',

src/js/editor/event-manager.js

@@ -7,7 +7,6 @@ import Range from 'mobiledoc-kit/utils/cursor/range';
 import { filter, forEach, contains } from 'mobiledoc-kit/utils/array-utils';
 import Key from 'mobiledoc-kit/utils/key';
 import { TAB } from 'mobiledoc-kit/utils/characters';
-import { DIRECTION } from 'mobiledoc-kit/utils/key';
 
 const ELEMENT_EVENT_TYPES = ['keydown', 'keyup', 'cut', 'copy', 'paste', 'keypress'];
 const DOCUMENT_EVENT_TYPES = ['mouseup'];
@@ -107,27 +106,19 @@ export default class EventManager {
     }
 
     let key = Key.fromEvent(event);
-    let range, nextPosition;
+    let range = editor.range;
 
     switch(true) {
       case key.isHorizontalArrow():
-        range = editor.cursor.offsets;
-        let position = range.tail;
-        if (range.direction === DIRECTION.BACKWARD) {
-          position = range.head;
-        }
         let newRange;
-        nextPosition = position.move(key.direction);
-        if (nextPosition) {
-          if (key.isShift()) {
-            newRange = range.moveFocusedPosition(key.direction);
-          } else {
-            newRange = new Range(nextPosition);
-          }
-
-          editor.selectRange(newRange);
-          event.preventDefault();
+        if (key.isShift()) {
+          newRange = range.extend(key.direction);
+        } else {
+          newRange = range.move(key.direction);
         }
+
+        editor.selectRange(newRange);
+        event.preventDefault();
         break;
       case key.isDelete():
         editor.handleDeletion(event);

src/js/utils/cursor/position.js

@@ -139,12 +139,14 @@ const Position = class Position {
   }
 
   /**
-   * @return {Position|null}
+   * The position to the left of this position.
+   * If this position is the post's headPosition it returns itself.
+   * @return {Position}
    */
   moveLeft() {
     if (this.isHead()) {
       let prev = this.section.previousLeafSection();
-      return prev && prev.tailPosition();
+      return prev ? prev.tailPosition() : this;
     } else {
       let offset = this.offset - 1;
       if (this.isMarkerable && this.marker) {
@@ -158,12 +160,14 @@ const Position = class Position {
   }
 
   /**
-   * @return {Position|null}
+   * The position to the right of this position.
+   * If this position is the post's tailPosition it returns itself.
+   * @return {Position}
    */
   moveRight() {
     if (this.isTail()) {
       let next = this.section.nextLeafSection();
-      return next && next.headPosition();
+      return next ? next.headPosition() : this;
     } else {
       let offset = this.offset + 1;
       if (this.isMarkerable && this.marker) {

src/js/utils/cursor/range.js

@@ -2,16 +2,17 @@ import Position from './position';
 import { DIRECTION } from '../key';
 
 export default class Range {
-  constructor(head, tail=head, direction=DIRECTION.FORWARD) {
+  constructor(head, tail=head, direction=null) {
     this.head = head;
     this.tail = tail;
     this.direction = direction;
   }
 
-  static create(headSection, headOffset, tailSection=headSection, tailOffset=headOffset) {
+  static create(headSection, headOffset, tailSection=headSection, tailOffset=headOffset, direction=null) {
     return new Range(
       new Position(headSection, headOffset),
-      new Position(tailSection, tailOffset)
+      new Position(tailSection, tailOffset),
+      direction
     );
   }
 
@@ -43,17 +44,51 @@ export default class Range {
     return Range.create(section, headOffset, section, tailOffset);
   }
 
-  moveFocusedPosition(direction) {
-    switch (this.direction) {
+  /**
+   * Expands the range in the given direction
+   * @param {Direction} newDirection
+   * @return {Range} Always returns an expanded, non-collapsed range
+   * @public
+   */
+  extend(newDirection) {
+    let { head, tail, direction } = this;
+    switch (direction) {
       case DIRECTION.FORWARD:
-        return new Range(this.head, this.tail.move(direction), this.direction);
+        return new Range(head, tail.move(newDirection), direction);
       case DIRECTION.BACKWARD:
-        return new Range(this.head.move(direction), this.tail, this.direction);
+        return new Range(head.move(newDirection), tail, direction);
       default:
-        return new Range(this.head, this.tail, direction).moveFocusedPosition(direction);
+        return new Range(head, tail, newDirection).extend(newDirection);
+    }
+  }
+
+  /**
+   * Moves this range in {newDirection}.
+   * If the range is collapsed, returns a collapsed range shifted 1 unit in
+   * {newDirection}, otherwise collapses this range to the position at the
+   * {newDirection} end of the range.
+   * @param {Direction} newDirection
+   * @return {Range} Always returns a collapsed range
+   * @public
+   */
+  move(newDirection) {
+    let { focusedPosition, isCollapsed } = this;
+
+    if (isCollapsed) {
+      return new Range(focusedPosition.move(newDirection));
+    } else {
+      return this._collapse(newDirection);
     }
   }
 
+  _collapse(direction) {
+    return new Range(direction === DIRECTION.BACKWARD ? this.head : this.tail);
+  }
+
+  get focusedPosition() {
+    return this.direction === DIRECTION.BACKWARD ? this.head : this.tail;
+  }
+
   isEqual(other) {
     return other &&
       this.head.isEqual(other.head) &&

tests/helpers/assertions.js

@@ -208,4 +208,47 @@ export default function registerAssertions() {
     }
   };
 
+  QUnit.assert.rangeIsEqual = function(range, expected,
+                                          message=`range is equal`) {
+    let { head, tail, isCollapsed, direction } = range;
+    let {
+      head: expectedHead,
+      tail: expectedTail,
+      isCollapsed: expectedIsCollapsed,
+      direction: expectedDirection
+    } = expected;
+
+    let failed = false;
+
+    if (!head.isEqual(expectedHead)) {
+      failed = true;
+      this.push(false,
+                `${head.section.type}:${head.section.tagName}`,
+                `${expectedHead.section.type}:${expectedHead.section.tagName}`,
+                'incorrect head position');
+    }
+
+    if (!tail.isEqual(expectedTail)) {
+      failed = true;
+      this.push(false,
+                `${tail.section.type}:${tail.section.tagName}`,
+                `${expectedTail.section.type}:${expectedTail.section.tagName}`,
+                'incorrect tail position');
+    }
+
+    if (isCollapsed !== expectedIsCollapsed) {
+      failed = true;
+      this.push(false, isCollapsed, expectedIsCollapsed, 'wrong value for isCollapsed');
+    }
+
+    if (direction !== expectedDirection) {
+      failed = true;
+      this.push(false, direction, expectedDirection, 'wrong value for direction');
+    }
+
+    if (!failed) {
+      this.push(true, range, expected, message);
+    }
+  };
+
 }

tests/unit/utils/cursor-position-test.js

@@ -157,6 +157,20 @@ test('#move across and beyond card section into list section', (assert) => {
   assert.positionIsEqual(midTail.moveRight(), cHead, 'right to next section');
 });
 
+test('#move left at headPosition or right at tailPosition returns self', (assert) => {
+  let post = Helpers.postAbstract.build(({post, markupSection, marker}) => {
+    return post([
+      markupSection('p', [marker('abc')]),
+      markupSection('p', [marker('def')])
+    ]);
+  });
+
+  let head = post.headPosition(),
+      tail = post.tailPosition();
+  assert.positionIsEqual(head.moveLeft(), head, 'head move left is head');
+  assert.positionIsEqual(tail.moveRight(), tail, 'tail move right is tail');
+});
+
 test('#fromNode when node is marker text node', (assert) => {
   editor = Helpers.mobiledoc.renderInto(editorElement,
     ({post, markupSection, marker}) => {

tests/unit/utils/cursor-range-test.js

@@ -1,6 +1,8 @@
 import Helpers from '../../test-helpers';
 import Range from 'mobiledoc-kit/utils/cursor/range';
+import { DIRECTION } from 'mobiledoc-kit/utils/key';
 
+const { FORWARD, BACKWARD } = DIRECTION;
 const {module, test} = Helpers;
 
 module('Unit: Utils: Range');
@@ -65,3 +67,60 @@ test('#trimTo middle section', (assert) => {
   assert.equal(newRange.head.offset, 0, 'head offset');
   assert.equal(newRange.tail.offset, section2.text.length, 'tail offset');
 });
+
+test('#move moves collapsed range 1 character in direction', (assert) => {
+  let section = Helpers.postAbstract.build(({markupSection, marker}) => {
+    return markupSection('p', [marker('abc')]);
+  });
+  let range = Range.create(section, 0);
+  let nextRange = Range.create(section, 1);
+
+  assert.ok(range.isCollapsed, 'precond - range.isCollapsed');
+  assert.rangeIsEqual(range.move(DIRECTION.FORWARD), nextRange, 'move forward');
+
+  assert.rangeIsEqual(nextRange.move(DIRECTION.BACKWARD), range, 'move backward');
+});
+
+test('#move collapses non-collapsd range in direction', (assert) => {
+  let section = Helpers.postAbstract.build(({markupSection, marker}) => {
+    return markupSection('p', [marker('abcd')]);
+  });
+  let range = Range.create(section, 1, section, 3);
+  let collapseForward = Range.create(section, 3);
+  let collapseBackward = Range.create(section, 1);
+
+  assert.ok(!range.isCollapsed, 'precond - !range.isCollapsed');
+  assert.rangeIsEqual(range.move(FORWARD), collapseForward,
+                      'collapse forward');
+  assert.rangeIsEqual(range.move(BACKWARD), collapseBackward,
+                      'collapse forward');
+});
+
+test('#extend expands range in direction', (assert) => {
+  let section = Helpers.postAbstract.build(({markupSection, marker}) => {
+    return markupSection('p', [marker('abcd')]);
+  });
+  let collapsedRange = Range.create(section, 1);
+  let collapsedRangeForward = Range.create(section, 1, section, 2, FORWARD);
+  let collapsedRangeBackward = Range.create(section, 0, section, 1, BACKWARD);
+
+  let nonCollapsedRange = Range.create(section, 1, section, 2);
+  let nonCollapsedRangeForward = Range.create(section, 1, section, 3, FORWARD);
+  let nonCollapsedRangeBackward = Range.create(section, 0, section, 2, BACKWARD);
+
+  assert.ok(collapsedRange.isCollapsed, 'precond - collapsedRange.isCollapsed');
+  assert.rangeIsEqual(collapsedRange.extend(FORWARD),
+                      collapsedRangeForward,
+                      'collapsedRange extend forward');
+  assert.rangeIsEqual(collapsedRange.extend(BACKWARD),
+                      collapsedRangeBackward,
+                      'collapsedRange extend backward');
+
+  assert.ok(!nonCollapsedRange.isCollapsed, 'precond -nonCollapsedRange.isCollapsed');
+  assert.rangeIsEqual(nonCollapsedRange.extend(FORWARD),
+                      nonCollapsedRangeForward,
+                      'nonCollapsedRange extend forward');
+  assert.rangeIsEqual(nonCollapsedRange.extend(BACKWARD),
+                      nonCollapsedRangeBackward,
+                      'nonCollapsedRange extend backward');
+});