From 6a84ddec84474b18137c94c0f6bde9a6232b86b9 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:58:57 +0200 Subject: [PATCH] fix(geometry): check height overflow when white-space is nowrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check_unwrappable_text` — the only check left for a nowrap node — compares width only and always emits `Axis::X` (geometry.rs:781-793). So the Y axis is unchecked for any `white-space: nowrap|pre` text. `{"type":"text","content":"Hi","style":{"font-size":"120px","white-space":"nowrap","height":"40px"}}` has a single line ~156px tall painted out of a 40px content box and validates clean; delete the `white-space` key and the identical fixture is correctly reported as `ContentOverflowsBox`/`Axis::Y` (proved by the existing test `wrapped_text_taller_than_its_fixed_height_card_is_flagged`, geometry.rs:2806). The stated rationale only justifies skipping the *wrap-dependent* width re-measure; a nowrap node's height is exactly one `line_height` and needs no wrapping to compute. Refs #220 --- .../rustmotion/src/cli/commands/geometry.rs | 116 +++++++++---- .../rustmotion/src/cli/commands/validate.rs | 21 ++- crates/rustmotion/tests/audit_ws_b.rs | 164 ++++++++++++++++++ 3 files changed, 268 insertions(+), 33 deletions(-) diff --git a/crates/rustmotion/src/cli/commands/geometry.rs b/crates/rustmotion/src/cli/commands/geometry.rs index e1c10bf4..f90c2b08 100644 --- a/crates/rustmotion/src/cli/commands/geometry.rs +++ b/crates/rustmotion/src/cli/commands/geometry.rs @@ -300,7 +300,7 @@ fn walk( check_unwrappable_text( &child.component, &child_path, - &raw_bbox, + layout, viewport, vi, si, @@ -791,10 +791,32 @@ fn measurer_and_nowrap(component: &Component) -> Option<(Box bbox.w + 0.5 { + if natural_w > cw + 0.5 { let kind = component_kind(component); out.push(GeometryViolation { view_index: vi, @@ -830,11 +842,16 @@ fn check_unwrappable_text( component: kind.to_string(), axis: Axis::X, kind: ViolationKind::UnwrappableTextOverflow, - bbox: *bbox, + bbox: BBox { + x: cx, + y: cy, + w: cw, + h: ch, + }, viewport, hint: format!( "{kind} natural width is {natural_w:.0}px but only {:.0}px available — remove style.white-space: nowrap (or set it to normal) so it can wrap, or reduce style.font-size", - bbox.w + cw ), }); } @@ -856,14 +873,24 @@ fn check_unwrappable_text( /// (`codeblock`/`terminal` are deliberately excluded: their `auto_scroll` /// escape hatch makes a smaller-than-natural box intentional). /// -/// Complementary to `check_unwrappable_text`, not overlapping with it: -/// that one covers `white-space: nowrap`/`pre` (single unwrapped line, width -/// only, measured at natural/unconstrained width). This one covers the -/// default wrapping case — measured at the width the box actually *has* -/// (`content_box().2`, unconstrained height) so it also catches a single -/// unbreakable word/token/URL that's wider than the box even though wrap is -/// on (wrapping can't break within a word), plus the width axis stays -/// consistent with what will actually be painted. +/// Complementary to `check_unwrappable_text`, not overlapping with it on the +/// WIDTH axis: that one covers `white-space: nowrap`/`pre` (single unwrapped +/// line, measured at natural/unconstrained width). This function covers the +/// default wrapping case's width — measured at the width the box actually +/// *has* (`content_box().2`, unconstrained height) so it also catches a +/// single unbreakable word/token/URL that's wider than the box even though +/// wrap is on (wrapping can't break within a word), plus the width axis +/// stays consistent with what will actually be painted. +/// +/// the HEIGHT axis is this function's job regardless of `nowrap` — a +/// nowrap node used to return here before measuring height at all, so a +/// single unwrapped line taller than its box validated clean. Re-measuring +/// nowrap's WIDTH at a constrained space would wrap text that actually +/// paints as one (too-wide) line, which is exactly why `check_unwrappable_ +/// text` owns that axis instead — but a single line's height is exactly one +/// `line_height`, independent of any width constraint, so it's measured at +/// `(MaxContent, Definite(ch))` and reported on `Axis::Y` only, leaving +/// `Axis::X` to `check_unwrappable_text`. fn check_content_overflows_box( component: &Component, path: &str, @@ -876,19 +903,44 @@ fn check_content_overflows_box( let Some((intrinsic, nowrap)) = measurer_and_nowrap(component) else { return; }; - // nowrap/pre is check_unwrappable_text's territory: re-measuring it - // here at a constrained width would wrap text that will actually - // paint as one (too-wide) line, producing a height number that - // doesn't correspond to anything that gets painted. - if nowrap { - return; - } let (cx, cy, cw, ch) = layout.content_box(); if cw <= 0.0 || ch <= 0.0 { return; } + if nowrap { + let (_, natural_h) = intrinsic.measure( + (None, None), + (AvailableSpace::MaxContent, AvailableSpace::Definite(ch)), + ); + let eps = 0.5; + if natural_h <= ch + eps { + return; + } + let kind = component_kind(component); + out.push(GeometryViolation { + view_index: vi, + scene_index: si, + path: path.to_string(), + component: kind.to_string(), + axis: Axis::Y, + kind: ViolationKind::ContentOverflowsBox, + bbox: BBox { + x: cx, + y: cy, + w: cw, + h: ch, + }, + viewport, + hint: format!( + "{kind} line is {natural_h:.0}px tall but its box is only {:.0}px tall — increase style.height (or the parent's), or reduce style.font-size", + ch + ), + }); + return; + } + // Height is bounded by the node's own resolved content-box height `ch` // (not `MaxContent`) for the same reason width is bounded by `cw`: a // `text-autofit: true` node can only try to shrink into a target it's diff --git a/crates/rustmotion/src/cli/commands/validate.rs b/crates/rustmotion/src/cli/commands/validate.rs index 62f8dc17..6c995d9b 100644 --- a/crates/rustmotion/src/cli/commands/validate.rs +++ b/crates/rustmotion/src/cli/commands/validate.rs @@ -108,6 +108,25 @@ fn refuse_fix(input: &Path, raw_source: &str) -> Option { None } +/// The JSON `--fix` writes back, sourced independently of +/// `LoadedScenario::raw`. +/// +/// `loaded.raw` is captured *after* `rustmotion::assets::rebase_relative_paths` +/// runs (`validation.rs`), which rewrites every `src`/`track` naming an +/// existing file next to the scenario into a canonicalised ABSOLUTE path — +/// serialising it back would silently replace `"assets/logo.png"` with +/// this machine's own absolute path, which resolves nowhere else. By the +/// time `refuse_fix` has let a file reach this function, it carries no +/// `config`/`$var`/`include`/`for-each`/`use`, so variable substitution and +/// directive expansion are no-ops on it too — parsing `raw_source` (the +/// exact bytes still on disk) fresh yields the identical tree +/// `apply_fixes`/`navigate`'s path indices were computed against, minus the +/// rebase. +fn fixable_source(raw_source: &str) -> Result { + serde_json::from_str(raw_source) + .map_err(|e| RustmotionError::Generic(format!("re-parse source for --fix: {}", e))) +} + pub fn cmd_validate( input: &PathBuf, report: Option<&Path>, @@ -142,7 +161,7 @@ pub fn cmd_validate( if let Some(refusal) = refuse_fix(input, &raw_source) { return Err(RustmotionError::Generic(refusal.explain(input))); } - let mut json_value = loaded.raw.clone(); + let mut json_value = fixable_source(&raw_source)?; applied_fixes = apply_fixes(&mut json_value, &report_out.geom_violations); if applied_fixes > 0 { let pretty = serde_json::to_string_pretty(&json_value) diff --git a/crates/rustmotion/tests/audit_ws_b.rs b/crates/rustmotion/tests/audit_ws_b.rs index 25b511ac..c9c53469 100644 --- a/crates/rustmotion/tests/audit_ws_b.rs +++ b/crates/rustmotion/tests/audit_ws_b.rs @@ -282,3 +282,167 @@ fn static_translate_percent_resolves_per_axis_not_against_max_of_both() { "{report_json}" ); } + +// ─── --fix must not bake in machine-absolute asset paths ─────────── + +/// `--fix` used to serialise `LoadedScenario::raw`, captured AFTER +/// `rustmotion::assets::rebase_relative_paths` rewrites every existing-file +/// `src`/`track` into a canonicalised ABSOLUTE path — so fixing an +/// unrelated violation (here, a too-wide nowrap text) silently replaced +/// `"assets/logo.png"` with this machine's own absolute path. The asset +/// file only needs to EXIST (rebasing is gated on `Path::is_file()`); its +/// content is irrelevant here since geometry validation never decodes it +/// (`Image` uses a fixed 400×300 default intrinsic size, not real pixel +/// dimensions). +#[test] +fn fix_leaves_relative_asset_paths_untouched() { + let dir = ScratchDir::new("rm16"); + std::fs::create_dir_all(dir.0.join("assets")).expect("mkdir assets"); + std::fs::write( + dir.0.join("assets/logo.png"), + b"not a real png, just needs to exist", + ) + .expect("write asset"); + let scenario_path = dir.0.join("scenario.json"); + let json = r##"{ + "video": { "width": 1920, "height": 4000 }, + "scenes": [{ + "duration": 1.0, + "children": [ + { + "type": "image", + "src": "assets/logo.png", + "style": { "width": "200px", "height": "150px" } + }, + { + "type": "text", + "content": "This is a fairly long sentence with several short words that will wrap nicely across many lines without any single word being too wide for the box.", + "style": { + "width": "300px", "height": "2000px", + "color": "#ffffff", "font-size": "32px", "white-space": "nowrap" + } + } + ] + }] + }"##; + std::fs::write(&scenario_path, json).expect("write scenario"); + + let output = run_validate(&scenario_path, None, /*fix=*/ true, false); + assert!( + output.status.success(), + "the only violation (the nowrap text) is fixed in place, so this run should now \ + validate clean; stdout={} stderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let fixed: serde_json::Value = serde_json::from_str( + &std::fs::read_to_string(&scenario_path).expect("read fixed scenario"), + ) + .expect("fixed scenario is valid JSON"); + let src = fixed["scenes"][0]["children"][0]["src"] + .as_str() + .expect("image src is a string"); + assert_eq!( + src, "assets/logo.png", + "the image src must stay exactly as authored, not rewritten to an absolute path: {fixed}" + ); + + let text_style = &fixed["scenes"][0]["children"][1]["style"]; + assert!( + text_style.get("white-space").is_none(), + "the actual violation --fix targeted must still be fixed: {fixed}" + ); +} + +// ─── unwrappable_text_overflow must measure the CONTENT box ──────── + +/// A nowrap text's own painter draws inside its CONTENT box +/// (`LegacyPaintDispatcher` hands it `layout.content_box()`, not the raw +/// layout box, for every component except `codeblock`) — so the geometry +/// check must compare the natural line width against the content box too. +/// Content box width here is 2000 - 1900 = 100px (950px of padding on each +/// side); the border box is 2000px. Any real natural width for this +/// string/font-size sits comfortably in between, so the violation fires if +/// and only if the content box is used. +#[test] +fn unwrappable_text_overflow_is_measured_against_the_content_box() { + let scenario = ScratchFile::new("rm31-scenario"); + let report = ScratchFile::new("rm31-report"); + let json = r##"{ + "video": { "width": 2400, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "text", + "content": "Hello World Example", + "position": "absolute", + "x": 50, "y": 50, + "style": { + "width": "2000px", "height": "300px", + "padding": { "top": "20px", "right": "950px", "bottom": "20px", "left": "950px" }, + "white-space": "nowrap", + "font-size": "48px", + "color": "#ffffff" + } + }] + }] + }"##; + std::fs::write(&scenario.0, json).expect("write scenario"); + + let output = run_validate(&scenario.0, Some(&report.0), false, false); + let report_json = read_report(&report.0); + assert!( + !output.status.success(), + "the 100px content box (2000px border box minus 1900px of padding) is too narrow \ + for this nowrap line; report={report_json}" + ); + let violation = find_kind(&report_json, "unwrappable_text_overflow") + .expect("expected an unwrappable_text_overflow violation"); + let width = violation["bbox"]["w"].as_f64().expect("bbox.w is a number"); + assert!( + (width - 100.0).abs() < 1.0, + "violation bbox should be the 100px CONTENT box, not the 2000px border box: {report_json}" + ); +} + +// ─── white-space: nowrap must not silence the height check ───────── + +/// A single unwrapped 120px-font line is ~144px tall, well past a 40px-tall +/// box — the exact case `content_overflows_box` already catches for +/// wrapping text. `white-space: nowrap` used to return before measuring +/// height at all, so this validated clean. +#[test] +fn nowrap_text_taller_than_its_box_is_still_flagged() { + let scenario = ScratchFile::new("rm32-scenario"); + let report = ScratchFile::new("rm32-report"); + let json = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "text", + "content": "Hi", + "position": "absolute", + "x": 50, "y": 50, + "style": { + "width": "500px", "height": "40px", + "white-space": "nowrap", + "font-size": "120px", + "color": "#ffffff" + } + }] + }] + }"##; + std::fs::write(&scenario.0, json).expect("write scenario"); + + let output = run_validate(&scenario.0, Some(&report.0), false, false); + let report_json = read_report(&report.0); + assert!( + !output.status.success(), + "a 120px-font single line is far taller than a 40px box; report={report_json}" + ); + let violation = find_kind(&report_json, "content_overflows_box") + .expect("expected a content_overflows_box violation"); + assert_eq!(violation["axis"], "y", "{report_json}"); +}