Bug 1216427 - part 1 - Ensure a character+VS sequence or a ligated Regional-Indicator flag symbol is deleted as a single unit when backspacing. r=emk
authorJonathan Kew <jkew@mozilla.com>
Mon, 26 Oct 2015 10:47:16 +0000
changeset 304664 019608d8a449322f00784012d18be0bad0b61291
parent 304663 007390c592ebe683d57923ce5155117a56a57355
child 304665 9a59037687802067d469167e5eafe5cdf3ce1ecb
push id1001
push userraliiev@mozilla.com
push dateMon, 18 Jan 2016 19:06:03 +0000
treeherdermozilla-release@8b89261f3ac4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemk
bugs1216427
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1216427 - part 1 - Ensure a character+VS sequence or a ligated Regional-Indicator flag symbol is deleted as a single unit when backspacing. r=emk
editor/libeditor/nsPlaintextEditor.cpp
editor/libeditor/tests/test_backspace_vs.html
gfx/thebes/gfxFont.cpp
gfx/thebes/gfxFont.h
gfx/thebes/gfxFontUtils.h
gfx/thebes/gfxTextRun.h
layout/generic/nsTextFrame.cpp
--- a/editor/libeditor/nsPlaintextEditor.cpp
+++ b/editor/libeditor/nsPlaintextEditor.cpp
@@ -1,15 +1,16 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsPlaintextEditor.h"
 
+#include "gfxFontUtils.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/dom/Selection.h"
 #include "mozilla/TextComposition.h"
 #include "mozilla/TextEvents.h"
 #include "mozilla/dom/Element.h"
 #include "mozilla/mozalloc.h"
 #include "nsAString.h"
@@ -595,35 +596,38 @@ nsPlaintextEditor::ExtendSelectionForDel
         *aAction = eNone;
         break;
       case eNext:
         result = selCont->CharacterExtendForDelete();
         // Don't set aAction to eNone (see Bug 502259)
         break;
       case ePrevious: {
         // Only extend the selection where the selection is after a UTF-16
-        // surrogate pair.  For other cases we don't want to do that, in order
+        // surrogate pair or a variation selector.
+        // For other cases we don't want to do that, in order
         // to make sure that pressing backspace will only delete the last
         // typed character.
         nsCOMPtr<nsIDOMNode> node;
         int32_t offset;
         result = GetStartNodeAndOffset(aSelection, getter_AddRefs(node), &offset);
         NS_ENSURE_SUCCESS(result, result);
         NS_ENSURE_TRUE(node, NS_ERROR_FAILURE);
 
         if (IsTextNode(node)) {
           nsCOMPtr<nsIDOMCharacterData> charData = do_QueryInterface(node);
           if (charData) {
             nsAutoString data;
             result = charData->GetData(data);
             NS_ENSURE_SUCCESS(result, result);
 
-            if (offset > 1 &&
-                NS_IS_LOW_SURROGATE(data[offset - 1]) &&
-                NS_IS_HIGH_SURROGATE(data[offset - 2])) {
+            if ((offset > 1 &&
+                 NS_IS_LOW_SURROGATE(data[offset - 1]) &&
+                 NS_IS_HIGH_SURROGATE(data[offset - 2])) ||
+                (offset > 0 &&
+                 gfxFontUtils::IsVarSelector(data[offset - 1]))) {
               result = selCont->CharacterExtendForBackspace();
             }
           }
         }
         break;
       }
       case eToBeginningOfLine:
         selCont->IntraLineMove(true, false);          // try to move to end
--- a/editor/libeditor/tests/test_backspace_vs.html
+++ b/editor/libeditor/tests/test_backspace_vs.html
@@ -49,33 +49,33 @@ addLoadEvent(runTest);
 
 function test(edit, bsCount) {
   edit.focus();
   var sel = window.getSelection();
   sel.collapse(edit.childNodes[0], edit.textContent.length - 1);
   for (i = 0; i < bsCount; ++i) {
     synthesizeKey("VK_BACK_SPACE", {});
   }
-  todo_is(edit.textContent, "ab", "The backspace key should delete the characters correctly");
+  is(edit.textContent, "ab", "The backspace key should delete the characters correctly");
 }
 
 function testWithMove(edit, offset, bsCount) {
   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", {});
   }
   for (i = 0; i < bsCount; ++i) {
     synthesizeKey("VK_BACK_SPACE", {});
   }
-  todo_is(edit.textContent, "ab", "The backspace key should delete the characters correctly");
+  is(edit.textContent, "ab", "The backspace key should delete the characters correctly");
 }
 
 function runTest() {
   /* test backspace-deletion of the middle character(s) */
   test(document.getElementById("edit1"), 1);
   test(document.getElementById("edit2"), 1);
   test(document.getElementById("edit3"), 1);
   test(document.getElementById("edit4"), 1);
--- a/gfx/thebes/gfxFont.cpp
+++ b/gfx/thebes/gfxFont.cpp
@@ -568,19 +568,16 @@ gfxShapedText::SetupClusterBoundaries(ui
         // advance iter to the next cluster-start (or end of text)
         iter.Next();
         // step past the first char of the cluster
         aString++;
         glyphs++;
         // mark all the rest as cluster-continuations
         while (aString < iter) {
             *glyphs = extendCluster;
-            if (NS_IS_LOW_SURROGATE(*aString)) {
-                glyphs->SetIsLowSurrogate();
-            }
             glyphs++;
             aString++;
         }
     }
 }
 
 void
 gfxShapedText::SetupClusterBoundaries(uint32_t       aOffset,
--- a/gfx/thebes/gfxFont.h
+++ b/gfx/thebes/gfxFont.h
@@ -736,18 +736,17 @@ public:
             // the mGlyphID is actually the UTF16 character code. The bit is
             // inverted so we can memset the array to zero to indicate all missing.
             FLAG_NOT_MISSING              = 0x01,
             FLAG_NOT_CLUSTER_START        = 0x02,
             FLAG_NOT_LIGATURE_GROUP_START = 0x04,
 
             FLAG_CHAR_IS_TAB              = 0x08,
             FLAG_CHAR_IS_NEWLINE          = 0x10,
-            FLAG_CHAR_IS_LOW_SURROGATE    = 0x20,
-            CHAR_IDENTITY_FLAGS_MASK      = 0x38,
+            CHAR_IDENTITY_FLAGS_MASK      = 0x18,
 
             GLYPH_COUNT_MASK = 0x00FFFF00U,
             GLYPH_COUNT_SHIFT = 8
         };
 
         // "Simple glyphs" have a simple glyph ID, simple advance and their
         // x and y offsets are zero. Also the glyph extents do not overflow
         // the font-box defined by the font ascent, descent and glyph advance width.
@@ -788,19 +787,16 @@ public:
         }
 
         bool CharIsTab() const {
             return !IsSimpleGlyph() && (mValue & FLAG_CHAR_IS_TAB) != 0;
         }
         bool CharIsNewline() const {
             return !IsSimpleGlyph() && (mValue & FLAG_CHAR_IS_NEWLINE) != 0;
         }
-        bool CharIsLowSurrogate() const {
-            return !IsSimpleGlyph() && (mValue & FLAG_CHAR_IS_LOW_SURROGATE) != 0;
-        }
 
         uint32_t CharIdentityFlags() const {
             return IsSimpleGlyph() ? 0 : (mValue & CHAR_IDENTITY_FLAGS_MASK);
         }
 
         void SetClusterStart(bool aIsClusterStart) {
             NS_ASSERTION(!IsSimpleGlyph(),
                          "can't call SetClusterStart on simple glyphs");
@@ -865,20 +861,16 @@ public:
         void SetIsTab() {
             NS_ASSERTION(!IsSimpleGlyph(), "Expected non-simple-glyph");
             mValue |= FLAG_CHAR_IS_TAB;
         }
         void SetIsNewline() {
             NS_ASSERTION(!IsSimpleGlyph(), "Expected non-simple-glyph");
             mValue |= FLAG_CHAR_IS_NEWLINE;
         }
-        void SetIsLowSurrogate() {
-            NS_ASSERTION(!IsSimpleGlyph(), "Expected non-simple-glyph");
-            mValue |= FLAG_CHAR_IS_LOW_SURROGATE;
-        }
 
     private:
         uint32_t mValue;
     };
 
     // Accessor for the array of CompressedGlyph records, which will be in
     // a different place in gfxShapedWord vs gfxTextRun
     virtual CompressedGlyph *GetCharacterGlyphs() = 0;
@@ -903,21 +895,16 @@ public:
                    const DetailedGlyph *aGlyphs);
 
     void SetMissingGlyph(uint32_t aIndex, uint32_t aChar, gfxFont *aFont);
 
     void SetIsSpace(uint32_t aIndex) {
         GetCharacterGlyphs()[aIndex].SetIsSpace();
     }
 
-    void SetIsLowSurrogate(uint32_t aIndex) {
-        SetGlyphs(aIndex, CompressedGlyph().SetComplex(false, false, 0), nullptr);
-        GetCharacterGlyphs()[aIndex].SetIsLowSurrogate();
-    }
-
     bool HasDetailedGlyphs() const {
         return mDetailedGlyphs != nullptr;
     }
 
     bool IsLigatureGroupStart(uint32_t aPos) {
         NS_ASSERTION(aPos < GetLength(), "aPos out of range");
         return GetCharacterGlyphs()[aPos].IsLigatureGroupStart();
     }
--- a/gfx/thebes/gfxFontUtils.h
+++ b/gfx/thebes/gfxFontUtils.h
@@ -898,16 +898,26 @@ public:
         kUnicodeVS256 = 0xE01EF
     };
 
     static inline bool IsVarSelector(uint32_t ch) {
         return (ch >= kUnicodeVS1 && ch <= kUnicodeVS16) ||
                (ch >= kUnicodeVS17 && ch <= kUnicodeVS256);
     }
 
+    enum {
+        kUnicodeRegionalIndicatorA = 0x1F1E6,
+        kUnicodeRegionalIndicatorZ = 0x1F1FF
+    };
+
+    static inline bool IsRegionalIndicator(uint32_t aCh) {
+        return aCh >= kUnicodeRegionalIndicatorA &&
+               aCh <= kUnicodeRegionalIndicatorZ;
+    }
+
     static inline bool IsInvalid(uint32_t ch) {
         return (ch == 0xFFFD);
     }
 
     // Font code may want to know if there is the potential for bidi behavior
     // to be triggered by any of the characters in a text run; this can be
     // used to test that possibility.
     enum {
--- a/gfx/thebes/gfxTextRun.h
+++ b/gfx/thebes/gfxTextRun.h
@@ -123,20 +123,16 @@ public:
     bool CharIsTab(uint32_t aPos) const {
         NS_ASSERTION(aPos < GetLength(), "aPos out of range");
         return mCharacterGlyphs[aPos].CharIsTab();
     }
     bool CharIsNewline(uint32_t aPos) const {
         NS_ASSERTION(aPos < GetLength(), "aPos out of range");
         return mCharacterGlyphs[aPos].CharIsNewline();
     }
-    bool CharIsLowSurrogate(uint32_t aPos) const {
-        NS_ASSERTION(aPos < GetLength(), "aPos out of range");
-        return mCharacterGlyphs[aPos].CharIsLowSurrogate();
-    }
 
     // All uint32_t aStart, uint32_t aLength ranges below are restricted to
     // grapheme cluster boundaries! All offsets are in terms of the string
     // passed into MakeTextRun.
     
     // All coordinates are in layout/app units
 
     /**
@@ -530,20 +526,16 @@ public:
             DetailedGlyph *details = AllocateDetailedGlyphs(aIndex, 1);
             details->mGlyphID = g->GetSimpleGlyph();
             details->mAdvance = g->GetSimpleAdvance();
             details->mXOffset = details->mYOffset = 0;
             SetGlyphs(aIndex, CompressedGlyph().SetComplex(true, true, 1), details);
         }
         g->SetIsNewline();
     }
-    void SetIsLowSurrogate(uint32_t aIndex) {
-        SetGlyphs(aIndex, CompressedGlyph().SetComplex(false, false, 0), nullptr);
-        mCharacterGlyphs[aIndex].SetIsLowSurrogate();
-    }
 
     /**
      * Prefetch all the glyph extents needed to ensure that Measure calls
      * on this textrun not requesting tight boundingBoxes will succeed. Note
      * that some glyph extents might not be fetched due to OOM or other
      * errors.
      */
     void FetchGlyphExtents(gfxContext *aRefContext);
--- a/layout/generic/nsTextFrame.cpp
+++ b/layout/generic/nsTextFrame.cpp
@@ -7021,23 +7021,47 @@ IsAcceptableCaretPosition(const gfxSkipC
 {
   if (aIter.IsOriginalCharSkipped())
     return false;
   uint32_t index = aIter.GetSkippedOffset();
   if (aRespectClusters && !aTextRun->IsClusterStart(index))
     return 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.
+    // surrogate pair, or before a Variation Selector character;
+    // 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 (aTextRun->CharIsLowSurrogate(index)) {
+    uint32_t offs = aIter.GetOriginalOffset();
+    const nsTextFragment* frag = aFrame->GetContent()->GetText();
+    uint32_t ch = frag->CharAt(offs);
+
+    if (gfxFontUtils::IsVarSelector(ch) ||
+        (NS_IS_LOW_SURROGATE(ch) && offs > 0 &&
+         NS_IS_HIGH_SURROGATE(frag->CharAt(offs - 1)))) {
       return false;
     }
+
+    // If the proposed position is before a high surrogate, we need to decode
+    // the surrogate pair (if valid) and check the resulting character.
+    if (NS_IS_HIGH_SURROGATE(ch) && offs + 1 < frag->GetLength()) {
+      uint32_t ch2 = frag->CharAt(offs + 1);
+      if (NS_IS_LOW_SURROGATE(ch2)) {
+        ch = SURROGATE_TO_UCS4(ch, ch2);
+        // If the character is a (Plane-14) variation selector,
+        // or a Regional Indicator character that is ligated with the previous
+        // character, this is not a valid boundary.
+        if (gfxFontUtils::IsVarSelector(ch) ||
+            (gfxFontUtils::IsRegionalIndicator(ch) &&
+             !aTextRun->IsLigatureGroupStart(index))) {
+          return false;
+        }
+      }
+    }
   }
   return true;
 }
 
 nsIFrame::FrameSearchResult
 nsTextFrame::PeekOffsetCharacter(bool aForward, int32_t* aOffset,
                                  bool aRespectClusters)
 {