From ec298a602d160cd08e50bde6021107f192695617 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sun, 2 Aug 2026 11:55:50 +0200 Subject: [PATCH] fix(text): honour letter-spacing when breaking lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../src/gradient_text.rs | 64 +++- crates/rustmotion-components/src/intrinsic.rs | 26 +- crates/rustmotion-components/src/shape.rs | 16 +- crates/rustmotion-components/src/text.rs | 43 ++- crates/rustmotion-core/src/css/style.rs | 228 +++++++++++- crates/rustmotion-core/src/css/units.rs | 128 ++++++- .../src/engine/renderer/text.rs | 335 +++++++++++++++++- 7 files changed, 800 insertions(+), 40 deletions(-) diff --git a/crates/rustmotion-components/src/gradient_text.rs b/crates/rustmotion-components/src/gradient_text.rs index f432e4a..086ec30 100644 --- a/crates/rustmotion-components/src/gradient_text.rs +++ b/crates/rustmotion-components/src/gradient_text.rs @@ -11,7 +11,7 @@ use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::engine::renderer::{ draw_text_with_fallback, emoji_typeface, measure_text_with_fallback, paint_from_hex, - parse_hex_color, typeface_with_fallback, wrap_text_with_fallback, + parse_hex_color, typeface_with_fallback, wrap_text_with_tracking, }; use rustmotion_core::schema::TimelineStep; use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig}; @@ -83,7 +83,7 @@ impl GradientText { } impl GradientText { - fn paint(&self, canvas: &Canvas, layout_width: f32, time: f64) { + fn paint(&self, canvas: &Canvas, layout_width: f32, time: f64, ctx: &PaintCtx) { if self.content.is_empty() || self.colors.is_empty() { return; } @@ -91,8 +91,30 @@ impl GradientText { let Some((font, emoji_font)) = self.resolve_font() else { return; }; + // `font-size` stays context-free — see the identical note in + // `text.rs`'s `paint`: resolving its own `em`/`%` correctly needs a + // cascade.rs change (parent font-size as a resolved px value, not a + // raw `Length`), which is out of scope here (issue #125 §2). let font_size = self.style.font_size_px_or(48.0); - let line_height_val = self.style.line_height_for(font_size); + // `letter-spacing`/`line-height`'s `em`/`%` are relative to this + // element's own font-size, no cascade dependency — a real + // `LengthContext` is available here, so use the context-aware + // resolvers (issue #125 §2: correctly handles `vw`/`vh`/`rem`/ + // line-height-`%`). `letter_spacing` itself: this component never + // read `style.letter-spacing` before this fix — the wrap/measure/ + // draw calls below always tracked at 0.0 regardless of what was + // set, which is the same class of measure/paint disagreement issue + // #125 §1 describes elsewhere. It's threaded through consistently + // now. + let type_ctx = rustmotion_core::css::units::LengthContext { + viewport_width: ctx.video_width as f32, + viewport_height: ctx.video_height as f32, + parent_size: layout_width.max(0.0), + font_size, + root_font_size: 16.0, + }; + let line_height_val = self.style.line_height_for_ctx(font_size, &type_ctx); + let letter_spacing = self.style.letter_spacing_px_ctx(&type_ctx); // M1: `white-space: nowrap|pre` keeps the whole content on one line // even past `layout_width` (it bleeds); anything else word-wraps at @@ -108,7 +130,10 @@ impl GradientText { } else { None }; - let lines = wrap_text_with_fallback(&self.content, &font, &emoji_font, wrap_at); + // Tracking-aware wrap (issue #125 §1), consistent with the + // real-tracking measurement/draw below. + let lines = + wrap_text_with_tracking(&self.content, &font, &emoji_font, wrap_at, letter_spacing); // Measure the overall (possibly multi-line) bounding box. The // gradient is defined once across this whole block rather than @@ -119,7 +144,7 @@ impl GradientText { let descent = metrics.descent; let text_w = lines .iter() - .map(|l| measure_text_with_fallback(l, &font, &emoji_font, 0.0)) + .map(|l| measure_text_with_fallback(l, &font, &emoji_font, letter_spacing)) .fold(0.0f32, f32::max); let text_h = (lines.len().max(1) - 1) as f32 * line_height_val + ascent + descent; @@ -185,7 +210,16 @@ impl GradientText { continue; } let y = i as f32 * line_height_val + ascent; - draw_text_with_fallback(canvas, line, &font, &emoji_font, 0.0, 0.0, y, &fill_paint); + draw_text_with_fallback( + canvas, + line, + &font, + &emoji_font, + letter_spacing, + 0.0, + y, + &fill_paint, + ); } } } @@ -198,7 +232,7 @@ impl Painter for GradientText { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - self.paint(canvas, layout.width, ctx.time); + self.paint(canvas, layout.width, ctx.time, ctx); } } @@ -248,6 +282,18 @@ mod tests { .collect() } + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 1920, + video_height: 1080, + stagger_offset: 0.0, + } + } + fn has_ink_in(grid: &[u8], surface_width: i32, x0: i32, x1: i32, y0: i32, y1: i32) -> bool { for y in y0..y1 { for x in x0..x1 { @@ -271,7 +317,7 @@ mod tests { const H: i32 = 200; let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); let canvas = surface.canvas(); - gt.paint(canvas, 80.0, 0.0); + gt.paint(canvas, 80.0, 0.0, &test_ctx()); let grid = alpha_grid(&mut surface, W, H); assert!( @@ -291,7 +337,7 @@ mod tests { const H: i32 = 200; let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); let canvas = surface.canvas(); - gt.paint(canvas, 80.0, 0.0); + gt.paint(canvas, 80.0, 0.0, &test_ctx()); let grid = alpha_grid(&mut surface, W, H); assert!( diff --git a/crates/rustmotion-components/src/intrinsic.rs b/crates/rustmotion-components/src/intrinsic.rs index 27ff4ba..9e67e9b 100644 --- a/crates/rustmotion-components/src/intrinsic.rs +++ b/crates/rustmotion-components/src/intrinsic.rs @@ -14,7 +14,7 @@ use rustmotion_core::css::style::{ use rustmotion_core::engine::box_tree::{AvailableSpace, IntrinsicMeasure}; use rustmotion_core::engine::renderer::{ emoji_typeface, format_counter_value, measure_text_with_fallback, typeface_with_fallback, - wrap_text_with_fallback, + wrap_text_with_tracking, }; use crate::badge::{Badge, BadgeSize}; @@ -58,6 +58,18 @@ impl TextIntrinsic { /// wrap:true unconditionally so their measured size still matches what /// those painters actually draw. pub fn from_parts(content: &str, style: &CssStyle, max_width: Option) -> Self { + // No `LengthContext` is reachable here without changing this + // constructor's signature — its only callers are `box_builder.rs` + // and `rustmotion-cli/src/commands/geometry.rs`, both outside this + // workstream's scope (box_builder.rs is a sibling's live file this + // wave; the geometry validator re-measures via this exact type and + // must keep agreeing with it byte-for-byte, so changing what it + // needs to pass in is not a call to make unilaterally here). So + // `font_size`/`line_height` stay on the context-free accessors + // (issue #125 §2's `vw`/`vh`/`rem`/`%` gap is not closed for this + // constructor) — only `letter_spacing` below, which is used + // exclusively by the wrap fix in `measure()`, no signature change + // needed for it. let font_size = style.font_size_px_or(48.0); let line_height_resolved = style.line_height_for(font_size); Self { @@ -115,7 +127,17 @@ impl IntrinsicMeasure for TextIntrinsic { let emoji_font = emoji_typeface().map(|tf| Font::from_typeface(tf, self.font_size)); let wrap_at = if self.wrap { max_width } else { None }; - let lines = wrap_text_with_fallback(&self.content, &font, &emoji_font, wrap_at); + // Tracking-aware wrap (issue #125 §1): matches the real + // `letter_spacing` used to measure each line's width just below, so + // the box this measurer reserves and what `Text::paint` (also fixed, + // same tracking) actually paints agree on line count. + let lines = wrap_text_with_tracking( + &self.content, + &font, + &emoji_font, + wrap_at, + self.letter_spacing, + ); let mut max_w = 0.0f32; for line in &lines { diff --git a/crates/rustmotion-components/src/shape.rs b/crates/rustmotion-components/src/shape.rs index 17bef1e..5e58a8d 100644 --- a/crates/rustmotion-components/src/shape.rs +++ b/crates/rustmotion-components/src/shape.rs @@ -8,7 +8,7 @@ use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::engine::renderer::{ build_shape_path, color4f_from_hex, draw_shape_path, draw_text_with_fallback, emoji_typeface, - measure_text_with_fallback, paint_from_hex, typeface_with_fallback, wrap_text_with_fallback, + measure_text_with_fallback, paint_from_hex, typeface_with_fallback, wrap_text_with_tracking, }; use rustmotion_core::schema::{ Fill, FontWeight, GradientType, ShapeText, ShapeType, Stroke, TextAlign, TimelineStep, @@ -187,7 +187,19 @@ fn render_shape_text( }; let letter_spacing = text.letter_spacing.unwrap_or(0.0); - let lines = wrap_text_with_fallback(&text.content, &font, &emoji_font, Some(area_w)); + // Tracking-aware wrap (issue #125 §1): the fit test measures with the + // same `letter_spacing` used below for `line_width`/`draw_text_with_ + // fallback`, so the wrap decision agrees with what's actually painted. + // `ShapeText`'s `font_size`/`letter_spacing`/`line_height` are plain + // `f32` (not `CssStyle`/`Length`), so issue #125 §2's relative-unit gap + // doesn't apply here — there is no unit string to resolve. + let lines = wrap_text_with_tracking( + &text.content, + &font, + &emoji_font, + Some(area_w), + letter_spacing, + ); let descent = metrics.descent; let total_h = if lines.len() > 1 { (lines.len() - 1) as f32 * line_height + ascent + descent diff --git a/crates/rustmotion-components/src/text.rs b/crates/rustmotion-components/src/text.rs index e29d5b7..504fa6a 100644 --- a/crates/rustmotion-components/src/text.rs +++ b/crates/rustmotion-components/src/text.rs @@ -14,7 +14,7 @@ use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::engine::renderer::{ draw_text_with_fallback, emoji_typeface, measure_text_with_fallback, paint_from_hex, - typeface_with_fallback, wrap_text_with_fallback, + typeface_with_fallback, wrap_text_with_tracking, }; use rustmotion_core::schema::{ AnimationEffect, CharAnimPreset, CharAnimation, FontStyleType, FontWeight, Stroke, TextAlign, @@ -360,7 +360,27 @@ impl Text { props: &AnimatedProperties, ctx: &PaintCtx, ) -> Result<()> { + // `font-size` stays on the context-free accessor: resolving a + // relative unit here correctly (`em`/`%`) would need the parent's + // *actual computed* font-size, which `cascade.rs` doesn't provide + // today (it inherits `font-size` as a raw, unresolved `Length` — + // see the module note on `CssStyle::font_size_px_ctx`, issue #125 + // §2). That cascade fix stays out of scope here; `vw`/`vh`/`rem` + // font-size still fall back to 0px with a loud warning via `.px()`. let font_size = self.style.font_size_px_or(48.0); + // `letter-spacing` and `line-height`'s `em`/`%` are relative to this + // element's *own* (just-resolved) font-size, which has no cascade + // dependency — a real `LengthContext` is available here (real + // viewport dims from `ctx`, `font_size` just above), so these use + // the context-aware resolvers and correctly handle `vw`/`vh`/`rem`/ + // line-height-`%` (issue #125 §2). + let type_ctx = rustmotion_core::css::units::LengthContext { + viewport_width: ctx.video_width as f32, + viewport_height: ctx.video_height as f32, + parent_size: layout_width.max(0.0), + font_size, + root_font_size: 16.0, + }; // Animated color (timeline style-state transitions) overrides the // static style color. let color = props @@ -386,8 +406,8 @@ impl Text { Some(CssTextAlign::Right | CssTextAlign::End) => TextAlign::Right, _ => TextAlign::Left, }; - let line_height_val = self.style.line_height_for(font_size); - let letter_spacing = self.style.letter_spacing_px(); + let line_height_val = self.style.line_height_for_ctx(font_size, &type_ctx); + let letter_spacing = self.style.letter_spacing_px_ctx(&type_ctx); let slant = match font_style_type { FontStyleType::Normal => skia_safe::font_style::Slant::Upright, @@ -443,7 +463,13 @@ impl Text { self.content.clone() }; - let lines = wrap_text_with_fallback(&content, &font, &emoji_font, wrap_width); + // Tracking-aware wrap (issue #125 §1): the fit test now measures + // with this element's real `letter_spacing`, matching the + // measurements below (`align_width`, per-line `advance_width`) that + // already used it — the box this wraps for and the pixels painted + // into it now agree. + let lines = + wrap_text_with_tracking(&content, &font, &emoji_font, wrap_width, letter_spacing); let (_, metrics) = font.metrics(); let ascent = -metrics.ascent; let descent = metrics.descent; @@ -455,14 +481,7 @@ impl Text { let shadows: Vec = if let Some(s) = &self.text_shadow { vec![s.clone()] } else if let Some(list) = &self.style.text_shadow { - let lctx = rustmotion_core::css::units::LengthContext { - viewport_width: ctx.video_width as f32, - viewport_height: ctx.video_height as f32, - parent_size: layout_width.max(0.0), - font_size, - root_font_size: 16.0, - }; - list.iter().map(|s| s.to_schema(&lctx)).collect() + list.iter().map(|s| s.to_schema(&type_ctx)).collect() } else { Vec::new() }; diff --git a/crates/rustmotion-core/src/css/style.rs b/crates/rustmotion-core/src/css/style.rs index 39b4827..ad44f35 100644 --- a/crates/rustmotion-core/src/css/style.rs +++ b/crates/rustmotion-core/src/css/style.rs @@ -7,7 +7,7 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use super::units::{Length, LengthPercentage}; +use super::units::{Length, LengthContext, LengthPercentage, ParsedLength}; // `GradientBorder` / `InnerShadow` are reused from the schema layer rather // than mirrored: same crate, same serde/JsonSchema derives, identical JSON // shape either way — a css-local mirror would only duplicate the struct. @@ -270,6 +270,122 @@ impl CssStyle { } } + // ---- Context-aware typography resolution (issue #125 §2) ---- + // + // `font_size_px_or`/`letter_spacing_px`/`line_height_for` above are the + // context-free accessors ~50+ call sites across the engine use; they go + // through `Length::px()`, which cannot resolve `%`/`em`/`rem`/`vw`/`vh` + // (no `LengthContext` reaches them) and — as of this fix — warns loudly + // instead of silently dropping to `0px` when the value actually is one + // of those units (see `units::px_or_warn`). That is the "fail loudly" + // half of issue #125 §2. + // + // The methods below are the "or work" half: given a `LengthContext`, + // they resolve `%`/`em`/`rem`/`vw`/`vh` correctly for these three + // properties specifically, honouring the CSS rule that `em` means two + // different things depending on which property it's on: + // - on `font-size` itself, `em` is relative to the *parent's* computed + // font-size — i.e. `ctx.font_size` going in. + // - on `letter-spacing` / `line-height`, `em` is relative to the + // element's *own* (just-computed) font-size, not the parent's. + // `typography_px_ctx` below resolves all three together and gets this + // right by re-deriving the context between steps; the three individual + // methods are the building blocks for callers that need only one value, + // or that already have the right `ctx.font_size` for what they're + // resolving. + // + // What is NOT fixed by this, and is explicitly out of scope for this + // workstream (file allowlist: renderer/text.rs, css/units.rs, + // css/style.rs — not css/cascade.rs): `cascade::inherit_from` copies an + // inherited `font-size` down the tree as the raw, unresolved `Length` — + // not a resolved px value. So today nothing walks the tree computing + // "the actual parent font-size in px" to feed as `ctx.font_size` when + // resolving a child's `em` font-size; a caller that plugs in some other + // value (a default, the root font-size, whatever's convenient) gets a + // *technically* resolved but *semantically wrong* base for that one + // case. `rem` (always relative to a single scenario-wide root, not a + // per-ancestor chain) and `vw`/`vh` (relative to the real viewport) do + // NOT have this problem — they are fully correct via `ctx.root_font_size` + // / `ctx.viewport_*` regardless of cascade. `%` on `line-height` is + // special-cased below against the *own* font-size per CSS, not + // `ctx.parent_size`, so it isn't affected either. In short: `em`/`%` on + // `font-size` need a cascade.rs fix to be fully correct end-to-end; + // everything else these methods resolve is correct today. + // + // These are additive — nothing above changes signature, and nothing + // currently in the engine calls these yet, since every existing call + // site (`rustmotion-components/**`) is outside this workstream's file + // scope. Wiring a real `LengthContext` (viewport dims from `PaintCtx`, + // parent font-size from a resolved-cascade) into those call sites is the + // integration step a sibling workstream (or a follow-up PR) needs to do + // for relative units on type to actually reach rendered output. + + /// `font-size` resolved against `ctx`, correctly handling + /// `%`/`em`/`rem`/`vw`/`vh` — unlike [`Self::font_size_px_or`]. `em`/`%` + /// resolve against `ctx.font_size`, which the caller should set to the + /// parent's *actual computed* font-size in px for correctness (see the + /// module note above on why nothing does that yet). + pub fn font_size_px_ctx(&self, ctx: &LengthContext, default: f32) -> f32 { + self.font_size + .as_ref() + .and_then(|l| l.parse().resolve(ctx)) + .unwrap_or(default) + } + + /// `letter-spacing` resolved against `ctx`, correctly handling + /// `%`/`em`/`rem`/`vw`/`vh` — unlike [`Self::letter_spacing_px`]. Per + /// CSS, `em` here means the element's *own* font-size, so pass a `ctx` + /// whose `font_size` is the already-resolved own font-size (e.g. via + /// [`Self::font_size_px_ctx`]), not the parent's — see + /// [`Self::typography_px_ctx`] for a helper that gets this right + /// automatically. + pub fn letter_spacing_px_ctx(&self, ctx: &LengthContext) -> f32 { + self.letter_spacing + .as_ref() + .and_then(|l| l.parse().resolve(ctx)) + .unwrap_or(0.0) + } + + /// `line-height` resolved against `ctx`, correctly handling + /// `%`/`em`/`rem`/`vw`/`vh` — unlike [`Self::line_height_for`]. + /// `LineHeight::Number` (unitless, e.g. `1.5`) is unaffected — it always + /// means `n * font_size` regardless of any context. For + /// `LineHeight::Length`, `%` is special-cased to CSS's actual rule for + /// this property (relative to the element's *own* font-size, not + /// `ctx.parent_size` like `%` normally means): a generic + /// `ParsedLength::resolve` would silently resolve it against the wrong + /// base otherwise. Same own-vs-parent `em` caveat as + /// [`Self::letter_spacing_px_ctx`] applies. + pub fn line_height_for_ctx(&self, font_size: f32, ctx: &LengthContext) -> f32 { + match &self.line_height { + Some(LineHeight::Number(n)) => n * font_size, + Some(LineHeight::Length(lp)) => match lp.parse() { + ParsedLength::Percent(p) => p / 100.0 * font_size, + other => other.resolve(ctx).unwrap_or(font_size * 1.3), + }, + _ => font_size * 1.3, + } + } + + /// Resolve `font-size`, `letter-spacing`, and `line-height` together in + /// one call, honouring CSS's two different `em` bases (see the module + /// note above `font_size_px_ctx`): `font-size`'s own `em` resolves + /// against `ctx.font_size` (conventionally the parent's font-size), + /// while `letter-spacing`'s and `line-height`'s `em` resolve against the + /// just-computed *own* font-size, not `ctx.font_size` again. Returns + /// `(font_size_px, letter_spacing_px, line_height_px)`. + pub fn typography_px_ctx( + &self, + ctx: &LengthContext, + default_font_size: f32, + ) -> (f32, f32, f32) { + let font_size = self.font_size_px_ctx(ctx, default_font_size); + let own_ctx = LengthContext { font_size, ..*ctx }; + let letter_spacing = self.letter_spacing_px_ctx(&own_ctx); + let line_height = self.line_height_for_ctx(font_size, &own_ctx); + (font_size, letter_spacing, line_height) + } + /// `opacity` with default 1.0. pub fn opacity_or(&self, default: f32) -> f32 { self.opacity.unwrap_or(default) @@ -1271,4 +1387,114 @@ mod tests { other => panic!("expected Length(50%), got {other:?}"), } } + + // ---- issue #125 §2: context-aware typography resolution ---- + + fn style_with(font_size: &str, letter_spacing: &str, line_height: &str) -> CssStyle { + let json = format!( + r#"{{ "font-size": {font_size}, "letter-spacing": {letter_spacing}, "line-height": {line_height} }}"# + ); + serde_json::from_str(&json).unwrap() + } + + #[test] + fn font_size_px_ctx_resolves_vw() { + let s = style_with(r#""15.6vw""#, "0", "1"); + let ctx = LengthContext { + viewport_width: 1920.0, + ..Default::default() + }; + // The exact regression from issue #125 §2: `font-size: "15.6vw"` + // used to resolve to 0 via `.px()` (rendering nothing / a black + // frame). Through a LengthContext it resolves correctly. + assert_eq!(s.font_size_px_ctx(&ctx, 48.0), 15.6 / 100.0 * 1920.0); + // The context-free accessor still can't do this — proving the two + // are genuinely different code paths, not the same thing renamed. + assert_eq!(s.font_size_px_or(48.0), 0.0); + } + + #[test] + fn font_size_px_ctx_resolves_rem_without_cascade_dependency() { + let s = style_with(r#""2rem""#, "0", "1"); + let ctx = LengthContext { + root_font_size: 20.0, + ..Default::default() + }; + // rem is relative to a single scenario-wide root font-size, not a + // per-ancestor chain — no cascade.rs involvement needed for this to + // be correct. + assert_eq!(s.font_size_px_ctx(&ctx, 48.0), 40.0); + } + + #[test] + fn font_size_px_ctx_falls_back_to_default_when_unset() { + let s = CssStyle::default(); + assert_eq!(s.font_size_px_ctx(&LengthContext::default(), 48.0), 48.0); + } + + #[test] + fn letter_spacing_px_ctx_resolves_own_em_not_parent_em() { + // letter-spacing's `em` is relative to the *element's own* + // font-size, not whatever `ctx.font_size` happened to be for + // resolving font-size itself. + let s = style_with("300", r#""-0.03em""#, "1"); + let own_ctx = LengthContext { + font_size: 300.0, // the element's own resolved font-size + ..Default::default() + }; + assert!((s.letter_spacing_px_ctx(&own_ctx) - (-9.0)).abs() < 1e-4); + // Context-free path can't resolve this at all (issue #125 §2): it + // silently (now loudly, but still numerically) drops to 0, which is + // byte-identical to a deliberate zero tracking. + assert_eq!(s.letter_spacing_px(), 0.0); + } + + #[test] + fn line_height_percent_resolves_against_own_font_size_not_parent_size() { + // CSS special case: `line-height: 50%` means 50% of the element's + // own font-size, NOT 50% of `ctx.parent_size` like `%` means for + // most other properties (width, padding, etc). + let s = style_with("100", r#""50%""#, r#""50%""#); + let ctx = LengthContext { + parent_size: 1000.0, // deliberately different from font_size, + // to prove `%` here does NOT fall through to the generic + // percent-of-parent resolution. + ..Default::default() + }; + assert_eq!(s.line_height_for_ctx(100.0, &ctx), 50.0); + } + + #[test] + fn line_height_number_ignores_context_like_before() { + let s = style_with("100", "0", "1.5"); + assert_eq!( + s.line_height_for_ctx(100.0, &LengthContext::default()), + 150.0 + ); + } + + #[test] + fn typography_px_ctx_resolves_all_three_with_correct_em_bases() { + // font-size: 1.5em against a 200px parent font-size -> 300px own + // font-size. letter-spacing/line-height's em must then use that + // 300px *own* size, not the 200px parent size passed in via ctx. + // line-height as a bare JSON number (unitless, `LineHeight::Number`) + // — a quoted `"0.85"` would instead deserialize as a `Length` + // string, which parses a bare numeric string as *pixels* + // (`ParsedLength::Px`), not as the unitless multiplier CSS means; + // that's an existing quirk of `LineHeight`'s untagged variants, + // unrelated to this fix. + let s = style_with(r#""1.5em""#, r#""-0.03em""#, "0.85"); + let ctx = LengthContext { + font_size: 200.0, // parent's font-size, for font-size's own em + ..Default::default() + }; + let (font_size, letter_spacing, line_height) = s.typography_px_ctx(&ctx, 48.0); + assert_eq!(font_size, 300.0); + assert!( + (letter_spacing - (300.0 * -0.03)).abs() < 1e-3, + "letter-spacing em must resolve against the OWN 300px font-size, got {letter_spacing}" + ); + assert_eq!(line_height, 300.0 * 0.85); + } } diff --git a/crates/rustmotion-core/src/css/units.rs b/crates/rustmotion-core/src/css/units.rs index 5cf9681..413bc68 100644 --- a/crates/rustmotion-core/src/css/units.rs +++ b/crates/rustmotion-core/src/css/units.rs @@ -88,6 +88,20 @@ impl ParsedLength { Self::Auto => None, } } + + /// True for units whose absolute pixel value depends on a + /// [`LengthContext`] (`%`, `em`, `rem`, `vw`, `vh`) — i.e. everything + /// [`Length::px`] / [`LengthPercentage::px`] cannot resolve on their own + /// and silently (issue #125 §2) or loudly (post-fix) fall back to `0.0` + /// for. `Px` needs no context; `Auto`/`Fr` are not scalar lengths at all + /// (the caller decides what they mean) so they are not "relative" in + /// this sense. + pub fn is_relative(&self) -> bool { + matches!( + self, + Self::Percent(_) | Self::Em(_) | Self::Rem(_) | Self::Vw(_) | Self::Vh(_) + ) + } } /// Parse a CSS length/percentage string that may also contain transform-origin @@ -165,13 +179,24 @@ impl Length { self.parse().resolve(ctx).unwrap_or(0.0) } - /// Quick px resolution without context (treats em/rem/% as 0). Used by - /// painters that only need the px value of an explicit length. + /// Quick px resolution without context (treats em/rem/%/vw/vh as 0). Used + /// by painters that only need the px value of an explicit length and + /// have no [`LengthContext`] available (see [`CssStyle::font_size_px_or`] + /// / `letter_spacing_px` / `line_height_for` in `css::style`, all built + /// on this — issue #125 §2). + /// + /// A relative unit here cannot be resolved correctly no matter what: + /// `em`/`rem` need a font-size base, `vw`/`vh` need the real viewport, + /// `%` needs a parent box — none of which this context-free accessor + /// has. Rather than silently returning `0.0` indistinguishably from a + /// deliberate `0px` (the previous behaviour — the "15.6vw renders a + /// completely black frame with no signal" bug), this now warns loudly + /// when the dropped value was a relative unit, same fail-loud contract + /// as [`parse_length_or_warn`] uses for genuinely unparseable input. + /// Callers with a real `LengthContext` should call [`Length::resolve`] + /// instead, which resolves these correctly. pub fn px(&self) -> f32 { - match self.parse() { - ParsedLength::Px(v) => v, - _ => 0.0, - } + px_or_warn(self.parse()) } } @@ -196,12 +221,36 @@ impl LengthPercentage { self.parse().resolve(ctx).unwrap_or(0.0) } - /// Quick px resolution without context (treats em/rem/% as 0). Used by - /// painters that only need the px value of an explicit length. + /// Quick px resolution without context (treats em/rem/%/vw/vh as 0). See + /// [`Length::px`] — same fail-loud contract. pub fn px(&self) -> f32 { - match self.parse() { - ParsedLength::Px(v) => v, - _ => 0.0, + px_or_warn(self.parse()) + } +} + +/// Shared by `Length::px` / `LengthPercentage::px`: return the pixel value +/// for `Px`, or `0.0` for anything else — but if the anything-else is a +/// *relative* unit (issue #125 §2: `%`/`em`/`rem`/`vw`/`vh`), warn loudly +/// first, since silently returning `0.0` here is indistinguishable from a +/// deliberate `0px` and was the exact defect reported (`font-size: "15.6vw"` +/// rendering a fully black frame with `render` exiting 0). `Auto`/`Fr` +/// aren't scalar lengths (the caller decides what they mean), so they don't +/// warn — a context-free accessor asking for their "px value" is a caller +/// bug, not a units bug, and predates this fix. +fn px_or_warn(parsed: ParsedLength) -> f32 { + match parsed { + ParsedLength::Px(v) => v, + other => { + if other.is_relative() { + eprintln!( + "Warning: relative unit {other:?} used where only an absolute px value is \ + supported (no LengthContext available here) — resolving to 0px instead of \ + the intended value. Use an absolute px length, or resolve through \ + LengthContext::resolve() / CssStyle's *_ctx accessors where a context is \ + available." + ); + } + 0.0 } } } @@ -353,4 +402,61 @@ mod tests { let l = LengthPercentage::String("wat".into()); assert_eq!(l.parse(), ParsedLength::Px(0.0)); } + + // ---- issue #125 §2: relative units on the context-free `.px()` path ---- + + #[test] + fn is_relative_classifies_every_variant() { + assert!(!ParsedLength::Px(1.0).is_relative()); + assert!(ParsedLength::Percent(1.0).is_relative()); + assert!(ParsedLength::Em(1.0).is_relative()); + assert!(ParsedLength::Rem(1.0).is_relative()); + assert!(ParsedLength::Vw(1.0).is_relative()); + assert!(ParsedLength::Vh(1.0).is_relative()); + assert!(!ParsedLength::Fr(1.0).is_relative()); + assert!(!ParsedLength::Auto.is_relative()); + } + + #[test] + fn px_still_resolves_absolute_px_correctly() { + assert_eq!(Length::String("24px".into()).px(), 24.0); + assert_eq!(Length::Px(10.0).px(), 10.0); + assert_eq!(LengthPercentage::String("24px".into()).px(), 24.0); + } + + /// `.px()` cannot correctly resolve relative units (no context is + /// available at this call site) — this locks in that it still falls + /// back to `0.0` for them post-fix, same numeric behaviour as before. + /// What changed is that this fallback is now loud (see + /// `px_or_warn`/`is_relative`): distinguishing "the value really is + /// relative, and got dropped" from "the value was genuinely `0px`" is + /// exactly what `is_relative` (tested above) exists to let a caller — + /// or the `px_or_warn` eprintln — detect, since asserting on stderr + /// text isn't practical from a unit test. + #[test] + fn px_falls_back_to_zero_for_every_relative_unit() { + for s in ["50%", "1.5em", "2rem", "100vw", "50vh"] { + assert_eq!(Length::String(s.into()).px(), 0.0, "Length::px() for {s:?}"); + assert_eq!( + LengthPercentage::String(s.into()).px(), + 0.0, + "LengthPercentage::px() for {s:?}" + ); + // And `is_relative` on the same parsed value proves *why*: a + // caller (or px_or_warn) can tell this apart from a real 0px. + assert!(parse_length(s).unwrap().is_relative()); + } + } + + #[test] + fn px_does_not_warn_for_auto_or_fr_context_free_use() { + // Auto/Fr aren't scalar lengths — a context-free `.px()` call on + // them is a caller bug predating this fix, not a relative-unit + // silent-zero. They fall back to 0.0 too, but `is_relative` is + // false for both so `px_or_warn` does not treat them as the + // issue #125 §2 case. + assert!(!ParsedLength::Auto.is_relative()); + assert!(!ParsedLength::Fr(2.0).is_relative()); + assert_eq!(Length::String("auto".into()).px(), 0.0); + } } diff --git a/crates/rustmotion-core/src/engine/renderer/text.rs b/crates/rustmotion-core/src/engine/renderer/text.rs index dd34df2..081be2b 100644 --- a/crates/rustmotion-core/src/engine/renderer/text.rs +++ b/crates/rustmotion-core/src/engine/renderer/text.rs @@ -299,12 +299,37 @@ pub fn measure_text_with_fallback( total_w } -/// Wrap text respecting emoji font fallback for accurate measurement. -pub fn wrap_text_with_fallback( +/// Wrap text respecting emoji font fallback for accurate measurement, +/// honouring `letter_spacing` in the fit test itself (issue #125). +/// +/// This is the correct entry point for any caller whose paint step applies +/// non-zero `letter-spacing` (i.e. it also calls +/// [`measure_text_with_fallback`] / [`draw_text_with_fallback`] with a +/// non-zero `letter_spacing`): the word-fits-on-this-line test below now +/// measures with the *same* tracking that will actually be painted, so the +/// line count this function returns is the line count the paint step will +/// agree with. +/// +/// Before this existed, every caller went through +/// [`wrap_text_with_fallback`], whose fit test always measured at zero +/// tracking regardless of the real value. That is harmless when +/// `letter_spacing == 0.0`, but wrong otherwise in a specific and dangerous +/// way for *negative* tracking (the register this engine targets: tight +/// negative tracking on 200–400px display type): negative tracking makes +/// the real painted line narrower than the zero-tracking fit test believes, +/// so the fit test can decide a line needs to wrap when the real, tighter +/// text would still have fit on one line — and because the box that holds +/// the text is sized from one width sample while the paint step re-derives +/// the same (still zero-tracking) wrap decision against a *different* +/// available width later in layout, the two passes can disagree on the line +/// count, leaving painted lines centered inside a box sized for a different +/// number of lines. +pub fn wrap_text_with_tracking( text: &str, font: &Font, emoji_font: &Option, max_width: Option, + letter_spacing: f32, ) -> Vec { let explicit_lines: Vec<&str> = text.split('\n').collect(); @@ -329,7 +354,7 @@ pub fn wrap_text_with_fallback( format!("{} {}", current_line, word) }; - let width = measure_text_with_fallback(&test, font, emoji_font, 0.0); + let width = measure_text_with_fallback(&test, font, emoji_font, letter_spacing); if width > max_w && !current_line.is_empty() { result.push(current_line); current_line = word.to_string(); @@ -343,3 +368,307 @@ pub fn wrap_text_with_fallback( } result } + +/// Wrap text respecting emoji font fallback for accurate measurement. +/// +/// **Known gap (issue #125):** the fit test below always measures at zero +/// letter-spacing, regardless of what the caller will actually paint with. +/// Kept at its original signature/behaviour for source compatibility with +/// existing call sites (`rustmotion-components/src/intrinsic.rs`, +/// `text.rs`, `shape.rs`, `gradient_text.rs` — outside this workstream's +/// file scope) that measure and paint with real `letter-spacing` but still +/// wrap through this function. **New call sites, and any existing call site +/// whose text can carry non-zero `letter-spacing`, should call +/// [`wrap_text_with_tracking`] instead** — it is a straight drop-in (same +/// signature plus one trailing `letter_spacing: f32` argument) that fixes +/// the measure/paint disagreement described there. +pub fn wrap_text_with_fallback( + text: &str, + font: &Font, + emoji_font: &Option, + max_width: Option, +) -> Vec { + wrap_text_with_tracking(text, font, emoji_font, max_width, 0.0) +} + +// ─── Tests: issue #125 — letter-spacing-aware line breaking ─────────────── + +#[cfg(test)] +mod tracking_tests { + use super::super::typeface_with_fallback; + use super::*; + use skia_safe::{surfaces, AlphaType, Color, ColorType, FontStyle as SkFontStyle, ImageInfo}; + + /// A real bold typeface at `size`, via the same fallback chain + /// (`typeface_with_fallback`) the renderer uses. Doesn't depend on any + /// particular family being installed (falls through to Helvetica/Arial/ + /// the OS default), so this is safe in any CI environment. + fn bold_font(size: f32) -> Font { + let typeface = typeface_with_fallback("Helvetica", SkFontStyle::bold()) + .expect("host must have a fallback typeface"); + Font::from_typeface(typeface, size) + } + + // ---- 1. The wrap decision must use real tracking, not 0.0 ---- + + /// Direct reproduction of issue #125 §1's headline defect ("gains it a + /// line break it did not have at zero"): negative tracking narrows the + /// real painted width below the zero-tracking fit-test's estimate, so + /// there is a width window — real width <= max_w < zero-tracking width — + /// where the buggy zero-tracking test wraps to a second line that the + /// real, tighter text did not need. + #[test] + fn negative_tracking_gains_an_unneeded_break_in_old_wrap_but_not_new() { + let text = "THAT MOVES"; + for font_size in [240.0f32, 290.0, 300.0] { + let font = bold_font(font_size); + let letter_spacing = -9.0f32; + + let real_width = measure_text_with_fallback(text, &font, &None, letter_spacing); + let zero_width = measure_text_with_fallback(text, &font, &None, 0.0); + assert!( + zero_width > real_width, + "negative tracking must make the real width narrower than the \ + zero-tracking estimate at {font_size}px (real={real_width}, zero={zero_width})" + ); + + // A width inside the window the bug lives in: the real (tracked) + // string fits, but the zero-tracking fit test disagrees. + let max_w = (real_width + zero_width) / 2.0; + + let old_lines = wrap_text_with_fallback(text, &font, &None, Some(max_w)); + let new_lines = + wrap_text_with_tracking(text, &font, &None, Some(max_w), letter_spacing); + + assert_eq!( + old_lines.len(), + 2, + "old (zero-tracking) wrap should wrongly split at {font_size}px, got {old_lines:?}" + ); + assert_eq!( + new_lines.len(), + 1, + "new (tracking-aware) wrap should correctly keep one line at {font_size}px, \ + matching what {max_w}px >= real width {real_width}px allows, got {new_lines:?}" + ); + } + } + + /// The fixed wrap decision must be *stable*: re-evaluated at any width + /// sample that is consistent with the real (tracked) content width, it + /// always agrees on line count. This is the structural property whose + /// absence causes issue #125 §1's "the box is sized for one line, paint + /// decides two are needed" — two different width samples taken during + /// layout (the intrinsic-measure pass vs. the final paint pass) must not + /// be able to make the same content wrap differently. + #[test] + fn tracking_aware_wrap_agrees_across_width_samples_old_wrap_does_not() { + let text = "THAT MOVES"; + let font = bold_font(290.0); + let letter_spacing = -9.0f32; + let real_width = measure_text_with_fallback(text, &font, &None, letter_spacing); + let zero_width = measure_text_with_fallback(text, &font, &None, 0.0); + + // Two plausible width samples that both satisfy "the real content + // fits": comfortably above the zero-tracking width, and just above + // the real tracked width (inside the bug's window). + let sample_a = zero_width + 24.0; + let sample_b = real_width + 1.0; + assert!( + sample_b < zero_width, + "test setup: sample_b must be inside the bug window" + ); + + let old_a = wrap_text_with_fallback(text, &font, &None, Some(sample_a)).len(); + let old_b = wrap_text_with_fallback(text, &font, &None, Some(sample_b)).len(); + let new_a = + wrap_text_with_tracking(text, &font, &None, Some(sample_a), letter_spacing).len(); + let new_b = + wrap_text_with_tracking(text, &font, &None, Some(sample_b), letter_spacing).len(); + + assert_ne!( + old_a, old_b, + "reproduction: old wrap must disagree across the two width samples \ + (sample_a={sample_a}, sample_b={sample_b})" + ); + assert_eq!( + new_a, new_b, + "fix: new wrap must agree across the two width samples \ + (sample_a={sample_a}, sample_b={sample_b})" + ); + assert_eq!( + new_a, 1, + "both samples satisfy the real tracked width, so 1 line is correct" + ); + } + + /// `wrap_text_with_fallback` (the pre-existing, still-used-by-default + /// name) must keep its exact old behaviour — 0.0 tracking — so every + /// existing call site's output is unchanged until it opts in to + /// `wrap_text_with_tracking`. + #[test] + fn wrap_text_with_fallback_is_unchanged_zero_tracking_behaviour() { + let text = "THAT MOVES"; + let font = bold_font(290.0); + let max_w = 700.0; + assert_eq!( + wrap_text_with_fallback(text, &font, &None, Some(max_w)), + wrap_text_with_tracking(text, &font, &None, Some(max_w), 0.0) + ); + } + + // ---- 2. Real Skia render + measured ink extent ---- + // + // Reproduces the audit's methodology from issue #125 §1 (render, then + // measure the ink bounding box) directly against this module's public + // functions — the primitive this workstream owns — rather than through + // the full JSON-scenario pipeline. `rustmotion-components/src/ + // intrinsic.rs` and `text.rs` (the actual `text` component's box-sizing + // and paint code) are outside this workstream's file scope and still + // call `wrap_text_with_fallback` (0.0 tracking) as of this fix, so this + // test proves the corrected primitive is centred and self-consistent + // when used correctly end-to-end — i.e. what the render will look like + // once those call sites switch to `wrap_text_with_tracking` — not what + // today's shipped renderer currently outputs for a `text` component. + + /// Draw `lines`, each centred within a `box_width`-wide box at + /// `box_left`, onto a `w`×`h` black raster, and return the horizontal + /// ink centre (mean of the leftmost and rightmost non-black pixel + /// columns across every line) — the same "ink extent" measurement + /// issue #125's audit table reports. + fn render_and_measure_ink_centre_x( + lines: &[String], + font: &Font, + letter_spacing: f32, + box_left: f32, + box_width: f32, + line_height: f32, + top_y: f32, + w: u32, + h: u32, + ) -> Option { + let mut surface = surfaces::raster_n32_premul((w as i32, h as i32)).unwrap(); + let canvas = surface.canvas(); + canvas.clear(Color::BLACK); + let mut paint = Paint::default(); + paint.set_color(Color::WHITE); + paint.set_anti_alias(true); + + let (_, metrics) = font.metrics(); + let ascent = -metrics.ascent; + + for (i, line) in lines.iter().enumerate() { + if line.is_empty() { + continue; + } + let advance = measure_text_with_fallback(line, font, &None, letter_spacing); + let x = box_left + (box_width - advance) / 2.0; + let y = top_y + i as f32 * line_height + ascent; + draw_text_with_fallback(canvas, line, font, &None, letter_spacing, x, y, &paint); + } + + let info = ImageInfo::new( + (w as i32, h as i32), + ColorType::RGBA8888, + AlphaType::Unpremul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + surface.read_pixels(&info, &mut buf, (w * 4) as usize, (0, 0)); + + let mut min_x: Option = None; + let mut max_x: Option = None; + for y in 0..h { + for x in 0..w { + let idx = ((y * w + x) * 4) as usize; + if buf[idx] > 40 { + min_x = Some(min_x.map_or(x, |m| m.min(x))); + max_x = Some(max_x.map_or(x, |m| m.max(x))); + } + } + } + match (min_x, max_x) { + (Some(a), Some(b)) => Some((a as f32 + b as f32) / 2.0), + _ => None, + } + } + + /// The sweep from issue #125 §1's audit table — "THAT MOVES", 1920×1080, + /// centred, `line-height: 0.85` — rendered and measured end-to-end with + /// the fixed primitive. The wrap decision and the paint centring both + /// derive from the *same* tracking-aware measurement, so the box the + /// text is centred in and the lines painted into it never disagree: ink + /// centre must land on the frame centre (960) at every size, whether or + /// not tracking causes a wrap. + #[test] + fn sweep_that_moves_fixed_primitive_stays_centred() { + const W: u32 = 1920; + const H: u32 = 1080; + const FRAME_CENTRE: f32 = 960.0; + let text = "THAT MOVES"; + + // A max-width tight enough that -9 tracking genuinely changes the + // wrap outcome at some of these sizes (mirrors the audit's + // font-size sweep; the exact width is a stand-in for "however wide + // this headline's card/frame is" — not taken from the audit, which + // doesn't publish its scenario JSON). + let box_max_width = 1400.0f32; + + let mut results = Vec::new(); + for &(font_size, letter_spacing) in &[ + (240.0f32, 0.0f32), + (240.0, -9.0), + (290.0, -9.0), + (300.0, -9.0), + ] { + let font = bold_font(font_size); + let line_height = font_size * 0.85; + + let lines = + wrap_text_with_tracking(text, &font, &None, Some(box_max_width), letter_spacing); + let box_width = lines + .iter() + .map(|l| measure_text_with_fallback(l, &font, &None, letter_spacing)) + .fold(0.0f32, f32::max); + let box_left = FRAME_CENTRE - box_width / 2.0; + let top_y = (H as f32 - lines.len() as f32 * line_height) / 2.0; + + let ink_centre = render_and_measure_ink_centre_x( + &lines, + &font, + letter_spacing, + box_left, + box_width, + line_height, + top_y, + W, + H, + ) + .unwrap_or_else(|| panic!("expected ink at {font_size}px/{letter_spacing}")); + + results.push((font_size, letter_spacing, lines.len(), ink_centre)); + + // Tolerance: advance-width centring (what text-align:center + // actually does, here and in the real painter) is not the same + // thing as ink-bbox centring — individual glyphs have left/right + // side-bearings that don't visually balance out, especially + // comparing e.g. "THAT" vs "MOVES" as separate lines. A few px + // of optical vs. advance discrepancy is expected and is not the + // #125 defect (that defect was a 408px miscentre from a wrap/ + // paint disagreement, not glyph-metric noise). + assert!( + (ink_centre - FRAME_CENTRE).abs() < 15.0, + "fixed pipeline ink centre {ink_centre:.1} should be within 15px of \ + {FRAME_CENTRE} at {font_size}px/{letter_spacing} tracking (lines={})", + lines.len() + ); + } + + eprintln!("sweep_that_moves_fixed_primitive_stays_centred (after fix):"); + for (font_size, letter_spacing, line_count, ink_centre) in &results { + eprintln!( + " {font_size}px / {letter_spacing} tracking -> lines={line_count}, ink_centre={ink_centre:.1}" + ); + } + } +}