Fix terminal combining marks (#53176)
Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Closes #46942 ## Problem The terminal's monospace grid forcing logic in `LineLayoutCache` breaks rendering of combining marks in complex scripts. Thai, Lao, Arabic, Hebrew, Myanmar, Khmer, and other scripts use combining characters (vowels, tone marks, diacritics) that should render on top of or below their base character — not in a separate cell. The root cause is in `apply_force_width` within `line_layout.rs`. After HarfBuzz correctly shapes the text (positioning combining marks at the same x coordinate as their base character), the force-width loop blindly increments the cell position counter for **every** glyph — including zero-advance combining marks. This displaces them into the next cell, breaking the visual output. For example, Thai `สวัสดี` renders as `ส ว ั ส ด ี` with vowels and tone marks scattered across separate cells. #39526 previously fixed Latin combining characters (like NFD-normalized é) by handling them at the terminal cell level via `append_zero_width_chars`. That fix works when alacritty's cell model already identifies the characters as zero-width. However, many complex script combining marks still get mispositioned at the shaping layer — which is what this PR addresses. ## Solution Extracted `apply_force_width_to_layout()` in `line_layout.rs` that detects combining marks during the force-width pass by checking whether a glyph's shaped x position has advanced by at least half a cell width beyond the previous base character. Combining marks (which HarfBuzz places at the same x as their base) are kept at their shaped position relative to the forced base, rather than being pushed to the next cell. This approach is **script-agnostic** — it works for all complex scripts (Thai, Arabic, Devanagari, Myanmar, Khmer, etc.) without hardcoding Unicode ranges or disabling the monospace grid. The fix lives at the text shaping layer in GPUI, so it benefits any consumer of `LineLayoutCache` that uses `force_width`. The two duplicate force-width loops (in `layout_line` and `layout_line_by_hash`) are consolidated into the single shared helper. ## Testing **Unit tests** cover the following scenarios: - Latin text positions remain unchanged - A single combining mark stays at its base character's cell - A base character following combining marks gets the correct next cell - Multiple stacked combining marks all stay anchored to cell 0 - Drifted base positions respect the existing 1px tolerance **Manual testing** with the following scripts in the integrated terminal: | Script | Test text | What to look for | |--------|-----------|-----------------| | Thai | `สวัสดีครับ ภาษาไทย ก้อนหิน น้ำตก` | Vowel signs (ั ี) and tone marks (้) render above/below consonants, not in separate cells | | Lao | `ສະບາຍດີ ພາສາລາວ` | Vowel signs stay attached to consonants | | Arabic | `بِسْمِ اللَّهِ الرَّحْمَٰنِ` | Diacritics (harakat) render on their base letters | | Hebrew | `בְּרֵאשִׁית בָּרָא אֱלֹהִים` | Vowel points (nikud) stay under/over their letters | | Hindi | `नमस्ते हिन्दी भाषा` | Vowel matras render correctly on consonants | | Bengali | `বাংলা ভাষা নমস্কার` | Combining marks stay attached | | Tamil | `தமிழ் மொழி வணக்கம்` | Vowel signs render correctly | | Myanmar | `မြန်မာဘာသာ မင်္ဂလာပါ` | Combining marks and medial consonants stay in place | | Khmer | `ភាសាខ្មែរ សួស្តី` | Subscript consonants and vowel signs render correctly | <details> <summary>Script to reproduce</summary> ```sh echo "=== Thai ===" echo "สวัสดีครับ ภาษาไทย ก้อนหิน น้ำตก" echo "" echo "=== Lao ===" echo "ສະບາຍດີ ພາສາລາວ" echo "" echo "=== Arabic ===" echo "بِسْمِ اللَّهِ الرَّحْمَٰنِ" echo "" echo "=== Hebrew ===" echo "בְּרֵאשִׁית בָּרָא אֱלֹהִים" echo "" echo "=== Hindi (Devanagari) ===" echo "नमस्ते हिन्दी भाषा" echo "" echo "=== Bengali ===" echo "বাংলা ভাষা নমস্কার" echo "" echo "=== Tamil ===" echo "தமிழ் மொழி வணக்கம்" echo "" echo "=== Myanmar ===" echo "မြန်မာဘာသာ မင်္ဂလာပါ" echo "" echo "=== Khmer ===" echo "ភាសាខ្មែរ សួស្តី" ``` </details> ### Before <img width="1144" height="740" alt="Screenshot 2569-04-05 at 10 43 00" src="https://github.com/user-attachments/assets/bce2075a-9e19-40dd-81ea-0e29a451b781" /> ### After <img width="1144" height="747" alt="Screenshot 2569-04-05 at 11 22 55" src="https://github.com/user-attachments/assets/e0613bf6-8452-4c65-b893-f9931a471b2e" /> ### Tests passing <img width="1144" height="740" alt="Screenshot 2569-04-05 at 11 20 22" src="https://github.com/user-attachments/assets/9100bd77-4ea6-4f77-ae31-5129484552fd" /> Release Notes: - Fixed terminal rendering of combining marks in complex scripts (Thai, Arabic, Hebrew, Devanagari, Myanmar, Khmer, and others) where vowel signs and tone marks were incorrectly displaced to adjacent cells instead of rendering on their base characters. --------- Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>
This commit is contained in:
MX48@LiveandConrad Irwin authored and GitHub committed
1 parent
08029066e6
commit
b5827ede3a
1 file changed
+152
-18
@@ -610,15 +610,7 @@ impl LineLayoutCache {
|
||||
.layout_line(&text, font_size, runs);
|
||||
|
||||
if let Some(force_width) = force_width {
|
||||
let mut glyph_pos = 0;
|
||||
for run in layout.runs.iter_mut() {
|
||||
for glyph in run.glyphs.iter_mut() {
|
||||
if (glyph.position.x - glyph_pos * force_width).abs() > px(1.) {
|
||||
glyph.position.x = glyph_pos * force_width;
|
||||
}
|
||||
glyph_pos += 1;
|
||||
}
|
||||
}
|
||||
apply_force_width_to_layout(&mut layout, force_width);
|
||||
}
|
||||
|
||||
let key = Arc::new(CacheKey {
|
||||
@@ -767,15 +759,7 @@ impl LineLayoutCache {
|
||||
.layout_line(&text, font_size, runs);
|
||||
|
||||
if let Some(force_width) = force_width {
|
||||
let mut glyph_pos = 0;
|
||||
for run in layout.runs.iter_mut() {
|
||||
for glyph in run.glyphs.iter_mut() {
|
||||
if (glyph.position.x - glyph_pos * force_width).abs() > px(1.) {
|
||||
glyph.position.x = glyph_pos * force_width;
|
||||
}
|
||||
glyph_pos += 1;
|
||||
}
|
||||
}
|
||||
apply_force_width_to_layout(&mut layout, force_width);
|
||||
}
|
||||
|
||||
let key = Arc::new(HashedCacheKey {
|
||||
@@ -795,6 +779,36 @@ impl LineLayoutCache {
|
||||
}
|
||||
}
|
||||
|
||||
// Combining marks (e.g. Thai vowel signs, Arabic diacritics) are shaped by
|
||||
// HarfBuzz at the same x position as their base character. The force-width
|
||||
// loop must not advance the cell counter for these zero-advance glyphs,
|
||||
// otherwise they get displaced into the next cell. We detect them by checking
|
||||
// whether shaped x has advanced by at least half a cell beyond the last base.
|
||||
fn apply_force_width_to_layout(layout: &mut LineLayout, force_width: Pixels) {
|
||||
let mut glyph_pos: usize = 0;
|
||||
// NEG_INFINITY ensures the first glyph is always classified as a base.
|
||||
let mut last_base_shaped_x = px(f32::NEG_INFINITY);
|
||||
let mut last_base_actual_x = px(0.);
|
||||
|
||||
for run in layout.runs.iter_mut() {
|
||||
for glyph in run.glyphs.iter_mut() {
|
||||
let shaped_x = glyph.position.x;
|
||||
|
||||
if shaped_x > last_base_shaped_x + force_width * 0.5 {
|
||||
let forced_x = glyph_pos * force_width;
|
||||
if (shaped_x - forced_x).abs() > px(1.) {
|
||||
glyph.position.x = forced_x;
|
||||
}
|
||||
last_base_shaped_x = shaped_x;
|
||||
last_base_actual_x = glyph.position.x;
|
||||
glyph_pos += 1;
|
||||
} else {
|
||||
glyph.position.x = last_base_actual_x + (shaped_x - last_base_shaped_x);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// A run of text with a single font.
|
||||
#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
|
||||
#[expect(missing_docs)]
|
||||
@@ -942,3 +956,123 @@ impl AsCacheKeyRef for CacheKeyRef<'_> {
|
||||
*self
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::GlyphId;
|
||||
|
||||
fn glyph_at(x: f32, index: usize) -> ShapedGlyph {
|
||||
ShapedGlyph {
|
||||
id: GlyphId(0),
|
||||
position: point(px(x), px(0.)),
|
||||
index,
|
||||
is_emoji: false,
|
||||
}
|
||||
}
|
||||
|
||||
fn make_layout(glyphs: Vec<ShapedGlyph>) -> LineLayout {
|
||||
LineLayout {
|
||||
font_size: px(16.),
|
||||
width: px(100.),
|
||||
ascent: px(12.),
|
||||
descent: px(4.),
|
||||
runs: vec![ShapedRun {
|
||||
font_id: FontId(0),
|
||||
glyphs,
|
||||
}],
|
||||
len: 0,
|
||||
}
|
||||
}
|
||||
|
||||
fn glyph_x_positions(layout: &LineLayout) -> Vec<f32> {
|
||||
layout.runs[0]
|
||||
.glyphs
|
||||
.iter()
|
||||
.map(|g| f32::from(g.position.x))
|
||||
.collect()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_force_width_latin_unchanged() {
|
||||
let cell_width = px(8.);
|
||||
let mut layout = make_layout(vec![glyph_at(0., 0), glyph_at(8., 1), glyph_at(16., 2)]);
|
||||
|
||||
apply_force_width_to_layout(&mut layout, cell_width);
|
||||
|
||||
let positions = glyph_x_positions(&layout);
|
||||
assert_eq!(positions, vec![0., 8., 16.]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_force_width_combining_marks_not_advanced() {
|
||||
let cell_width = px(8.);
|
||||
// Simulates Thai "กี" — base consonant at x=0, combining vowel also at x=0
|
||||
let mut layout = make_layout(vec![
|
||||
glyph_at(0., 0), // ก (base)
|
||||
glyph_at(0., 3), // ี (combining mark, same x)
|
||||
]);
|
||||
|
||||
apply_force_width_to_layout(&mut layout, cell_width);
|
||||
|
||||
let positions = glyph_x_positions(&layout);
|
||||
assert_eq!(positions, vec![0., 0.]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_force_width_base_after_combining_mark() {
|
||||
let cell_width = px(8.);
|
||||
let mut layout = make_layout(vec![glyph_at(0., 0), glyph_at(0., 3), glyph_at(8., 6)]);
|
||||
|
||||
apply_force_width_to_layout(&mut layout, cell_width);
|
||||
|
||||
let positions = glyph_x_positions(&layout);
|
||||
assert_eq!(positions, vec![0., 0., 8.]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_force_width_multiple_combining_marks() {
|
||||
let cell_width = px(8.);
|
||||
// Simulates "ก้" — base + vowel + tone mark (two combining marks stacked)
|
||||
let mut layout = make_layout(vec![
|
||||
glyph_at(0., 0), // ก (base)
|
||||
glyph_at(0., 3), // vowel (combining)
|
||||
glyph_at(0., 6), // tone mark (combining)
|
||||
glyph_at(8., 9), // next base
|
||||
]);
|
||||
|
||||
apply_force_width_to_layout(&mut layout, cell_width);
|
||||
|
||||
let positions = glyph_x_positions(&layout);
|
||||
assert_eq!(positions, vec![0., 0., 0., 8.]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_force_width_corrects_drifted_base_positions() {
|
||||
let cell_width = px(8.);
|
||||
// Font metrics don't perfectly match cell grid — glyphs drift >1px from cell boundary
|
||||
let mut layout = make_layout(vec![
|
||||
glyph_at(0.5, 0), // within 1px tolerance, kept as-is
|
||||
glyph_at(10.2, 1), // >1px off from 8.0, corrected
|
||||
glyph_at(19.8, 2), // >1px off from 16.0, corrected
|
||||
]);
|
||||
|
||||
apply_force_width_to_layout(&mut layout, cell_width);
|
||||
|
||||
let positions = glyph_x_positions(&layout);
|
||||
assert_eq!(positions, vec![0.5, 8., 16.]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_force_width_combining_mark_after_within_tolerance_base() {
|
||||
let cell_width = px(8.);
|
||||
// Base glyph is within 1px of grid so it keeps its shaped position.
|
||||
// The combining mark must align to the base's actual position, not the grid slot.
|
||||
let mut layout = make_layout(vec![glyph_at(0.5, 0), glyph_at(0.5, 3)]);
|
||||
|
||||
apply_force_width_to_layout(&mut layout, cell_width);
|
||||
|
||||
let positions = glyph_x_positions(&layout);
|
||||
assert_eq!(positions, vec![0.5, 0.5]);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user