Skip to content

Commit 34faaa1

Browse files
authored
fix(text): honour letter-spacing when breaking lines (#132)
The fit test that decides where a line breaks measured with zero tracking while every consumer measured the resulting lines with the real value, so measurement and paint no longer shared a fixed point: the box was sized for one line, paint decided two were needed, and left-aligned them in a box sized for one. Measured on a centred headline, frame centre 960: at 290px with -9px tracking the ink centre sat at 552, a 408px error, and the validator called the scenario clean whenever the text carried an explicit height — it re-measures through the same intrinsic and inherited the same blind spot. At 240px a negative tracking, which makes the string narrower, gained it a break it did not have at zero. Wrapping now takes the tracking. All four sweep rows converge on the frame centre, and the spurious breaks are gone. This matters because tight negative tracking on large type is the defining trait of the register this project targets — it was the one thing that could not be expressed. gradient_text turned out to drop letter-spacing entirely: it was hardcoded to zero at wrap, at width measurement and at draw, while its intrinsic sized the box with the real value. Fixing only the wrap would have replaced one mismatch with another, so all three now agree. Relative units are the same defect one layer down: em, rem, vw and vh parse cleanly and are then discarded by Length::px(), which returns 0 — indistinguishable from a deliberate zero, and advertised as valid by the exported schema. A scenario sized in vw rendered a black frame and exited 0. They now resolve through a LengthContext where one is reachable, and warn loudly where it is not. em on font-size itself still needs a resolved parent size from the cascade; that is left, and documented, rather than half-done. All seven tracked examples render pixel-identically. Closes #125
1 parent 4a519ba commit 34faaa1

7 files changed

Lines changed: 800 additions & 40 deletions

File tree

crates/rustmotion-components/src/gradient_text.rs

Lines changed: 55 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use rustmotion_core::engine::animator::AnimatedProperties;
1111
use rustmotion_core::engine::layout_pass::BoxLayout;
1212
use rustmotion_core::engine::renderer::{
1313
draw_text_with_fallback, emoji_typeface, measure_text_with_fallback, paint_from_hex,
14-
parse_hex_color, typeface_with_fallback, wrap_text_with_fallback,
14+
parse_hex_color, typeface_with_fallback, wrap_text_with_tracking,
1515
};
1616
use rustmotion_core::schema::TimelineStep;
1717
use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig};
@@ -83,16 +83,38 @@ impl GradientText {
8383
}
8484

8585
impl GradientText {
86-
fn paint(&self, canvas: &Canvas, layout_width: f32, time: f64) {
86+
fn paint(&self, canvas: &Canvas, layout_width: f32, time: f64, ctx: &PaintCtx) {
8787
if self.content.is_empty() || self.colors.is_empty() {
8888
return;
8989
}
9090

9191
let Some((font, emoji_font)) = self.resolve_font() else {
9292
return;
9393
};
94+
// `font-size` stays context-free — see the identical note in
95+
// `text.rs`'s `paint`: resolving its own `em`/`%` correctly needs a
96+
// cascade.rs change (parent font-size as a resolved px value, not a
97+
// raw `Length`), which is out of scope here (issue #125 §2).
9498
let font_size = self.style.font_size_px_or(48.0);
95-
let line_height_val = self.style.line_height_for(font_size);
99+
// `letter-spacing`/`line-height`'s `em`/`%` are relative to this
100+
// element's own font-size, no cascade dependency — a real
101+
// `LengthContext` is available here, so use the context-aware
102+
// resolvers (issue #125 §2: correctly handles `vw`/`vh`/`rem`/
103+
// line-height-`%`). `letter_spacing` itself: this component never
104+
// read `style.letter-spacing` before this fix — the wrap/measure/
105+
// draw calls below always tracked at 0.0 regardless of what was
106+
// set, which is the same class of measure/paint disagreement issue
107+
// #125 §1 describes elsewhere. It's threaded through consistently
108+
// now.
109+
let type_ctx = rustmotion_core::css::units::LengthContext {
110+
viewport_width: ctx.video_width as f32,
111+
viewport_height: ctx.video_height as f32,
112+
parent_size: layout_width.max(0.0),
113+
font_size,
114+
root_font_size: 16.0,
115+
};
116+
let line_height_val = self.style.line_height_for_ctx(font_size, &type_ctx);
117+
let letter_spacing = self.style.letter_spacing_px_ctx(&type_ctx);
96118

97119
// M1: `white-space: nowrap|pre` keeps the whole content on one line
98120
// even past `layout_width` (it bleeds); anything else word-wraps at
@@ -108,7 +130,10 @@ impl GradientText {
108130
} else {
109131
None
110132
};
111-
let lines = wrap_text_with_fallback(&self.content, &font, &emoji_font, wrap_at);
133+
// Tracking-aware wrap (issue #125 §1), consistent with the
134+
// real-tracking measurement/draw below.
135+
let lines =
136+
wrap_text_with_tracking(&self.content, &font, &emoji_font, wrap_at, letter_spacing);
112137

113138
// Measure the overall (possibly multi-line) bounding box. The
114139
// gradient is defined once across this whole block rather than
@@ -119,7 +144,7 @@ impl GradientText {
119144
let descent = metrics.descent;
120145
let text_w = lines
121146
.iter()
122-
.map(|l| measure_text_with_fallback(l, &font, &emoji_font, 0.0))
147+
.map(|l| measure_text_with_fallback(l, &font, &emoji_font, letter_spacing))
123148
.fold(0.0f32, f32::max);
124149
let text_h = (lines.len().max(1) - 1) as f32 * line_height_val + ascent + descent;
125150

@@ -185,7 +210,16 @@ impl GradientText {
185210
continue;
186211
}
187212
let y = i as f32 * line_height_val + ascent;
188-
draw_text_with_fallback(canvas, line, &font, &emoji_font, 0.0, 0.0, y, &fill_paint);
213+
draw_text_with_fallback(
214+
canvas,
215+
line,
216+
&font,
217+
&emoji_font,
218+
letter_spacing,
219+
0.0,
220+
y,
221+
&fill_paint,
222+
);
189223
}
190224
}
191225
}
@@ -198,7 +232,7 @@ impl Painter for GradientText {
198232
_props: &AnimatedProperties,
199233
ctx: &PaintCtx,
200234
) {
201-
self.paint(canvas, layout.width, ctx.time);
235+
self.paint(canvas, layout.width, ctx.time, ctx);
202236
}
203237
}
204238

@@ -248,6 +282,18 @@ mod tests {
248282
.collect()
249283
}
250284

285+
fn test_ctx() -> PaintCtx {
286+
PaintCtx {
287+
time: 0.0,
288+
scene_duration: 1.0,
289+
frame_index: 0,
290+
fps: 30,
291+
video_width: 1920,
292+
video_height: 1080,
293+
stagger_offset: 0.0,
294+
}
295+
}
296+
251297
fn has_ink_in(grid: &[u8], surface_width: i32, x0: i32, x1: i32, y0: i32, y1: i32) -> bool {
252298
for y in y0..y1 {
253299
for x in x0..x1 {
@@ -271,7 +317,7 @@ mod tests {
271317
const H: i32 = 200;
272318
let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface");
273319
let canvas = surface.canvas();
274-
gt.paint(canvas, 80.0, 0.0);
320+
gt.paint(canvas, 80.0, 0.0, &test_ctx());
275321
let grid = alpha_grid(&mut surface, W, H);
276322

277323
assert!(
@@ -291,7 +337,7 @@ mod tests {
291337
const H: i32 = 200;
292338
let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface");
293339
let canvas = surface.canvas();
294-
gt.paint(canvas, 80.0, 0.0);
340+
gt.paint(canvas, 80.0, 0.0, &test_ctx());
295341
let grid = alpha_grid(&mut surface, W, H);
296342

297343
assert!(

crates/rustmotion-components/src/intrinsic.rs

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use rustmotion_core::css::style::{
1414
use rustmotion_core::engine::box_tree::{AvailableSpace, IntrinsicMeasure};
1515
use rustmotion_core::engine::renderer::{
1616
emoji_typeface, format_counter_value, measure_text_with_fallback, typeface_with_fallback,
17-
wrap_text_with_fallback,
17+
wrap_text_with_tracking,
1818
};
1919

2020
use crate::badge::{Badge, BadgeSize};
@@ -58,6 +58,18 @@ impl TextIntrinsic {
5858
/// wrap:true unconditionally so their measured size still matches what
5959
/// those painters actually draw.
6060
pub fn from_parts(content: &str, style: &CssStyle, max_width: Option<f32>) -> Self {
61+
// No `LengthContext` is reachable here without changing this
62+
// constructor's signature — its only callers are `box_builder.rs`
63+
// and `rustmotion-cli/src/commands/geometry.rs`, both outside this
64+
// workstream's scope (box_builder.rs is a sibling's live file this
65+
// wave; the geometry validator re-measures via this exact type and
66+
// must keep agreeing with it byte-for-byte, so changing what it
67+
// needs to pass in is not a call to make unilaterally here). So
68+
// `font_size`/`line_height` stay on the context-free accessors
69+
// (issue #125 §2's `vw`/`vh`/`rem`/`%` gap is not closed for this
70+
// constructor) — only `letter_spacing` below, which is used
71+
// exclusively by the wrap fix in `measure()`, no signature change
72+
// needed for it.
6173
let font_size = style.font_size_px_or(48.0);
6274
let line_height_resolved = style.line_height_for(font_size);
6375
Self {
@@ -115,7 +127,17 @@ impl IntrinsicMeasure for TextIntrinsic {
115127
let emoji_font = emoji_typeface().map(|tf| Font::from_typeface(tf, self.font_size));
116128

117129
let wrap_at = if self.wrap { max_width } else { None };
118-
let lines = wrap_text_with_fallback(&self.content, &font, &emoji_font, wrap_at);
130+
// Tracking-aware wrap (issue #125 §1): matches the real
131+
// `letter_spacing` used to measure each line's width just below, so
132+
// the box this measurer reserves and what `Text::paint` (also fixed,
133+
// same tracking) actually paints agree on line count.
134+
let lines = wrap_text_with_tracking(
135+
&self.content,
136+
&font,
137+
&emoji_font,
138+
wrap_at,
139+
self.letter_spacing,
140+
);
119141

120142
let mut max_w = 0.0f32;
121143
for line in &lines {

crates/rustmotion-components/src/shape.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use rustmotion_core::engine::animator::AnimatedProperties;
88
use rustmotion_core::engine::layout_pass::BoxLayout;
99
use rustmotion_core::engine::renderer::{
1010
build_shape_path, color4f_from_hex, draw_shape_path, draw_text_with_fallback, emoji_typeface,
11-
measure_text_with_fallback, paint_from_hex, typeface_with_fallback, wrap_text_with_fallback,
11+
measure_text_with_fallback, paint_from_hex, typeface_with_fallback, wrap_text_with_tracking,
1212
};
1313
use rustmotion_core::schema::{
1414
Fill, FontWeight, GradientType, ShapeText, ShapeType, Stroke, TextAlign, TimelineStep,
@@ -187,7 +187,19 @@ fn render_shape_text(
187187
};
188188
let letter_spacing = text.letter_spacing.unwrap_or(0.0);
189189

190-
let lines = wrap_text_with_fallback(&text.content, &font, &emoji_font, Some(area_w));
190+
// Tracking-aware wrap (issue #125 §1): the fit test measures with the
191+
// same `letter_spacing` used below for `line_width`/`draw_text_with_
192+
// fallback`, so the wrap decision agrees with what's actually painted.
193+
// `ShapeText`'s `font_size`/`letter_spacing`/`line_height` are plain
194+
// `f32` (not `CssStyle`/`Length`), so issue #125 §2's relative-unit gap
195+
// doesn't apply here — there is no unit string to resolve.
196+
let lines = wrap_text_with_tracking(
197+
&text.content,
198+
&font,
199+
&emoji_font,
200+
Some(area_w),
201+
letter_spacing,
202+
);
191203
let descent = metrics.descent;
192204
let total_h = if lines.len() > 1 {
193205
(lines.len() - 1) as f32 * line_height + ascent + descent

crates/rustmotion-components/src/text.rs

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use rustmotion_core::engine::animator::AnimatedProperties;
1414
use rustmotion_core::engine::layout_pass::BoxLayout;
1515
use rustmotion_core::engine::renderer::{
1616
draw_text_with_fallback, emoji_typeface, measure_text_with_fallback, paint_from_hex,
17-
typeface_with_fallback, wrap_text_with_fallback,
17+
typeface_with_fallback, wrap_text_with_tracking,
1818
};
1919
use rustmotion_core::schema::{
2020
AnimationEffect, CharAnimPreset, CharAnimation, FontStyleType, FontWeight, Stroke, TextAlign,
@@ -360,7 +360,27 @@ impl Text {
360360
props: &AnimatedProperties,
361361
ctx: &PaintCtx,
362362
) -> Result<()> {
363+
// `font-size` stays on the context-free accessor: resolving a
364+
// relative unit here correctly (`em`/`%`) would need the parent's
365+
// *actual computed* font-size, which `cascade.rs` doesn't provide
366+
// today (it inherits `font-size` as a raw, unresolved `Length` —
367+
// see the module note on `CssStyle::font_size_px_ctx`, issue #125
368+
// §2). That cascade fix stays out of scope here; `vw`/`vh`/`rem`
369+
// font-size still fall back to 0px with a loud warning via `.px()`.
363370
let font_size = self.style.font_size_px_or(48.0);
371+
// `letter-spacing` and `line-height`'s `em`/`%` are relative to this
372+
// element's *own* (just-resolved) font-size, which has no cascade
373+
// dependency — a real `LengthContext` is available here (real
374+
// viewport dims from `ctx`, `font_size` just above), so these use
375+
// the context-aware resolvers and correctly handle `vw`/`vh`/`rem`/
376+
// line-height-`%` (issue #125 §2).
377+
let type_ctx = rustmotion_core::css::units::LengthContext {
378+
viewport_width: ctx.video_width as f32,
379+
viewport_height: ctx.video_height as f32,
380+
parent_size: layout_width.max(0.0),
381+
font_size,
382+
root_font_size: 16.0,
383+
};
364384
// Animated color (timeline style-state transitions) overrides the
365385
// static style color.
366386
let color = props
@@ -386,8 +406,8 @@ impl Text {
386406
Some(CssTextAlign::Right | CssTextAlign::End) => TextAlign::Right,
387407
_ => TextAlign::Left,
388408
};
389-
let line_height_val = self.style.line_height_for(font_size);
390-
let letter_spacing = self.style.letter_spacing_px();
409+
let line_height_val = self.style.line_height_for_ctx(font_size, &type_ctx);
410+
let letter_spacing = self.style.letter_spacing_px_ctx(&type_ctx);
391411

392412
let slant = match font_style_type {
393413
FontStyleType::Normal => skia_safe::font_style::Slant::Upright,
@@ -443,7 +463,13 @@ impl Text {
443463
self.content.clone()
444464
};
445465

446-
let lines = wrap_text_with_fallback(&content, &font, &emoji_font, wrap_width);
466+
// Tracking-aware wrap (issue #125 §1): the fit test now measures
467+
// with this element's real `letter_spacing`, matching the
468+
// measurements below (`align_width`, per-line `advance_width`) that
469+
// already used it — the box this wraps for and the pixels painted
470+
// into it now agree.
471+
let lines =
472+
wrap_text_with_tracking(&content, &font, &emoji_font, wrap_width, letter_spacing);
447473
let (_, metrics) = font.metrics();
448474
let ascent = -metrics.ascent;
449475
let descent = metrics.descent;
@@ -455,14 +481,7 @@ impl Text {
455481
let shadows: Vec<rustmotion_core::schema::TextShadow> = if let Some(s) = &self.text_shadow {
456482
vec![s.clone()]
457483
} else if let Some(list) = &self.style.text_shadow {
458-
let lctx = rustmotion_core::css::units::LengthContext {
459-
viewport_width: ctx.video_width as f32,
460-
viewport_height: ctx.video_height as f32,
461-
parent_size: layout_width.max(0.0),
462-
font_size,
463-
root_font_size: 16.0,
464-
};
465-
list.iter().map(|s| s.to_schema(&lctx)).collect()
484+
list.iter().map(|s| s.to_schema(&type_ctx)).collect()
466485
} else {
467486
Vec::new()
468487
};

0 commit comments

Comments
 (0)