From 726af253592427e9cfd10afdf7d4e4ec40c55210 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sun, 9 Aug 2026 02:19:09 +0200 Subject: [PATCH] fix(layout): resolve viewport units against the real viewport and run the cascade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five confirmed audit findings on CSS→taffy translation and intrinsic sizing. - `vw`/`vh` were resolved against a hardcoded 1920x1080 and `em`/`rem` against a hardcoded 16px, because both production layout sites passed `ConversionContext::default()` while the real dimensions sat in scope one line away. On a 1080x1920 vertical video — a routine format here — `50vw` came out 960px instead of 540px, a 78% error. A shared `viewport_conversion_context` now builds the context from the real viewport; at 1920x1080 it is bit-identical to the old default, so the common case does not move. - The CSS cascade was never executed. `cascade::inherit_from` was correct and tested, but nothing called it: `color` and `font-*` set on a container reached no child. It is now applied in `build_child` after position and before timeline states and animations, so an own value still beats an inherited one. - `box-sizing`, `justify-items` and `justify-self` were declared by the schema and never translated. They are now. `order` cannot be — taffy has no style-level reordering primitive, its `order` is source order assigned during layout — so it warns instead of vanishing, once per process rather than once per node per frame. - The default-size guard for components with no intrinsic measurer wrote a hardcoded width *and* height, each gated only by `is_none()` on its own axis, so an explicit `aspect-ratio` was overwritten: `width: 400` with `aspect-ratio: 16/9` produced height 80 instead of 225. The other axis is now derived from the ratio when one axis is explicit. - `rules/html-css-mental-model.md` taught `margin-top` / `margin-left` and `padding: [32, 48]`. `CssStyle` has `deny_unknown_fields` and only `margin: Edges`, so those forms fail deserialization and drop the whole component from the video. This is a rule read by the LLMs that generate scenarios: it was actively teaching them to write JSON that deletes components. Verified by running the CLI, not by reading the struct. Cascade and aspect-ratio both change how existing scenarios could render, so both were checked against every shipped example: no node inherits a property it was missing, and no example uses `aspect-ratio`. Tests: full workspace green on this branch alone. --- .../rustmotion/rules/html-css-mental-model.md | 11 +- .../rustmotion-components/src/box_builder.rs | 549 ++++++++++++------ .../html_css_mental_model_margin_padding.rs | 111 ++++ .../rustmotion-core/src/css/taffy_bridge.rs | 133 ++++- crates/rustmotion/src/engine/render/scene.rs | 29 +- crates/rustmotion/tests/viewport_units.rs | 66 +++ 6 files changed, 701 insertions(+), 198 deletions(-) create mode 100644 crates/rustmotion-components/tests/html_css_mental_model_margin_padding.rs create mode 100644 crates/rustmotion/tests/viewport_units.rs diff --git a/.claude/skills/rustmotion/rules/html-css-mental-model.md b/.claude/skills/rustmotion/rules/html-css-mental-model.md index 736b5b9..17619ac 100644 --- a/.claude/skills/rustmotion/rules/html-css-mental-model.md +++ b/.claude/skills/rustmotion/rules/html-css-mental-model.md @@ -83,17 +83,19 @@ Tout ce qui est **espace, alignement, distribution** se règle via les propriét | Besoin | Propriété | Sur quel élément | Exemple | |---|---|---|---| | Espace entre enfants frères | `gap` | Parent (flex ou grid) | `"gap": 24` | -| Espace entre contenu et bordure du container | `padding` | Le container lui-même | `"padding": 40` ou `"padding": [32, 48]` | -| Décaler UN seul enfant par rapport aux autres | `margin` | L'enfant en question | `"margin-top": 16` | +| Espace entre contenu et bordure du container | `padding` | Le container lui-même | `"padding": 40` ou `"padding": {"top": 32, "bottom": 32, "left": 48, "right": 48}` | +| Décaler UN seul enfant par rapport aux autres | `margin` | L'enfant en question | `"margin": {"top": 16}` | | Centrer horizontalement (axe principal = column) | `align-items: "center"` | Parent flex | `"align-items": "center"` | | Centrer verticalement (axe principal = column) | `justify-content: "center"` | Parent flex | `"justify-content": "center"` | -| Pousser un enfant à droite | `margin-left: "auto"` | Cet enfant | `"margin-left": "auto"` | +| Pousser un enfant à droite | `margin: {"left": "auto"}` | Cet enfant | `"margin": {"left": "auto"}` | | Élément prend tout l'espace restant | `flex-grow: 1` | L'enfant | `"flex-grow": 1` | | Alignement différent pour un seul enfant | `align-self` | L'enfant | `"align-self": "flex-end"` | | 2 colonnes égales | `grid-template-columns` | Parent grid | `["1fr","1fr"]` | | 3 colonnes proportionnelles | `grid-template-columns` | Parent grid | `["2fr","1fr","1fr"]` | | Colonne de taille fixe + reste | `grid-template-columns` | Parent grid | `[240, "1fr"]` | +**Piège `margin-top` / `margin-left` :** il n'existe **pas** de champ `margin-top`, `margin-left`, `margin-right`, `margin-bottom` séparé — seulement `margin: Option` (`CssStyle` a `deny_unknown_fields`, donc un `"margin-top"` fait échouer la désérialisation du composant entier, qui disparaît silencieusement de la vidéo). `margin` accepte soit une valeur uniforme (`"margin": 16`), soit un objet par côté avec les côtés omis valant 0 : `"margin": {"top": 16}`, `"margin": {"left": "auto"}`. `padding` a exactement la même forme (`padding: Option`) et la même limitation — pas de `padding-top` isolé, et pas de raccourci tableau `[v, h]` façon CSS shorthand : `"padding": {"top": 32, "bottom": 32, "left": 48, "right": 48}`, pas `"padding": [32, 48]`. + ### Règle de décision ``` @@ -174,8 +176,7 @@ Besoin d'une exception pour UN seul enfant ? { "type": "text", "position": "absolute", "x": 60, "y": 40 } // ✅ — padding sur le container, les enfants sont en flow -{ "type": "card", "style": { "padding": [40, 60], "gap": 24, "width": 900 }, "children": [...] } -// ↑top/bottom ↑left/right +{ "type": "card", "style": { "padding": { "top": 40, "bottom": 40, "left": 60, "right": 60 }, "gap": 24, "width": 900 }, "children": [...] } ``` --- diff --git a/crates/rustmotion-components/src/box_builder.rs b/crates/rustmotion-components/src/box_builder.rs index f1f0780..2ec06c0 100644 --- a/crates/rustmotion-components/src/box_builder.rs +++ b/crates/rustmotion-components/src/box_builder.rs @@ -142,6 +142,7 @@ where format!("/children/{i}"), 0.0, (1.0, 0.0), + &root_css, )); } @@ -226,6 +227,7 @@ fn build_ghosts<'a>( stagger_delay: f64, time_remap: (f64, f64), effects: &[AnimationEffect], + parent_css: &CssStyle, ) -> Vec { let (mb, tr) = detect_ghost_effects(effects); @@ -274,6 +276,9 @@ fn build_ghosts<'a>( if let Some(z) = child.z_index { css.z_index = Some(z); } + // Cascade: a ghost is the same component as the principal, painted + // at a different sampled time, so it inherits from the same parent. + rustmotion_core::css::cascade::inherit_from(parent_css, &mut css); // Apply timeline style states at the ghost time. if let Some(animatable) = child.component.as_animatable() { let steps = animatable.timeline_steps(); @@ -399,6 +404,7 @@ fn build_child<'a>( path: String, stagger_delay: f64, time_remap: (f64, f64), + parent_css: &CssStyle, ) -> Vec { // Compute the local animation context for this node — remapped by the // accumulated affine time transform from ancestor containers. @@ -428,6 +434,7 @@ fn build_child<'a>( stagger_delay, time_remap, &effects, + parent_css, ); } } @@ -450,6 +457,15 @@ fn build_child<'a>( css.z_index = Some(z); } + // CSS cascade (round 4 audit, lot LAYOUT, constat 2): propagate + // inheritable properties (color, font-*, text-align, white-space, ...) + // from the parent's already-cascaded style into any of this node's own + // unset properties — mirrors CSS's "specified value" resolution, which + // happens before state/animation overrides compute the final value. + // `crates/rustmotion-core/src/css/cascade.rs::inherit_from` existed but + // nothing called it until this fix. + rustmotion_core::css::cascade::inherit_from(parent_css, &mut css); + // Timeline style states: merge every state whose (at + stagger) <= t // into the box CSS. Opacity is excluded when a `transition` smooths it // (the synthesized keyframes then own its whole history). States affect @@ -569,6 +585,7 @@ fn build_child<'a>( &path, stagger_delay, time_remap, + &css, ); let intrinsic = component_intrinsic(&child.component); @@ -908,6 +925,7 @@ fn container_children<'a>( parent_path: &str, inherited_delay: f64, time_remap: (f64, f64), + parent_css: &CssStyle, ) -> Vec { let (children, stagger, child_scale, child_offset): (&[ChildComponent], Option, f64, f64) = match component { @@ -973,6 +991,7 @@ fn container_children<'a>( format!("{parent_path}/children/{j}"), inherited_delay + j as f64 * step, child_remap, + parent_css, )); } result @@ -1305,6 +1324,14 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { } } + // ── Round 4 audit, lot LAYOUT, constat 4: the 23-components block + // below (`Callout` through `Lottie`) now routes every default size + // through `apply_default_size`, which honours an explicit + // `aspect-ratio` (see its own doc comment) instead of the two + // guards below reaching separate, aspect-ratio-blind defaults — + // `width: 400` + `aspect-ratio: 16/9` used to still get the + // component's unrelated hardcoded default height (e.g. `shape`'s + // 80px) instead of the 225px the ratio implies. // ── #126 / W3: the 23 components with no size source ───────────── // // A card's default flex column gives every child its width via @@ -1345,12 +1372,11 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { CalloutArrowDirection::Left | CalloutArrowDirection::Right => (t.arrow_size, 0.0), CalloutArrowDirection::Top | CalloutArrowDirection::Bottom => (0.0, t.arrow_size), }; - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(text_w + h_pad * 2.0 + extra_w))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(line_h + v_pad + extra_h))); - } + apply_default_size( + css, + text_w + h_pad * 2.0 + extra_w, + line_h + v_pad + extra_h, + ); } Tooltip(t) => { // Same shape as Callout above; padding value borrowed from @@ -1368,12 +1394,11 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { (0.0, t.arrow_size) } }; - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(text_w + h_pad * 2.0 + extra_w))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(line_h + v_pad + extra_h))); - } + apply_default_size( + css, + text_w + h_pad * 2.0 + extra_w, + line_h + v_pad + extra_h, + ); } PillNav(p) => { // `height` is already a declared field on the component (like @@ -1382,24 +1407,17 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { // formula (h_pad = font_size*1.2 per side, `gap` before/after/ // between every pill) using the same public fields and the same // `measure_text_with_fallback` call it makes internally. - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(p.height))); - } - if css.width.is_none() { - let font_size = p.style.font_size_px_or(14.0); - let family = p.style.font_family_or("Inter"); - let h_pad = font_size * 1.2; - let n = p.items.len() as f32; - let labels_w: f32 = p - .items - .iter() - .map(|label| { - measure_text_line_width(label, font_size, family, false) + h_pad * 2.0 - }) - .sum(); - let total_w = labels_w + p.gap * (n + 1.0).max(1.0); - css.width = Some(CSize::Length(CLP::Px(total_w))); - } + let font_size = p.style.font_size_px_or(14.0); + let family = p.style.font_family_or("Inter"); + let h_pad = font_size * 1.2; + let n = p.items.len() as f32; + let labels_w: f32 = p + .items + .iter() + .map(|label| measure_text_line_width(label, font_size, family, false) + h_pad * 2.0) + .sum(); + let total_w = labels_w + p.gap * (n + 1.0).max(1.0); + apply_default_size(css, total_w, p.height); } Marquee(m) => { // Marquee's whole purpose is to scroll unbounded content, so @@ -1418,12 +1436,7 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { // `font_size: 24` paired with `style.height: 48`, i.e. // `2 × font_size`. let font_size = m.style.font_size_px_or(m.font_size); - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(800.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(font_size * 2.0))); - } + apply_default_size(css, 800.0, font_size * 2.0); } Stepper(s) => { // Same shape as `Timeline`'s formula above (r*2 + label metrics), @@ -1448,36 +1461,26 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { .fold(0.0_f32, f32::max); match s.orientation { StepperOrientation::Horizontal => { - if css.width.is_none() { - // Per-step allocation: the node needs ~3 diameters of - // breathing room (a common stepper-UI spacing - // convention), or enough for its longest label/desc, - // whichever is larger. - let per_step = (s.node_size * 3.0).max(max_label_w.max(max_desc_w) + 24.0); - css.width = Some(CSize::Length(CLP::Px(per_step * n))); - } - if css.height.is_none() { - let label_h = LABEL_FS * 1.3; - let desc_h = if has_desc { DESC_FS * 1.3 + 4.0 } else { 0.0 }; - let h = s.node_size + 4.0 + 12.0 + label_h + desc_h; - css.height = Some(CSize::Length(CLP::Px(h))); - } + // Per-step allocation: the node needs ~3 diameters of + // breathing room (a common stepper-UI spacing + // convention), or enough for its longest label/desc, + // whichever is larger. + let per_step = (s.node_size * 3.0).max(max_label_w.max(max_desc_w) + 24.0); + let label_h = LABEL_FS * 1.3; + let desc_h = if has_desc { DESC_FS * 1.3 + 4.0 } else { 0.0 }; + let h = s.node_size + 4.0 + 12.0 + label_h + desc_h; + apply_default_size(css, per_step * n, h); } StepperOrientation::Vertical => { - if css.width.is_none() { - let label_w = max_label_w.max(max_desc_w); - let w = s.node_size + 12.0 + label_w + 24.0; - css.width = Some(CSize::Length(CLP::Px(w))); - } - if css.height.is_none() { - let label_block = if has_desc { - LABEL_FS * 1.3 + DESC_FS * 1.3 + 8.0 - } else { - LABEL_FS * 1.3 + 8.0 - }; - let per_step = (s.node_size * 2.0).max(label_block); - css.height = Some(CSize::Length(CLP::Px(per_step * n))); - } + let label_w = max_label_w.max(max_desc_w); + let w = s.node_size + 12.0 + label_w + 24.0; + let label_block = if has_desc { + LABEL_FS * 1.3 + DESC_FS * 1.3 + 8.0 + } else { + LABEL_FS * 1.3 + 8.0 + }; + let per_step = (s.node_size * 2.0).max(label_block); + apply_default_size(css, w, per_step * n); } } } @@ -1512,12 +1515,7 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { let lines = (total_w / box_w).ceil().max(1.0); let line_h = tc.max_font_size * 1.3; let box_h = lines * line_h + (lines - 1.0).max(0.0) * V_GAP; - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(box_w))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(box_h))); - } + apply_default_size(css, box_w, box_h); } } Heatmap(h) => { @@ -1528,25 +1526,15 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { let rows = h.data.len(); let cols = h.data.iter().map(|r| r.len()).max().unwrap_or(0); let step = h.cell_size + h.cell_gap; - if css.width.is_none() { - let w = (cols.max(1) as f32 - 1.0).max(0.0) * step + h.cell_size; - css.width = Some(CSize::Length(CLP::Px(w))); - } - if css.height.is_none() { - let hh = (rows.max(1) as f32 - 1.0).max(0.0) * step + h.cell_size; - css.height = Some(CSize::Length(CLP::Px(hh))); - } + let w = (cols.max(1) as f32 - 1.0).max(0.0) * step + h.cell_size; + let hh = (rows.max(1) as f32 - 1.0).max(0.0) * step + h.cell_size; + apply_default_size(css, w, hh); } Sparkline(_) => { // "Sparkline: no axes, no labels, compact (120x40 default), // inline use" — documented in // .claude/skills/rustmotion/rules/data-viz-components.md. - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(120.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(40.0))); - } + apply_default_size(css, 120.0, 40.0); } Stat(_) => { // Documented default from @@ -1555,12 +1543,7 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { // the fix for the issue's second bug: three `stat`s in a flex // row with no explicit size rendered zero pixels because width // (not just height) collapsed to 0 in a row context. - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(280.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(180.0))); - } + apply_default_size(css, 280.0, 180.0); } Gauge(g) => { // Square — gauge.rs's own paint() derives its ring radius from @@ -1575,12 +1558,7 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { // icon-sizing-hierarchy.md. const TARGET_RADIUS: f32 = 88.0; let size = 2.0 * (TARGET_RADIUS + g.track_width / 2.0 + 4.0); - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(size))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(size))); - } + apply_default_size(css, size, size); } DotMap(_) => { // 2:1 — the standard aspect ratio for an equirectangular world @@ -1588,24 +1566,14 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { // dot_map.rs's own `geo_to_screen` implements. dot_map.rs always // paints a full-box background rect first, so any positive size // shows ink even with zero points. - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(640.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(320.0))); - } + apply_default_size(css, 640.0, 320.0); } Comparison(_) => { // No natural intrinsic size (the painter just splits whatever // box it's given at the divider) — matches this project's own // reference usage in examples/mega-showcase.json's `comparison` // block. - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(520.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(280.0))); - } + apply_default_size(css, 520.0, 280.0); } Treemap(_) => { // Slice-and-dice treemap fills whatever box it's given — matches @@ -1613,12 +1581,7 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { // examples/mega-showcase.json's `treemap` block (near-square, // the conventional treemap aspect since its rectangles are area- // proportional in both axes). - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(416.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(368.0))); - } + apply_default_size(css, 416.0, 368.0); } Chart(c) => { // Pie/donut/radar/radial_bar are inherently circular — a square @@ -1631,12 +1594,12 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { c.chart_type, ChartType::Pie | ChartType::Donut | ChartType::Radar | ChartType::RadialBar ); - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(if round { 320.0 } else { 400.0 }))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(if round { 320.0 } else { 300.0 }))); - } + let (dw, dh) = if round { + (320.0, 320.0) + } else { + (400.0, 300.0) + }; + apply_default_size(css, dw, dh); } Skeleton(s) => { // `rectangle`: documented default from data-viz-components.md's @@ -1648,31 +1611,12 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { // formula above — matches skeleton.rs's own per-line paint loop // (`y = i * (line_height + line_gap)`) exactly. match s.variant { - SkeletonVariant::Rectangle => { - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(400.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(200.0))); - } - } - SkeletonVariant::Circle => { - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(64.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(64.0))); - } - } + SkeletonVariant::Rectangle => apply_default_size(css, 400.0, 200.0), + SkeletonVariant::Circle => apply_default_size(css, 64.0, 64.0), SkeletonVariant::Text => { let n = s.lines.max(1) as f32; - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(240.0))); - } - if css.height.is_none() { - let h = n * s.line_height + (n - 1.0).max(0.0) * s.line_gap; - css.height = Some(CSize::Length(CLP::Px(h))); - } + let h = n * s.line_height + (n - 1.0).max(0.0) * s.line_gap; + apply_default_size(css, 240.0, h); } } } @@ -1686,12 +1630,7 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { MockupDevice::Laptop => (640.0, 400.0), MockupDevice::Browser => (640.0, 360.0), }; - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(dw))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(dh))); - } + apply_default_size(css, dw, dh); } Icon(_) => { // 64×64 — the midpoint of the documented "card / feature icon" @@ -1699,12 +1638,7 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { // icon-sizing-hierarchy.md (desktop 40–56px, mobile 72–96px, // square 60–80px), and a size icon asset systems near-universally // ship as a default export (24/32/48/64 being the common family). - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(64.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(64.0))); - } + apply_default_size(css, 64.0, 64.0); } Svg(_) => { // 200×200 — square, since an arbitrary vector graphic (icon, @@ -1712,12 +1646,7 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { // common equal-aspect SVG viewBox convention and sits above // Icon's 64px "card icon" role for the more elaborate content // `svg` typically carries (illustrations/diagrams, not glyphs). - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(200.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(200.0))); - } + apply_default_size(css, 200.0, 200.0); } Shape(_) => { // 80×80 — matches the median of this project's own decorative @@ -1726,51 +1655,92 @@ fn apply_intrinsic_overrides(component: &Component, css: &mut CssStyle) { // (26, 36, 44, 60, 70, 140 — median ~55, rounded up for // visibility as a standalone default rather than a same-scene // accent tuned against neighbours). - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(80.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(80.0))); - } + apply_default_size(css, 80.0, 80.0); } Image(_) => { // 4:3 (400×300) — the traditional default photo aspect ratio, // distinct from Video/Gif's 16:9 below so a generic still image // doesn't presume widescreen framing. - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(400.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(300.0))); - } + apply_default_size(css, 400.0, 300.0); } Video(_) | Gif(_) => { // 16:9 (400×225) — the industry-standard video aspect ratio // (matches every render resolution this project documents: // 1920×1080, 1280×720), scaled down to a card-sized default. - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(400.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(225.0))); - } + apply_default_size(css, 400.0, 225.0); } Lottie(_) => { // 300×300 — square, matching the aspect the vast majority of // Lottie animation assets ship at (LottieFiles' own marketplace // preview convention is a 1:1 canvas). - if css.width.is_none() { - css.width = Some(CSize::Length(CLP::Px(300.0))); - } - if css.height.is_none() { - css.height = Some(CSize::Length(CLP::Px(300.0))); - } + apply_default_size(css, 300.0, 300.0); } _ => {} } } +/// Apply a component's natural default size (`dw` × `dh`) to `css`, honouring +/// an explicit `aspect-ratio` instead of always falling back to `dw`/`dh` +/// independently (round 4 audit, lot LAYOUT, constat 4 — the previous code +/// guarded each axis with its own `is_none()` check and never looked at +/// `aspect-ratio`, so `width: 400` + `aspect-ratio: 16/9` still got the +/// component's unrelated hardcoded default height instead of 225). +/// +/// - Both axes already set: untouched (the author fully specified the box). +/// - One axis set to a fixed pixel length, `aspect-ratio` present: the other +/// axis is derived from it (`h = w / ratio` or `w = h * ratio`) — the CSS +/// replaced-element sizing rule for a single definite axis plus a +/// preferred aspect ratio. +/// - Neither axis set: the natural default width is kept (there is no +/// author-declared axis to derive from), and height is derived from +/// `aspect-ratio` when present, the natural default height otherwise. +/// +/// `min-*`/`max-*` need no equivalent guard here: taffy clamps the final +/// used size against them at layout time regardless of what `size` resolves +/// to (`style.min_size`/`max_size` in `taffy_bridge::to_taffy_style`), so a +/// default below `min-width` is corrected downstream, not silently wrong. +fn apply_default_size(css: &mut CssStyle, dw: f32, dh: f32) { + let ratio = css.aspect_ratio.filter(|r| *r > 0.0); + match (css.width.is_some(), css.height.is_some()) { + (true, true) => {} + (true, false) => { + let h = fixed_px(css.width.as_ref()) + .zip(ratio) + .map(|(w, r)| w / r) + .unwrap_or(dh); + css.height = Some(CSize::Length(CLP::Px(h))); + } + (false, true) => { + let w = fixed_px(css.height.as_ref()) + .zip(ratio) + .map(|(h, r)| h * r) + .unwrap_or(dw); + css.width = Some(CSize::Length(CLP::Px(w))); + } + (false, false) => { + css.width = Some(CSize::Length(CLP::Px(dw))); + let h = ratio.map(|r| dw / r).unwrap_or(dh); + css.height = Some(CSize::Length(CLP::Px(h))); + } + } +} + +/// Extract a fixed pixel value from a `Size`, if it resolves to one without a +/// `LengthContext` (only `Size::Length(LengthPercentage::Px(_))` — a bare +/// number or `"NNpx"`). `%`/`vw`/`vh`/`em`/`rem` and `auto` return `None`: +/// `apply_default_size` can't derive a ratio from a length it can't resolve +/// at build time, so it falls back to the component's hardcoded default. +fn fixed_px(size: Option<&CSize>) -> Option { + match size? { + CSize::Length(lp) => match lp.try_parse()? { + rustmotion_core::css::units::ParsedLength::Px(v) => Some(v), + _ => None, + }, + _ => None, + } +} + /// Borrow the `CssStyle` from any component. fn component_style(c: &Component) -> &CssStyle { use Component::*; @@ -1947,6 +1917,27 @@ mod tests { } } + fn make_text(content: &str, style: CssStyle) -> ChildComponent { + ChildComponent { + component: Component::Text(crate::text::Text { + content: content.to_string(), + max_width: None, + timing: Default::default(), + style, + timeline: Vec::new(), + stagger: None, + text_shadow: None, + stroke: None, + text_background: None, + }), + position: None, + x: None, + y: None, + z_index: None, + bleed: false, + } + } + #[test] fn empty_scene_has_only_root() { let built = build_scene(&[], (1920.0, 1080.0)); @@ -2752,4 +2743,192 @@ mod tests { ); } } + + // ── Round 4 audit, lot LAYOUT, constat 2: the CSS cascade is wired ────── + // `crates/rustmotion-core/src/css/cascade.rs::inherit_from` existed but + // nothing called it — `color`/`font-*` set on a container never reached + // children lacking their own value. + + #[test] + fn card_color_cascades_to_text_child_with_no_color_of_its_own() { + use rustmotion_core::css::style::Color; + + let card = make_card( + vec![make_text("hello", CssStyle::default())], + CssStyle { + color: Some(Color::String("#ff0000".into())), + ..Default::default() + }, + ); + let scene = vec![ChildComponent { + component: card, + position: Some(crate::PositionMode::Absolute { x: 0.0, y: 0.0 }), + x: None, + y: None, + z_index: None, + bleed: false, + }]; + let built = build_scene(&scene, (800.0, 600.0)); + let text_box = &built.root.children[0].children[0]; + assert_eq!( + text_box.css.color, + Some(Color::String("#ff0000".into())), + "text child declares no color of its own — it should inherit the card's" + ); + } + + #[test] + fn text_own_color_wins_over_inherited_card_color() { + use rustmotion_core::css::style::Color; + + let card = make_card( + vec![make_text( + "hello", + CssStyle { + color: Some(Color::String("#00ff00".into())), + ..Default::default() + }, + )], + CssStyle { + color: Some(Color::String("#ff0000".into())), + ..Default::default() + }, + ); + let scene = vec![ChildComponent { + component: card, + position: Some(crate::PositionMode::Absolute { x: 0.0, y: 0.0 }), + x: None, + y: None, + z_index: None, + bleed: false, + }]; + let built = build_scene(&scene, (800.0, 600.0)); + let text_box = &built.root.children[0].children[0]; + assert_eq!( + text_box.css.color, + Some(Color::String("#00ff00".into())), + "text child's own explicit color must win over the inherited card color" + ); + } + + #[test] + fn card_display_does_not_cascade_to_text_child() { + // `display` is not an inheritable CSS property — only the documented + // inheritable list (color, font-*, text-align, white-space, ...) + // should propagate. + let card = make_card( + vec![make_text("hello", CssStyle::default())], + CssStyle { + display: Some(Display::Flex), + ..Default::default() + }, + ); + let scene = vec![ChildComponent { + component: card, + position: Some(crate::PositionMode::Absolute { x: 0.0, y: 0.0 }), + x: None, + y: None, + z_index: None, + bleed: false, + }]; + let built = build_scene(&scene, (800.0, 600.0)); + let text_box = &built.root.children[0].children[0]; + assert_eq!(text_box.css.display, None); + } + + // ── Round 4 audit, lot LAYOUT, constat 4: `apply_intrinsic_overrides`'s + // default size ignored an explicit `aspect-ratio`. ───────────────────── + + fn make_aspect_shape(width: f32, aspect_ratio: f32) -> ChildComponent { + ChildComponent { + component: Component::Shape(crate::shape::Shape { + shape: rustmotion_core::schema::ShapeType::Rect, + text: None, + timing: Default::default(), + style: CssStyle { + width: Some(CSize::Length(CLP::Px(width))), + aspect_ratio: Some(aspect_ratio), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + fill: None, + stroke: None, + }), + position: Some(crate::PositionMode::Absolute { x: 0.0, y: 0.0 }), + x: None, + y: None, + z_index: None, + bleed: false, + } + } + + #[test] + fn explicit_width_with_aspect_ratio_derives_height_instead_of_the_hardcoded_default() { + // `shape`'s hardcoded default is 80×80 (see `apply_intrinsic_overrides`). + // `width: 400` + `aspect-ratio: 16/9` should derive height = 225, not + // fall back to the unrelated 80px default. + let scene = vec![make_aspect_shape(400.0, 16.0 / 9.0)]; + let built = build_scene(&scene, (1920.0, 1080.0)); + let layout = run_layout(&built.root, (1920.0, 1080.0), &ConversionContext::default()); + let l = layout + .get(built.root.children[0].id) + .expect("shape laid out"); + assert!( + (l.width - 400.0).abs() < 1.0, + "width should stay the author's explicit 400, got {}", + l.width + ); + assert!( + (l.height - 225.0).abs() < 1.0, + "height should derive from width/aspect-ratio (400/1.778=225), got {}", + l.height + ); + } + + #[test] + fn neither_axis_set_with_aspect_ratio_derives_height_from_the_default_width() { + // No width/height at all: the natural default width (80 for shape) + // is kept, but height should come from the aspect-ratio, not the + // unrelated 80px default. + let scene = vec![make_aspect_shape_no_width(2.0)]; + let built = build_scene(&scene, (1920.0, 1080.0)); + let layout = run_layout(&built.root, (1920.0, 1080.0), &ConversionContext::default()); + let l = layout + .get(built.root.children[0].id) + .expect("shape laid out"); + assert!( + (l.width - 80.0).abs() < 1.0, + "width should keep the natural default (80), got {}", + l.width + ); + assert!( + (l.height - 40.0).abs() < 1.0, + "height should derive from the default width/aspect-ratio (80/2=40), got {}", + l.height + ); + } + + fn make_aspect_shape_no_width(aspect_ratio: f32) -> ChildComponent { + ChildComponent { + component: Component::Shape(crate::shape::Shape { + shape: rustmotion_core::schema::ShapeType::Rect, + text: None, + timing: Default::default(), + style: CssStyle { + aspect_ratio: Some(aspect_ratio), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + fill: None, + stroke: None, + }), + position: Some(crate::PositionMode::Absolute { x: 0.0, y: 0.0 }), + x: None, + y: None, + z_index: None, + bleed: false, + } + } } diff --git a/crates/rustmotion-components/tests/html_css_mental_model_margin_padding.rs b/crates/rustmotion-components/tests/html_css_mental_model_margin_padding.rs new file mode 100644 index 0000000..3a2547e --- /dev/null +++ b/crates/rustmotion-components/tests/html_css_mental_model_margin_padding.rs @@ -0,0 +1,111 @@ +//! Regression test — audit round 4, lot LAYOUT, constat 5. +//! +//! `.claude/skills/rustmotion/rules/html-css-mental-model.md` — a rules file +//! read by the LLMs that generate rustmotion scenarios — documented +//! `"margin-top"` / `"margin-left"` as valid per-child JSON keys, and +//! `"padding": [40, 60]` as a valid CSS-shorthand-style array. Neither is: +//! `CssStyle` only exposes `margin: Option` / `padding: Option` +//! (`#[serde(deny_unknown_fields)]`), and `Edges` is `Uniform(LengthPercentage)` +//! or a `{top, right, bottom, left}` object — never a per-key kebab-case field, +//! never an array. Following the old doc verbatim makes the whole component +//! fail typed deserialization, so it silently disappears from the render +//! instead of being spaced as intended. +//! +//! These tests pin both failure modes (so a future change can't quietly +//! re-introduce them) and confirm the corrected syntax the doc now teaches +//! actually round-trips. + +use rustmotion_components::Component; + +fn component_json_parses(json: serde_json::Value) -> bool { + serde_json::from_value::(json).is_ok() +} + +#[test] +fn old_doc_margin_top_shorthand_fails_to_deserialize() { + // What the doc used to teach (html-css-mental-model.md, old line 87): + // `"margin-top": 16` as a direct style key. + let json = serde_json::json!({ + "type": "text", + "content": "hi", + "style": { "margin-top": 16 } + }); + let err = serde_json::from_value::(json) + .expect_err("margin-top must not deserialize — printed below for the audit red-phase log"); + eprintln!("captured deserialize error (constat 5 red phase): {err}"); + assert!( + err.to_string().contains("margin-top") || err.to_string().contains("unknown field"), + "expected an unknown-field error mentioning the rejected key, got: {err}" + ); +} + +#[test] +fn old_doc_margin_left_auto_shorthand_fails_to_deserialize() { + // What the doc used to teach (html-css-mental-model.md, old line 90): + // `"margin-left": "auto"` as a direct style key. + let json = serde_json::json!({ + "type": "badge", + "text": "NEW", + "style": { "margin-left": "auto" } + }); + assert!( + !component_json_parses(json), + "`margin-left` is not a CssStyle field — same failure mode as margin-top" + ); +} + +#[test] +fn old_doc_padding_array_shorthand_fails_to_deserialize() { + // What the doc used to teach (html-css-mental-model.md, old line 177): + // `"padding": [40, 60]`, a CSS `padding: 40px 60px` -style array shorthand + // `Edges` does not implement. + let json = serde_json::json!({ + "type": "card", + "style": { "padding": [40, 60] }, + "children": [] + }); + assert!( + !component_json_parses(json), + "`padding` only accepts a uniform scalar or a {{top,right,bottom,left}} \ + object, never an array — the old doc's example should fail" + ); +} + +#[test] +fn corrected_margin_object_syntax_round_trips() { + let json = serde_json::json!({ + "type": "text", + "content": "hi", + "style": { "margin": { "top": 16 } } + }); + assert!( + component_json_parses(json), + "the doc's corrected `\"margin\": {{\"top\": 16}}` syntax must actually parse" + ); +} + +#[test] +fn corrected_margin_left_auto_object_syntax_round_trips() { + let json = serde_json::json!({ + "type": "badge", + "text": "NEW", + "style": { "margin": { "left": "auto" } } + }); + assert!( + component_json_parses(json), + "the doc's corrected `\"margin\": {{\"left\": \"auto\"}}` syntax must actually parse" + ); +} + +#[test] +fn corrected_padding_object_syntax_round_trips() { + let json = serde_json::json!({ + "type": "card", + "style": { "padding": { "top": 40, "bottom": 40, "left": 60, "right": 60 } }, + "children": [] + }); + assert!( + component_json_parses(json), + "the doc's corrected per-side `padding` object syntax must actually parse" + ); +} diff --git a/crates/rustmotion-core/src/css/taffy_bridge.rs b/crates/rustmotion-core/src/css/taffy_bridge.rs index c7a27b9..9376ebb 100644 --- a/crates/rustmotion-core/src/css/taffy_bridge.rs +++ b/crates/rustmotion-core/src/css/taffy_bridge.rs @@ -9,9 +9,9 @@ use taffy::prelude as tf; use super::style::{ - AlignContent, AlignItems, AlignSelf, CssStyle, Display, Edges, FlexDirection, FlexWrap, Gap, - GridAutoFlow, GridLine, GridLineEnd, GridTrack, GridTrackKeyword, JustifyContent, Overflow, - Position, Size, + AlignContent, AlignItems, AlignSelf, BoxSizing, CssStyle, Display, Edges, FlexDirection, + FlexWrap, Gap, GridAutoFlow, GridLine, GridLineEnd, GridTrack, GridTrackKeyword, + JustifyContent, JustifyItems, JustifySelf, Overflow, Position, Size, }; use super::units::{LengthContext, LengthPercentage, ParsedLength}; @@ -65,6 +65,18 @@ pub fn to_taffy_style(css: &CssStyle, ctx: &ConversionContext) -> tf::Style { }; style.aspect_ratio = css.aspect_ratio; + // `box-sizing` (round 4 audit, lot LAYOUT, constat 3): taffy supports it + // natively (`Style::box_sizing`, default `BorderBox`) — schema-valid but + // untranslated before this fix, so `content-box` was silently ignored + // and every sized box behaved as `border-box` regardless of what the + // author declared. + if let Some(bs) = css.box_sizing { + style.box_sizing = match bs { + BoxSizing::ContentBox => tf::BoxSizing::ContentBox, + BoxSizing::BorderBox => tf::BoxSizing::BorderBox, + }; + } + // Margin / padding / border (border WIDTH only — border style/color are paint props) style.margin = edges_to_rect_lpa(css.margin.as_ref(), ctx); style.padding = edges_to_rect_lp(css.padding.as_ref(), ctx); @@ -114,6 +126,29 @@ pub fn to_taffy_style(css: &CssStyle, ctx: &ConversionContext) -> tf::Style { if let Some(basis) = css.flex_basis.as_ref() { style.flex_basis = size_to_dim(Some(basis), ctx); } + // `order` (round 4 audit, lot LAYOUT, constat 3): schema-valid but has no + // taffy equivalent — taffy has no flex/grid item-reordering primitive + // (its internal `order` on `Layout` is source order, assigned during + // layout, not settable via `Style`). Translating is not possible, so — + // per the same "fail loud instead of a silent no-op" contract this + // module's `Length`/`LengthPercentage` parsing already uses (see + // `units.rs`'s `px_or_warn` / `parse_length_or_warn`) — warn instead of + // dropping it without a trace. Reorder the JSON `children` array itself + // to get the equivalent effect. + // Emitted at most once per process: `to_taffy_style` runs per node per + // layout pass, and layout runs per frame — an unguarded `eprintln!` here + // would print the same line a thousand times over a single render and + // slow it down while doing so. + if css.order.is_some() { + static WARNED_ORDER: std::sync::Once = std::sync::Once::new(); + WARNED_ORDER.call_once(|| { + eprintln!( + "Warning: `order` is not supported by the layout engine (no flex/grid item \ + reordering primitive) — it is ignored. Reorder the component's JSON `children` \ + array instead to change paint/layout order." + ); + }); + } // Gap if let Some(gap) = css.gap.as_ref() { @@ -154,6 +189,17 @@ pub fn to_taffy_style(css: &CssStyle, ctx: &ConversionContext) -> tf::Style { if let Some(gr) = css.grid_row.as_ref() { style.grid_row = grid_placement_line(gr); } + // `justify-items` / `justify-self` (round 4 audit, lot LAYOUT, constat 3): + // taffy supports both natively for grid children, reusing the same + // `AlignItems`/`AlignSelf` types as `align-items`/`align-self` (the + // block-axis equivalents) — same untranslated-but-schema-valid gap as + // `box-sizing` above. + if let Some(ji) = css.justify_items { + style.justify_items = Some(justify_items_to_taffy(ji)); + } + if let Some(js) = css.justify_self { + style.justify_self = justify_self_to_taffy(js); + } // Overflow if let Some(o) = css.overflow { @@ -191,6 +237,33 @@ fn align_self_to_taffy(a: AlignSelf) -> Option { }) } +fn justify_items_to_taffy(j: JustifyItems) -> tf::AlignItems { + match j { + JustifyItems::Stretch => tf::AlignItems::Stretch, + JustifyItems::Start => tf::AlignItems::Start, + JustifyItems::End => tf::AlignItems::End, + JustifyItems::Center => tf::AlignItems::Center, + // `legacy` (old CSS2-era grid keyword, only meaningful combined with + // `left`/`right`/`center` which this schema doesn't expose) has no + // taffy analog; `Start` is the closest normal-flow behaviour and + // matches this bridge's own `Auto`-ish fallbacks elsewhere. + JustifyItems::Legacy => tf::AlignItems::Start, + } +} + +fn justify_self_to_taffy(j: JustifySelf) -> Option { + Some(match j { + // `auto` computes to the parent's `justify-items` — `None` is + // exactly how this bridge already models `align-self: auto` + // inheriting `align-items` above. + JustifySelf::Auto => return None, + JustifySelf::Stretch => tf::AlignSelf::Stretch, + JustifySelf::Start => tf::AlignSelf::Start, + JustifySelf::End => tf::AlignSelf::End, + JustifySelf::Center => tf::AlignSelf::Center, + }) +} + fn align_content_to_taffy(a: AlignContent) -> tf::AlignContent { match a { AlignContent::Stretch => tf::AlignContent::Stretch, @@ -724,4 +797,58 @@ mod tests { "each 1fr column should be ~300px wide, got {w1}" ); } + + // ── Round 4 audit, lot LAYOUT, constat 3: box-sizing / justify-items / + // justify-self are schema-valid but were never translated to taffy. ──── + + #[test] + fn box_sizing_content_box_is_translated() { + let css = CssStyle { + box_sizing: Some(BoxSizing::ContentBox), + ..Default::default() + }; + let s = to_taffy_style(&css, &ctx()); + assert_eq!(s.box_sizing, tf::BoxSizing::ContentBox); + } + + #[test] + fn box_sizing_defaults_to_border_box() { + let css = CssStyle::default(); + let s = to_taffy_style(&css, &ctx()); + assert_eq!(s.box_sizing, tf::BoxSizing::BorderBox); + } + + #[test] + fn justify_items_is_translated_for_grid_children() { + let css = CssStyle { + display: Some(Display::Grid), + justify_items: Some(JustifyItems::Center), + ..Default::default() + }; + let s = to_taffy_style(&css, &ctx()); + assert_eq!(s.justify_items, Some(tf::AlignItems::Center)); + } + + #[test] + fn justify_self_is_translated() { + let css = CssStyle { + justify_self: Some(JustifySelf::End), + ..Default::default() + }; + let s = to_taffy_style(&css, &ctx()); + assert_eq!(s.justify_self, Some(tf::AlignSelf::End)); + } + + #[test] + fn justify_self_auto_falls_back_to_parent_justify_items() { + // `auto` computes to the parent's `justify-items` — taffy models + // this the same way `align-self: auto` models inheriting + // `align-items`: `None`, not an explicit `Start`. + let css = CssStyle { + justify_self: Some(JustifySelf::Auto), + ..Default::default() + }; + let s = to_taffy_style(&css, &ctx()); + assert_eq!(s.justify_self, None); + } } diff --git a/crates/rustmotion/src/engine/render/scene.rs b/crates/rustmotion/src/engine/render/scene.rs index 3ff5839..0a5ab7a 100644 --- a/crates/rustmotion/src/engine/render/scene.rs +++ b/crates/rustmotion/src/engine/render/scene.rs @@ -10,11 +10,32 @@ use rustmotion_core::css::style::{ AlignItems as CssAlignItems, CssStyle, Edges, FlexDirection as CssFlexDirection, Gap, JustifyContent as CssJustifyContent, }; -use rustmotion_core::css::units::LengthPercentage; +use rustmotion_core::css::taffy_bridge::ConversionContext; +use rustmotion_core::css::units::{LengthContext, LengthPercentage}; use rustmotion_core::engine::animator::safe_div; use rustmotion_core::engine::paint_pass::PlaneCamera; use rustmotion_core::engine::renderer::color4f_from_hex; +/// Build the `ConversionContext` that resolves `vw`/`vh`/`%` units for a +/// layout pass, anchored to the *real* output viewport instead of +/// `ConversionContext::default()`'s hardcoded 1920×1080 (round 4 audit, +/// lot LAYOUT, constat 1). On a 1080×1920 vertical video — a resolution +/// this project documents as a common target — `width: "50vw"` used to +/// resolve as 50% of a phantom 1920px-wide viewport (960px) instead of 50% +/// of the real 1080px one (540px), a 78% error, and `vh` was off by the +/// same margin in the other axis. `font-size`/`root-font-size` stay at the +/// CSS initial `16px`: nothing upstream of this call resolves and threads a +/// root font-size through yet. +fn viewport_conversion_context(viewport_w: f32, viewport_h: f32) -> ConversionContext { + ConversionContext { + length: LengthContext { + viewport_width: viewport_w, + viewport_height: viewport_h, + ..LengthContext::default() + }, + } +} + /// Internal render-time context — bundles per-scene timing/dimension info that /// the scene renderer threads down into its helpers. This is intentionally /// private to the scene renderer; component painters receive `PaintCtx`. @@ -405,7 +426,6 @@ fn render_with_new_pipeline_iter<'a, I>( { use rustmotion_components::box_builder::{build_scene_from_refs, BuildAnimationCtx}; use rustmotion_components::legacy_dispatch::LegacyPaintDispatcher; - use rustmotion_core::css::taffy_bridge::ConversionContext; use rustmotion_core::engine::layout_pass::run_layout; use rustmotion_core::engine::paint_pass::{paint_tree, PaintFrame}; @@ -422,7 +442,7 @@ fn render_with_new_pipeline_iter<'a, I>( let layout = run_layout( &built.root, (viewport_w, viewport_h), - &ConversionContext::default(), + &viewport_conversion_context(viewport_w, viewport_h), ); let dispatcher = LegacyPaintDispatcher::for_scene(&built); let frame = PaintFrame { @@ -597,7 +617,6 @@ pub fn render_scene_hits( build_scene_from_refs, component_kind, BuildAnimationCtx, }; use rustmotion_components::legacy_dispatch::LegacyPaintDispatcher; - use rustmotion_core::css::taffy_bridge::ConversionContext; use rustmotion_core::engine::layout_pass::run_layout; use rustmotion_core::engine::paint_pass::{paint_tree_with_hits, EnrichedHit, PaintFrame}; @@ -644,7 +663,7 @@ pub fn render_scene_hits( fps: config.fps, }); let built = build_scene_from_refs(children.iter(), (vw, vh), root_css, anim); - let layout = run_layout(&built.root, (vw, vh), &ConversionContext::default()); + let layout = run_layout(&built.root, (vw, vh), &viewport_conversion_context(vw, vh)); let dispatcher = LegacyPaintDispatcher::for_scene(&built); let frame = PaintFrame { time, diff --git a/crates/rustmotion/tests/viewport_units.rs b/crates/rustmotion/tests/viewport_units.rs new file mode 100644 index 0000000..b23ea0b --- /dev/null +++ b/crates/rustmotion/tests/viewport_units.rs @@ -0,0 +1,66 @@ +//! Regression test — audit round 4, lot LAYOUT, constat 1. +//! +//! `vw`/`vh` (and `%`, `em`, `rem`) used to resolve against a hardcoded +//! 1920×1080 viewport and a hardcoded 16px font-size, no matter what +//! resolution the video was actually configured at. The culprit was +//! `ConversionContext::default()` being passed to `run_layout` in +//! `crates/rustmotion/src/engine/render/scene.rs`, instead of a context +//! built from the real `VideoConfig::{width,height}`. +//! +//! Vertical video (1080×1920, a common format for this project) is the +//! sharpest reproduction: `width: "50vw"` should be 540px (50% of the real +//! 1080px-wide viewport), not 960px (50% of the hardcoded 1920). Likewise +//! `height: "50vh"` should be 960px (50% of 1920), not 540px (50% of the +//! hardcoded 1080) — the bug doesn't just miscalculate, it silently swaps +//! which axis gets which fraction. + +use rustmotion::engine::render::render_scene_hits; +use rustmotion::schema::{Scene, VideoConfig}; + +fn vertical_config() -> VideoConfig { + serde_json::from_value(serde_json::json!({ + "width": 1080, + "height": 1920, + "fps": 30 + })) + .expect("video config is schema-valid") +} + +fn scene_with_vw_vh_shape() -> Scene { + serde_json::from_value(serde_json::json!({ + "duration": 1.0, + "children": [ + { + "type": "shape", + "shape": "rect", + "style": { "width": "50vw", "height": "50vh" } + } + ] + })) + .expect("scene is schema-valid") +} + +#[test] +fn vw_and_vh_resolve_against_the_real_video_viewport() { + let config = vertical_config(); + let scene = scene_with_vw_vh_shape(); + + let hits = render_scene_hits(&config, &scene, 0); + let shape_hit = hits + .iter() + .find(|h| h.kind == "shape") + .expect("shape hit present in render_scene_hits output"); + + // 50% of the real 1080px-wide / 1920px-tall viewport — not 50% of a + // hardcoded 1920×1080 (which would swap the two: 960 / 540). + assert!( + (shape_hit.rect.w - 540.0).abs() < 1.0, + "expected 50vw on a 1080px-wide viewport to resolve to ~540px, got {}", + shape_hit.rect.w + ); + assert!( + (shape_hit.rect.h - 960.0).abs() < 1.0, + "expected 50vh on a 1920px-tall viewport to resolve to ~960px, got {}", + shape_hit.rect.h + ); +}