Bug 1644511 - Part 2: Make selection collapsing follow bidi.edit.caret_movement_style r=jfkthame

Differential Revision: https://phabricator.services.mozilla.com/D79005

layout/generic/nsFrameSelection.cpp

@@ -680,6 +680,16 @@ static nsINode* GetClosestInclusiveTableCellAncestor(nsINode* aDomNode) {
   return nullptr;
 }
 
+static nsDirection GetCaretDirection(const nsIFrame& aFrame,
+                                     nsDirection aDirection,
+                                     bool aVisualMovement) {
+  const nsBidiDirection paragraphDirection =
+      nsBidiPresUtils::ParagraphDirection(&aFrame);
+  return (aVisualMovement && paragraphDirection == NSBIDI_RTL)
+             ? nsDirection(1 - aDirection)
+             : aDirection;
+}
+
 nsresult nsFrameSelection::MoveCaret(nsDirection aDirection,
                                      bool aContinueSelection,
                                      const nsSelectionAmount aAmount,
@@ -749,12 +759,37 @@ nsresult nsFrameSelection::MoveCaret(nsDirection aDirection,
     mDesiredCaretPos.Set(desiredPos);
   }
 
+  bool visualMovement =
+      mCaret.IsVisualMovement(aContinueSelection, aMovementStyle);
+  nsIFrame* frame;
+  int32_t offsetused = 0;
+  nsresult rv =
+      sel->GetPrimaryFrameForFocusNode(&frame, &offsetused, visualMovement);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  if (!frame) {
+    return NS_ERROR_FAILURE;
+  }
+
+  Result<bool, nsresult> isIntraLineCaretMove = IsIntraLineCaretMove(aAmount);
+  nsDirection direction{aDirection};
+  if (isIntraLineCaretMove.isErr()) {
+    return isIntraLineCaretMove.unwrapErr();
+  }
+  if (isIntraLineCaretMove.inspect()) {
+    // Forget old caret position for moving caret to different line since
+    // caret position may be changed.
+    mDesiredCaretPos.Invalidate();
+    direction = GetCaretDirection(*frame, aDirection, visualMovement);
+  }
+
   if (doCollapse) {
     const nsRange* anchorFocusRange = sel->GetAnchorFocusRange();
     if (anchorFocusRange) {
       nsINode* node;
       int32_t offset;
-      if (aDirection == eDirPrevious) {
+      if (direction == eDirPrevious) {
         node = anchorFocusRange->GetStartContainer();
         offset = anchorFocusRange->StartOffset();
       } else {
@@ -768,31 +803,11 @@ nsresult nsFrameSelection::MoveCaret(nsDirection aDirection,
     return NS_OK;
   }
 
-  const bool visualMovement =
-      mCaret.IsVisualMovement(aContinueSelection, aMovementStyle);
-  nsIFrame* frame;
-  int32_t offsetused = 0;
-  nsresult rv =
-      sel->GetPrimaryFrameForFocusNode(&frame, &offsetused, visualMovement);
-  if (NS_FAILED(rv) || !frame) {
-    return NS_FAILED(rv) ? rv : NS_ERROR_FAILURE;
-  }
-
   CaretAssociateHint tHint(mCaret.mHint);  // temporary variable so we dont set
                                            // mCaret.mHint until it is necessary
 
-  Result<bool, nsresult> isIntraLineCaretMove = IsIntraLineCaretMove(aAmount);
-  if (isIntraLineCaretMove.isErr()) {
-    return NS_ERROR_FAILURE;
-  }
-  if (isIntraLineCaretMove.inspect()) {
-    // Forget old caret position for moving caret to different line since
-    // caret position may be changed.
-    mDesiredCaretPos.Invalidate();
-  }
-
   Result<nsPeekOffsetStruct, nsresult> result = PeekOffsetForCaretMove(
-      aDirection, aContinueSelection, aAmount, aMovementStyle, desiredPos);
+      direction, aContinueSelection, aAmount, aMovementStyle, desiredPos);
   if (result.isOk() && result.inspect().mResultContent) {
     const nsPeekOffsetStruct& pos = result.inspect();
     nsIFrame* theFrame;
@@ -858,7 +873,7 @@ nsresult nsFrameSelection::MoveCaret(nsDirection aDirection,
                                     : FocusMode::kCollapseToNewPoint;
     rv = TakeFocus(MOZ_KnownLive(pos.mResultContent), pos.mContentOffset,
                    pos.mContentOffset, tHint, focusMode);
-  } else if (aAmount <= eSelectWordNoSpace && aDirection == eDirNext &&
+  } else if (aAmount <= eSelectWordNoSpace && direction == eDirNext &&
              !aContinueSelection) {
     // Collapse selection if PeekOffset failed, we either
     //  1. bumped into the BRFrame, bug 207623
@@ -907,27 +922,12 @@ Result<nsPeekOffsetStruct, nsresult> nsFrameSelection::PeekOffsetForCaretMove(
       selection->IsEditorSelection()
           ? nsPeekOffsetStruct::ForceEditableRegion::Yes
           : nsPeekOffsetStruct::ForceEditableRegion::No;
-  const nsBidiDirection kParagraphDirection =
-      nsBidiPresUtils::ParagraphDirection(frame);
-
-  nsDirection direction{aDirection};
-  Result<bool, nsresult> isIntraLineCareMove = IsIntraLineCaretMove(aAmount);
-  if (isIntraLineCareMove.isErr()) {
-    return Err(NS_ERROR_FAILURE);
-  }
-  if (isIntraLineCareMove.inspect()) {
-    // If caret is moving in same line and user expects visual movement,
-    // we revert the direction if direction of current paragraph is RTL.
-    direction = (visualMovement && kParagraphDirection == NSBIDI_RTL)
-                    ? nsDirection(1 - aDirection)
-                    : aDirection;
-  }
 
   // set data using mLimiters.mLimiter to stop on scroll views.  If we have a
   // limiter then we stop peeking when we hit scrollable views.  If no limiter
   // then just let it go ahead
-  nsPeekOffsetStruct pos(aAmount, direction, offsetused, aDesiredCaretPos, true,
-                         !!mLimiters.mLimiter, true, visualMovement,
+  nsPeekOffsetStruct pos(aAmount, aDirection, offsetused, aDesiredCaretPos,
+                         true, !!mLimiters.mLimiter, true, visualMovement,
                          aContinueSelection, kForceEditableRegion);
   if (NS_FAILED(rv = frame->PeekOffset(&pos))) {
     return Err(rv);

layout/generic/test/mochitest.ini

@@ -102,6 +102,7 @@ support-files = file_bug1307853.html
 [test_bug1566783.html]
 support-files = file_bug1566783.html
 [test_bug1642588.html]
+[test_bug1644511.html]
 [test_contained_plugin_transplant.html]
 skip-if = os=='win'
 [test_crash_on_mouse_move.html]

layout/generic/test/test_bug1644511.html

@@ -0,0 +1,86 @@
+<!DOCTYPE html>
+<title>Test for Bug 1644511</title>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<link rel="stylesheet" href="/tests/SimpleTest/test.css">
+<style>
+  [contenteditable] {
+    padding: .5em 40%;
+  }
+</style>
+
+<div id="host" contenteditable="" dir="rtl">مرحبا عالم!</div>
+
+<script>
+const caretMovementStyleFlag = "bidi.edit.caret_movement_style";
+
+/**
+ * Can't use synthesizeKey("KEY_Arrow*") as it triggers
+ * nsFrameSelection::PhysicalMove() instead of CharacterMove() and thus
+ * suppresses the flag. See also Bug 1644489.
+ */
+function moveCaret(aRight, aSelect) {
+  const dir = aRight ? "Next" : "Previous";
+  const select = aSelect ? "selectChar" : "char";
+  SpecialPowers.doCommand(window, `cmd_${select}${dir}`);
+}
+
+async function testLogicalMovement() {
+  await SpecialPowers.pushPrefEnv({
+    set: [[caretMovementStyleFlag, 0]]
+  });
+  getSelection().collapse(host);
+  moveCaret(true, true);
+  is(getSelection().anchorOffset, 0, "Shift+ArrowRight should select from right");
+  is(getSelection().focusOffset, 1, "Shift+ArrowRight should select to left");
+  moveCaret(true, false);
+  is(getSelection().anchorOffset, 1, "Collapsing by ArrowRight should put the caret to the left side");
+
+  getSelection().collapse(host);
+  moveCaret(true, true);
+  moveCaret(false, false);
+  is(getSelection().anchorOffset, 0, "Collapsing by ArrowLeft should put the caret to the right side");
+}
+
+async function testVisualMovement() {
+  await SpecialPowers.pushPrefEnv({
+    set: [[caretMovementStyleFlag, 1]]
+  });
+  getSelection().collapse(host);
+  moveCaret(false, true);
+  is(getSelection().anchorOffset, 0, "Shift+ArrowLeft should select from right");
+  is(getSelection().focusOffset, 1, "Shift+ArrowLeft should select to left");
+  moveCaret(false, false);
+  is(getSelection().anchorOffset, 1, "Collapsing by ArrowLeft should put the caret to the left side");
+
+  getSelection().collapse(host);
+  moveCaret(false, true);
+  moveCaret(true, false);
+  is(getSelection().anchorOffset, 0, "Collapsing by ArrowRight should put the caret to the right side");
+}
+
+async function testHybridMovement() {
+  await SpecialPowers.pushPrefEnv({
+    set: [[caretMovementStyleFlag, 2]]
+  });
+  getSelection().collapse(host);
+  moveCaret(true, true);
+  is(getSelection().anchorOffset, 0, "Shift+ArrowRight should select from right");
+  is(getSelection().focusOffset, 1, "Shift+ArrowRight should select to left");
+  moveCaret(false, false);
+  is(getSelection().anchorOffset, 1, "Collapsing by ArrowLeft should put the caret to the left side");
+
+  getSelection().collapse(host);
+  moveCaret(true, true);
+  moveCaret(true, false);
+  is(getSelection().anchorOffset, 0, "Collapsing by ArrowRight should put the caret to the right side");
+}
+
+SimpleTest.waitForExplicitFinish();
+SimpleTest.promiseFocus().then(async () => {
+  await testLogicalMovement();
+  await testVisualMovement();
+  await testHybridMovement();
+  SimpleTest.finish();
+});
+</script>