From 17b3fbc7cf52f2237e1ce971cbbce2d0cde7caeb Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Fri, 30 Jul 2021 15:17:49 +0200 Subject: [PATCH] Fix cursor navigation when using combining characters The cursor navigation left/right (and subsequently text selection) needs to respect grapheme boundaries. Since we already depend on the unicode-segmentation crate through femtovg, we might as well use the functionality for determining grapheme boundaries from there. The only place where the cursor navigation is allowed to break that is when using backspace, as that allows the user to break glyph clusters. --- sixtyfps_runtime/corelib/Cargo.toml | 1 + sixtyfps_runtime/corelib/items/text.rs | 18 ++++---- tests/cases/text/cursor_move_grapheme.60 | 57 ++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 8 deletions(-) create mode 100644 tests/cases/text/cursor_move_grapheme.60 diff --git a/sixtyfps_runtime/corelib/Cargo.toml b/sixtyfps_runtime/corelib/Cargo.toml index 45edaab80..2035532cf 100644 --- a/sixtyfps_runtime/corelib/Cargo.toml +++ b/sixtyfps_runtime/corelib/Cargo.toml @@ -40,6 +40,7 @@ auto_enums = "0.7" weak-table = "0.3" scopeguard = "1.1.0" cfg-if = "1" +unicode-segmentation = "1.8.0" [target.'cfg(target_arch = "wasm32")'.dependencies] instant = { version = "0.1", features = [ "wasm-bindgen", "now" ] } diff --git a/sixtyfps_runtime/corelib/items/text.rs b/sixtyfps_runtime/corelib/items/text.rs index 4782c10dc..2c4171d52 100644 --- a/sixtyfps_runtime/corelib/items/text.rs +++ b/sixtyfps_runtime/corelib/items/text.rs @@ -432,6 +432,7 @@ impl ItemConsts for TextInput { enum TextCursorDirection { Forward, Backward, + PreviousCharacter, // breaks grapheme boundaries, so only used by delete-previous-char StartOfLine, EndOfLine, } @@ -487,17 +488,17 @@ impl TextInput { let last_cursor_pos = (self.cursor_position() as usize).max(0).min(text.len()); + let mut grapheme_cursor = + unicode_segmentation::GraphemeCursor::new(last_cursor_pos, text.len(), true); + let new_cursor_pos = match direction { TextCursorDirection::Forward => { - let mut i = last_cursor_pos; - loop { - i = i.checked_add(1).unwrap_or_default().min(text.len()); - if text.is_char_boundary(i) { - break i; - } - } + grapheme_cursor.next_boundary(&text, 0).ok().flatten().unwrap_or_else(|| text.len()) } TextCursorDirection::Backward => { + grapheme_cursor.prev_boundary(&text, 0).ok().flatten().unwrap_or(0) + } + TextCursorDirection::PreviousCharacter => { let mut i = last_cursor_pos; loop { i = i.checked_sub(1).unwrap_or_default(); @@ -538,7 +539,8 @@ impl TextInput { self.delete_selection(); return; } - if self.move_cursor(TextCursorDirection::Backward, AnchorMode::MoveAnchor, window) { + if self.move_cursor(TextCursorDirection::PreviousCharacter, AnchorMode::MoveAnchor, window) + { self.delete_char(window); } } diff --git a/tests/cases/text/cursor_move_grapheme.60 b/tests/cases/text/cursor_move_grapheme.60 new file mode 100644 index 000000000..ddaa53bad --- /dev/null +++ b/tests/cases/text/cursor_move_grapheme.60 @@ -0,0 +1,57 @@ +/* LICENSE BEGIN + This file is part of the SixtyFPS Project -- https://sixtyfps.io + Copyright (c) 2021 Olivier Goffart + Copyright (c) 2021 Simon Hausmann + + SPDX-License-Identifier: GPL-3.0-only + This file is also available under commercial licensing terms. + Please contact info@sixtyfps.io for more information. +LICENSE END */ +TestCase := TextInput { + width: 100phx; + height: 100phx; + property test_text: self.text; + property test_cursor_pos: self.cursor_position; + property test_anchor_pos: self.anchor_position; + property has_selection: self.cursor_position != self.anchor_position; + property input_focused: self.has_focus; +} + +/* +```rust + +// from input.rs +const LEFT_CODE: char = '\u{000E}'; // shift out +const BACK_CODE: char = '\u{0007}'; // backspace \b + +let shift_modifier = sixtyfps::re_exports::KeyboardModifiers { + shift: true, + ..Default::default() +}; + +let instance = TestCase::new(); +sixtyfps::testing::send_mouse_click(&instance, 50., 50.); +assert!(instance.get_input_focused()); +assert_eq!(instance.get_test_text(), ""); +sixtyfps::testing::send_keyboard_string_sequence(&instance, "e\u{0301}"); +assert_eq!(instance.get_test_text(), "e\u{0301}"); +assert!(!instance.get_has_selection()); + +// Test that selecting the grapheme works +sixtyfps::testing::set_current_keyboard_modifiers(&instance, shift_modifier); +sixtyfps::testing::send_keyboard_string_sequence(&instance, &LEFT_CODE.to_string()); +sixtyfps::testing::set_current_keyboard_modifiers(&instance, sixtyfps::re_exports::KeyboardModifiers::default()); +assert!(instance.get_has_selection()); +sixtyfps::testing::send_keyboard_string_sequence(&instance, &BACK_CODE.to_string()); + +assert_eq!(instance.get_test_text(), ""); + +sixtyfps::testing::send_keyboard_string_sequence(&instance, "e\u{0301}"); + +// Test that backspace does not operate on the grapheme and just removes the +// diacritic. +sixtyfps::testing::send_keyboard_string_sequence(&instance, &BACK_CODE.to_string()); +assert_eq!(instance.get_test_cursor_pos(), 1); +assert_eq!(instance.get_test_text(), "e"); +``` +*/