Bug 989012 - Part 1: Stop after passing over a non-selectable frame if one is found during the frame traversal; r=roc

The caret movement code already handles unselectable text frames if we happen to land in the middle of one in nsTextFrame::PeekOffsetCharacter/Word. However, when performing frame traversal to find the next frame to jump to, we don't remember if we skipped over an unselectable frame, which causes us to jump one offset too much when the caret is on the boundary of selectable and unselectable content. The test cases demonstrate the scenario. Note that an <img alt=foo> is implemented by adding a generated content to the inline frame representing it, so as far as the caret movement code is concerned, both test cases are treated similarly. Note that we need to do this only when moving the selection, and not when extending it. We are adding an aExtend argument to nsPeekOffsetStruct's constructor in order to be able to special case that.
2015-01-15 11:24:49 -05:00 · 2015-01-15 11:24:49 -05:00 · 084e7e0b3c
--- a/layout/base/SelectionCarets.cpp
+++ b/layout/base/SelectionCarets.cpp
@ -697,6 +697,7 @@ CompareRangeWithContentOffset(nsRange* aRange,
                         true,
                         true,  //limit on scrolled views
                         false,
+                         false,
                         false);
  nsresult rv = theFrame->PeekOffset(&pos);
  if (NS_FAILED(rv)) {
--- a/layout/base/nsCaret.cpp
+++ b/layout/base/nsCaret.cpp
@ -702,7 +702,7 @@ nsCaret::GetCaretFrameForNodeOffset(nsFrameSelection*    aFrameSelection,
                {
                  nsPeekOffsetStruct pos(eSelectBeginLine, eDirPrevious, 0,
                                         nsPoint(0, 0), false, true, false,
-                                         true);
+                                         true, false);
                  if (NS_SUCCEEDED(frameAfter->PeekOffset(&pos))) {
                    theFrame = pos.mResultFrame;
                    theFrameOffset = pos.mContentOffset;
@ -737,7 +737,7 @@ nsCaret::GetCaretFrameForNodeOffset(nsFrameSelection*    aFrameSelection,
                {
                  nsPeekOffsetStruct pos(eSelectEndLine, eDirNext, 0,
                                         nsPoint(0, 0), false, true, false,
-                                         true);
+                                         true, false);
                  if (NS_SUCCEEDED(frameBefore->PeekOffset(&pos))) {
                    theFrame = pos.mResultFrame;
                    theFrameOffset = pos.mContentOffset;
--- a/layout/base/tests/bug989012-1-ref.html
+++ b/layout/base/tests/bug989012-1-ref.html
@ -0,0 +1,21 @@
+<html class="reftest-wait">
+  <head>
+    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
+    <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  </head>
+  <body onload="start()">
+    <div onfocus="done()" contenteditable>foo<img alt="IMAGE">bar</div>
+    <script>
+      var div = document.querySelector("div");
+      function start() {
+        div.focus();
+      }
+      function done() {
+        var sel = getSelection();
+        // Set the caret right before "bar"
+        sel.collapse(div.lastChild, 0);
+        document.documentElement.removeAttribute("class");
+      }
+    </script>
+  </body>
+</html>
--- a/layout/base/tests/bug989012-1.html
+++ b/layout/base/tests/bug989012-1.html
@ -0,0 +1,24 @@
+<html class="reftest-wait">
+  <head>
+    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
+    <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  </head>
+  <body onload="start()">
+    <div onfocus="done()" contenteditable>foo<img alt="IMAGE">bar</div>
+    <script>
+      var div = document.querySelector("div");
+      function start() {
+        div.focus();
+      }
+      function done() {
+        var sel = getSelection();
+        sel.collapse(div, 0);
+        // Press Right four times to set the caret right before "bar"
+        for (var i = 0; i < 4; ++i) {
+          synthesizeKey("VK_RIGHT", {});
+        }
+        document.documentElement.removeAttribute("class");
+      }
+    </script>
+  </body>
+</html>
--- a/layout/base/tests/bug989012-2-ref.html
+++ b/layout/base/tests/bug989012-2-ref.html
@ -0,0 +1,26 @@
+<html class="reftest-wait">
+  <head>
+    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
+    <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+    <style>
+      span:before {
+        content: "IMAGE";
+      }
+    </style>
+  </head>
+  <body onload="start()">
+    <div onfocus="done()" contenteditable>foo<span></span>bar</div>
+    <script>
+      var div = document.querySelector("div");
+      function start() {
+        div.focus();
+      }
+      function done() {
+        var sel = getSelection();
+        // Set the caret right before "bar"
+        sel.collapse(div.lastChild, 0);
+        document.documentElement.removeAttribute("class");
+      }
+    </script>
+  </body>
+</html>
--- a/layout/base/tests/bug989012-2.html
+++ b/layout/base/tests/bug989012-2.html
@ -0,0 +1,29 @@
+<html class="reftest-wait">
+  <head>
+    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
+    <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+    <style>
+      span:before {
+        content: "IMAGE";
+      }
+    </style>
+  </head>
+  <body onload="start()">
+    <div onfocus="done()" contenteditable>foo<span></span>bar</div>
+    <script>
+      var div = document.querySelector("div");
+      function start() {
+        div.focus();
+      }
+      function done() {
+        var sel = getSelection();
+        sel.collapse(div, 0);
+        // Press Right four times to set the caret right before "bar"
+        for (var i = 0; i < 4; ++i) {
+          synthesizeKey("VK_RIGHT", {});
+        }
+        document.documentElement.removeAttribute("class");
+      }
+    </script>
+  </body>
+</html>
--- a/layout/base/tests/mochitest.ini
+++ b/layout/base/tests/mochitest.ini
@ -53,6 +53,10 @@ support-files =
  bug570378-persian-5.html
  bug570378-persian-5-ref.html
  bug644768.html
+  bug989012-1.html
+  bug989012-1-ref.html
+  bug989012-2.html
+  bug989012-2-ref.html
  bug1061468.html
  bug1061468-ref.html
  bug1109968-1-ref.html
--- a/layout/base/tests/test_reftests_with_caret.html
+++ b/layout/base/tests/test_reftests_with_caret.html
@ -107,6 +107,8 @@ var tests = [
    [ 'bug106855-2.html' , 'bug106855-1-ref.html' ] ,
    [ 'bug389321-2.html' , 'bug389321-2-ref.html' ] ,
    [ 'bug613807-1.html' , 'bug613807-1-ref.html' ] ,
+    [ 'bug989012-1.html' , 'bug989012-1-ref.html' ] ,
+    [ 'bug989012-2.html' , 'bug989012-2-ref.html' ] ,
    [ 'bug1082486-1.html', 'bug1082486-1-ref.html'] ,
    [ 'bug1082486-2.html', 'bug1082486-2-ref.html'] ,
    // The following test cases are all involving with one sending
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@ -3099,6 +3099,7 @@ nsFrame::PeekBackwardAndForward(nsSelectionAmount aAmountBack,
                           aJumpLines,
                           true,  //limit on scrolled views
                           false,
+                           false,
                           false);
    rv = PeekOffset(&pos);
    if (NS_SUCCEEDED(rv)) {
@ -3115,6 +3116,7 @@ nsFrame::PeekBackwardAndForward(nsSelectionAmount aAmountBack,
                              aJumpLines,
                              true,  //limit on scrolled views
                              false,
+                              false,
                              false);
  rv = baseFrame->PeekOffset(&startpos);
  if (NS_FAILED(rv))
@ -3127,6 +3129,7 @@ nsFrame::PeekBackwardAndForward(nsSelectionAmount aAmountBack,
                            aJumpLines,
                            true,  //limit on scrolled views
                            false,
+                            false,
                            false);
  rv = PeekOffset(&endpos);
  if (NS_FAILED(rv))
@ -6491,10 +6494,12 @@ nsIFrame::PeekOffset(nsPeekOffsetStruct* aPos)
        movedOverNonSelectableText |= (peekSearchState == CONTINUE_UNSELECTABLE);

        if (peekSearchState != FOUND) {
+          bool movedOverNonSelectable = false;
          result =
            current->GetFrameFromDirection(aPos->mDirection, aPos->mVisual,
                                           aPos->mJumpLines, aPos->mScrollViewStop,
-                                           &current, &offset, &jumpedLine);
+                                           &current, &offset, &jumpedLine,
+                                           &movedOverNonSelectable);
          if (NS_FAILED(result))
            return result;

@ -6502,11 +6507,18 @@ nsIFrame::PeekOffset(nsPeekOffsetStruct* aPos)
          // to eat non-renderable content on the new line.
          if (jumpedLine)
            eatingNonRenderableWS = true;
+
+          // Remember if we moved over non-selectable text when finding another frame.
+          if (movedOverNonSelectable) {
+            movedOverNonSelectableText = true;
+          }
        }

        // Found frame, but because we moved over non selectable text we want the offset
-        // to be at the frame edge.
-        if (peekSearchState == FOUND && movedOverNonSelectableText)
+        // to be at the frame edge. Note that if we are extending the selection, this
+        // doesn't matter.
+        if (peekSearchState == FOUND && movedOverNonSelectableText &&
+            !aPos->mExtend)
        {
          int32_t start, end;
          current->GetOffsets(start, end);
@ -6584,11 +6596,12 @@ nsIFrame::PeekOffset(nsPeekOffsetStruct* aPos)
        if (!done) {
          nsIFrame* nextFrame;
          int32_t nextFrameOffset;
-          bool jumpedLine;
+          bool jumpedLine, movedOverNonSelectableText;
          result =
            current->GetFrameFromDirection(aPos->mDirection, aPos->mVisual,
                                           aPos->mJumpLines, aPos->mScrollViewStop,
-                                           &nextFrame, &nextFrameOffset, &jumpedLine);
+                                           &nextFrame, &nextFrameOffset, &jumpedLine,
+                                           &movedOverNonSelectableText);
          // We can't jump lines if we're looking for whitespace following
          // non-whitespace, and we already encountered non-whitespace.
          if (NS_FAILED(result) ||
@ -6936,8 +6949,9 @@ nsFrame::GetLineNumber(nsIFrame *aFrame, bool aLockScroll, nsIFrame** aContainin

 nsresult
 nsIFrame::GetFrameFromDirection(nsDirection aDirection, bool aVisual,
-                                bool aJumpLines, bool aScrollViewStop, 
-                                nsIFrame** aOutFrame, int32_t* aOutOffset, bool* aOutJumpedLine)
+                                bool aJumpLines, bool aScrollViewStop,
+                                nsIFrame** aOutFrame, int32_t* aOutOffset,
+                                bool* aOutJumpedLine, bool* aOutMovedOverNonSelectableText)
 {
  nsresult result;

@ -6948,6 +6962,7 @@ nsIFrame::GetFrameFromDirection(nsDirection aDirection, bool aVisual,
  *aOutFrame = nullptr;
  *aOutOffset = 0;
  *aOutJumpedLine = false;
+  *aOutMovedOverNonSelectableText = false;

  // Find the prev/next selectable frame
  bool selectable = false;
@ -7037,6 +7052,9 @@ nsIFrame::GetFrameFromDirection(nsDirection aDirection, bool aVisual,
      return NS_ERROR_FAILURE;

    traversedFrame->IsSelectable(&selectable, nullptr);
+    if (!selectable) {
+      *aOutMovedOverNonSelectableText = true;
+    }
  } // while (!selectable)

  *aOutOffset = (aDirection == eDirNext) ? 0 : -1;
--- a/layout/generic/nsFrameSelection.h
+++ b/layout/generic/nsFrameSelection.h
@ -68,6 +68,7 @@ struct MOZ_STACK_CLASS nsPeekOffsetStruct
                     bool aScrollViewStop,
                     bool aIsKeyboardSelect,
                     bool aVisual,
+                     bool aExtend,
                     mozilla::EWordMovementType aWordMovementType = mozilla::eDefaultBehavior);

  // Note: Most arguments (input and output) are only used with certain values
@ -123,6 +124,9 @@ struct MOZ_STACK_CLASS nsPeekOffsetStruct
  //          Used with: eSelectCharacter, eSelectWord, eSelectBeginLine, eSelectEndLine.
  bool mVisual;

+  // mExtend: Whether the selection is being extended or moved.
+  bool mExtend;
+
  /*** Output arguments ***/

  // mResultContent: Content reached as a result of the peek.
--- a/layout/generic/nsIFrame.h
+++ b/layout/generic/nsIFrame.h
@ -2450,10 +2450,13 @@ public:
   *  @param aOutOffset [out] 0 indicates that we arrived at the beginning of the output frame;
   *                          -1 indicates that we arrived at its end.
   *  @param aOutJumpedLine [out] whether this frame and the returned frame are on different lines
+   *  @param aOutMovedOverNonSelectableText [out] whether we jumped over a non-selectable
+   *                                              frame during the search
   */
  nsresult GetFrameFromDirection(nsDirection aDirection, bool aVisual,
-                                 bool aJumpLines, bool aScrollViewStop, 
-                                 nsIFrame** aOutFrame, int32_t* aOutOffset, bool* aOutJumpedLine);
+                                 bool aJumpLines, bool aScrollViewStop,
+                                 nsIFrame** aOutFrame, int32_t* aOutOffset,
+                                 bool* aOutJumpedLine, bool* aOutMovedOverNonSelectableText);

  /**
   *  called to see if the children of the frame are visible from indexstart to index end.
--- a/layout/generic/nsSelection.cpp
+++ b/layout/generic/nsSelection.cpp
@ -112,6 +112,7 @@ nsPeekOffsetStruct::nsPeekOffsetStruct(nsSelectionAmount aAmount,
                                       bool aScrollViewStop,
                                       bool aIsKeyboardSelect,
                                       bool aVisual,
+                                       bool aExtend,
                                       EWordMovementType aWordMovementType)
  : mAmount(aAmount)
  , mDirection(aDirection)
@ -122,6 +123,7 @@ nsPeekOffsetStruct::nsPeekOffsetStruct(nsSelectionAmount aAmount,
  , mScrollViewStop(aScrollViewStop)
  , mIsKeyboardSelect(aIsKeyboardSelect)
  , mVisual(aVisual)
+  , mExtend(aExtend)
  , mResultContent()
  , mResultFrame(nullptr)
  , mContentOffset(0)
@ -852,7 +854,8 @@ nsFrameSelection::MoveCaret(nsDirection       aDirection,
  //set data using 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, eDirPrevious, offsetused, desiredPos,
-                         true, mLimiter != nullptr, true, visualMovement);
+                         true, mLimiter != nullptr, true, visualMovement,
+                         aContinueSelection);

  nsBidiDirection paraDir = nsBidiPresUtils::ParagraphDirection(frame);

@ -1139,10 +1142,11 @@ nsFrameSelection::GetPrevNextBidiLevels(nsIContent*        aNode,

  nsIFrame *newFrame;
  int32_t offset;
-  bool jumpedLine;
+  bool jumpedLine, movedOverNonSelectableText;
  nsresult rv = currentFrame->GetFrameFromDirection(direction, false,
                                                    aJumpLines, true,
-                                                    &newFrame, &offset, &jumpedLine);
+                                                    &newFrame, &offset, &jumpedLine,
+                                                    &movedOverNonSelectableText);
  if (NS_FAILED(rv))
    newFrame = nullptr;

@ -1444,7 +1448,7 @@ nsFrameSelection::HandleDrag(nsIFrame *aFrame, nsPoint aPoint)
      // first move one character forward.
      nsPeekOffsetStruct charPos(eSelectCharacter, eDirNext, offset,
                                 nsPoint(0, 0), false, mLimiter != nullptr,
-                                 false, false);
+                                 false, false, false);
      if (NS_SUCCEEDED(frame->PeekOffset(&charPos))) {
        frame = charPos.mResultFrame;
        offset = charPos.mContentOffset;
@ -1452,7 +1456,7 @@ nsFrameSelection::HandleDrag(nsIFrame *aFrame, nsPoint aPoint)
    }

    nsPeekOffsetStruct pos(amount, direction, offset, nsPoint(0, 0),
-                           false, mLimiter != nullptr, false, false);
+                           false, mLimiter != nullptr, false, false, false);

    if (frame && NS_SUCCEEDED(frame->PeekOffset(&pos)) && pos.mResultContent) {
      offsets.content = pos.mResultContent;