diff --git a/.claude/skills/rustmotion/rules/module-structure.md b/.claude/skills/rustmotion/rules/module-structure.md index c72d631..6d1ffc5 100644 --- a/.claude/skills/rustmotion/rules/module-structure.md +++ b/.claude/skills/rustmotion/rules/module-structure.md @@ -35,7 +35,7 @@ src/ │ │ ├── shapes.rs # rounded_rect, circle, arrow paths │ │ └── text.rs # Skia text measurement (line metrics, wrapping) │ └── text/ -│ └── cosmic.rs # cosmic-text FontSystem global + Skia glyph bridge +│ └── cosmic.rs # cosmic-text FontSystem — dormant, pas sur le chemin de rendu ├── schema/ # JSON-serializable data models │ ├── scenario.rs # Scenario, ResolvedScenario, View, ResolvedView, Scene, VideoConfig │ ├── style.rs # Specialized types: CardBorder, CardShadow, Fill, Gradient, etc. diff --git a/CLAUDE.md b/CLAUDE.md index e559b6b..50b85c9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -138,7 +138,7 @@ crates/ │ │ ├── animator.rs # Résolution animations, easing, spring solver │ │ ├── transition.rs # Transitions entre scènes │ │ ├── renderer/ # Primitives Skia (colors, fonts, shapes, text) -│ │ └── text/cosmic.rs # Bridge cosmic-text ↔ Skia (mesure + glyphs) +│ │ └── text/cosmic.rs # Bridge cosmic-text — PAS branché sur le rendu réel │ ├── schema/ # Modèles de données JSON │ │ ├── scenario.rs # Scenario, ResolvedScenario, View, Scene, VideoConfig │ │ ├── style.rs # Specialized types (CardBorder, CardShadow, Fill, etc.) diff --git a/crates/rustmotion-components/src/caption.rs b/crates/rustmotion-components/src/caption.rs index f4da120..056ebe6 100644 --- a/crates/rustmotion-components/src/caption.rs +++ b/crates/rustmotion-components/src/caption.rs @@ -2,7 +2,11 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use skia_safe::{Canvas, Font, FontStyle, Rect}; -use rustmotion_core::css::style::WhiteSpace as CssWhiteSpace; +use rustmotion_core::css::style::{ + FontStyle as CssFontStyle, FontWeight as CssFontWeight, FontWeightKw, + WhiteSpace as CssWhiteSpace, +}; +use rustmotion_core::css::units::LengthContext; use rustmotion_core::css::CssStyle; use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; @@ -43,12 +47,32 @@ rustmotion_core::impl_traits!(Caption { }); impl Caption { - fn paint(&self, canvas: &Canvas, layout_width: f32, layout_height: f32, time: f64) { + fn paint(&self, canvas: &Canvas, layout_width: f32, layout_height: f32, ctx: &PaintCtx) { + let time = ctx.time; let font_size = self.style.font_size_px_or(48.0); let color = self.style.color_str_or("#FFFFFF"); let font_family = self.style.font_family_or("Inter"); - let Ok(typeface) = typeface_with_fallback(font_family, FontStyle::bold()) else { + // #9: `letter-spacing`/`line-height` `em`/`%` resolve against this + // element's own font-size (just above); `vw`/`vh` resolve against + // the real viewport, available here via `ctx` (mirrors + // `text.rs::paint`'s `type_ctx`). + let type_ctx = 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, + }; + + // #9: derive weight/slant from `style.font-weight`/`font-style` + // instead of always painting bold. `CaptionIntrinsic` (via + // `TextIntrinsic`) measures at whatever weight the style declares + // (400/normal when unset) — painting an unconditional bold made the + // glyphs wider than the box that was centred/measured for them. + let font_style = Self::resolve_font_style(&self.style); + + let Ok(typeface) = typeface_with_fallback(font_family, font_style) else { return; }; @@ -182,8 +206,20 @@ impl Caption { self.style.white_space, Some(CssWhiteSpace::Nowrap | CssWhiteSpace::Pre) ); + // #1: when `max_width` is unset, wrap at the box `layout` + // actually gave this caption (matches `text.rs:442-451`) + // instead of never wrapping — `CaptionIntrinsic` measures + // (and taffy reserves a box) against that same width, so + // painting at `f32::MAX` here painted a single line far + // wider than the reserved box, bleeding past it and past + // the viewport with `validate` never seeing the mismatch + // (it re-measures via the same intrinsic, not this paint + // path). let max_width = if nowrap { f32::MAX + } else if layout_width.is_finite() && layout_width > 0.0 { + self.max_width + .map_or(layout_width, |mw| mw.min(layout_width)) } else { self.max_width.unwrap_or(f32::MAX) }; @@ -203,7 +239,16 @@ impl Caption { current_x += word_width + space_width; } - let line_height = font_size * 1.4; + // #9: honour `style.line-height` like `CaptionIntrinsic` + // does (via `TextIntrinsic::from_parts` -> + // `line_height_for_ctx`) instead of a hardcoded 1.4 — the + // box taffy reserves is sized from the former, so painting + // with the latter drifted the line spacing away from what + // was measured (7.7% at the unset default, arbitrarily more + // with an explicit `line-height`), and the caption's own + // vertical clip (below in the outer `paint`) silently crops + // whatever spills past the mismatch. + let line_height = self.style.line_height_for_ctx(font_size, &type_ctx); let cx = layout_width / 2.0; if let Some(bg_color) = self.style.background_color_str() { @@ -291,6 +336,29 @@ impl Caption { let paint = paint_from_hex(self.pill_color.as_deref().unwrap_or(DEFAULT_PILL_COLOR)); canvas.draw_rrect(skia_safe::RRect::new_rect_xy(rect, radius, radius), &paint); } + + /// #9: the Skia `FontStyle` to paint with, derived from `style.font- + /// weight`/`font-style` — mirrors `text.rs`'s weight/slant mapping and + /// `intrinsic.rs`'s `weight_to_u16` (used to measure the box), so the + /// weight the box was measured at and the weight painted into it always + /// agree. Pulled out as its own function so it's directly unit-testable + /// without needing to render anything. + fn resolve_font_style(style: &CssStyle) -> FontStyle { + let weight = match &style.font_weight { + Some(CssFontWeight::Keyword(FontWeightKw::Bold | FontWeightKw::Bolder)) => { + skia_safe::font_style::Weight::BOLD + } + Some(CssFontWeight::Number(n)) if *n >= 600 => skia_safe::font_style::Weight::BOLD, + Some(CssFontWeight::Number(n)) => skia_safe::font_style::Weight::from(*n as i32), + _ => skia_safe::font_style::Weight::NORMAL, + }; + let slant = match style.font_style { + Some(CssFontStyle::Italic) => skia_safe::font_style::Slant::Italic, + Some(CssFontStyle::Oblique) => skia_safe::font_style::Slant::Oblique, + _ => skia_safe::font_style::Slant::Upright, + }; + FontStyle::new(weight, skia_safe::font_style::Width::NORMAL, slant) + } } impl Painter for Caption { @@ -301,7 +369,7 @@ impl Painter for Caption { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - self.paint(canvas, layout.width, layout.height, ctx.time); + self.paint(canvas, layout.width, layout.height, ctx); } } @@ -333,7 +401,30 @@ mod tests { use rustmotion_core::css::Length; use rustmotion_core::schema::CaptionWord; + /// A `PaintCtx` for tests that don't care about frame/fps bookkeeping — + /// only `time` and, since #9, the viewport dims threaded into the + /// `LengthContext` used to resolve `vw`/`vh` typography units. + fn test_ctx(time: f64) -> PaintCtx { + PaintCtx { + time, + scene_duration: 2.0, + frame_index: (time * 30.0) as u32, + fps: 30, + video_width: 1920, + video_height: 1080, + stagger_offset: 0.0, + } + } + fn make_caption(text: &str, white_space: Option) -> Caption { + make_caption_with_max_width(text, white_space, Some(80.0)) + } + + fn make_caption_with_max_width( + text: &str, + white_space: Option, + max_width: Option, + ) -> Caption { let words = text .split_whitespace() .map(|w| CaptionWord { @@ -346,7 +437,7 @@ mod tests { words, active_color: default_active_color(), mode: CaptionStyle::Highlight, - max_width: Some(80.0), + max_width, pill_color: None, style: CssStyle { font_size: Some(Length::Px(28.0)), @@ -410,7 +501,7 @@ mod tests { let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); { let canvas = surface.canvas(); - caption.paint(canvas, W as f32, H as f32, 0.5); + caption.paint(canvas, W as f32, H as f32, &test_ctx(0.5)); } let (_minx, _maxx, miny, _maxy) = ink_bounds(&mut surface, W, H).expect("caption must paint something"); @@ -431,7 +522,7 @@ mod tests { let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); { let canvas = surface.canvas(); - caption.paint(canvas, W as f32, H as f32, 0.1); + caption.paint(canvas, W as f32, H as f32, &test_ctx(0.1)); } let (_minx, _maxx, miny, _maxy) = ink_bounds(&mut surface, W, H).expect("word_pop caption must paint something"); @@ -453,7 +544,7 @@ mod tests { { let canvas = surface.canvas(); canvas.translate((800.0, 250.0)); - caption.paint(canvas, 80.0, H as f32, 0.5); + caption.paint(canvas, 80.0, H as f32, &test_ctx(0.5)); } let (minx, maxx, miny, maxy) = ink_bounds(&mut surface, W, H).expect("nowrap caption must paint something"); @@ -479,7 +570,7 @@ mod tests { { let canvas = surface.canvas(); canvas.translate((800.0, 250.0)); - caption.paint(canvas, 80.0, H as f32, 0.5); + caption.paint(canvas, 80.0, H as f32, &test_ctx(0.5)); } let (minx, maxx, miny, maxy) = ink_bounds(&mut surface, W, H).expect("wrapped caption must paint something"); @@ -495,4 +586,186 @@ mod tests { maxy - miny ); } + + // ─── #1: wrap at the box's layout_width when max_width is unset ─────── + + #[test] + fn wraps_at_layout_width_when_max_width_is_unset() { + // Reproduction: no `max_width` on the caption (the common case — a + // caption's box comes from wherever it's placed, e.g. a card), but + // the layout pass still hands `paint` a real, finite `layout_width` + // (mirrors `CaptionIntrinsic`, which measures against exactly this + // width). Before the fix, `max_width.unwrap_or(f32::MAX)` ignored + // `layout_width` entirely and painted one line stretching far past + // the box — and past the viewport in the audit's repro. + let caption = make_caption_with_max_width( + "the quick brown fox jumps over the lazy dog again", + None, + None, // no explicit max_width + ); + const W: i32 = 1600; + const H: i32 = 400; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + canvas.translate((800.0, 200.0)); + // The box the layout pass assigned: 300px wide, well short of + // this sentence's unwrapped width at font-size 28. + caption.paint(canvas, 300.0, H as f32, &test_ctx(0.5)); + } + let (minx, maxx, miny, maxy) = + ink_bounds(&mut surface, W, H).expect("caption must paint something"); + + assert!( + maxx - minx < 320, + "must wrap within ~layout_width (300px), got ink width {}", + maxx - minx + ); + assert!( + maxy - miny > 50, + "must spread across multiple lines when max_width is unset, got ink height {}", + maxy - miny + ); + } + + #[test] + fn nowrap_still_ignores_layout_width_when_max_width_is_unset() { + // Regression guard: the #1 fix must not touch `white-space: + // nowrap`'s existing "always ignore any width constraint" contract. + let caption = make_caption_with_max_width( + "the quick brown fox jumps over the lazy dog", + Some(CssWhiteSpace::Nowrap), + None, + ); + const W: i32 = 1600; + const H: i32 = 400; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + canvas.translate((800.0, 200.0)); + caption.paint(canvas, 300.0, H as f32, &test_ctx(0.5)); + } + let (minx, maxx, miny, maxy) = + ink_bounds(&mut surface, W, H).expect("caption must paint something"); + + assert!( + maxx - minx > 400, + "nowrap must still bleed past layout_width, got ink width {}", + maxx - minx + ); + assert!( + maxy - miny < 50, + "nowrap must stay on one line, got ink height {}", + maxy - miny + ); + } + + // ─── #9: line-height / font-weight measure-vs-paint parity ──────────── + + #[test] + fn honours_style_line_height_instead_of_hardcoded_1_4() { + // Reproduction: `style.line-height: 0.9` must change the vertical + // gap between wrapped lines. Before the fix, the painter always + // used `font_size * 1.4` regardless of `style.line-height`, while + // `CaptionIntrinsic` (the box taffy reserves) honoured it — a + // caption author following rules/typography-readability.md's + // guidance to set `line-height` got a box sized for their value but + // glyphs painted at a fixed 1.4. + let mut tight = make_caption_with_max_width( + "one two three four five six seven eight", + None, + Some(80.0), + ); + tight.style.line_height = Some(rustmotion_core::css::style::LineHeight::Number(0.9)); + let mut loose = make_caption_with_max_width( + "one two three four five six seven eight", + None, + Some(80.0), + ); + loose.style.line_height = Some(rustmotion_core::css::style::LineHeight::Number(2.0)); + + const W: i32 = 1600; + const H: i32 = 800; + + let mut surf_tight = + skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surf_tight.canvas(); + canvas.translate((800.0, 50.0)); + tight.paint(canvas, 80.0, H as f32, &test_ctx(0.5)); + } + let (_, _, _, tight_maxy) = + ink_bounds(&mut surf_tight, W, H).expect("tight caption must paint something"); + + let mut surf_loose = + skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surf_loose.canvas(); + canvas.translate((800.0, 50.0)); + loose.paint(canvas, 80.0, H as f32, &test_ctx(0.5)); + } + let (_, _, _, loose_maxy) = + ink_bounds(&mut surf_loose, W, H).expect("loose caption must paint something"); + + assert!( + loose_maxy > tight_maxy + 50, + "line-height: 2.0 must spread lines much further than 0.9 \ + (tight bottom={tight_maxy}, loose bottom={loose_maxy})" + ); + } + + // `Caption::resolve_font_style` is the exact weight/slant computation + // `paint` uses; testing it directly is deterministic regardless of + // whether the system's resolved "bold" and "normal" typefaces happen to + // have visually/metrically distinct advance widths on this particular + // host (on this machine, Helvetica's bold and normal share identical + // glyph metrics — a pixel-width comparison would pass whether or not + // `paint` used the right weight, which isn't a real check of the fix). + + #[test] + fn resolve_font_style_defaults_to_normal_matching_the_intrinsic_measurement() { + // #9 (weight half): `CaptionIntrinsic` (via `TextIntrinsic`'s + // `weight_to_u16`) measures at weight 400 when `style.font-weight` + // is unset. Before the fix, `paint` ignored `style.font-weight` + // entirely and always painted `FontStyle::bold()` (weight 700) — a + // silent measure-vs-paint weight mismatch on every caption that + // doesn't set an explicit font-weight (the common case). + let style = CssStyle::default(); + let resolved = Caption::resolve_font_style(&style); + assert_eq!( + *resolved.weight(), + 400, + "unset font-weight must resolve to normal (400), not a hardcoded bold" + ); + } + + #[test] + fn resolve_font_style_honours_explicit_bold_and_numeric_weight() { + let bold = CssStyle { + font_weight: Some(CssFontWeight::Keyword(FontWeightKw::Bold)), + ..Default::default() + }; + assert_eq!(*Caption::resolve_font_style(&bold).weight(), 700); + + // Below the >=600 "treat as bold" threshold (same threshold + // `text.rs`'s equivalent mapping uses), so the exact numeric value + // passes through unchanged. + let numeric = CssStyle { + font_weight: Some(CssFontWeight::Number(350)), + ..Default::default() + }; + assert_eq!(*Caption::resolve_font_style(&numeric).weight(), 350); + } + + #[test] + fn resolve_font_style_honours_italic() { + let italic = CssStyle { + font_style: Some(CssFontStyle::Italic), + ..Default::default() + }; + assert_eq!( + Caption::resolve_font_style(&italic).slant(), + skia_safe::font_style::Slant::Italic + ); + } } diff --git a/crates/rustmotion-components/src/codeblock/highlight.rs b/crates/rustmotion-components/src/codeblock/highlight.rs index 8dfc5cf..dc9c6b5 100644 --- a/crates/rustmotion-components/src/codeblock/highlight.rs +++ b/crates/rustmotion-components/src/codeblock/highlight.rs @@ -539,6 +539,25 @@ pub(crate) fn resolve_monospace_font(family: &str, size: f32, weight: FontWeight skia_safe::font_style::Width::NORMAL, skia_safe::font_style::Slant::Upright, ); + + // #7: a custom/Google font declared in the scenario for `family` must + // win over the hardcoded monospace fallback list below — check the + // custom registry directly, first. Previously the only place that + // consulted it was `typeface_with_fallback`, reached solely through the + // final `.or_else` below; but the `fallbacks` list's `match_family_style` + // calls (which try `family` itself first, among plain system families) + // already return *something* on essentially every real system — Skia's + // system `FontMgr` almost never returns `None` for "JetBrains Mono"/ + // "Fira Code"/"Menlo"/"Courier New"/"monospace" collectively — so that + // `.or_else` was never reached and a declared custom font (an Anton + // `.ttf`, a Google "IBM Plex Mono") was silently ignored (see commit + // b4603f9, which fixed the equivalent regression for `text`). + if let Some(typeface) = + rustmotion_core::engine::renderer::resolve_custom_typeface(family, style) + { + return Some(Font::from_typeface(typeface, size)); + } + let fallbacks = [ family, "JetBrains Mono", @@ -556,3 +575,61 @@ pub(crate) fn resolve_monospace_font(family: &str, size: f32, weight: FontWeight })?; Some(Font::from_typeface(typeface, size)) } + +#[cfg(test)] +mod monospace_font_tests { + use super::*; + + /// #7 reproduction: a codeblock's declared custom/Google font must + /// actually be used, not silently shadowed by the hardcoded monospace + /// fallback chain. Registers a real display face (Anton — visually + /// nothing like any of "JetBrains Mono"/"Fira Code"/"Menlo"/"Courier + /// New"/"monospace") under a family name that collides with none of + /// them, and asserts `resolve_monospace_font` actually resolves to it. + /// Skips on a cold font cache (no network access in CI) — the render QA + /// in `examples/` (e.g. `cb_anton.json`) is the visual counterpart. + #[test] + fn declared_custom_font_wins_over_hardcoded_monospace_fallbacks() { + let path = format!( + "{}/.cache/rustmotion/fonts/anton-400.ttf", + std::env::var("HOME").unwrap_or_default() + ); + let Ok(bytes) = std::fs::read(&path) else { + return; // cold font cache → skip (render QA covers it) + }; + + let font_mgr = rustmotion_core::engine::renderer::font_mgr(); + let parsed = font_mgr + .new_from_data(&skia_safe::Data::new_copy(&bytes), None) + .expect("cached TTF must parse"); + let parsed_style = parsed.font_style(); + rustmotion_core::engine::renderer::register_custom_font_variant( + "RmProbeCodeblockAnton", + bytes, + *parsed_style.weight(), + false, + ); + + let font = resolve_monospace_font("RmProbeCodeblockAnton", 20.0, FontWeight::Normal) + .expect("resolve_monospace_font must succeed"); + assert_eq!( + font.typeface().family_name(), + "Anton", + "declared custom font must win over the hardcoded monospace fallback chain, got {}", + font.typeface().family_name() + ); + } + + /// Regression guard: an *undeclared* family (nothing in the custom + /// registry) must still fall through to a real monospace font via the + /// hardcoded chain, not fail or silently switch to some arbitrary + /// serif/sans system default. + #[test] + fn unregistered_family_still_falls_back_to_a_monospace_font() { + let font = resolve_monospace_font("RmProbeNoSuchFamilyXYZ", 20.0, FontWeight::Normal) + .expect("must still resolve a fallback font"); + // Not asserting a specific family name (host-dependent) — just that + // resolution succeeds and doesn't panic/None out. + assert!(font.size() > 0.0); + } +} diff --git a/crates/rustmotion-components/src/codeblock/render.rs b/crates/rustmotion-components/src/codeblock/render.rs index 6c770ba..1c98c46 100644 --- a/crates/rustmotion-components/src/codeblock/render.rs +++ b/crates/rustmotion-components/src/codeblock/render.rs @@ -94,7 +94,7 @@ pub(super) fn render_codeblock( let x = layout.x; let y = layout.y; - let (pad_top, pad_right, _pad_bottom, pad_left) = padding; + let (pad_top, pad_right, pad_bottom, pad_left) = padding; let corner_radius = layer.style.border_radius_px_or(12.0); let bg_color = layer.style.background_color_str().unwrap_or("#2b303b"); @@ -114,8 +114,40 @@ pub(super) fn render_codeblock( let code_x = x + pad_left + gutter_width; let code_y = y + chrome_height + pad_top; - let scroll_offset = if layer.auto_scroll && natural_height > total_height + 0.5 { - natural_height - total_height + // #4: the non-transition (typewriter/reveal) path only paints + // `visible_lines` lines, not the full `current_code` — `natural_height` + // (used below) is the height of *all* the code, revealed or not. Using + // it for the scroll offset made the offset constant and maximal from + // t=0, translating the not-yet-revealed lines' eventual position + // upward by the full amount immediately: the first lines to reveal sit + // above the clip, invisible, until the reveal has caught up with that + // fixed offset (reproduced: 60% of a 4s typewriter reveal painted zero + // text pixels). Compute reveal state up front so the scroll offset can + // be based on what's actually drawn — matches `terminal.rs`'s + // `content_h = visible_lines * line_h + padding + chrome_h` formula, + // which has never had this bug. + let reveal_state = if transition.is_none() { + let highlighted = highlight_code(¤t_code, &layer.language, theme); + let (visible_lines, visible_chars, last_line_opacity) = + compute_reveal(layer, time, &highlighted); + Some((highlighted, visible_lines, visible_chars, last_line_opacity)) + } else { + None + }; + + // Diff transitions (`render_diff_transition`) always paint the entire + // lerped diff, with no partial reveal — `natural_height` (the lerped + // dims_a/dims_b height) already matches what gets drawn for that path, + // so it needs no `visible_lines` adjustment; only the reveal path did. + let drawn_height = match &reveal_state { + Some((_, visible_lines, _, _)) => { + *visible_lines as f32 * actual_line_height + pad_top + pad_bottom + chrome_height + } + None => natural_height, + }; + + let scroll_offset = if layer.auto_scroll { + (drawn_height - total_height).max(0.0) } else { 0.0 }; @@ -149,9 +181,8 @@ pub(super) fn render_codeblock( trans, ); } else { - let highlighted = highlight_code(¤t_code, &layer.language, theme); - let (visible_lines, visible_chars, last_line_opacity) = - compute_reveal(layer, time, &highlighted); + let (highlighted, visible_lines, visible_chars, last_line_opacity) = + reveal_state.expect("reveal_state is always Some when transition is None"); if layer.show_line_numbers { draw_line_numbers( diff --git a/crates/rustmotion-components/src/intrinsic.rs b/crates/rustmotion-components/src/intrinsic.rs index 9e67e9b..5eb0215 100644 --- a/crates/rustmotion-components/src/intrinsic.rs +++ b/crates/rustmotion-components/src/intrinsic.rs @@ -24,7 +24,12 @@ use crate::gradient_text::GradientText; use crate::kbd::Kbd; use crate::text::Text; -/// Cosmic-text–backed intrinsic measurer for [`Text`]. +/// Skia-backed intrinsic measurer for [`Text`] (audit #10: despite the name +/// this module's doc header suggests, this uses `skia_safe::Font:: +/// measure_str` via `engine::renderer::text`'s fallback-aware helpers — the +/// same primitives `Text::paint` draws with — not `engine::text::cosmic`, +/// which has no callers on the real render path at all; see that module's +/// doc comment). pub struct TextIntrinsic { content: String, font_family: Option, @@ -58,20 +63,38 @@ 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 + // No *real* `LengthContext` (real viewport, real parent width) 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. + // needs to pass in is not a call to make unilaterally here). + // + // But `letter-spacing`'s and `line-height`'s `em`/`%` resolve + // against this element's *own* font-size (not the parent's, not the + // viewport) — CSS spec, also documented on + // `CssStyle::letter_spacing_px_ctx`/`line_height_for_ctx` — and that + // own font-size is already known right here, with zero signature + // change needed. Building a `LengthContext` carrying just that + // resolved `font_size` (defaults for everything else) and using the + // `_ctx` resolvers closes the measure-vs-paint divergence for `em`/ + // `%` specifically (`Text`/`Caption`'s painters already resolve + // these two properties with the real `PaintCtx`'s viewport, but + // `em`/`%` on them don't read the viewport at all, so the two agree + // regardless of what viewport this default carries). `vw`/`vh`/ + // `rem` on `letter-spacing`/`line-height` remain unresolved against + // the *real* viewport here (they fall back to this struct's default + // 1920×1080/16px root) — closing that fully needs the real + // `VideoConfig` plumbed through `box_builder.rs`/`geometry.rs`, + // still out of scope for the reasons above. let font_size = style.font_size_px_or(48.0); - let line_height_resolved = style.line_height_for(font_size); + let own_ctx = rustmotion_core::css::units::LengthContext { + font_size, + ..rustmotion_core::css::units::LengthContext::default() + }; + let line_height_resolved = style.line_height_for_ctx(font_size, &own_ctx); Self { content: content.to_string(), font_family: style.font_family.clone(), @@ -79,7 +102,7 @@ impl TextIntrinsic { line_height_resolved, weight: weight_to_u16(style.font_weight.as_ref()), italic: matches!(style.font_style, Some(CssFontStyle::Italic)), - letter_spacing: style.letter_spacing_px(), + letter_spacing: style.letter_spacing_px_ctx(&own_ctx), max_width, wrap: true, } @@ -404,8 +427,8 @@ fn _line_height_unused(_: Option<&LineHeight>) {} // ───────────────────────────────────────────────────────────────────────────── use crate::terminal::{ - Terminal, CHROME_HEIGHT, FONT_SIZE as TERM_FONT_SIZE, LINE_HEIGHT as TERM_LINE_HEIGHT, - PADDING as TERM_PADDING, + resolve_typeface as resolve_terminal_typeface, Terminal, CHROME_HEIGHT, + FONT_SIZE as TERM_FONT_SIZE, LINE_HEIGHT as TERM_LINE_HEIGHT, PADDING as TERM_PADDING, }; /// Intrinsic measurer for [`Terminal`]. @@ -445,8 +468,10 @@ impl TerminalIntrinsic { } fn measure_max_width(t: &Terminal, font_size: f32) -> f32 { - let font_style = skia_safe::FontStyle::normal(); - let Ok(typeface) = typeface_with_fallback("SF Mono", font_style) else { + // Same resolver the painter calls — see `terminal::resolve_typeface`. + // Measuring with one face and painting with another is how text ends up + // overflowing a box the geometry pass has already approved. + let Some(typeface) = resolve_terminal_typeface(&t.style) else { // Font unavailable (CI without fonts); return 0 — the layout will // be width-unconstrained and the container drives the size. return 0.0; @@ -1091,4 +1116,147 @@ mod tests { w ); } + + // ─── #2 / #5: em/% typography resolve against own font-size, not 0 ──── + + fn text_with_style(content: &str, style: CssStyle) -> Text { + Text { + content: content.into(), + max_width: None, + timing: Default::default(), + style, + timeline: Vec::new(), + stagger: None, + text_shadow: None, + stroke: None, + text_background: None, + } + } + + #[test] + fn line_height_percent_no_longer_collapses_the_box_to_zero_height() { + // #2 reproduction: `line-height: "150%"` went through the + // context-free `line_height_for`, which cannot resolve `%` and + // silently fell back to 0 — the intrinsic then reported a + // `line_count * 0.0 = 0` height, so `paint_pass.rs`'s `if height <= + // 0.0 { return }` guard skipped painting the node (and its + // subtree) entirely, even though `validate` reported success. + use rustmotion_core::css::units::LengthPercentage; + let text = text_with_style( + "VISIBLE?", + CssStyle { + font_size: Some(Length::Px(60.0)), + line_height: Some(LineHeight::Length(LengthPercentage::String("150%".into()))), + ..Default::default() + }, + ); + let m = TextIntrinsic::from_text(&text); + let (_w, h) = m.measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ); + assert!( + (h - 90.0).abs() < 0.5, + "line-height: 150% of a 60px font-size must resolve to 90px (own font-size, per \ + CSS), got {h}" + ); + } + + #[test] + fn line_height_em_no_longer_collapses_the_box_to_zero_height() { + use rustmotion_core::css::units::LengthPercentage; + let text = text_with_style( + "VISIBLE?", + CssStyle { + font_size: Some(Length::Px(60.0)), + line_height: Some(LineHeight::Length(LengthPercentage::String("1.5em".into()))), + ..Default::default() + }, + ); + let m = TextIntrinsic::from_text(&text); + let (_w, h) = m.measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ); + assert!( + (h - 90.0).abs() < 0.5, + "line-height: 1.5em of a 60px font-size must resolve to 90px, got {h}" + ); + // Sanity: matches the already-correct unitless-number form exactly, + // proving em and the bare-number multiplier agree. + let numeric = text_with_style( + "VISIBLE?", + CssStyle { + font_size: Some(Length::Px(60.0)), + line_height: Some(LineHeight::Number(1.5)), + ..Default::default() + }, + ); + let (_w, h_numeric) = TextIntrinsic::from_text(&numeric).measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ); + assert_eq!(h, h_numeric); + } + + #[test] + fn letter_spacing_em_matches_the_equivalent_px_measurement() { + // #5 reproduction: `letter-spacing: "1.2em"` at font-size 200 + // (=240px) went through the context-free `letter_spacing_px`, which + // returns 0 for `em` — the intrinsic reserved a box as if tracking + // were 0 while `Text::paint` (which already uses the `_ctx` + // resolver) painted with the real 240px tracking, so `validate`'s + // `unwrappable_text_overflow`/viewport checks (which re-measure via + // this same intrinsic) never saw the real, wider painted width. + let em_style = CssStyle { + font_size: Some(Length::Px(200.0)), + letter_spacing: Some(Length::String("1.2em".into())), + white_space: Some(WhiteSpace::Nowrap), + ..Default::default() + }; + let px_style = CssStyle { + font_size: Some(Length::Px(200.0)), + letter_spacing: Some(Length::Px(240.0)), + white_space: Some(WhiteSpace::Nowrap), + ..Default::default() + }; + let w_em = TextIntrinsic::from_text(&text_with_style("TRACKING", em_style)) + .measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ) + .0; + let w_px = TextIntrinsic::from_text(&text_with_style("TRACKING", px_style)) + .measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ) + .0; + assert!( + (w_em - w_px).abs() < 1.0, + "letter-spacing: 1.2em (font-size 200) must measure the same as the equivalent \ + 240px value: em={w_em}, px={w_px}" + ); + // And it must differ from the old (broken) zero-tracking width — + // otherwise this test would pass vacuously even if em still + // resolved to 0. + let w_zero_tracking = TextIntrinsic::from_text(&text_with_style( + "TRACKING", + CssStyle { + font_size: Some(Length::Px(200.0)), + white_space: Some(WhiteSpace::Nowrap), + ..Default::default() + }, + )) + .measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ) + .0; + assert!( + w_em > w_zero_tracking + 100.0, + "em tracking must measurably widen the line versus zero tracking: em={w_em}, \ + zero={w_zero_tracking}" + ); + } } diff --git a/crates/rustmotion-components/src/terminal.rs b/crates/rustmotion-components/src/terminal.rs index 668e176..0e1d8ce 100644 --- a/crates/rustmotion-components/src/terminal.rs +++ b/crates/rustmotion-components/src/terminal.rs @@ -6,7 +6,8 @@ use rustmotion_core::css::CssStyle; use rustmotion_core::engine::animator::{ease, AnimatedProperties}; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::engine::renderer::{ - draw_text_with_fallback, emoji_typeface, paint_from_hex, typeface_with_fallback, + draw_text_with_fallback, emoji_typeface, paint_from_hex, resolve_custom_typeface, + typeface_with_fallback, }; use rustmotion_core::schema::{CodeblockReveal, RevealMode, TimelineStep}; use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig}; @@ -130,10 +131,24 @@ pub(crate) const FONT_SIZE: f32 = 14.0; pub(crate) const LINE_HEIGHT: f32 = 22.0; pub(crate) const PADDING: f32 = 16.0; +/// The typeface both the painter and the intrinsic measurement resolve. +/// +/// These must never diverge. The measurement reserves the box the painter then +/// fills, so a different face on either side produces text that overflows a box +/// the geometry validator has already declared safe — the exact failure mode the +/// audit found across the text components. Both sides used to hardcode +/// `"SF Mono"` independently, which agreed only by coincidence and ignored an +/// explicit `font-family` outright, custom or not. +pub(crate) fn resolve_typeface(style: &CssStyle) -> Option { + let font_style = skia_safe::FontStyle::normal(); + let family = style.font_family_or("SF Mono"); + resolve_custom_typeface(family, font_style) + .or_else(|| typeface_with_fallback(family, font_style).ok()) +} + impl Terminal { fn make_font(&self) -> Option { - let font_style = skia_safe::FontStyle::normal(); - let typeface = typeface_with_fallback("SF Mono", font_style).ok()?; + let typeface = resolve_typeface(&self.style)?; let size = self.style.font_size_px_or(FONT_SIZE); Some(skia_safe::Font::from_typeface(typeface, size)) } diff --git a/crates/rustmotion-components/tests/caption_presets.rs b/crates/rustmotion-components/tests/caption_presets.rs index 6b5a680..44183f4 100644 --- a/crates/rustmotion-components/tests/caption_presets.rs +++ b/crates/rustmotion-components/tests/caption_presets.rs @@ -19,10 +19,16 @@ const H: u32 = 300; /// Renders a single caption component (absolutely positioned at y=150 so the /// baseline-anchored glyphs are fully on-canvas) and returns the RGBA buffer. fn render_caption(json: serde_json::Value, time: f64) -> Vec { + render_caption_at(json, time, 150.0) +} + +/// Same as [`render_caption`] but with an explicit vertical position, for +/// tests whose caption spans more lines than fit below the default y=150. +fn render_caption_at(json: serde_json::Value, time: f64, y: f32) -> Vec { let component: Component = serde_json::from_value(json).expect("deserialize caption"); let child = ChildComponent { component, - position: Some(PositionMode::Absolute { x: 0.0, y: 150.0 }), + position: Some(PositionMode::Absolute { x: 0.0, y }), x: None, y: None, z_index: None, @@ -193,3 +199,65 @@ fn karaoke_pop_highlights_active_word_with_pill() { "inactive words not visible: {inactive} white pixels" ); } + +/// Audit finding #1: a caption with no `max_width` set, given a box +/// narrower than its unwrapped content via `style.width` (mirrors a caption +/// placed inside a card, the documented use case), must wrap to fit that +/// box — not paint one wide line that bleeds out of it. Routed through the +/// real pipeline so `CaptionIntrinsic` (the box taffy reserves) and +/// `Caption::paint` (what actually gets drawn) are exercised together: this +/// is exactly the measure-vs-paint pairing the geometry validator depends +/// on to catch overflow, and before the fix the two disagreed silently. +#[test] +fn wraps_within_its_layout_box_when_max_width_is_unset() { + let json = serde_json::json!({ + "type": "caption", + "mode": "highlight", + "words": [ + { "text": "the", "start": 0.0, "end": 100.0 }, + { "text": "quick", "start": 0.0, "end": 100.0 }, + { "text": "brown", "start": 0.0, "end": 100.0 }, + { "text": "fox", "start": 0.0, "end": 100.0 }, + { "text": "jumps", "start": 0.0, "end": 100.0 }, + { "text": "over", "start": 0.0, "end": 100.0 }, + { "text": "the", "start": 0.0, "end": 100.0 }, + { "text": "lazy", "start": 0.0, "end": 100.0 }, + { "text": "dog", "start": 0.0, "end": 100.0 } + ], + // No `max_width` — the box comes entirely from `style.width` below, + // mirroring a caption inside a fixed-width card. + "style": { "width": "150px", "font-size": 24, "color": "#FFFFFF" } + }); + let buf = render_caption_at(json, 0.5, 20.0); + let (minx, maxx, miny, maxy) = ink_bounds(&buf).expect("caption must paint something"); + + assert!( + maxx - minx < 200, + "must wrap to roughly the 150px box width, got ink width {}", + maxx - minx + ); + assert!( + maxy - miny > 60, + "must spread across multiple lines (9 words don't fit 150px on one line at 30px \ + font-size), got ink height {}", + maxy - miny + ); +} + +/// Bounding box (min_x, max_x, min_y, max_y) of every non-transparent pixel +/// in an RGBA8888 `W`x`H` buffer, or `None` if nothing was painted. +fn ink_bounds(buf: &[u8]) -> Option<(i32, i32, i32, i32)> { + let (mut minx, mut maxx, mut miny, mut maxy) = (i32::MAX, i32::MIN, i32::MAX, i32::MIN); + for y in 0..H as i32 { + for x in 0..W as i32 { + let idx = ((y * W as i32 + x) * 4 + 3) as usize; + if buf[idx] > 0 { + minx = minx.min(x); + maxx = maxx.max(x); + miny = miny.min(y); + maxy = maxy.max(y); + } + } + } + (minx <= maxx).then_some((minx, maxx, miny, maxy)) +} diff --git a/crates/rustmotion-components/tests/codeblock_auto_scroll.rs b/crates/rustmotion-components/tests/codeblock_auto_scroll.rs new file mode 100644 index 0000000..80bcabe --- /dev/null +++ b/crates/rustmotion-components/tests/codeblock_auto_scroll.rs @@ -0,0 +1,141 @@ +//! Pixel test for codeblock `auto_scroll` during a typewriter reveal +//! (audit finding #4). +//! +//! Routes a `codeblock` through the real pipeline (box_builder, run_layout, +//! paint_tree) exactly like `caption_presets.rs` does, and counts painted +//! (non-background) pixels at several points during a typewriter reveal. +//! Before the fix, `scroll_offset` was computed from the *full* code's +//! natural height regardless of how many lines the reveal had actually +//! painted, so the box stayed empty for the majority of the reveal — the +//! newly-revealed lines were translated above the clip and invisible until +//! the reveal caught up with that constant offset. + +use rustmotion_components::box_builder::{build_scene_with_anim, BuildAnimationCtx}; +use rustmotion_components::legacy_dispatch::LegacyPaintDispatcher; +use rustmotion_components::{ChildComponent, Component, PositionMode}; +use rustmotion_core::css::taffy_bridge::ConversionContext; +use rustmotion_core::engine::layout_pass::run_layout; +use rustmotion_core::engine::paint_pass::{paint_tree, PaintFrame}; + +const W: u32 = 1000; +const H: u32 = 600; +const SCENE_DURATION: f64 = 4.5; + +fn codeblock_json() -> serde_json::Value { + let lines: Vec = (0..40).map(|i| format!("let line_{i} = {i};")).collect(); + let code = lines.join("\n"); + serde_json::json!({ + "type": "codeblock", + "code": code, + "language": "rust", + "auto_scroll": true, + "reveal": { "mode": "typewriter", "start": 0.0, "duration": 4.0 }, + "style": { "width": "900px", "height": "300px", "font-size": 20 } + }) +} + +/// Count pixels whose color differs measurably from the codeblock's own +/// dark background (`#2b303b` when unset) — i.e. actual glyph ink, not just +/// "anything non-transparent" (the background rect itself is opaque and +/// covers the whole box). +fn text_ink_pixels(buf: &[u8]) -> usize { + // Background is #2b303b ~ (43, 48, 59). Count pixels that deviate from + // that by a wide margin in any channel — syntect's theme colors are all + // much brighter than the near-black background. + buf.chunks_exact(4) + .filter(|p| { + let (r, g, b, a) = (p[0] as i32, p[1] as i32, p[2] as i32, p[3] as i32); + a > 200 && ((r - 43).abs() > 40 || (g - 48).abs() > 40 || (b - 59).abs() > 40) + }) + .count() +} + +fn render_codeblock_at(time: f64) -> Vec { + let component: Component = serde_json::from_value(codeblock_json()).expect("deserialize"); + let child = ChildComponent { + component, + position: Some(PositionMode::Absolute { x: 50.0, y: 150.0 }), + x: None, + y: None, + z_index: None, + bleed: false, + }; + let children = vec![child]; + + let mut surface = + skia_safe::surfaces::raster_n32_premul((W as i32, H as i32)).expect("raster surface"); + let canvas = surface.canvas(); + canvas.clear(skia_safe::Color4f::new(0.0, 0.0, 0.0, 0.0)); + + let built = build_scene_with_anim( + &children, + (W as f32, H as f32), + BuildAnimationCtx { + time, + scene_duration: SCENE_DURATION, + fps: 30, + }, + ); + let layout = run_layout( + &built.root, + (W as f32, H as f32), + &ConversionContext::default(), + ); + let dispatcher = LegacyPaintDispatcher::for_scene(&built); + let frame = PaintFrame { + time, + frame_index: (time * 30.0) as u32, + fps: 30, + video_width: W, + video_height: H, + scene_duration: SCENE_DURATION, + camera: None, + }; + paint_tree(canvas, &built.root, &layout, &frame, &dispatcher); + + let row_bytes = W as usize * 4; + let mut pixels = vec![0u8; row_bytes * H as usize]; + let info = skia_safe::ImageInfo::new( + (W as i32, H as i32), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + surface.read_pixels(&info, &mut pixels, row_bytes, (0, 0)); + pixels +} + +#[test] +fn early_reveal_shows_text_not_an_empty_box() { + // At t=0.5s (12.5% into a 4s typewriter reveal, well past the first + // line), some text must already be visible — before the fix, this + // stayed at 0 ink pixels until ~75% of the reveal had elapsed. + let ink = text_ink_pixels(&render_codeblock_at(0.5)); + assert!( + ink > 20, + "expected visible text ink early in the reveal (t=0.5s), got {ink} ink pixels" + ); +} + +#[test] +fn mid_reveal_shows_text_not_an_empty_box() { + // t=2.0s: 50% into the reveal — well within the range the audit + // measured as a completely empty box (0 ink pixels at t=0.5/1.0/2.0). + let ink = text_ink_pixels(&render_codeblock_at(2.0)); + assert!( + ink > 20, + "expected visible text ink mid-reveal (t=2.0s), got {ink} ink pixels" + ); +} + +#[test] +fn ink_grows_monotonically_enough_across_the_reveal() { + // Sanity: as more of the reveal completes, more text should be on + // screen (loosely monotonic — auto_scroll keeps only the visible + // window, so it won't be strictly increasing forever, but the box must + // never regress to empty once text has started appearing). + let t_early = text_ink_pixels(&render_codeblock_at(0.5)); + let t_late = text_ink_pixels(&render_codeblock_at(3.9)); + assert!(t_early > 0, "must have visible ink at t=0.5s, got 0"); + assert!(t_late > 0, "must have visible ink at t=3.9s, got 0"); +} diff --git a/crates/rustmotion-core/src/engine/renderer/fonts.rs b/crates/rustmotion-core/src/engine/renderer/fonts.rs index 9388ab3..879d86c 100644 --- a/crates/rustmotion-core/src/engine/renderer/fonts.rs +++ b/crates/rustmotion-core/src/engine/renderer/fonts.rs @@ -13,57 +13,131 @@ use super::google_fonts::{font_cache_dir, resolve_google_font}; thread_local! { static THREAD_FONT_MGR: FontMgr = FontMgr::default(); // Per-thread cache of Typefaces built from the global custom-font bytes, - // so each render thread builds each custom face at most once. - static CUSTOM_TYPEFACES: RefCell> = RefCell::new(HashMap::new()); + // keyed by (family, weight, italic) — the exact variant `custom_typeface` + // picked — so each render thread builds each custom face at most once. + static CUSTOM_TYPEFACES: RefCell> = + RefCell::new(HashMap::new()); } pub fn font_mgr() -> FontMgr { THREAD_FONT_MGR.with(|mgr| mgr.clone()) } +/// One registered custom-font file: its raw bytes plus the `(weight, +/// italic)` style Skia parsed out of the file itself when it was +/// registered — the ground truth for what that file actually renders as, +/// independent of which nominal weight the caller happened to request it +/// under. +#[derive(Clone)] +struct CustomFontVariant { + data: Vec, + weight: i32, + italic: bool, +} + /// Global registry of custom/Google-font bytes, keyed by family name. Filled /// once by [`load_custom_fonts`] on the main thread; read by every render -/// thread through [`custom_typeface`]. A family maps to the first file -/// registered for it (one weight per custom family via this path — sufficient -/// for accent display faces; multi-weight custom families are future work). -fn custom_font_registry() -> &'static Mutex>> { - static REG: OnceLock>>> = OnceLock::new(); +/// thread through [`custom_typeface`]. Each family holds every distinct +/// `(weight, italic)` variant registered for it — e.g. a Google Fonts +/// declaration with `weights: [400, 700]` registers two variants — so +/// [`custom_typeface`] can pick whichever is the closest match to what a +/// paint call asks for, instead of always returning the first file that +/// happened to register (the previous behaviour: every variant after the +/// first was invisible, and every weight/style request resolved to +/// whichever one file won the race). +fn custom_font_registry() -> &'static Mutex>> { + static REG: OnceLock>>> = OnceLock::new(); REG.get_or_init(|| Mutex::new(HashMap::new())) } -/// Store a custom font's bytes under `family` (first registration wins). -pub fn register_custom_font_bytes(family: &str, data: Vec) { +/// Register a custom font's bytes under `family`, tagged with the `(weight, +/// italic)` style Skia reports for the parsed file. A no-op if that exact +/// `(family, weight, italic)` combination is already registered. +pub fn register_custom_font_variant(family: &str, data: Vec, weight: i32, italic: bool) { let mut reg = custom_font_registry() .lock() .unwrap_or_else(|e| e.into_inner()); - reg.entry(family.to_string()).or_insert(data); + let variants = reg.entry(family.to_string()).or_default(); + if !variants + .iter() + .any(|v| v.weight == weight && v.italic == italic) + { + variants.push(CustomFontVariant { + data, + weight, + italic, + }); + } } -/// The raw bytes registered for `family`, if any (test/introspection helper). -pub fn custom_font_bytes(family: &str) -> Option> { - custom_font_registry() +/// The raw bytes registered for `family`'s closest `(weight, italic)` match, +/// if any variant is registered under that family (test/introspection +/// helper). +#[cfg(test)] +fn custom_font_bytes(family: &str, weight: i32, italic: bool) -> Option> { + let reg = custom_font_registry() .lock() - .unwrap_or_else(|e| e.into_inner()) - .get(family) - .cloned() + .unwrap_or_else(|e| e.into_inner()); + let variants = reg.get(family)?; + closest_variant(variants, weight, italic).map(|v| v.data.clone()) +} + +/// Pick the registered variant closest to `(weight, italic)`: exact +/// italic-ness match preferred, then the smallest weight distance — the +/// same nearest-match spirit as CSS font matching (`font-weight`/ +/// `font-style` never fail to resolve to *something*, they resolve to the +/// closest available face). +fn closest_variant( + variants: &[CustomFontVariant], + weight: i32, + italic: bool, +) -> Option<&CustomFontVariant> { + variants.iter().min_by_key(|v| { + let italic_penalty = if v.italic == italic { 0 } else { 1_000_000 }; + italic_penalty + (v.weight - weight).abs() + }) } -/// Resolve a registered custom font to a Typeface, building it from the global -/// bytes on first use per thread and caching it thereafter. `None` when no -/// custom font is registered under `family`. -fn custom_typeface(family: &str) -> Option { +/// Resolve a registered custom font to a Typeface for the requested `style`, +/// building it from the global bytes on first use per thread and caching it +/// thereafter. `None` when no custom font is registered under `family`. +fn custom_typeface(family: &str, style: FontStyle) -> Option { + let weight = *style.weight(); + let italic = style.slant() != skia_safe::font_style::Slant::Upright; + let cache_key = (family.to_string(), weight, italic); CUSTOM_TYPEFACES.with(|cache| { - if let Some(tf) = cache.borrow().get(family) { + if let Some(tf) = cache.borrow().get(&cache_key) { return Some(tf.clone()); } - let data = custom_font_bytes(family)?; + let data = { + let reg = custom_font_registry() + .lock() + .unwrap_or_else(|e| e.into_inner()); + let variants = reg.get(family)?; + closest_variant(variants, weight, italic)?.data.clone() + }; let sk_data = skia_safe::Data::new_copy(&data); let tf = font_mgr().new_from_data(&sk_data, None)?; - cache.borrow_mut().insert(family.to_string(), tf.clone()); + cache.borrow_mut().insert(cache_key, tf.clone()); Some(tf) }) } +/// Look up only the custom/Google-font registry for `family` at the +/// requested `style`, without falling through to any system font. Exposed +/// for callers (e.g. `codeblock`/`terminal`'s monospace font resolver) that +/// need to check "did the scenario declare a custom font for this family" +/// *before* trying their own family-specific system fallback chain — unlike +/// [`typeface_with_fallback`], which interleaves a single system-family +/// lookup between the custom check and its own generic Helvetica/Arial +/// catch-all, an order that doesn't suit every caller (see issue: codeblock/ +/// terminal's hardcoded monospace fallback list was never reached because +/// `typeface_with_fallback`'s own system lookup already matched a decoy +/// system family, e.g. "JetBrains Mono"). +pub fn resolve_custom_typeface(family: &str, style: FontStyle) -> Option { + custom_typeface(family, style) +} + /// Validate a `FontEntry` and resolve it to a list of TTF file paths. /// /// - Local entry (`path` set, `source` absent): returns `[path]` as-is. @@ -132,20 +206,28 @@ fn register_font_file(font_mgr: &FontMgr, family: &str, path: &std::path::Path) match std::fs::read(path) { Ok(data) => { let sk_data = skia_safe::Data::new_copy(&data); - if font_mgr.new_from_data(&sk_data, None).is_none() { + let Some(tf) = font_mgr.new_from_data(&sk_data, None) else { eprintln!( "Warning: failed to register custom font '{}' from '{}'", family, path.display() ); return; - } + }; // Skia's default FontMgr can build a Typeface from `new_from_data` // but never exposes it to `match_family_style` (name lookup only // sees installed system fonts). So keep the raw bytes in a global // registry; `typeface_with_fallback` builds and caches a Typeface - // from them per thread, ahead of the system match. - register_custom_font_bytes(family, data); + // from them per thread, ahead of the system match. Tag the + // variant with the (weight, italic) Skia parsed out of the file + // itself — the ground truth for what it actually renders as — + // so a family with several registered weights (e.g. Google + // Fonts `weights: [400, 700]`) exposes every one of them instead + // of only whichever file happened to register first. + let parsed_style = tf.font_style(); + let weight = *parsed_style.weight(); + let italic = parsed_style.slant() != skia_safe::font_style::Slant::Upright; + register_custom_font_variant(family, data, weight, italic); } Err(e) => { eprintln!( @@ -166,8 +248,10 @@ fn register_font_file(font_mgr: &FontMgr, family: &str, path: &std::path::Path) pub fn typeface_with_fallback(family: &str, style: FontStyle) -> Result { // Custom/Google fonts declared in the scenario win over system fonts: // they are not visible to `match_family_style`, so resolve them from the - // registry first. - if let Some(t) = custom_typeface(family) { + // registry first — matched against the requested `style` so a family + // registered with several weights picks the right one instead of + // whichever file happened to register first (#6). + if let Some(t) = custom_typeface(family, style) { return Ok(t); } let fm = font_mgr(); @@ -200,6 +284,42 @@ pub fn emoji_typeface() -> Option { EMOJI_TF.with(|tf| tf.clone()) } +/// Resolve a system fallback typeface that actually contains a glyph for +/// `c`, for when `primary_family`'s own face doesn't cover it (audit #3: +/// CJK/Arabic/Devanagari/other scripts rendered as `.notdef` tofu when only +/// a Latin `font-family` was requested, because neither measurement nor +/// painting ever looked past the single requested typeface). This is +/// Skia's font-fallback-by-character API — the same mechanism a browser +/// uses to substitute, say, a CJK font for Chinese text embedded in an +/// otherwise-Latin paragraph, instead of leaving `.notdef` tofu. Memoized +/// per thread (keyed on the inputs that actually affect the OS's fallback +/// decision) since callers may probe this once per uncovered code point +/// during run segmentation. Returns `None` if no installed font covers `c` +/// either — the caller falls back to the originally requested (tofu- +/// producing) font, exactly the pre-fix behaviour, not worse. +pub fn fallback_typeface_for_char( + primary_family: &str, + style: FontStyle, + c: char, +) -> Option { + thread_local! { + static FALLBACK_CACHE: RefCell>> = + RefCell::new(HashMap::new()); + } + let weight = *style.weight(); + let italic = style.slant() != skia_safe::font_style::Slant::Upright; + let key = (primary_family.to_string(), weight, italic, c as u32); + FALLBACK_CACHE.with(|cache| { + if let Some(hit) = cache.borrow().get(&key) { + return hit.clone(); + } + let resolved = + font_mgr().match_family_style_character(primary_family, style, &[], c as i32); + cache.borrow_mut().insert(key, resolved.clone()); + resolved + }) +} + // ─── Unit tests ────────────────────────────────────────────────────────────── #[cfg(test)] @@ -251,15 +371,60 @@ mod tests { } #[test] - fn custom_font_registry_stores_first_and_serves_bytes() { - register_custom_font_bytes("RmProbeRegistryFamily", vec![1, 2, 3]); - // First registration wins (a later weight must not clobber it). - register_custom_font_bytes("RmProbeRegistryFamily", vec![9, 9]); + fn custom_font_registry_stores_distinct_weights_and_serves_bytes() { + register_custom_font_variant("RmProbeRegistryFamily", vec![1, 2, 3], 400, false); + // A *different* (weight, italic) is a genuinely new variant — not a + // clobber of the first (the old `family`-only-keyed `or_insert` + // registry made every registration after the first invisible; #6). + register_custom_font_variant("RmProbeRegistryFamily", vec![9, 9], 700, false); assert_eq!( - custom_font_bytes("RmProbeRegistryFamily"), + custom_font_bytes("RmProbeRegistryFamily", 400, false), Some(vec![1, 2, 3]) ); - assert!(custom_font_bytes("RmProbeUnregistered").is_none()); + assert_eq!( + custom_font_bytes("RmProbeRegistryFamily", 700, false), + Some(vec![9, 9]) + ); + assert!(custom_font_bytes("RmProbeUnregistered", 400, false).is_none()); + } + + #[test] + fn registering_the_same_weight_twice_keeps_the_first() { + register_custom_font_variant("RmProbeDupeFamily", vec![1, 2, 3], 400, false); + register_custom_font_variant("RmProbeDupeFamily", vec![9, 9], 400, false); + assert_eq!( + custom_font_bytes("RmProbeDupeFamily", 400, false), + Some(vec![1, 2, 3]), + "re-registering the same (weight, italic) must not clobber the first file" + ); + } + + #[test] + fn custom_typeface_lookup_picks_the_closest_registered_weight() { + // Pure selection-logic reproduction of #6's fix mechanism, + // independent of any font actually installed on the host: three + // variants registered under one family; a lookup for an + // intermediate weight must pick the *closest* one, not always the + // first registered — the defect the audit measured (bold and + // normal always resolving to the same file). + register_custom_font_variant("RmProbeClosestFamily", vec![1], 400, false); + register_custom_font_variant("RmProbeClosestFamily", vec![2], 700, false); + register_custom_font_variant("RmProbeClosestFamily", vec![3], 900, false); + + let reg = custom_font_registry() + .lock() + .unwrap_or_else(|e| e.into_inner()); + let variants = reg.get("RmProbeClosestFamily").expect("registered above"); + assert_eq!( + closest_variant(variants, 650, false).unwrap().weight, + 700, + "650 should resolve to the nearest registered weight, 700" + ); + assert_eq!( + closest_variant(variants, 100, false).unwrap().weight, + 400, + "100 should resolve to the nearest registered weight, 400" + ); } /// The bug this fix targets: a registered custom family must resolve to the @@ -275,7 +440,17 @@ mod tests { let Ok(bytes) = std::fs::read(&path) else { return; // cold font cache → skip (render QA covers it) }; - register_custom_font_bytes("Anton", bytes); + let fm = font_mgr(); + let parsed = fm + .new_from_data(&skia_safe::Data::new_copy(&bytes), None) + .expect("cached TTF must parse"); + let style = parsed.font_style(); + register_custom_font_variant( + "Anton", + bytes, + *style.weight(), + style.slant() != skia_safe::font_style::Slant::Upright, + ); let tf = typeface_with_fallback("Anton", FontStyle::normal()).unwrap(); assert_eq!( tf.family_name(), @@ -284,6 +459,56 @@ mod tests { ); } + /// End-to-end reproduction of #6: a family registered with two distinct + /// weights (mirrors `fonts: [{"family":"Inter","source":"google", + /// "weights":[400,700]}]`) must resolve *different* typefaces for + /// `font-weight: normal` vs `font-weight: bold`. Before the fix, + /// `custom_typeface` ignored `style` entirely and `register_custom_ + /// font_bytes` kept only the first-registered file, so the audit's two + /// rendered PNGs (bold vs normal) came out byte-for-byte identical. + /// Skips on a cold font cache (no network access in CI) — the render QA + /// in `examples/` is the visual counterpart. + #[test] + fn family_with_two_registered_weights_resolves_distinct_typefaces() { + let cache_dir = format!( + "{}/.cache/rustmotion/fonts", + std::env::var("HOME").unwrap_or_default() + ); + let (Ok(normal_bytes), Ok(bold_bytes)) = ( + std::fs::read(format!("{cache_dir}/inter-400.ttf")), + std::fs::read(format!("{cache_dir}/inter-700.ttf")), + ) else { + return; // cold font cache → skip (render QA covers it) + }; + + let fm = font_mgr(); + let normal_parsed = fm + .new_from_data(&skia_safe::Data::new_copy(&normal_bytes), None) + .expect("cached TTF must parse"); + let bold_parsed = fm + .new_from_data(&skia_safe::Data::new_copy(&bold_bytes), None) + .expect("cached TTF must parse"); + let normal_weight = *normal_parsed.font_style().weight(); + let bold_weight = *bold_parsed.font_style().weight(); + + register_custom_font_variant("RmProbeInterFamily", normal_bytes, normal_weight, false); + register_custom_font_variant("RmProbeInterFamily", bold_bytes, bold_weight, false); + + let resolved_normal = + typeface_with_fallback("RmProbeInterFamily", FontStyle::normal()).unwrap(); + let resolved_bold = + typeface_with_fallback("RmProbeInterFamily", FontStyle::bold()).unwrap(); + + assert_ne!( + *resolved_normal.font_style().weight(), + *resolved_bold.font_style().weight(), + "requesting normal vs bold on the same custom family must resolve different weights \ + (both used to resolve to whichever file registered first)" + ); + assert_eq!(*resolved_bold.font_style().weight(), bold_weight); + assert_eq!(*resolved_normal.font_style().weight(), normal_weight); + } + #[test] fn neither_path_nor_source_is_error() { let entry = neither_entry(); diff --git a/crates/rustmotion-core/src/engine/renderer/text.rs b/crates/rustmotion-core/src/engine/renderer/text.rs index 081be2b..d607dad 100644 --- a/crates/rustmotion-core/src/engine/renderer/text.rs +++ b/crates/rustmotion-core/src/engine/renderer/text.rs @@ -1,4 +1,4 @@ -use skia_safe::{Canvas, Font, Paint, Point, TextBlob}; +use skia_safe::{Canvas, Font, Paint, Point, TextBlob, Typeface}; // ─── Counter formatting ───────────────────────────────────────────────────── @@ -120,8 +120,14 @@ pub fn make_text_blob_with_spacing(text: &str, font: &Font, spacing: f32) -> Opt // ─── Emoji support ────────────────────────────────────────────────────────── -/// Check if a character is an emoji or emoji-related codepoint. -fn is_emoji(c: char) -> bool { +/// Code points that render as emoji **by default**, in every context, +/// regardless of any following variation selector — genuine pictograph +/// blocks (Miscellaneous Symbols and Pictographs, Emoticons, Transport, +/// Supplemental Symbols/Pictographs, flags, keycaps, ZWJ sequences...). +/// Nothing in these ranges has a meaningful plain-text rendering, so there +/// is no narrowing to do here — contrast with +/// [`is_text_presentation_by_default`], which needs one (audit #8). +fn is_emoji_presentation_default(c: char) -> bool { let cp = c as u32; matches!(cp, // Miscellaneous Symbols and Pictographs (includes skin tone modifiers 1F3FB-1F3FF) @@ -136,12 +142,6 @@ fn is_emoji(c: char) -> bool { 0x1FA00..=0x1FA6F | // Symbols and Pictographs Extended-B 0x1FA70..=0x1FAFF | - // Dingbats (includes ✂️..➰ and arrows/symbols) - 0x2702..=0x27B0 | - // Miscellaneous Symbols (includes ☀️..⛿) - 0x2600..=0x26FF | - // Variation Selectors (keep with preceding emoji) - 0xFE00..=0xFE0F | // Zero-Width Joiner 0x200D | // Combining Enclosing Keycap @@ -152,68 +152,247 @@ fn is_emoji(c: char) -> bool { 0xE0020..=0xE007F | // Playing cards, mahjong 0x1F004 | 0x1F0CF | - // Misc technical (⌚ ⌛ ⏩..⏳ ⏸..⏺) - 0x231A..=0x231B | + // Misc technical, unconditionally emoji (⏩..⏳ ⏸..⏺) 0x23E9..=0x23F3 | 0x23F8..=0x23FA | - // Arrows and geometric symbols used as emoji + // Arrows and geometric symbols, unconditionally emoji + 0x2B1B..=0x2B1C | + 0x2B50 | 0x2B55 + ) +} + +/// Code points that are TEXT-presentation **by default** — a normal glyph +/// in the primary font, honouring `style.color` — but that Unicode still +/// marks `Emoji=Yes`: they render as color emoji only when the author +/// explicitly opts in with a following U+FE0F variation selector (audit +/// #8). Routing these unconditionally to the emoji font (the previous +/// behaviour, lumped in with [`is_emoji_presentation_default`]) either +/// painted a color bitmap that ignores `style.color` (✔, ©, ®, ™ — all +/// covered by Apple Color Emoji) or a `.notdef` tofu square for code points +/// the emoji font itself doesn't cover even though the primary font does +/// (✓ U+2713 — absent from Apple Color Emoji even though U+2714 sits one +/// code point over and is present). +fn is_text_presentation_by_default(c: char) -> bool { + let cp = c as u32; + matches!(cp, + // Copyright, registered, trademark + 0x00A9 | 0x00AE | 0x2122 | + // Misc technical (⌚ ⌛) + 0x231A..=0x231B | + // Miscellaneous Symbols (☀️..⛿) + 0x2600..=0x26FF | + // Dingbats (✂️..➰ and arrows/symbols), includes ✓/✔ U+2713/2714 + 0x2702..=0x27B0 | + // Arrows and geometric symbols used as emoji only with VS16 0x2934..=0x2935 | 0x25AA..=0x25AB | 0x25B6 | 0x25C0 | 0x25FB..=0x25FE | - // Arrows 0x2B05..=0x2B07 | - 0x2B1B..=0x2B1C | - 0x2B50 | 0x2B55 | // CJK symbols 0x3030 | 0x303D | - 0x3297 | 0x3299 | - // Copyright, registered, trademark - 0x00A9 | 0x00AE | 0x2122 + 0x3297 | 0x3299 ) } -/// A segment of text that uses either the primary font or the emoji font. -struct TextRun { - start: usize, // byte offset - end: usize, // byte offset - is_emoji: bool, +/// Variation Selector-16 — forces the *preceding* text-presentation-default +/// code point (see [`is_text_presentation_by_default`]) into emoji +/// presentation. Narrower than the old catch-all `0xFE00..=0xFE0F` range: +/// VS-15 (U+FE0E, forces *text* presentation) and the rest of that block +/// are not emoji-forcing. +const VARIATION_SELECTOR_EMOJI: char = '\u{FE0F}'; + +/// True if `c` should be painted/measured with the emoji font, given the +/// character immediately following it (`next`). Needed because +/// [`is_text_presentation_by_default`] code points only opt into emoji +/// presentation when explicitly followed by U+FE0F — a bare lookup of `c` +/// alone can't tell "©" (text) from "©️" (explicit emoji presentation) +/// apart. +fn char_wants_emoji_font(c: char, next: Option) -> bool { + if c == VARIATION_SELECTOR_EMOJI { + return true; // always grouped with whatever code point selected it + } + if is_emoji_presentation_default(c) { + return true; + } + if is_text_presentation_by_default(c) { + return next == Some(VARIATION_SELECTOR_EMOJI); + } + false } -/// Segment text into runs of emoji vs non-emoji characters. -fn segment_text_runs(text: &str) -> Vec { - let mut runs = Vec::new(); - let mut chars = text.char_indices().peekable(); +/// Which font a run of text should be painted/measured with. +enum RunKind { + Primary, + Emoji, + /// #3: a system fallback typeface resolved for a code point the + /// primary font doesn't cover (e.g. CJK/Arabic/Devanagari when only a + /// Latin `font-family` was requested). + Fallback(Typeface), +} + +/// True if `a` and `b` are the "same" run kind for the purpose of merging +/// adjacent characters into one run. Two `Fallback` runs merge only if they +/// resolved to the *same* typeface (compared by Skia's unique id) — two +/// characters that both need fallback but belong to different scripts +/// (e.g. mixed CJK + Arabic) must not merge into a single run painted with +/// only one of the two fonts. +fn same_run_kind(a: &RunKind, b: &RunKind) -> bool { + match (a, b) { + (RunKind::Primary, RunKind::Primary) => true, + (RunKind::Emoji, RunKind::Emoji) => true, + (RunKind::Fallback(ta), RunKind::Fallback(tb)) => ta.unique_id() == tb.unique_id(), + _ => false, + } +} - while let Some(&(start_byte, c)) = chars.peek() { - let emoji = is_emoji(c); +/// True if `font` has an actual glyph (not `.notdef`, glyph id 0) for every +/// character in `text`. Used both to gate whether a code point needs a +/// fallback lookup at all (#3), and as a coverage guard before actually +/// using the emoji font for a run classified as emoji (#8): some code +/// points Unicode marks emoji-capable are, on a given platform, absent +/// from the color emoji font even though the primary font has a perfectly +/// good text glyph for them (U+2713 on Apple Color Emoji). +fn font_covers(font: &Font, text: &str) -> bool { + let glyphs = font.str_to_glyphs_vec(text); + !glyphs.is_empty() && glyphs.iter().all(|&g| g != 0) +} + +/// Classify a single character's font choice: emoji presentation first +/// (#8), then primary-font glyph coverage, then a system fallback typeface +/// resolved via [`super::fallback_typeface_for_char`] for the primary +/// font's own `(family, style)` (#3). Whitespace/control code points are +/// always `Primary` (assumed universally present, or invisible) so the +/// fallback lookup isn't triggered by the spaces between same-script words. +fn classify_char(c: char, primary: &Font, next: Option) -> RunKind { + if char_wants_emoji_font(c, next) { + return RunKind::Emoji; + } + if c.is_whitespace() || (c as u32) < 0x20 { + return RunKind::Primary; + } + if primary.unichar_to_glyph(c as i32) != 0 { + return RunKind::Primary; + } + let primary_typeface = primary.typeface(); + let style = primary_typeface.font_style(); + let family = primary_typeface.family_name(); + match super::fallback_typeface_for_char(&family, style, c) { + Some(tf) => RunKind::Fallback(tf), + // No installed font covers `c` either — same `.notdef` tofu as + // before this fix, not worse. + None => RunKind::Primary, + } +} + +/// Segment text into runs, each tagged with which font should paint/measure +/// it (see [`classify_char`]). +fn segment_text_runs(text: &str, primary: &Font) -> Vec { + let chars: Vec<(usize, char)> = text.char_indices().collect(); + let mut runs = Vec::new(); + let mut i = 0; + while i < chars.len() { + let (start_byte, c) = chars[i]; + let next = chars.get(i + 1).map(|&(_, ch)| ch); + let kind = classify_char(c, primary, next); let mut end_byte = start_byte + c.len_utf8(); - chars.next(); + i += 1; - while let Some(&(_, next_c)) = chars.peek() { - if is_emoji(next_c) != emoji { + while let Some(&(nb, nc)) = chars.get(i) { + let nnext = chars.get(i + 1).map(|&(_, ch)| ch); + let nkind = classify_char(nc, primary, nnext); + if !same_run_kind(&kind, &nkind) { break; } - end_byte += next_c.len_utf8(); - chars.next(); + end_byte = nb + nc.len_utf8(); + i += 1; } runs.push(TextRun { start: start_byte, end: end_byte, - is_emoji: emoji, + kind, }); } runs } -/// Check if text contains any emoji characters. +/// A segment of text tagged with which font it should be painted/measured +/// with (see [`RunKind`]). +struct TextRun { + start: usize, // byte offset + end: usize, // byte offset + kind: RunKind, +} + +/// True if `text` contains any character that wants emoji presentation +/// (#8-aware: honours the VS16 opt-in for text-presentation-default code +/// points, so plain "©"/"✓" no longer count as emoji here). pub fn has_emoji(text: &str) -> bool { - text.chars().any(is_emoji) + let chars: Vec = text.chars().collect(); + chars + .iter() + .enumerate() + .any(|(i, &c)| char_wants_emoji_font(c, chars.get(i + 1).copied())) } -/// Draw a text line with emoji font fallback. -/// If `emoji_font` is None, falls back to drawing everything with the primary font. +/// True if `text` needs the full run-segmentation machinery in +/// [`draw_text_with_fallback`]/[`measure_text_with_fallback`]: it contains +/// emoji-presentation content (and an emoji font is actually available), or +/// at least one non-whitespace/control code point the primary `font` +/// doesn't cover (#3). When neither is true, every caller's existing +/// single-font fast path is exactly correct and segmentation would be +/// wasted work. +fn needs_segmentation(text: &str, primary: &Font, emoji_font: &Option) -> bool { + if emoji_font.is_some() && has_emoji(text) { + return true; + } + text.chars().any(|c| { + // ASCII short-circuits before the `unichar_to_glyph` FFI call: every + // font this engine resolves covers printable ASCII, and callers + // that draw a lot of short spans per frame (codeblock's per-token + // syntax highlighting, in particular) call this once per span — + // skipping the Skia round-trip for the overwhelmingly common + // all-ASCII case keeps #3's fix from adding per-glyph FFI overhead + // to code that was never affected by the tofu/coverage bug it + // fixes. + !(c.is_ascii() || c.is_whitespace() || (c as u32) < 0x20) + && primary.unichar_to_glyph(c as i32) == 0 + }) +} + +/// Resolve which `Font` a run should actually be painted/measured with, +/// applying the emoji coverage guard (#8) and building a same-size `Font` +/// from a resolved fallback [`Typeface`] (#3). Returns a borrow of one of +/// the two inputs, or an owned font stored in `owned` to keep the borrow +/// alive at the call site. +fn resolve_run_font<'a>( + kind: &RunKind, + segment: &str, + primary: &'a Font, + emoji_font: &'a Option, + owned: &'a mut Option, +) -> &'a Font { + match kind { + RunKind::Primary => primary, + RunKind::Emoji => match emoji_font { + Some(ef) if font_covers(ef, segment.trim_end_matches(VARIATION_SELECTOR_EMOJI)) => ef, + // Coverage guard (#8): the emoji font doesn't actually have + // this glyph (or isn't available at all) — fall back to the + // primary font rather than paint `.notdef`. + _ => primary, + }, + RunKind::Fallback(tf) => { + *owned = Some(Font::from_typeface(tf.clone(), primary.size())); + owned.as_ref().unwrap() + } + } +} + +/// Draw a text line with emoji-font and glyph-coverage fallback (#3, #8). +/// If `emoji_font` is None, falls back to drawing everything with the +/// primary font (plus any `#3` system fallback the primary font's coverage +/// gap needs). pub fn draw_text_with_fallback( canvas: &Canvas, text: &str, @@ -224,8 +403,9 @@ pub fn draw_text_with_fallback( y: f32, paint: &Paint, ) { - // Fast path: no emoji font or no emoji in text - if emoji_font.is_none() || !has_emoji(text) { + // Fast path: nothing here needs emoji presentation or a glyph-coverage + // fallback — every earlier caller's exact previous behaviour. + if !needs_segmentation(text, font, emoji_font) { if letter_spacing.abs() > 0.01 { if let Some(blob) = make_text_blob_with_spacing(text, font, letter_spacing) { canvas.draw_text_blob(&blob, (x, y), paint); @@ -236,13 +416,13 @@ pub fn draw_text_with_fallback( return; } - let emoji_font = emoji_font.as_ref().unwrap(); - let runs = segment_text_runs(text); + let runs = segment_text_runs(text, font); let mut cursor_x = x; for run in &runs { let segment = &text[run.start..run.end]; - let f = if run.is_emoji { emoji_font } else { font }; + let mut owned = None; + let f = resolve_run_font(&run.kind, segment, font, emoji_font, &mut owned); if letter_spacing.abs() > 0.01 { if let Some(blob) = make_text_blob_with_spacing(segment, f, letter_spacing) { @@ -263,7 +443,9 @@ pub fn draw_text_with_fallback( } } -/// Measure the width of a text line with emoji font fallback. +/// Measure the width of a text line with emoji-font and glyph-coverage +/// fallback (#3, #8) — mirrors [`draw_text_with_fallback`] run-for-run so +/// the width this returns always matches what actually gets painted. pub fn measure_text_with_fallback( text: &str, font: &Font, @@ -271,7 +453,7 @@ pub fn measure_text_with_fallback( letter_spacing: f32, ) -> f32 { // Fast path - if emoji_font.is_none() || !has_emoji(text) { + if !needs_segmentation(text, font, emoji_font) { let (w, _) = font.measure_str(text, None); let extra = if letter_spacing.abs() > 0.01 { letter_spacing * (text.chars().count() as f32 - 1.0).max(0.0) @@ -281,13 +463,13 @@ pub fn measure_text_with_fallback( return w + extra; } - let emoji_font = emoji_font.as_ref().unwrap(); - let runs = segment_text_runs(text); + let runs = segment_text_runs(text, font); let mut total_w = 0.0f32; for run in &runs { let segment = &text[run.start..run.end]; - let f = if run.is_emoji { emoji_font } else { font }; + let mut owned = None; + let f = resolve_run_font(&run.kind, segment, font, emoji_font, &mut owned); let (w, _) = f.measure_str(segment, None); let extra = if letter_spacing.abs() > 0.01 { letter_spacing * (segment.chars().count() as f32 - 1.0).max(0.0) @@ -672,3 +854,350 @@ mod tracking_tests { } } } + +// ─── Tests: #8 — emoji presentation must default to text, not tofu/color ── + +#[cfg(test)] +mod emoji_presentation_tests { + use super::super::{emoji_typeface, typeface_with_fallback}; + use super::*; + + // These assertions are pure code-point classification — no font, no + // machine dependency — so they hold on every host. + + #[test] + fn text_presentation_default_symbols_are_not_emoji_without_vs16() { + // #8's headline repro: ✓ (U+2713) and ✔ (U+2714) — both inside the + // old unconditional 0x2702..=0x27B0 dingbats range — must NOT be + // classified as emoji when they appear bare, since Unicode marks + // them text-presentation by default. + assert!( + !char_wants_emoji_font('\u{2713}', None), + "✓ bare must be text" + ); + assert!( + !char_wants_emoji_font('\u{2714}', None), + "✔ bare must be text" + ); + // Copyright/registered/trademark: also text-presentation by default. + assert!( + !char_wants_emoji_font('\u{00A9}', None), + "© bare must be text" + ); + assert!( + !char_wants_emoji_font('\u{00AE}', None), + "® bare must be text" + ); + assert!( + !char_wants_emoji_font('\u{2122}', None), + "™ bare must be text" + ); + } + + #[test] + fn text_presentation_default_symbols_opt_into_emoji_with_vs16() { + // Explicit author intent (U+FE0F immediately after) must still be + // honoured — this is what "presentation *by default*" means. + assert!(char_wants_emoji_font('\u{2713}', Some('\u{FE0F}'))); + assert!(char_wants_emoji_font('\u{00A9}', Some('\u{FE0F}'))); + } + + #[test] + fn genuine_pictographs_are_always_emoji_regardless_of_vs16() { + // Regression guard: the narrowing must not touch the actual emoji + // blocks (grinning face, etc.) — these have no meaningful text + // rendering at all. + assert!( + char_wants_emoji_font('\u{1F600}', None), + "😀 must stay emoji" + ); + assert!(char_wants_emoji_font('\u{1F600}', Some('\u{FE0F}'))); + } + + #[test] + fn variation_selector_16_itself_is_always_emoji() { + // So it merges into whatever run selected it, rather than becoming + // its own (invisible, harmless either way) primary-font run. + assert!(char_wants_emoji_font('\u{FE0F}', None)); + } + + #[test] + fn variation_selector_15_does_not_force_emoji() { + // VS-15 (U+FE0E) forces *text* presentation — must not be conflated + // with VS-16 the way the old catch-all 0xFE00..=0xFE0F range did. + assert!(!char_wants_emoji_font('\u{2713}', Some('\u{FE0E}'))); + } + + #[test] + fn has_emoji_reflects_the_narrowed_classification() { + assert!(!has_emoji("2713:\u{2713} copyright:\u{00A9}")); + assert!(has_emoji("checked \u{2713}\u{FE0F}")); + assert!(has_emoji("grinning \u{1F600}")); + } + + // ---- render-level reproduction: color and coverage, no emoji font needed ---- + + fn helvetica_font(size: f32) -> Font { + let typeface = typeface_with_fallback("Helvetica", skia_safe::FontStyle::normal()) + .expect("host must have a fallback typeface"); + Font::from_typeface(typeface, size) + } + + /// Renders `text` at `size` in white and returns `(ink_pixel_count, + /// mean_r, mean_g, mean_b)` over every non-transparent pixel. + fn render_and_sample(text: &str, size: f32) -> (usize, f64, f64, f64) { + use skia_safe::{surfaces, AlphaType, Color, ColorType, ImageInfo}; + const W: i32 = 200; + const H: i32 = 200; + let font = helvetica_font(size); + let emoji_font = emoji_typeface().map(|tf| Font::from_typeface(tf, size)); + let mut surface = surfaces::raster_n32_premul((W, H)).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; + draw_text_with_fallback( + canvas, + text, + &font, + &emoji_font, + 0.0, + 10.0, + ascent + 10.0, + &paint, + ); + + let info = ImageInfo::new((W, H), 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 n, mut sr, mut sg, mut sb) = (0usize, 0f64, 0f64, 0f64); + for px in buf.chunks_exact(4) { + if px[3] > 40 { + n += 1; + sr += px[0] as f64; + sg += px[1] as f64; + sb += px[2] as f64; + } + } + if n == 0 { + (0, 0.0, 0.0, 0.0) + } else { + (n, sr / n as f64, sg / n as f64, sb / n as f64) + } + } + + #[test] + fn bare_check_mark_paints_requested_white_not_tofu_or_color_bitmap() { + // #8 render-level reproduction, environment-independent: whether or + // not an emoji font is installed on this host is irrelevant here — + // with the fix, U+2713 alone is classified as *text*, so + // `draw_text_with_fallback` never even looks at `emoji_font` for + // it. Guard on the primary font actually covering the glyph so + // this doesn't fail on some exotic host where even Helvetica lacks + // it (the audit's own finding: Helvetica/Menlo DO have it, only + // Apple Color Emoji doesn't). + let font = helvetica_font(64.0); + if !font_covers(&font, "\u{2713}") { + eprintln!("skip: primary font doesn't cover U+2713 on this host"); + return; + } + let (ink, r, g, b) = render_and_sample("\u{2713}", 64.0); + assert!(ink > 20, "expected visible ink for ✓, got {ink} pixels"); + // White request -> painted channels should be bright and roughly + // neutral (not a dark/colored emoji-bitmap tint). + assert!( + r > 150.0 && g > 150.0 && b > 150.0, + "✓ should paint near-white, got mean rgb=({r:.0},{g:.0},{b:.0})" + ); + } +} + +// ─── Tests: #3 — glyph fallback for scripts the primary font doesn't cover ─ + +#[cfg(test)] +mod glyph_fallback_tests { + use super::super::{fallback_typeface_for_char, typeface_with_fallback}; + use super::*; + + fn helvetica_font(size: f32) -> Font { + let typeface = typeface_with_fallback("Helvetica", skia_safe::FontStyle::normal()) + .expect("host must have a fallback typeface"); + Font::from_typeface(typeface, size) + } + + #[test] + fn font_covers_detects_missing_glyphs_deterministically() { + // The detection mechanism itself, independent of whatever fallback + // font may or may not be installed: Helvetica (guaranteed present + // by `typeface_with_fallback`'s own contract) covers plain ASCII + // and does not cover CJK. + let font = helvetica_font(32.0); + assert!(font_covers(&font, "ABC"), "Helvetica must cover ASCII"); + assert!( + !font_covers(&font, "\u{4F60}\u{597D}"), // 你好 + "Helvetica must not cover CJK" + ); + } + + #[test] + fn classify_char_stays_primary_for_a_codepoint_no_font_covers() { + // A Private Use Area code point is uncovered by the primary font + // (nothing standard assigns glyphs there); it may or may not be + // "covered" by *some* installed font's own PUA convention (icon + // fonts, vendor glyphs) depending on the host, so skip gracefully + // rather than assume every machine agrees — the point of this test + // is that `classify_char` doesn't crash/loop when nothing covers a + // code point, falling back to `Primary` (the pre-fix, tofu- + // producing behaviour) rather than panicking. + let font = helvetica_font(32.0); + let pua = '\u{E000}'; + assert!( + !font_covers(&font, &pua.to_string()), + "test setup: PUA code point must be uncovered by the primary font" + ); + let style = font.typeface().font_style(); + let family = font.typeface().family_name(); + if fallback_typeface_for_char(&family, style, pua).is_some() { + eprintln!( + "skip: host has some font claiming to cover U+E000 (PUA) — can't exercise the \ + 'nothing covers it anywhere' branch on this host" + ); + return; + } + let kind = classify_char(pua, &font, None); + assert!( + matches!(kind, RunKind::Primary), + "an uncoverable-anywhere code point must resolve to Primary, not panic" + ); + } + + #[test] + fn classify_char_resolves_a_fallback_when_the_host_has_one() { + // Environment-dependent by nature (issue: CJK coverage depends on + // installed fonts) — skip gracefully, per this workstream's brief, + // rather than asserting a specific glyph renders. When a fallback + // *is* available (true on stock macOS/Windows/most Linux desktops + // via Noto/PingFang/MS-Gothic-class fonts), `classify_char` itself + // — not just the underlying `fallback_typeface_for_char` primitive + // — must actually route to it, and the resolved typeface must + // cover the character that triggered the lookup. + let font = helvetica_font(32.0); + let style = font.typeface().font_style(); + let family = font.typeface().family_name(); + if fallback_typeface_for_char(&family, style, '\u{4F60}').is_none() { + eprintln!("skip: no CJK-capable font installed on this host"); + return; + } + let kind = classify_char('\u{4F60}', &font, None); + let RunKind::Fallback(fallback) = kind else { + panic!( + "classify_char must resolve a Fallback run for an uncovered CJK code point when \ + the host has a capable font" + ); + }; + let fallback_font = Font::from_typeface(fallback, 32.0); + assert!( + font_covers(&fallback_font, "\u{4F60}"), + "resolved fallback typeface must actually cover the code point that triggered it" + ); + } + + #[test] + fn measure_and_paint_agree_on_cjk_width_when_fallback_is_available() { + // Measure-vs-paint parity (this workstream's core mandate): + // `measure_text_with_fallback` must report the width that actually + // gets painted. Skips gracefully (see above) when the host has no + // CJK-capable font at all — on such a host both measure and paint + // agree on the pre-fix degraded behaviour (0-width primary-font + // tofu), which is a separate, already-covered case. + let font = helvetica_font(48.0); + let text = "\u{4F60}\u{597D}"; // 你好 + if !text.chars().any(|c| { + fallback_typeface_for_char("Helvetica", font.typeface().font_style(), c).is_some() + }) { + eprintln!("skip: no CJK-capable font installed on this host"); + return; + } + + // Directly prove segmentation actually routes through the fallback + // mechanism (not just that `fallback_typeface_for_char` the + // primitive would resolve *something* if called) — this is what + // distinguishes "the fix is wired up" from "the fix exists but + // nothing calls it". + let runs = segment_text_runs(text, &font); + assert!( + runs.iter().any(|r| matches!(r.kind, RunKind::Fallback(_))), + "expected at least one Fallback run when segmenting CJK text with a fallback font \ + available, got kinds: {:?}", + runs.iter() + .map(|r| match &r.kind { + RunKind::Primary => "Primary", + RunKind::Emoji => "Emoji", + RunKind::Fallback(_) => "Fallback", + }) + .collect::>() + ); + + let measured_w = measure_text_with_fallback(text, &font, &None, 0.0); + // Not tofu-narrow: with a real CJK fallback, two full-width + // ideographs at 48px measure well beyond a couple of `.notdef` box + // glyphs' worth of width. + assert!( + measured_w > 30.0, + "expected a real CJK measurement, got suspiciously narrow {measured_w}" + ); + + use skia_safe::{surfaces, AlphaType, Color, ColorType, ImageInfo}; + const W: i32 = 400; + const H: i32 = 200; + let mut surface = surfaces::raster_n32_premul((W, H)).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; + draw_text_with_fallback(canvas, text, &font, &None, 0.0, 10.0, ascent + 10.0, &paint); + + // Ink detection: the canvas is cleared to opaque black (alpha=255 + // everywhere), so — unlike a transparent-cleared surface — alpha + // can't distinguish text from background here. White text against + // black shows up as a bright *red* (or green/blue) channel instead + // (same technique `tracking_tests::render_and_measure_ink_centre_x` + // above uses). + let info = ImageInfo::new((W, H), 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))); + } + } + } + let (min_x, max_x) = ( + min_x.expect("must paint something"), + max_x.expect("must paint something"), + ); + let painted_width = (max_x - min_x) as f32; + + // Loose tolerance: ink-bbox width vs advance width can differ from + // glyph side-bearings/overhang even for a well-behaved font — this + // is not the #125-style "wrap/paint disagreement" defect (a wild, + // multiple-hundred-px miscentre), just normal glyph-metric slack. + assert!( + (painted_width - measured_w).abs() < measured_w * 0.5 + 20.0, + "measured width {measured_w} should roughly match the painted ink width \ + {painted_width} (min_x={min_x}, max_x={max_x})" + ); + } +} diff --git a/crates/rustmotion-core/src/engine/text/cosmic.rs b/crates/rustmotion-core/src/engine/text/cosmic.rs index 2f52815..5af0656 100644 --- a/crates/rustmotion-core/src/engine/text/cosmic.rs +++ b/crates/rustmotion-core/src/engine/text/cosmic.rs @@ -9,6 +9,23 @@ //! Shaping & line-breaking are delegated to cosmic-text. Paint is done by //! rasterizing each glyph to an alpha mask, tinting it with the requested //! color, and blitting it as a small `Image` into Skia. +//! +//! **Not currently wired into the real render path (audit #10).** Every +//! component's actual measure/paint goes through `skia_safe::Font:: +//! measure_str` / `TextBlob::new` in `engine::renderer::text` + +//! `rustmotion-components::intrinsic::TextIntrinsic`, not through this +//! module — `measure_text`/`paint_text` below have no callers outside their +//! own tests (`grep -rn "engine::text\|text::cosmic" crates/` confirms +//! this). If you're chasing a text overflow/measure-vs-paint bug, look in +//! `engine::renderer::text.rs` and `rustmotion-components::intrinsic` +//! instead — the shaping/bidi/glyph-fallback behaviour cosmic-text would +//! provide here is not what actually renders today. Kept building (and the +//! `cosmic-text` dependency kept) as a candidate landing spot for a future +//! real shaping engine; not deleted unilaterally by this fix since that +//! call — wire it in for real vs. remove the module and its dependency — +//! is bigger than any single finding in this pass. See +//! `rustmotion-components::intrinsic` module doc for the other side of this +//! (it also used to claim a cosmic-text backing it doesn't have). use std::sync::{Mutex, OnceLock};