From 443de79e2ff81faea25efdb730e909d4871c6492 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Sat, 19 Sep 2026 02:11:40 -0400 Subject: [PATCH] fix(textarea): Up and Down land on the soft-wrapped row they target A soft-wrapped row and the next row share a byte: the row's exclusive end is the next row's start. Up or Down with a goal column past the target row's width put the cursor on that byte, so it drew on the wrong row. Up made no progress and Down skipped a row. `row_with_end` tells a soft wrap from a hard break (byte-adjacent rows are soft). `move_to_display_col_on_line` takes the row and its `RowEnd` and clamps a soft row to its last grapheme, or to the start of an element that closes the row. A hard-break row still reaches its newline. `clamp_to_line` now cuts on graphemes, not chars, so a ZWJ cluster stays whole. Kigi differs from upstream's premise that elements wrap whole: with no display override in the buffer, `wrapped_lines` uses plain textwrap and an element can span rows. Snapping to such an element's start would leave the target row (Down from byte 0 stuck at 0), so an element that began on an earlier row keeps the boundary `display_col_to_buffer_pos` chose, as before this change. `wrapped_navigation_with_zwj_graphemes` asserted the bug: Up from the end stayed on the last row (byte 22). It now expects row 1's cluster start (byte 11), as upstream does. Tests: upstream's four cases, the row-spanning element, and a seeded property test (500 cases): each Up or Down moves exactly one row. The property test fails with the clamp disabled. Ported from upstream (grok-build ratatui-textarea/textarea.rs, 1.0.28). --- .../kigi-ratatui-textarea/src/textarea.rs | 118 ++++++++++------- .../src/textarea/soft_wrap_nav_tests.rs | 121 ++++++++++++++++++ 2 files changed, 191 insertions(+), 48 deletions(-) create mode 100644 crates/codegen/kigi-ratatui-textarea/src/textarea/soft_wrap_nav_tests.rs diff --git a/crates/codegen/kigi-ratatui-textarea/src/textarea.rs b/crates/codegen/kigi-ratatui-textarea/src/textarea.rs index f26b5e7..05de93a 100644 --- a/crates/codegen/kigi-ratatui-textarea/src/textarea.rs +++ b/crates/codegen/kigi-ratatui-textarea/src/textarea.rs @@ -267,6 +267,13 @@ struct WrapCache { lines: Vec>, } +/// Whether a wrapped row's exclusive end is a cursor position on that row. +#[derive(Debug, Clone, Copy)] +enum RowEnd { + HardBreak, + SoftWrap, +} + #[derive(Debug, Default, Clone, Copy)] pub struct TextAreaState { /// Index into wrapped lines of the first visible line. @@ -382,27 +389,13 @@ impl TextArea { } } - /// Clamp a buffer position so it stays within a wrapped line's range - /// `[line_start, line_end)`. Without this, `display_col_to_buffer_pos` - /// can return `line_end` when the column exceeds the line's display - /// width — and `line_end` equals the *next* wrapped line's start, - /// which confuses `effective_scroll` into thinking the cursor hasn't - /// actually moved to the target line. - /// - /// Uses `self.text` to find the last valid char boundary inside the line - /// so we never land in the middle of a multi-byte character. + /// Clamp `pos` to the start of the last grapheme in `[line_start, line_end)`. fn clamp_to_line(&self, pos: usize, line_start: usize, line_end: usize) -> usize { - if line_end > line_start { - // Find the start of the last character in the line. - let last_char_start = self.text[line_start..line_end] - .char_indices() - .next_back() - .map(|(i, _)| line_start + i) - .unwrap_or(line_start); - pos.min(last_char_start) - } else { - line_start - } + let last_grapheme_start = self.text[line_start..line_end] + .grapheme_indices(true) + .next_back() + .map_or(line_start, |(i, _)| line_start + i); + pos.min(last_grapheme_start) } pub fn new() -> Self { @@ -1699,6 +1692,35 @@ impl TextArea { if idx == 0 { None } else { Some(idx - 1) } } + /// Soft-wrapped rows are byte-adjacent; a hard row leaves its newline between it and the next. + fn row_with_end(lines: &[Range], idx: usize) -> Option<(Range, RowEnd)> { + let row = lines.get(idx)?; + let end_kind = match lines.get(idx + 1) { + Some(next) if next.start == row.end => RowEnd::SoftWrap, + _ => RowEnd::HardBreak, + }; + Some((row.clone(), end_kind)) + } + + fn clamp_to_row(&self, pos: usize, row: &Range, end_kind: RowEnd) -> usize { + match end_kind { + RowEnd::HardBreak => pos, + RowEnd::SoftWrap => { + let last_grapheme_start = self.clamp_to_line(pos, row.start, row.end); + let Some(idx) = self.find_element_containing(last_grapheme_start) else { + return last_grapheme_start; + }; + let element_start = self.elements[idx].range.start; + // A display-less element wraps as text; one that began on an earlier row keeps `pos`. + if element_start < row.start { + pos + } else { + element_start + } + } + } + } + /// Map a display column to a buffer byte position on a given wrapped line. /// /// Pure query — does not mutate any state. Handles elements (snapping to @@ -1791,13 +1813,12 @@ impl TextArea { fn move_to_display_col_on_line( &mut self, - line_start: usize, - line_end: usize, + row: Range, target_col: usize, + end_kind: RowEnd, ) { - self.cursor_pos = self - .display_col_to_buffer_pos(line_start, line_end, target_col) - .0; + let (pos, _hit_element) = self.display_col_to_buffer_pos(row.start, row.end, target_col); + self.cursor_pos = self.clamp_to_row(pos, &row, end_kind); } fn beginning_of_line(&self, pos: usize) -> usize { @@ -2652,14 +2673,10 @@ impl TextArea { let target_col = self.preferred_col.unwrap_or_else(|| { self.display_width_of_range(cur_range.start, self.cursor_pos) }); - if idx > 0 { - let prev = &lines[idx - 1]; - let line_start = prev.start; - let line_end = prev.end; - Some((target_col, Some((line_start, line_end)))) - } else { - Some((target_col, None)) - } + let prev = idx + .checked_sub(1) + .and_then(|prev_idx| Self::row_with_end(lines, prev_idx)); + Some((target_col, prev)) } else { None } @@ -2669,11 +2686,11 @@ impl TextArea { } { // We had wrapping info. Apply movement accordingly. match maybe_line { - Some((line_start, line_end)) => { + Some((prev, end_kind)) => { if self.preferred_col.is_none() { self.preferred_col = Some(target_col); } - self.move_to_display_col_on_line(line_start, line_end, target_col); + self.move_to_display_col_on_line(prev, target_col, end_kind); return; } None => { @@ -2697,7 +2714,11 @@ impl TextArea { }; let prev_line_start = self.text[..prev_nl].rfind('\n').map(|i| i + 1).unwrap_or(0); let prev_line_end = prev_nl; - self.move_to_display_col_on_line(prev_line_start, prev_line_end, target_col); + self.move_to_display_col_on_line( + prev_line_start..prev_line_end, + target_col, + RowEnd::HardBreak, + ); } else { self.cursor_pos = 0; self.preferred_col = None; @@ -2716,14 +2737,7 @@ impl TextArea { let target_col = self.preferred_col.unwrap_or_else(|| { self.display_width_of_range(cur_range.start, self.cursor_pos) }); - if idx + 1 < lines.len() { - let next = &lines[idx + 1]; - let line_start = next.start; - let line_end = next.end; - Some((target_col, Some((line_start, line_end)))) - } else { - Some((target_col, None)) - } + Some((target_col, Self::row_with_end(lines, idx + 1))) } else { None } @@ -2732,11 +2746,11 @@ impl TextArea { } } { match move_to_last { - Some((line_start, line_end)) => { + Some((next, end_kind)) => { if self.preferred_col.is_none() { self.preferred_col = Some(target_col); } - self.move_to_display_col_on_line(line_start, line_end, target_col); + self.move_to_display_col_on_line(next, target_col, end_kind); return; } None => { @@ -2766,7 +2780,11 @@ impl TextArea { .find('\n') .map(|i| i + next_line_start) .unwrap_or(self.text.len()); - self.move_to_display_col_on_line(next_line_start, next_line_end, target_col); + self.move_to_display_col_on_line( + next_line_start..next_line_end, + target_col, + RowEnd::HardBreak, + ); } else { self.cursor_pos = self.text.len(); self.preferred_col = None; @@ -3899,6 +3917,9 @@ fn truncate_line_display(line: &Line<'static>, max_width: usize) -> Line<'static Line::from(new_spans) } +#[cfg(test)] +mod soft_wrap_nav_tests; + #[cfg(test)] mod tests { use super::*; @@ -6293,6 +6314,7 @@ mod tests { assert_eq!(t.cursor(), "👍👍".len()); } + /// Each ZWJ cluster fills a 4-col row; Up from the end lands on row 1's cluster start. #[test] fn wrapped_navigation_with_zwj_graphemes() { let grapheme = "👩\u{200D}💻"; @@ -6306,7 +6328,7 @@ mod tests { assert!(pos_after_down >= grapheme.len() * 2); t.move_cursor_up(); - assert_eq!(t.cursor(), grapheme.len() * 2); + assert_eq!(t.cursor(), grapheme.len()); } #[test] diff --git a/crates/codegen/kigi-ratatui-textarea/src/textarea/soft_wrap_nav_tests.rs b/crates/codegen/kigi-ratatui-textarea/src/textarea/soft_wrap_nav_tests.rs new file mode 100644 index 0000000..66c3ceb --- /dev/null +++ b/crates/codegen/kigi-ratatui-textarea/src/textarea/soft_wrap_nav_tests.rs @@ -0,0 +1,121 @@ +//! Up/Down across soft-wrapped rows: a soft row's exclusive end is the next row's start. + +use rand::prelude::*; +use ratatui::layout::Rect; +use ratatui::text::Line; + +use super::{ElementKind, TextArea}; + +fn ta_with(text: &str) -> TextArea { + let mut t = TextArea::new(); + t.insert_str(text); + t +} + +fn cursor_row(t: &TextArea, width: u16) -> usize { + TextArea::wrapped_line_index_by_start(&t.wrapped_lines(width), t.cursor()).unwrap() +} + +#[test] +fn up_from_wide_row_lands_on_last_char_of_narrower_soft_row() { + let width = 8; + let mut t = ta_with("aaa bb ccccccc"); + assert_eq!(vec![0..7, 7..14], *t.wrapped_lines(width)); + + t.move_cursor_up(); + assert_eq!(6, t.cursor()); + assert_eq!(0, cursor_row(&t, width)); + assert_eq!(Some((6, 0)), t.cursor_pos(Rect::new(0, 0, width, 5))); + + t.move_cursor_down(); + assert_eq!(14, t.cursor()); + + t.move_cursor_up(); + assert_eq!(6, t.cursor()); + t.move_cursor_up(); + assert_eq!(0, t.cursor()); +} + +#[test] +fn up_onto_newline_terminated_row_lands_on_the_newline() { + let mut t = ta_with("ab\ncdef"); + assert_eq!(vec![0..2, 3..7], *t.wrapped_lines(8)); + + t.move_cursor_up(); + assert_eq!(2, t.cursor()); + assert_eq!(0, cursor_row(&t, 8)); +} + +#[test] +fn down_from_wide_row_lands_on_last_char_of_narrower_soft_row() { + let mut t = ta_with("ccccccc aaa bb ddddddd"); + assert_eq!(vec![0..8, 8..15, 15..22], *t.wrapped_lines(8)); + t.set_cursor(7); + + t.move_cursor_down(); + assert_eq!(14, t.cursor()); + assert_eq!(1, cursor_row(&t, 8)); + + t.move_cursor_down(); + assert_eq!(22, t.cursor()); +} + +#[test] +fn up_onto_soft_row_closed_by_element_lands_before_the_element() { + let mut t = TextArea::new(); + t.insert_element("xyz", ElementKind(0), Some(Line::from("[ELEM]"))); + t.insert_str("longword"); + assert_eq!(vec![0..3, 3..11], *t.wrapped_lines(8)); + + t.move_cursor_up(); + assert_eq!(0, t.cursor()); + assert_eq!(0, cursor_row(&t, 8)); +} + +#[test] +fn down_through_element_spanning_rows_reaches_the_end() { + let mut t = TextArea::new(); + t.insert_element("abcdefgh", ElementKind(0), None); + t.insert_str("z"); + assert_eq!(vec![0..3, 3..6, 6..9], *t.wrapped_lines(3)); + t.set_cursor(0); + + t.move_cursor_down(); + assert_eq!(8, t.cursor()); +} + +#[test] +fn up_and_down_move_one_row_at_a_time() { + let mut rng = StdRng::seed_from_u64(7); + for _ in 0..500 { + let words = rng.random_range(1..12); + let text = (0..words) + .map(|_| "x".repeat(rng.random_range(1..9))) + .collect::>() + .join(" "); + let width = rng.random_range(3..12); + let mut t = ta_with(&text); + let rows = t.wrapped_lines(width).len(); + t.set_cursor(rng.random_range(0..=text.len())); + + let mut row = cursor_row(&t, width); + while row + 1 < rows { + t.move_cursor_down(); + assert_eq!( + row + 1, + cursor_row(&t, width), + "down in {text:?} at width {width}" + ); + row += 1; + } + while row > 0 { + t.move_cursor_up(); + assert_eq!( + row - 1, + cursor_row(&t, width), + "up in {text:?} at width {width}" + ); + row -= 1; + } + } +}