From 97a09e52878295fb68319ba306f302aa9e4dd6c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20H=C3=B6rmann?= Date: Thu, 16 Jul 2026 12:46:58 +0200 Subject: [PATCH] Delete one grapheme cluster in `backdelete` and `delete` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backspace and forward delete should each remove one grapheme cluster β€” the unit a reader perceives as a character. Both removed less, so a single perceived character took several presses to erase. `backdelete` deleted the whole upstream cluster only for a hard line break or an emoji cluster, and otherwise fell back to `char_indices().next_back()`, i.e. one codepoint. `delete` had no special case at all: it removed the downstream *layout* cluster, which comes from shaping and so tracks the font's glyphs rather than perceived characters β€” splitting a grapheme wherever the font does not ligate the sequence, and splitting a CRLF pair in two. Presses to erase a single grapheme, before β†’ after: | input | backdelete | delete | | ---------------------------------------- | ---------- | ------ | | `πŸ‘¨β€πŸ‘©β€πŸ‘§β€πŸ‘¦` ZWJ family | 7 β†’ 1 | 7 β†’ 1 | | `πŸ‡―πŸ‡΅` regional-indicator flag | 2 β†’ 1 | 2 β†’ 1 | | `❀️` `U+2764 U+FE0F` | 2 β†’ 1 | 2 β†’ 1 | | `πŸ‘‹πŸ½` waving hand + skin tone | 1 β†’ 1 | 2 β†’ 1 | | `e` + combining acute `U+0301` | 2 β†’ 1 | 2 β†’ 1 | | `각` Hangul jamo `U+1100 U+1161 U+11A8` | 1 β†’ 1 | 3 β†’ 1 | | `CRLF` | 1 β†’ 1 | 2 β†’ 1 | Note the last three rows are not emoji at all, so no amount of refining the emoji check would have fixed them: the operations were reaching for the wrong unit. Segment on grapheme-cluster boundaries instead, using the ICU grapheme segmenter already reachable via `AnalysisDataSources`. The `is_hard_line_break() || is_emoji()` special case then becomes unnecessary and is dropped β€” both are simply graphemes. Add tests covering each case above in both directions, plus a guard that two separate graphemes still take two presses. Co-authored-by: Claude Opus 4.8 (1M context) --- parley/src/editing/editor.rs | 87 ++++++++++++++++++++++++++---------- parley_tests/tests/editor.rs | 76 +++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 24 deletions(-) diff --git a/parley/src/editing/editor.rs b/parley/src/editing/editor.rs index 9582ef628..504278c66 100644 --- a/parley/src/editing/editor.rs +++ b/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( + layout_cx: &LayoutContext, + text: &str, +) -> Option { + 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( + layout_cx: &LayoutContext, + text: &str, + from: usize, +) -> Option { + 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); diff --git a/parley_tests/tests/editor.rs b/parley_tests/tests/editor.rs index 25dfc8bfa..4d79c3b14 100644 --- a/parley_tests/tests/editor.rs +++ b/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"); +}