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}" + ); + } + } +}