Fix RTL shaping across text runs

contributors/people.toml

@@ -28,3 +28,6 @@ sync_github_name = false
 
 [people.mo0kid]
 sync_github_name = true
+
+[people.omd0]
+sync_github_name = true

crates/text/src/layout.rs

@@ -761,10 +761,15 @@ fn break_line(text: &str, g: &[SGlyph], i: usize, width: f64, hyphenate: bool, b
     let mut j = i;
     while j < g.len() {
         let gl = &g[j];
-        if j > i && !gl.is_space() && x + gl.adv > width + EPS {
+        let mut cluster_end = j + 1;
+        while g.get(cluster_end).is_some_and(|next| next.byte == gl.byte) {
+            cluster_end += 1;
+        }
+        let cluster_width: f64 = g[j..cluster_end].iter().map(|glyph| glyph.adv).sum();
+        if j > i && !gl.is_space() && x + cluster_width > width + EPS {
             // It ends the line with the spaces after it.
-            if burasagari != Burasagari::None && hangs(gl) && kinsoku_allows(g, j) && g.get(j + 1).is_none_or(|n| n.byte != gl.byte) {
-                let mut end = j + 1;
+            if burasagari != Burasagari::None && hangs(gl) && kinsoku_allows(g, cluster_end - 1) {
+                let mut end = cluster_end;
                 while g.get(end).is_some_and(SGlyph::is_space) {
                     end += 1;
                 }
@@ -772,17 +777,18 @@ fn break_line(text: &str, g: &[SGlyph], i: usize, width: f64, hyphenate: bool, b
             }
             break;
         }
-        x += gl.adv;
-        if gl.break_after() && kinsoku_allows(g, j) {
-            if gl.is_soft_hyphen() {
-                if x + hyphen_glyph(gl).adv <= width + EPS {
-                    last_break = Some((j + 1, true));
+        x += cluster_width;
+        let last = &g[cluster_end - 1];
+        if last.break_after() && kinsoku_allows(g, cluster_end - 1) {
+            if last.is_soft_hyphen() {
+                if x + hyphen_glyph(last).adv <= width + EPS {
+                    last_break = Some((cluster_end, true));
                 }
             } else {
-                last_break = Some((j + 1, false));
+                last_break = Some((cluster_end, false));
             }
         }
-        j += 1;
+        j = cluster_end;
     }
     if j >= g.len() {
         return (g.len(), false);
@@ -805,14 +811,12 @@ fn break_line(text: &str, g: &[SGlyph], i: usize, width: f64, hyphenate: bool, b
             }
         }
     }
-    // Don't separate a cluster's glyphs.
-    let (mut end, hy) = match last_break {
+    // Break candidates and greedy overflow are cluster boundaries; hyphenation also returns only
+    // cluster starts, so the resulting end always keeps each source cluster together.
+    let (end, hy) = match last_break {
         Some(b) if b.0 > i => b,
         _ => (j, false),
     };
-    while end > i + 1 && end < g.len() && g[end].byte == g[end - 1].byte {
-        end -= 1;
-    }
     (end, hy)
 }
 
@@ -835,6 +839,123 @@ fn kinsoku_between(before: char, after: Option<char>) -> bool {
     !no_line_end(before) && after.is_none_or(|c| !no_line_start(c))
 }
 
+#[cfg(test)]
+mod wrapping_tests {
+    use super::*;
+    use std::sync::Arc;
+
+    fn synthetic_glyphs(specs: &[(usize, usize, f64, char)]) -> Vec<SGlyph> {
+        // The face is only a required SGlyph field here; advances and cluster boundaries below are
+        // entirely synthetic, so the regressions don't depend on a particular font's shaping.
+        static DB: std::sync::OnceLock<FontDb> = std::sync::OnceLock::new();
+        let db = DB.get_or_init(|| FontDb::with_font_dirs(vec![]));
+        let face = db.face("Source Sans 3", "Regular").expect("bundled test face");
+        specs
+            .iter()
+            .map(|&(byte, len, adv, ch)| SGlyph {
+                face: Arc::clone(&face),
+                gid: 0,
+                byte,
+                len,
+                run: 0,
+                adv,
+                dx: 0.0,
+                dy: 0.0,
+                sx: 1.0,
+                sy: 1.0,
+                bshift: 0.0,
+                rotation: 0.0,
+                ascent: 8.0,
+                descent: 2.0,
+                leading: 0.0,
+                cap: 6.0,
+                xh: 4.0,
+                ch,
+                tcy: None,
+                lead: 0.0,
+                level: unicode_bidi::Level::ltr(),
+            })
+            .collect()
+    }
+
+    fn source_range(glyphs: &[SGlyph], start: usize, end: usize) -> Range<usize> {
+        let first = glyphs.get(start).expect("nonempty source range");
+        let last = glyphs.get(end - 1).expect("nonempty source range");
+        first.byte..last.byte + last.len
+    }
+
+    #[test]
+    fn greedy_wrap_keeps_an_oversized_first_cluster_and_its_source_range() {
+        let text = "بَت";
+        let glyphs = synthetic_glyphs(&[(0, 4, 2.0, 'ب'), (0, 4, 2.0, 'ب'), (4, 2, 1.0, 'ت')]);
+
+        let (end, hyphenated) = break_line(text, &glyphs, 0, 3.0, false, Burasagari::None);
+
+        assert!(!hyphenated);
+        assert_eq!(end, 2, "oversized base-plus-mark cluster must stay together");
+        assert_eq!(source_range(&glyphs, 0, end), 0..4);
+        assert_eq!(glyphs.get(end).map(|g| g.byte), Some(4));
+    }
+
+    #[test]
+    fn greedy_wrap_keeps_an_oversized_cluster_after_a_line_start_atomic() {
+        let text = "بَتَث";
+        let glyphs = synthetic_glyphs(&[(0, 4, 1.0, 'ب'), (4, 4, 2.0, 'ت'), (4, 4, 2.0, 'ت'), (8, 2, 1.0, 'ث')]);
+
+        let (first_end, _) = break_line(text, &glyphs, 0, 3.0, false, Burasagari::None);
+        assert_eq!(first_end, 1);
+        assert_eq!(source_range(&glyphs, 0, first_end), 0..4);
+
+        let (second_end, hyphenated) = break_line(text, &glyphs, first_end, 3.0, false, Burasagari::None);
+
+        assert!(!hyphenated);
+        assert_eq!(second_end, 3, "oversized cluster at the next line start must stay together");
+        assert_eq!(source_range(&glyphs, first_end, second_end), 4..8);
+        assert_eq!(glyphs.get(second_end).map(|g| g.byte), Some(8));
+    }
+
+    #[test]
+    fn narrow_arabic_area_wrap_keeps_base_and_mark_together_when_supported() {
+        let db = FontDb::global();
+        let Some(face) = db.face_covering('ب').filter(|face| face.covers('َ')) else {
+            // Arabic integration coverage depends on an installed font; synthetic tests above are
+            // the font-independent regression oracle.
+            return;
+        };
+        let text = "بَت";
+        let style = CharStyle { font_family: face.family.clone(), font_style: face.style.clone(), size: 20.0, ..CharStyle::default() };
+        let bidi = para_bidi(text, None);
+        let levels = bidi.as_ref().map_or(&[][..], |info| &info.levels);
+        let mut shaped = Vec::new();
+        shape_range(db, text, 0..text.len(), &[(0..text.len(), &style)], &OtFeatures::default(), levels, &mut shaped);
+        let Some(first) = shaped.first() else { return };
+        let first_end = shaped.iter().take_while(|g| g.byte == first.byte).count();
+        if first.byte != 0 || first_end < 2 || first_end >= shaped.len() {
+            return;
+        }
+        // Pick a narrow width that fits glyphs before one positive-advance glyph but not that next
+        // glyph. This ensures the old per-glyph loop would have returned inside the first cluster.
+        let mut prefix = shaped.first().map_or(0.0, |g| g.adv);
+        let width = (1..first_end).find_map(|k| {
+            let advance = shaped.get(k)?.adv;
+            if prefix > EPS && advance > EPS {
+                Some(prefix)
+            } else {
+                prefix += advance;
+                None
+            }
+        });
+        let Some(width) = width else { return };
+
+        let mut object = TextObject::point(Point::ZERO, text, style);
+        object.kind = TextKind::Area { frame: PathData::from_bezpath(&Rect::new(0.0, 0.0, width, 100.0).to_path(0.1)) };
+        let result = layout_with(db, &object, &LayoutOptions::default());
+
+        let next_cluster_byte = shaped.get(first_end).map(|g| g.byte);
+        assert_eq!(result.lines.first().map(|line| line.end), next_cluster_byte);
+    }
+}
+
 #[cfg(test)]
 #[test]
 fn japanese_letters_for_the_latin_space_are_kana_kanji_and_marks_not_hangul_or_punctuation() {

crates/text/src/shape.rs

@@ -196,20 +196,43 @@ pub(crate) fn shape_range(
     levels: &[Level],
     out: &mut Vec<SGlyph>,
 ) {
+    if text.get(range.clone()).is_none() || range.is_empty() {
+        return;
+    }
     let level_at = |i: usize| levels.get(i - range.start).copied().unwrap_or_else(Level::ltr);
     let output_start = out.len();
+
+    // Text runs also carry non-shaping attributes (fill, stroke, etc.). Coalesce adjacent equal
+    // styles so an editor split does not become an accidental OpenType shaping boundary. Keep
+    // original run spans: each emitted cluster is attributed by its source byte below.
+    let mut spans: Vec<(Range<usize>, &CharStyle, Range<usize>)> = Vec::new();
     for (ri, (rr, st)) in runs.iter().enumerate() {
         let a = rr.start.max(range.start);
         let b = rr.end.min(range.end);
-        if a >= b {
+        if a >= b || text.get(a..b).is_none() {
             continue;
         }
+        if let Some((previous, previous_style, source_runs)) = spans.last_mut()
+            && previous.end == a
+            && *previous_style == *st
+        {
+            previous.end = b;
+            source_runs.end = ri + 1;
+            continue;
+        }
+        spans.push((a..b, *st, ri..ri + 1));
+    }
+
+    for (span, st, source_runs) in spans {
+        let a = span.start;
+        let b = span.end;
         let Some(primary) = db.face(&st.font_family, &st.font_style) else { continue };
         let pmap = primary.skrifa().map(|f| f.charmap());
         // Synthesized Small Caps shape lowercase letters separately (as smaller capitals).
         let small_caps = st.small_caps.is_some() && !st.all_caps;
         // Split into segments by font coverage (and case, for Small Caps).
-        let mut seg = Segment { range: a..a, run: ri, st, face: primary.clone(), small: false, level: level_at(a) };
+        let group_output_start = out.len();
+        let mut seg = Segment { range: a..a, run: source_runs.start, st, face: primary.clone(), small: false, level: level_at(a) };
         let mut cache: Vec<(char, Arc<FontFace>)> = Vec::new();
         let mut script = Script::Common;
         for (i, c) in text[a..b].char_indices() {
@@ -233,7 +256,7 @@ pub(crate) fn shape_range(
             if (face.id() != seg.face.id() || small != seg.small || level != seg.level || script_change) && !is_mark(c) {
                 if i > seg.range.start {
                     seg.range.end = i;
-                    shape_segment(text, &seg, feats, out);
+                    shape_segment(text, &range, &seg, feats, out);
                 }
                 seg = Segment { range: i..i, face, small, level, ..seg };
             }
@@ -243,7 +266,10 @@ pub(crate) fn shape_range(
         }
         if b > seg.range.start {
             seg.range.end = b;
-            shape_segment(text, &seg, feats, out);
+            shape_segment(text, &range, &seg, feats, out);
+        }
+        for glyph in &mut out[group_output_start..] {
+            glyph.run = source_run_at(runs, &source_runs, glyph.byte).unwrap_or(source_runs.start);
         }
     }
     // Right-to-left segments come out of the shaper in visual order: back to logical order for
@@ -255,6 +281,15 @@ pub(crate) fn shape_range(
     }
 }
 
+/// Index in the original run list containing `byte`, limited to one coalesced shaping span.
+/// Source runs are ordered and non-overlapping; partitioning keeps attribution bounded when a
+/// document contains many adjacent style runs.
+fn source_run_at(runs: &[(Range<usize>, &CharStyle)], span: &Range<usize>, byte: usize) -> Option<usize> {
+    let source = runs.get(span.clone())?;
+    let i = source.partition_point(|(range, _)| range.end <= byte);
+    source.get(i).filter(|(range, _)| range.contains(&byte)).map(|_| span.start + i)
+}
+
 /// The script `c` is shaped in: Japanese and Chinese text mixes Han, Hiragana, Katakana and
 /// Bopomofo, shaped together (splitting them would cost a shaper call per change of script).
 fn shaping_script(c: char) -> Script {
@@ -281,7 +316,7 @@ fn is_mark(c: char) -> bool {
         || matches!(c, '\u{200D}' | '\u{FE00}'..='\u{FE0F}')
 }
 
-fn shape_segment(text: &str, seg: &Segment, feats: &OtFeatures, out: &mut Vec<SGlyph>) {
+fn shape_segment(text: &str, context: &Range<usize>, seg: &Segment, feats: &OtFeatures, out: &mut Vec<SGlyph>) {
     let Segment { range, run, st, face, small, level } = seg;
     let (range, run, small) = (range.clone(), *run, *small);
     let text_seg = &text[range.clone()];
@@ -320,6 +355,15 @@ fn shape_segment(text: &str, seg: &Segment, feats: &OtFeatures, out: &mut Vec<SG
         }
         buf.set_direction(if level.is_rtl() { Direction::RightToLeft } else { Direction::LeftToRight });
         buf.guess_segment_properties();
+        // Harfrust keeps this Unicode context for joining/positional shaping, but does not emit
+        // glyphs for it. This retains context at real style/font segmentation boundaries without
+        // shaping across those boundaries or claiming cross-style ligatures/mark attachment.
+        if let Some(pre) = text.get(context.start..range.start) {
+            buf.set_pre_context(pre);
+        }
+        if let Some(post) = text.get(range.end..context.end) {
+            buf.set_post_context(post);
+        }
         let feats: Vec<Feature> = feats.resolve(st);
         let gb = shaper.shape(buf, ShapeOptions::new().features(&feats));
         for (info, pos) in gb.glyph_infos().iter().zip(gb.glyph_positions()) {

crates/text/src/tests_bidi.rs

@@ -1,12 +1,42 @@
 //! Direction, source-cluster and editing regressions; no RTL font assets required.
 use super::*;
 use kurbo::{Point, Rect, Shape};
-use vectorcraft_doc::{CharStyle, TextKind};
+use std::sync::Arc;
+use vectorcraft_doc::{CharStyle, TextKind, TextRun};
 
 fn text_layout(s: &str) -> TextLayout {
     layout(FontDb::global(), &TextObject::point(Point::ZERO, s, CharStyle::default()))
 }
 
+fn arabic_test_face(test: &str) -> Option<Arc<FontFace>> {
+    let db = FontDb::global();
+    let face = ["Geeza Pro", "Arial", "Noto Sans Arabic"]
+        .into_iter()
+        .filter_map(|name| db.face(name, "Regular"))
+        .find(|face| face.covers('ب') && face.covers('ل') && face.covers('ا'));
+    if let Some(face) = &face {
+        let path = face.path().map_or_else(|| "bundled font".to_string(), |p| p.display().to_string());
+        eprintln!("ASSERTIONS RUN: {test}; Arabic font {} {} ({path})", face.family, face.style);
+    } else {
+        eprintln!("SKIPPED: {test}; no installed candidate font covers Arabic beh, lam, and alef");
+    }
+    face
+}
+
+fn arabic_style(face: &FontFace) -> CharStyle {
+    CharStyle { font_family: face.family.clone(), font_style: face.style.clone(), ..CharStyle::default() }
+}
+
+fn assert_same_shaped_glyphs(left: &TextLayout, right: &TextLayout) {
+    assert_eq!(left.glyphs.len(), right.glyphs.len());
+    for (a, b) in left.glyphs.iter().zip(&right.glyphs) {
+        assert_eq!(
+            (a.gid, a.byte, a.len, a.font_id, a.advance, a.origin, a.xf, a.rtl),
+            (b.gid, b.byte, b.len, b.font_id, b.advance, b.origin, b.xf, b.rtl),
+        );
+    }
+}
+
 #[test]
 fn hebrew_and_arabic_are_visually_rtl_with_exact_source_clusters() {
     for s in ["שלום", "مرحبا"] {
@@ -85,10 +115,14 @@ fn hebrew_marks_stay_with_base_clusters() {
 fn arabic_contextual_forms_and_lam_alef_use_the_shaper() {
     let db = FontDb::global();
     // Installed fonts are optional; no proprietary or test font is copied into the repo.
-    let face =
-        ["Geeza Pro", "Arial", "Noto Sans Arabic"].into_iter().filter_map(|name| db.face(name, "Regular")).find(|f| f.covers('ل') && f.covers('ا'));
-    let Some(face) = face else { return };
-    let st = CharStyle { font_family: face.family.clone(), font_style: face.style.clone(), ..CharStyle::default() };
+    let Some(face) = arabic_test_face("arabic_contextual_forms_and_lam_alef_use_the_shaper") else { return };
+    let st = arabic_style(&face);
+    let pair = "لا";
+    let lam_alef = layout(db, &TextObject::point(Point::ZERO, pair, st.clone()));
+    let ligature = lam_alef.glyphs.iter().find(|g| g.byte == 0 && g.len == pair.len());
+    assert!(ligature.is_some(), "lam-alef must retain one cluster spanning both source letters: {lam_alef:?}");
+    assert_ne!(ligature.unwrap().gid, face.glyph_for('ل'), "lam-alef must use a shaped glyph");
+
     for s in ["سلام", "שלום سلام"] {
         let l = layout(db, &TextObject::point(Point::ZERO, s, st.clone()));
         let arabic: Vec<_> = l.glyphs.iter().filter(|g| s.get(g.byte..).is_some_and(|t| t.starts_with(['س', 'ل', 'ا', 'م']))).collect();
@@ -100,6 +134,70 @@ fn arabic_contextual_forms_and_lam_alef_use_the_shaper() {
     }
 }
 
+#[test]
+fn identical_style_arabic_run_splits_keep_joining_and_source_attribution() {
+    let db = FontDb::global();
+    let Some(face) = arabic_test_face("identical_style_arabic_run_splits_keep_joining_and_source_attribution") else { return };
+    let style = arabic_style(&face);
+    let text = "بب";
+
+    let mut single = TextObject::point(Point::ZERO, text, style.clone());
+    single.runs = vec![TextRun { text: text.to_string(), style: style.clone() }];
+    let mut split = TextObject::point(Point::ZERO, text, style.clone());
+    split.runs = vec![TextRun { text: "ب".into(), style: style.clone() }, TextRun { text: "ب".into(), style }];
+
+    let single = layout(db, &single);
+    let split = layout(db, &split);
+    assert!(single.glyphs.iter().any(|glyph| glyph.gid != face.glyph_for('ب')), "single run must exercise contextual beh forms");
+    assert_same_shaped_glyphs(&single, &split);
+    assert_eq!(split.glyphs.len(), 2, "beh-beh should remain two source clusters");
+    for glyph in &split.glyphs {
+        let expected_run = usize::from(glyph.byte >= "ب".len());
+        assert_eq!(glyph.run, expected_run, "source byte {} belongs to run {expected_run}", glyph.byte);
+    }
+}
+
+#[test]
+fn different_style_arabic_boundary_receives_joining_context() {
+    let db = FontDb::global();
+    let Some(face) = arabic_test_face("different_style_arabic_boundary_receives_joining_context") else { return };
+    let style = arabic_style(&face);
+    let text = "بب";
+    let joined = layout(db, &TextObject::point(Point::ZERO, text, style.clone()));
+    assert!(joined.glyphs.iter().any(|glyph| glyph.gid != face.glyph_for('ب')), "reference must use contextual beh forms");
+
+    let mut split = TextObject::point(Point::ZERO, text, style.clone());
+    split.runs =
+        vec![TextRun { text: "ب".into(), style: style.clone() }, TextRun { text: "ب".into(), style: CharStyle { size: style.size * 1.5, ..style } }];
+    let split = layout(db, &split);
+    assert_eq!(joined.glyphs.len(), 2);
+    assert_eq!(split.glyphs.len(), 2);
+    for glyph in &joined.glyphs {
+        let actual = split.glyphs.iter().find(|other| other.byte == glyph.byte).expect("same source cluster");
+        assert_eq!(actual.gid, glyph.gid, "joining form changed at style boundary for byte {}", glyph.byte);
+        assert_eq!(actual.len, glyph.len);
+        assert_eq!(actual.run, usize::from(glyph.byte >= "ب".len()));
+    }
+}
+
+#[test]
+fn shape_range_handles_empty_runs_and_selected_utf8_subranges() {
+    let db = FontDb::global();
+    let style = CharStyle::default();
+    let text = "Aب";
+    let runs = [(0..text.len(), &style)];
+    let mut glyphs = Vec::new();
+
+    shape::shape_range(db, text, 0..0, &runs, &OtFeatures::default(), &[], &mut glyphs);
+    shape::shape_range(db, text, 0..text.len(), &[], &OtFeatures::default(), &[], &mut glyphs);
+    shape::shape_range(db, text, 1..2, &runs, &OtFeatures::default(), &[], &mut glyphs);
+    assert!(glyphs.is_empty());
+
+    shape::shape_range(db, text, 1..text.len(), &runs, &OtFeatures::default(), &[], &mut glyphs);
+    assert!(!glyphs.is_empty());
+    assert!(glyphs.iter().all(|g| (1..text.len()).contains(&g.byte)));
+}
+
 #[test]
 fn bidi_isolates_numbers_and_brackets_keep_valid_clusters() {
     for s in ["שלום (123)", "مرحبا (Rust 42)", "English \u{2067}שלום 123\u{2069} end"] {