Bug 1123606 - Fix RTL Logic error affecting Handle Movement in TextAreas, r=margaret

mobile/android/base/tests/roboextender/testInputSelections.html

@@ -262,10 +262,12 @@ function testLTR_dragAnchorHandleToSelf() {
   var initialSelectionText = sh._getSelectedText();
 
   // Drag anchor handle and note results.
+  // Note, due to edge case with border boundaries, we actually
+  // move inside a pixel, to maintain Selection position.
   sh.observe(null, "TextSelection:Move",
     JSON.stringify({ handleType : ANCHOR,
       x : initialSelection.anchorPt.x,
-      y : initialSelection.anchorPt.y
+      y : initialSelection.anchorPt.y - 1
     })
   );
   sh.observe(null, "TextSelection:Position",
@@ -287,14 +289,11 @@ function testLTR_dragAnchorHandleToSelf() {
     selectionExists(initialSelection,
       "LTR Selection initially existed at points"),
 
-    todo(false, "testLTR_dragAnchorHandleToSelf: " +
+    is(anchorDragSelectionText, LTR_INPUT_TEXT_VALUE,
-    // is(anchorDragSelectionText, LTR_INPUT_TEXT_VALUE,
       "LTR Selection text after anchor drag should match expected value."),
-    todo(false, "testLTR_dragAnchorHandleToSelf: " +
+    selectionExists(anchorDraggedSelection,
-    // selectionExists(anchorDraggedSelection,
       "LTR Selection after anchor drag existed at points"),
-    todo(false, "testLTR_dragAnchorHandleToSelf: " +
+    selectionEquals(anchorDraggedSelection, initialSelection,
-    // selectionEquals(anchorDraggedSelection, initialSelection,
       "LTR Selection points after anchor drag " +
       "should match initial selection points."),
 
@@ -348,14 +347,11 @@ function testRTL_dragFocusHandleToSelf() {
     selectionExists(initialSelection,
       "RTL Selection initially existed at points"),
 
-    todo(false, "testRTL_dragAnchorHandleToSelf: " +
+    is(focusDragSelectionText, RTL_INPUT_TEXT_VALUE,
-    // is(focusDragSelectionText, RTL_INPUT_TEXT_VALUE,
       "RTL Selection text after focus drag should match expected value."),
-    todo(false, "testRTL_dragAnchorHandleToSelf: " +
+    selectionExists(focusDraggedSelection,
-    // selectionExists(focusDraggedSelection,
       "RTL Selection after focus drag existed at points"),
-    todo(false, "testRTL_dragAnchorHandleToSelf: " +
+    selectionEquals(focusDraggedSelection, initialSelection,
-    // selectionEquals(focusDraggedSelection, initialSelection,
       "RTL Selection points after focus drag " +
       "should match initial selection points."),
 
@@ -383,10 +379,12 @@ function testRTL_dragAnchorHandleToSelf() {
   var initialSelectionText = sh._getSelectedText();
 
   // Drag anchor handle and note results.
+  // Note, due to edge case with border boundaries, we actually
+  // move inside a pixel, to maintain Selection position.
   sh.observe(null, "TextSelection:Move",
     JSON.stringify({ handleType : ANCHOR,
       x : initialSelection.anchorPt.x,
-      y : initialSelection.anchorPt.y
+      y : initialSelection.anchorPt.y - 1
     })
   );
   sh.observe(null, "TextSelection:Position",
@@ -493,10 +491,15 @@ function greaterThan(n1, n2, msg) {
   });
 }
 
+// Use fuzzy logic to compare screen coords.
+function truncPoint(point) {
+  return new Point(Math.trunc(point.x), Math.trunc(point.y));
+}
+
 function pointEquals(p1, p2, msg) {
   return Messaging.sendRequestForResult({
     type: "Robocop:testInputSelections",
-    result: p1.equals(p2),
+    result: truncPoint(p1).equals(truncPoint(p2)),
     msg: msg + " : " + p1.toString() + " == " + p2.toString()
   });
 }
@@ -504,7 +507,7 @@ function pointEquals(p1, p2, msg) {
 function pointNotEquals(p1, p2, msg) {
   return Messaging.sendRequestForResult({
     type: "Robocop:testInputSelections",
-    result: !p1.equals(p2),
+    result: !truncPoint(p1).equals(truncPoint(p2)),
     msg: msg + " : " + p1.toString() + " == " + p2.toString()
   });
 }
@@ -512,7 +515,7 @@ function pointNotEquals(p1, p2, msg) {
 function selectionExists(selection, msg) {
   return Messaging.sendRequestForResult({
     type: "Robocop:testInputSelections",
-    result: !selection.anchorPt.equals(selection.focusPt),
+    result: !truncPoint(selection.anchorPt).equals(truncPoint(selection.focusPt)),
     msg: msg + " : anchor:" + selection.anchorPt.toString() +
       " focus:" + selection.focusPt.toString()
   });
@@ -521,7 +524,8 @@ function selectionExists(selection, msg) {
 function selectionEquals(s1, s2, msg) {
   return Messaging.sendRequestForResult({
     type: "Robocop:testInputSelections",
-    result: s1.anchorPt.equals(s2.anchorPt) && s1.focusPt.equals(s2.focusPt),
+    result: truncPoint(s1.anchorPt).equals(truncPoint(s2.anchorPt)) &&
+            truncPoint(s1.focusPt).equals(truncPoint(s2.focusPt)),
     msg: msg
   });
 }

mobile/android/base/tests/roboextender/testTextareaSelections.html

@@ -518,17 +518,14 @@ function testRTL_moveFocusHandleDown() {
       "RTL Initial selection anchorPt.x should be greater than (right of) focusPt.x"),
 
     selectionExists(changedSelection, "RTL Changed selection existed at points"),
-    todo(false, "testRTL_moveFocusHandleDown: " +
+    pointEquals(changedSelection.anchorPt, initialSelection.anchorPt,
-    // pointEquals(changedSelection.anchorPt, initialSelection.anchorPt,
       "RTL Changed selection focus handle moving down " +
       "should not change anchor handle position."),
-    todo(false, "testRTL_moveFocusHandleDown: " +
+    greaterThan(changedSelection.focusPt.y, changedSelection.anchorPt.y,
-    // greaterThan(changedSelection.focusPt.y, changedSelection.anchorPt.y,
       "RTL Changed selection focusPt.y " +
       "should be greater than (below) changed anchorPt.y"),
 
-    todo(false, "testRTL_moveFocusHandleDown: " +
+    greaterThan(changedSelection.focusPt.y, initialSelection.focusPt.y,
-    // greaterThan(changedSelection.focusPt.y, initialSelection.focusPt.y,
       "RTL Changed selection focusPt.y " +
       "should be greater than (below) Initial selection focusPt.y"),
   ]);
@@ -585,22 +582,18 @@ function testRTL_moveFocusHandleUp() {
       "RTL Initial selection anchorPt.x should be greater than (right of) focusPt.x"),
 
     selectionExists(changedSelection, "RTL Changed selection existed at points"),
-    todo(false, "testRTL_moveFocusHandleUp: " +
+    pointEquals(changedSelection.focusPt, initialSelection.anchorPt,
-    // pointEquals(changedSelection.focusPt, initialSelection.anchorPt,
       "RTL Reversed Changed selection focus handle moving up " +
       "becomes new anchor handle, " +
       "new focus handle is initial anchor handle."),
-    todo(false, "testRTL_moveFocusHandleUp: " +
+    greaterThan(changedSelection.focusPt.y, changedSelection.anchorPt.y,
-    // greaterThan(changedSelection.focusPt.y, changedSelection.anchorPt.y,
       "RTL Reversed Changed selection focusPt.y " +
       "should be greater than (below) changed anchorPt.y"),
 
-    todo(false, "testRTL_moveFocusHandleUp: " +
+    is(changedSelection.focusPt.y, initialSelection.focusPt.y,
-    // is(changedSelection.focusPt.y, initialSelection.focusPt.y,
       "RTL Reversed Changed selection focusPt.y " +
       "should be equal to Initial selection focusPt.y"),
-    todo(false, "testRTL_moveFocusHandleUp: " +
+    lessThan(changedSelection.anchorPt.y, initialSelection.anchorPt.y,
-    // lessThan(changedSelection.anchorPt.y, initialSelection.anchorPt.y,
       "RTL Reversed Changed selection anchorPt.y " +
       "should be less than (above) Initial selection anchorPt.y"),
   ]);
@@ -657,17 +650,14 @@ function testRTL_moveAnchorHandleUp() {
       "RTL Initial selection anchorPt.x should be greater than (right of) focusPt.x"),
 
     selectionExists(changedSelection, "RTL Changed selection existed at points"),
-    todo(false, "testRTL_moveAnchorHandleUp: " +
+    pointEquals(changedSelection.focusPt, initialSelection.focusPt,
-    // pointEquals(changedSelection.focusPt, initialSelection.focusPt,
       "RTL Changed selection anchor handle moving up " +
       "should not change focus handle position."),
-    todo(false, "testRTL_moveAnchorHandleUp: " +
+    greaterThan(changedSelection.focusPt.y, changedSelection.anchorPt.y,
-    // greaterThan(changedSelection.focusPt.y, changedSelection.anchorPt.y,
       "RTL Changed selection focusPt.y " +
       "should be greater than (below) changed anchorPt.y"),
 
-    todo(false, "testRTL_moveAnchorHandleUp: " +
+    lessThan(changedSelection.anchorPt.y, initialSelection.anchorPt.y,
-    // lessThan(changedSelection.anchorPt.y, initialSelection.anchorPt.y,
       "RTL Changed selection anchorPt.y " +
       "should be less than (above) Initial selection anchorPt.y"),
   ]);
@@ -724,22 +714,18 @@ function testRTL_moveAnchorHandleDown() {
       "RTL Initial selection anchorPt.x should be greater than (right of) focusPt.x"),
 
     selectionExists(changedSelection, "RTL Changed selection existed at points"),
-    todo(false, "testRTL_moveAnchorHandleDown: " +
+    pointEquals(changedSelection.anchorPt, initialSelection.focusPt,
-    // pointEquals(changedSelection.anchorPt, initialSelection.focusPt,
       "RTL Reversed Changed selection anchor handle moving down " +
       "becomes new focus handle, " +
       "new anchor handle is initial focus handle."),
-    todo(false, "testRTL_moveAnchorHandleDown: " +
+    greaterThan(changedSelection.focusPt.y, changedSelection.anchorPt.y,
-    // greaterThan(changedSelection.focusPt.y, changedSelection.anchorPt.y,
       "RTL Reversed Changed selection focusPt.y " +
       "should be greater than (below) changed anchorPt.y"),
 
-    todo(false, "testRTL_moveAnchorHandleDown: " +
+    is(changedSelection.anchorPt.y, initialSelection.anchorPt.y,
-    // is(changedSelection.anchorPt.y, initialSelection.anchorPt.y,
       "RTL Reversed Changed selection anchorPt.y " +
       "should be equal to Initial selection anchorPt.y"),
-    todo(false, "testRTL_moveAnchorHandleDown: " +
+    greaterThan(changedSelection.focusPt.y, initialSelection.focusPt.y,
-    // greaterThan(changedSelection.focusPt.y, initialSelection.focusPt.y,
       "RTL Reversed Changed selection focusPt.y " +
       "should be greater than (below) Initial selection focusPt.y"),
   ]);
@@ -810,10 +796,15 @@ function greaterThan(n1, n2, msg) {
   });
 }
 
+// Use fuzzy logic to compare screen coords.
+function truncPoint(point) {
+  return new Point(Math.trunc(point.x), Math.trunc(point.y));
+}
+
 function pointEquals(p1, p2, msg) {
   return Messaging.sendRequestForResult({
     type: "Robocop:testTextareaSelections",
-    result: p1.equals(p2),
+    result: truncPoint(p1).equals(truncPoint(p2)),
     msg: msg + " : " + p1.toString() + " == " + p2.toString()
   });
 }
@@ -821,7 +812,7 @@ function pointEquals(p1, p2, msg) {
 function pointNotEquals(p1, p2, msg) {
   return Messaging.sendRequestForResult({
     type: "Robocop:testTextareaSelections",
-    result: !p1.equals(p2),
+    result: !truncPoint(p1).equals(truncPoint(p2)),
     msg: msg + " : " + p1.toString() + " == " + p2.toString()
   });
 }
@@ -829,7 +820,7 @@ function pointNotEquals(p1, p2, msg) {
 function selectionExists(selection, msg) {
   return Messaging.sendRequestForResult({
     type: "Robocop:testTextareaSelections",
-    result: !selection.anchorPt.equals(selection.focusPt),
+    result: !truncPoint(selection.anchorPt).equals(truncPoint(selection.focusPt)),
     msg: msg + " : anchor:" + selection.anchorPt.toString() +
       " focus:" + selection.focusPt.toString()
   });

mobile/android/chrome/content/SelectionHandler.js

@@ -40,7 +40,11 @@ var SelectionHandler = {
   // stored here are relative to the _contentWindow window.
   _cache: null,
   _activeType: 0, // TYPE_NONE
+
   _draggingHandles: false, // True while user drags text selection handles
+  _dragStartAnchorOffset: null, // Editables need initial pos during HandleMove events
+  _dragStartFocusOffset: null, // Editables need initial pos during HandleMove events
+
   _ignoreCompositionChanges: false, // Persist caret during IME composition updates
   _prevHandlePositions: [], // Avoid issuing duplicate "TextSelection:Position" messages
   _deferCloseTimer: null, // Used to defer _closeSelection() actions during programmatic changes
@@ -152,11 +156,12 @@ var SelectionHandler = {
         }
         break;
       }
+
       case "TextSelection:Move": {
         let data = JSON.parse(aData);
         if (this._activeType == this.TYPE_SELECTION) {
           this._startDraggingHandles();
-          this._moveSelection(data.handleType == this.HANDLE_TYPE_ANCHOR, data.x, data.y);
+          this._moveSelection(data.handleType, new Point(data.x, data.y));
 
         } else if (this._activeType == this.TYPE_CURSOR) {
           this._startDraggingHandles();
@@ -170,30 +175,16 @@ var SelectionHandler = {
         }
         break;
       }
+
       case "TextSelection:Position": {
         if (this._activeType == this.TYPE_SELECTION) {
           this._startDraggingHandles();
-
+          this._ensureSelectionDirection();
-          // Check to see if the handles should be reversed.
-          let isStartHandle = JSON.parse(aData).handleType == this.HANDLE_TYPE_ANCHOR;
-          try {
-            let selectionReversed = this._updateCacheForSelection(isStartHandle);
-            if (selectionReversed) {
-              // Reverse the anchor and focus to correspond to the new start and end handles.
-              let selection = this._getSelection();
-              let anchorNode = selection.anchorNode;
-              let anchorOffset = selection.anchorOffset;
-              selection.collapse(selection.focusNode, selection.focusOffset);
-              selection.extend(anchorNode, anchorOffset);
-            }
-          } catch (e) {
-            // User finished handle positioning with one end off the screen
-            this._closeSelection();
-            break;
-          }
-
           this._stopDraggingHandles();
+
+          this._updateCacheForSelection();
           this._positionHandles();
+
           // Changes to handle position can affect selection context and actionbar display
           this._updateMenu();
 
@@ -225,6 +216,9 @@ var SelectionHandler = {
   _startDraggingHandles: function sh_startDraggingHandles() {
     if (!this._draggingHandles) {
       this._draggingHandles = true;
+      let selection = this._getSelection();
+      this._dragStartAnchorOffset = selection.anchorOffset;
+      this._dragStartFocusOffset = selection.focusOffset;
       Messaging.sendRequest({ type: "TextSelection:DraggingHandle", dragging: true });
     }
   },
@@ -234,6 +228,8 @@ var SelectionHandler = {
   _stopDraggingHandles: function sh_stopDraggingHandles() {
     if (this._draggingHandles) {
       this._draggingHandles = false;
+      this._dragStartAnchorOffset = null;
+      this._dragStartFocusOffset = null;
       Messaging.sendRequest({ type: "TextSelection:DraggingHandle", dragging: false });
     }
   },
@@ -862,35 +858,21 @@ var SelectionHandler = {
   },
 
   /*
-   * Helper function for moving the selection inside an editable element.
+   * Moves the selection as the user drags a handle.
-   *
+   * @param handleType: Specifies either the anchor or the focus handle.
-   * @param aAnchorX the stationary handle's x-coordinate in client coordinates
+   * @param handlePt: selection point in client coordinates.
-   * @param aX the moved handle's x-coordinate in client coordinates
-   * @param aCaretPos the current position of the caret
    */
-  _moveSelectionInEditable: function sh_moveSelectionInEditable(aAnchorX, aX, aCaretPos) {
+  _moveSelection: function sh_moveSelection(handleType, handlePt) {
-    let anchorOffset = aX < aAnchorX ? this._targetElement.selectionEnd
+    let isAnchorHandle = (handleType == this.HANDLE_TYPE_ANCHOR);
-                                     : this._targetElement.selectionStart;
-    let newOffset = aCaretPos.offset;
-    let [start, end] = anchorOffset <= newOffset ?
-                       [anchorOffset, newOffset] :
-                       [newOffset, anchorOffset];
-    this._targetElement.setSelectionRange(start, end);
-  },
 
-  /*
+    // Determine new caret position from handlePt, exit if user
-   * Moves the selection as the user drags a selection handle.
+    // moved it offscreen.
-   *
-   * @param aIsStartHandle whether the user is moving the start handle (as opposed to the end handle)
-   * @param aX, aY selection point in client coordinates
-   */
-  _moveSelection: function sh_moveSelection(aIsStartHandle, aX, aY) {
-    // XXX We should be smarter about the coordinates we pass to caretPositionFromPoint, especially
-    // in editable targets. We should factor out the logic that's currently in _moveCaret.
     let viewOffset = this._getViewOffset();
-    let caretPos = this._contentWindow.document.caretPositionFromPoint(aX - viewOffset.x, aY - viewOffset.y);
+    let ptX = handlePt.x - viewOffset.x;
+    let ptY = handlePt.y - viewOffset.y;
+    let cwd = this._contentWindow.document;
+    let caretPos = cwd.caretPositionFromPoint(ptX, ptY);
     if (!caretPos) {
-      // User moves handle offscreen while positioning
       return;
     }
 
@@ -900,37 +882,37 @@ var SelectionHandler = {
       return;
     }
 
-    // Update the cache as the handle is dragged (keep the cache in client coordinates).
+    // Update the Selection for editable elements. Selection Change
-    if (aIsStartHandle) {
+    // logic is the same, regardless of RTL/LTR. Selection direction is
-      this._cache.anchorPt.x = aX;
+    // maintained always forward (startOffset <= endOffset).
-      this._cache.anchorPt.y = aY;
+    if (targetIsEditable) {
+      let start = this._dragStartAnchorOffset;
+      let end = this._dragStartFocusOffset;
+      if (isAnchorHandle) {
+        start = caretPos.offset;
       } else {
-      this._cache.focusPt.x = aX;
+        end = caretPos.offset;
-      this._cache.focusPt.y = aY;
+      }
+      if (start > end) {
+        [start, end] = [end, start];
+      }
+      this._targetElement.setSelectionRange(start, end);
+      return;
     }
 
+    // Update the Selection for non-editable elements. Selection Change
+    // logic is the same, regardless of RTL/LTR. Selection direction internally
+    // can finish reversed by user drag. ie: Forward is (a,o ---> f,o),
+    // and reversed is (a,o <--- f,o).
     let selection = this._getSelection();
-
+    if (isAnchorHandle) {
-    // The handles work the same on both LTR and RTL pages, but the anchor/focus nodes
-    // are reversed, so we need to reverse the logic to extend the selection.
-    if ((aIsStartHandle && !this._isRTL) || (!aIsStartHandle && this._isRTL)) {
-      if (targetIsEditable) {
-        let anchorX = this._isRTL ? this._cache.anchorPt.x : this._cache.focusPt.x;
-        this._moveSelectionInEditable(anchorX, aX, caretPos);
-      } else {
       let focusNode = selection.focusNode;
       let focusOffset = selection.focusOffset;
       selection.collapse(caretPos.offsetNode, caretPos.offset);
       selection.extend(focusNode, focusOffset);
-      }
-    } else {
-      if (targetIsEditable) {
-        let anchorX = this._isRTL ? this._cache.focusPt.x : this._cache.anchorPt.x;
-        this._moveSelectionInEditable(anchorX, aX, caretPos);
     } else {
       selection.extend(caretPos.offsetNode, caretPos.offset);
     }
-    }
   },
 
   _moveCaret: function sh_moveCaret(aX, aY) {
@@ -1149,29 +1131,56 @@ var SelectionHandler = {
     return offset;
   },
 
-  // Returns true if the selection has been reversed. Takes optional aIsStartHandle
+  /*
-  // param to decide whether the selection has been reversed.
+   * The direction of the Selection is ensured for editables while the user drags
-  _updateCacheForSelection: function sh_updateCacheForSelection(aIsStartHandle) {
+   * the handles (per "TextSelection:Move" event). For non-editables, we just let
+   * the user change direction, but fix it up at the end of handle movement (final
+   * "TextSelection:Position" event).
+   */
+  _ensureSelectionDirection: function() {
+    // Never needed at this time.
+    if (this._targetElement instanceof Ci.nsIDOMNSEditableElement) {
+      return;
+    }
+
+    // Nothing needed if not reversed.
+    let qcEventResult = this._domWinUtils.sendQueryContentEvent(
+      this._domWinUtils.QUERY_SELECTED_TEXT, 0, 0, 0, 0);
+    if (!qcEventResult.reversed) {
+      return;
+    }
+
+    // Reverse the Selection.
+    let selection = this._getSelection();
+    let newFocusNode = selection.anchorNode;
+    let newFocusOffset = selection.anchorOffset;
+
+    selection.collapse(selection.focusNode, selection.focusOffset);
+    selection.extend(newFocusNode, newFocusOffset);
+  },
+
+  /*
+   * Updates the TYPE_SELECTION cache, with the handle anchor/focus point values
+   * of the current selection. Passed to Java for UI positioning only.
+   */
+  _updateCacheForSelection: function() {
     let rects = this._getSelection().getRangeAt(0).getClientRects();
-    if (!rects[0]) {
+    if (rects.length == 0) {
       // nsISelection object exists, but there's nothing actually selected
       throw "Failed to update cache for invalid selection";
     }
 
-    let start = { x: this._isRTL ? rects[0].right : rects[0].left, y: rects[0].bottom };
+    let anchorIdx = 0;
-    let end = { x: this._isRTL ? rects[rects.length - 1].left : rects[rects.length - 1].right, y: rects[rects.length - 1].bottom };
+    let focusIdx = rects.length - 1;
-
+    if (this._isRTL) {
-    let selectionReversed = false;
+      // Right-to-Left (ie: Hebrew) anchorPt is on right, focusPt is on left.
-    if (this._cache.anchorPt) {
+      this._cache.anchorPt = new Point(rects[anchorIdx].right, rects[anchorIdx].bottom);
-      // If the end moved past the old end, but we're dragging the start handle, then that handle should become the end handle (and vice versa)
+      this._cache.focusPt = new Point(rects[focusIdx].left, rects[focusIdx].bottom);
-      selectionReversed = (aIsStartHandle && (end.y > this._cache.focusPt.y || (end.y == this._cache.focusPt.y && end.x > this._cache.focusPt.x))) ||
+    } else {
-                          (!aIsStartHandle && (start.y < this._cache.anchorPt.y || (start.y == this._cache.anchorPt.y && start.x < this._cache.anchorPt.x)));
+      // Left-to-Right (ie: English) anchorPt is on left, focusPt is on right.
+      this._cache.anchorPt = new Point(rects[anchorIdx].left, rects[anchorIdx].bottom);
+      this._cache.focusPt = new Point(rects[focusIdx].right, rects[focusIdx].bottom);
     }
-
-    this._cache.anchorPt = start;
-    this._cache.focusPt = end;
-
-    return selectionReversed;
   },
 
   _getHandlePositions: function sh_getHandlePositions(scroll) {

mobile/android/chrome/content/browser.js

@@ -298,8 +298,8 @@ XPCOMUtils.defineLazyGetter(this, "ContentAreaUtils", function() {
   return ContentAreaUtils;
 });
 
-XPCOMUtils.defineLazyModuleGetter(this, "Rect",
+XPCOMUtils.defineLazyModuleGetter(this, "Rect", "resource://gre/modules/Geometry.jsm");
-                                  "resource://gre/modules/Geometry.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Point", "resource://gre/modules/Geometry.jsm");
 
 function resolveGeckoURI(aURI) {
   if (!aURI)