diff --git a/crates/op-orchestrator/src/geometry_validation.rs b/crates/op-orchestrator/src/geometry_validation.rs index 3f1c93984..11db41a9a 100644 --- a/crates/op-orchestrator/src/geometry_validation.rs +++ b/crates/op-orchestrator/src/geometry_validation.rs @@ -188,7 +188,7 @@ pub fn geometry_validate_and_fix(sink: &mut dyn DocSink, root_id: &str) -> usize collect_frame_overflow_fixes(&v, &rects, &mut cmds); collect_oversized_image_fixes(&v, &mut cmds); collect_absolute_fill_image_fixes(&v, &rects, &mut cmds); - collect_image_slot_fixes(&v, &mut cmds); + collect_image_slot_fixes(&v, &rects, &mut cmds); collect_grow_to_fit_fixes(&v, &rects, &mut cmds); collect_starved_rail_card_fixes(&v, &rects, &mut cmds); collect_row_gap_fixes(&v, &rects, &mut cmds); @@ -1126,6 +1126,17 @@ fn collect_grow_to_fit_fixes( /// cards straight back to fill_container. const RAIL_STARVE_EPS: f64 = 12.0; const RAIL_MIN_CARDS: usize = 3; +/// A card is only STARVED when it is genuinely unusable — the measured case +/// was 58px cards around 160px photos. A card that merely CROPS an oversized +/// photo (a 400x300 plate clipped into a 170px card) looks right and is not +/// starved; widening it to the plate would blow one card across the whole +/// screen. That happened (user report 2026-07-12: "自检又违背设计意图了"). +const RAIL_CARD_STARVED_W: f64 = 120.0; +/// …and the demand is only credible as an INTENT if the resulting cards still +/// read as a scroll rail (the next card peeks). A "demand" wider than this +/// share of the rail is an oversized image, not a width intent — that class +/// belongs to `collect_oversized_image_fixes`, which shrinks the image. +const RAIL_CARD_MAX_FRACTION: f64 = 0.72; /// The widest fixed-width DESCENDANT plus the card's own side padding — the /// width this card provably needs to show its authored content. Descendants, @@ -1169,14 +1180,23 @@ fn collect_starved_rail_card_fixes( && c.get("width").and_then(Value::as_str) == Some("fill_container") }) .collect(); - if cards.len() >= RAIL_MIN_CARDS { + let rail_w = v + .get("id") + .and_then(Value::as_str) + .and_then(|id| rects.get(id)) + .map(|r| r.w) + .unwrap_or(0.0); + if cards.len() >= RAIL_MIN_CARDS && rail_w > 0.0 { let all_starved = cards.iter().all(|c| { let demand = fixed_content_demand(c); demand > 0.0 + && demand <= rail_w * RAIL_CARD_MAX_FRACTION && c.get("id") .and_then(Value::as_str) .and_then(|id| rects.get(id)) - .is_some_and(|r| demand > r.w + RAIL_STARVE_EPS) + .is_some_and(|r| { + r.w < RAIL_CARD_STARVED_W && demand > r.w + RAIL_STARVE_EPS + }) }); if all_starved { for c in &cards { @@ -1290,6 +1310,9 @@ fn collect_absolute_fill_image_fixes( /// height. (A wrapper that already fits, or crops only mildly, is left /// alone - intentional letterboxing stays intentional.) const IMAGE_BAND_MIN_VISIBLE_FRACTION: f64 = 0.5; +/// Tallest a grown photo band may be relative to its own width — a card photo +/// is at most mildly portrait; beyond this the "photo" is an oversized plate. +const IMAGE_BAND_MAX_ASPECT: f64 = 1.2; fn is_image_filled_frame(v: &Value) -> bool { matches!( @@ -1315,7 +1338,11 @@ fn fixed_height(v: &Value) -> Option { } } -fn collect_image_slot_fixes(v: &Value, cmds: &mut Vec) { +fn collect_image_slot_fixes( + v: &Value, + rects: &HashMap, + cmds: &mut Vec, +) { let kids = children(v); let image_nodes: Vec<&Value> = kids .iter() @@ -1353,7 +1380,19 @@ fn collect_image_slot_fixes(v: &Value, cmds: &mut Vec) { .filter_map(|f| fixed_height(f)) .fold(0.0, f64::max) }; - if photo_h > 0.0 && band < photo_h * IMAGE_BAND_MIN_VISIBLE_FRACTION { + // The photo's declared height is only a credible BAND height if the + // band would still read as a card photo. A plate taller than its own + // band is wide is an oversized image (a 400x300 plate in a phone + // card), not art direction — growing the band to it left Deals cards + // with a wall of empty space (user report 2026-07-12). + let band_w = v + .get("id") + .and_then(Value::as_str) + .and_then(|id| rects.get(id)) + .map(|r| r.w) + .unwrap_or(0.0); + let plausible = band_w > 0.0 && photo_h <= band_w * IMAGE_BAND_MAX_ASPECT; + if photo_h > 0.0 && plausible && band < photo_h * IMAGE_BAND_MIN_VISIBLE_FRACTION { if let Some(id) = v.get("id").and_then(Value::as_str) { cmds.push(EditorCommand::UpdateNode { node_id: NodeId::new(id.to_string()), @@ -1369,7 +1408,7 @@ fn collect_image_slot_fixes(v: &Value, cmds: &mut Vec) { } } for c in kids { - collect_image_slot_fixes(c, cmds); + collect_image_slot_fixes(c, rects, cmds); } } diff --git a/crates/op-orchestrator/src/geometry_validation_tests.rs b/crates/op-orchestrator/src/geometry_validation_tests.rs index 14f6c3bd7..ce1a7389b 100644 --- a/crates/op-orchestrator/src/geometry_validation_tests.rs +++ b/crates/op-orchestrator/src/geometry_validation_tests.rs @@ -2242,7 +2242,8 @@ fn double_image_slot() -> serde_json::Value { #[test] fn duplicate_image_plate_is_dropped_and_the_band_keeps_the_photo_height() { let mut cmds = Vec::new(); - collect_image_slot_fixes(&double_image_slot(), &mut cmds); + let slot = double_image_slot(); + collect_image_slot_fixes(&slot, &slot_rects(&slot), &mut cmds); assert!( cmds.iter().any(|c| matches!(c, @@ -2278,7 +2279,7 @@ fn a_lone_image_filled_plate_is_never_deleted() { ] }); let mut cmds = Vec::new(); - collect_image_slot_fixes(&slot, &mut cmds); + collect_image_slot_fixes(&slot, &slot_rects(&slot), &mut cmds); assert!( cmds.is_empty(), "the only image in the slot stays: {cmds:?}" @@ -2300,7 +2301,7 @@ fn an_image_filled_hero_with_content_on_it_is_not_a_duplicate_plate() { ] }); let mut cmds = Vec::new(); - collect_image_slot_fixes(&slot, &mut cmds); + collect_image_slot_fixes(&slot, &slot_rects(&slot), &mut cmds); assert!( !cmds .iter() @@ -2322,6 +2323,130 @@ fn a_mild_crop_stays_intentional() { ] }); let mut cmds = Vec::new(); - collect_image_slot_fixes(&slot, &mut cmds); + collect_image_slot_fixes(&slot, &slot_rects(&slot), &mut cmds); assert!(cmds.is_empty(), "a mild crop is left alone: {cmds:?}"); } + +/// Resolved rects for a slot fixture: the band spans a 200px card, children +/// take their authored size (enough for the aspect guard to reason about). +fn slot_rects(slot: &serde_json::Value) -> std::collections::HashMap { + fn walk(v: &serde_json::Value, out: &mut std::collections::HashMap, w: f64) { + if let Some(id) = v.get("id").and_then(|x| x.as_str()) { + let cw = v.get("width").and_then(|x| x.as_f64()).unwrap_or(w); + let ch = v.get("height").and_then(|x| x.as_f64()).unwrap_or(120.0); + out.insert( + id.to_string(), + Rect { + x: 0.0, + y: 0.0, + w: cw, + h: ch, + }, + ); + } + for c in v + .get("children") + .and_then(|c| c.as_array()) + .map(Vec::as_slice) + .unwrap_or(&[]) + { + walk(c, out, w); + } + } + let mut out = std::collections::HashMap::new(); + walk(slot, &mut out, 200.0); + out +} + +/// The regression that wrecked a GOOD design: a 400x300 plate cropped inside a +/// phone card is an oversized image, NOT the card's width intent. Widening the +/// cards to it blew one card across the whole screen and clipped the rest. +#[test] +fn an_oversized_plate_is_not_a_width_intent_for_the_rail() { + let cards: Vec = (0..4) + .map(|i| { + json!({ + "type": "frame", "id": format!("card{i}"), "width": "fill_container", + "height": "fill_container", "layout": "vertical", "clipContent": true, + "children": [ + { "type": "frame", "id": format!("img{i}"), "width": "fill_container", + "height": 200, "clipContent": true, "children": [ + { "type": "image", "id": format!("plate{i}"), "width": 400, "height": 300, + "src": "data:image/jpeg;base64,AAAA" } + ]} + ] + }) + }) + .collect(); + let rail = json!({ + "type": "frame", "id": "rail", "width": "fill_container", + "layout": "horizontal", "gap": 12, "children": cards + }); + let mut rects = std::collections::HashMap::new(); + rects.insert( + "rail".to_string(), + Rect { + x: 0.0, + y: 0.0, + w: 390.0, + h: 260.0, + }, + ); + for i in 0..4 { + // Healthy cards: two visible per screen, the rest scroll. + rects.insert( + format!("card{i}"), + Rect { + x: i as f64 * 182.0, + y: 0.0, + w: 170.0, + h: 260.0, + }, + ); + } + let mut cmds = Vec::new(); + collect_starved_rail_card_fixes(&rail, &rects, &mut cmds); + assert!( + cmds.is_empty(), + "a 170px card cropping a 400px plate is fine — leave it alone: {cmds:?}" + ); +} + +/// Same guard on the band: growing a 200px band to a 300px plate's height +/// left a wall of empty space under the photo in every deal card. +#[test] +fn an_oversized_plate_does_not_stretch_the_photo_band() { + let slot = json!({ + "type": "frame", "id": "band", "width": "fill_container", "height": 120, + "clipContent": true, + "children": [ + { "type": "image", "id": "plate", "width": 400, "height": 300, + "src": "data:image/jpeg;base64,AAAA" } + ] + }); + let mut rects = std::collections::HashMap::new(); + rects.insert( + "band".to_string(), + Rect { + x: 0.0, + y: 0.0, + w: 170.0, + h: 120.0, + }, + ); + rects.insert( + "plate".to_string(), + Rect { + x: 0.0, + y: 0.0, + w: 400.0, + h: 300.0, + }, + ); + let mut cmds = Vec::new(); + collect_image_slot_fixes(&slot, &rects, &mut cmds); + assert!( + cmds.is_empty(), + "a 300px plate in a 170px-wide band is oversized, not a band height: {cmds:?}" + ); +}