Fix crash when selecting a character before the punctuation sign (#1151)

## Proposed Changes Fix OutOfBounds logical error leading to crash. Refactor testing of caret placement to be more apparent. ## Testing Test: CupertinoTextFieldDelegateTest.determineCursorDesiredOffset_tap_before_punctuation ## Issues Fixed Fixes: JetBrains/compose-multiplatform#4388 ## Note This PR fixes the crash, the behavior is still incorrect. Consult with code owner @mazunin-v-jb.
JetBrains · Feb 29, 2024 · 8407410 · 8407410
1 parent c148554
commit 8407410
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 85 deletions.
diff --git a/...n/foundation/src/skikoMain/kotlin/androidx/compose/foundation/text/StringHelpers.skiko.kt b/...n/foundation/src/skikoMain/kotlin/androidx/compose/foundation/text/StringHelpers.skiko.kt
@@ -121,8 +121,9 @@ internal fun findNextNonWhitespaceSymbolsSubsequenceStartOffset(
     }
     currentOffset = nextOffset
 
+    nextOffset = charIterator.next()
+
     while (nextOffset != BreakIterator.DONE) {
-        nextOffset = charIterator.next()
         if (currentText.codePointAt(currentOffset).isWhitespace() && !currentText.codePointAt(
                 nextOffset
             ).isWhitespace()
@@ -131,6 +132,8 @@ internal fun findNextNonWhitespaceSymbolsSubsequenceStartOffset(
         } else {
             currentOffset = nextOffset
         }
+
+        nextOffset = charIterator.next()
     }
     return currentOffset
 }

diff --git a/...n/src/skikoTest/kotlin/androidx/compose/foundation/text/CupertinoTextFieldDelegateTest.kt b/...n/src/skikoTest/kotlin/androidx/compose/foundation/text/CupertinoTextFieldDelegateTest.kt
@@ -43,83 +43,84 @@ class CupertinoTextFieldDelegateTest {
     private val defaultDensity = Density(density = 1f)
     private val fontFamilyResolver = createFontFamilyResolver()
 
-    @Test
-    fun determineCursorDesiredOffset_tap_in_the_middle_of_the_word() {
-        val givenOffset = 3
-        val desiredOffset = 4
-        val result = determineCursorDesiredOffset(
+    fun testDetermineCursorDesiredOffset(
+        givenOffset: Int,
+        expected: Int,
+        sampleString: String,
+        cursorOffset: Int? = null
+    ) {
+        val actual = determineCursorDesiredOffset(
             offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = null),
+            createSimpleTextFieldValue(text = sampleText, cursorOffset = cursorOffset),
             textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
             currentText = sampleText
         )
-        assertEquals(result, desiredOffset)
+
+        // add the substring "<here>" at the expected and actual offsets
+        val expectedString = sampleString.substring(0, expected) + "<expected here($expected)>" + sampleString.substring(expected)
+        val actualString = sampleString.substring(0, actual) + "<got here($actual)>" + sampleString.substring(actual)
+
+        assertEquals(expected, actual, "$expectedString\n<${"=".repeat(20)}>\n$actualString")
+    }
+
+    @Test
+    fun determineCursorDesiredOffset_tap_in_the_middle_of_the_word() {
+        val givenOffset = 3
+        val desiredOffset = 4
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText)
+    }
+
+    // TODO: ticket for reviewing our approach will be made and covered in this test
+    @Test
+    fun determineCursorDesiredOffset_tap_before_punctuation() {
+        // https://github.com/JetBrains/compose-multiplatform/issues/4388
+        val text = "0@1"
+        val givenOffset = 1
+        val desiredOffset = 0 // TODO: define what should happen exactly, this test only assures that no crash happens
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, text)
     }
 
     @Test
     fun determineCursorDesiredOffset_tap_in_the_first_half_of_word() {
         val givenOffset = 23
         val desiredOffset = 19
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = null),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText)
     }
 
     @Test
     fun determineCursorDesiredOffset_tap_in_the_middle_of_the_symbols_sequence() {
         val givenOffset = 34
         val desiredOffset = 53
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = null),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText)
     }
 
     @Test
     fun determineCursorDesiredOffset_tap_in_the_middle_of_the_whitespaces() {
         val givenOffset = 48
         val desiredOffset = 53
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = null),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText)
     }
 
     @Test
     fun determineCursorDesiredOffset_tap_on_the_standalone_symbols_sequence() {
         val givenOffset = 56
         val desiredOffset = 57
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = null),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText)
     }
 
     @Test
     fun determineCursorDesiredOffset_tap_on_the_right_edge() {
         // Tap was on the first line in the given example string
         val givenOffset = 57
         val desiredOffset = 57
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = null),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText)
     }
 
     @Test
@@ -128,27 +129,17 @@ class CupertinoTextFieldDelegateTest {
         // Tap was on the last line in the given example string
         val givenOffset = 231
         val desiredOffset = 231
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = null),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText)
     }
 
     @Test
     fun determineCursorDesiredOffset_tap_on_the_left_edge() {
         // Tap was on the second line in the given example string
         val givenOffset = 58
         val desiredOffset = 58
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = null),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText)
     }
 
     @Test
@@ -157,39 +148,24 @@ class CupertinoTextFieldDelegateTest {
         // Tap was on the last line in the given example string
         val givenOffset = 231
         val desiredOffset = 231
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = null),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText)
     }
 
     @Test
     fun determineCursorDesiredOffset_tap_on_the_caret_at_the_same_position() {
         val givenOffset = 24
         val desiredOffset = 24
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = 24),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText, 24)
     }
 
     @Test
     fun determineCursorDesiredOffset_tap_between_two__emoji() {
         val givenOffset = 218
         val desiredOffset = 218
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = 24),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText, 24)
     }
 
     // TODO: remove ignore after fix two compound emojis considered as one whole word
@@ -198,13 +174,8 @@ class CupertinoTextFieldDelegateTest {
     fun determineCursorDesiredOffset_tap_between_two_compound_emoji() {
         val givenOffset = 96
         val desiredOffset = 96
-        val result = determineCursorDesiredOffset(
-            offset = givenOffset,
-            createSimpleTextFieldValue(text = sampleText, cursorOffset = 24),
-            textLayoutResult = createSimpleTextLayoutResultProxy(sampleText),
-            currentText = sampleText
-        )
-        assertEquals(result, desiredOffset)
+
+        testDetermineCursorDesiredOffset(givenOffset, desiredOffset, sampleText, 24)
     }
 
     private fun createSimpleTextFieldValue(text: String, cursorOffset: Int?) =