Merge pull request #2795 from dotCooCoo/fix/caret-steps-past-zero-width-clusters

Type: Left and Right reach both sides of a character drawn with no width

crates/text/src/lib.rs

@@ -474,23 +474,36 @@ pub fn selection_quads(layout: &TextLayout, a: usize, b: usize) -> Vec<[Point; 4
     out
 }
 
-/// Move horizontally through visual caret stops; source offsets stay in logical order.
+/// Move horizontally through visual caret stops; source offsets stay in logical order. Stops at
+/// one x (both ends of a cluster drawn with no advance) are visited one at a time in byte order.
+/// Going right, the caret takes them in increasing order on left-to-right glyphs and in
+/// decreasing order on right-to-left glyphs; going left, it takes them in the reverse order.
 pub fn caret_horizontal(layout: &TextLayout, byte: usize, right: bool) -> usize {
     let li = layout.line_of(byte);
     let Some(line) = layout.lines.get(li) else { return byte };
-    let x = layout.logical_point(caret_position(layout, byte).0).x;
-    let mut stops: Vec<_> =
-        layout.glyphs.get(line.glyph_start..line.glyph_end).unwrap_or_default().iter().flat_map(|g| [g.byte, g.byte + g.len]).collect();
+    let glyphs = layout.glyphs.get(line.glyph_start..line.glyph_end).unwrap_or_default();
+    // Returns a stop's x and its place among the stops at that x (larger is further right),
+    // taken from the glyph that `caret_position` puts it on.
+    let stop = |b: usize| {
+        let rtl =
+            glyphs.iter().find(|g| g.byte <= b && g.byte + g.len > b).or_else(|| glyphs.iter().max_by_key(|g| g.byte + g.len)).is_some_and(|g| g.rtl);
+        (layout.logical_point(caret_position(layout, b).0).x, if rtl { -(b as f64) } else { b as f64 })
+    };
+    let (x, order) = stop(byte);
+    let mut stops: Vec<_> = glyphs.iter().flat_map(|g| [g.byte, g.byte + g.len]).collect();
     stops.sort_unstable();
     stops.dedup();
     stops
         .into_iter()
         .filter_map(|b| {
-            let bx = layout.logical_point(caret_position(layout, b).0).x;
-            let d = if right { bx - x } else { x - bx };
-            (d > 1e-6).then_some((d, b))
+            // Both the distance and the place among stops at one x are counted in the step's direction.
+            let (bx, bo) = stop(b);
+            let (d, o) = if right { (bx - x, bo - order) } else { (x - bx, order - bo) };
+            // The end of a soft-wrapped line is measured on the next line: it never wins a tie.
+            let o = if layout.line_of(b) == li { o } else { f64::INFINITY };
+            if d.abs() <= 1e-6 { (o > 0.0 && o.is_finite()).then_some((0.0, o, b)) } else { (d > 1e-6).then_some((d, o, b)) }
         })
-        .min_by(|a, b| a.0.total_cmp(&b.0))
+        .min_by(|a, b| if (a.0 - b.0).abs() <= 1e-6 { a.1.total_cmp(&b.1) } else { a.0.total_cmp(&b.0) })
         .map_or_else(
             || {
                 if right != line.rtl {
@@ -499,7 +512,7 @@ pub fn caret_horizontal(layout: &TextLayout, byte: usize, right: bool) -> usize
                     li.checked_sub(1).and_then(|previous| layout.lines.get(previous)).map_or(byte, |previous| previous.end)
                 }
             },
-            |(_, b)| b,
+            |(_, _, b)| b,
         )
 }
 

crates/text/src/tests_bidi.rs

@@ -1,6 +1,6 @@
 //! Direction, source-cluster and editing regressions; no RTL font assets required.
 use super::*;
-use kurbo::{Point, Rect, Shape};
+use kurbo::{Affine, BezPath, Point, Rect, Shape};
 use std::sync::Arc;
 use vectorcraft_doc::{CharStyle, TextKind, TextRun};
 
@@ -103,6 +103,81 @@ fn rtl_hit_testing_arrows_and_selection_follow_visual_positions() {
     assert!(!selection_quads(&l, 0, s.len()).is_empty());
 }
 
+/// Lines of glyphs, one glyph per cluster, each line given in visual order as (byte, len, advance)
+/// and starting at x 0.
+fn lines_of_clusters(lines: &[&[(usize, usize, f64)]], rtl: bool) -> TextLayout {
+    let mut l = TextLayout::default();
+    for (li, clusters) in lines.iter().enumerate() {
+        let (glyph_start, baseline) = (l.glyphs.len(), li as f64 * 20.0);
+        let mut x = 0.0;
+        for &(byte, len, advance) in *clusters {
+            l.glyphs.push(PositionedGlyph {
+                outline: BezPath::new(),
+                run: 0,
+                byte,
+                origin: Point::new(x, baseline),
+                advance,
+                len,
+                angle: 0.0,
+                line: li,
+                font_id: 0,
+                gid: 0,
+                xf: Affine::IDENTITY,
+                rtl,
+            });
+            x += advance;
+        }
+        l.lines.push(LineInfo {
+            rtl,
+            baseline,
+            x0: 0.0,
+            x1: x,
+            ascent: 10.0,
+            descent: 3.0,
+            first_baseline: 0.0,
+            start: clusters.iter().map(|c| c.0).min().unwrap_or(0),
+            end: clusters.iter().map(|c| c.0 + c.1).max().unwrap_or(0),
+            glyph_start,
+            glyph_end: l.glyphs.len(),
+            avail: (0.0, x),
+            region: 0,
+        });
+    }
+    l
+}
+
+fn arrow_steps(l: &TextLayout, from: usize, right: bool, steps: usize) -> Vec<usize> {
+    let mut caret = from;
+    (0..steps)
+        .map(|_| {
+            caret = caret_horizontal(l, caret, right);
+            caret
+        })
+        .collect()
+}
+
+#[test]
+fn arrows_step_through_both_caret_stops_of_a_cluster_without_advance() {
+    // A letter (bytes 0..6), a zero width non-joiner drawn as its own glyph with no advance
+    // (6..9) and a letter (9..12): the stops at 6 and 9 share one x.
+    let l = lines_of_clusters(&[&[(0, 6, 10.0), (6, 3, 0.0), (9, 3, 10.0)]], false);
+    assert_eq!(arrow_steps(&l, 0, true, 4), [6, 9, 12, 12]);
+    assert_eq!(arrow_steps(&l, 12, false, 4), [9, 6, 0, 0]);
+    // The same clusters right to left: the glyphs run from byte 9 at the left to byte 0 at the right.
+    let l = lines_of_clusters(&[&[(9, 3, 10.0), (6, 3, 0.0), (0, 6, 10.0)]], true);
+    assert_eq!(arrow_steps(&l, 12, true, 4), [9, 6, 0, 0]);
+    assert_eq!(arrow_steps(&l, 0, false, 4), [6, 9, 12, 12]);
+}
+
+#[test]
+fn arrows_keep_to_their_line_where_a_wrapped_line_starts_at_the_same_x() {
+    // A soft wrap at byte 2: the end of the first line is the start of the second, at the same x
+    // as the start of the first.
+    let l = lines_of_clusters(&[&[(0, 1, 10.0), (1, 1, 10.0)], &[(2, 1, 10.0), (3, 1, 10.0)]], false);
+    assert_eq!(arrow_steps(&l, 0, true, 4), [1, 2, 3, 4]);
+    assert_eq!(caret_horizontal(&l, 1, false), 0);
+}
+
 #[test]
 fn wrapping_retains_logical_line_ranges_and_reorders_each_line() {
     let s = "שלום עולם שלום עולם שלום עולם";

docs/type-parity.md

@@ -42,7 +42,7 @@ against Illustrator's public documentation; **estimated** unless noted.
 | Hidden characters (Show Hidden Characters) | done | `type.hiddenCharacters` |
 | Spell check (Check Spelling, Auto Spell Check, custom dictionary) | missing | menu stubs; needs an openly licensed dictionary |
 | Smart Punctuation, Change Case, Find & Replace | done | |
-| Caret: the arrows and Delete step over whole clusters (Thai consonant with its marks, combining accents, emoji); Backspace removes one character | done | |
+| Caret: the arrows and Delete step over whole clusters (Thai consonant with its marks, combining accents, emoji); Backspace removes one character | done | in text with right-to-left characters the arrows move in visual order and stop on both sides of a character drawn with no width, such as a zero width non-joiner |
 | Touch Type tool | done | moves, scales and rotates one character through its kerning, baseline shift, scales and rotation (`text.touchType`); the first character of a line and right-to-left characters move only up and down |
 | Retype (identify fonts in outlines/images) | missing | Illustrator's uses a cloud model |
 | Snap to Glyph | missing | menu stub |