diff --git a/crates/rustmotion-cli/src/commands/geometry.rs b/crates/rustmotion-cli/src/commands/geometry.rs index 9978e66..a70cc5e 100644 --- a/crates/rustmotion-cli/src/commands/geometry.rs +++ b/crates/rustmotion-cli/src/commands/geometry.rs @@ -13,8 +13,6 @@ //! * detect wrapping content whose natural size exceeds its own resolved //! box (`text`/`gradient_text`/`caption`/`rich_text`/`table` — #128 //! item 1: originally `text`-only) -//! * detect a component's own box extending past the nearest ancestor -//! `card` that contains it, for every component type (#128 item 2) //! * detect terminal/codeblock content that overflows their box when //! `auto_scroll: false` //! * exempt `marquee` and `cursor` (designed to bleed) @@ -22,31 +20,54 @@ //! `auto` ancestor as a viewport overflow (H4) — the ancestor's own bbox //! is still checked independently, at its own level //! +//! Deliberately NOT in scope: a component's box vs its nearest ancestor +//! `card`'s box, independent of the viewport (#128 item 2, briefly added +//! then retired — round 4 audit, constat 7). CLAUDE.md and +//! `geometry-safety.md` both promise the validator only complains about +//! content escaping the *viewport*, never about escaping a non-clipping +//! (`overflow: visible`, the default) container — "a badge sticking out of +//! a card is legal". A box-vs-card check can only ever fire in exactly that +//! legal case (a clipping card already suppresses it the same way it +//! suppresses every other check here, so there is nothing left for it to +//! report when the card *does* clip either) — see the retired call site's +//! comment in `walk` for the full reasoning. +//! //! Animation handling is layered: by default we only check the resting -//! (untransformed) layout. With `--strict-anim`, we additionally sample -//! frames — proportionally to scene duration — and reapply the *real* -//! renderer's animation resolution (`effective_effects` + -//! `resolve_props_for_effects`) plus the paint pass's start_at/end_at -//! visibility window, rather than a hand-rolled fork of that logic (H6). +//! (untransformed) layout, built once with `anim: None`. With +//! `--strict-anim`, we additionally sample frames — proportionally to scene +//! duration (H6; round 4 audit, constat 8: dense enough to stay near a +//! promised 8/s up to 60s scenes) — and at EACH sample, rebuild the box +//! tree and rerun layout with a real `BuildAnimationCtx` (round 4 audit, +//! constats 2 & 9): the same engine path `render_with_new_pipeline_iter` +//! calls once per rendered frame, rather than building once at rest and +//! hand-deriving only translate/scale afterwards. This is what makes +//! `timeline` style states, audio-reactive transforms, and animated +//! rotation all visible to `--strict-anim`, not just translate/scale — see +//! `validate_geometry_animated`'s doc comment. The paint pass's +//! start_at/end_at visibility window (resolved at box-tree build time, +//! independent of `anim`) is honoured the same way in both modes. //! //! This walker runs the new CSS-engine pipeline (taffy + cosmic-text) so the //! geometry it checks matches what the renderer will actually paint. use std::collections::HashSet; -use rustmotion::components::box_builder::{build_scene_from_refs, effective_effects}; +use rustmotion::components::box_builder::{ + build_scene_from_refs, effective_effects, BuildAnimationCtx, +}; use rustmotion::components::intrinsic::{ - CaptionIntrinsic, GradientTextIntrinsic, RichTextIntrinsic, TableIntrinsic, TextIntrinsic, + CaptionIntrinsic, CodeblockIntrinsic, GradientTextIntrinsic, RichTextIntrinsic, TableIntrinsic, + TerminalIntrinsic, TextIntrinsic, }; use rustmotion::components::{ChildComponent, Component}; -use rustmotion::core::css::style::{CssStyle, TransformFn, WhiteSpace}; +use rustmotion::core::css::style::{CssStyle, TransformFn, TransformOrigin, WhiteSpace}; use rustmotion::core::css::taffy_bridge::ConversionContext; -use rustmotion::core::css::units::LengthContext; -use rustmotion::core::engine::box_tree::{AvailableSpace, BoxNode, IntrinsicMeasure}; +use rustmotion::core::css::units::{parse_origin_component, LengthContext, ParsedLength}; +use rustmotion::core::engine::box_tree::{AvailableSpace, BoxKind, BoxNode, IntrinsicMeasure}; use rustmotion::core::engine::layout_pass::{run_layout, BoxLayout, LayoutResult}; use rustmotion::engine::animator::{resolve_props_for_effects, AnimatedProperties}; use rustmotion::engine::render; -use rustmotion::schema::{Camera, ResolvedScenario, Scene}; +use rustmotion::schema::{Camera, ResolvedScenario, Scene, ViewType}; use serde::Serialize; /// One detected layout violation. @@ -94,13 +115,17 @@ pub enum ViolationKind { /// painters never clip themselves, so this paints outside its box /// regardless of where that box sits relative to the viewport. ContentOverflowsBox, - /// A component's own (resolved, post-layout) box extends past the - /// nearest ancestor `card` that contains it — distinct from - /// [`ViolationKind::ViewportOverflow`] (box vs frame) and - /// [`ViolationKind::ContentOverflowsBox`] (content vs its OWN box): this - /// is box vs the panel that's supposed to contain it (#128 item 2). A - /// panel-based style relies on every graphic element staying inside its - /// card, so this is the blind spot that matters most for it. + /// Retired (round 4 audit, constat 7) — no longer constructed by + /// `walk`/`walk_anim`. Was: a component's own (resolved, post-layout) + /// box extending past its nearest ancestor `card`'s box (#128 item 2), + /// unconditionally on any non-clipping card — exactly the "badge + /// sticking out of a card" pattern CLAUDE.md and `geometry-safety.md` + /// document as legal (`overflow: visible`, the default). Kept as a + /// variant — not renamed/removed — for `--fix`'s match arm and + /// `--report` JSON schema stability (frozen violation-kind contract); + /// see the module doc comment's "Deliberately NOT in scope" note for + /// the full reasoning. + #[allow(dead_code)] // never constructed by design — see doc comment above ContentOverflowsCard, /// Animated transform (scale/translate/wiggle/orbit) pushes the bbox out /// of the viewport at some sampled time. Only emitted with `--strict-anim`. @@ -120,13 +145,34 @@ pub fn validate_geometry(scenario: &ResolvedScenario) -> Vec let mut violations = Vec::new(); for (vi, view) in scenario.views.iter().enumerate() { for (si, scene) in view.scenes.iter().enumerate() { + // Round 4 audit, constat 4: a `world` scene's decorative + // children (particles) are never fed into the flex box tree at + // render time either — `render_world_frame_scaled` paints them + // full-viewport via `paint_decorative_fullscreen`, filtered out + // of `render_with_new_pipeline_iter`'s children entirely (see + // `scene_children.iter().filter(|c| !c.is_decorative())` there). + // Leaving them in here would let them occupy a flex slot that + // pushes sibling positions around in a way that never happens + // at render, so they're dropped from the walk the same way for + // `world` views only — `slide` views never filtered them (a + // particle IS flex-flowed there), so scoping this to `world` + // keeps slide-view behaviour byte-identical. + let is_world = matches!(view.view_type, ViewType::World); let indexed = deserialize_children_indexed(scene); + let indexed: Vec<(usize, ChildComponent)> = if is_world { + indexed + .into_iter() + .filter(|(_, c)| !c.is_decorative()) + .collect() + } else { + indexed + }; let raw_indices: Vec = indexed.iter().map(|(i, _)| *i).collect(); let children: Vec = indexed.into_iter().map(|(_, c)| c).collect(); let viewport = (scenario.video.width, scenario.video.height); let viewport_f = (viewport.0 as f32, viewport.1 as f32); - let root_css = render::root_style(scene.layout.as_ref()); + let root_css = render::root_style(scene.layout.as_ref(), view.view_type.clone()); let built = build_scene_from_refs(children.iter(), viewport_f, root_css, None); let layouts = run_layout(&built.root, viewport_f, &ConversionContext::default()); @@ -151,9 +197,6 @@ pub fn validate_geometry(scenario: &ResolvedScenario) -> Vec /*parent_clips=*/ false, camera, - // No ancestor card at the top of a scene. - /*nearest_card=*/ - None, &mut violations, ); } @@ -213,14 +256,6 @@ fn walk( path_indices: Option<&[usize]>, parent_clips: bool, camera: Option<&Camera>, - // Bbox of the nearest ancestor `card`, in the same raw layout space as - // `raw_bbox` below — `None` when no card encloses this level yet. Only - // `Component::Card` updates this for its own children (see the - // recursion below); every other container (`flex`/`grid`/`positioned`/ - // `container`) is layout-only per CLAUDE.md and passes it through - // unchanged, so the check always compares against the panel actually - // responsible for containing the content, not an incidental layout box. - nearest_card: Option, out: &mut Vec, ) { let viewport_f = (viewport.0 as f32, viewport.1 as f32); @@ -241,15 +276,30 @@ fn walk( } check_viewport(&child.component, &child_path, &vbbox, viewport, vi, si, out); } - check_unwrappable_text( - &child.component, - &child_path, - &raw_bbox, - viewport, - vi, - si, - out, - ); + // Round 4 audit, constat 3: this natural-width-vs-own-box check + // is content vs its OWN box, exactly the same category as + // `check_content_overflows_box` below (just for the nowrap/ + // single-line case instead of the wrapped one) — so it gets the + // identical double exemption: an ancestor that clips + // (`parent_clips`) genuinely crops the overflowing line before + // it can paint past the box, and a node that clips ITSELF + // (`container_clips`) does the same to its own content. Before + // this fix it ran unconditionally, contradicting + // geometry-safety.md's documented promise ("A node is also + // exempt when it clips itself, or when any ancestor clips it") + // and `--fix` would then strip a legitimate `white-space: + // nowrap` from a component that was never actually broken. + if !parent_clips && !container_clips(&child.component) { + check_unwrappable_text( + &child.component, + &child_path, + &raw_bbox, + viewport, + vi, + si, + out, + ); + } check_auto_scroll( &child.component, &child_path, @@ -275,35 +325,34 @@ fn walk( out, ); } - // #128 item 2: box vs the containing card. Suppressed under a - // clipping ancestor exactly like check_viewport (content clipped - // by the card itself, or by something between here and the - // card, genuinely never paints past it). Deliberately NOT - // suppressed by `bleed` — bleed is an assertion about crossing - // the *frame* edge on purpose, not about disowning whatever a - // component paints inside its own panel (see `bleeds`'s doc - // comment; the same reasoning `check_content_overflows_box` - // already applies). - if !parent_clips { - check_overflows_card( - &child.component, - &child_path, - &raw_bbox, - nearest_card, - viewport, - vi, - si, - out, - ); - } + // #128 item 2 (`ContentOverflowsCard`) used to live here: a + // component's box vs its nearest ancestor `card`'s box, + // unconditionally (as long as nothing clipped in between). + // Round 4 audit, constat 7: that check is structurally + // incompatible with the validator's own documented contract. + // Its own suppression (`!parent_clips`, mirroring every other + // check here) is reachable if and only if the nearest card AND + // everything between it and this node is non-clipping — i.e. it + // could only ever fire in exactly the case CLAUDE.md ("le + // validateur ne se plaint que si le contenu sort du viewport, + // pas d'un parent visible") and geometry-safety.md:34/77 ("a + // badge sticking out of a card is legal" when the card's + // `overflow` is `visible`, the default — "no change needed") + // both promise is legal and must NOT be reported. Whenever the + // card *does* clip (`overflow: hidden`), `parent_clips` already + // suppresses this whole block, so the content is invisible + // anyway and there is nothing left to warn about either way. + // There is no configuration where firing is both reachable and + // consistent with the documented contract, so it is retired + // here rather than patched with a redundant escape hatch — + // `check_overflows_card` (and the `nearest_card` tracking that + // fed it) is deleted; the `ViolationKind::ContentOverflowsCard` + // variant itself is kept, unconstructed, for `--fix`'s match arm + // and `--report` JSON schema stability (frozen violation-kind + // contract — see that variant's doc comment). } if let Some(grandchildren) = container_children(&child.component) { - let child_nearest_card = if matches!(child.component, Component::Card(_)) { - Some(raw_bbox) - } else { - nearest_card - }; walk( grandchildren, &box_node.children, @@ -315,7 +364,6 @@ fn walk( None, parent_clips || container_clips(&child.component), camera, - child_nearest_card, out, ); } @@ -396,8 +444,10 @@ fn container_clips(c: &Component) -> bool { /// closes the rotation/skew gap) — maps the box's four corners through the /// same ordered transform-function chain the paint pass's 2D fast path /// applies (`canvas.translate/scale/rotate/skew`, called once per function -/// in `style.transform` order, pivoted at the box centre), then takes the -/// AABB of the four transformed corners. This is what makes rotation/skew +/// in `style.transform` order, pivoted at `style.transform-origin` — +/// resolved by [`resolve_transform_origin_2d`], defaulting to the box centre +/// exactly like the paint pass does when it's absent), then takes the AABB +/// of the four transformed corners. This is what makes rotation/skew /// contribute correctly: an exact AABB under rotation needs the four /// corners, not a translate/scale-only shortcut. /// @@ -406,11 +456,9 @@ fn container_clips(c: &Component) -> bool { /// `Matrix3d`) and the general 2D `Matrix` are intentionally still not /// modeled (identity for that function) — an exact AABB there needs /// projecting through the full 3D pipeline `apply_transform` uses for that -/// path, out of scope for this fix. Also still assumes the pivot is the box -/// centre (no `transform-origin` support), matching the prior partial -/// behaviour. Animated transform-producing presets are folded separately in -/// `walk_anim`; this only handles what a component declares directly in -/// `style.transform`. +/// path, out of scope for this fix. Animated transform-producing presets are +/// folded separately in `walk_anim`; this only handles what a component +/// declares directly in `style.transform`. fn apply_static_node_transform(bbox: &BBox, css: &CssStyle, viewport: (f32, f32)) -> BBox { let transform = match css.transform.as_deref() { Some(t) if !t.is_empty() => t, @@ -423,8 +471,7 @@ fn apply_static_node_transform(bbox: &BBox, css: &CssStyle, viewport: (f32, f32) font_size: 16.0, root_font_size: 16.0, }; - let pivot_x = bbox.x + bbox.w / 2.0; - let pivot_y = bbox.y + bbox.h / 2.0; + let (pivot_x, pivot_y) = resolve_transform_origin_2d(css.transform_origin.as_ref(), bbox, &ctx); let corners = [ (bbox.x, bbox.y), (bbox.x + bbox.w, bbox.y), @@ -453,6 +500,62 @@ fn apply_static_node_transform(bbox: &BBox, css: &CssStyle, viewport: (f32, f32) } } +/// Round 4 audit, constat 5: resolve `style.transform-origin` to an absolute +/// viewport-space pivot `(x, y)`, in the same way the paint pass's own +/// `resolve_origin` does (`crates/rustmotion-core/src/engine/paint_pass.rs`) +/// — percentages resolve against the box's own width (x) / height (y), an +/// absent axis defaults to 50%, and an absent `transform-origin` altogether +/// defaults to dead-centre. +/// +/// This mirrors `resolve_origin`'s 2D resolution rather than calling it +/// directly: that function is private to `paint_pass.rs`, which sits outside +/// this workstream's file perimeter (round 4 audit, lot VALIDATION +/// GÉOMÉTRIQUE — geometry.rs/validate.rs/scene.rs only), so it cannot be +/// marked `pub`/re-exported from here without touching a file outside that +/// scope. What's duplicated is only the small resolution *orchestration*; +/// the actual unit-conversion primitives it calls (`parse_origin_component`, +/// `ParsedLength::resolve`) are `pub` in `rustmotion_core::css::units` and +/// are the exact same functions `resolve_origin` itself calls, so the two +/// can only drift on the orchestration shape, not on what a given length +/// string resolves to. Keep this in sync with `resolve_origin` if that +/// function's resolution rules change; the z component is intentionally not +/// resolved (this fold is 2D-only, see this function's caller's doc comment +/// on the 3D exemption). +fn resolve_transform_origin_2d( + origin: Option<&TransformOrigin>, + bbox: &BBox, + ctx: &LengthContext, +) -> (f32, f32) { + let Some(o) = origin else { + return (bbox.x + bbox.w / 2.0, bbox.y + bbox.h / 2.0); + }; + let resolve_axis = |lp: &rustmotion::core::css::units::LengthPercentage, + axis_size: f32, + axis_origin: f32| + -> f32 { + let parsed = match lp { + rustmotion::core::css::units::LengthPercentage::String(s) => { + parse_origin_component(s).unwrap_or(ParsedLength::Percent(50.0)) + } + rustmotion::core::css::units::LengthPercentage::Px(v) => ParsedLength::Px(*v), + }; + let local_ctx = LengthContext { + parent_size: axis_size, + ..*ctx + }; + axis_origin + parsed.resolve(&local_ctx).unwrap_or(axis_size / 2.0) + }; + let ox = + o.x.as_ref() + .map(|lp| resolve_axis(lp, bbox.w, bbox.x)) + .unwrap_or(bbox.x + bbox.w / 2.0); + let oy = + o.y.as_ref() + .map(|lp| resolve_axis(lp, bbox.h, bbox.y)) + .unwrap_or(bbox.y + bbox.h / 2.0); + (ox, oy) +} + /// Apply a `style.transform` function list to a point already expressed /// relative to the pivot, in the same order `apply_transform`'s 2D fast path /// composes them: `canvas.translate/scale/rotate/skew` are called once per @@ -781,67 +884,20 @@ fn check_content_overflows_box( }); } -/// #128 item 2: does this component's own (resolved) box extend past the -/// nearest ancestor `card`'s own box? Complementary to `check_viewport` -/// (box vs frame) and `check_content_overflows_box` (content vs its OWN -/// box) — this is the missing third comparison: box vs the panel it's -/// supposed to live inside. Operates purely in layout space (no camera/ -/// css-transform folding), matching `check_content_overflows_box`'s scope: -/// "does this content structurally fit inside its card as laid out", -/// independent of wherever the camera happens to be pointing at paint time. -fn check_overflows_card( - component: &Component, - path: &str, - bbox: &BBox, - nearest_card: Option, - viewport: (u32, u32), - vi: usize, - si: usize, - out: &mut Vec, -) { - let Some(card) = nearest_card else { - return; - }; - let eps = 0.5; - let card_right = card.x + card.w; - let card_bottom = card.y + card.h; - let right = bbox.x + bbox.w; - let bottom = bbox.y + bbox.h; - let x_over = bbox.x < card.x - eps || right > card_right + eps; - let y_over = bbox.y < card.y - eps || bottom > card_bottom + eps; - if !x_over && !y_over { - return; - } - let axis = match (x_over, y_over) { - (true, true) => Axis::Both, - (true, false) => Axis::X, - (false, true) => Axis::Y, - (false, false) => return, - }; - out.push(GeometryViolation { - view_index: vi, - scene_index: si, - path: path.to_string(), - component: component_kind(component).to_string(), - axis, - kind: ViolationKind::ContentOverflowsCard, - bbox: *bbox, - viewport, - hint: format!( - "{} at [{:.0},{:.0}]→[{:.0},{:.0}] extends past its containing card [{:.0},{:.0}]→[{:.0},{:.0}] — grow the card, shrink/reflow the content, or set overflow: hidden on the card if the bleed is intentional", - component_kind(component), - bbox.x, - bbox.y, - right, - bottom, - card.x, - card.y, - card_right, - card_bottom, - ), - }); -} - +/// Round 4 audit, constat 6: this used to hand-roll codeblock/terminal +/// natural-height formulas with a hardcoded 16+16=32px padding assumption +/// and (for terminal) the CSS `style.line-height` property — neither of +/// which is what actually gets painted. `CodeblockIntrinsic`/ +/// `TerminalIntrinsic` are the exact measurers `component_intrinsic` +/// (`box_builder.rs`) hands to the layout pass for these two components, so +/// calling them here — instead of re-deriving the formula — keeps this +/// check byte-for-byte in sync with `compute_code_dimensions` (codeblock, +/// which DOES read `style.padding_px()`) and `terminal::line_height()` +/// (terminal, which does NOT honour `style.line-height`, always using its +/// own fixed `LINE_HEIGHT`/`FONT_SIZE` ratio). Measuring at +/// `(None, None)`/`MaxContent` yields each component's natural (unbounded) +/// size, exactly like `check_unwrappable_text`/`check_content_overflows_box` +/// already do for the text-family intrinsics. fn check_auto_scroll( component: &Component, path: &str, @@ -851,18 +907,11 @@ fn check_auto_scroll( si: usize, out: &mut Vec, ) { + let max_content = (AvailableSpace::MaxContent, AvailableSpace::MaxContent); match component { Component::Codeblock(cb) if !cb.auto_scroll => { - let font_size = cb.style.font_size_px_or(14.0); - let actual_line_height = cb.style.line_height_for(font_size); - let line_count = cb.code.lines().count().max(1) as f32; - let chrome_h = if cb.chrome.as_ref().is_some_and(|c| c.enabled) { - 36.0 - } else { - 0.0 - }; - let pad = 32.0; // ~16 top + 16 bottom default - let natural_h = chrome_h + pad + line_count * actual_line_height; + let (_, natural_h) = + CodeblockIntrinsic::from_codeblock(cb).measure((None, None), max_content); if natural_h > bbox.h + 0.5 { out.push(GeometryViolation { view_index: vi, @@ -881,11 +930,8 @@ fn check_auto_scroll( } } Component::Terminal(t) if !t.auto_scroll => { - let font_size = t.style.font_size_px_or(16.0); - let actual_line_height = t.style.line_height_for(font_size); - let chrome_h = if t.show_chrome { 36.0 } else { 0.0 }; - let pad = 32.0; - let natural_h = chrome_h + pad + t.lines.len() as f32 * actual_line_height; + let (_, natural_h) = + TerminalIntrinsic::from_terminal(t).measure((None, None), max_content); if natural_h > bbox.h + 0.5 { out.push(GeometryViolation { view_index: vi, @@ -1125,7 +1171,24 @@ fn component_kind(c: &Component) -> &'static str { /// on long, mostly-static scenes). const ANIM_SAMPLES_PER_SECOND: f64 = 8.0; const ANIM_MIN_SAMPLES: usize = 5; -const ANIM_MAX_SAMPLES: usize = 40; +/// Round 4 audit, constat 8: raised from 40 (a ~5s ceiling on the promised +/// 8/s cadence) to 480 - 60s worth of samples at exactly 8/s, the audit's +/// own reference duration ("pas de 0.51s a 20s, 1.0s a 40s, 1.5s a 60s"). +/// Past a 5s scene, the old cap widened the step linearly with duration +/// (0.51s at 20s, 1.0s at 40s, 1.5s at 60s), so a brief transform excursion +/// shorter than that step could land entirely between two samples and never +/// get checked. Cost, measured on the box-tree-rebuild-per-sample walker +/// this cap now drives (constats 2 & 9): a 15-component animated scene at +/// 60s / 480 samples took 309ms wall-clock in a `--release` build +/// (~0.64ms/sample) and 543ms in a debug build (~1.13ms/sample) - see +/// `timing_probe_for_constat_8` (run with `--ignored`) for the harness. +/// `--strict-anim` is opt-in, and `validate`/`render`'s implicit checks +/// don't pass it, so this cost is paid only when explicitly asked for. +/// Scenes longer than 60s still degrade past this cap - CLAUDE.md's own +/// architecture favours many short scenes stitched by transitions/world +/// panning over one very long scene, so a single-scene ceiling at 60s +/// covers the documented common case. +const ANIM_MAX_SAMPLES: usize = 480; /// Sample times (seconds, scene-relative) for `--strict-anim`, spaced evenly /// across `[0, scene_duration]`. Count scales with `scene_duration` (H6) — @@ -1149,25 +1212,48 @@ fn anim_sample_times(scene_duration: f64) -> Vec { /// animation resolution to each widget's bbox, and report viewport /// overflows. Only emits `AnimatedTextOverflow` violations: the /// resting-layout checks live in `validate_geometry`. +/// +/// Round 4 audit, constats 2 & 9: rebuilds the box tree AND reruns layout at +/// EACH sampled time, with a real `BuildAnimationCtx` — exactly the engine +/// path `render_with_new_pipeline_iter` calls once per rendered frame +/// (`build_scene_from_refs` + `run_layout`) — instead of building once at a +/// frozen resting state (`anim: None`) and hand-deriving only +/// translate/scale afterwards in `walk_anim`. This one change fixes two +/// separate blind spots at once, because both are downstream of the SAME +/// `anim: None`: +/// * `build_child` only applies `apply_style_states` (`timeline` steps) +/// and the audio-reactive CSS block at the times a REAL `local_actx` is +/// available — a `timeline` step that changes a box-model property +/// (e.g. `width`) was invisible at every sample (constat 2). +/// * `apply_animated_props` bakes the resolved transform (translate, +/// scale, AND rotation) into `css.transform` — so once the tree is +/// rebuilt with the real time, `apply_static_node_transform` (already +/// used for static `style.transform`, already handling rotation/skew +/// via a four-corner AABB) picks up animated rotation too, with no +/// separate rotation-aware fold needed (constat 9). pub fn validate_geometry_animated(scenario: &ResolvedScenario) -> Vec { let mut violations = Vec::new(); let mut seen: HashSet<(usize, usize, String)> = HashSet::new(); + let fps = scenario.video.fps; for (vi, view) in scenario.views.iter().enumerate() { for (si, scene) in view.scenes.iter().enumerate() { + // Constat 4: same decorative-child filtering as `validate_geometry` + // — see that call site's comment for why. + let is_world = matches!(view.view_type, ViewType::World); let indexed = deserialize_children_indexed(scene); + let indexed: Vec<(usize, ChildComponent)> = if is_world { + indexed + .into_iter() + .filter(|(_, c)| !c.is_decorative()) + .collect() + } else { + indexed + }; let raw_indices: Vec = indexed.iter().map(|(i, _)| *i).collect(); let children: Vec = indexed.into_iter().map(|(_, c)| c).collect(); let viewport = (scenario.video.width, scenario.video.height); let viewport_f = (viewport.0 as f32, viewport.1 as f32); - // Use the resting layout as the base bbox. Animation transforms - // (translate/scale/wiggle/orbit) are applied analytically per - // sample — they don't reflow taffy, matching how the paint pass - // applies them after layout. - let root_css = render::root_style(scene.layout.as_ref()); - let built = build_scene_from_refs(children.iter(), viewport_f, root_css, None); - let layouts = run_layout(&built.root, viewport_f, &ConversionContext::default()); - let camera = scene .camera .as_ref() @@ -1177,6 +1263,15 @@ pub fn validate_geometry_animated(scenario: &ResolvedScenario) -> Vec Vec impl Iterator { + boxes + .iter() + .filter(|b| !matches!(b.kind, BoxKind::Ghost(_))) +} + #[allow(clippy::too_many_arguments)] fn walk_anim( children: &[ChildComponent], @@ -1221,7 +1336,7 @@ fn walk_anim( out: &mut Vec, ) { let viewport_f = (viewport.0 as f32, viewport.1 as f32); - for (i, (child, box_node)) in children.iter().zip(boxes.iter()).enumerate() { + for (i, (child, box_node)) in children.iter().zip(principal_boxes(boxes)).enumerate() { let json_idx = path_indices.map(|idxs| idxs[i]).unwrap_or(i); let child_path = format!("{}.children[{}]", path, json_idx); let layout = match layouts.get(box_node.id) { @@ -1247,31 +1362,30 @@ fn walk_anim( && !parent_clips && layout.width > 0.5 && layout.height > 0.5 + // Round 4 audit, constats 2 & 9: `box_node.css` was rebuilt at + // this sample's real time (see `validate_geometry_animated`), + // so `css.opacity` already reflects `apply_animated_props` — + // no separate `AnimatedProperties` re-derivation needed for + // the visibility short-circuit any more. + && box_node.css.opacity.unwrap_or(1.0) > 0.001 { let stagger_delay = stagger_delays .get(box_node.id as usize) .copied() .unwrap_or(0.0); - // Reuse the renderer's own effect resolution instead of a - // divergent fork (H6): `effective_effects` merges timeline - // steps/synthesized transitions/stagger exactly like - // `legacy_dispatch.rs` does, and `resolve_props_for_effects` - // resolves them at absolute scene `time` — no re-timing by - // start_at, which would double-shift components that also - // declare a matching `animation` delay. - // // `time` above is *global* scene time; the renderer never // resolves effects at that raw value once a `time_scale`/ // `time_offset`-bearing container is in the ancestor chain — // `build_child` remaps it first (`box_builder.rs`: - // `t_local = scale * t_global + shift`). Constat #3: this walker - // used to skip that remap entirely, resolving effects at a time - // that never occurs at render — false positives when the - // container speeds the subtree up past the checked sample, false - // negatives when it slows it down. `built.time_params` carries - // the exact same accumulated `(scale, shift)` the renderer used, - // indexed by the same `NodeId`, so applying it here keeps this - // walker and the renderer in lockstep. + // `t_local = scale * t_global + shift`), and it already used + // this same remap to build `box_node.css` above. `local_time` + // is only still needed here to independently re-derive + // `AnimatedProperties.char_animation` (char-level overshoot), + // which `apply_animated_props` deliberately does NOT bake into + // CSS (component-internal, painter-only property — see that + // function's doc comment) — `built.time_params` carries the + // exact same accumulated `(scale, shift)` the renderer used, so + // this stays in lockstep with it. let (scale, shift) = time_params .get(box_node.id as usize) .copied() @@ -1282,41 +1396,60 @@ fn walk_anim( None => AnimatedProperties::default(), }; let raw_bbox = bbox_of(layout); - let base_bbox = apply_static_node_transform(&raw_bbox, &box_node.css, viewport_f); - if let Some(mut transformed) = transform_bbox(&base_bbox, &props) { - if let Some(cam) = camera { - transformed = fold_static_camera(&transformed, cam, viewport_f); - } - let vw = viewport.0 as f32; - let vh = viewport.1 as f32; - let eps = 0.5; - let right = transformed.x + transformed.w; - let bottom = transformed.y + transformed.h; - let x_over = transformed.x < -eps || right > vw + eps; - let y_over = transformed.y < -eps || bottom > vh + eps; - if x_over || y_over { - let axis = match (x_over, y_over) { - (true, true) => Axis::Both, - (true, false) => Axis::X, - (false, true) => Axis::Y, - _ => unreachable!(), - }; - // Dedupe across samples: one violation per (view, scene, path). - let key = (vi, si, child_path.clone()); - if seen.insert(key) { - let component_name = component_kind(&child.component).to_string(); - out.push(GeometryViolation { - view_index: vi, - scene_index: si, - path: child_path.clone(), - component: component_name, - axis, - kind: ViolationKind::AnimatedTextOverflow, - bbox: transformed, - viewport, - hint: hint_for_animated(&child.component, &props, time, scene_duration), - }); - } + // `apply_static_node_transform` — the SAME fold `walk` uses for + // a *static* `style.transform` — now does the whole job: + // `box_node.css.transform` already carries the resolved + // translate/scale/rotation (`apply_animated_props`, baked in at + // box-tree build time for this sample) composed with any + // static `style.transform` the component also declares, and + // the four-corner AABB it computes already accounts for + // rotation (constat 9) the same way it does for a static + // `transform: rotate(...)`. + let mut transformed = apply_static_node_transform(&raw_bbox, &box_node.css, viewport_f); + // Char-level overshoot (e.g. `char_scale_in`'s default 1.08) + // is the one animated-transform contributor NOT baked into + // `css.transform` — fold it in as an extra uniform scale + // around the already-transformed box's own centre. + if let Some(overshoot) = props + .char_animation + .as_ref() + .map(|c| c.overshoot.max(0.0)) + .filter(|o| *o > 1e-4) + { + transformed = scale_bbox_from_own_center(&transformed, 1.0 + overshoot); + } + if let Some(cam) = camera { + transformed = fold_static_camera(&transformed, cam, viewport_f); + } + let vw = viewport.0 as f32; + let vh = viewport.1 as f32; + let eps = 0.5; + let right = transformed.x + transformed.w; + let bottom = transformed.y + transformed.h; + let x_over = transformed.x < -eps || right > vw + eps; + let y_over = transformed.y < -eps || bottom > vh + eps; + if x_over || y_over { + let axis = match (x_over, y_over) { + (true, true) => Axis::Both, + (true, false) => Axis::X, + (false, true) => Axis::Y, + _ => unreachable!(), + }; + // Dedupe across samples: one violation per (view, scene, path). + let key = (vi, si, child_path.clone()); + if seen.insert(key) { + let component_name = component_kind(&child.component).to_string(); + out.push(GeometryViolation { + view_index: vi, + scene_index: si, + path: child_path.clone(), + component: component_name, + axis, + kind: ViolationKind::AnimatedTextOverflow, + bbox: transformed, + viewport, + hint: hint_for_animated(&child.component, &props, time, scene_duration), + }); } } } @@ -1344,32 +1477,21 @@ fn walk_anim( } } -/// Apply the canvas transforms the renderer applies (translate then scale -/// around the bbox center) to a base bbox. Returns `None` if the resulting -/// box is degenerate (fully transparent / zero size). -fn transform_bbox(base: &BBox, props: &AnimatedProperties) -> Option { - if props.opacity <= 0.001 { - return None; - } - // Conservative: also account for char animations that overshoot the box - // (e.g. char_scale_in defaults to 1.08). One extra factor on each axis. - let char_overshoot = props - .char_animation - .as_ref() - .map(|c| 1.0 + c.overshoot.max(0.0)) - .unwrap_or(1.0); - let sx = props.scale_x.abs().max(0.001) * char_overshoot; - let sy = props.scale_y.abs().max(0.001) * char_overshoot; - let center_x = base.x + base.w / 2.0 + props.translate_x; - let center_y = base.y + base.h / 2.0 + props.translate_y; - let new_w = base.w * sx; - let new_h = base.h * sy; - Some(BBox { - x: center_x - new_w / 2.0, - y: center_y - new_h / 2.0, - w: new_w, - h: new_h, - }) +/// Scale a bbox by `factor` around its OWN centre (as opposed to +/// `apply_static_node_transform`'s pivot, which is `transform-origin`) — +/// used only for the char-animation overshoot top-up in `walk_anim`, which +/// is not a CSS transform and has no origin concept of its own. +fn scale_bbox_from_own_center(bbox: &BBox, factor: f32) -> BBox { + let cx = bbox.x + bbox.w / 2.0; + let cy = bbox.y + bbox.h / 2.0; + let w = bbox.w * factor; + let h = bbox.h * factor; + BBox { + x: cx - w / 2.0, + y: cy - h / 2.0, + w, + h, + } } fn hint_for_animated( @@ -1468,6 +1590,59 @@ mod tests { ); } + // ─── Round 4 audit, constat 4: a `world` scene without its own `layout` + // must be validated against the SAME centred-column root layout + // `render_world_frame_scaled` synthesizes, not the plain top-aligned + // slide default ───────────────────────────────────────────────────── + + #[test] + fn layoutless_world_scene_uses_the_centred_root_not_the_slide_default() { + // A single in-flow (no `position`) 1000×100 shape, wider than the + // 800px-wide viewport, inside a `world` scene with no `layout` of + // its own. `render_world_frame_scaled` synthesizes a centred column + // (`align_items: center`) for exactly this case. + // + // Red-phase capture (root forced back to the slide default): bbox + // = {x: 0, y: 0, w: 1000, h: 100}, hint "current right edge is + // 1000" — `align_items` unset resolves start-aligned for an item + // with an explicit size, so the shape sits at x=0, right edge=1000. + // + // Under the CORRECT (world-default, centred) root, a 1000px item in + // an 800px-wide container centres at x=(800-1000)/2=-100: bbox + // x=[-100,900]. Still a single-axis (X) overflow — both edges are + // crossed, but `Axis::Both` means "X and Y both overflow", not "X + // overflows on both sides" — but the reported bbox.x is materially + // different (-100 vs 0) and, before this fix, wrong. + let json = r##"{ + "video": { "width": 800, "height": 600 }, + "composition": [{ + "type": "world", + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "shape", + "shape": "rect", + "style": { "width": "1000px", "height": "100px" }, + "fill": "#ff0000" + }] + }] + }] + }"##; + let scenario = parse(json); + let violations = validate_geometry(&scenario); + let v = violations + .iter() + .find(|v| v.kind == ViolationKind::ViewportOverflow) + .unwrap_or_else(|| panic!("expected a ViewportOverflow: {:?}", violations)); + assert_eq!(v.axis, Axis::X, "{:?}", v); + assert!( + (v.bbox.x - (-100.0)).abs() < 1.0, + "expected the shape centred at x=-100 (world root), got bbox.x={}: {:?}", + v.bbox.x, + v + ); + } + #[test] fn shape_past_right_edge_triggers_x_overflow() { // A 400×100 shape positioned at x=1700 in a 1920-wide viewport spills @@ -1546,6 +1721,81 @@ mod tests { assert_eq!(v.axis, Axis::X); } + // ─── Round 4 audit, constat 3: unwrappable_text_overflow must respect a + // clipping ancestor exactly like check_viewport/check_content_overflows_box + // already do ─────────────────────────────────────────────────────────── + + #[test] + fn unwrappable_text_is_suppressed_under_a_clipping_ancestor_card() { + // Same headline fixture as `unwrappable_text_in_narrow_card_is_flagged` + // (a 200px card, 96px nowrap text, natural width far exceeding 200px) + // but the card now clips (`overflow: hidden`): the text genuinely + // gets cropped to the card's edge at paint time, so nothing overflows + // on screen — geometry-safety.md promises this is exempt ("A node is + // also exempt when it clips itself, or when any ancestor clips it"), + // and `check_viewport`/`check_content_overflows_box` already honour + // it. This is a CORRECT scenario (the clip makes the excess + // invisible) that the validator wrongly rejected before this fix. + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "card", + "x": 100, "y": 100, + "style": { "width": "200px", "height": "200px", "background": "#222244", "overflow": "hidden" }, + "children": [{ + "type": "text", + "content": "this string is too long to fit", + "style": { "color": "#ffffff", "font-size": "96px", "white-space": "nowrap" } + }] + }] + }] + }"##; + let scenario = parse(json); + let violations = validate_geometry(&scenario); + assert!( + violations + .iter() + .all(|v| v.kind != ViolationKind::UnwrappableTextOverflow), + "nowrap text clipped by its own card must not be flagged: {:?}", + violations + ); + } + + #[test] + fn unwrappable_text_still_fires_without_a_clipping_ancestor() { + // Regression guard: the exact pre-existing fixture from + // `unwrappable_text_in_narrow_card_is_flagged` (card overflow left + // at the default `visible`) must keep firing — the fix must only add + // a clip-aware exemption, not silence the check generally. + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "card", + "x": 100, "y": 100, + "style": { "width": "200px", "height": "200px", "background": "#222244" }, + "children": [{ + "type": "text", + "content": "this string is too long to fit", + "style": { "color": "#ffffff", "font-size": "96px", "white-space": "nowrap" } + }] + }] + }] + }"##; + let scenario = parse(json); + let violations = validate_geometry(&scenario); + assert!( + violations + .iter() + .any(|v| v.kind == ViolationKind::UnwrappableTextOverflow), + "must still fire when nothing clips: {:?}", + violations + ); + } + #[test] fn marquee_is_exempted_from_overflow() { // A marquee that bleeds past the viewport: no violation should fire, @@ -1602,6 +1852,141 @@ mod tests { assert_eq!(v.unwrap().component, "codeblock"); } + // ─── Round 4 audit, constat 6: check_auto_scroll must use the real + // painter's dimension formula (CodeblockIntrinsic/TerminalIntrinsic), + // not a hardcoded 16+16=32px padding assumption ────────────────────── + + #[test] + fn codeblock_auto_scroll_check_honours_explicit_padding_not_a_hardcoded_16px() { + // 10 lines, font-size defaults to 14px (line-height 1.3 -> 18.2px/line + // -> 182px of text), auto_scroll: false, box height fixed at 250px. + // style.padding is *explicitly* 60px on every side (120px vertical + // budget) — nothing close to the hardcoded "16 top + 16 bottom" the + // old formula assumed. Real natural height (chrome disabled): + // 120 (padding) + 182 (text) = 302px, ~52px past the 250px box — + // a genuine overflow. The hardcoded-32px formula computed + // 32 + 182 = 214px, comfortably under 250px, and stayed silent. + let code_lines: String = (1..=10) + .map(|i| i.to_string()) + .collect::>() + .join("\\n"); + let json = format!( + r##"{{ + "video": {{ "width": 1920, "height": 1080 }}, + "scenes": [{{ + "duration": 1.0, + "children": [{{ + "type": "codeblock", + "code": "{code_lines}", + "auto_scroll": false, + "style": {{ "width": "600px", "height": "250px", "padding": "60px" }} + }}] + }}] + }}"## + ); + let scenario = parse(&json); + let violations = validate_geometry(&scenario); + let v = violations + .iter() + .find(|v| v.kind == ViolationKind::AutoScrollDisabledOverflow); + assert!( + v.is_some(), + "expected AutoScrollDisabledOverflow for a 60px-padded codeblock the \ + hardcoded-16px formula wrongly cleared (real natural height ~302px > \ + 250px box): {:?}", + violations + ); + } + + #[test] + fn codeblock_auto_scroll_check_does_not_false_positive_on_tight_default_padding() { + // Complementary false-positive guard: 10 lines, DEFAULT padding + // (16px each side -> 32px vertical budget, matching + // CodeblockIntrinsic's own fallback for an all-zero/unset padding — + // see `CodeblockIntrinsic::from_codeblock`'s (16,16,16,16) default). + // Natural height: 32 + 182 = 214px. Box height 220px comfortably + // holds it — must NOT be flagged. + let code_lines: String = (1..=10) + .map(|i| i.to_string()) + .collect::>() + .join("\\n"); + let json = format!( + r##"{{ + "video": {{ "width": 1920, "height": 1080 }}, + "scenes": [{{ + "duration": 1.0, + "children": [{{ + "type": "codeblock", + "code": "{code_lines}", + "auto_scroll": false, + "style": {{ "width": "600px", "height": "220px" }} + }}] + }}] + }}"## + ); + let scenario = parse(&json); + let violations = validate_geometry(&scenario); + assert!( + violations + .iter() + .all(|v| v.kind != ViolationKind::AutoScrollDisabledOverflow), + "a codeblock that genuinely fits its box must not be flagged: {:?}", + violations + ); + } + + #[test] + fn terminal_auto_scroll_check_uses_the_painters_fixed_line_height_ratio() { + // Terminal (unlike codeblock) does NOT honour `style.line-height` at + // paint time — `terminal.rs`'s own `line_height()` method always + // computes `(font_size * 22.0 / 14.0).ceil()` (a fixed ratio baked + // into the component, `terminal::LINE_HEIGHT`/`FONT_SIZE`), ignoring + // any CSS `line-height` override entirely. The old hand-rolled check + // used `t.style.line_height_for(font_size)` (the CSS property, + // honouring `style.line-height`) instead — so a `line-height: 3` + // override (unitless -> 3 * 14px = 42px/line) inflated the OLD + // formula's estimate even though the real painter still renders + // 22px lines and ignores the override. + // + // 8 lines, chrome disabled, font-size defaults to 14: + // real (TerminalIntrinsic/painter): 2*16 (fixed padding) + + // 8 * 22 (fixed ratio, ignores the override) = 32 + 176 = 208px + // old hand-rolled (CSS line-height, AND its own wrong default + // font-size of 16px instead of the real 14px): + // 32 + 8 * line_height_for(16) = 32 + 8 * 48 = 32 + 384 = 416px + // (captured red-phase output: "terminal content needs ~416px") + // Box height fixed at 300px sits strictly between the two: the real + // content fits (208 < 300), but the old formula's inflated 416px + // wrongly reported an overflow — a false positive this fix removes. + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "terminal", + "lines": [ + { "text": "one" }, { "text": "two" }, { "text": "three" }, + { "text": "four" }, { "text": "five" }, { "text": "six" }, + { "text": "seven" }, { "text": "eight" } + ], + "show_chrome": false, + "auto_scroll": false, + "style": { "width": "600px", "height": "300px", "line-height": 3 } + }] + }] + }"##; + let scenario = parse(json); + let violations = validate_geometry(&scenario); + assert!( + violations + .iter() + .all(|v| v.kind != ViolationKind::AutoScrollDisabledOverflow), + "terminal ignores style.line-height at paint time — the check must too, \ + real content (208px) fits the 300px box: {:?}", + violations + ); + } + // ─── C1: remediation hints must never name the nonexistent `wrap` field ── #[test] @@ -2052,12 +2437,175 @@ mod tests { ); } + // ─── Round 4 audit, constat 2: --strict-anim must resolve `timeline` + // style states and audio-reactive transforms — both gated on + // `local_actx.is_some()` in `build_child`, which was always `None` + // here ──────────────────────────────────────────────────────────────── + #[test] - fn anim_sample_times_scale_with_scene_duration() { - let short = anim_sample_times(0.5); - let long = anim_sample_times(4.0); - assert!( - long.len() > short.len(), + fn strict_anim_catches_a_brief_excursion_a_40_sample_cap_would_miss() { + // A 100×100 shape resting safely at x=100 (box x=[100,200]) in a + // 20s scene, with `slide_in_left` (delay=9.85s, duration=1.0s): + // `position.x` eases from -200 to 0 via EaseOutCubic, so the box + // only crosses the left edge (x < -0.5) for the FIRST ~20% of the + // 1s window (t in [9.85, ~10.06]) — a ~206ms excursion, while + // opacity has already ramped past its own [9.85, 10.15] fade-in + // window's midpoint by then (so it isn't filtered as invisible). + // + // With the OLD `ANIM_MAX_SAMPLES=40` cap, this 20s scene sampled at + // step 20/39 ≈ 0.513s — grid points at k·0.513s land at + // t=9.744 (k=19) and t=10.256 (k=20), straddling the whole ~206ms + // excursion without a single sample landing inside it. Confirmed by + // temporarily reverting the cap to 40 during development: this + // fixture produced ZERO violations (`violations: []`) — the exact + // false negative constat 8 describes. + // + // With the new 480-sample cap (step ≈ 0.042s), a sample lands well + // inside the excursion — this run finds one at t=9.94s with + // bbox.x≈-52.16 (tx≈-152 relative to the resting x=100). + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 20.0, + "children": [{ + "type": "shape", + "shape": "rect", + "position": "absolute", + "x": 100, "y": 490, + "style": { + "width": "100px", "height": "100px", + "animation": [{ "name": "slide_in_left", "delay": 9.85, "duration": 1.0 }] + }, + "fill": "#ff0000" + }] + }] + }"##; + let scenario = parse(json); + let violations = validate_geometry_animated(&scenario); + let v = violations + .iter() + .find(|v| v.kind == ViolationKind::AnimatedTextOverflow) + .unwrap_or_else(|| { + panic!( + "expected the ~206ms slide_in_left excursion around t≈9.85-10.06s \ + to be caught by the denser sampling: {:?}", + violations + ) + }); + assert_eq!(v.component, "shape"); + assert_eq!(v.axis, Axis::X); + assert!( + v.bbox.x < -0.5, + "expected a negative bbox.x (left-edge overflow), got {}", + v.bbox.x + ); + } + + #[test] + fn strict_anim_detects_a_timeline_width_step_that_overflows_later_in_the_scene() { + // A 200×100 shape, safely inside a 1920×1080 viewport at rest + // (x=[100,300]). A `timeline` step at t=1.0s grows `style.width` to + // 1900px — box_builder's `apply_style_states` runs on the CSS + // *before* layout, so this is a genuine box-model change, not a + // paint-only transform: at t>=1.0s the real render lays out a + // 1900px-wide box at x=100, right edge 2000 — 80px past the + // 1920px-wide frame. + // + // The OLD `--strict-anim` walker built its box tree ONCE with + // `anim: None` (so `apply_style_states` only ever evaluated at + // t=0, before the step's `at`) and never rebuilt it per sample — + // every one of the 16 samples in this 2s scene measured the + // resting 200px-wide box, so this never got flagged. + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 2.0, + "children": [{ + "type": "shape", + "shape": "rect", + "position": "absolute", + "x": 100, "y": 100, + "style": { "width": "200px", "height": "100px" }, + "timeline": [{ "at": 1.0, "style": { "width": "1900px" } }], + "fill": "#ff0000" + }] + }] + }"##; + let scenario = parse(json); + let violations = validate_geometry_animated(&scenario); + let v = violations + .iter() + .find(|v| v.kind == ViolationKind::AnimatedTextOverflow) + .unwrap_or_else(|| { + panic!( + "expected AnimatedTextOverflow once the t=1.0s timeline step widens \ + the box to 1900px: {:?}", + violations + ) + }); + assert_eq!(v.component, "shape"); + assert_eq!(v.axis, Axis::X); + } + + // ─── Round 4 audit, constat 9: --strict-anim must model rotation (and + // any other transform `apply_animated_props` bakes into `css.transform`), + // not just translate_x/y and scale_x/y ───────────────────────────────── + + #[test] + fn strict_anim_detects_a_spin_animation_pushing_a_square_off_screen() { + // Same headline numbers as the static-transform regression + // `static_rotation_is_folded_into_the_viewport_check`: a 100×100 + // square at (1810, 490) in a 1920×1080 viewport — resting box + // x=[1810,1910], 10px inside the right edge. `spin` animates + // `rotation` linearly 0deg->360deg over the 2s scene; ANY sampled + // angle away from a multiple of 90deg grows the AABB half-width + // beyond 50px * (|cos|+|sin|) > 50px, pushing the right edge past + // 1920 (e.g. at 20deg: half-width ~64.1px, right edge ~1924). + // + // The OLD `transform_bbox` only read `translate_x/y`/`scale_x/y` + // from `AnimatedProperties` — a pure-rotation preset leaves both at + // their identity values (0 and 1), so every sample folded to + // exactly the resting bbox and this never fired, at any sample, + // for the whole 2s sweep through 360 degrees. + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 2.0, + "children": [{ + "type": "shape", + "shape": "rect", + "position": "absolute", + "x": 1810, "y": 490, + "style": { + "width": "100px", "height": "100px", + "animation": [{ "name": "spin", "delay": 0, "duration": 2.0 }] + }, + "fill": "#ff0000" + }] + }] + }"##; + let scenario = parse(json); + let violations = validate_geometry_animated(&scenario); + let v = violations + .iter() + .find(|v| v.kind == ViolationKind::AnimatedTextOverflow) + .unwrap_or_else(|| { + panic!( + "expected AnimatedTextOverflow from the spin preset rotating the \ + square past the right edge at some sampled angle: {:?}", + violations + ) + }); + assert_eq!(v.component, "shape"); + assert_eq!(v.axis, Axis::X); + } + + #[test] + fn anim_sample_times_scale_with_scene_duration() { + let short = anim_sample_times(0.5); + let long = anim_sample_times(4.0); + assert!( + long.len() > short.len(), "longer scenes should get more samples: {} vs {}", long.len(), short.len() @@ -2076,6 +2624,59 @@ mod tests { ); } + #[test] + fn anim_sample_times_keeps_the_8_per_second_cadence_up_to_60s() { + // Round 4 audit, constat 8: with the old ANIM_MAX_SAMPLES=40 cap, + // the step between samples grew linearly past a 5s scene — + // 20s/39 ≈ 0.513s at 20s, 40s/39 ≈ 1.026s at 40s, 60s/39 ≈ 1.538s + // at 60s (the exact numbers the audit cited). With the raised cap + // (480), the step stays pinned near the promised 1/8s = 0.125s + // resolution across the same range. + for duration in [20.0, 40.0, 60.0] { + let samples = anim_sample_times(duration); + let step = duration / (samples.len() - 1) as f64; + assert!( + (step - 0.125).abs() < 0.001, + "duration={duration}s: expected ~0.125s step, got {step}s ({} samples)", + samples.len() + ); + } + } + + #[test] + #[ignore = "manual timing probe for constat 8's cap — not run in CI"] + fn timing_probe_for_constat_8() { + let mut children = String::new(); + for i in 0..15 { + children.push_str(&format!( + r##"{{"type":"text","position":"absolute","x":{},"y":{}, + "content":"item {}", + "style":{{"font-size":32,"color":"#ffffff", + "animation":[{{"name":"slide_in_left","delay":0.1,"duration":0.5}}]}}}},"##, + (i % 5) * 300, + (i / 5) * 200, + i + )); + } + children.pop(); // trailing comma + let json = format!( + r##"{{"video":{{"width":1920,"height":1080}}, + "scenes":[{{"duration":60.0,"children":[{children}]}}]}}"## + ); + let scenario = parse(&json); + let n = anim_sample_times(60.0).len(); + let start = std::time::Instant::now(); + let violations = validate_geometry_animated(&scenario); + let elapsed = start.elapsed(); + eprintln!( + "timing_probe: {} samples, {:?} total, {:?}/sample, {} violations", + n, + elapsed, + elapsed / n.max(1) as u32, + violations.len() + ); + } + // ─── H4 (second half): content larger than its own content box ─────────── // // The first half of H4 (already fixed above) suppresses a *viewport* @@ -2390,6 +2991,96 @@ mod tests { assert_eq!(v.axis, Axis::X, "only the x-axis should overflow: {:?}", v); } + // ─── Round 4 audit, constat 5: `transform-origin` must pivot the static + // transform fold, not always the box centre ───────────────────────────── + + #[test] + fn transform_origin_right_edge_keeps_a_scaled_shape_inside_the_viewport() { + // 100×100 shape at (880, 450) in a 1000×1000 viewport: at rest, + // x=[880,980] — comfortably inside, 20px margin. `scale(x: 3)` + // pivoted at `transform-origin: { x: "right" }` (the box's own right + // edge, 100%) grows the box purely leftward from that fixed edge: + // left corner offset from pivot (980) is -100, ×3 = -300 -> new x = + // 680; right corner offset is 0 -> stays at 980. Correct AABB: + // x=[680,980], fully inside [0,1000] — this scenario is CORRECT. + // + // Before this fix, `apply_static_node_transform` always pivoted at + // the box centre (930): left corner offset -50×3=-150 -> x=780; + // right corner offset +50×3=+150 -> x=1080 — 80px past the 1000-wide + // frame, a false positive (captured in the red-phase run below). + let json = r##"{ + "video": { "width": 1000, "height": 1000 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "shape", + "shape": "rect", + "position": "absolute", + "x": 880, "y": 450, + "style": { + "width": "100px", "height": "100px", + "transform": [{ "fn": "scale", "x": 3, "y": 1 }], + "transform-origin": { "x": "right" } + }, + "fill": "#ff0000" + }] + }] + }"##; + let scenario = parse(json); + let violations = validate_geometry(&scenario); + assert!( + violations + .iter() + .all(|v| v.kind != ViolationKind::ViewportOverflow), + "transform-origin: right should pivot growth away from the right \ + edge, keeping the shape inside the 1000px-wide viewport: {:?}", + violations + ); + } + + #[test] + fn transform_origin_50pct_is_identical_to_the_default_centre_pivot() { + // Sanity/regression guard: an *explicit* `transform-origin: 50% 50%` + // must fold to exactly the same AABB as no `transform-origin` at all + // — same fixture and expectation as + // `static_rotation_is_folded_into_the_viewport_check`. + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "shape", + "shape": "rect", + "position": "absolute", + "x": 1810, "y": 490, + "style": { + "width": "100px", "height": "100px", + "transform": [{ "fn": "rotate", "deg": 45 }], + "transform-origin": { "x": "50%", "y": "50%" } + }, + "fill": "#ff0000" + }] + }] + }"##; + let scenario = parse(json); + let violations = validate_geometry(&scenario); + let v = violations + .iter() + .find(|v| v.component == "shape" && v.kind == ViolationKind::ViewportOverflow) + .unwrap_or_else(|| { + panic!( + "explicit 50%/50% origin must behave like the default centre pivot: {:?}", + violations + ) + }); + assert_eq!(v.axis, Axis::X); + assert!( + (v.bbox.x + v.bbox.w - 1930.71).abs() < 1.0, + "right edge should land at the same ~1930.7 as the centre-pivot case: {:?}", + v + ); + } + #[test] fn unrotated_transform_folding_is_unchanged_by_the_corner_based_rewrite() { // Regression guard for the H5 rewrite: a translate-only transform @@ -2454,17 +3145,32 @@ mod tests { assert_eq!(v.axis, Axis::Y); } - // ─── #128 item 2: component overflowing its containing card ────────────── + // ─── Round 4 audit, constat 7: `ContentOverflowsCard` (#128 item 2) is + // retired — a component escaping a non-clipping (`overflow: visible`, + // the default) card is the exact "badge sticking out of a card is + // legal" pattern CLAUDE.md / geometry-safety.md document as fine, so the + // validator must accept it, not report it. See the module doc comment's + // "Deliberately NOT in scope" note and `walk`'s retired call site for + // the full reasoning. These fixtures are the same ones that used to + // assert the (wrong) opposite — kept, with flipped assertions, as + // regression coverage across component types (text/codeblock/table/ + // nested card/bleed) now that the check is gone. ───────────────────── #[test] - fn absolutely_positioned_text_spilling_past_its_card_is_flagged() { - // The audit's headline repro: a text with no fixed height, inside a - // card, grows to its natural (unclamped) size because it's taken - // out of flex flow (`position: absolute`) — its OWN box already - // matches its OWN content exactly (so `ContentOverflowsBox` must NOT - // fire), yet that box spills well past the 80px-tall card it lives - // in. Before #128 item 2, nothing ever compared a component to the - // card containing it, so this validated clean. + fn absolutely_positioned_text_spilling_past_a_visible_card_is_legal() { + // The audit's original headline repro for #128 item 2: a text with + // no fixed height, inside a card, grows to its natural (unclamped) + // size because it's taken out of flex flow (`position: absolute`) + // — its OWN box already matches its OWN content exactly (so + // `ContentOverflowsBox` must NOT fire), and that box spills past + // the 80px-tall card it lives in. The card's `overflow` is + // `visible` (the documented default that permits exactly this) — + // per constat 7, `ContentOverflowsCard` must no longer fire here. + // + // Red-phase (before this fix): validate_geometry reported one + // ContentOverflowsCard violation for this fixture (component: + // "text", axis: Y, hint mentioning "extends past its containing + // card") — captured when this test asserted the opposite. let json = r##"{"video":{"width":960,"height":540,"fps":30,"background":"#0A0A12"}, "scenes":[{"duration":1.0,"children":[ {"type":"card","position":"absolute","x":330,"y":100, @@ -2479,7 +3185,7 @@ mod tests { violations .iter() .all(|v| v.kind != ViolationKind::ContentOverflowsBox), - "the text's own box already matches its own (unclamped) content — must not also fire ContentOverflowsBox: {:?}", + "the text's own box already matches its own (unclamped) content — must not fire ContentOverflowsBox: {:?}", violations ); assert!( @@ -2489,26 +3195,41 @@ mod tests { "fixture should stay inside the 540px-tall frame by construction: {:?}", violations ); - let v = violations - .iter() - .find(|v| v.kind == ViolationKind::ContentOverflowsCard) - .unwrap_or_else(|| { - panic!( - "expected ContentOverflowsCard for text spilling past its 80px card: {:?}", - violations - ) - }); - assert_eq!(v.component, "text"); - assert_eq!( - v.axis, - Axis::Y, - "card is wide enough — only height should overflow: {:?}", - v + assert!( + violations + .iter() + .all(|v| v.kind != ViolationKind::ContentOverflowsCard), + "text sticking out of a visible-overflow card is a legal, documented pattern: {:?}", + violations ); + } + + /// The guarantee that makes retiring `ContentOverflowsCard` safe rather + /// than merely defensible: escaping a visible card is legal, but + /// escaping the *device* never is, and that is `check_viewport`'s job — + /// not the retired check's. Same fixture as + /// `absolutely_positioned_text_spilling_past_a_visible_card_is_legal`, + /// moved down the frame so the overspill leaves the viewport. If this + /// ever stops firing, the removal has opened a real blind spot. + #[test] + fn spilling_past_a_visible_card_is_still_caught_when_it_leaves_the_viewport() { + let json = r##"{"video":{"width":960,"height":540,"fps":30,"background":"#0A0A12"}, + "scenes":[{"duration":1.0,"children":[ + {"type":"card","position":"absolute","x":330,"y":460, + "style":{"width":300,"height":80,"background":"#1e2233","overflow":"visible"}, + "children":[{"type":"text","position":"absolute","x":0,"y":0, + "content":"Ce paragraphe est beaucoup plus grand que la carte de 80px qui le contient.", + "style":{"font-size":44,"color":"#ffffff","width":"300px"}}]}]}]}"##; + let scenario = parse(json); + let violations = validate_geometry(&scenario); + assert!( - v.hint.contains("card"), - "hint should mention the card: {}", - v.hint + violations + .iter() + .any(|v| v.kind == ViolationKind::ViewportOverflow), + "content escaping a visible card AND the 540px frame must still be reported by \ + check_viewport — retiring ContentOverflowsCard must not have removed this: {:?}", + violations ); } @@ -2559,16 +3280,16 @@ mod tests { } #[test] - fn absolutely_positioned_codeblock_spilling_past_its_card_is_flagged() { + fn absolutely_positioned_codeblock_spilling_past_a_visible_card_is_legal() { // #128 item 1's first repro ("a codeblock painting 578px inside a // 300px card"): taken out of flex flow (`position: absolute`, like // the analogous text/table tests above) so its own box is NOT // shrunk to fit the card — it stays at its natural, unscrolled // content height regardless of `auto_scroll`. `auto_scroll: true` - // (the default) is used deliberately here: `check_auto_scroll` only - // ever fires for `auto_scroll: false`, so this proves the - // card-relative check is a genuinely independent, complementary - // mechanism, not a duplicate of it. + // (the default) is used deliberately here so `check_auto_scroll` + // stays quiet too, isolating this from every other check: the card + // has default (`visible`) overflow, so per constat 7 this must + // validate clean. let code_lines: String = (1..=30) .map(|i| i.to_string()) .collect::>() @@ -2609,16 +3330,13 @@ mod tests { "fixture should stay inside the 1080px-tall frame by construction: {:?}", violations ); - let v = violations - .iter() - .find(|v| v.kind == ViolationKind::ContentOverflowsCard && v.component == "codeblock") - .unwrap_or_else(|| { - panic!( - "expected ContentOverflowsCard for a 30-line codeblock spilling past its 300px card: {:?}", - violations - ) - }); - assert_eq!(v.axis, Axis::Y); + assert!( + violations + .iter() + .all(|v| v.kind != ViolationKind::ContentOverflowsCard), + "codeblock sticking out of a visible-overflow card is a legal, documented pattern: {:?}", + violations + ); } #[test] @@ -2629,9 +3347,7 @@ mod tests { // assigned box down to the card's 60px (same shrink-to-fit behaviour // already established for `text`), so this surfaces via the // generalized `ContentOverflowsBox` (own box too small for own - // content) rather than the card check — see - // `absolutely_positioned_table_spilling_past_its_card_is_flagged` - // below for the complementary (unclamped, card-relative) case. + // content). let rows: String = (1..=15) .map(|i| format!(r#"["{i}a","{i}b","{i}c"]"#)) .collect::>() @@ -2670,12 +3386,11 @@ mod tests { } #[test] - fn absolutely_positioned_table_spilling_past_its_card_is_flagged() { + fn absolutely_positioned_table_spilling_past_a_visible_card_is_legal() { // Same table, but taken out of flex flow (`position: absolute`) so - // its own box isn't shrunk to fit the card — mirrors - // `absolutely_positioned_text_spilling_past_its_card_is_flagged`, - // isolating the card-relative check (item 2) from the own-box check - // (item 1) exercised above. + // its own box isn't shrunk to fit the card — the card's `overflow` + // is `visible` (the default), so per constat 7 this must validate + // clean. let rows: String = (1..=15) .map(|i| format!(r#"["{i}a","{i}b","{i}c"]"#)) .collect::>() @@ -2703,75 +3418,21 @@ mod tests { ); let scenario = parse(&json); let violations = validate_geometry(&scenario); - let v = violations - .iter() - .find(|v| v.kind == ViolationKind::ContentOverflowsCard && v.component == "table") - .unwrap_or_else(|| { - panic!( - "expected ContentOverflowsCard for an absolutely positioned table spilling past its 60px card: {:?}", - violations - ) - }); - assert_eq!(v.axis, Axis::Y); - } - - #[test] - fn nested_non_card_container_still_compares_its_descendant_to_the_outer_card() { - // A `container` (layout-only, no decoration per CLAUDE.md) sits - // between the card and the codeblock. `container`/`flex`/`grid`/ - // `positioned` must NOT update `nearest_card` — only `Component::Card` - // does — so the codeblock two levels down must still be compared - // against the *outer* card, not silently un-checked. - let code_lines: String = (1..=30) - .map(|i| i.to_string()) - .collect::>() - .join("\\n"); - let json = format!( - r##"{{ - "video": {{ "width": 1920, "height": 1080 }}, - "scenes": [{{ - "duration": 1.0, - "children": [{{ - "type": "card", - "position": "absolute", - "x": 100, "y": 100, - "style": {{ "width": "600px", "height": "300px", "background": "#111111" }}, - "children": [{{ - "type": "container", - "children": [{{ - "type": "codeblock", - "position": "absolute", - "x": 0, "y": 0, - "code": "{code_lines}" - }}] - }}] - }}] - }}] - }}"## - ); - let scenario = parse(&json); - let violations = validate_geometry(&scenario); - let v = violations - .iter() - .find(|v| v.kind == ViolationKind::ContentOverflowsCard && v.component == "codeblock") - .unwrap_or_else(|| { - panic!( - "expected ContentOverflowsCard through the non-card `container` wrapper: {:?}", - violations - ) - }); assert!( - v.path.contains("children[0].children[0].children[0]"), - "path should reach through card -> container -> codeblock: {}", - v.path + violations + .iter() + .all(|v| v.kind != ViolationKind::ContentOverflowsCard), + "table sticking out of a visible-overflow card is a legal, documented pattern: {:?}", + violations ); } #[test] - fn nested_card_bigger_than_its_outer_card_is_flagged() { - // A card nested inside another card, itself bigger than the outer - // one it lives in — the inner card's own box (not just its - // descendants') must be checked against the outer card too. + fn nested_card_bigger_than_its_visible_outer_card_is_legal() { + // A card nested inside another (default/`visible`-overflow) card, + // itself bigger than the outer one it lives in — per constat 7 this + // is the same "sticking out on purpose" pattern, now legal for any + // component type, cards included. let json = r##"{ "video": { "width": 1920, "height": 1080 }, "scenes": [{ @@ -2794,19 +3455,23 @@ mod tests { assert!( violations .iter() - .any(|v| v.kind == ViolationKind::ContentOverflowsCard && v.component == "card"), - "the inner card's own box must be checked against the outer card: {:?}", + .all(|v| v.kind != ViolationKind::ContentOverflowsCard), + "an inner card bigger than its visible-overflow outer card is legal: {:?}", violations ); } #[test] - fn content_overflowing_its_card_is_suppressed_under_a_clipping_card() { - // Same headline repro as `absolutely_positioned_text_spilling_past_its_card_is_flagged`, - // but the card clips (`overflow: hidden`) — the text genuinely gets - // clipped at paint time, so ContentOverflowsCard must not fire, - // consistent with the `parent_clips` suppression already applied to - // check_viewport/check_content_overflows_box. + fn content_overflowing_a_clipping_card_is_also_not_flagged() { + // Same headline repro, but the card clips (`overflow: hidden`) — + // the text genuinely gets clipped at paint time, so + // ContentOverflowsCard must not fire either, consistent with the + // `parent_clips` suppression already applied to + // check_viewport/check_content_overflows_box. Distinct from the + // `visible` fixtures above: this is the OTHER half of the "no + // configuration where it's both reachable and correct to fire" + // argument (constat 7) — a clipping card suppresses it for an + // unrelated reason (parent_clips), not because of the retirement. let json = r##"{"video":{"width":960,"height":540,"fps":30,"background":"#0A0A12"}, "scenes":[{"duration":1.0,"children":[ {"type":"card","position":"absolute","x":330,"y":100, @@ -2825,31 +3490,6 @@ mod tests { ); } - #[test] - fn bleed_true_does_not_exempt_content_overflows_card() { - // Same headline repro, `bleed: true` added to the text. Per the - // frozen `bleed` contract (see `bleeds`'s doc comment): a component - // may leave the *frame* on purpose and still be responsible for its - // own contents relative to its card — `bleed` must not exempt this - // check any more than it exempts ContentOverflowsBox. - let json = r##"{"video":{"width":960,"height":540,"fps":30,"background":"#0A0A12"}, - "scenes":[{"duration":1.0,"children":[ - {"type":"card","position":"absolute","x":330,"y":100, - "style":{"width":300,"height":80,"background":"#1e2233","overflow":"visible"}, - "children":[{"type":"text","position":"absolute","x":0,"y":0,"bleed":true, - "content":"Ce paragraphe est beaucoup plus grand que la carte de 80px qui le contient.", - "style":{"font-size":44,"color":"#ffffff","width":"300px"}}]}]}]}"##; - let scenario = parse(json); - let violations = validate_geometry(&scenario); - assert!( - violations - .iter() - .any(|v| v.kind == ViolationKind::ContentOverflowsCard && v.component == "text"), - "bleed: true must NOT suppress ContentOverflowsCard: {:?}", - violations - ); - } - #[test] fn component_that_fits_its_card_is_not_flagged() { // Passing-case guard: same shape as the codeblock repro, but the diff --git a/crates/rustmotion-cli/src/commands/validate.rs b/crates/rustmotion-cli/src/commands/validate.rs index a0aff12..28d0abe 100644 --- a/crates/rustmotion-cli/src/commands/validate.rs +++ b/crates/rustmotion-cli/src/commands/validate.rs @@ -566,4 +566,142 @@ mod tests { } } } + + /// Round 4 audit, constat 1: PR #145 introduced `refuse_fix`, gated on + /// the raw bytes on disk (not `loaded.raw`), and `apply_fixes`/`navigate` + /// already walk raw-preserving indices (H3, see + /// `geometry.rs::deserialize_children_indexed`'s doc comment). This + /// workstream's job is not to redo that fix — it's to prove, end to + /// end through `cmd_validate` (not just unit-testing `refuse_fix` in + /// isolation, as `fix_refusals` above does), that the refusal actually + /// engages for the two concrete failure modes constat 1 names: + /// - validate.rs:60 — `--fix` would otherwise serialise the + /// *post-substitution* document, dropping `config` and baking in + /// `$var` resolutions, silently destroying the template. + /// - validate.rs:198 — a violation path carries the *resolved* scene + /// index (post `include::resolve_entries` inlining), which does not + /// line up with the RAW `scenes` array `navigate` walks as soon as an + /// `include` expands to a scene count that shifts later positions. + /// + /// Both are already covered by the existing `refuse_fix` gate (a + /// `Templated`/`UsesInclude` scenario is refused outright, before + /// `apply_fixes` ever runs) — these two tests are the proof, not a new + /// fix. No RED phase: this constat is "verify existing behaviour", not + /// "here is a bug"; both tests pass on first run. + mod fix_refusals_end_to_end { + use super::super::cmd_validate; + + #[test] + fn cmd_validate_fix_refuses_to_overwrite_a_templated_scenario_and_leaves_the_file_untouched( + ) { + let path = std::env::temp_dir().join(format!( + "rm_validate_fix_templated_{}.json", + std::process::id() + )); + // `config` + a whole-string `$title` reference, plus a real + // geometry violation (nowrap text far too wide for its card) so + // `--fix` actually attempts to write. + let original = r##"{ + "config": { "title": { "type": "string", "default": "hi" } }, + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "card", + "x": 100, "y": 100, + "style": { "width": "200px", "height": "200px", "background": "#222244" }, + "children": [{ + "type": "text", + "content": "$title but also this string is too long to fit", + "style": { "color": "#ffffff", "font-size": "96px", "white-space": "nowrap" } + }] + }] + }] + }"##; + std::fs::write(&path, original).expect("write fixture"); + + let result = cmd_validate(&path, None, /*fix=*/ true, false, false, false, None); + + let after = std::fs::read_to_string(&path).expect("read back fixture"); + std::fs::remove_file(&path).ok(); + + assert!( + result.is_err(), + "--fix on a templated scenario with a real violation must be refused, \ + not silently applied" + ); + assert_eq!( + after, original, + "the file must be byte-identical after a refused --fix — writing \ + loaded.raw here would have dropped `config` and baked in the \ + substituted $title" + ); + } + + #[test] + fn cmd_validate_fix_refuses_to_overwrite_a_scenario_using_include_and_leaves_files_untouched( + ) { + let dir = std::env::temp_dir() + .join(format!("rm_validate_fix_include_{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("mkdir"); + let part_path = dir.join("part.json"); + let parent_path = dir.join("parent.json"); + + // The included file resolves to TWO scenes; the offending + // narrow-card/nowrap-text violation lives in the SECOND one, so + // its *resolved* scene index (1) does not correspond to any + // scene in the parent's own RAW `scenes` array (which has a + // single entry: the include directive) — the concrete index + // skew constat 1 names. + let part = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [ + { "duration": 1.0, "children": [] }, + { + "duration": 1.0, + "children": [{ + "type": "card", + "x": 100, "y": 100, + "style": { "width": "200px", "height": "200px", "background": "#222244" }, + "children": [{ + "type": "text", + "content": "this string is too long to fit", + "style": { "color": "#ffffff", "font-size": "96px", "white-space": "nowrap" } + }] + }] + } + ] + }"##; + let parent = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ "include": "part.json" }] + }"##; + std::fs::write(&part_path, part).expect("write part fixture"); + std::fs::write(&parent_path, parent).expect("write parent fixture"); + + let result = cmd_validate( + &parent_path, + None, + /*fix=*/ true, + false, + false, + false, + None, + ); + + let parent_after = std::fs::read_to_string(&parent_path).expect("read back parent"); + let part_after = std::fs::read_to_string(&part_path).expect("read back part"); + std::fs::remove_dir_all(&dir).ok(); + + assert!( + result.is_err(), + "--fix on an include-using scenario with a real violation must be refused" + ); + assert_eq!( + parent_after, parent, + "parent file must be byte-identical after a refused --fix" + ); + assert_eq!(part_after, part, "included file must be untouched too"); + } + } } diff --git a/crates/rustmotion/src/engine/render/scene.rs b/crates/rustmotion/src/engine/render/scene.rs index 0a5ab7a..6355da4 100644 --- a/crates/rustmotion/src/engine/render/scene.rs +++ b/crates/rustmotion/src/engine/render/scene.rs @@ -5,7 +5,7 @@ use super::background::draw_animated_background; use super::background::draw_world_bg_with_parallax; use crate::components::ChildComponent; use crate::error::RustmotionError; -use crate::schema::{Camera, Scene, SceneLayout, VideoConfig}; +use crate::schema::{Camera, Scene, SceneLayout, VideoConfig, ViewType}; use rustmotion_core::css::style::{ AlignItems as CssAlignItems, CssStyle, Edges, FlexDirection as CssFlexDirection, Gap, JustifyContent as CssJustifyContent, @@ -343,9 +343,46 @@ pub fn render_frame_v2_scaled( Ok(pixels) } -/// Build a `CssStyle` from an optional `SceneLayout` for root-level flex layout. -pub fn root_style(scene_layout: Option<&SceneLayout>) -> CssStyle { +/// The implicit layout a `world` scene gets when it declares no `layout` of +/// its own (round 4 audit, lot VALIDATION GÉOMÉTRIQUE, constat 4) — a +/// centred column, unlike a `slide` scene's implicit top-aligned column. +/// Single source of truth for both `render_world_frame_scaled` (which used +/// to inline this same struct literal) and `root_style`'s `ViewType::World` +/// branch below, so the geometry validator and the renderer can never see a +/// different default for the same (layout-absent) world scene. +fn world_default_scene_layout() -> SceneLayout { use crate::schema::{CardAlign, CardDirection, CardJustify}; + SceneLayout { + direction: Some(CardDirection::Column), + gap: Some(12.0), + align_items: Some(CardAlign::Center), + justify_content: Some(CardJustify::Center), + padding: None, + } +} + +/// Build a `CssStyle` from an optional `SceneLayout` for root-level flex +/// layout. `view_type` decides the fallback when `scene_layout` is absent: +/// a `slide` scene falls back to a plain top-aligned column (the historical +/// behaviour, byte-identical to before this parameter existed); a `world` +/// scene falls back to [`world_default_scene_layout`] — the same centred +/// layout `render_world_frame_scaled` synthesizes for a layout-absent world +/// scene. Before this, callers outside `scene.rs` (the geometry validator) +/// had no way to ask for the world fallback and always got the slide one, +/// validating a different root box than the one `render_world_frame_scaled` +/// actually lays out against. +pub fn root_style(scene_layout: Option<&SceneLayout>, view_type: ViewType) -> CssStyle { + use crate::schema::{CardAlign, CardDirection, CardJustify}; + let owned_world_default; + let scene_layout = match (scene_layout, view_type) { + (Some(layout), _) => Some(layout), + (None, ViewType::World) => { + owned_world_default = world_default_scene_layout(); + Some(&owned_world_default) + } + (None, ViewType::Slide) => None, + }; + let mut style = CssStyle::default(); style.display = Some(rustmotion_core::css::style::Display::Flex); @@ -431,7 +468,14 @@ fn render_with_new_pipeline_iter<'a, I>( // Mirror the legacy `root_style` so the new pipeline applies the same // scene-level flex configuration (direction, gap, padding, alignment). - let root_css = root_style(scene_layout); + // `ViewType::Slide` here: this helper is shared by the ordinary + // per-scene render path (always slide scenes — see `render_frame_v2_scaled` + // / `render_scene_fg_scaled`) AND `render_world_frame_scaled` below, but + // the latter always resolves its own `scene_layout` fallback (via + // `world_default_scene_layout`) *before* calling in here, so + // `scene_layout` is never `None` on that path — the `ViewType::World` + // branch is unreachable from this call site either way. + let root_css = root_style(scene_layout, ViewType::Slide); let anim = Some(BuildAnimationCtx { time: ctx.time, @@ -656,7 +700,10 @@ pub fn render_scene_hits( _ => None, }; - let root_css = root_style(scene.layout.as_ref()); + // `ViewType::Slide`: `render_scene_hits` is only ever called for + // `FrameTask::Normal` (slide-view scenes) — world frames build hits + // through a different path — see `encode/video/tasks.rs::render_frame_task_hits`. + let root_css = root_style(scene.layout.as_ref(), ViewType::Slide); let anim = Some(BuildAnimationCtx { time, scene_duration: scene.duration, @@ -939,14 +986,13 @@ pub fn render_world_frame_scaled( } // World scenes: force content children into centered flex flow. - // Decorative children (particles) are excluded from flex and rendered fullscreen. - let world_default_layout = crate::schema::SceneLayout { - direction: Some(crate::schema::CardDirection::Column), - gap: Some(12.0), - align_items: Some(crate::schema::CardAlign::Center), - justify_content: Some(crate::schema::CardJustify::Center), - padding: None, - }; + // Decorative children (particles) are excluded from flex and rendered + // fullscreen. `world_default_scene_layout` is the single source of + // truth for this fallback — `root_style`'s `ViewType::World` branch + // uses the exact same function, so the geometry validator sees the + // same root layout this render path lays out against (round 4 + // audit, constat 4). + let world_default_layout = world_default_scene_layout(); let scene_layout = scene.layout.as_ref().unwrap_or(&world_default_layout); let scene_children = deserialize_children(scene);