From 1836f9928ebfe6a67c286f65e6835fb72653579d Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sun, 9 Aug 2026 03:36:12 +0200 Subject: [PATCH] fix(validate): make the geometry pass call the engine instead of reimplementing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine confirmed audit findings on the geometry pass. Almost all of them are the same structural mistake: the validator forked a piece of the engine — a padding constant, a root style, a transform pivot — instead of calling the function the engine uses. Then the two drifted, and the validator started reasoning about a geometry that is not the one being rendered. The fix is mostly deletion of the duplicate, not correction of its value. False negatives — a broken video shipped with no warning: - `timeline` style states and audio-reactive transforms were invisible even under `--strict-anim`: the box tree was built once with `anim: None`, and both features only apply when a real animation context is present. It is now rebuilt per sample through `build_scene_from_refs` with a real `BuildAnimationCtx` — the same call `render_with_new_pipeline_iter` makes per rendered frame. - Animated rotation was not modelled at all: `transform_bbox` read only translate and scale, so a `spin` left the frame undetected at every sample. It now folds through `apply_static_node_transform`, the same 4-corner AABB the static path already used. - The 40-sample cap dropped `--strict-anim` to one sample every 1.5s on a 60s scene, so any excursion shorter than the step slipped through. Raised to 480, which holds the promised 8/s up to 60s. Measured cost on a 60s scene with 15 animated components: 309ms release, 543ms debug. False positives — a correct scenario blocked, and `--fix` then damaging it: - `unwrappable_text_overflow` was the only content-vs-own-box check outside the clipping-ancestor guard, so text legitimately clipped by an `overflow: hidden` ancestor was rejected and `--fix` stripped a correct `white-space`. - `transform-origin` was ignored by the static transform fold, which always pivoted at the box centre. - `content_overflows_card` is retired rather than patched. Its guard made it reachable only when neither the card nor anything between it and the node clips — which is exactly the `overflow: visible` case both CLAUDE.md and geometry-safety.md document as legal ("a badge sticking out of a card is legal"). When the card does clip, the guard already suppresses the whole block. There is no configuration where firing is both reachable and consistent with the documented contract. Content leaving the *device* is still caught — that is `check_viewport`'s job, and a test now pins that guarantee so the removal cannot silently become a blind spot. Wrong geometry reported: - `world` views were validated against the slide root layout. The centred default the world renderer synthesises is now a single shared `world_default_scene_layout`, used by both. - `check_auto_scroll` assumed 16+16px of padding and, for terminal, the wrong default font size and a CSS line-height the painter ignores. It now calls `CodeblockIntrinsic` and `TerminalIntrinsic`, the measurers layout itself uses. The `--fix` refusals from PR #145 were verified rather than rewritten: two end-to-end tests now drive `cmd_validate` over a templated scenario and a two-file `include` scenario, both with a real violation, and assert the source files come back byte-identical. Doing better than refusing is not possible for `include`, and not safe for templates: variable substitution can replace a scalar with an object, so a path computed on the resolved tree need not exist in the source. Tests: full workspace green on this branch alone. 7/8 examples validate; the eighth is issue #157, pre-existing on main. --- .../rustmotion-cli/src/commands/geometry.rs | 1426 ++++++++++++----- .../rustmotion-cli/src/commands/validate.rs | 138 ++ crates/rustmotion/src/engine/render/scene.rs | 72 +- 3 files changed, 1230 insertions(+), 406 deletions(-) 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);