Delete one grapheme cluster in backdelete and delete

parley/src/editing/editor.rs

@@ -22,6 +22,44 @@ use crate::layout::LayoutAccessibility;
 #[cfg(feature = "accesskit")]
 use accesskit::{Node, NodeId, TreeUpdate};
 
+/// The byte offset of the last grapheme-cluster boundary strictly before the end
+/// of `text`, i.e. the start of the grapheme a backspace at the end should
+/// remove. Returns `None` for empty text.
+///
+/// Grapheme clusters are what a reader perceives as single characters, so they
+/// are the unit editing operates in — as distinct from `char`s (a ZWJ emoji
+/// family is seven of them) and from layout clusters (which follow the font's
+/// glyphs).
+fn previous_grapheme_boundary<B: Brush>(
+    layout_cx: &LayoutContext<B>,
+    text: &str,
+) -> Option<usize> {
+    if text.is_empty() {
+        return None;
+    }
+    layout_cx
+        .analysis_data_sources
+        .grapheme_segmenter()
+        .segment_str(text)
+        .take_while(|boundary| *boundary < text.len())
+        .last()
+}
+
+/// The byte offset of the first grapheme-cluster boundary strictly after `from`
+/// in `text`, i.e. the end of the grapheme a forward delete at `from` should
+/// remove. Returns `None` at the end of the text.
+fn next_grapheme_boundary<B: Brush>(
+    layout_cx: &LayoutContext<B>,
+    text: &str,
+    from: usize,
+) -> Option<usize> {
+    layout_cx
+        .analysis_data_sources
+        .grapheme_segmenter()
+        .segment_str(text)
+        .find(|boundary| *boundary > from)
+}
+
 /// Opaque representation of a generation.
 ///
 /// Obtained from [`PlainEditor::generation`].
@@ -243,16 +281,14 @@ where
     /// Delete the selection or the next cluster (typical ‘delete’ behavior).
     pub fn delete(&mut self) {
         if self.editor.selection.is_collapsed() {
-            // Upstream cluster range
-            if let Some(range) = self
-                .editor
-                .selection
-                .focus()
-                .logical_clusters(&self.editor.layout)[1]
-                .as_ref()
-                .map(|cluster| cluster.text_range())
-                .and_then(|range| (!range.is_empty()).then_some(range))
-            {
+            // Delete one grapheme cluster forward, mirroring `backdelete`. The
+            // downstream layout cluster is not the right unit: it follows the
+            // font's glyphs, so it splits a grapheme wherever the font does not
+            // ligate it — and, unlike `backdelete`, this path never even had the
+            // newline special-case, so it could split a CRLF pair in two.
+            let start = self.editor.selection.focus().index();
+            if let Some(end) = next_grapheme_boundary(self.layout_cx, &self.editor.buffer, start) {
+                let range = start..end;
                 self.editor.buffer.replace_range(range.clone(), "");
                 self.editor.update_compose_for_replaced_range(range, 0);
                 self.update_layout();
@@ -295,20 +331,23 @@ where
             {
                 let range = cluster.text_range();
                 let end = range.end;
-                let start = if cluster.is_hard_line_break() || cluster.is_emoji() {
-                    // For newline sequences and emoji, delete the previous cluster
-                    range.start
-                } else {
-                    // Otherwise, delete the previous character
-                    let Some((start, _)) = self
-                        .editor
-                        .buffer
-                        .get(..end)
-                        .and_then(|str| str.char_indices().next_back())
-                    else {
-                        return;
-                    };
-                    start
+                // Delete one *grapheme cluster*: that is the unit a reader
+                // perceives as a character, and what backspace is expected to
+                // remove. Deleting one `char` instead peels a grapheme apart one
+                // codepoint at a time — a ZWJ emoji family takes seven presses,
+                // a regional-indicator flag two, and even `e` + a combining
+                // acute two.
+                //
+                // The layout cluster is not usable for this: it comes from
+                // shaping, so it tracks glyphs rather than perceived characters
+                // and splits wherever the font does not ligate the sequence.
+                let Some(start) = self
+                    .editor
+                    .buffer
+                    .get(..end)
+                    .and_then(|text| previous_grapheme_boundary(self.layout_cx, text))
+                else {
+                    return;
                 };
                 self.editor.buffer.replace_range(start..end, "");
                 self.editor.update_compose_for_replaced_range(start..end, 0);

parley_tests/tests/editor.rs

@@ -116,3 +116,79 @@ fn editor_insert_regular_text_set_upstream_affinity() {
     assert_eq!(sel.focus().index(), 2);
     assert_eq!(sel.focus().affinity(), Affinity::Upstream);
 }
+
+/// Backspace must delete exactly one grapheme cluster — the unit a reader
+/// perceives as a character — not one `char`.
+///
+/// The cases are deliberately not all emoji: `e` + a combining acute and a
+/// Hangul syllable built from jamo are ordinary text that was equally affected,
+/// which is why this is grapheme segmentation rather than an emoji special case.
+#[test]
+fn editor_backdelete_deletes_one_grapheme() {
+    let mut env = TestEnv::new(test_name!(), None);
+    for (name, text) in [
+        // Family: man, ZWJ, woman, ZWJ, girl, ZWJ, boy — one grapheme of 7 chars.
+        ("zwj family", "\u{1F468}\u{200D}\u{1F469}\u{200D}\u{1F467}\u{200D}\u{1F466}"),
+        // Flag: two regional indicators.
+        ("regional indicator flag", "\u{1F1EF}\u{1F1F5}"),
+        // Heart plus the emoji presentation selector.
+        ("emoji presentation selector", "\u{2764}\u{FE0F}"),
+        // Waving hand plus a skin-tone modifier.
+        ("skin tone modifier", "\u{1F44B}\u{1F3FD}"),
+        // Not emoji: `e` plus a combining acute accent.
+        ("combining mark", "e\u{301}"),
+        // Not emoji: Hangul jamo composing one syllable.
+        ("hangul jamo", "\u{1100}\u{1161}\u{11A8}"),
+        // Not emoji: a CRLF pair is a single grapheme.
+        ("crlf", "\r\n"),
+    ] {
+        let mut editor = env.editor(text);
+        {
+            let mut drv = env.driver(&mut editor);
+            drv.move_to_text_end();
+            drv.backdelete();
+        }
+        assert_eq!(editor.text().to_string(), "", "backdelete: {name}");
+    }
+}
+
+/// Forward delete must likewise remove exactly one grapheme cluster, mirroring
+/// [`editor_backdelete_deletes_one_grapheme`].
+#[test]
+fn editor_delete_deletes_one_grapheme() {
+    let mut env = TestEnv::new(test_name!(), None);
+    for (name, text) in [
+        ("zwj family", "\u{1F468}\u{200D}\u{1F469}\u{200D}\u{1F467}\u{200D}\u{1F466}"),
+        ("regional indicator flag", "\u{1F1EF}\u{1F1F5}"),
+        ("emoji presentation selector", "\u{2764}\u{FE0F}"),
+        ("skin tone modifier", "\u{1F44B}\u{1F3FD}"),
+        ("combining mark", "e\u{301}"),
+        ("hangul jamo", "\u{1100}\u{1161}\u{11A8}"),
+        ("crlf", "\r\n"),
+    ] {
+        let mut editor = env.editor(text);
+        {
+            let mut drv = env.driver(&mut editor);
+            drv.move_to_text_start();
+            drv.delete();
+        }
+        assert_eq!(editor.text().to_string(), "", "delete: {name}");
+    }
+}
+
+/// Deleting must still stop at each grapheme rather than swallowing the line:
+/// two separate graphemes take two presses in either direction.
+#[test]
+fn editor_delete_stops_at_grapheme_boundaries() {
+    let mut env = TestEnv::new(test_name!(), None);
+
+    let mut editor = env.editor("ab");
+    env.driver(&mut editor).move_to_text_end();
+    env.driver(&mut editor).backdelete();
+    assert_eq!(editor.text().to_string(), "a");
+
+    let mut editor = env.editor("ab");
+    env.driver(&mut editor).move_to_text_start();
+    env.driver(&mut editor).delete();
+    assert_eq!(editor.text().to_string(), "b");
+}