Takes two delete key presses to delete pasted emoji up-pointing index finger with...
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jun 2015 23:49:49 +0000 (23:49 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jun 2015 23:49:49 +0000 (23:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145823

Reviewed by Anders Carlsson.

Source/WebCore:

Tests: editing/deleting/delete-emoji.html

* rendering/RenderText.cpp:
(WebCore::isHangulLVT): Use constants instead of macros. Also changed to take a UChar since
the Hangul processing can work on UTF-16 code unit at a time and doesn't have to handle
surrogate pairs.
(WebCore::isMark): Use U_GC_M_MASK instead of writing the algorithm out another way.
(WebCore::isInArmenianToLimbuRange): Added.
(WebCore::RenderText::previousOffsetForBackwardDeletion): Refactored for clarity and to use
the U16_PREV macro instead of doing what it does in a sloppier way. Added code to allow a
variation selector before an emoji modifier to fix the bug. Changed Hangul logic to work a
code unit at a time, since it can, to use an enum class, and to use constants rather than
all capital macros. Also changed the "dumb" case to use a more appropriate ICU macro.

LayoutTests:

* editing/deleting/delete-emoji-expected.txt: Updated to expect a little more testing.
* editing/deleting/delete-emoji.html: Added a test case and streamlined the test a bit.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@185393 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/editing/deleting/delete-emoji-expected.txt
LayoutTests/editing/deleting/delete-emoji.html
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderText.cpp

index b608ef0539c84215fa9c338bc4817cafb4ecaf40..15580714aa45f8f2adcea37476067fa97322714f 100644 (file)
@@ -1,3 +1,13 @@
+2015-06-09  Darin Adler  <darin@apple.com>
+
+        Takes two delete key presses to delete pasted emoji up-pointing index finger with skin tone
+        https://bugs.webkit.org/show_bug.cgi?id=145823
+
+        Reviewed by Anders Carlsson.
+
+        * editing/deleting/delete-emoji-expected.txt: Updated to expect a little more testing.
+        * editing/deleting/delete-emoji.html: Added a test case and streamlined the test a bit.
+
 2015-06-09  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         feComposite filter does not clip the paint rect to its effect rect when the operator is 'in' or 'atop'
index a51a62d4750e72cf12ad061f3c30cf5bcd46a670..15d85572010bee4ca3630fefc5a567e4b72e70f6 100644 (file)
@@ -1,29 +1,38 @@
 This test verifies that emoji groups and emoji with variations are deleted correctly
 
 Dump of markup 1:
-| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦πŸ‘©β€β€οΈβ€πŸ‘©πŸ‘¨β€β€οΈβ€πŸ‘¨πŸ‘©β€β€οΈβ€πŸ’‹β€πŸ‘©πŸ‘¨β€β€οΈβ€πŸ’‹β€πŸ‘¨<#selection-caret>
+| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦πŸ‘©β€β€οΈβ€πŸ‘©πŸ‘¨β€β€οΈβ€πŸ‘¨πŸ‘©β€β€οΈβ€πŸ’‹β€πŸ‘©πŸ‘¨β€β€οΈβ€πŸ’‹β€πŸ‘¨β˜οΈπŸ»<#selection-caret>
 "
 
 Dump of markup 2:
-| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦πŸ‘©β€β€οΈβ€πŸ‘©πŸ‘¨β€β€οΈβ€πŸ‘¨πŸ‘©β€β€οΈβ€πŸ’‹β€πŸ‘©<#selection-caret>"
+| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦πŸ‘©β€β€οΈβ€πŸ‘©πŸ‘¨β€β€οΈβ€πŸ‘¨πŸ‘©β€β€οΈβ€πŸ’‹β€πŸ‘©πŸ‘¨β€β€οΈβ€πŸ’‹β€πŸ‘¨<#selection-caret>"
 
 Dump of markup 3:
-| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦πŸ‘©β€β€οΈβ€πŸ‘©πŸ‘¨β€β€οΈβ€πŸ‘¨<#selection-caret>"
+| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦πŸ‘©β€β€οΈβ€πŸ‘©πŸ‘¨β€β€οΈβ€πŸ‘¨πŸ‘©β€β€οΈβ€πŸ’‹β€πŸ‘©<#selection-caret>"
 
 Dump of markup 4:
-| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦πŸ‘©β€β€οΈβ€πŸ‘©<#selection-caret>"
+| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦πŸ‘©β€β€οΈβ€πŸ‘©πŸ‘¨β€β€οΈβ€πŸ‘¨<#selection-caret>"
 
 Dump of markup 5:
-| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦<#selection-caret>"
+| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦πŸ‘©β€β€οΈβ€πŸ‘©<#selection-caret>"
 
 Dump of markup 6:
-| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦<#selection-caret>"
+| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦πŸ‘©β€πŸ‘©β€πŸ‘¦<#selection-caret>"
 
 Dump of markup 7:
-| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎ<#selection-caret>"
+| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎπŸ‘¦<#selection-caret>"
 
 Dump of markup 8:
-| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»<#selection-caret>"
+| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»πŸ‘¦πŸΎ<#selection-caret>"
 
 Dump of markup 9:
+| "πŸ‘¦πŸ»πŸ‘¦πŸΎπŸ»<#selection-caret>"
+
+Dump of markup 10:
 | "πŸ‘¦πŸ»πŸ‘¦πŸΎ<#selection-caret>"
+
+Dump of markup 11:
+| "πŸ‘¦πŸ»<#selection-caret>"
+
+Dump of markup 12:
+| <br>
index a5a5e71e81047212532a82a6fc5b55b85020022f..77b895bd7aff7d91ca214c18e334872efdcb692b 100644 (file)
@@ -1,32 +1,18 @@
 <!DOCTYPE html>
 <html>
 <body>
-<div id="test" contenteditable="true">&#x1F466;&#x1F3FB;&#x1F466;&#x1F3FE;&#x1F3FB;&#x1F466;&#x1F3FE;&#x1F466;&#x1F469;&#x200D;&#x1F469;&#x200D;&#x1F466;&#x1F469;&#x200D;&#x2764;&#xFE0F;&#x200D;&#x1F469;&#x1F468;&#x200D;&#x2764;&#xFE0F;&#x200D;&#x1F468;&#x1F469;&#x200D;&#x2764;&#xFE0F;&#x200D;&#x1F48B;&#x200D;&#x1F469;&#x1F468;&#x200D;&#x2764;&#xFE0F;&#x200D;&#x1F48B;&#x200D;&#x1F468;
+<div id="test" contenteditable="true">&#x1F466;&#x1F3FB;&#x1F466;&#x1F3FE;&#x1F3FB;&#x1F466;&#x1F3FE;&#x1F466;&#x1F469;&#x200D;&#x1F469;&#x200D;&#x1F466;&#x1F469;&#x200D;&#x2764;&#xFE0F;&#x200D;&#x1F469;&#x1F468;&#x200D;&#x2764;&#xFE0F;&#x200D;&#x1F468;&#x1F469;&#x200D;&#x2764;&#xFE0F;&#x200D;&#x1F48B;&#x200D;&#x1F469;&#x1F468;&#x200D;&#x2764;&#xFE0F;&#x200D;&#x1F48B;&#x200D;&#x1F468;&#x261D;&#xFE0F;&#x1F3FB;
 </div>
 <script src="../../resources/dump-as-markup.js"></script>
 <script>
 Markup.description("This test verifies that emoji groups and emoji with variations are deleted correctly");
-
-var selection = window.getSelection();
 var testElement = document.getElementById('test');
-selection.setBaseAndExtent(testElement.firstChild, 56, testElement.firstChild, 56);
-Markup.dump("test");
-document.execCommand("Delete");
-Markup.dump("test");
-document.execCommand("Delete");
-Markup.dump("test");
-document.execCommand("Delete");
-Markup.dump("test");
-document.execCommand("Delete");
-Markup.dump("test");
-document.execCommand("Delete");
-Markup.dump("test");
-document.execCommand("Delete");
-Markup.dump("test");
-document.execCommand("Delete");
-Markup.dump("test");
-document.execCommand("Delete");
+getSelection().setBaseAndExtent(testElement.firstChild, testElement.firstChild.length, testElement.firstChild, testElement.firstChild.length);
 Markup.dump("test");
+while (testElement.firstChild.length) {
+    document.execCommand("Delete");
+    Markup.dump("test");
+}
 </script>
 </body>
 </html>
index 542772c42bfd3a974f90157bae571db966205b0b..6f8228836eb29db24a8b84caa85349f9fd8dcbf2 100644 (file)
@@ -1,3 +1,24 @@
+2015-06-09  Darin Adler  <darin@apple.com>
+
+        Takes two delete key presses to delete pasted emoji up-pointing index finger with skin tone
+        https://bugs.webkit.org/show_bug.cgi?id=145823
+
+        Reviewed by Anders Carlsson.
+
+        Tests: editing/deleting/delete-emoji.html
+
+        * rendering/RenderText.cpp:
+        (WebCore::isHangulLVT): Use constants instead of macros. Also changed to take a UChar since
+        the Hangul processing can work on UTF-16 code unit at a time and doesn't have to handle
+        surrogate pairs.
+        (WebCore::isMark): Use U_GC_M_MASK instead of writing the algorithm out another way.
+        (WebCore::isInArmenianToLimbuRange): Added.
+        (WebCore::RenderText::previousOffsetForBackwardDeletion): Refactored for clarity and to use
+        the U16_PREV macro instead of doing what it does in a sloppier way. Added code to allow a
+        variation selector before an emoji modifier to fix the bug. Changed Hangul logic to work a
+        code unit at a time, since it can, to use an enum class, and to use constants rather than
+        all capital macros. Also changed the "dumb" case to use a more appropriate ICU macro.
+
 2015-06-09  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         feComposite filter does not clip the paint rect to its effect rect when the operator is 'in' or 'atop'
index 0944a187ddccc30ce099cf260e6323fd0efb2481..d016b19503546647a943e2c7807eb9305202a578 100644 (file)
@@ -1397,34 +1397,26 @@ int RenderText::previousOffset(int current) const
 
 #if PLATFORM(COCOA) || PLATFORM(EFL) || PLATFORM(GTK)
 
-#define HANGUL_CHOSEONG_START (0x1100)
-#define HANGUL_CHOSEONG_END (0x115F)
-#define HANGUL_JUNGSEONG_START (0x1160)
-#define HANGUL_JUNGSEONG_END (0x11A2)
-#define HANGUL_JONGSEONG_START (0x11A8)
-#define HANGUL_JONGSEONG_END (0x11F9)
-#define HANGUL_SYLLABLE_START (0xAC00)
-#define HANGUL_SYLLABLE_END (0xD7AF)
-#define HANGUL_JONGSEONG_COUNT (28)
-
-enum HangulState {
-    HangulStateL,
-    HangulStateV,
-    HangulStateT,
-    HangulStateLV,
-    HangulStateLVT,
-    HangulStateBreak
-};
+const UChar hangulChoseongStart = 0x1100;
+const UChar hangulChoseongEnd = 0x115F;
+const UChar hangulJungseongStart = 0x1160;
+const UChar hangulJungseongEnd = 0x11A2;
+const UChar hangulJongseongStart = 0x11A8;
+const UChar hangulJongseongEnd = 0x11F9;
+const UChar hangulSyllableStart = 0xAC00;
+const UChar hangulSyllableEnd = 0xD7AF;
+const UChar hangulJongseongCount = 28;
+
+enum class HangulState { L, V, T, LV, LVT, Break };
 
-static inline bool isHangulLVT(UChar32 character)
+static inline bool isHangulLVT(UChar character)
 {
-    return (character - HANGUL_SYLLABLE_START) % HANGUL_JONGSEONG_COUNT;
+    return (character - hangulSyllableStart) % hangulJongseongCount;
 }
 
 static inline bool isMark(UChar32 character)
 {
-    int8_t charType = u_charType(character);
-    return charType == U_NON_SPACING_MARK || charType == U_ENCLOSING_MARK || charType == U_COMBINING_SPACING_MARK;
+    return U_GET_GC_MASK(character) & U_GC_M_MASK;
 }
 
 static inline bool isRegionalIndicator(UChar32 character)
@@ -1433,25 +1425,31 @@ static inline bool isRegionalIndicator(UChar32 character)
     return 0x1F1E6 <= character && character <= 0x1F1FF;
 }
 
+static inline bool isInArmenianToLimbuRange(UChar32 character)
+{
+    return character >= 0x0530 && character < 0x1950;
+}
+
 #endif
 
 int RenderText::previousOffsetForBackwardDeletion(int current) const
 {
-#if PLATFORM(COCOA) || PLATFORM(EFL) || PLATFORM(GTK)
-    ASSERT(m_text);
+    ASSERT(!m_text.isNull());
     StringImpl& text = *m_text.impl();
-    UChar32 character;
+
+    // FIXME: Unclear why this has so much handrolled code rather than using TextBreakIterator.
+    // Also unclear why this is so different from advanceByCombiningCharacterSequence.
+
+    // FIXME: Seems like this fancier case could be used on all platforms now, no
+    // need for the #else case below.
+#if PLATFORM(COCOA) || PLATFORM(EFL) || PLATFORM(GTK)
     bool sawRegionalIndicator = false;
     bool sawEmojiGroupCandidate = false;
     bool sawEmojiModifier = false;
     
     while (current > 0) {
-        if (U16_IS_TRAIL(text[--current]))
-            --current;
-        if (current < 0)
-            break;
-
-        UChar32 character = text.characterStartingAt(current);
+        UChar32 character;
+        U16_PREV(text, 0, current, character);
 
         if (sawEmojiGroupCandidate) {
             sawEmojiGroupCandidate = false;
@@ -1464,9 +1462,13 @@ int RenderText::previousOffsetForBackwardDeletion(int current) const
         }
 
         if (sawEmojiModifier) {
-            if (isEmojiModifier(character))
+            if (isEmojiModifier(character)) {
+                // Don't treat two emoji modifiers in a row as a group.
                 U16_FWD_1_UNSAFE(text, current);
-            break;
+                break;
+            }
+            if (!isVariationSelector(character))
+                break;
         }
 
         if (sawRegionalIndicator) {
@@ -1480,7 +1482,7 @@ int RenderText::previousOffsetForBackwardDeletion(int current) const
         }
 
         // We don't combine characters in Armenian ... Limbu range for backward deletion.
-        if ((character >= 0x0530) && (character < 0x1950))
+        if (isInArmenianToLimbuRange(character))
             break;
 
         if (isRegionalIndicator(character)) {
@@ -1498,7 +1500,8 @@ int RenderText::previousOffsetForBackwardDeletion(int current) const
             continue;
         }
 
-        if (!isMark(character) && (character != 0xFF9E) && (character != 0xFF9F))
+        // FIXME: Why are FF9E and FF9F special cased here?
+        if (!isMark(character) && character != 0xFF9E && character != 0xFF9F)
             break;
     }
 
@@ -1506,55 +1509,50 @@ int RenderText::previousOffsetForBackwardDeletion(int current) const
         return current;
 
     // Hangul
-    character = text.characterStartingAt(current);
-    if (((character >= HANGUL_CHOSEONG_START) && (character <= HANGUL_JONGSEONG_END)) || ((character >= HANGUL_SYLLABLE_START) && (character <= HANGUL_SYLLABLE_END))) {
+    UChar character = text[current];
+    if ((character >= hangulChoseongStart && character <= hangulJongseongEnd) || (character >= hangulSyllableStart && character <= hangulSyllableEnd)) {
         HangulState state;
 
-        if (character < HANGUL_JUNGSEONG_START)
-            state = HangulStateL;
-        else if (character < HANGUL_JONGSEONG_START)
-            state = HangulStateV;
-        else if (character < HANGUL_SYLLABLE_START)
-            state = HangulStateT;
+        if (character < hangulJungseongStart)
+            state = HangulState::L;
+        else if (character < hangulJongseongStart)
+            state = HangulState::V;
+        else if (character < hangulSyllableStart)
+            state = HangulState::T;
         else
-            state = isHangulLVT(character) ? HangulStateLVT : HangulStateLV;
+            state = isHangulLVT(character) ? HangulState::LVT : HangulState::LV;
 
-        while (current > 0 && ((character = text.characterStartingAt(current - 1)) >= HANGUL_CHOSEONG_START) && (character <= HANGUL_SYLLABLE_END) && ((character <= HANGUL_JONGSEONG_END) || (character >= HANGUL_SYLLABLE_START))) {
+        while (current > 0 && (character = text[current - 1]) >= hangulChoseongStart && character <= hangulSyllableEnd && (character <= hangulJongseongEnd || character >= hangulSyllableStart)) {
             switch (state) {
-            case HangulStateV:
-                if (character <= HANGUL_CHOSEONG_END)
-                    state = HangulStateL;
-                else if ((character >= HANGUL_SYLLABLE_START) && (character <= HANGUL_SYLLABLE_END) && !isHangulLVT(character))
-                    state = HangulStateLV;
-                else if (character > HANGUL_JUNGSEONG_END)
-                    state = HangulStateBreak;
+            case HangulState::V:
+                if (character <= hangulChoseongEnd)
+                    state = HangulState::L;
+                else if (character >= hangulSyllableStart && character <= hangulSyllableEnd && !isHangulLVT(character))
+                    state = HangulState::LV;
+                else if (character > hangulJungseongEnd)
+                    state = HangulState::Break;
                 break;
-            case HangulStateT:
-                if ((character >= HANGUL_JUNGSEONG_START) && (character <= HANGUL_JUNGSEONG_END))
-                    state = HangulStateV;
-                else if ((character >= HANGUL_SYLLABLE_START) && (character <= HANGUL_SYLLABLE_END))
-                    state = (isHangulLVT(character) ? HangulStateLVT : HangulStateLV);
-                else if (character < HANGUL_JUNGSEONG_START)
-                    state = HangulStateBreak;
+            case HangulState::T:
+                if (character >= hangulJungseongStart && character <= hangulJungseongEnd)
+                    state = HangulState::V;
+                else if (character >= hangulSyllableStart && character <= hangulSyllableEnd)
+                    state = isHangulLVT(character) ? HangulState::LVT : HangulState::LV;
+                else if (character < hangulJungseongStart)
+                    state = HangulState::Break;
                 break;
             default:
-                state = (character < HANGUL_JUNGSEONG_START) ? HangulStateL : HangulStateBreak;
+                state = (character < hangulJungseongStart) ? HangulState::L : HangulState::Break;
                 break;
             }
-            if (state == HangulStateBreak)
+            if (state == HangulState::Break)
                 break;
-
             --current;
         }
     }
 
     return current;
 #else
-    // Platforms other than Mac delete by one code point.
-    if (U16_IS_TRAIL(m_text[--current]))
-        --current;
-    if (current < 0)
-        current = 0;
+    U16_BACK_1(text, 0, current);
     return current;
 #endif
 }