From ff36af49cbb0829f17ccd6a45a0342d0f5194318 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 1 Aug 2026 18:10:40 +0200 Subject: [PATCH 1/3] refactor(schema): remove the remaining LayerStyle-era shadow types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave 1 deleted LayerStyle itself but could not reach the types it dragged along, because they sat behind a re-export chain outside that wave's file partition. schema::GridTrack was the worst of them: a three-variant, externally-tagged shadow of the live css::style::GridTrack that actually drives grid layout. Two incompatible types sharing one name is what led the documentation to teach {"fr": 1}, a form the engine rejects. Its re-export chain through traits::container turned out to have no consumers at all. Spacing and GridPlacement, and five more orphans that were exclusively fields of the deleted LayerStyle — CardBorder, CardShadow, FilterConfig, DropShadow and TextGradient — go with it. Pure removal: 111 deletions, one re-export line changed, no logic touched. The exported schema keeps its 178 definitions, since none of these were ever reachable from Component or Scenario. Refs #111 --- crates/rustmotion-core/src/schema/style.rs | 42 ------------ crates/rustmotion-core/src/schema/video.rs | 68 ------------------- .../rustmotion-core/src/traits/container.rs | 1 - crates/rustmotion-core/src/traits/mod.rs | 2 +- 4 files changed, 1 insertion(+), 112 deletions(-) diff --git a/crates/rustmotion-core/src/schema/style.rs b/crates/rustmotion-core/src/schema/style.rs index da63738..5735cda 100644 --- a/crates/rustmotion-core/src/schema/style.rs +++ b/crates/rustmotion-core/src/schema/style.rs @@ -47,48 +47,6 @@ pub enum CardJustify { SpaceEvenly, } -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -#[serde(untagged)] -pub enum Spacing { - Uniform(f32), - Sides { - top: f32, - right: f32, - bottom: f32, - left: f32, - }, -} - -impl Spacing { - pub fn resolve(&self) -> (f32, f32, f32, f32) { - match self { - Spacing::Uniform(v) => (*v, *v, *v, *v), - Spacing::Sides { - top, - right, - bottom, - left, - } => (*top, *right, *bottom, *left), - } - } -} - -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -#[serde(rename_all = "snake_case")] -pub enum GridTrack { - Px(f32), - Fr(f32), - Auto, -} - -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -pub struct GridPlacement { - #[serde(default)] - pub start: Option, - #[serde(default)] - pub span: Option, -} - /// A single step in a component's animation timeline. /// Triggers a set of animations at a specific time within the scene. #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] diff --git a/crates/rustmotion-core/src/schema/video.rs b/crates/rustmotion-core/src/schema/video.rs index f1d1235..7e66c23 100644 --- a/crates/rustmotion-core/src/schema/video.rs +++ b/crates/rustmotion-core/src/schema/video.rs @@ -584,13 +584,6 @@ pub enum CaptionStyle { KaraokePop, } -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -pub struct CardBorder { - pub color: String, - #[serde(default = "default_card_border_width")] - pub width: f32, -} - #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] pub struct GradientBorder { pub colors: Vec, @@ -604,17 +597,6 @@ fn default_gradient_border_width() -> f32 { 2.0 } -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -pub struct CardShadow { - pub color: String, - #[serde(default)] - pub offset_x: f32, - #[serde(default)] - pub offset_y: f32, - #[serde(default)] - pub blur: f32, -} - /// Inner shadow configuration (inset shadow). #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] pub struct InnerShadow { @@ -654,44 +636,6 @@ pub struct TextBackground { // --- Visual Effect Types --- -/// CSS-like color filter configuration -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -pub struct FilterConfig { - #[serde(default)] - pub brightness: Option, - #[serde(default)] - pub contrast: Option, - #[serde(default)] - pub grayscale: Option, - #[serde(default)] - pub hue_rotate: Option, - #[serde(default)] - pub saturate: Option, - #[serde(default)] - pub sepia: Option, -} - -/// Universal drop shadow -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -pub struct DropShadow { - #[serde(default)] - pub dx: f32, - #[serde(default)] - pub dy: f32, - #[serde(default = "default_drop_shadow_blur")] - pub blur: f32, - #[serde(default = "default_drop_shadow_color")] - pub color: String, -} - -fn default_drop_shadow_blur() -> f32 { - 4.0 -} - -fn default_drop_shadow_color() -> String { - "#00000080".to_string() -} - /// Glow effect (colored luminous halo around the element) #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] pub struct GlowConfig { @@ -718,14 +662,6 @@ fn default_glow_intensity() -> f32 { 1.0 } -/// Gradient fill for text -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -pub struct TextGradient { - pub colors: Vec, - #[serde(default)] - pub angle: Option, -} - // --- Default functions --- fn default_font_size() -> f32 { @@ -767,7 +703,3 @@ fn default_shadow_blur() -> f32 { fn default_text_bg_padding() -> f32 { 8.0 } - -fn default_card_border_width() -> f32 { - 1.0 -} diff --git a/crates/rustmotion-core/src/traits/container.rs b/crates/rustmotion-core/src/traits/container.rs index 1922bf4..c8b68b8 100644 --- a/crates/rustmotion-core/src/traits/container.rs +++ b/crates/rustmotion-core/src/traits/container.rs @@ -1,4 +1,3 @@ pub use crate::schema::CardAlign as Align; pub use crate::schema::CardDirection as Direction; pub use crate::schema::CardJustify as Justify; -pub use crate::schema::GridTrack; diff --git a/crates/rustmotion-core/src/traits/mod.rs b/crates/rustmotion-core/src/traits/mod.rs index 8ae391c..fb13be7 100644 --- a/crates/rustmotion-core/src/traits/mod.rs +++ b/crates/rustmotion-core/src/traits/mod.rs @@ -23,7 +23,7 @@ pub use animatable::Animatable; pub use backgrounded::{Backgrounded, BackgroundedMut}; pub use bordered::{Border, Bordered, BorderedMut}; pub use clipped::Clipped; -pub use container::{Align, Direction, GridTrack, Justify}; +pub use container::{Align, Direction, Justify}; pub use painter::{AvailableSize, MeasureCtx, PaintCtx, Painter}; pub use rounded::{Rounded, RoundedMut}; pub use shadowed::{Shadow, Shadowed, ShadowedMut}; From e6ee78bb5bd4c208ee4029b595bad278cb31d59b Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 1 Aug 2026 18:10:40 +0200 Subject: [PATCH 2/3] fix(components): make white-space, rich_text and glow do what the schema promises MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three properties the schema accepted and the engine ignored. white-space was read in exactly two places repo-wide — cascade inheritance and the geometry validator — while the intrinsic hardcoded wrap: true and the painter wrapped unconditionally. So the validator's unwrappable_text_overflow fired on a state the renderer could not produce. Derive wrap from the property in both measurement and painting, for text, gradient_text and caption. gradient_text had no line-breaking path at all and gained one, with the gradient computed across the whole multi-line box so wrapping does not fragment the colour transition. Nowrap text also gets min-width: 0 so its box stays clamped while the line bleeds past it, which is what CSS does — without it, automatic minimum size lets the box grow and silently resize siblings. rich_text had no entry in component_intrinsic, so it measured 0x0 and rendered nothing unless the author guessed a width — for the component whose whole purpose is accent-coloured words inside a sentence. It now measures itself, through the same word-level line-breaker the painter uses, so measure and paint cannot disagree. Line breaking was previously only possible at span boundaries; a long single span now wraps internally. glow was stored by extract_effects and never read by resolve_props_for_effects: renders with and without it were byte-identical. It now builds a correctly coloured drop shadow. Note the separate keyframe path (glow_radius / glow_intensity as raw animated properties) still produces a black halo because GlowConfig.color is not plumbed there; that lives in css/animation.rs and is left for a follow-up. Refs #109 --- .../rustmotion-components/src/box_builder.rs | 281 ++++++++++ crates/rustmotion-components/src/caption.rs | 146 +++++- .../src/gradient_text.rs | 187 ++++++- crates/rustmotion-components/src/intrinsic.rs | 393 +++++++++++++- crates/rustmotion-components/src/rich_text.rs | 487 +++++++++++++----- crates/rustmotion-components/src/text.rs | 155 +++++- crates/rustmotion-core/src/engine/animator.rs | 76 +++ 7 files changed, 1553 insertions(+), 172 deletions(-) diff --git a/crates/rustmotion-components/src/box_builder.rs b/crates/rustmotion-components/src/box_builder.rs index 69d267f..6c38ab9 100644 --- a/crates/rustmotion-components/src/box_builder.rs +++ b/crates/rustmotion-components/src/box_builder.rs @@ -291,6 +291,7 @@ fn build_ghosts<'a>( if props_has_paint_overrides(&props) { apply_animated_props(&mut css, &props); } + apply_glow_effect(&mut css, &ghost_effects); } // Scale opacity: multiply the base opacity by the ghost opacity factor. let base_opacity = css.opacity.unwrap_or(1.0); @@ -468,6 +469,7 @@ fn build_child<'a>( if props_has_paint_overrides(&props) { apply_animated_props(&mut css, &props); } + apply_glow_effect(&mut css, &effects); } } @@ -797,6 +799,55 @@ fn props_has_paint_overrides(p: &AnimatedProperties) -> bool { || p.perspective > 0.0 } +/// M3: apply the static `glow` animation effect (a coloured halo — not +/// time-varying, see `AnimationEffect::shift_delay`'s doc comment) as a CSS +/// `filter: drop-shadow(...)`. +/// +/// This is deliberately *not* routed through `resolve_props_for_effects` / +/// `AnimatedProperties::glow_radius`+`glow_intensity` even though those +/// fields exist: `AnimatedProperties`'s public shape is frozen for this +/// workstream (can't add a `glow_color` field), and the existing bridge that +/// *does* consume those two fields — `css::animation::apply_animated_props`, +/// in a file outside this workstream's scope — hardcodes `color: None` on +/// the `DropShadow` it builds, which resolves to black. Piping the `glow` +/// effect through that path would either still render black, or — if we did +/// find a way to set the color fields too — double up into two stacked +/// drop-shadows (one colourless from the frozen bridge, one coloured from +/// here). Building the filter directly here, from the raw `GlowConfig` +/// (found via `animator::find_glow_effect`), keeps it a single, correctly +/// coloured shadow and needs no change to any frozen file. +/// +/// The pre-existing `glow_radius`/`glow_intensity` numeric-property path +/// (animating those as arbitrary `keyframes` targets, unrelated to this +/// named `glow` effect) is untouched and still produces a colourless halo — +/// that is `css::animation.rs`'s DropShadow-with-`color:None` bug, out of +/// this workstream's file ownership. +fn apply_glow_effect(css: &mut CssStyle, effects: &[rustmotion_core::schema::AnimationEffect]) { + use rustmotion_core::css::style::{Color, FilterFn}; + use rustmotion_core::css::units::Length; + use rustmotion_core::engine::animator::find_glow_effect; + + let Some(glow) = find_glow_effect(effects) else { + return; + }; + + let (r, g, b, a) = rustmotion_core::engine::renderer::parse_css_color(&glow.color) + .unwrap_or((255, 255, 255, 255)); + let alpha = ((a as f32 / 255.0) * glow.intensity.max(0.0)).clamp(0.0, 1.0); + let radius = glow.radius.max(0.0); + if radius <= 0.0 || alpha <= 0.0 { + return; + } + + let shadow = FilterFn::DropShadow { + offset_x: Length::Px(0.0), + offset_y: Length::Px(0.0), + blur: Some(Length::Px(radius)), + color: Some(Color::Rgba { r, g, b, a: alpha }), + }; + css.filter.get_or_insert_with(Vec::new).push(shadow); +} + /// Build an [`IntrinsicMeasure`] for components whose box size depends on /// their content (text, codeblock, terminal, etc.). Returns `None` for /// components with explicit dimensions or pure containers. @@ -824,6 +875,12 @@ fn component_intrinsic( Codeblock(c) => Some(Arc::new( crate::intrinsic::CodeblockIntrinsic::from_codeblock(c), )), + // M2: rich_text had no intrinsic measurer at all, so it laid out + // 0×0 and rendered nothing unless the author guessed an explicit + // width/height. + RichText(rt) => Some(Arc::new( + crate::intrinsic::RichTextIntrinsic::from_rich_text(rt), + )), _ => None, } } @@ -945,6 +1002,62 @@ fn apply_default_display(component: &Component, css: &mut CssStyle) { fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { use Component::*; match component { + Text(t) => { + // M1: `white-space: nowrap|pre` must style the *content*, not + // silently resize the *box*. Without this, CSS's "automatic + // minimum size" (min-width: auto + the default overflow: + // visible) lets a nowrap text's own auto-width box grow to its + // full natural width inside a flex container — which can push + // or resize flex siblings the author never touched, a + // surprising failure mode for a tool whose whole model is + // authors declaring explicit positions/sizes. Forcing + // `min-width: 0` (only when the author hasn't set their own) + // keeps the box within whatever space its container gives it; + // the painter (`text.rs`) still draws the full unwrapped line + // regardless of that box width, so the text visibly bleeds past + // it exactly as `white-space: nowrap` should — it's the box + // that stays put, not the content. + let nowrap = matches!( + t.style.white_space, + Some( + rustmotion_core::css::style::WhiteSpace::Nowrap + | rustmotion_core::css::style::WhiteSpace::Pre + ) + ); + if nowrap && css.min_width.is_none() { + css.min_width = Some(CSize::Length(CLP::Px(0.0))); + } + } + // M1 follow-up: same reasoning as the `Text` arm above — now that + // `gradient_text.rs` and `caption.rs` word-wrap and respect + // `white-space: nowrap|pre` too (see `intrinsic.rs`'s + // `GradientTextIntrinsic`/`CaptionIntrinsic`), their auto-width + // boxes can hit the same CSS automatic-minimum-size growth when + // nowrap/pre is set with no explicit width. + GradientText(t) => { + let nowrap = matches!( + t.style.white_space, + Some( + rustmotion_core::css::style::WhiteSpace::Nowrap + | rustmotion_core::css::style::WhiteSpace::Pre + ) + ); + if nowrap && css.min_width.is_none() { + css.min_width = Some(CSize::Length(CLP::Px(0.0))); + } + } + Caption(c) => { + let nowrap = matches!( + c.style.white_space, + Some( + rustmotion_core::css::style::WhiteSpace::Nowrap + | rustmotion_core::css::style::WhiteSpace::Pre + ) + ); + if nowrap && css.min_width.is_none() { + css.min_width = Some(CSize::Length(CLP::Px(0.0))); + } + } Divider(d) => match d.direction { DividerDirection::Horizontal => { // Stretch horizontally in flex row/column parents (cross-axis @@ -1759,4 +1872,172 @@ mod tests { assert_eq!(l.width, 100.0); assert_eq!(l.height, 60.0); } + + // ─── M2: rich_text intrinsic wired into component_intrinsic ───────────── + + #[test] + fn rich_text_child_gets_a_non_zero_intrinsic_size() { + // Before the fix: rich_text had no `component_intrinsic` entry, so + // an auto-sized rich_text laid out at 0×0 and was invisible. + use crate::rich_text::{RichText, RichTextSpan}; + use rustmotion_core::css::units::Length; + + let rich_text = ChildComponent { + component: Component::RichText(RichText { + spans: vec![ + RichTextSpan { + text: "Save ".into(), + color: None, + font_size: None, + font_weight: None, + font_family: None, + font_style: None, + letter_spacing: None, + }, + RichTextSpan { + text: "40%".into(), + color: Some("#5C39EE".into()), + font_size: None, + font_weight: None, + font_family: None, + font_style: None, + letter_spacing: None, + }, + ], + max_width: None, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::Px(40.0)), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }), + position: Some(crate::PositionMode::Absolute { x: 0.0, y: 0.0 }), + x: None, + y: None, + z_index: None, + }; + let scene = vec![rich_text]; + let built = build_scene(&scene, (800.0, 600.0)); + let layout = run_layout(&built.root, (800.0, 600.0), &ConversionContext::default()); + let id = built.root.children[0].id; + let l = layout.get(id).expect("rich_text laid out"); + assert!( + l.width > 0.0, + "rich_text width should be > 0, got {}", + l.width + ); + assert!( + l.height > 0.0, + "rich_text height should be > 0, got {}", + l.height + ); + } + + // ─── M3: the `glow` effect renders as a coloured drop-shadow ──────────── + + #[test] + fn glow_effect_adds_a_coloured_drop_shadow_filter() { + use rustmotion_core::css::style::{Color, FilterFn}; + use rustmotion_core::css::units::Length as CLength; + use rustmotion_core::schema::{AnimationEffect, GlowConfig}; + + let mut shape = make_shape(100.0, 100.0); + if let Component::Shape(ref mut s) = shape.component { + s.style.animation = vec![AnimationEffect::Glow(GlowConfig { + color: "#5C39EE".to_string(), + radius: 12.0, + intensity: 1.0, + })]; + } + let anim = BuildAnimationCtx { + time: 0.0, + scene_duration: 1.0, + fps: 30, + }; + let scene = [shape]; + let built = build_scene_with_anim(&scene, (400.0, 400.0), anim); + let filters = built.root.children[0] + .css + .filter + .as_ref() + .expect("glow must add a filter list"); + let shadow = filters + .iter() + .find(|f| matches!(f, FilterFn::DropShadow { .. })) + .expect("a DropShadow filter must be present"); + let FilterFn::DropShadow { blur, color, .. } = shadow else { + unreachable!() + }; + match blur { + Some(CLength::Px(px)) => { + assert_eq!(*px, 12.0, "blur radius must match GlowConfig.radius") + } + other => panic!("expected Some(Length::Px(12.0)) blur, got {:?}", other), + } + // The defect this fixes: `color: None` resolves to black downstream + // (see `paint_pass.rs`'s `unwrap_or(SColor::BLACK)`). The colour must + // be `Some` and must match the configured glow colour, not black. + match color { + Some(Color::Rgba { r, g, b, a }) => { + assert_eq!(*r, 0x5C); + assert_eq!(*g, 0x39); + assert_eq!(*b, 0xEE); + assert!(*a > 0.0, "alpha must be > 0 for the glow to be visible"); + } + other => panic!("expected Some(Color::Rgba{{..}}), got {:?}", other), + } + } + + #[test] + fn glow_effect_scales_alpha_by_intensity() { + use rustmotion_core::css::style::{Color, FilterFn}; + use rustmotion_core::schema::{AnimationEffect, GlowConfig}; + + let mut shape = make_shape(100.0, 100.0); + if let Component::Shape(ref mut s) = shape.component { + s.style.animation = vec![AnimationEffect::Glow(GlowConfig { + color: "#FFFFFFFF".to_string(), // opaque white + radius: 10.0, + intensity: 0.5, + })]; + } + let anim = BuildAnimationCtx { + time: 0.0, + scene_duration: 1.0, + fps: 30, + }; + let scene = [shape]; + let built = build_scene_with_anim(&scene, (400.0, 400.0), anim); + let filters = built.root.children[0].css.filter.as_ref().unwrap(); + let FilterFn::DropShadow { color, .. } = filters + .iter() + .find(|f| matches!(f, FilterFn::DropShadow { .. })) + .unwrap() + else { + unreachable!() + }; + let Some(Color::Rgba { a, .. }) = color else { + panic!("expected Rgba color"); + }; + assert!( + (*a - 0.5).abs() < 1e-4, + "intensity 0.5 on an opaque colour must scale alpha to ~0.5, got {}", + a + ); + } + + #[test] + fn no_glow_effect_leaves_filter_untouched() { + let shape = make_shape(100.0, 100.0); + let anim = BuildAnimationCtx { + time: 0.0, + scene_duration: 1.0, + fps: 30, + }; + let scene = [shape]; + let built = build_scene_with_anim(&scene, (400.0, 400.0), anim); + assert!(built.root.children[0].css.filter.is_none()); + } } diff --git a/crates/rustmotion-components/src/caption.rs b/crates/rustmotion-components/src/caption.rs index 4900095..f979c49 100644 --- a/crates/rustmotion-components/src/caption.rs +++ b/crates/rustmotion-components/src/caption.rs @@ -2,6 +2,7 @@ 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::CssStyle; use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; @@ -140,7 +141,21 @@ impl Caption { } } CaptionStyle::Highlight | CaptionStyle::Karaoke | CaptionStyle::KaraokePop => { - let max_width = self.max_width.unwrap_or(f32::MAX); + // M1: `white-space: nowrap|pre` keeps every word on one + // line — ignore `max_width` entirely so the line can bleed + // past it, same rule `text.rs` uses. (`WordByWord`/`WordPop` + // above show a single word at a time; wrapping is moot + // there, same as the existing kbd/counter/badge "atomic" + // components, so they don't need this branch.) + let nowrap = matches!( + self.style.white_space, + Some(CssWhiteSpace::Nowrap | CssWhiteSpace::Pre) + ); + let max_width = if nowrap { + f32::MAX + } else { + self.max_width.unwrap_or(f32::MAX) + }; let space_width = measure_text_with_fallback(" ", &font, &emoji_font, 0.0); let mut lines: Vec> = vec![vec![]]; @@ -278,3 +293,132 @@ fn ease_out_back(t: f32) -> f32 { let p = t - 1.0; 1.0 + C3 * p * p * p + C1 * p * p } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::style::CssStyle; + use rustmotion_core::css::Length; + use rustmotion_core::schema::CaptionWord; + + fn make_caption(text: &str, white_space: Option) -> Caption { + let words = text + .split_whitespace() + .map(|w| CaptionWord { + text: w.to_string(), + start: 0.0, + end: 1000.0, + }) + .collect(); + Caption { + words, + active_color: default_active_color(), + mode: CaptionStyle::Highlight, + max_width: Some(80.0), + pill_color: None, + style: CssStyle { + font_size: Some(Length::Px(28.0)), + white_space, + ..Default::default() + }, + timing: Default::default(), + timeline: Vec::new(), + stagger: None, + } + } + + /// Bounding box (min_x, max_x, min_y, max_y) of every non-transparent + /// pixel on the surface, or `None` if nothing was painted. + fn ink_bounds( + surface: &mut skia_safe::Surface, + w: i32, + h: i32, + ) -> Option<(i32, i32, i32, i32)> { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (w, h), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (w * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + let (mut minx, mut maxx, mut miny, mut maxy) = (i32::MAX, i32::MIN, i32::MAX, i32::MIN); + for y in 0..h { + for x in 0..w { + if buf[((y * w + x) * 4 + 3) as usize] > 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)) + } + + #[test] + fn nowrap_paints_one_wide_line_instead_of_wrapping_at_max_width() { + // M1 render-level proof, caption (Highlight mode): `white-space: + // nowrap` keeps every word on one line — much wider than + // `max_width: 80`, and only one line tall — instead of wrapping. + let caption = make_caption( + "the quick brown fox jumps over the lazy dog", + Some(CssWhiteSpace::Nowrap), + ); + 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, 250.0)); + caption.paint(canvas, 80.0, 0.5); + } + let (minx, maxx, miny, maxy) = + ink_bounds(&mut surface, W, H).expect("nowrap caption must paint something"); + + assert!( + maxx - minx > 240, + "nowrap caption must bleed far past its 80px max_width, got ink width {}", + maxx - minx + ); + assert!( + maxy - miny < 50, + "nowrap caption must stay on one line, got ink height {}", + maxy - miny + ); + } + + #[test] + fn normal_white_space_wraps_at_max_width() { + let caption = make_caption("the quick brown fox jumps over the lazy dog", 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, 250.0)); + caption.paint(canvas, 80.0, 0.5); + } + let (minx, maxx, miny, maxy) = + ink_bounds(&mut surface, W, H).expect("wrapped caption must paint something"); + + assert!( + maxx - minx < 200, + "wrapped caption must pack close to its 80px max_width, got ink width {}", + maxx - minx + ); + assert!( + maxy - miny > 50, + "wrapped caption must spread across multiple lines, got ink height {}", + maxy - miny + ); + } +} diff --git a/crates/rustmotion-components/src/gradient_text.rs b/crates/rustmotion-components/src/gradient_text.rs index 9b4bad8..f432e4a 100644 --- a/crates/rustmotion-components/src/gradient_text.rs +++ b/crates/rustmotion-components/src/gradient_text.rs @@ -1,15 +1,17 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use skia_safe::{Canvas, Font, FontStyle, Point, TextBlob}; +use skia_safe::{Canvas, Font, FontStyle, Point}; use rustmotion_core::css::style::{ FontStyle as CssFontStyle, FontWeight as CssFontWeight, FontWeightKw, + WhiteSpace as CssWhiteSpace, }; use rustmotion_core::css::CssStyle; use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::engine::renderer::{ - emoji_typeface, paint_from_hex, parse_hex_color, typeface_with_fallback, + draw_text_with_fallback, emoji_typeface, measure_text_with_fallback, paint_from_hex, + parse_hex_color, typeface_with_fallback, wrap_text_with_fallback, }; use rustmotion_core::schema::TimelineStep; use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig}; @@ -81,20 +83,45 @@ impl GradientText { } impl GradientText { - fn paint(&self, canvas: &Canvas, time: f64) { + fn paint(&self, canvas: &Canvas, layout_width: f32, time: f64) { if self.content.is_empty() || self.colors.is_empty() { return; } - let Some((font, _emoji_font)) = self.resolve_font() else { + let Some((font, emoji_font)) = self.resolve_font() else { return; }; - let _font_size = self.style.font_size_px_or(48.0); + let font_size = self.style.font_size_px_or(48.0); + let line_height_val = self.style.line_height_for(font_size); + + // M1: `white-space: nowrap|pre` keeps the whole content on one line + // even past `layout_width` (it bleeds); anything else word-wraps at + // the box width — same rule `text.rs` uses. + let nowrap = matches!( + self.style.white_space, + Some(CssWhiteSpace::Nowrap | CssWhiteSpace::Pre) + ); + let wrap_at = if nowrap { + None + } else if layout_width.is_finite() && layout_width > 0.0 { + Some(layout_width) + } else { + None + }; + let lines = wrap_text_with_fallback(&self.content, &font, &emoji_font, wrap_at); - // Measure text + // Measure the overall (possibly multi-line) bounding box. The + // gradient is defined once across this whole block rather than + // per-line, so wrapping reflows the text without fragmenting the + // colour transition. let (_, metrics) = font.metrics(); - let text_w = font.measure_str(&self.content, None).0; - let text_h = -metrics.ascent + metrics.descent; + let ascent = -metrics.ascent; + let descent = metrics.descent; + let text_w = lines + .iter() + .map(|l| measure_text_with_fallback(l, &font, &emoji_font, 0.0)) + .fold(0.0f32, f32::max); + let text_h = (lines.len().max(1) - 1) as f32 * line_height_val + ascent + descent; // Compute angle (possibly animated) let angle = if self.animate_angle { @@ -103,7 +130,7 @@ impl GradientText { self.angle }; - // Compute gradient endpoints from angle + // Compute gradient endpoints from angle, spanning the full block. let angle_rad = angle * std::f32::consts::PI / 180.0; let cx = text_w / 2.0; let cy = text_h / 2.0; @@ -138,26 +165,27 @@ impl GradientText { None, ); - if let Some(shader) = shader { - // Create paint with gradient shader - let mut gradient_paint = skia_safe::Paint::default(); - gradient_paint.set_anti_alias(true); - gradient_paint.set_shader(shader); - - // Create text blob and draw - if let Some(blob) = TextBlob::new(&self.content, &font) { - let y = -metrics.ascent; - canvas.draw_text_blob(&blob, Point::new(0.0, y), &gradient_paint); + let fill_paint = match shader { + Some(shader) => { + let mut p = skia_safe::Paint::default(); + p.set_anti_alias(true); + p.set_shader(shader); + p } - } else { - // Fallback: draw with first color - let mut paint = paint_from_hex(&self.colors[0]); - paint.set_anti_alias(true); + None => { + // Fallback: draw with the first color, no gradient. + let mut p = paint_from_hex(&self.colors[0]); + p.set_anti_alias(true); + p + } + }; - if let Some(blob) = TextBlob::new(&self.content, &font) { - let y = -metrics.ascent; - canvas.draw_text_blob(&blob, Point::new(0.0, y), &paint); + for (i, line) in lines.iter().enumerate() { + if line.is_empty() { + 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); } } } @@ -166,10 +194,113 @@ impl Painter for GradientText { fn paint_content( &self, canvas: &Canvas, - _layout: &BoxLayout, + layout: &BoxLayout, _props: &AnimatedProperties, ctx: &PaintCtx, ) { - self.paint(canvas, ctx.time); + self.paint(canvas, layout.width, ctx.time); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::style::CssStyle; + use rustmotion_core::css::Length; + + fn make_gradient_text(content: &str, white_space: Option) -> GradientText { + GradientText { + content: content.into(), + colors: default_colors(), + angle: default_angle(), + animate_angle: false, + speed: default_speed(), + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::Px(28.0)), + white_space, + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + } + } + + fn alpha_grid(surface: &mut skia_safe::Surface, width: i32, height: i32) -> Vec { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (width, height), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (width * height * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (width * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + (0..(width * height) as usize) + .map(|i| buf[i * 4 + 3]) + .collect() + } + + 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 { + if grid[(y * surface_width + x) as usize] > 0 { + return true; + } + } + } + false + } + + #[test] + fn nowrap_paints_a_single_line_past_the_layout_width() { + // M1 render-level proof, gradient_text: `white-space: nowrap` stays + // on one line and bleeds past `layout_width`. + let gt = make_gradient_text( + "the quick brown fox jumps over the lazy dog", + Some(CssWhiteSpace::Nowrap), + ); + const W: i32 = 600; + 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); + let grid = alpha_grid(&mut surface, W, H); + + assert!( + has_ink_in(&grid, W, 300, W, 0, 45), + "nowrap gradient_text must paint past its 80px box on line 1" + ); + assert!( + !has_ink_in(&grid, W, 0, W, 55, H), + "nowrap gradient_text must stay on a single line" + ); + } + + #[test] + fn normal_white_space_wraps_within_the_layout_width() { + let gt = make_gradient_text("the quick brown fox jumps over the lazy dog", None); + const W: i32 = 600; + 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); + let grid = alpha_grid(&mut surface, W, H); + + assert!( + !has_ink_in(&grid, W, 300, W, 0, 45), + "wrapped gradient_text must not reach x∈[300,600) on line 1 within an 80px box" + ); + assert!( + has_ink_in(&grid, W, 0, W, 55, H), + "wrapped gradient_text must spill onto a second line within the box width" + ); } } diff --git a/crates/rustmotion-components/src/intrinsic.rs b/crates/rustmotion-components/src/intrinsic.rs index 67f21b4..27ff4ba 100644 --- a/crates/rustmotion-components/src/intrinsic.rs +++ b/crates/rustmotion-components/src/intrinsic.rs @@ -9,6 +9,7 @@ use skia_safe::{Font, FontStyle as SkFontStyle}; use rustmotion_core::css::style::{ CssStyle, FontStyle as CssFontStyle, FontWeight as CssFontWeight, FontWeightKw, LineHeight, + WhiteSpace, }; use rustmotion_core::engine::box_tree::{AvailableSpace, IntrinsicMeasure}; use rustmotion_core::engine::renderer::{ @@ -37,10 +38,25 @@ pub struct TextIntrinsic { } impl TextIntrinsic { + /// M1: `white-space: nowrap|pre` disables wrapping — the geometry + /// validator's `unwrappable_text_overflow`/`ContentOverflowsBox` checks + /// (crates/rustmotion-cli/src/commands/geometry.rs) already branch on + /// exactly this pair of variants and re-measure via this same + /// `TextIntrinsic`, so the wrap decision here must match theirs exactly + /// or the validator's assumption about what the renderer produces is + /// false. pub fn from_text(text: &Text) -> Self { - Self::from_parts(&text.content, &text.style, text.max_width) + let wrap = !matches!( + text.style.white_space, + Some(WhiteSpace::Nowrap | WhiteSpace::Pre) + ); + Self::from_parts_with_wrap(&text.content, &text.style, text.max_width, wrap) } + /// Generic constructor shared by [`GradientText`]/[`Caption`] intrinsics, + /// whose painters don't (yet) implement `white-space: nowrap` — kept + /// 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 { let font_size = style.font_size_px_or(48.0); let line_height_resolved = style.line_height_for(font_size); @@ -53,8 +69,6 @@ impl TextIntrinsic { italic: matches!(style.font_style, Some(CssFontStyle::Italic)), letter_spacing: style.letter_spacing_px(), max_width, - // CssStyle has no `wrap` field — use white-space/overflow-wrap heuristics. - // For now, default to true (CSS default). wrap: true, } } @@ -152,7 +166,16 @@ impl GradientTextIntrinsic { Some(CSize::Length(LengthPercentage::Px(v))) => Some(*v), _ => None, }; - Self(TextIntrinsic::from_parts(&t.content, &t.style, max_width)) + // M1 follow-up (issue #109 review): gradient_text now word-wraps + // like `text` (see `gradient_text.rs::paint`) — mirror the same + // white-space: nowrap|pre rule here so measure and paint agree. + let wrap = !matches!( + t.style.white_space, + Some(WhiteSpace::Nowrap | WhiteSpace::Pre) + ); + Self(TextIntrinsic::from_parts_with_wrap( + &t.content, &t.style, max_width, wrap, + )) } } @@ -178,7 +201,22 @@ impl CaptionIntrinsic { .map(|w| w.text.as_str()) .collect::>() .join(" "); - Self(TextIntrinsic::from_parts(&joined, &c.style, c.max_width)) + // M1 follow-up: `Highlight`/`Karaoke`/`KaraokePop` word-wrap all + // words (see `caption.rs::paint`); `white-space: nowrap|pre` now + // forces them onto one line there too — mirror it here. (`WordByWord` + // /`WordPop` show one word at a time; wrapping is moot for those, + // same as the existing kbd/counter/badge "atomic, never wraps" + // components, so this doesn't need a mode-specific branch.) + let wrap = !matches!( + c.style.white_space, + Some(WhiteSpace::Nowrap | WhiteSpace::Pre) + ); + Self(TextIntrinsic::from_parts_with_wrap( + &joined, + &c.style, + c.max_width, + wrap, + )) } } @@ -593,6 +631,67 @@ impl IntrinsicMeasure for CodeblockIntrinsic { } } +// ───────────────────────────────────────────────────────────────────────────── +// RichText intrinsic measurer +// ───────────────────────────────────────────────────────────────────────────── + +use crate::rich_text::{RichText, RichTextSpan}; + +/// Intrinsic measurer for [`RichText`]. +/// +/// M2: previously absent from `component_intrinsic` entirely, so a +/// `rich_text` with no explicit `width`/`height` laid out 0×0 and rendered +/// nothing. Reuses `RichText::compute_layout` — the exact same word-wrapped +/// line-breaking algorithm the painter uses — so the box taffy reserves +/// always matches what gets painted (same measure/paint-parity rationale as +/// [`TextIntrinsic`]). +/// +/// Always measures the full (untruncated) content — a `visible_chars` +/// typewriter animation must not reflow layout as it plays. +pub struct RichTextIntrinsic { + spans: Vec, + style: CssStyle, + max_width: Option, +} + +impl RichTextIntrinsic { + pub fn from_rich_text(rt: &RichText) -> Self { + Self { + spans: rt.spans.clone(), + style: rt.style.clone(), + max_width: rt.max_width, + } + } +} + +impl IntrinsicMeasure for RichTextIntrinsic { + fn measure( + &self, + known: (Option, Option), + available: (AvailableSpace, AvailableSpace), + ) -> (f32, f32) { + let max_width = if let Some(w) = known.0 { + Some(w) + } else { + let avail_w = match available.0 { + AvailableSpace::Definite(w) => Some(w), + AvailableSpace::MaxContent => None, + AvailableSpace::MinContent => Some(0.0), + }; + match (self.max_width, avail_w) { + (Some(a), Some(b)) => Some(a.min(b)), + (Some(a), None) => Some(a), + (None, Some(b)) => Some(b), + (None, None) => None, + } + }; + + let layout = RichText::compute_layout(&self.spans, &self.style, max_width, -1.0); + let line_count = layout.lines.len().max(1) as f32; + (layout.max_width, line_count * layout.line_height) + } +} + #[cfg(test)] mod tests { use super::*; @@ -686,4 +785,288 @@ mod tests { assert_eq!(w, 0.0); assert!(h > 0.0); } + + // ─── M1: white-space: nowrap/pre ──────────────────────────────────────── + + fn nowrap_text(content: &str, white_space: Option) -> Text { + Text { + content: content.into(), + max_width: None, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::Px(20.0)), + white_space, + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + text_shadow: None, + stroke: None, + text_background: None, + } + } + + #[test] + fn nowrap_ignores_a_constrained_width_and_stays_one_line() { + let text = nowrap_text( + "the quick brown fox jumps over the lazy dog", + Some(WhiteSpace::Nowrap), + ); + let m = TextIntrinsic::from_text(&text); + let (w_unconstrained, h_unconstrained) = m.measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ); + let (w_constrained, h_constrained) = m.measure( + (None, None), + (AvailableSpace::Definite(80.0), AvailableSpace::MaxContent), + ); + assert!( + w_constrained > 80.0, + "nowrap must ignore the 80px constraint, got width {}", + w_constrained + ); + assert_eq!( + w_constrained, w_unconstrained, + "nowrap width must equal the natural (unconstrained) width regardless of available space" + ); + assert_eq!( + h_constrained, h_unconstrained, + "nowrap must always report a single line's height, constrained or not" + ); + } + + #[test] + fn pre_disables_wrap_exactly_like_nowrap() { + let text = nowrap_text("this string is too long to fit", Some(WhiteSpace::Pre)); + let m = TextIntrinsic::from_text(&text); + let (w, _h) = m.measure( + (None, None), + (AvailableSpace::Definite(80.0), AvailableSpace::MaxContent), + ); + assert!( + w > 80.0, + "white-space: pre must also ignore the width constraint, got {}", + w + ); + } + + #[test] + fn normal_white_space_still_wraps_at_a_constrained_width() { + // Regression guard: making `nowrap`/`pre` real must not touch the + // default (`normal`/unset) wrapping path. + let wrapped = nowrap_text( + "the quick brown fox jumps over the lazy dog", + Some(WhiteSpace::Normal), + ); + let unset = nowrap_text("the quick brown fox jumps over the lazy dog", None); + for text in [wrapped, unset] { + let m = TextIntrinsic::from_text(&text); + let (w, _h) = m.measure( + (None, None), + (AvailableSpace::Definite(80.0), AvailableSpace::MaxContent), + ); + assert!( + w <= 80.0 + 0.5, + "white-space: normal (or unset) must still wrap at an 80px constraint, got {}", + w + ); + } + } + + // ─── M2: rich_text intrinsic ───────────────────────────────────────────── + + fn span(text: &str) -> RichTextSpan { + RichTextSpan { + text: text.into(), + color: None, + font_size: None, + font_weight: None, + font_family: None, + font_style: None, + letter_spacing: None, + } + } + + #[test] + fn rich_text_intrinsic_is_non_zero_without_explicit_size() { + // M2's core defect: rich_text had no `component_intrinsic` entry at + // all, so it measured 0×0 unless the author guessed a width/height. + let spans = vec![span("Hello "), span("world")]; + let style = CssStyle { + font_size: Some(Length::Px(32.0)), + ..Default::default() + }; + let intrinsic = RichTextIntrinsic { + spans, + style, + max_width: None, + }; + let (w, h) = intrinsic.measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ); + assert!(w > 0.0, "rich_text natural width must be > 0, got {}", w); + assert!(h > 0.0, "rich_text natural height must be > 0, got {}", h); + } + + #[test] + fn rich_text_intrinsic_wraps_a_single_long_span_internally() { + // M2's second ask: a long single span must wrap like any other text, + // not just break at span boundaries (there is only one span here). + let spans = vec![span( + "the quick brown fox jumps over the lazy dog and keeps going", + )]; + let style = CssStyle { + font_size: Some(Length::Px(24.0)), + ..Default::default() + }; + let intrinsic = RichTextIntrinsic { + spans, + style, + max_width: None, + }; + let (_w_unconstrained, h_unconstrained) = intrinsic.measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ); + let (w_constrained, h_constrained) = intrinsic.measure( + (None, None), + (AvailableSpace::Definite(150.0), AvailableSpace::MaxContent), + ); + assert!( + w_constrained <= 150.0 + 0.5, + "wrapped width must fit the 150px constraint, got {}", + w_constrained + ); + assert!( + h_constrained > h_unconstrained, + "constraining width must add lines (wrap within the single span): {} vs {}", + h_constrained, + h_unconstrained + ); + } + + #[test] + fn rich_text_intrinsic_matches_compute_layout_used_by_the_painter() { + // Measure/paint parity: the intrinsic must reuse the exact same + // layout algorithm the painter does, or taffy could reserve a box + // that doesn't match what gets drawn. + let spans = vec![span("Total: "), span("42"), span(" items")]; + let style = CssStyle::default(); + let intrinsic = RichTextIntrinsic { + spans: spans.clone(), + style: style.clone(), + max_width: None, + }; + let (w, h) = intrinsic.measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ); + let layout = RichText::compute_layout(&spans, &style, None, -1.0); + assert_eq!(w, layout.max_width); + assert_eq!(h, layout.lines.len().max(1) as f32 * layout.line_height); + } + + // ─── M1 follow-up: gradient_text / caption honor white-space too ─────── + + #[test] + fn gradient_text_intrinsic_ignores_constrained_width_when_nowrap() { + let gt = GradientText { + content: "the quick brown fox jumps over the lazy dog".into(), + colors: vec!["#3B82F6".into(), "#8B5CF6".into()], + angle: 90.0, + animate_angle: false, + speed: 0.5, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::Px(20.0)), + white_space: Some(WhiteSpace::Nowrap), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + let m = GradientTextIntrinsic::from_gradient_text(>); + let (w, h) = m.measure( + (None, None), + (AvailableSpace::Definite(80.0), AvailableSpace::MaxContent), + ); + assert!( + w > 80.0, + "nowrap gradient_text must ignore the 80px constraint, got {}", + w + ); + // Single line: height should be one line, not several. + let (_, h_unconstrained) = m.measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ); + assert_eq!(h, h_unconstrained); + } + + #[test] + fn gradient_text_intrinsic_wraps_by_default() { + let gt = GradientText { + content: "the quick brown fox jumps over the lazy dog".into(), + colors: vec!["#3B82F6".into(), "#8B5CF6".into()], + angle: 90.0, + animate_angle: false, + speed: 0.5, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::Px(20.0)), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + let m = GradientTextIntrinsic::from_gradient_text(>); + let (_w, h_unconstrained) = m.measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::MaxContent), + ); + let (w_constrained, h_constrained) = m.measure( + (None, None), + (AvailableSpace::Definite(80.0), AvailableSpace::MaxContent), + ); + assert!(w_constrained <= 80.0 + 0.5); + assert!(h_constrained > h_unconstrained); + } + + #[test] + fn caption_intrinsic_ignores_constrained_width_when_nowrap() { + let caption = Caption { + words: "the quick brown fox jumps over the lazy dog" + .split_whitespace() + .map(|w| rustmotion_core::schema::CaptionWord { + text: w.to_string(), + start: 0.0, + end: 10.0, + }) + .collect(), + active_color: "#FFFF00".into(), + mode: Default::default(), + max_width: Some(80.0), + pill_color: None, + style: CssStyle { + font_size: Some(Length::Px(20.0)), + white_space: Some(WhiteSpace::Nowrap), + ..Default::default() + }, + timing: Default::default(), + timeline: Vec::new(), + stagger: None, + }; + let m = CaptionIntrinsic::from_caption(&caption); + let (w, _h) = m.measure( + (None, None), + (AvailableSpace::Definite(80.0), AvailableSpace::MaxContent), + ); + assert!( + w > 80.0, + "nowrap caption intrinsic must ignore the 80px constraint, got {}", + w + ); + } } diff --git a/crates/rustmotion-components/src/rich_text.rs b/crates/rustmotion-components/src/rich_text.rs index 624a79e..86bc0ba 100644 --- a/crates/rustmotion-components/src/rich_text.rs +++ b/crates/rustmotion-components/src/rich_text.rs @@ -77,189 +77,295 @@ fn make_font( Some(Font::from_typeface(typeface, size)) } -/// A prepared span ready for rendering (with resolved font, paint, measurements). -struct PreparedSpan { - text: String, +/// Resolved font/paint metadata for one span. +struct SpanFontInfo { font: Font, color: String, letter_spacing: f32, - width: f32, } -impl RichText { - fn paint(&self, canvas: &Canvas, layout_width: f32, props: &AnimatedProperties) { - let default_size = self.style.font_size_px_or(48.0); - let default_color = self.style.color_str_or("#FFFFFF"); - let default_family = self.style.font_family_or("Inter"); - let default_weight = match &self.style.font_weight { - Some(CssFontWeight::Keyword(FontWeightKw::Bold | FontWeightKw::Bolder)) => { - FontWeight::Bold - } - Some(CssFontWeight::Number(n)) if *n >= 600 => FontWeight::Bold, - Some(CssFontWeight::Number(n)) => FontWeight::Weight(*n), - _ => FontWeight::Normal, - }; - let default_font_style = match self.style.font_style { - Some(CssFontStyle::Italic) => FontStyleType::Italic, - Some(CssFontStyle::Oblique) => FontStyleType::Oblique, - _ => FontStyleType::Normal, - }; - let align = match self.style.text_align { - Some(CssTextAlign::Center) => TextAlign::Center, - Some(CssTextAlign::Right | CssTextAlign::End) => TextAlign::Right, - _ => TextAlign::Left, - }; - let line_height_val = self.style.line_height_for(default_size); +/// Resolve each span's font/color/letter-spacing, falling back to the +/// component-level style for anything a span doesn't override. `None` at +/// index `i` means the span's font failed to load — that span is skipped +/// during tokenization (same behaviour as the original `filter_map`). +fn resolve_span_fonts(spans: &[RichTextSpan], style: &CssStyle) -> Vec> { + let default_size = style.font_size_px_or(48.0); + let default_color = style.color_str_or("#FFFFFF"); + let default_family = style.font_family_or("Inter"); + let default_weight = match &style.font_weight { + Some(CssFontWeight::Keyword(FontWeightKw::Bold | FontWeightKw::Bolder)) => FontWeight::Bold, + Some(CssFontWeight::Number(n)) if *n >= 600 => FontWeight::Bold, + Some(CssFontWeight::Number(n)) => FontWeight::Weight(*n), + _ => FontWeight::Normal, + }; + let default_font_style = match style.font_style { + Some(CssFontStyle::Italic) => FontStyleType::Italic, + Some(CssFontStyle::Oblique) => FontStyleType::Oblique, + _ => FontStyleType::Normal, + }; + spans + .iter() + .map(|span| { + let size = span.font_size.unwrap_or(default_size); + let family = span.font_family.as_deref().unwrap_or(default_family); + let weight = span.font_weight.as_ref().unwrap_or(&default_weight); + let fstyle = span.font_style.as_ref().unwrap_or(&default_font_style); + let color = span.color.as_deref().unwrap_or(default_color).to_string(); + let letter_spacing = span.letter_spacing.unwrap_or(0.0); + make_font(family, weight, fstyle, size).map(|font| SpanFontInfo { + font, + color, + letter_spacing, + }) + }) + .collect() +} + +/// One word-wrapped token ready to be measured or drawn. +pub struct RichTextToken { + pub span_idx: usize, + pub text: String, + pub x: f32, + pub width: f32, +} + +/// One wrapped line of a [`RichText`] layout. +pub struct RichTextLine { + pub tokens: Vec, + pub width: f32, +} + +/// Word-wrapped layout for a [`RichText`], shared by +/// [`crate::intrinsic::RichTextIntrinsic`] and the painter so the box taffy +/// reserves always matches what gets drawn. +pub struct RichTextLayout { + pub lines: Vec, + pub max_width: f32, + pub line_height: f32, + pub max_ascent: f32, +} + +impl RichText { + /// Word-wrap `spans` at `wrap_width` (`None` = unconstrained/natural + /// width). `visible_chars_progress >= 0.0` truncates the content for the + /// typewriter effect (same char-count semantics as `text`); `-1.0` shows + /// everything (used for intrinsic/natural-size measurement, which must + /// not shrink as the typewriter plays out — layout space is reserved for + /// the full content up front). + /// + /// Unlike the original implementation (which only ever broke a line at a + /// span boundary — a single long span never wrapped internally), this + /// tokenizes every span's text into words and packs them greedily across + /// span boundaries, so a long single span wraps like any other text. + /// Whitespace runs collapse to a single rendered space (CSS + /// `white-space: normal` semantics, matching `text`'s default wrap + /// behaviour); the exact inter-word spacing of source whitespace is not + /// preserved, matching how `wrap_text_with_fallback` already treats + /// plain `text` content. + pub fn compute_layout( + spans: &[RichTextSpan], + style: &CssStyle, + wrap_width: Option, + visible_chars_progress: f32, + ) -> RichTextLayout { + let default_size = style.font_size_px_or(48.0); + let line_height_val = style.line_height_for(default_size); + let span_fonts = resolve_span_fonts(spans, style); let emoji_tf = emoji_typeface(); - // Prepare all spans with their fonts and measurements - let mut prepared: Vec = self - .spans - .iter() - .filter_map(|span| { - let size = span.font_size.unwrap_or(default_size); - let family = span.font_family.as_deref().unwrap_or(default_family); - let weight = span.font_weight.as_ref().unwrap_or(&default_weight); - let fstyle = span.font_style.as_ref().unwrap_or(&default_font_style); - let color = span.color.as_deref().unwrap_or(default_color).to_string(); - let letter_spacing = span.letter_spacing.unwrap_or(0.0); - let font = make_font(family, weight, fstyle, size)?; - let emoji_font = emoji_tf - .as_ref() - .map(|tf| Font::from_typeface(tf.clone(), size)); - let width = - measure_text_with_fallback(&span.text, &font, &emoji_font, letter_spacing); - - Some(PreparedSpan { - text: span.text.clone(), - font, - color, - letter_spacing, - width, - }) - }) - .collect(); - - // Typewriter animation: truncate spans based on visible_chars_progress - if props.visible_chars_progress >= 0.0 { - let total_chars: usize = prepared.iter().map(|ps| ps.text.chars().count()).sum(); - let visible = ((props.visible_chars_progress * total_chars as f32).round() as usize) - .min(total_chars); - if visible == 0 { - return; - } - if visible < total_chars { - let mut remaining = visible; - let mut truncated = Vec::new(); - for mut ps in prepared.into_iter() { - let char_count = ps.text.chars().count(); + // Typewriter truncation operates on each span's text by char count + // (mirrors `text`'s approach), producing a truncated copy that is + // then tokenized below exactly like the full content would be. + let texts: Vec = if visible_chars_progress >= 0.0 { + let total_chars: usize = spans.iter().map(|s| s.text.chars().count()).sum(); + let visible = + ((visible_chars_progress * total_chars as f32).round() as usize).min(total_chars); + let mut remaining = visible; + spans + .iter() + .map(|s| { + let char_count = s.text.chars().count(); if remaining >= char_count { remaining -= char_count; - truncated.push(ps); + s.text.clone() + } else if remaining == 0 { + String::new() } else { - let truncated_text: String = ps.text.chars().take(remaining).collect(); - let emoji_font = emoji_tf - .as_ref() - .map(|tf| Font::from_typeface(tf.clone(), ps.font.size())); - ps.width = measure_text_with_fallback( - &truncated_text, - &ps.font, - &emoji_font, - ps.letter_spacing, - ); - ps.text = truncated_text; - truncated.push(ps); - break; + let truncated: String = s.text.chars().take(remaining).collect(); + remaining = 0; + truncated } - } - prepared = truncated; - } - } - - let wrap_width = if layout_width.is_finite() && layout_width > 0.0 { - match self.max_width { - Some(mw) => mw.min(layout_width), - None => layout_width, - } + }) + .collect() } else { - self.max_width.unwrap_or(f32::INFINITY) + spans.iter().map(|s| s.text.clone()).collect() }; - // Simple line-breaking: pack spans into lines - // A span stays on the current line if it fits; otherwise start a new line - struct LineSpan { + // Tokenize into words, tracking whether a rendered space separates + // each token from the previous one (within a span: any whitespace + // run; across spans: only if either side's source text had + // whitespace at the boundary — otherwise the spans are "glued", e.g. + // `"Total: "` followed by `"42"` followed by `" items"`). + struct Tok { span_idx: usize, - x: f32, + text: String, + space_before: bool, } - struct Line { - spans: Vec, - width: f32, + let mut tokens: Vec = Vec::new(); + let mut prev_trailing_ws = true; + for (span_idx, text) in texts.iter().enumerate() { + if span_fonts.get(span_idx).and_then(|f| f.as_ref()).is_none() || text.is_empty() { + continue; + } + let starts_ws = text.chars().next().is_some_and(char::is_whitespace); + for (wi, w) in text.split_whitespace().enumerate() { + let space_before = if tokens.is_empty() { + false + } else if wi > 0 { + true + } else { + prev_trailing_ws || starts_ws + }; + tokens.push(Tok { + span_idx, + text: w.to_string(), + space_before, + }); + } + prev_trailing_ws = text.chars().last().is_none_or(char::is_whitespace); } - let mut lines: Vec = vec![Line { - spans: vec![], + // Greedy line packing, mirroring `wrap_text_with_fallback`'s rule: a + // token always fits on an otherwise-empty line, even if it alone + // exceeds `wrap_width` (never split a single word). + let effective_wrap = wrap_width.unwrap_or(f32::INFINITY); + let mut lines: Vec = vec![RichTextLine { + tokens: Vec::new(), width: 0.0, }]; - for (i, ps) in prepared.iter().enumerate() { + for tok in &tokens { + let sf = span_fonts[tok.span_idx] + .as_ref() + .expect("font presence checked during tokenization"); + let emoji_font = emoji_tf + .as_ref() + .map(|tf| Font::from_typeface(tf.clone(), sf.font.size())); + let tok_width = + measure_text_with_fallback(&tok.text, &sf.font, &emoji_font, sf.letter_spacing); + let space_width = if tok.space_before { + measure_text_with_fallback(" ", &sf.font, &emoji_font, 0.0) + } else { + 0.0 + }; + let current = lines.last_mut().unwrap(); - if current.width + ps.width > wrap_width && !current.spans.is_empty() { - // Start new line - lines.push(Line { - spans: vec![LineSpan { - span_idx: i, + let has_content = !current.tokens.is_empty(); + let extra = if has_content { space_width } else { 0.0 }; + let projected = current.width + extra + tok_width; + + if projected > effective_wrap && has_content { + lines.push(RichTextLine { + tokens: vec![RichTextToken { + span_idx: tok.span_idx, + text: tok.text.clone(), x: 0.0, + width: tok_width, }], - width: ps.width, + width: tok_width, }); } else { - let x = current.width; - current.spans.push(LineSpan { span_idx: i, x }); - current.width += ps.width; + let x = current.width + extra; + current.tokens.push(RichTextToken { + span_idx: tok.span_idx, + text: tok.text.clone(), + x, + width: tok_width, + }); + current.width = x + tok_width; } } - let align_width = if layout_width.is_finite() && layout_width > 0.0 { - layout_width - } else { - lines.iter().map(|l| l.width).fold(0.0f32, f32::max) - }; - - // Find max ascent for baseline alignment - let max_ascent = prepared + let max_width = lines.iter().map(|l| l.width).fold(0.0f32, f32::max); + let max_ascent = span_fonts .iter() - .map(|ps| { - let (_, m) = ps.font.metrics(); + .flatten() + .map(|sf| { + let (_, m) = sf.font.metrics(); -m.ascent }) .fold(0.0f32, f32::max); - let baseline_offset = (line_height_val + max_ascent) / 2.0; + RichTextLayout { + lines, + max_width, + line_height: line_height_val, + max_ascent, + } + } + + fn paint(&self, canvas: &Canvas, layout_width: f32, props: &AnimatedProperties) { + let align = match self.style.text_align { + Some(CssTextAlign::Center) => TextAlign::Center, + Some(CssTextAlign::Right | CssTextAlign::End) => TextAlign::Right, + _ => TextAlign::Left, + }; + + let wrap_width = if layout_width.is_finite() && layout_width > 0.0 { + match self.max_width { + Some(mw) => Some(mw.min(layout_width)), + None => Some(layout_width), + } + } else { + self.max_width + }; + + let layout = RichText::compute_layout( + &self.spans, + &self.style, + wrap_width, + props.visible_chars_progress, + ); + if layout.lines.iter().all(|l| l.tokens.is_empty()) { + return; + } + + let span_fonts = resolve_span_fonts(&self.spans, &self.style); + let emoji_tf = emoji_typeface(); + + let align_width = if layout_width.is_finite() && layout_width > 0.0 { + layout_width + } else { + layout.max_width + }; + + let baseline_offset = (layout.line_height + layout.max_ascent) / 2.0; - for (line_idx, line) in lines.iter().enumerate() { + for (line_idx, line) in layout.lines.iter().enumerate() { let line_x_offset = match align { TextAlign::Left => 0.0, TextAlign::Center => (align_width - line.width) / 2.0, TextAlign::Right => align_width - line.width, }; + let y = line_idx as f32 * layout.line_height + baseline_offset; - let y = line_idx as f32 * line_height_val + baseline_offset; - - for ls in &line.spans { - let ps = &prepared[ls.span_idx]; - let paint = paint_from_hex(&ps.color); + for tok in &line.tokens { + let sf = span_fonts[tok.span_idx] + .as_ref() + .expect("font presence matches compute_layout's tokenization"); + let paint = paint_from_hex(&sf.color); let emoji_font = emoji_tf .as_ref() - .map(|tf| Font::from_typeface(tf.clone(), ps.font.size())); + .map(|tf| Font::from_typeface(tf.clone(), sf.font.size())); draw_text_with_fallback( canvas, - &ps.text, - &ps.font, + &tok.text, + &sf.font, &emoji_font, - ps.letter_spacing, - line_x_offset + ls.x, + sf.letter_spacing, + line_x_offset + tok.x, y, &paint, ); @@ -279,3 +385,114 @@ impl Painter for RichText { self.paint(canvas, layout.width, props); } } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::CssStyle; + + fn span(text: &str) -> RichTextSpan { + RichTextSpan { + text: text.into(), + color: None, + font_size: None, + font_weight: None, + font_family: None, + font_style: None, + letter_spacing: None, + } + } + + fn style(font_px: f32) -> CssStyle { + CssStyle { + font_size: Some(rustmotion_core::css::Length::Px(font_px)), + ..Default::default() + } + } + + #[test] + fn single_long_span_wraps_into_multiple_lines_at_constrained_width() { + // M2's second ask: line-breaking must not be limited to span + // boundaries — a single long span must wrap word-by-word. + let spans = vec![span( + "the quick brown fox jumps over the lazy dog and keeps going", + )]; + let s = style(24.0); + let unconstrained = RichText::compute_layout(&spans, &s, None, -1.0); + assert_eq!( + unconstrained.lines.len(), + 1, + "unconstrained width must fit on one line" + ); + + let constrained = RichText::compute_layout(&spans, &s, Some(150.0), -1.0); + assert!( + constrained.lines.len() > 1, + "a single long span must wrap into multiple lines at 150px, got {} line(s)", + constrained.lines.len() + ); + for line in &constrained.lines { + assert!( + line.width <= 150.0 + 0.5, + "each wrapped line must fit the constraint, got {}", + line.width + ); + } + } + + #[test] + fn spans_glue_without_extra_space_when_source_has_none() { + // "Total: " + "42" + " items" — no extra space should appear between + // "Total:" and "42" beyond the one already in the first span's text, + // and none at all between "42" and " items" beyond the leading space + // already in the third span. + let glued = vec![span("Total:"), span("42"), span(" items")]; + let s = style(20.0); + let layout = RichText::compute_layout(&glued, &s, None, -1.0); + assert_eq!(layout.lines.len(), 1); + let tokens = &layout.lines[0].tokens; + assert_eq!( + tokens.iter().map(|t| t.text.as_str()).collect::>(), + vec!["Total:", "42", "items"] + ); + // "Total:" is glued directly to "42" (no whitespace at the + // boundary), so token 1 starts exactly where token 0's glyphs end. + assert_eq!(tokens[1].x, tokens[0].width, "no space between glued spans"); + // "42" and " items" DO have a boundary space (leading space in the + // third span's source text), so token 2 must start strictly after + // token 1 ends. + assert!( + tokens[2].x > tokens[1].x + tokens[1].width, + "a space must separate '42' and 'items' (source had a leading space)" + ); + } + + #[test] + fn typewriter_truncation_hides_tail_tokens() { + let spans = vec![span("Hello "), span("world")]; + let s = style(20.0); + let full = RichText::compute_layout(&spans, &s, None, -1.0); + let half = RichText::compute_layout(&spans, &s, None, 0.5); + let none = RichText::compute_layout(&spans, &s, None, 0.0); + + let full_tokens: usize = full.lines.iter().map(|l| l.tokens.len()).sum(); + let half_tokens: usize = half.lines.iter().map(|l| l.tokens.len()).sum(); + let none_tokens: usize = none.lines.iter().map(|l| l.tokens.len()).sum(); + + assert!(full_tokens >= half_tokens); + assert_eq!(none_tokens, 0, "progress 0.0 must show nothing"); + assert!( + half_tokens >= 1, + "progress 0.5 must show at least one token" + ); + } + + #[test] + fn empty_spans_produce_one_empty_line_not_a_panic() { + let spans: Vec = vec![]; + let s = style(20.0); + let layout = RichText::compute_layout(&spans, &s, None, -1.0); + assert_eq!(layout.lines.len(), 1); + assert_eq!(layout.max_width, 0.0); + } +} diff --git a/crates/rustmotion-components/src/text.rs b/crates/rustmotion-components/src/text.rs index b0f96be..9534f16 100644 --- a/crates/rustmotion-components/src/text.rs +++ b/crates/rustmotion-components/src/text.rs @@ -6,7 +6,8 @@ use skia_safe::{Canvas, Font, FontStyle, Paint, PaintStyle, Rect}; use rustmotion_core::engine::animator::ease; use rustmotion_core::css::style::{ - FontStyle as CssFontStyle, FontWeight as CssFontWeight, FontWeightKw, TextAlign as CssTextAlign, + FontStyle as CssFontStyle, FontWeight as CssFontWeight, FontWeightKw, + TextAlign as CssTextAlign, WhiteSpace as CssWhiteSpace, }; use rustmotion_core::css::CssStyle; use rustmotion_core::engine::animator::AnimatedProperties; @@ -365,8 +366,20 @@ impl Text { let mut paint = paint_from_hex(color); paint.set_alpha_f(1.0); - // Use layout width as wrapping constraint, combined with max_width - let wrap_width = if layout_width.is_finite() && layout_width > 0.0 { + // Use layout width as wrapping constraint, combined with max_width. + // M1: `white-space: nowrap|pre` disables wrapping entirely — the + // line may then exceed `layout_width`. That's the point: it makes + // the property mean something, and it's exactly the condition the + // geometry validator's `unwrappable_text_overflow` check (which + // re-measures via `TextIntrinsic::from_text`, now wrap-aware too) + // assumes the renderer can produce. + let nowrap = matches!( + self.style.white_space, + Some(CssWhiteSpace::Nowrap | CssWhiteSpace::Pre) + ); + let wrap_width = if nowrap { + None + } else if layout_width.is_finite() && layout_width > 0.0 { match self.max_width { Some(mw) => Some(mw.min(layout_width)), None => Some(layout_width), @@ -558,3 +571,139 @@ impl Painter for Text { let _ = self.paint(canvas, layout.width, ctx.time, props, ctx); } } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::style::CssStyle; + use rustmotion_core::css::Length; + + fn make_text(content: &str, white_space: Option) -> Text { + Text { + content: content.into(), + max_width: None, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::Px(28.0)), + color: Some(rustmotion_core::css::style::Color::String("#FFFFFF".into())), + white_space, + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + text_shadow: None, + stroke: None, + text_background: None, + } + } + + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 600, + video_height: 200, + stagger_offset: 0.0, + } + } + + /// Reads back the full alpha channel of the surface as a `width × height` + /// row-major byte grid. + fn alpha_grid(surface: &mut skia_safe::Surface, width: i32, height: i32) -> Vec { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (width, height), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (width * height * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (width * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + (0..(width * height) as usize) + .map(|i| buf[i * 4 + 3]) + .collect() + } + + /// Does any pixel in `[x0, x1) × [y0, y1)` have non-zero alpha? Scanning + /// a region rather than a single exact pixel avoids flaking on the gap + /// between two glyphs or on a space character. + 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 { + if grid[(y * surface_width + x) as usize] > 0 { + return true; + } + } + } + false + } + + #[test] + fn nowrap_paints_a_single_line_past_the_layout_width() { + // M1 render-level proof: a `white-space: nowrap` line stays on one + // line and its glyphs visibly extend past `layout_width` — the box + // it was allocated. + let text = make_text( + "the quick brown fox jumps over the lazy dog", + Some(CssWhiteSpace::Nowrap), + ); + const W: i32 = 600; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + let canvas = surface.canvas(); + let ctx = test_ctx(); + let props = AnimatedProperties::default(); + text.paint(canvas, 80.0, 0.0, &props, &ctx) + .expect("paint succeeds"); + let grid = alpha_grid(&mut surface, W, H); + + // Far past the 80px box, on the first line's height band: nowrap + // must have painted ink there (the line didn't break at 80px). + assert!( + has_ink_in(&grid, W, 300, W, 0, 45), + "nowrap text must paint past its 80px box on line 1 (scanned x∈[300,600), y∈[0,45))" + ); + + // Nothing should be on a *second* line — nowrap never word-wraps + // (only literal newlines would start a new line, and there are + // none here), so all ink stays within the first line's height band. + assert!( + !has_ink_in(&grid, W, 0, W, 55, H), + "nowrap text must stay on a single line; found ink on what would be line 2" + ); + } + + #[test] + fn normal_white_space_wraps_within_the_layout_width() { + // Contrast case: default wrapping keeps ink within the box on the + // first line, and instead spills onto additional lines below. + let text = make_text("the quick brown fox jumps over the lazy dog", None); + const W: i32 = 600; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + let canvas = surface.canvas(); + let ctx = test_ctx(); + let props = AnimatedProperties::default(); + text.paint(canvas, 80.0, 0.0, &props, &ctx) + .expect("paint succeeds"); + let grid = alpha_grid(&mut surface, W, H); + + assert!( + !has_ink_in(&grid, W, 300, W, 0, 45), + "wrapped text must not reach x∈[300,600) on line 1 within an 80px box" + ); + assert!( + has_ink_in(&grid, W, 0, W, 55, H), + "wrapped text must spill onto a second line within the box width" + ); + } +} diff --git a/crates/rustmotion-core/src/engine/animator.rs b/crates/rustmotion-core/src/engine/animator.rs index e79aae2..4fde267 100644 --- a/crates/rustmotion-core/src/engine/animator.rs +++ b/crates/rustmotion-core/src/engine/animator.rs @@ -52,6 +52,23 @@ pub struct ExtractedEffects<'a> { pub char_animation: Option, } +/// M3: find the first `glow` effect in a list, if present. +/// +/// `glow` is a static (non-time-varying) coloured halo — unlike every other +/// effect `resolve_props_for_effects` resolves, it deliberately is *not* +/// folded into `AnimatedProperties`: `GlowConfig.color` has no corresponding +/// field there, and extending `AnimatedProperties`'s public shape is out of +/// scope for this workstream. Callers apply the returned config directly as +/// a CSS `filter: drop-shadow(...)` (see +/// `rustmotion_components::box_builder::apply_glow_effect`), which is the +/// only place that needs the raw colour string. +pub fn find_glow_effect(effects: &[AnimationEffect]) -> Option<&GlowConfig> { + effects.iter().find_map(|e| match e { + AnimationEffect::Glow(cfg) => Some(cfg), + _ => None, + }) +} + /// Split a slice of AnimationEffect into categorized buckets for the renderer. pub fn extract_effects(effects: &[AnimationEffect]) -> ExtractedEffects<'_> { let mut result = ExtractedEffects { @@ -1685,3 +1702,62 @@ mod spring_preset_tests { assert!(t.spring.is_none()); } } + +#[cfg(test)] +mod glow_tests { + //! M3: `find_glow_effect` extraction (issue #109). The colour/filter + //! side of the fix lives in + //! `rustmotion_components::box_builder::apply_glow_effect`, which is + //! covered in that crate's own tests since it needs `CssStyle`/`FilterFn` + //! (not available to this crate). + + use super::*; + use crate::schema::{AnimationEffect, AnimationTiming, GlowConfig}; + + fn glow(color: &str, radius: f32, intensity: f32) -> AnimationEffect { + AnimationEffect::Glow(GlowConfig { + color: color.to_string(), + radius, + intensity, + }) + } + + #[test] + fn finds_glow_among_other_effects() { + let effects = vec![ + AnimationEffect::FadeIn(AnimationTiming::default()), + glow("#5C39EE", 12.0, 1.0), + ]; + let found = find_glow_effect(&effects).expect("glow effect present"); + assert_eq!(found.color, "#5C39EE"); + assert_eq!(found.radius, 12.0); + } + + #[test] + fn returns_none_without_a_glow_effect() { + let effects = vec![AnimationEffect::FadeIn(AnimationTiming::default())]; + assert!(find_glow_effect(&effects).is_none()); + } + + #[test] + fn resolve_props_for_effects_does_not_touch_glow_radius_or_intensity() { + // Deliberate: the named `glow` effect is applied directly as a CSS + // filter by `box_builder::apply_glow_effect` (using the raw + // `GlowConfig.color`, which `AnimatedProperties` has no field for), + // not through this resolver. This guards against a future change + // accidentally routing it through `AnimatedProperties` too, which + // would double up into two stacked drop-shadows (see the doc comment + // on `apply_glow_effect`). + let effects = vec![glow("#5C39EE", 12.0, 1.0)]; + let props = resolve_props_for_effects(&effects, 0.0, 1.0); + assert_eq!( + props.glow_radius, + AnimatedProperties::default().glow_radius, + "glow_radius must stay at its sentinel; the named `glow` effect must not set it" + ); + assert_eq!( + props.glow_intensity, + AnimatedProperties::default().glow_intensity + ); + } +} From 164db0c36a84bdcdd988fae4fd86e0f7e42ce06b Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 1 Aug 2026 18:10:40 +0200 Subject: [PATCH 3/3] fix(validate): stop accepting input that has no effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four ways the tool could mislead an author, closed together. A misplaced attribute — color or font_size at component root instead of inside style — was a warning, and render exited 0 having silently used defaults. Both now fail. The checker also only ever inspected the top level of each component, so a misspelled scene key like "durration" passed silently; Scenario, Scene, View, VideoConfig, SceneLayout, Transition and Camera now reject unknown fields too. SceneEntry was serde(untagged), so the complete message for a missing duration or a misspelled transition type was "data did not match any variant of untagged enum SceneEntry" — no line, no scene index, no field name. Errors now name themselves. Text below roughly 1.2% of the output height is unreadable in a video even though it fits the frame perfectly, so validation said nothing. Threshold chosen by rendering a line at sizes from 8 to 28px and downscaling 50%, the way video is actually watched: everything below 14px degraded into a grey smear. Expressed as a fraction of height so it holds on 4K and vertical canvases, and it clears every shipped component default. Advisory, not blocking. Unresolvable colours now fail validation. Wave 1 made them loud at render time by painting magenta instead of black; catching them before anyone renders is the other half. --strict-attrs is left accepted so scripts keep working, but it now announces that it does nothing. A flag with no observable effect is the same defect as a schema property with no observable effect, which is what this whole change is about. Refs #110 --- .../rustmotion-cli/src/commands/geometry.rs | 312 ++++++++++++++++++ crates/rustmotion-cli/src/commands/render.rs | 19 +- .../rustmotion-cli/src/commands/validate.rs | 1 + .../src/commands/validate_attrs.rs | 64 +++- .../src/commands/validate_schema.rs | 226 ++++++++++++- .../rustmotion-cli/src/commands/validation.rs | 82 ++++- crates/rustmotion-cli/src/lib.rs | 18 +- crates/rustmotion-core/src/schema/scenario.rs | 278 +++++++++++++++- 8 files changed, 977 insertions(+), 23 deletions(-) diff --git a/crates/rustmotion-cli/src/commands/geometry.rs b/crates/rustmotion-cli/src/commands/geometry.rs index 81c814a..823a8d1 100644 --- a/crates/rustmotion-cli/src/commands/geometry.rs +++ b/crates/rustmotion-cli/src/commands/geometry.rs @@ -677,6 +677,152 @@ fn check_auto_scroll( } } +// ─── M4: legibility floor (issue #110 / #102) ────────────────────────────── +// +// "Fits in the frame" (checked above) is not "readable in a video". A table +// column, a badge, a codeblock line — any of them can validate perfectly +// clean while rendering at a font size nobody could read once the video is +// scaled down from its native resolution, which is how video is normally +// watched (embedded players, mobile feeds, thumbnails) unlike a web page, +// which is usually viewed close to 1:1. +// +// Threshold justification (rendered evidence, not a guess): a 1920×1080 +// scenario was rendered with the same sample line at 8/10/11/12/13/14/16/18/ +// 20/22/24/28px, then the frame was scaled down 50% (a realistic "not +// full-native" viewing size) to inspect. 8–13px degraded to an illegible +// grey smear at that scale; 14px was the first size that stayed readable. +// 0.012 (1.2% of output height) sits between those two bands — it equals +// ~13px on a 1080p frame — and clears every built-in component default +// already shipped (table/terminal/codeblock/pill_nav = 14px, badge `md` = +// 14px, kbd = 14px, tooltip = 13px), so it does not fire on scenarios that +// already validate clean today. Expressing it as a fraction of output +// height (rather than an absolute px count) makes the same *visual* size +// get flagged on a 4K or vertical-format canvas too. +const MIN_LEGIBLE_FONT_RATIO: f32 = 0.012; + +/// Check every text-bearing component's effective font size against +/// [`MIN_LEGIBLE_FONT_RATIO`] of the output height. Always advisory (a +/// warning, never a blocking error) — this is a legibility floor, not a +/// geometry correctness check, and the "right" size is ultimately an +/// authorial call. +/// +/// Coverage: every component whose `Painter` resolves its rendered font +/// size from `style.font-size` (falling back to that component's own +/// documented default when unset) — text, rich_text, gradient_text, +/// caption, counter, table, terminal, codeblock, callout, list, +/// notification (title + message), pill_nav, badge, kbd, tooltip, marquee. +/// Not covered: components whose text sizing isn't a simple +/// `style.font-size`-or-default resolution (chart axis/labels, gauge, stat, +/// sparkline, heatmap, treemap, dot_map, avatar initials, progress label, +/// rating, countdown, comparison, stepper, timeline, tag_cloud) — see the +/// workstream report for the full list. +pub fn check_legibility(scenario: &ResolvedScenario) -> Vec { + let mut warnings = Vec::new(); + let video_h = scenario.video.height as f32; + if video_h <= 0.0 { + return warnings; + } + let min_px = MIN_LEGIBLE_FONT_RATIO * video_h; + + for (vi, view) in scenario.views.iter().enumerate() { + for (si, scene) in view.scenes.iter().enumerate() { + let indexed = deserialize_children_indexed(scene); + let path_root = format!("views[{}].scenes[{}]", vi, si); + for (json_idx, child) in &indexed { + let path = format!("{}.children[{}]", path_root, json_idx); + walk_legibility(&child.component, &path, min_px, video_h, &mut warnings); + } + } + } + warnings +} + +fn walk_legibility( + component: &Component, + path: &str, + min_px: f32, + video_h: f32, + out: &mut Vec, +) { + for (label, effective_px) in text_sizes(component) { + // 0.05px tolerance for float rounding; not a meaningful visual gap. + if effective_px < min_px - 0.05 { + out.push(format!( + "{path}: {label} renders at ~{effective_px:.0}px on a {video_h:.0}px-tall frame \ + ({:.2}% of height) — likely illegible once the video is viewed at anything less \ + than native resolution. Raise the effective font size to at least {min_px:.0}px \ + (~{:.1}% of height).", + effective_px / video_h * 100.0, + MIN_LEGIBLE_FONT_RATIO * 100.0, + )); + } + } + + if let Some(children) = container_children(component) { + for (i, child) in children.iter().enumerate() { + walk_legibility( + &child.component, + &format!("{path}.children[{i}]"), + min_px, + video_h, + out, + ); + } + } +} + +/// Effective rendered font size(s) for a component, mirroring exactly the +/// default each `Painter` falls back to when `style.font-size` is unset +/// (see the file/line citations below — kept in sync by hand since these +/// defaults live in `rustmotion-components`, out of this workstream's +/// scope). A component can report more than one size (e.g. a notification's +/// title and message use different sizes). +fn text_sizes(component: &Component) -> Vec<(&'static str, f32)> { + match component { + // text.rs, rich_text.rs, gradient_text.rs, caption.rs, counter.rs: 48.0 + Component::Text(t) => vec![("text", t.style.font_size_px_or(48.0))], + Component::RichText(t) => vec![("rich_text", t.style.font_size_px_or(48.0))], + Component::GradientText(t) => vec![("gradient_text", t.style.font_size_px_or(48.0))], + Component::Caption(t) => vec![("caption", t.style.font_size_px_or(48.0))], + Component::Counter(c) => vec![("counter", c.style.font_size_px_or(48.0))], + // table.rs, terminal.rs, codeblock/{dimensions,render}.rs, pill_nav.rs: 14.0 + Component::Table(t) => vec![("table", t.style.font_size_px_or(14.0))], + Component::Terminal(t) => vec![("terminal", t.style.font_size_px_or(14.0))], + Component::Codeblock(c) => vec![("codeblock", c.style.font_size_px_or(14.0))], + Component::PillNav(p) => vec![("pill_nav", p.style.font_size_px_or(14.0))], + // callout.rs, list.rs, notification.rs (title): 16.0 + Component::Callout(c) => vec![("callout", c.style.font_size_px_or(16.0))], + Component::List(l) => vec![("list", l.style.font_size_px_or(16.0))], + Component::Notification(n) => { + let title = n.style.font_size_px_or(16.0); + let mut sizes = vec![("notification title", title)]; + if n.message.is_some() { + // notification.rs: message_font_size() = title_font_size() * 0.85 + sizes.push(("notification message", title * 0.85)); + } + sizes + } + // These carry their own `font_size` field (already serde-resolved + // to its component default when absent from JSON), overridable by + // `style.font-size` exactly like the rest — kbd.rs, tooltip.rs, + // marquee.rs. + Component::Kbd(k) => vec![("kbd", k.style.font_size_px_or(k.font_size))], + Component::Tooltip(t) => vec![("tooltip", t.style.font_size_px_or(t.font_size))], + Component::Marquee(m) => vec![("marquee", m.style.font_size_px_or(m.font_size))], + // badge.rs: BadgeSize::{Sm,Md,Lg}.params().0 = {12.0, 14.0, 18.0}. + // `params()` is private to badge.rs, so the table is duplicated here. + Component::Badge(b) => { + let default_fs = match b.badge_size { + rustmotion::components::badge::BadgeSize::Sm => 12.0, + rustmotion::components::badge::BadgeSize::Md => 14.0, + rustmotion::components::badge::BadgeSize::Lg => 18.0, + }; + vec![("badge", b.style.font_size_px_or(default_fs))] + } + _ => vec![], + } +} + fn component_kind(c: &Component) -> &'static str { match c { Component::Text(_) => "text", @@ -1854,3 +2000,169 @@ mod tests { .any(|v| v.kind == ViolationKind::UnwrappableTextOverflow)); } } + +/// M4 (issue #110 / #102): legibility floor tests. +#[cfg(test)] +mod legibility_tests { + use super::*; + use rustmotion::loader::load_scenario_from_source; + + fn parse(json: &str) -> rustmotion::schema::ResolvedScenario { + load_scenario_from_source(None, Some(json)).expect("scenario parses") + } + + #[test] + fn tiny_font_on_1080p_warns() { + // 11px on a 1080p frame is the audit's own worked example of + // unreadable text (~1.0% of height, well under the 1.2% floor). + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "text", + "content": "fine print", + "style": { "color": "#ffffff", "font-size": "11px" } + }] + }] + }"##; + let scenario = parse(json); + let warnings = check_legibility(&scenario); + assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}"); + assert!(warnings[0].contains("11px"), "got: {}", warnings[0]); + assert!( + warnings[0].contains("views[0].scenes[0].children[0]"), + "got: {}", + warnings[0] + ); + } + + #[test] + fn default_sized_text_on_1080p_has_no_legibility_warning() { + // No style.font-size override: falls back to text's own 48px + // default, comfortably above the floor. + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "text", + "content": "headline", + "style": { "color": "#ffffff" } + }] + }] + }"##; + let scenario = parse(json); + let warnings = check_legibility(&scenario); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + } + + #[test] + fn default_table_terminal_codeblock_on_1080p_do_not_warn() { + // 14px defaults must clear the floor so this check doesn't spam + // every scenario that never touched style.font-size. + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [ + { "type": "table", "headers": ["a"], "rows": [["1"]] }, + { "type": "terminal", "lines": [{ "text": "$ ok", "type": "input" }] }, + { "type": "codeblock", "code": "fn main() {}" } + ] + }] + }"##; + let scenario = parse(json); + let warnings = check_legibility(&scenario); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + } + + #[test] + fn same_absolute_px_warns_more_readily_on_a_taller_frame() { + // The floor is a fraction of output height, so the same 20px text + // that's fine on 1080p (1.85%) should warn on a much taller canvas + // where 20px is proportionally tiny. + let json = r##"{ + "video": { "width": 1080, "height": 4000 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "text", + "content": "small on a huge canvas", + "style": { "color": "#ffffff", "font-size": "20px" } + }] + }] + }"##; + let scenario = parse(json); + let warnings = check_legibility(&scenario); + assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}"); + } + + #[test] + fn small_badge_size_warns_using_its_own_default() { + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "badge", + "text": "new", + "badge_size": "sm" + }] + }] + }"##; + let scenario = parse(json); + let warnings = check_legibility(&scenario); + assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}"); + assert!(warnings[0].contains("badge"), "got: {}", warnings[0]); + } + + #[test] + fn legibility_never_blocks_validation() { + use crate::commands::validate_schema::validate_scenario; + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "text", + "content": "fine print", + "style": { "color": "#ffffff", "font-size": "6px" } + }] + }] + }"##; + let scenario = parse(json); + assert!(!check_legibility(&scenario).is_empty()); + let (errors, _warnings) = validate_scenario(&scenario); + assert!( + errors.is_empty(), + "legibility must never surface as a schema error: {errors:?}" + ); + } + + #[test] + fn nested_card_child_gets_a_nested_path() { + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "card", + "children": [{ + "type": "text", + "content": "fine print", + "style": { "color": "#ffffff", "font-size": "8px" } + }] + }] + }] + }"##; + let scenario = parse(json); + let warnings = check_legibility(&scenario); + assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}"); + assert!( + warnings[0].contains("views[0].scenes[0].children[0].children[0]"), + "got: {}", + warnings[0] + ); + } +} diff --git a/crates/rustmotion-cli/src/commands/render.rs b/crates/rustmotion-cli/src/commands/render.rs index 7d52f38..6aff1f4 100644 --- a/crates/rustmotion-cli/src/commands/render.rs +++ b/crates/rustmotion-cli/src/commands/render.rs @@ -10,15 +10,26 @@ use crate::commands::validation::{self, ValidationSource}; /// Load + validate a scenario for watch mode. On validation failure prints the /// report and returns the typed error so the caller can decide how to handle it. +/// +/// `strict_attrs` is accepted for CLI-surface parity with `validate` (M5, +/// issue #110) but is a no-op in practice: unknown component attributes +/// block by default now (`ValidationReport::is_blocking`), and `--watch` +/// mode never writes a `--report` JSON file, so there is no bucket left for +/// `promote_attr_warnings` to affect. +#[allow(clippy::too_many_arguments)] fn load_for_watch( input: &Path, no_validate: bool, lenient: bool, strict_anim: bool, + strict_attrs: bool, ) -> Result { let loaded = validation::load(ValidationSource::File(input))?; if !no_validate { - let report = validation::run_checks(&loaded, strict_anim); + let mut report = validation::run_checks(&loaded, strict_anim); + if strict_attrs { + report.promote_attr_warnings(); + } if !report.is_clean() { validation::print_report(&report, &input.display().to_string()); } @@ -204,6 +215,7 @@ pub fn cmd_render( Ok(()) } +#[allow(clippy::too_many_arguments)] pub fn cmd_watch( input: &PathBuf, output: &Path, @@ -217,6 +229,7 @@ pub fn cmd_watch( no_validate: bool, lenient: bool, strict_anim: bool, + strict_attrs: bool, ) -> Result<()> { use notify::{RecursiveMode, Watcher}; use std::sync::mpsc; @@ -250,7 +263,7 @@ pub fn cmd_watch( let mut initial_includes: Vec = Vec::new(); // Initial render - match load_for_watch(input, no_validate, lenient, strict_anim) { + match load_for_watch(input, no_validate, lenient, strict_anim, strict_attrs) { Ok(scenario) => { initial_includes = scenario.included_paths.clone(); @@ -367,7 +380,7 @@ pub fn cmd_watch( std::thread::sleep(std::time::Duration::from_millis(100)); while rx.try_recv().is_ok() {} - match load_for_watch(input, no_validate, lenient, strict_anim) { + match load_for_watch(input, no_validate, lenient, strict_anim, strict_attrs) { Ok(scenario) => { // Reset error backoff on a successful load if consecutive_err_count > 0 && suppressed { diff --git a/crates/rustmotion-cli/src/commands/validate.rs b/crates/rustmotion-cli/src/commands/validate.rs index 3ec4a87..5369769 100644 --- a/crates/rustmotion-cli/src/commands/validate.rs +++ b/crates/rustmotion-cli/src/commands/validate.rs @@ -23,6 +23,7 @@ pub fn cmd_validate( let mut report_out = validation::run_checks(&loaded, strict_anim); if strict_attrs { + validation::warn_strict_attrs_is_now_default(); report_out.promote_attr_warnings(); } diff --git a/crates/rustmotion-cli/src/commands/validate_attrs.rs b/crates/rustmotion-cli/src/commands/validate_attrs.rs index e179974..db66822 100644 --- a/crates/rustmotion-cli/src/commands/validate_attrs.rs +++ b/crates/rustmotion-cli/src/commands/validate_attrs.rs @@ -5,7 +5,17 @@ //! typo'd attribute like `` silently disappears at //! typed load. This module rebuilds the set of known top-level properties per //! component from the schemars JSON Schema of `Component` and reports any -//! unknown key as a warning (promoted to an error by `--strict-attrs`). +//! unknown key into `attr_warnings`. +//! +//! M5 (issue #110 / #102, decided at kickoff): unknown attributes error +//! **by default** now, not only under `--strict-attrs` as before. This +//! module still returns them separately as `(errors, warnings)` — callers +//! outside `run_checks` (e.g. this module's own tests) can inspect them +//! independently — but `validation::run_checks` folds the warnings straight +//! into `schema_errors` unconditionally before anyone sees a +//! `ValidationReport`, so `validate`/`render`/`watch` all block on them with +//! no extra wiring. `--strict-attrs` / `ValidationReport::promote_attr_warnings` +//! still exist, purely for CLI-surface stability — see their doc comments. //! //! It also surfaces typed-deserialization failures (e.g. an unknown CSS //! property, rejected by `CssStyle`'s `deny_unknown_fields`, or a missing @@ -208,7 +218,41 @@ mod tests { } #[test] - fn strict_attrs_promotes_warnings_to_errors() { + fn unknown_attr_blocks_by_default_without_strict_attrs() { + // M5 (issue #110): unknown attributes error by default — no + // `--strict-attrs` needed. `render` used to exit 0 here, having + // silently used default styling. `run_checks` folds them straight + // into `schema_errors` (see its doc comment), so every caller + // (`validate`, `render`, `watch`) blocks uniformly with zero extra + // wiring — `report.attr_warnings` itself is already empty by the + // time `run_checks` returns. + let json = serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ "duration": 2.0, "children": [ + { "type": "counter", "from": 0, "to": 100, "typo-attr": "x" } + ]}] + }) + .to_string(); + let loaded = load(ValidationSource::Inline(&json)).unwrap(); + let report = run_checks(&loaded, false); + assert!( + report.schema_errors.iter().any(|e| e.contains("typo-attr")), + "expected the unknown attribute in schema_errors: {:?}", + report.schema_errors + ); + assert!(report.attr_warnings.is_empty(), "already folded in"); + assert!( + report.is_blocking(false), + "unknown attributes must block by default, with no flag" + ); + } + + #[test] + fn strict_attrs_promotion_is_a_harmless_no_op_post_m5() { + // `--strict-attrs` / `promote_attr_warnings` are kept for CLI-surface + // stability, but since M5 there is nothing left for them to do by + // the time `run_checks` has already run: `attr_warnings` is already + // empty, so promoting it is a no-op, and the report already blocked. let json = serde_json::json!({ "video": { "width": 100, "height": 100 }, "scenes": [{ "duration": 2.0, "children": [ @@ -218,17 +262,19 @@ mod tests { .to_string(); let loaded = load(ValidationSource::Inline(&json)).unwrap(); let mut report = run_checks(&loaded, false); - assert!(!report.attr_warnings.is_empty(), "expected attr warnings"); - assert!(!report.is_blocking(false), "warnings must not block"); + let before = report.schema_errors.clone(); + assert!( + report.is_blocking(false), + "must already block pre-promotion" + ); report.promote_attr_warnings(); assert!(report.attr_warnings.is_empty()); - assert!( - report.schema_errors.iter().any(|e| e.contains("typo-attr")), - "promoted error missing: {:?}", - report.schema_errors + assert_eq!( + report.schema_errors, before, + "promotion must not change schema_errors when attr_warnings is already empty" ); - assert!(report.is_blocking(false), "strict attrs must block"); + assert!(report.is_blocking(false), "must still block post-promotion"); } #[test] diff --git a/crates/rustmotion-cli/src/commands/validate_schema.rs b/crates/rustmotion-cli/src/commands/validate_schema.rs index 256f369..29bdfb5 100644 --- a/crates/rustmotion-cli/src/commands/validate_schema.rs +++ b/crates/rustmotion-cli/src/commands/validate_schema.rs @@ -2,7 +2,9 @@ //! Returns (errors, warnings); errors block rendering, warnings are advisory only. use rustmotion::components::{ChildComponent, Component}; -use rustmotion::core::css::style::Display as CssDisplay; +use rustmotion::core::css::style::{ + Background, BackgroundLayer, Color, CssStyle, Display as CssDisplay, +}; use rustmotion::schema::{AnimationEffect, CharAnimationTiming, ResolvedScenario}; pub fn validate_scenario(scenario: &ResolvedScenario) -> (Vec, Vec) { @@ -90,6 +92,13 @@ fn validate_children( )); } + // C2 completion (issue #110 / #102): a colour that `parse_css_color` + // can't resolve used to fall back to black silently; wave 1 made + // that loud at render time (opaque magenta + stderr warning) via the + // same frozen `parse_css_color` entry point. This closes the loop by + // catching it before anyone renders. + check_style_colors(style, &p, errors); + if let Some(timed) = child.component.as_timed() { let (start, end) = timed.timing(); if let (Some(s), Some(e)) = (start, end) { @@ -230,6 +239,114 @@ fn validate_children( } } +/// C2 completion: walk every colour reachable from a component's `style` +/// (foreground `color`, `background` fills/gradient stops, box/text shadow +/// colours, border colours, gradient-border colours) and report any +/// `Color::String` that `parse_css_color` — the single frozen colour-parsing +/// entry point (`engine/renderer/colors.rs`) — cannot resolve. At render +/// time an unresolved colour already falls back to opaque magenta with a +/// stderr warning (wave 1); this makes the same failure a blocking +/// validation error so it's caught before anyone renders. +/// +/// Scope: only `CssStyle`'s `Color`-typed fields, which is exactly the +/// surface `paint_pass::parse_color` resolves through `parse_css_color` for +/// every component. Component-specific plain-`String` colour fields (e.g. +/// `Kbd.background_color`, `Table.header_color`, `Marquee.color`, +/// `Notification.accent_color`) go through a different, more lenient path +/// (`parse_hex_color`'s bare-hex retry) and are deliberately not covered +/// here — see the workstream report for the full list. +fn check_style_colors(style: &CssStyle, path: &str, errors: &mut Vec) { + if let Some(c) = &style.color { + check_color(c, "color", path, errors); + } + if let Some(bg) = &style.background { + check_background_colors(bg, path, errors); + } + if let Some(shadows) = &style.box_shadow { + for (i, s) in shadows.iter().enumerate() { + if let Some(c) = &s.color { + check_color(c, &format!("box-shadow[{i}].color"), path, errors); + } + } + } + if let Some(shadows) = &style.text_shadow { + for (i, s) in shadows.iter().enumerate() { + if let Some(c) = &s.color { + check_color(c, &format!("text-shadow[{i}].color"), path, errors); + } + } + } + if let Some(border) = &style.border { + if let Some(c) = &border.color { + check_color(c, "border.color", path, errors); + } + for (name, side) in [ + ("top", &border.top), + ("right", &border.right), + ("bottom", &border.bottom), + ("left", &border.left), + ] { + if let Some(side) = side { + if let Some(c) = &side.color { + check_color(c, &format!("border.{name}.color"), path, errors); + } + } + } + } + if let Some(gb) = &style.gradient_border { + for (i, s) in gb.colors.iter().enumerate() { + check_color_str(s, &format!("gradient-border.colors[{i}]"), path, errors); + } + } +} + +fn check_background_colors(bg: &Background, path: &str, errors: &mut Vec) { + match bg { + Background::Color(c) => check_color(c, "background", path, errors), + Background::Single(layer) => check_background_layer_colors(layer, path, errors), + Background::Layers(layers) => { + for layer in layers { + check_background_layer_colors(layer, path, errors); + } + } + } +} + +fn check_background_layer_colors(layer: &BackgroundLayer, path: &str, errors: &mut Vec) { + match layer { + BackgroundLayer::Color { color } => check_color(color, "background", path, errors), + BackgroundLayer::LinearGradient { stops, .. } + | BackgroundLayer::RadialGradient { stops, .. } + | BackgroundLayer::ConicGradient { stops, .. } => { + for (i, stop) in stops.iter().enumerate() { + check_color( + &stop.color, + &format!("background gradient stop[{i}]"), + path, + errors, + ); + } + } + BackgroundLayer::Image { .. } => {} + } +} + +fn check_color(color: &Color, label: &str, path: &str, errors: &mut Vec) { + if let Color::String(s) = color { + check_color_str(s, label, path, errors); + } +} + +fn check_color_str(s: &str, label: &str, path: &str, errors: &mut Vec) { + if rustmotion::engine::renderer::parse_css_color(s).is_none() { + errors.push(format!( + "{path}: {label} '{s}' is not a recognized CSS color (expected hex #rgb/#rrggbb[aa], \ + rgb()/rgba(...), hsl()/hsla(...), or a CSS named color) — it would render as opaque \ + magenta instead of {label}", + )); + } +} + /// The `time_scale` declared on a container component, if any. fn container_time_scale(component: &Component) -> Option { match component { @@ -451,3 +568,110 @@ mod style_warning_tests { assert!(errors.is_empty(), "unexpected errors: {errors:?}"); } } + +/// C2 completion (issue #110 / #102): an unresolved colour must fail +/// validation, not just render as opaque magenta with a stderr warning. +#[cfg(test)] +mod color_validation_tests { + use super::*; + + #[test] + fn unresolvable_text_color_is_a_blocking_error() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { "color": "not-a-real-color" } + })) + .unwrap(); + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + validate_children(&[child], "test", 4.0, &mut errors, &mut warnings); + assert!( + errors + .iter() + .any(|e| e.contains("not-a-real-color") && e.contains("color")), + "expected an unresolved-color error: {errors:?}" + ); + } + + #[test] + fn valid_colors_of_every_recognized_form_do_not_error() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { + "color": "#fff", + "background": "rgba(10, 20, 30, 0.5)", + "box-shadow": [{ "offset-x": 0, "offset-y": 2, "color": "hsl(200, 50%, 50%)" }], + "text-shadow": [{ "offset-x": 0, "offset-y": 1, "color": "cornflowerblue" }], + "border": { "color": "rebeccapurple" } + } + })) + .unwrap(); + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + validate_children(&[child], "test", 4.0, &mut errors, &mut warnings); + assert!(errors.is_empty(), "unexpected errors: {errors:?}"); + } + + #[test] + fn unresolvable_background_gradient_stop_color_is_reported() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "card", + "style": { + "background": { + "kind": "linear-gradient", + "angle": 45, + "stops": [ + { "color": "#111111", "offset": 0.0 }, + { "color": "totally-bogus", "offset": 1.0 } + ] + } + } + })) + .unwrap(); + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + validate_children(&[child], "test", 4.0, &mut errors, &mut warnings); + assert!( + errors.iter().any(|e| e.contains("totally-bogus")), + "expected the bad gradient-stop color to be reported: {errors:?}" + ); + } + + #[test] + fn unresolvable_border_color_is_reported_with_side() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "card", + "style": { + "border": { "top": { "color": "bogus-border-color" } } + } + })) + .unwrap(); + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + validate_children(&[child], "test", 4.0, &mut errors, &mut warnings); + assert!( + errors + .iter() + .any(|e| e.contains("bogus-border-color") && e.contains("border.top")), + "expected a border.top-labelled error: {errors:?}" + ); + } + + #[test] + fn rgba_object_color_form_never_errors() { + // Color::Rgba{r,g,b,a} is always valid by construction — only + // Color::String can fail to parse. + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { "color": { "r": 10, "g": 20, "b": 30, "a": 0.5 } } + })) + .unwrap(); + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + validate_children(&[child], "test", 4.0, &mut errors, &mut warnings); + assert!(errors.is_empty(), "unexpected errors: {errors:?}"); + } +} diff --git a/crates/rustmotion-cli/src/commands/validation.rs b/crates/rustmotion-cli/src/commands/validation.rs index db87aab..6477447 100644 --- a/crates/rustmotion-cli/src/commands/validation.rs +++ b/crates/rustmotion-cli/src/commands/validation.rs @@ -18,7 +18,9 @@ use rustmotion::variables; use std::collections::HashMap; use std::path::{Path, PathBuf}; -use super::geometry::{validate_geometry, validate_geometry_animated, GeometryViolation}; +use super::geometry::{ + check_legibility, validate_geometry, validate_geometry_animated, GeometryViolation, +}; use super::validate_schema::validate_scenario; /// Source of the scenario JSON to validate. @@ -54,14 +56,31 @@ pub struct ValidationReport { } impl ValidationReport { + /// True when there is nothing at all to show the author — no blocking + /// issue *and* no advisory one. Used to decide whether `render`'s + /// implicit validation pass prints anything; `warnings`/`attr_warnings` + /// are included so a non-blocking advisory (e.g. a legibility warning, + /// or an attr_warning before it is known to block) is never silently + /// dropped just because nothing else escalated it to an error. pub fn is_clean(&self) -> bool { self.schema_errors.is_empty() && self.geom_violations.is_empty() && self.unresolved_vars.is_empty() + && self.warnings.is_empty() + && self.attr_warnings.is_empty() } /// Whether the report contains any issue that should block rendering. /// In `lenient` mode geometry violations are downgraded to warnings. + /// + /// Unknown component attributes block **unconditionally** — M5 (issue + /// #110 / #102, "decided at kickoff"), not gated by `lenient` (that + /// flag's documented scope is geometry violations) and no longer + /// opt-in via `--strict-attrs`. In practice they already arrive folded + /// into `schema_errors` (`run_checks` does this unconditionally now, so + /// every caller blocks on them without extra wiring); the direct + /// `attr_warnings` check below is defence in depth for a + /// `ValidationReport` assembled some other way. pub fn is_blocking(&self, lenient: bool) -> bool { if !self.schema_errors.is_empty() { return true; @@ -69,6 +88,9 @@ impl ValidationReport { if !self.unresolved_vars.is_empty() { return true; } + if !self.attr_warnings.is_empty() { + return true; + } if !lenient && !self.geom_violations.is_empty() { return true; } @@ -77,14 +99,24 @@ impl ValidationReport { pub fn to_error(&self) -> RustmotionError { RustmotionError::ValidationFailed { - schema_errors: self.schema_errors.len(), + // `attr_warnings` is normally already empty here (`run_checks` + // folds it into `schema_errors` unconditionally — see there); + // `+ self.attr_warnings.len()` is defence in depth so this count + // stays honest even for a `ValidationReport` assembled another + // way. `RustmotionError::ValidationFailed` has no dedicated + // attr-warnings field of its own (out of this workstream's + // scope to add). + schema_errors: self.schema_errors.len() + self.attr_warnings.len(), geometry_violations: self.geom_violations.len(), unresolved_vars: self.unresolved_vars.len(), } } - /// `--strict-attrs`: turn unknown-attribute warnings into blocking schema - /// errors. + /// `--strict-attrs` / CLI compat only: by the time a `ValidationReport` + /// from `run_checks` reaches this call, `attr_warnings` is already + /// empty (folded into `schema_errors` there — see M5, issue #110), so + /// this is a no-op in practice. Kept so `--report` JSON output and any + /// hand-assembled `ValidationReport` still behave as documented. pub fn promote_attr_warnings(&mut self) { self.schema_errors.append(&mut self.attr_warnings); } @@ -168,9 +200,24 @@ pub fn run_checks(loaded: &LoadedScenario, strict_anim: bool) -> ValidationRepor } let (mut schema_errors, mut warnings) = validate_scenario(&loaded.scenario); warnings.extend(warn_misplaced_animation(&loaded.raw)); - let (attr_errors, attr_warnings) = + // M4 (issue #110 / #102): legibility floor — always advisory, never + // blocking (see `check_legibility`'s doc comment for the threshold + // justification). + warnings.extend(check_legibility(&loaded.scenario)); + let (attr_errors, mut attr_warnings) = super::validate_attrs::check_component_attrs(&loaded.scenario); schema_errors.extend(attr_errors); + // M5 (issue #110 / #102, decided at kickoff): unknown component + // attributes error by default now, not only under `--strict-attrs`. + // Folding them into `schema_errors` right here — rather than leaving + // them in the separate `attr_warnings` bucket for every *caller* of + // `run_checks` to remember to escalate — means `validate`, `render`, + // and `watch` all block on them uniformly with zero extra wiring, and + // none of their existing `!schema_errors.is_empty()` blocking checks + // needed to change. `attr_warnings` is left empty; `--strict-attrs` / + // `promote_attr_warnings` are kept as accepted, fully inert flags for + // CLI-surface stability (see their doc comments). + schema_errors.append(&mut attr_warnings); ValidationReport { schema_errors, geom_violations, @@ -270,8 +317,16 @@ pub fn print_report(report: &ValidationReport, source_label: &str) { for w in &report.warnings { eprintln!("Warning: {}", w); } + // M5 (issue #110 / #102): `report.attr_warnings` is normally already + // empty by the time it reaches here — `run_checks` folds unknown + // component attributes into `schema_errors` unconditionally now, so + // they print below as `Error:` lines, not `Warning:` ones. Printing + // "Warning" right before the process exits non-zero for that exact + // issue was the dishonest-label pattern this workstream exists to + // remove. This loop stays as a fallback for a `ValidationReport` + // assembled another way. for w in &report.attr_warnings { - eprintln!("Warning: {}", w); + eprintln!("Error: {}", w); } for name in &report.unresolved_vars { eprintln!( @@ -423,3 +478,18 @@ mod font_loading_wiring_tests { ); } } + +/// Notice printed when `--strict-attrs` is passed. +/// +/// Unknown component attributes became blocking errors by default, which left +/// this flag with nothing to promote. A flag that silently does nothing is the +/// same failure mode this validator exists to catch — an accepted input with no +/// observable effect — so it announces itself instead of staying mute. It is +/// still accepted so existing scripts and CI pipelines keep working. +pub fn warn_strict_attrs_is_now_default() { + eprintln!( + "Notice: --strict-attrs is deprecated and does nothing. Unknown \ + component attributes have been errors by default since the attribute \ + checker was hardened; you can drop the flag." + ); +} diff --git a/crates/rustmotion-cli/src/lib.rs b/crates/rustmotion-cli/src/lib.rs index 723833d..013f489 100644 --- a/crates/rustmotion-cli/src/lib.rs +++ b/crates/rustmotion-cli/src/lib.rs @@ -94,6 +94,12 @@ enum Commands { #[arg(long)] strict_anim: bool, + /// Deprecated. Unknown component attributes are errors by default, so + /// this flag no longer changes anything; it is accepted so existing + /// scripts keep working, and prints a notice when used. + #[arg(long)] + strict_attrs: bool, + /// Load variable overrides from a JSON object file (e.g. {"color":"#f00"}). /// Keys must match variables declared in the scenario's `config` block. #[arg(long, value_name = "FILE")] @@ -193,7 +199,9 @@ enum Commands { #[arg(long)] strict_anim: bool, - /// Treat unknown component attributes as errors instead of warnings. + /// Deprecated. Unknown component attributes are errors by default, so + /// this flag no longer changes anything; it is accepted so existing + /// scripts keep working, and prints a notice when used. #[arg(long)] strict_attrs: bool, @@ -447,6 +455,7 @@ pub fn run() -> Result<()> { no_validate, lenient, strict_anim, + strict_attrs, props, var, } => { @@ -476,6 +485,7 @@ pub fn run() -> Result<()> { no_validate, lenient, strict_anim, + strict_attrs, ) } else { let source = match (file.as_ref(), json.as_deref()) { @@ -492,7 +502,11 @@ pub fn run() -> Result<()> { } if !no_validate { - let report = commands::validation::run_checks(&loaded, strict_anim); + let mut report = commands::validation::run_checks(&loaded, strict_anim); + if strict_attrs { + commands::validation::warn_strict_attrs_is_now_default(); + report.promote_attr_warnings(); + } if !report.is_clean() { let label = loaded .source_path diff --git a/crates/rustmotion-core/src/schema/scenario.rs b/crates/rustmotion-core/src/schema/scenario.rs index 01ae27c..64f3813 100644 --- a/crates/rustmotion-core/src/schema/scenario.rs +++ b/crates/rustmotion-core/src/schema/scenario.rs @@ -33,6 +33,7 @@ pub enum VariableType { } #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct Scenario { #[serde(default = "default_version")] pub version: String, @@ -41,7 +42,7 @@ pub struct Scenario { pub audio: Vec, #[serde(default)] pub fonts: Vec, - #[serde(default)] + #[serde(default, deserialize_with = "deserialize_scene_entries")] pub scenes: Vec, /// Composition: a sequence of views (slide or world). Mutually exclusive with top-level `scenes`. #[serde(default)] @@ -119,10 +120,11 @@ fn default_camera_pan_duration() -> f64 { } #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct View { #[serde(rename = "type", default = "default_view_type")] pub view_type: ViewType, - #[serde(default)] + #[serde(default, deserialize_with = "deserialize_scene_entries")] pub scenes: Vec, /// Transition entering this view (between views). #[serde(default)] @@ -181,6 +183,13 @@ pub struct ResolvedView { } /// An entry in the `scenes` array: either a concrete scene or an include directive. +/// +/// `#[serde(untagged)]` is kept so [`schemars`] still emits the correct +/// (flat, non-wrapped) JSON Schema for this enum, and so a direct +/// `SceneEntry::deserialize` call elsewhere keeps working. It is **not** +/// how `Scenario.scenes` / `View.scenes` actually deserialize an entry from +/// JSON, though: those two fields use [`deserialize_scene_entries`] instead +/// (see its doc comment for why — M6, issue #110). #[derive(Debug, Serialize, Deserialize, JsonSchema)] #[serde(untagged)] #[allow(clippy::large_enum_variant)] // untagged serde enum; boxing Scene would break all match arms @@ -191,8 +200,54 @@ pub enum SceneEntry { Include(IncludeDirective), } +/// Deserializer for `Scenario.scenes` / `View.scenes`, used in place of +/// `SceneEntry`'s derived `#[serde(untagged)]` deserialization (M6, issue +/// #110 / #102). +/// +/// `#[serde(untagged)]` deserializes by trying each variant in declaration +/// order and keeping the first one that succeeds; when *all* variants fail +/// (e.g. a scene missing its required `duration`, or a `transition.type` +/// typo three levels down) serde discards every per-variant error and +/// reports only `data did not match any variant of untagged enum +/// SceneEntry` — no scene index, no field name. The wave-2 audit called +/// this the worst diagnostic in the product. +/// +/// The two variants are unambiguous by shape — `IncludeDirective`'s only +/// required field is `include`; a `Scene` never has that key — so this +/// classifies each entry explicitly instead of trying-and-discarding, then +/// deserializes it as its concrete type and reports *that* type's real +/// error, prefixed with the entry's index in the `scenes` array. +/// +/// Note on precision: this still deserializes from an already-parsed +/// [`serde_json::Value`] (as the untagged path did too), which has no +/// source line/column to report — the fix here is naming the scene index +/// and field, not a source position that doesn't exist at this layer. +fn deserialize_scene_entries<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + use serde::de::Error as _; + + let raw: Vec = Vec::deserialize(deserializer)?; + let mut out = Vec::with_capacity(raw.len()); + for (i, entry) in raw.into_iter().enumerate() { + let is_include = entry.get("include").is_some(); + if is_include { + let directive: IncludeDirective = serde_json::from_value(entry) + .map_err(|e| D::Error::custom(format!("scenes[{i}] (include directive): {e}")))?; + out.push(SceneEntry::Include(directive)); + } else { + let scene: Scene = serde_json::from_value(entry) + .map_err(|e| D::Error::custom(format!("scenes[{i}]: {e}")))?; + out.push(SceneEntry::Scene(scene)); + } + } + Ok(out) +} + /// Directive to inject scenes from an external scenario file. #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct IncludeDirective { /// Path (relative to parent file) or URL (http/https) to a scenario JSON file. pub include: String, @@ -259,6 +314,7 @@ fn default_volume() -> f32 { } #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct VideoConfig { pub width: u32, pub height: u32, @@ -281,6 +337,7 @@ pub struct WorldPosition { } #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct Scene { pub duration: f64, /// Unified background: color string, animated entry (with optional $ref), or array. @@ -324,6 +381,7 @@ pub struct Scene { /// Virtual camera for pan/zoom/rotation effects at the scene level. #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct Camera { /// Camera center X offset from scene center (pixels). Default: 0. #[serde(default)] @@ -349,6 +407,7 @@ pub struct Camera { /// Focal point of the camera in frame pixels. #[derive(Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct CameraOrigin { #[serde(default)] pub x: f32, @@ -358,6 +417,7 @@ pub struct CameraOrigin { /// A keyframe for a camera property. #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct CameraKeyframe { /// The camera property to animate: "x", "y", "zoom", "rotation", /// "origin.x", "origin.y" (dotted form, matching the component keyframe @@ -372,6 +432,7 @@ pub struct CameraKeyframe { /// A single time-value point in a camera keyframe. #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct CameraKeyframePoint { /// Time in seconds (relative to scene start). pub time: f64, @@ -466,6 +527,7 @@ fn default_blur_max_radius() -> f32 { /// Scene-level flex layout configuration #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct SceneLayout { #[serde(default)] pub direction: Option, @@ -480,6 +542,7 @@ pub struct SceneLayout { } #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct Transition { #[serde(rename = "type")] pub transition_type: TransitionType, @@ -612,3 +675,214 @@ mod annotation_tests { assert_eq!(s.annotations[0].frame, None); } } + +/// M6 (issue #110 / #102): `SceneEntry` used to be a bare `#[serde(untagged)]` +/// enum, so a bad `scenes[]` entry collapsed to "data did not match any +/// variant of untagged enum SceneEntry" — no scene index, no field name. +/// `deserialize_scene_entries` replaces the auto-try-each-variant behaviour +/// with an explicit classify-then-deserialize pass that keeps the real +/// per-scene error and prefixes it with the entry's index. +#[cfg(test)] +mod scene_entry_error_tests { + use super::*; + + #[test] + fn missing_duration_names_the_scene_index_and_field_not_the_untagged_message() { + let json = r#"{ + "video": { "width": 100, "height": 100 }, + "scenes": [ + { "duration": 1.0, "children": [] }, + { "children": [] } + ] + }"#; + let err = serde_json::from_str::(json).expect_err("missing duration must fail"); + let msg = err.to_string(); + assert!( + !msg.contains("did not match any variant of untagged enum"), + "must not regress to the opaque untagged message: {msg}" + ); + assert!( + msg.contains("scenes[1]"), + "must name the offending scene index: {msg}" + ); + assert!( + msg.contains("duration"), + "must name the missing field: {msg}" + ); + } + + #[test] + fn misspelled_transition_type_names_itself() { + let json = r#"{ + "video": { "width": 100, "height": 100 }, + "scenes": [ + { + "duration": 1.0, + "children": [], + "transition": { "type": "wip_left" } + } + ] + }"#; + let err = + serde_json::from_str::(json).expect_err("bad transition type must fail"); + let msg = err.to_string(); + assert!( + !msg.contains("did not match any variant of untagged enum"), + "must not regress to the opaque untagged message: {msg}" + ); + assert!( + msg.contains("scenes[0]"), + "must name the offending scene index: {msg}" + ); + assert!( + msg.contains("wip_left"), + "must echo the bad value so the author can spot the typo: {msg}" + ); + } + + #[test] + fn include_directive_still_works() { + let json = r#"{ + "video": { "width": 100, "height": 100 }, + "scenes": [ + { "include": "does/not/matter.json" } + ] + }"#; + let s: Scenario = serde_json::from_str(json).expect("include entry must parse"); + assert!(matches!(s.scenes[0], SceneEntry::Include(_))); + } + + #[test] + fn broken_include_directive_names_itself() { + // `scenes` on IncludeDirective must be an array of indices — a typo'd + // shape should not silently be mistaken for a Scene. + let json = r#"{ + "video": { "width": 100, "height": 100 }, + "scenes": [ + { "include": "x.json", "scenes": "not-an-array" } + ] + }"#; + let err = serde_json::from_str::(json).expect_err("must fail"); + let msg = err.to_string(); + assert!(msg.contains("scenes[0]"), "got: {msg}"); + assert!(msg.contains("include directive"), "got: {msg}"); + } + + #[test] + fn view_scenes_field_uses_the_same_precise_errors() { + let json = r#"{ + "video": { "width": 100, "height": 100 }, + "composition": [ + { "type": "slide", "scenes": [ { "children": [] } ] } + ] + }"#; + let err = serde_json::from_str::(json).expect_err("must fail"); + let msg = err.to_string(); + assert!( + !msg.contains("did not match any variant of untagged enum"), + "got: {msg}" + ); + assert!(msg.contains("scenes[0]"), "got: {msg}"); + assert!(msg.contains("duration"), "got: {msg}"); + } +} + +/// M5 (issue #110 / #102): the unknown-attribute checker only ever inspected +/// the top level of each *component*; a typo in `Scenario`/`Scene`/`View`/ +/// `VideoConfig`/`SceneLayout`/`Transition`/`Camera` (e.g. `durration` on a +/// scene, `framerate` on `video`) passed silently because nothing checked +/// those structs at all. `deny_unknown_fields` closes that gap directly at +/// parse time, for every one of them. +#[cfg(test)] +mod strict_schema_tests { + use super::*; + + #[test] + fn misspelled_scene_field_is_rejected() { + let json = r#"{ + "video": { "width": 100, "height": 100 }, + "scenes": [ { "durration": 3.0, "children": [] } ] + }"#; + let err = serde_json::from_str::(json).expect_err("must fail"); + assert!(err.to_string().contains("durration"), "got: {err}"); + } + + #[test] + fn misspelled_video_field_is_rejected() { + let json = r#"{ + "video": { "width": 100, "height": 100, "framerate": 30 }, + "scenes": [ { "duration": 1.0, "children": [] } ] + }"#; + let err = serde_json::from_str::(json).expect_err("must fail"); + assert!(err.to_string().contains("framerate"), "got: {err}"); + } + + #[test] + fn misspelled_top_level_scenario_field_is_rejected() { + let json = r#"{ + "video": { "width": 100, "height": 100 }, + "scenes": [ { "duration": 1.0, "children": [] } ], + "titel": "typo" + }"#; + let err = serde_json::from_str::(json).expect_err("must fail"); + assert!(err.to_string().contains("titel"), "got: {err}"); + } + + #[test] + fn misspelled_camera_field_is_rejected() { + let json = r#"{ + "video": { "width": 100, "height": 100 }, + "scenes": [ { + "duration": 1.0, + "children": [], + "camera": { "zooom": 1.5 } + } ] + }"#; + let err = serde_json::from_str::(json).expect_err("must fail"); + assert!(err.to_string().contains("zooom"), "got: {err}"); + } + + #[test] + fn misspelled_scene_layout_field_is_rejected() { + let json = r#"{ + "video": { "width": 100, "height": 100 }, + "scenes": [ { + "duration": 1.0, + "children": [], + "layout": { "gapp": 10 } + } ] + }"#; + let err = serde_json::from_str::(json).expect_err("must fail"); + assert!(err.to_string().contains("gapp"), "got: {err}"); + } + + #[test] + fn misspelled_view_field_is_rejected() { + let json = r#"{ + "video": { "width": 100, "height": 100 }, + "composition": [ { "typ": "slide", "scenes": [] } ] + }"#; + let err = serde_json::from_str::(json).expect_err("must fail"); + assert!(err.to_string().contains("typ"), "got: {err}"); + } + + #[test] + fn valid_scenario_with_every_covered_struct_still_parses() { + // Regression guard: deny_unknown_fields must not reject any + // currently-valid field across Scenario/Scene/View/VideoConfig/ + // SceneLayout/Transition/Camera. + let json = r##"{ + "version": "1.0", + "video": { "width": 100, "height": 100, "fps": 30, "background": "#000000" }, + "scenes": [ { + "duration": 1.0, + "children": [], + "layout": { "direction": "column", "gap": 10, "align_items": "center", "justify_content": "center", "padding": 5 }, + "transition": { "type": "fade", "duration": 0.5, "easing": "ease_in_out" }, + "camera": { "x": 0, "y": 0, "zoom": 1.0, "rotation": 0, "origin": { "x": 1, "y": 2 }, "keyframes": [ { "property": "zoom", "values": [ { "time": 0.0, "value": 1.0 } ], "easing": "linear" } ] } + } ] + }"##; + let s: Scenario = serde_json::from_str(json).expect("valid scenario must still parse"); + assert_eq!(s.scenes.len(), 1); + } +}