From 9e6c115481dad97a9cc04a8df6a108aff11eeac0 Mon Sep 17 00:00:00 2001 From: Emad Fussi <43019044+omd0@users.noreply.github.com> Date: Fri, 9 Oct 2026 00:34:00 +0300 Subject: [PATCH] Fix RTL shaping across text runs --- contributors/people.toml | 3 + crates/text/src/layout.rs | 151 ++++++++++++++++++++++++++++++---- crates/text/src/shape.rs | 54 ++++++++++-- crates/text/src/tests_bidi.rs | 108 ++++++++++++++++++++++-- 4 files changed, 291 insertions(+), 25 deletions(-) diff --git a/contributors/people.toml b/contributors/people.toml index 26f68cda..02f8742e 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -28,3 +28,6 @@ sync_github_name = false [people.mo0kid] sync_github_name = true + +[people.omd0] +sync_github_name = true diff --git a/crates/text/src/layout.rs b/crates/text/src/layout.rs index cbd3e49b..41701c3c 100644 --- a/crates/text/src/layout.rs +++ b/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) -> 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 { + // 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 = 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 { + 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() { diff --git a/crates/text/src/shape.rs b/crates/text/src/shape.rs index 98dd71f1..f14f9ed1 100644 --- a/crates/text/src/shape.rs +++ b/crates/text/src/shape.rs @@ -196,20 +196,43 @@ pub(crate) fn shape_range( levels: &[Level], out: &mut Vec, ) { + 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, &CharStyle, Range)> = 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)> = 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, &CharStyle)], span: &Range, byte: usize) -> Option { + 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) { +fn shape_segment(text: &str, context: &Range, seg: &Segment, feats: &OtFeatures, out: &mut Vec) { 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 = feats.resolve(st); let gb = shaper.shape(buf, ShapeOptions::new().features(&feats)); for (info, pos) in gb.glyph_infos().iter().zip(gb.glyph_positions()) { diff --git a/crates/text/src/tests_bidi.rs b/crates/text/src/tests_bidi.rs index b912affe..0f088423 100644 --- a/crates/text/src/tests_bidi.rs +++ b/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> { + 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"] {