From e79e7e7398804bdd892ffe29cd126ea2d18ad213 Mon Sep 17 00:00:00 2001
From: Jonathan Kew <jfkthame@gmail.com>
Date: Thu, 16 Dec 2010 14:17:41 -0800
Subject: [PATCH] bug 332636 - fix editor's handling of surrogate-pair
 combining marks with backspace. r=ehsan a=roc

---
 accessible/src/html/nsHyperTextAccessible.cpp |  2 +-
 .../libeditor/html/tests/test_bug332636.html  | 37 +++++++++++++++++--
 layout/generic/nsBRFrame.cpp                  |  6 ++-
 layout/generic/nsContainerFrame.cpp           |  3 +-
 layout/generic/nsContainerFrame.h             |  3 +-
 layout/generic/nsFrame.cpp                    |  7 +++-
 layout/generic/nsFrame.h                      |  3 +-
 layout/generic/nsIFrame.h                     | 25 +++++++++----
 layout/generic/nsInlineFrame.cpp              |  3 +-
 layout/generic/nsInlineFrame.h                |  3 +-
 layout/generic/nsSelection.cpp                | 10 ++---
 layout/generic/nsTextFrame.h                  |  3 +-
 layout/generic/nsTextFrameThebes.cpp          | 26 ++++++++++---
 13 files changed, 99 insertions(+), 32 deletions(-)

diff --git a/accessible/src/html/nsHyperTextAccessible.cpp b/accessible/src/html/nsHyperTextAccessible.cpp
index 88ced8f86f26..77b81867a6e2 100644
--- a/accessible/src/html/nsHyperTextAccessible.cpp
+++ b/accessible/src/html/nsHyperTextAccessible.cpp
@@ -907,7 +907,7 @@ nsresult nsHyperTextAccessible::GetTextHelper(EGetTextType aType, nsAccessibleTe
   PRBool needsStart = PR_FALSE;
   switch (aBoundaryType) {
     case BOUNDARY_CHAR:
-      amount = eSelectCharacter;
+      amount = eSelectCluster;
       if (aType == eGetAt)
         aType = eGetAfter; // Avoid returning 2 characters
       break;
diff --git a/editor/libeditor/html/tests/test_bug332636.html b/editor/libeditor/html/tests/test_bug332636.html
index 54461e4c705c..8b616da78406 100644
--- a/editor/libeditor/html/tests/test_bug332636.html
+++ b/editor/libeditor/html/tests/test_bug332636.html
@@ -14,8 +14,15 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=332636
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=332636">Mozilla Bug 332636</a>
 <p id="display"></p>
 <div id="content">
-  <div id="edit" contenteditable="true">a𐐀b</div>
-  <div id="edit2" contenteditable="true">a&#x10a0f;b</div>
+  <div id="edit0" contenteditable="true">axb</div><!-- reference: plane 0 base character -->
+  <div id="edit1" contenteditable="true">a&#x0308;b</div><!-- reference: plane 0 diacritic -->
+  <div id="edit2" contenteditable="true">a&#x10400;b</div><!-- plane 1 base character -->
+  <div id="edit3" contenteditable="true">a&#x10a0f;b</div><!-- plane 1 diacritic -->
+
+  <div id="edit0b" contenteditable="true">axb</div><!-- reference: plane 0 base character -->
+  <div id="edit1b" contenteditable="true">a&#x0308;b</div><!-- reference: plane 0 diacritic -->
+  <div id="edit2b" contenteditable="true">a&#x10400;b</div><!-- plane 1 base character -->
+  <div id="edit3b" contenteditable="true">a&#x10a0f;b</div><!-- plane 1 diacritic -->
 </div>
 <pre id="test">
 <script type="application/javascript">
@@ -33,9 +40,33 @@ function test(edit) {
   is(edit.textContent, "ab", "The backspace key should delete the UTF-16 surrogate pair correctly");
 }
 
+function testWithMove(edit, offset) {
+  edit.focus();
+  var sel = window.getSelection();
+  sel.collapse(edit.childNodes[0], 0);
+  var i;
+  for (i = 0; i < offset; ++i) {
+    synthesizeKey("VK_RIGHT", {});
+    synthesizeKey("VK_LEFT", {});
+    synthesizeKey("VK_RIGHT", {});
+  }
+  synthesizeKey("VK_BACK_SPACE", {});
+  is(edit.textContent, "ab", "The backspace key should delete the UTF-16 surrogate pair correctly");
+}
+
 function runTest() {
-  test(document.getElementById("edit"));
+  /* test backspace-deletion of the middle character */
+  test(document.getElementById("edit0"));
+  test(document.getElementById("edit1"));
   test(document.getElementById("edit2"));
+  test(document.getElementById("edit3"));
+
+  /* extra tests with the use of RIGHT and LEFT to get to the right place */
+  testWithMove(document.getElementById("edit0b"), 2);
+  testWithMove(document.getElementById("edit1b"), 1);
+  testWithMove(document.getElementById("edit2b"), 2);
+  testWithMove(document.getElementById("edit3b"), 1);
+
   SimpleTest.finish();
 }
 
diff --git a/layout/generic/nsBRFrame.cpp b/layout/generic/nsBRFrame.cpp
index e7e8c8e58de1..3f487d9ae4db 100644
--- a/layout/generic/nsBRFrame.cpp
+++ b/layout/generic/nsBRFrame.cpp
@@ -69,7 +69,8 @@ public:
   virtual ContentOffsets CalcContentOffsetsFromFramePoint(nsPoint aPoint);
 
   virtual PRBool PeekOffsetNoAmount(PRBool aForward, PRInt32* aOffset);
-  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset);
+  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                                     PRBool aRespectClusters = PR_TRUE);
   virtual PRBool PeekOffsetWord(PRBool aForward, PRBool aWordSelectEatSpace, PRBool aIsKeyboardSelect,
                                 PRInt32* aOffset, PeekWordState* aState);
 
@@ -274,7 +275,8 @@ BRFrame::PeekOffsetNoAmount(PRBool aForward, PRInt32* aOffset)
 }
 
 PRBool
-BRFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset)
+BRFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                             PRBool aRespectClusters)
 {
   NS_ASSERTION (aOffset && *aOffset <= 1, "aOffset out of range");
   // Keep going. The actual line jumping will stop us.
diff --git a/layout/generic/nsContainerFrame.cpp b/layout/generic/nsContainerFrame.cpp
index 188762477dba..7fe00034922d 100644
--- a/layout/generic/nsContainerFrame.cpp
+++ b/layout/generic/nsContainerFrame.cpp
@@ -397,7 +397,8 @@ nsContainerFrame::PeekOffsetNoAmount(PRBool aForward, PRInt32* aOffset)
 }
 
 PRBool
-nsContainerFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset)
+nsContainerFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                                      PRBool aRespectClusters)
 {
   NS_ASSERTION (aOffset && *aOffset <= 1, "aOffset out of range");
   // Don't allow the caret to stay in an empty (leaf) container frame.
diff --git a/layout/generic/nsContainerFrame.h b/layout/generic/nsContainerFrame.h
index f4c670f11ab1..2160a73c2a22 100644
--- a/layout/generic/nsContainerFrame.h
+++ b/layout/generic/nsContainerFrame.h
@@ -98,7 +98,8 @@ public:
 
   virtual PRBool IsLeaf() const;
   virtual PRBool PeekOffsetNoAmount(PRBool aForward, PRInt32* aOffset);
-  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset);
+  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                                     PRBool aRespectClusters = PR_TRUE);
   
 #ifdef DEBUG
   NS_IMETHOD List(FILE* out, PRInt32 aIndent) const;
diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
index 235857596b8d..930efdb02bad 100644
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -5404,6 +5404,7 @@ nsIFrame::PeekOffset(nsPeekOffsetStruct* aPos)
   
   switch (aPos->mAmount) {
     case eSelectCharacter:
+    case eSelectCluster:
     {
       PRBool eatingNonRenderableWS = PR_FALSE;
       PRBool done = PR_FALSE;
@@ -5416,7 +5417,8 @@ nsIFrame::PeekOffset(nsPeekOffsetStruct* aPos)
         if (eatingNonRenderableWS)
           done = current->PeekOffsetNoAmount(movingInFrameDirection, &offset); 
         else
-          done = current->PeekOffsetCharacter(movingInFrameDirection, &offset); 
+          done = current->PeekOffsetCharacter(movingInFrameDirection, &offset,
+                                              aPos->mAmount == eSelectCluster);
 
         if (!done) {
           result =
@@ -5705,7 +5707,8 @@ nsFrame::PeekOffsetNoAmount(PRBool aForward, PRInt32* aOffset)
 }
 
 PRBool
-nsFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset)
+nsFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                             PRBool aRespectClusters)
 {
   NS_ASSERTION (aOffset && *aOffset <= 1, "aOffset out of range");
   PRInt32 startOffset = *aOffset;
diff --git a/layout/generic/nsFrame.h b/layout/generic/nsFrame.h
index cae31be38fe6..de3ec1803af6 100644
--- a/layout/generic/nsFrame.h
+++ b/layout/generic/nsFrame.h
@@ -238,7 +238,8 @@ public:
   NS_IMETHOD  GetSelectionController(nsPresContext *aPresContext, nsISelectionController **aSelCon);
 
   virtual PRBool PeekOffsetNoAmount(PRBool aForward, PRInt32* aOffset);
-  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset);
+  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                                     PRBool aRespectClusters = PR_TRUE);
   virtual PRBool PeekOffsetWord(PRBool aForward, PRBool aWordSelectEatSpace, PRBool aIsKeyboardSelect,
                                 PRInt32* aOffset, PeekWordState *aState);
   /**
diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h
index 90fa2e81a93c..99cc07fc2d42 100644
--- a/layout/generic/nsIFrame.h
+++ b/layout/generic/nsIFrame.h
@@ -289,13 +289,18 @@ typedef PRUint64 nsFrameState;
 //----------------------------------------------------------------------
 
 enum nsSelectionAmount {
-  eSelectCharacter = 0,
-  eSelectWord      = 1,
-  eSelectLine      = 2,  //previous drawn line in flow.
-  eSelectBeginLine = 3,
-  eSelectEndLine   = 4,
-  eSelectNoAmount  = 5,   //just bounce back current offset.
-  eSelectParagraph = 6    //select a "paragraph"
+  eSelectCharacter = 0, // a single Unicode character;
+                        // do not use this (prefer Cluster) unless you
+                        // are really sure it's what you want
+  eSelectCluster   = 1, // a grapheme cluster: this is usually the right
+                        // choice for movement or selection by "character"
+                        // as perceived by the user
+  eSelectWord      = 2,
+  eSelectLine      = 3, // previous drawn line in flow.
+  eSelectBeginLine = 4,
+  eSelectEndLine   = 5,
+  eSelectNoAmount  = 6, // just bounce back current offset.
+  eSelectParagraph = 7  // select a "paragraph"
 };
 
 enum nsDirection {
@@ -2680,11 +2685,15 @@ protected:
    * @param  aForward [in] Are we moving forward (or backward) in content order.
    * @param  aOffset [in/out] At what offset into the frame to start looking.
    *         on output - what offset was reached (whether or not we found a place to stop).
+   * @param  aRespectClusters [in] Whether to restrict result to valid cursor locations
+   *         (between grapheme clusters) - default TRUE maintains "normal" behavior,
+   *         FALSE is used for selection by "code unit" (instead of "character")
    * @return PR_TRUE: An appropriate offset was found within this frame,
    *         and is given by aOffset.
    *         PR_FALSE: Not found within this frame, need to try the next frame.
    */
-  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset) = 0;
+  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                                     PRBool aRespectClusters = PR_TRUE) = 0;
   
   /**
    * Search the frame for the next word boundary
diff --git a/layout/generic/nsInlineFrame.cpp b/layout/generic/nsInlineFrame.cpp
index 944336b043fc..961143388eb7 100644
--- a/layout/generic/nsInlineFrame.cpp
+++ b/layout/generic/nsInlineFrame.cpp
@@ -166,7 +166,8 @@ nsInlineFrame::IsEmpty()
 }
 
 PRBool
-nsInlineFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset)
+nsInlineFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                                   PRBool aRespectClusters)
 {
   // Override the implementation in nsFrame, to skip empty inline frames
   NS_ASSERTION (aOffset && *aOffset <= 1, "aOffset out of range");
diff --git a/layout/generic/nsInlineFrame.h b/layout/generic/nsInlineFrame.h
index 5a0af8e42cc6..acdd234f17c8 100644
--- a/layout/generic/nsInlineFrame.h
+++ b/layout/generic/nsInlineFrame.h
@@ -97,7 +97,8 @@ public:
   virtual PRBool IsEmpty();
   virtual PRBool IsSelfEmpty();
 
-  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset);
+  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                                     PRBool aRespectClusters = PR_TRUE);
   
   // nsIHTMLReflow overrides
   virtual void AddInlineMinWidth(nsIRenderingContext *aRenderingContext,
diff --git a/layout/generic/nsSelection.cpp b/layout/generic/nsSelection.cpp
index c0689565221d..1ef6c4dcdb40 100644
--- a/layout/generic/nsSelection.cpp
+++ b/layout/generic/nsSelection.cpp
@@ -1247,7 +1247,7 @@ nsFrameSelection::MoveCaret(PRUint32          aKeycode,
     nsIFrame *theFrame;
     PRInt32 currentOffset, frameStart, frameEnd;
 
-    if (aAmount == eSelectCharacter || aAmount == eSelectWord)
+    if (aAmount >= eSelectCharacter && aAmount <= eSelectWord)
     {
       // For left/right, PeekOffset() sets pos.mResultFrame correctly, but does not set pos.mAttachForward,
       // so determine the hint here based on the result frame and offset:
@@ -2191,15 +2191,15 @@ nsresult
 nsFrameSelection::CharacterMove(PRBool aForward, PRBool aExtend)
 {
   if (aForward)
-    return MoveCaret(nsIDOMKeyEvent::DOM_VK_RIGHT,aExtend,eSelectCharacter);
+    return MoveCaret(nsIDOMKeyEvent::DOM_VK_RIGHT, aExtend, eSelectCluster);
   else
-    return MoveCaret(nsIDOMKeyEvent::DOM_VK_LEFT,aExtend,eSelectCharacter);
+    return MoveCaret(nsIDOMKeyEvent::DOM_VK_LEFT, aExtend, eSelectCluster);
 }
 
 nsresult
 nsFrameSelection::CharacterExtendForDelete()
 {
-  return MoveCaret(nsIDOMKeyEvent::DOM_VK_DELETE, PR_TRUE, eSelectCharacter);
+  return MoveCaret(nsIDOMKeyEvent::DOM_VK_DELETE, PR_TRUE, eSelectCluster);
 }
 
 nsresult
@@ -5847,7 +5847,7 @@ nsTypedSelection::Modify(const nsAString& aAlter, const nsAString& aDirection,
   nsSelectionAmount amount;
   PRUint32 keycode;
   if (aGranularity.LowerCaseEqualsLiteral("character")) {
-    amount = eSelectCharacter;
+    amount = eSelectCluster;
     keycode = forward ? (PRUint32) nsIDOMKeyEvent::DOM_VK_RIGHT :
                         (PRUint32) nsIDOMKeyEvent::DOM_VK_LEFT;
   }
diff --git a/layout/generic/nsTextFrame.h b/layout/generic/nsTextFrame.h
index c0a214ae2279..8aaab4b0ab64 100644
--- a/layout/generic/nsTextFrame.h
+++ b/layout/generic/nsTextFrame.h
@@ -169,7 +169,8 @@ public:
                         SelectionType aType);
 
   virtual PRBool PeekOffsetNoAmount(PRBool aForward, PRInt32* aOffset);
-  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset);
+  virtual PRBool PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                                     PRBool aRespectClusters = PR_TRUE);
   virtual PRBool PeekOffsetWord(PRBool aForward, PRBool aWordSelectEatSpace, PRBool aIsKeyboardSelect,
                                 PRInt32* aOffset, PeekWordState* aState);
 
diff --git a/layout/generic/nsTextFrameThebes.cpp b/layout/generic/nsTextFrameThebes.cpp
index 26f3c88a9098..57349f9c1b9b 100644
--- a/layout/generic/nsTextFrameThebes.cpp
+++ b/layout/generic/nsTextFrameThebes.cpp
@@ -5580,19 +5580,35 @@ private:
 };
 
 static PRBool
-IsAcceptableCaretPosition(const gfxSkipCharsIterator& aIter, gfxTextRun* aTextRun,
+IsAcceptableCaretPosition(const gfxSkipCharsIterator& aIter,
+                          PRBool aRespectClusters,
+                          gfxTextRun* aTextRun,
                           nsIFrame* aFrame)
 {
   if (aIter.IsOriginalCharSkipped())
     return PR_FALSE;
   PRUint32 index = aIter.GetSkippedOffset();
-  if (!aTextRun->IsClusterStart(index))
+  if (aRespectClusters && !aTextRun->IsClusterStart(index))
     return PR_FALSE;
+  if (index > 0) {
+    // Check whether the proposed position is in between the two halves of a
+    // surrogate pair; if so, this is not a valid character boundary.
+    // (In the case where we are respecting clusters, we won't actually get
+    // this far because the low surrogate is also marked as non-clusterStart
+    // so we'll return FALSE above.)
+    // If the textrun is 8-bit it can't have any surrogates, so we only need
+    // to check the actual characters if GetTextUnicode() returns non-null.
+    const PRUnichar *txt = aTextRun->GetTextUnicode();
+    if (txt && NS_IS_LOW_SURROGATE(txt[index]) &&
+               NS_IS_HIGH_SURROGATE(txt[index-1]))
+      return PR_FALSE;
+  }
   return PR_TRUE;
 }
 
 PRBool
-nsTextFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset)
+nsTextFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset,
+                                 PRBool aRespectClusters)
 {
   PRInt32 contentLength = GetContentLength();
   NS_ASSERTION(aOffset && *aOffset <= contentLength, "aOffset out of range");
@@ -5617,7 +5633,7 @@ nsTextFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset)
     for (PRInt32 i = NS_MIN(trimmed.GetEnd(), startOffset) - 1;
          i >= trimmed.mStart; --i) {
       iter.SetOriginalOffset(i);
-      if (IsAcceptableCaretPosition(iter, mTextRun, this)) {
+      if (IsAcceptableCaretPosition(iter, aRespectClusters, mTextRun, this)) {
         *aOffset = i - mContentOffset;
         return PR_TRUE;
       }
@@ -5634,7 +5650,7 @@ nsTextFrame::PeekOffsetCharacter(PRBool aForward, PRInt32* aOffset)
       for (PRInt32 i = startOffset + 1; i <= trimmed.GetEnd(); ++i) {
         iter.SetOriginalOffset(i);
         if (i == trimmed.GetEnd() ||
-            IsAcceptableCaretPosition(iter, mTextRun, this)) {
+            IsAcceptableCaretPosition(iter, aRespectClusters, mTextRun, this)) {
           *aOffset = i - mContentOffset;
           return PR_TRUE;
         }