Skip to content

Commit 2ec0659

Browse files
authored
fix(engine): stop accepting input that has no observable effect
Wave 2 of the post-audit chantier (#102): the medium findings, plus three items wave 1 deferred. Wave 1 fixed what produced wrong output. This wave fixes what produced no output and no complaint — properties the schema accepts and the engine silently ignores. Every item is the same defect wearing a different hat. white-space was read in exactly two places repo-wide, neither of them the painter, so the validator flagged a state the renderer could not produce. It is now honoured in measurement and painting across text, gradient_text and caption; gradient_text had no line-breaking path at all and gained one. rich_text had no intrinsic, so it measured 0x0 and rendered nothing unless the author guessed a width — for the component whose whole purpose is accent- coloured words inside a sentence. glow was stored by extract_effects and never read: renders with and without it were byte-identical. Misplaced attributes were warnings, and render exited 0 having silently used default styling. Both now fail, and unknown keys are rejected on Scenario, Scene, View, VideoConfig, SceneLayout, Transition and Camera as well. SceneEntry's untagged deserialization collapsed every scene-level error into "data did not match any variant of untagged enum SceneEntry". Errors now name themselves. Text below 1.2% of output height is unreadable in a video even though it fits the frame perfectly. Threshold established by rendering a line at 8-28px and downscaling 50%, the way video is actually watched. Unresolvable colours now fail validation, completing the wave-1 change that made them loud at render time. schema::GridTrack and seven more orphans left behind by the LayerStyle removal are deleted. --strict-attrs is kept so existing scripts keep working, but announces that it no longer does anything: a flag with no observable effect is the same defect as a schema property with no observable effect. 580 tests pass, clippy is clean, and all 7 example scenarios still validate under the stricter rules — none needed weakening to pass. Closes #109 Closes #110 Closes #111
1 parent 38596ca commit 2ec0659

19 files changed

Lines changed: 2531 additions & 307 deletions

File tree

crates/rustmotion-cli/src/commands/geometry.rs

Lines changed: 312 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,152 @@ fn check_auto_scroll(
677677
}
678678
}
679679

680+
// ─── M4: legibility floor (issue #110 / #102) ──────────────────────────────
681+
//
682+
// "Fits in the frame" (checked above) is not "readable in a video". A table
683+
// column, a badge, a codeblock line — any of them can validate perfectly
684+
// clean while rendering at a font size nobody could read once the video is
685+
// scaled down from its native resolution, which is how video is normally
686+
// watched (embedded players, mobile feeds, thumbnails) unlike a web page,
687+
// which is usually viewed close to 1:1.
688+
//
689+
// Threshold justification (rendered evidence, not a guess): a 1920×1080
690+
// scenario was rendered with the same sample line at 8/10/11/12/13/14/16/18/
691+
// 20/22/24/28px, then the frame was scaled down 50% (a realistic "not
692+
// full-native" viewing size) to inspect. 8–13px degraded to an illegible
693+
// grey smear at that scale; 14px was the first size that stayed readable.
694+
// 0.012 (1.2% of output height) sits between those two bands — it equals
695+
// ~13px on a 1080p frame — and clears every built-in component default
696+
// already shipped (table/terminal/codeblock/pill_nav = 14px, badge `md` =
697+
// 14px, kbd = 14px, tooltip = 13px), so it does not fire on scenarios that
698+
// already validate clean today. Expressing it as a fraction of output
699+
// height (rather than an absolute px count) makes the same *visual* size
700+
// get flagged on a 4K or vertical-format canvas too.
701+
const MIN_LEGIBLE_FONT_RATIO: f32 = 0.012;
702+
703+
/// Check every text-bearing component's effective font size against
704+
/// [`MIN_LEGIBLE_FONT_RATIO`] of the output height. Always advisory (a
705+
/// warning, never a blocking error) — this is a legibility floor, not a
706+
/// geometry correctness check, and the "right" size is ultimately an
707+
/// authorial call.
708+
///
709+
/// Coverage: every component whose `Painter` resolves its rendered font
710+
/// size from `style.font-size` (falling back to that component's own
711+
/// documented default when unset) — text, rich_text, gradient_text,
712+
/// caption, counter, table, terminal, codeblock, callout, list,
713+
/// notification (title + message), pill_nav, badge, kbd, tooltip, marquee.
714+
/// Not covered: components whose text sizing isn't a simple
715+
/// `style.font-size`-or-default resolution (chart axis/labels, gauge, stat,
716+
/// sparkline, heatmap, treemap, dot_map, avatar initials, progress label,
717+
/// rating, countdown, comparison, stepper, timeline, tag_cloud) — see the
718+
/// workstream report for the full list.
719+
pub fn check_legibility(scenario: &ResolvedScenario) -> Vec<String> {
720+
let mut warnings = Vec::new();
721+
let video_h = scenario.video.height as f32;
722+
if video_h <= 0.0 {
723+
return warnings;
724+
}
725+
let min_px = MIN_LEGIBLE_FONT_RATIO * video_h;
726+
727+
for (vi, view) in scenario.views.iter().enumerate() {
728+
for (si, scene) in view.scenes.iter().enumerate() {
729+
let indexed = deserialize_children_indexed(scene);
730+
let path_root = format!("views[{}].scenes[{}]", vi, si);
731+
for (json_idx, child) in &indexed {
732+
let path = format!("{}.children[{}]", path_root, json_idx);
733+
walk_legibility(&child.component, &path, min_px, video_h, &mut warnings);
734+
}
735+
}
736+
}
737+
warnings
738+
}
739+
740+
fn walk_legibility(
741+
component: &Component,
742+
path: &str,
743+
min_px: f32,
744+
video_h: f32,
745+
out: &mut Vec<String>,
746+
) {
747+
for (label, effective_px) in text_sizes(component) {
748+
// 0.05px tolerance for float rounding; not a meaningful visual gap.
749+
if effective_px < min_px - 0.05 {
750+
out.push(format!(
751+
"{path}: {label} renders at ~{effective_px:.0}px on a {video_h:.0}px-tall frame \
752+
({:.2}% of height) — likely illegible once the video is viewed at anything less \
753+
than native resolution. Raise the effective font size to at least {min_px:.0}px \
754+
(~{:.1}% of height).",
755+
effective_px / video_h * 100.0,
756+
MIN_LEGIBLE_FONT_RATIO * 100.0,
757+
));
758+
}
759+
}
760+
761+
if let Some(children) = container_children(component) {
762+
for (i, child) in children.iter().enumerate() {
763+
walk_legibility(
764+
&child.component,
765+
&format!("{path}.children[{i}]"),
766+
min_px,
767+
video_h,
768+
out,
769+
);
770+
}
771+
}
772+
}
773+
774+
/// Effective rendered font size(s) for a component, mirroring exactly the
775+
/// default each `Painter` falls back to when `style.font-size` is unset
776+
/// (see the file/line citations below — kept in sync by hand since these
777+
/// defaults live in `rustmotion-components`, out of this workstream's
778+
/// scope). A component can report more than one size (e.g. a notification's
779+
/// title and message use different sizes).
780+
fn text_sizes(component: &Component) -> Vec<(&'static str, f32)> {
781+
match component {
782+
// text.rs, rich_text.rs, gradient_text.rs, caption.rs, counter.rs: 48.0
783+
Component::Text(t) => vec![("text", t.style.font_size_px_or(48.0))],
784+
Component::RichText(t) => vec![("rich_text", t.style.font_size_px_or(48.0))],
785+
Component::GradientText(t) => vec![("gradient_text", t.style.font_size_px_or(48.0))],
786+
Component::Caption(t) => vec![("caption", t.style.font_size_px_or(48.0))],
787+
Component::Counter(c) => vec![("counter", c.style.font_size_px_or(48.0))],
788+
// table.rs, terminal.rs, codeblock/{dimensions,render}.rs, pill_nav.rs: 14.0
789+
Component::Table(t) => vec![("table", t.style.font_size_px_or(14.0))],
790+
Component::Terminal(t) => vec![("terminal", t.style.font_size_px_or(14.0))],
791+
Component::Codeblock(c) => vec![("codeblock", c.style.font_size_px_or(14.0))],
792+
Component::PillNav(p) => vec![("pill_nav", p.style.font_size_px_or(14.0))],
793+
// callout.rs, list.rs, notification.rs (title): 16.0
794+
Component::Callout(c) => vec![("callout", c.style.font_size_px_or(16.0))],
795+
Component::List(l) => vec![("list", l.style.font_size_px_or(16.0))],
796+
Component::Notification(n) => {
797+
let title = n.style.font_size_px_or(16.0);
798+
let mut sizes = vec![("notification title", title)];
799+
if n.message.is_some() {
800+
// notification.rs: message_font_size() = title_font_size() * 0.85
801+
sizes.push(("notification message", title * 0.85));
802+
}
803+
sizes
804+
}
805+
// These carry their own `font_size` field (already serde-resolved
806+
// to its component default when absent from JSON), overridable by
807+
// `style.font-size` exactly like the rest — kbd.rs, tooltip.rs,
808+
// marquee.rs.
809+
Component::Kbd(k) => vec![("kbd", k.style.font_size_px_or(k.font_size))],
810+
Component::Tooltip(t) => vec![("tooltip", t.style.font_size_px_or(t.font_size))],
811+
Component::Marquee(m) => vec![("marquee", m.style.font_size_px_or(m.font_size))],
812+
// badge.rs: BadgeSize::{Sm,Md,Lg}.params().0 = {12.0, 14.0, 18.0}.
813+
// `params()` is private to badge.rs, so the table is duplicated here.
814+
Component::Badge(b) => {
815+
let default_fs = match b.badge_size {
816+
rustmotion::components::badge::BadgeSize::Sm => 12.0,
817+
rustmotion::components::badge::BadgeSize::Md => 14.0,
818+
rustmotion::components::badge::BadgeSize::Lg => 18.0,
819+
};
820+
vec![("badge", b.style.font_size_px_or(default_fs))]
821+
}
822+
_ => vec![],
823+
}
824+
}
825+
680826
fn component_kind(c: &Component) -> &'static str {
681827
match c {
682828
Component::Text(_) => "text",
@@ -1854,3 +2000,169 @@ mod tests {
18542000
.any(|v| v.kind == ViolationKind::UnwrappableTextOverflow));
18552001
}
18562002
}
2003+
2004+
/// M4 (issue #110 / #102): legibility floor tests.
2005+
#[cfg(test)]
2006+
mod legibility_tests {
2007+
use super::*;
2008+
use rustmotion::loader::load_scenario_from_source;
2009+
2010+
fn parse(json: &str) -> rustmotion::schema::ResolvedScenario {
2011+
load_scenario_from_source(None, Some(json)).expect("scenario parses")
2012+
}
2013+
2014+
#[test]
2015+
fn tiny_font_on_1080p_warns() {
2016+
// 11px on a 1080p frame is the audit's own worked example of
2017+
// unreadable text (~1.0% of height, well under the 1.2% floor).
2018+
let json = r##"{
2019+
"video": { "width": 1920, "height": 1080 },
2020+
"scenes": [{
2021+
"duration": 1.0,
2022+
"children": [{
2023+
"type": "text",
2024+
"content": "fine print",
2025+
"style": { "color": "#ffffff", "font-size": "11px" }
2026+
}]
2027+
}]
2028+
}"##;
2029+
let scenario = parse(json);
2030+
let warnings = check_legibility(&scenario);
2031+
assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}");
2032+
assert!(warnings[0].contains("11px"), "got: {}", warnings[0]);
2033+
assert!(
2034+
warnings[0].contains("views[0].scenes[0].children[0]"),
2035+
"got: {}",
2036+
warnings[0]
2037+
);
2038+
}
2039+
2040+
#[test]
2041+
fn default_sized_text_on_1080p_has_no_legibility_warning() {
2042+
// No style.font-size override: falls back to text's own 48px
2043+
// default, comfortably above the floor.
2044+
let json = r##"{
2045+
"video": { "width": 1920, "height": 1080 },
2046+
"scenes": [{
2047+
"duration": 1.0,
2048+
"children": [{
2049+
"type": "text",
2050+
"content": "headline",
2051+
"style": { "color": "#ffffff" }
2052+
}]
2053+
}]
2054+
}"##;
2055+
let scenario = parse(json);
2056+
let warnings = check_legibility(&scenario);
2057+
assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}");
2058+
}
2059+
2060+
#[test]
2061+
fn default_table_terminal_codeblock_on_1080p_do_not_warn() {
2062+
// 14px defaults must clear the floor so this check doesn't spam
2063+
// every scenario that never touched style.font-size.
2064+
let json = r##"{
2065+
"video": { "width": 1920, "height": 1080 },
2066+
"scenes": [{
2067+
"duration": 1.0,
2068+
"children": [
2069+
{ "type": "table", "headers": ["a"], "rows": [["1"]] },
2070+
{ "type": "terminal", "lines": [{ "text": "$ ok", "type": "input" }] },
2071+
{ "type": "codeblock", "code": "fn main() {}" }
2072+
]
2073+
}]
2074+
}"##;
2075+
let scenario = parse(json);
2076+
let warnings = check_legibility(&scenario);
2077+
assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}");
2078+
}
2079+
2080+
#[test]
2081+
fn same_absolute_px_warns_more_readily_on_a_taller_frame() {
2082+
// The floor is a fraction of output height, so the same 20px text
2083+
// that's fine on 1080p (1.85%) should warn on a much taller canvas
2084+
// where 20px is proportionally tiny.
2085+
let json = r##"{
2086+
"video": { "width": 1080, "height": 4000 },
2087+
"scenes": [{
2088+
"duration": 1.0,
2089+
"children": [{
2090+
"type": "text",
2091+
"content": "small on a huge canvas",
2092+
"style": { "color": "#ffffff", "font-size": "20px" }
2093+
}]
2094+
}]
2095+
}"##;
2096+
let scenario = parse(json);
2097+
let warnings = check_legibility(&scenario);
2098+
assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}");
2099+
}
2100+
2101+
#[test]
2102+
fn small_badge_size_warns_using_its_own_default() {
2103+
let json = r##"{
2104+
"video": { "width": 1920, "height": 1080 },
2105+
"scenes": [{
2106+
"duration": 1.0,
2107+
"children": [{
2108+
"type": "badge",
2109+
"text": "new",
2110+
"badge_size": "sm"
2111+
}]
2112+
}]
2113+
}"##;
2114+
let scenario = parse(json);
2115+
let warnings = check_legibility(&scenario);
2116+
assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}");
2117+
assert!(warnings[0].contains("badge"), "got: {}", warnings[0]);
2118+
}
2119+
2120+
#[test]
2121+
fn legibility_never_blocks_validation() {
2122+
use crate::commands::validate_schema::validate_scenario;
2123+
let json = r##"{
2124+
"video": { "width": 1920, "height": 1080 },
2125+
"scenes": [{
2126+
"duration": 1.0,
2127+
"children": [{
2128+
"type": "text",
2129+
"content": "fine print",
2130+
"style": { "color": "#ffffff", "font-size": "6px" }
2131+
}]
2132+
}]
2133+
}"##;
2134+
let scenario = parse(json);
2135+
assert!(!check_legibility(&scenario).is_empty());
2136+
let (errors, _warnings) = validate_scenario(&scenario);
2137+
assert!(
2138+
errors.is_empty(),
2139+
"legibility must never surface as a schema error: {errors:?}"
2140+
);
2141+
}
2142+
2143+
#[test]
2144+
fn nested_card_child_gets_a_nested_path() {
2145+
let json = r##"{
2146+
"video": { "width": 1920, "height": 1080 },
2147+
"scenes": [{
2148+
"duration": 1.0,
2149+
"children": [{
2150+
"type": "card",
2151+
"children": [{
2152+
"type": "text",
2153+
"content": "fine print",
2154+
"style": { "color": "#ffffff", "font-size": "8px" }
2155+
}]
2156+
}]
2157+
}]
2158+
}"##;
2159+
let scenario = parse(json);
2160+
let warnings = check_legibility(&scenario);
2161+
assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}");
2162+
assert!(
2163+
warnings[0].contains("views[0].scenes[0].children[0].children[0]"),
2164+
"got: {}",
2165+
warnings[0]
2166+
);
2167+
}
2168+
}

crates/rustmotion-cli/src/commands/render.rs

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,26 @@ use crate::commands::validation::{self, ValidationSource};
1010

1111
/// Load + validate a scenario for watch mode. On validation failure prints the
1212
/// report and returns the typed error so the caller can decide how to handle it.
13+
///
14+
/// `strict_attrs` is accepted for CLI-surface parity with `validate` (M5,
15+
/// issue #110) but is a no-op in practice: unknown component attributes
16+
/// block by default now (`ValidationReport::is_blocking`), and `--watch`
17+
/// mode never writes a `--report` JSON file, so there is no bucket left for
18+
/// `promote_attr_warnings` to affect.
19+
#[allow(clippy::too_many_arguments)]
1320
fn load_for_watch(
1421
input: &Path,
1522
no_validate: bool,
1623
lenient: bool,
1724
strict_anim: bool,
25+
strict_attrs: bool,
1826
) -> Result<ResolvedScenario> {
1927
let loaded = validation::load(ValidationSource::File(input))?;
2028
if !no_validate {
21-
let report = validation::run_checks(&loaded, strict_anim);
29+
let mut report = validation::run_checks(&loaded, strict_anim);
30+
if strict_attrs {
31+
report.promote_attr_warnings();
32+
}
2233
if !report.is_clean() {
2334
validation::print_report(&report, &input.display().to_string());
2435
}
@@ -204,6 +215,7 @@ pub fn cmd_render(
204215
Ok(())
205216
}
206217

218+
#[allow(clippy::too_many_arguments)]
207219
pub fn cmd_watch(
208220
input: &PathBuf,
209221
output: &Path,
@@ -217,6 +229,7 @@ pub fn cmd_watch(
217229
no_validate: bool,
218230
lenient: bool,
219231
strict_anim: bool,
232+
strict_attrs: bool,
220233
) -> Result<()> {
221234
use notify::{RecursiveMode, Watcher};
222235
use std::sync::mpsc;
@@ -250,7 +263,7 @@ pub fn cmd_watch(
250263
let mut initial_includes: Vec<PathBuf> = Vec::new();
251264

252265
// Initial render
253-
match load_for_watch(input, no_validate, lenient, strict_anim) {
266+
match load_for_watch(input, no_validate, lenient, strict_anim, strict_attrs) {
254267
Ok(scenario) => {
255268
initial_includes = scenario.included_paths.clone();
256269

@@ -367,7 +380,7 @@ pub fn cmd_watch(
367380
std::thread::sleep(std::time::Duration::from_millis(100));
368381
while rx.try_recv().is_ok() {}
369382

370-
match load_for_watch(input, no_validate, lenient, strict_anim) {
383+
match load_for_watch(input, no_validate, lenient, strict_anim, strict_attrs) {
371384
Ok(scenario) => {
372385
// Reset error backoff on a successful load
373386
if consecutive_err_count > 0 && suppressed {

crates/rustmotion-cli/src/commands/validate.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ pub fn cmd_validate(
2323

2424
let mut report_out = validation::run_checks(&loaded, strict_anim);
2525
if strict_attrs {
26+
validation::warn_strict_attrs_is_now_default();
2627
report_out.promote_attr_warnings();
2728
}
2829

0 commit comments

Comments
 (0)