From f7004e34bc87abffd0dc62611ad990aecd95155c Mon Sep 17 00:00:00 2001 From: Fini Date: Fri, 31 Jul 2026 00:29:22 +0800 Subject: [PATCH] fix(agent): harden generated mobile geometry --- crates/op-orchestrator/src/cleanup.rs | 5 + .../cleanup_mobile_bottom_nav_dedup_tests.rs | 6 +- .../src/cleanup_mobile_chrome.rs | 20 +- ...cleanup_mobile_chrome_nav_wrapper_tests.rs | 6 +- .../src/cleanup_mobile_nav_chrome_tests.rs | 105 ++++++++- .../src/geometry_bottom_gap.rs | 153 +++++++++---- .../src/geometry_bottom_gap_tests.rs | 206 +++++++++++++++++- .../src/geometry_echo_spill_tests.rs | 36 +++ .../op-orchestrator/src/geometry_row_fixes.rs | 94 +++++++- .../op-orchestrator/src/geometry_scale_ops.rs | 38 ++-- .../src/geometry_spill_diagnostics.rs | 9 +- .../src/geometry_table_scale_tests.rs | 61 ++++++ .../src/geometry_validation.rs | 13 +- .../src/geometry_value_readers.rs | 53 ++++- crates/op-orchestrator/src/loop_finalize.rs | 1 + 15 files changed, 688 insertions(+), 118 deletions(-) diff --git a/crates/op-orchestrator/src/cleanup.rs b/crates/op-orchestrator/src/cleanup.rs index 1bc04d742..c61e34301 100644 --- a/crates/op-orchestrator/src/cleanup.rs +++ b/crates/op-orchestrator/src/cleanup.rs @@ -525,6 +525,11 @@ pub fn run_cleanup_passes_with_summary( // the corrected tree, not the pre-repair one. crate::section_shell_fill_repair::repair_section_shell_fill_ownership(sink, rid); counter.checkpoint(summary, CheckCategory::Structure); + // No-nav mobile screens share one deterministic closing contract: + // 24-32px of bottom room. The repair reads the same resolved geometry + // as the diagnostic and grows only root padding, never business nodes. + crate::geometry_validation::repair_mobile_bottom_breathing(sink, rid); + counter.checkpoint(summary, CheckCategory::Layout); // Geometry-driven validation LOOP: run the REAL jian layout, detect + // fix what the resolved rects prove wrong (table columns overflowing // their row, fill containers collapsed to 0 height by a hugging ancestor diff --git a/crates/op-orchestrator/src/cleanup_mobile_bottom_nav_dedup_tests.rs b/crates/op-orchestrator/src/cleanup_mobile_bottom_nav_dedup_tests.rs index 1cb03fccf..8bf267c50 100644 --- a/crates/op-orchestrator/src/cleanup_mobile_bottom_nav_dedup_tests.rs +++ b/crates/op-orchestrator/src/cleanup_mobile_bottom_nav_dedup_tests.rs @@ -101,7 +101,11 @@ fn bottom_nav_detected_by_cjk_name() { // Authored absolute inset is CLEARED so the nav rejoins flex flow — // a written x (even 0) is absolute placement and gets buried at (0,0). assert_eq!(nav.base().x, None); - assert_eq!(nav.width_px(), Some(390.0)); + assert_eq!(nav.base().role.as_deref(), Some("bottom-tab-bar")); + assert_eq!( + serde_json::to_value(nav).expect("nav serializes")["width"], + serde_json::json!("fill_container") + ); assert_eq!(nav.height_px(), Some(72.0)); } diff --git a/crates/op-orchestrator/src/cleanup_mobile_chrome.rs b/crates/op-orchestrator/src/cleanup_mobile_chrome.rs index c1ba5d77f..bc4dbee3d 100644 --- a/crates/op-orchestrator/src/cleanup_mobile_chrome.rs +++ b/crates/op-orchestrator/src/cleanup_mobile_chrome.rs @@ -89,7 +89,6 @@ pub(crate) fn repair_mobile_structural_chrome(sink: &mut dyn DocSink, root_id: & if !super::is_mobile_root(root) { return; } - let root_width = root.width_px().unwrap_or(0.0); let Some(children) = root.children() else { return; }; @@ -107,7 +106,7 @@ pub(crate) fn repair_mobile_structural_chrome(sink: &mut dyn DocSink, root_id: & .structural_shells .push(NodeId::new(child.id_str().to_string())); } - collect_bottom_nav_chrome_repairs(child, root_width, allow_structural, &mut repairs); + collect_bottom_nav_chrome_repairs(child, allow_structural, &mut repairs); } repairs }; @@ -120,20 +119,18 @@ pub(crate) fn repair_mobile_structural_chrome(sink: &mut dyn DocSink, root_id: & page_id: None, }); } - for (node_id, root_width) in repairs.bottom_nav_surfaces { + for node_id in repairs.bottom_nav_surfaces { // `x`/`y` are CLEARED, never set: any authored position reads as // ABSOLUTE placement in jian and yanks the nav out of flex flow — // the old `"x":0` patch (without y) pinned a healthy 4-tab // BottomTabBar to the root's top-left corner where later-painted // siblings buried it ("the navbar vanished at finalize", - // test0711-22 23:34 run). The nav is the root's last FLEX child; - // the numeric width alone gives the full-bleed span. + // test0711-22 23:34 run). `fill_container` also makes horizontal + // padding part of the root-width slot; a numeric root width plus + // padding resolves wider than the artboard in jian. sink.apply(EditorCommand::PatchNodeData { node_id, - patch_json: format!( - r#"{{"x":null,"y":null,"width":{},"height":72,"layout":"horizontal","gap":0,"padding":[8,16,8,16],"justifyContent":"space_between","alignItems":"center","stroke":null,"effects":null,"cornerRadius":0}}"#, - root_width.round() - ), + patch_json: r#"{"role":"bottom-tab-bar","x":null,"y":null,"width":"fill_container","height":72,"layout":"horizontal","gap":0,"padding":[8,16,8,16],"justifyContent":"space_between","alignItems":"center","stroke":null,"effects":null,"cornerRadius":0}"#.to_string(), page_id: None, }); } @@ -337,7 +334,7 @@ fn contains_meaningful_business_content(node: &PenNode) -> bool { #[derive(Default)] struct MobileChromeRepairs { structural_shells: Vec, - bottom_nav_surfaces: Vec<(NodeId, f64)>, + bottom_nav_surfaces: Vec, bottom_nav_items: Vec, } @@ -459,7 +456,6 @@ fn is_input_like_haystack(hay: &str) -> bool { fn collect_bottom_nav_chrome_repairs( root_child: &PenNode, - root_width: f64, allow_structural: bool, repairs: &mut MobileChromeRepairs, ) { @@ -468,7 +464,7 @@ fn collect_bottom_nav_chrome_repairs( }; repairs .bottom_nav_surfaces - .push((NodeId::new(nav.id_str().to_string()), root_width)); + .push(NodeId::new(nav.id_str().to_string())); let Some(children) = nav.children() else { return; diff --git a/crates/op-orchestrator/src/cleanup_mobile_chrome_nav_wrapper_tests.rs b/crates/op-orchestrator/src/cleanup_mobile_chrome_nav_wrapper_tests.rs index 31210418a..c843fdf05 100644 --- a/crates/op-orchestrator/src/cleanup_mobile_chrome_nav_wrapper_tests.rs +++ b/crates/op-orchestrator/src/cleanup_mobile_chrome_nav_wrapper_tests.rs @@ -284,7 +284,7 @@ fn mixed_business_wrapper_promotes_real_tabbar_to_mobile_root() { assert_eq!(nav.height_px(), Some(72.0)); assert_eq!( serde_json::to_value(nav).expect("nav serializes")["width"], - json!(375.0), + json!("fill_container"), "normal nav normalization runs after promotion" ); } @@ -510,7 +510,7 @@ fn explicit_horizontal_text_only_bottom_nav_is_not_demoted() { assert_eq!(nav.base().role.as_deref(), Some("bottom-tab-bar")); assert_eq!(nav.base().name.as_deref(), Some("Tab Bar")); let nav_json = serde_json::to_value(nav).expect("nav serializes"); - assert_eq!(nav_json["width"], json!(375.0)); + assert_eq!(nav_json["width"], json!("fill_container")); assert_eq!(nav_json["height"], json!(72.0)); assert_eq!(nav_json["layout"], json!("horizontal")); } @@ -553,7 +553,7 @@ fn explicit_bottom_nav_with_missing_layout_is_repaired_not_demoted() { let nav = find_node(root, "nav").expect("nav survives"); assert_eq!(nav.base().role.as_deref(), Some("bottom-tab-bar")); let nav_json = serde_json::to_value(nav).expect("nav serializes"); - assert_eq!(nav_json["width"], json!(375.0)); + assert_eq!(nav_json["width"], json!("fill_container")); assert_eq!(nav_json["height"], json!(72.0)); assert_eq!(nav_json["layout"], json!("horizontal")); } diff --git a/crates/op-orchestrator/src/cleanup_mobile_nav_chrome_tests.rs b/crates/op-orchestrator/src/cleanup_mobile_nav_chrome_tests.rs index b48318e99..98013acda 100644 --- a/crates/op-orchestrator/src/cleanup_mobile_nav_chrome_tests.rs +++ b/crates/op-orchestrator/src/cleanup_mobile_nav_chrome_tests.rs @@ -121,7 +121,10 @@ fn cleanup_strips_mobile_bottom_nav_pill_chrome() { let nav = find_node(root, "bottom-nav").expect("nav survives"); let home = find_node(root, "home-tab").expect("home tab survives"); let search = find_node(root, "search-tab").expect("search tab survives"); - assert_eq!(nav.width_px(), Some(390.0)); + assert_eq!( + serde_json::to_value(nav).expect("nav serializes")["width"], + json!("fill_container") + ); match nav { PenNode::Frame(frame) => { assert!(frame.container.stroke.is_none(), "nav stroke is removed"); @@ -249,7 +252,7 @@ fn cleanup_normalizes_mobile_bottom_nav_spacing_and_tab_slots() { .expect("root survives"); let nav = find_node(root, "bottom-nav").expect("nav survives"); let nav_json = serde_json::to_value(nav).expect("nav serializes"); - assert_eq!(nav_json["width"], json!(390.0)); + assert_eq!(nav_json["width"], json!("fill_container")); assert_eq!(nav_json["height"], json!(72.0)); assert_eq!(nav_json["gap"], json!(0.0)); assert_eq!(nav_json["padding"], json!([8.0, 16.0, 8.0, 16.0])); @@ -324,7 +327,11 @@ fn cleanup_normalizes_structurally_detected_bottom_nav_without_role_or_name() { .expect("root survives"); let nav = find_node(root, "footer").expect("nav survives"); let nav_json = serde_json::to_value(nav).expect("nav serializes"); - assert_eq!(nav_json["width"], json!(390.0), "nav spread to full width"); + assert_eq!( + nav_json["width"], + json!("fill_container"), + "nav spread to full width" + ); assert_eq!(nav_json["justifyContent"], json!("space_between")); for id in ["home", "search", "orders", "profile"] { let tab = find_node(root, id).expect("tab survives"); @@ -333,6 +340,88 @@ fn cleanup_normalizes_structurally_detected_bottom_nav_without_role_or_name() { } } +#[test] +fn cjk_named_nav_stays_inside_real_jian_width_and_stamps_semantics() { + let mut sink = VecDocSink::new(); + let tree: PenNode = serde_json::from_value(json!({ + "type": "frame", "id": "root", "name": "Mobile", "width": 375, "height": 812, + "layout": "vertical", + "children": [ + { + "type": "frame", "id": "header", "name": "Header Actions", + "width": 180, "height": 56, "layout": "horizontal", + "children": [ + {"type": "frame", "id": "header-home", "role": "tab"}, + {"type": "frame", "id": "header-search", "role": "tab"}, + {"type": "frame", "id": "header-profile", "role": "tab"} + ] + }, + { + "type": "frame", "id": "content", "name": "Content", + "width": "fill_container", "height": 684 + }, + { + "type": "frame", "id": "nav", "name": "底部导航栏", + "width": 375, "height": 72, "layout": "horizontal", + "padding": [8, 16, 8, 16], + "children": [ + {"type": "frame", "id": "home", "name": "Home Tab", "role": "tab"}, + {"type": "frame", "id": "search", "name": "Search Tab", "role": "tab"}, + {"type": "frame", "id": "profile", "name": "Profile Tab", "role": "tab"} + ] + } + ] + })) + .expect("CJK nav fixture"); + sink.state.apply(EditorCommand::InsertAuthoredSubtree { + nodes: vec![tree], + parent_id: NodeId::NONE, + page_id: None, + }); + sink.applied.clear(); + + crate::cleanup::repair_mobile_structural_chrome_for_all_roots(&mut sink); + + let root = sink.state.active_children().first().expect("root"); + let header = find_node(root, "header").expect("header"); + let nav = find_node(root, "nav").expect("nav"); + assert_eq!( + serde_json::to_value(header).expect("header serializes")["width"], + json!(180.0), + "the top header row must not be normalized as bottom navigation" + ); + assert_eq!(nav.base().role.as_deref(), Some("bottom-tab-bar")); + assert_eq!( + serde_json::to_value(nav).expect("nav serializes")["width"], + json!("fill_container") + ); + + fn resolved_width(nodes: &[jian_scene::layout_scene::SceneNode], id: &str) -> Option { + nodes.iter().find_map(|node| { + (node.id == id) + .then(|| f64::from(node.aggregate_bounds().size.x)) + .or_else(|| resolved_width(&node.children, id)) + }) + } + let scene = op_pen_loader::editor_state_to_active_page_layout_scene(&sink.state); + let page = scene.active_page().expect("active page"); + let root_width = resolved_width(&page.children, "root").expect("resolved root"); + let nav_width = resolved_width(&page.children, "nav").expect("resolved nav"); + assert!( + (nav_width - root_width).abs() <= 1.0, + "nav padding must stay inside the root-width slot, got nav={nav_width} root={root_width}" + ); + + let issues = crate::geometry_validation::geometry_diagnostics(&sink.state); + assert!( + issues.iter().all(|issue| { + !issue.contains("mobile-bottom-flush") + && !issue.contains("mobile-root-bottom-nav-overflow") + }), + "normalized bottom navigation must not echo as missing or overflowing: {issues:?}" + ); +} + #[test] fn cleanup_does_not_treat_header_action_row_as_bottom_nav() { // Guard against over-broad structural detection: a HEADER row at the TOP — @@ -528,13 +617,9 @@ fn cleanup_structural_nav_only_matches_bottom_row_inside_single_wrapper() { // The bottom nav row IS spread to full width. let nav = find_node(root, "nav-row").expect("nav-row survives"); let nav_json = serde_json::to_value(nav).expect("nav serializes"); - // Full-width in FLEX FLOW: numeric root-width or fill_container both - // qualify. The nav must NOT carry an authored x/y — that reads as - // absolute placement and buries it at the root's top-left corner. - assert!( - nav_json["width"] == json!(390.0) || nav_json["width"] == json!("fill_container"), - "the bottom nav row should still be spread full-width: {nav_json}" - ); + // Full-width in FLEX FLOW: the nav must use fill sizing and carry no + // authored x/y, otherwise padding widens it or absolute-positions it. + assert_eq!(nav_json["width"], json!("fill_container")); assert!( nav_json.get("x").is_none_or(serde_json::Value::is_null), "nav must stay in flex flow (no authored x): {nav_json}" diff --git a/crates/op-orchestrator/src/geometry_bottom_gap.rs b/crates/op-orchestrator/src/geometry_bottom_gap.rs index 1ad7d96e0..7c9c6af06 100644 --- a/crates/op-orchestrator/src/geometry_bottom_gap.rs +++ b/crates/op-orchestrator/src/geometry_bottom_gap.rs @@ -6,11 +6,9 @@ //! deliberate exception: the nav IS the closing element and is supposed to sit //! flush. //! -//! This is detect-only. Whether a screen wants 24px, 32px, or a trailing -//! spacer is INTENT, and the "contract → auto-fix, intent → echo" split says -//! we report the fact and let the in-loop model decide. The corpus rule the -//! model is being held to lives in `skills/phases/generation/mobile-ui.md` -//! (MOBILE BOTTOM BREATHING ROOM). +//! The shared cleanup contract repairs a flush no-nav screen to 28px while +//! preserving every authored business node. The diagnostic uses the same +//! resolved-geometry predicate, so cleanup and echo cannot disagree. //! //! No name heuristics: the bottom-nav exception is decided from the authored //! `role` semantic plus resolved geometry (full-width trailing band, nav-band @@ -18,6 +16,7 @@ //! the model happened to call the frame. use super::*; +use op_editor_core::PenNodeExt; /// Widest root that still reads as a phone artboard — same threshold the /// bottom-nav containment echo uses. @@ -29,6 +28,10 @@ const MOBILE_ROOT_MIN_HEIGHT: f64 = 500.0; /// under the corpus minimum (24px) so the echo states a fact rather than /// nagging about a slightly tight but clearly intentional inset. const FLUSH_BOTTOM_GAP: f64 = 12.0; +/// Corpus-compliant minimum before cleanup leaves an authored gap alone. +const MIN_BOTTOM_GAP: f64 = 24.0; +/// Stable midpoint of the documented 24-32px breathing-room range. +const TARGET_BOTTOM_GAP: f64 = 28.0; /// Height band a bottom navigation bar occupies. The corpus asks for 62-72px; /// the band is widened at both ends so a nav that came out slightly short or /// tall still earns its exception instead of drawing a false echo. @@ -50,39 +53,10 @@ pub(super) fn push_mobile_bottom_gap_diagnostic( if out.len() >= MAX_DIAGNOSTICS { return; } - let Some(root_rect) = resolved(root, rects) else { + let Some(gap) = resolved_mobile_bottom_gap(root, rects) else { return; }; - if root_rect.w > MOBILE_ROOT_MAX_WIDTH || root_rect.h < MOBILE_ROOT_MIN_HEIGHT { - return; - } - // Only a flow-laid screen has a meaningful "last content edge"; an - // absolutely-positioned root stacks its children wherever it likes. - if layout_str(root) != Some("vertical") { - return; - } - let kids = children(root); - let Some(last) = kids.last() else { - return; - }; - if is_bottom_nav_shape(last, &root_rect, rects) { - return; - } - // The lowest resolved edge across the root's direct children — not just - // the last one in document order, since an overlay or a taller sibling can - // be what actually reaches the bottom. - let content_bottom = kids - .iter() - .filter_map(|child| resolved(child, rects)) - .map(|rect| rect.y + rect.h) - .fold(f64::NEG_INFINITY, f64::max); - if !content_bottom.is_finite() { - return; - } - let gap = root_rect.y + root_rect.h - content_bottom; - // A negative gap is content OVERFLOWING the root — a different fact, and - // the spill diagnostics already report it. - if !(0.0..FLUSH_BOTTOM_GAP).contains(&gap) { + if gap >= FLUSH_BOTTOM_GAP { return; } out.push(format!( @@ -95,16 +69,119 @@ pub(super) fn push_mobile_bottom_gap_diagnostic( )); } -/// Is this trailing child a bottom navigation bar? +/// Repair a no-nav mobile screen to the shared 28px bottom-room contract. +/// +/// OpenPencil's post-layout reconciliation can grow an unclipped numeric root +/// to include its content plus padding. Increasing only the root's bottom +/// padding therefore grows the resolved artboard without relocating any +/// business child. Existing compliant gaps, navigation chrome, desktop roots, +/// and expression-authored padding are left unchanged. +pub(crate) fn repair_mobile_bottom_breathing(sink: &mut dyn DocSink, root_id: &str) -> bool { + let rects = resolved_rects(sink.state()); + let Some(root) = op_editor_core::walkers::find_node( + sink.state().active_children(), + &NodeId::new(root_id.to_string()), + ) else { + return false; + }; + let Ok(value) = serde_json::to_value(root) else { + return false; + }; + let Some(gap) = resolved_mobile_bottom_gap(&value, &rects) else { + return false; + }; + if gap >= MIN_BOTTOM_GAP { + return false; + } + let Some(mut padding) = numeric_padding_sides(&value) else { + return false; + }; + if padding[2] >= TARGET_BOTTOM_GAP { + return false; + } + padding[2] = TARGET_BOTTOM_GAP; + sink.apply(EditorCommand::PatchNodeData { + node_id: NodeId::new(root_id.to_string()), + patch_json: serde_json::json!({ "padding": padding }).to_string(), + page_id: None, + }) +} + +pub(crate) fn repair_mobile_bottom_breathing_for_all_roots(sink: &mut dyn DocSink) -> bool { + let root_ids: Vec = sink + .state() + .active_children() + .iter() + .map(|root| root.id_str().to_string()) + .collect(); + let mut changed = false; + for root_id in root_ids { + changed |= repair_mobile_bottom_breathing(sink, &root_id); + } + changed +} + +fn resolved_mobile_bottom_gap(root: &Value, rects: &HashMap) -> Option { + let root_rect = resolved(root, rects)?; + if root_rect.w > MOBILE_ROOT_MAX_WIDTH || root_rect.h < MOBILE_ROOT_MIN_HEIGHT { + return None; + } + // Only a flow-laid screen has a meaningful "last content edge"; an + // absolutely-positioned root stacks its children wherever it likes. + if layout_str(root) != Some("vertical") { + return None; + } + let kids = children(root); + let last = kids.last()?; + if is_bottom_nav_shape(last, &root_rect, rects) { + return None; + } + // The lowest resolved edge across the root's direct children — not just + // the last one in document order, since an overlay or a taller sibling can + // be what actually reaches the bottom. + let content_bottom = kids + .iter() + .filter_map(|child| resolved(child, rects)) + .map(|rect| rect.y + rect.h) + .fold(f64::NEG_INFINITY, f64::max); + if !content_bottom.is_finite() { + return None; + } + let gap = root_rect.y + root_rect.h - content_bottom; + // A negative gap is content OVERFLOWING the root — a different fact, and + // the spill diagnostics already report it. + if gap < 0.0 { + return None; + } + Some(gap) +} + +/// Does this trailing root child close the screen with bottom navigation? /// /// Two independent sufficient signals, neither of which reads `name`: /// the authored `role` semantic the corpus mandates, or the resolved shape a /// tab bar always has — a full-width band in the nav height range laying out -/// at least three evenly-sized tap targets in a row. +/// at least three evenly-sized tap targets in a row. A generated screen may +/// keep that bar as the last child of one final content wrapper; treat that +/// one-level shape as the same closing fact so diagnostics, cleanup, and +/// interaction backfill cannot disagree. pub(super) fn is_bottom_nav_shape( child: &Value, root_rect: &Rect, rects: &HashMap, +) -> bool { + if is_bottom_nav_surface_shape(child, root_rect, rects) { + return true; + } + children(child) + .last() + .is_some_and(|nested| is_bottom_nav_surface_shape(nested, root_rect, rects)) +} + +fn is_bottom_nav_surface_shape( + child: &Value, + root_rect: &Rect, + rects: &HashMap, ) -> bool { if child.get("role").and_then(Value::as_str) == Some("bottom-tab-bar") { return true; diff --git a/crates/op-orchestrator/src/geometry_bottom_gap_tests.rs b/crates/op-orchestrator/src/geometry_bottom_gap_tests.rs index bf3e0a8e1..ba4f04c4f 100644 --- a/crates/op-orchestrator/src/geometry_bottom_gap_tests.rs +++ b/crates/op-orchestrator/src/geometry_bottom_gap_tests.rs @@ -7,7 +7,11 @@ //! include a trailing section that a name heuristic would have mistaken for //! chrome. -use crate::geometry_validation::geometry_diagnostics; +use crate::geometry_validation::{ + geometry_diagnostics, repair_mobile_bottom_breathing, + repair_mobile_bottom_breathing_for_all_roots, +}; +use op_editor_core::PenNodeExt; use serde_json::json; const ECHO: &str = "mobile-bottom-flush"; @@ -26,6 +30,51 @@ fn echoed(root: serde_json::Value) -> bool { diagnostics_for(root).iter().any(|d| d.contains(ECHO)) } +fn state_for(root: serde_json::Value) -> op_editor_core::EditorState { + let doc: jian_ops_schema::PenDocument = serde_json::from_value(json!({ + "version": "1.0", + "children": [root] + })) + .expect("valid document"); + op_editor_core::EditorState::from_document(doc) +} + +fn repair(root: serde_json::Value) -> (op_editor_core::EditorState, bool) { + let mut state = state_for(root); + let changed = { + let mut sink = crate::loop_finalize::StateDocSink { state: &mut state }; + repair_mobile_bottom_breathing(&mut sink, "root") + }; + (state, changed) +} + +fn resolved_direct_child_gap(state: &op_editor_core::EditorState) -> f64 { + let scene = op_pen_loader::editor_state_to_active_page_layout_scene(state); + let page = scene.active_page().expect("active page"); + let root = page.children.first().expect("root scene"); + let root_bounds = root.aggregate_bounds(); + let content_bottom = root + .children + .iter() + .map(|child| { + let bounds = child.aggregate_bounds(); + f64::from(bounds.origin.y + bounds.size.y) + }) + .fold(f64::NEG_INFINITY, f64::max); + f64::from(root_bounds.origin.y + root_bounds.size.y) - content_bottom +} + +fn fixed_gap_screen(gap: f64) -> serde_json::Value { + json!({ + "type": "frame", "id": "root", "name": "Mobile Screen", + "width": 390, "height": 844, "layout": "vertical", "gap": 0, + "children": [ + { "type": "frame", "id": "body", "name": "Body", + "width": "fill_container", "height": 844.0 - gap } + ] + }) +} + /// A movie-detail screen: no bottom nav, last section ends exactly at the /// screen bottom. This is the reported failure (2026-07-28 screenshot). fn detail_screen(trailing_bottom_padding: f64) -> serde_json::Value { @@ -194,3 +243,158 @@ fn content_overflowing_the_root_is_left_to_the_spill_diagnostics() { }); assert!(!echoed(root), "overflow is not a flush-bottom report"); } + +#[test] +fn cleanup_repairs_zero_and_eleven_pixel_gaps_to_twenty_eight() { + for initial_gap in [0.0, 11.0] { + let (state, changed) = repair(fixed_gap_screen(initial_gap)); + assert!(changed, "{initial_gap}px must be repaired"); + let gap = resolved_direct_child_gap(&state); + assert!( + (gap - 28.0).abs() <= 1.0, + "expected 28px after repairing {initial_gap}px, got {gap}" + ); + assert!( + !geometry_diagnostics(&state) + .iter() + .any(|issue| issue.contains(ECHO)), + "repair and echo must share one fact predicate" + ); + } +} + +#[test] +fn cleanup_preserves_compliant_gap_navigation_desktop_and_business_nodes() { + let (compliant, changed) = repair(fixed_gap_screen(24.0)); + assert!(!changed, "an existing 24px gap is already compliant"); + assert!((resolved_direct_child_gap(&compliant) - 24.0).abs() <= 1.0); + + let nav = json!({ + "type": "frame", "id": "root", "name": "Home", + "width": 390, "height": 844, "layout": "vertical", + "children": [ + { "type": "frame", "id": "body", "width": "fill_container", "height": 772 }, + { "type": "frame", "id": "nav", "role": "bottom-tab-bar", + "width": "fill_container", "height": 72 } + ] + }); + let (with_nav, changed) = repair(nav); + assert!(!changed, "bottom navigation deliberately closes the screen"); + assert_eq!(with_nav.active_children()[0].children().unwrap().len(), 2); + + let desktop = json!({ + "type": "frame", "id": "root", "name": "Dashboard", + "width": 1440, "height": 900, "layout": "vertical", + "children": [ + { "type": "frame", "id": "body", "width": "fill_container", "height": 900 } + ] + }); + let (desktop, changed) = repair(desktop); + assert!(!changed, "desktop roots are outside the mobile contract"); + assert_eq!(desktop.active_children()[0].children().unwrap().len(), 1); +} + +#[test] +fn cleanup_preserves_a_last_wrapper_closed_by_nested_bottom_navigation() { + let root = json!({ + "type": "frame", "id": "root", "name": "Explore", + "width": 390, "height": 844, "layout": "vertical", + "children": [ + { "type": "frame", "id": "status", "role": "status-bar", + "width": "fill_container", "height": 62 }, + { + "type": "frame", "id": "wrapper", "name": "Content Wrapper", + "width": "fill_container", "height": 782, "layout": "vertical", + "children": [ + { "type": "frame", "id": "content", + "width": "fill_container", "height": 710 }, + { "type": "frame", "id": "nav", "role": "bottom-tab-bar", + "width": "fill_container", "height": 72, + "layout": "horizontal" } + ] + } + ] + }); + let (state, changed) = repair(root); + + assert!( + !changed, + "a nested trailing bottom navigation already closes the screen" + ); + let value = serde_json::to_value(&state.active_children()[0]).expect("root serializes"); + assert!( + value.get("padding").is_none(), + "cleanup must not add blank space below nested navigation: {value}" + ); + assert!( + !geometry_diagnostics(&state) + .iter() + .any(|issue| issue.contains(ECHO)), + "the shared diagnostic must recognize the same closing nav fact" + ); +} + +#[test] +fn cleanup_repairs_a_last_wrapper_without_nested_bottom_navigation() { + let root = json!({ + "type": "frame", "id": "root", "name": "Article Detail", + "width": 390, "height": 844, "layout": "vertical", + "children": [{ + "type": "frame", "id": "wrapper", "name": "Content Wrapper", + "width": "fill_container", "height": 844, "layout": "vertical", + "children": [{ + "type": "frame", "id": "content", + "width": "fill_container", "height": 844 + }] + }] + }); + let (state, changed) = repair(root); + + assert!( + changed, + "an ordinary trailing wrapper still needs breathing room" + ); + assert!((resolved_direct_child_gap(&state) - 28.0).abs() <= 1.0); + let value = serde_json::to_value(&state.active_children()[0]).expect("root serializes"); + assert_eq!(value["padding"], json!([0.0, 0.0, 28.0, 0.0])); +} + +#[test] +fn all_roots_driver_repairs_every_mobile_screen() { + let doc: jian_ops_schema::PenDocument = serde_json::from_value(json!({ + "version": "1.0", + "children": [ + { + "type": "frame", "id": "root-a", "width": 390, "height": 844, + "layout": "vertical", + "children": [ + { "type": "frame", "id": "body-a", + "width": "fill_container", "height": 844 } + ] + }, + { + "type": "frame", "id": "root-b", "x": 440, "width": 390, "height": 844, + "layout": "vertical", + "children": [ + { "type": "frame", "id": "body-b", + "width": "fill_container", "height": 844 } + ] + } + ] + })) + .expect("valid two-screen document"); + let mut state = op_editor_core::EditorState::from_document(doc); + let changed = { + let mut sink = crate::loop_finalize::StateDocSink { state: &mut state }; + repair_mobile_bottom_breathing_for_all_roots(&mut sink) + }; + assert!(changed); + for root in state.active_children() { + let value = serde_json::to_value(root).expect("root serializes"); + assert_eq!( + value["padding"], + json!([0.0, 0.0, 28.0, 0.0]), + "every root must be visited" + ); + } +} diff --git a/crates/op-orchestrator/src/geometry_echo_spill_tests.rs b/crates/op-orchestrator/src/geometry_echo_spill_tests.rs index 0305a791e..4e1ea7ef4 100644 --- a/crates/op-orchestrator/src/geometry_echo_spill_tests.rs +++ b/crates/op-orchestrator/src/geometry_echo_spill_tests.rs @@ -110,6 +110,7 @@ fn image_much_taller_than_its_parent_is_echoed_vertically() { "children": [ { "type": "frame", "id": "avatar", "name": "Avatar", "width": "fill_container", "height": 42, "layout": "horizontal", + "padding": [8, 0], "children": [ { "type": "image", "id": "img", "name": "woman face headshot", "src": "", "width": "fill_container", "height": 300 } @@ -130,6 +131,41 @@ fn image_much_taller_than_its_parent_is_echoed_vertically() { ); } +/// The bottom-breathing cleanup adds numeric root padding without changing +/// business children. OpenPencil's post-layout reconciliation includes that +/// padding in the resolved root extent; it is not evidence of a tall child. +#[test] +fn numeric_root_padding_alone_is_not_echoed_as_vertical_spill() { + let doc: jian_ops_schema::PenDocument = serde_json::from_value(serde_json::json!({ + "version": "1.0", + "children": [{ + "type": "frame", "id": "root", "name": "Screen", + "width": 390, "height": 844, "layout": "vertical", + "padding": [0, 0, 28, 0], + "children": [ + { "type": "frame", "id": "body", "name": "Body", + "width": "fill_container", "height": 844 } + ] + }] + })) + .expect("doc"); + let state = op_editor_core::EditorState::from_document(doc); + let rects = resolved_rects(&state); + let resolved_root = rects.get("root").expect("root rect").h; + assert!( + resolved_root > 844.0 + VERTICAL_SPILL_SLACK, + "fixture must exercise post-layout padding growth, got {resolved_root}" + ); + + let issues = super::geometry_diagnostics(&state); + assert!( + !issues + .iter() + .any(|issue| issue.contains("Screen (root): declared")), + "numeric padding alone is not a vertical spill: {issues:?}" + ); +} + /// `clipContent` parents are intentional croppers — no vertical-spill noise. #[test] fn clipping_parent_suppresses_vertical_spill_echo() { diff --git a/crates/op-orchestrator/src/geometry_row_fixes.rs b/crates/op-orchestrator/src/geometry_row_fixes.rs index a0e034779..c5f408bc9 100644 --- a/crates/op-orchestrator/src/geometry_row_fixes.rs +++ b/crates/op-orchestrator/src/geometry_row_fixes.rs @@ -203,16 +203,92 @@ pub(super) const ROW_OVERFULL_EPS: f64 = 8.0; /// can't meaningfully absorb a deficit. pub(super) const MIN_FLEXIFY_W: f64 = 120.0; -/// Is `v` table-shaped (≥2 horizontal rows of ≥3 cells)? Overfull TABLE rows -/// belong to the column scaler, which keeps columns aligned across rows — -/// flexifying one row's widest column would break the vertical alignment. -pub(super) fn is_table_shape(v: &Value) -> bool { - layout_str(v) != Some("horizontal") - && children(v) +#[derive(Clone, Copy, PartialEq, Eq)] +enum TableCellWidthMode { + Fixed, + Fill, + Hug, + Other, +} + +fn table_row_signature(row: &Value) -> Option> { + if layout_str(row) != Some("horizontal") || children(row).len() < 3 { + return None; + } + Some( + children(row) .iter() - .filter(|r| layout_str(r) == Some("horizontal") && children(r).len() >= 3) - .count() - >= 2 + .map(|cell| { + if fixed_width(cell).is_some() { + TableCellWidthMode::Fixed + } else { + match cell.get("width").and_then(Value::as_str) { + Some("fill_container") => TableCellWidthMode::Fill, + Some("fit_content") | None => TableCellWidthMode::Hug, + Some(_) => TableCellWidthMode::Other, + } + } + }) + .collect(), + ) +} + +/// Return the repeated, contiguous row run that gives `v` a table shape. +/// +/// A data table has a header and data rows next to each other under one +/// container, with the same per-column sizing contract. Merely finding two +/// unrelated horizontal bands is not enough: a mobile content column often +/// contains a three-item top bar and a three-item bottom tab bar separated by +/// business sections. Treating those bands as table rows suppresses the normal +/// row fixers and can emit an impossible "too many columns" diagnostic. +/// +/// The predicate uses only layout and width-mode facts. Names and roles are +/// deliberately irrelevant, so unnamed generated tables remain covered. +pub(super) fn table_rows(v: &Value) -> Vec<&Value> { + if layout_str(v) == Some("horizontal") { + return Vec::new(); + } + + let mut best = Vec::new(); + let mut run = Vec::new(); + let mut run_signature: Option> = None; + for child in children(v) { + let Some(signature) = table_row_signature(child) else { + if run.len() > best.len() { + best = std::mem::take(&mut run); + } else { + run.clear(); + } + run_signature = None; + continue; + }; + if run_signature.as_ref() == Some(&signature) { + run.push(child); + } else { + if run.len() > best.len() { + best = std::mem::take(&mut run); + } else { + run.clear(); + } + run.push(child); + run_signature = Some(signature); + } + } + if run.len() > best.len() { + best = run; + } + if best.len() >= 2 { + best + } else { + Vec::new() + } +} + +/// Is `v` table-shaped? Overfull TABLE rows belong to the column scaler, which +/// keeps columns aligned across rows — flexifying one row's widest column would +/// break the vertical alignment. +pub(super) fn is_table_shape(v: &Value) -> bool { + !table_rows(v).is_empty() } /// A horizontal row whose children's RESOLVED widths + gaps sum wider than diff --git a/crates/op-orchestrator/src/geometry_scale_ops.rs b/crates/op-orchestrator/src/geometry_scale_ops.rs index 67ba1b666..f53f121b4 100644 --- a/crates/op-orchestrator/src/geometry_scale_ops.rs +++ b/crates/op-orchestrator/src/geometry_scale_ops.rs @@ -11,14 +11,8 @@ pub(super) fn collect_scale_ops( if let Some(scale) = table_overflow_scale(v, rects) { // Apply the same scale to EVERY row's fixed cells (columns stay aligned) // and to each row's gap. - for row in children(v) { - if layout_str(row) != Some("horizontal") { - continue; - } + for row in table_rows(v) { let cells = children(row); - if cells.len() < 3 { - continue; - } for cell in cells { if let (Some(w), Some(id)) = (fixed_width(cell), cell.get("id").and_then(Value::as_str)) @@ -52,25 +46,19 @@ pub(super) fn collect_scale_ops( } } -/// If `v` is a table-shaped container (≥2 horizontal rows of ≥3 cells — the -/// STRUCTURE is the gate, not the name; "VIP Client List" shipped a starved -/// 6px email column because a name gate only trusted `table`-named frames) -/// whose fixed columns crowd out the rows' RESOLVED inner width, return the -/// scale factor (< 1.0) to apply to its fixed columns + gap. Each row is -/// measured against its own inner width (rect minus padding) and each -/// text-bearing flex column reserves a readable floor; the WORST row decides, -/// so uneven header/data column sets can't hide the deficit. `None` when the -/// shape isn't a table or everything fits. +/// If `v` is a table-shaped container (a repeated contiguous run of ≥2 +/// horizontal rows with ≥3 cells and matching width modes — the STRUCTURE is +/// the gate, not the name; "VIP Client List" shipped a starved 6px email column +/// because a name gate only trusted `table`-named frames) whose fixed columns +/// crowd out the rows' RESOLVED inner width, return the scale factor (< 1.0) to +/// apply to its fixed columns + gap. Each row is measured against its own inner +/// width (rect minus padding) and each text-bearing flex column reserves a +/// readable floor; the WORST row decides, so uneven header/data column sets +/// can't hide the deficit. `None` when the shape isn't a table or everything +/// fits. pub(super) fn table_overflow_scale(v: &Value, rects: &HashMap) -> Option { - if layout_str(v) == Some("horizontal") { - return None; - } - let rows: Vec<&Value> = children(v) - .iter() - .filter(|r| layout_str(r) == Some("horizontal") && children(r).len() >= 3) - .collect(); - // Need at least a header + one data row to be a real table. - if rows.len() < 2 { + let rows = table_rows(v); + if rows.is_empty() { return None; } let mut worst: Option = None; diff --git a/crates/op-orchestrator/src/geometry_spill_diagnostics.rs b/crates/op-orchestrator/src/geometry_spill_diagnostics.rs index 0299253a1..6e0a36d5b 100644 --- a/crates/op-orchestrator/src/geometry_spill_diagnostics.rs +++ b/crates/op-orchestrator/src/geometry_spill_diagnostics.rs @@ -33,7 +33,14 @@ pub(super) fn collect_vertical_spill_diagnostics( else { return; }; - if resolved <= declared + VERTICAL_SPILL_SLACK { + // Taffy treats a numeric size as the border box, but OpenPencil's + // post-layout repair reconciles an open container to its children's + // measured extent plus authored padding. A generated container whose + // children already consume the declared height can therefore resolve + // taller by exactly its numeric vertical padding. That is breathing room, + // not an oversized child. Expression padding is deliberately not guessed. + let padding_allowance = numeric_vertical_padding(v).unwrap_or(0.0); + if resolved <= declared + padding_allowance + VERTICAL_SPILL_SLACK { return; } let culprit = children(v) diff --git a/crates/op-orchestrator/src/geometry_table_scale_tests.rs b/crates/op-orchestrator/src/geometry_table_scale_tests.rs index ce2fa9db2..731ff92cb 100644 --- a/crates/op-orchestrator/src/geometry_table_scale_tests.rs +++ b/crates/op-orchestrator/src/geometry_table_scale_tests.rs @@ -100,6 +100,67 @@ fn single_overflowing_row_is_not_a_table() { assert!(table_overflow_scale(&strip, &rects).is_none()); } +#[test] +fn separated_mobile_chrome_rows_are_not_a_table() { + // gallery-wander's destination screen: the content wrapper contained a + // three-item top bar and a three-item bottom tab bar with business sections + // between them. Counting horizontal children alone called this a table and + // emitted "3 columns cannot fit a 295px row". + let fixed = |id: &str| cell(id, json!(36)); + let top_bar = json!({ + "type": "frame", "id": "top", "layout": "horizontal", "children": [ + fixed("back"), + { "type": "text", "id": "title", "width": "fit_content", "content": "Destination Details" }, + fixed("bookmark") + ] + }); + let nav_item = |id: &str| { + json!({ "type": "frame", "id": id, "layout": "vertical", + "width": "fill_container", "children": [] }) + }; + let content = json!({ + "type": "frame", "id": "content", "layout": "vertical", "children": [ + top_bar, + { "type": "frame", "id": "hero", "layout": "vertical", "children": [] }, + { "type": "frame", "id": "body", "layout": "vertical", "children": [] }, + { "type": "frame", "id": "nav", "layout": "horizontal", "children": [ + nav_item("trips"), nav_item("destination"), nav_item("saved") + ]} + ] + }); + let mut rects = std::collections::HashMap::new(); + for id in ["top", "nav"] { + rects.insert( + id.to_string(), + Rect { + x: 0.0, + y: 0.0, + w: 295.0, + h: 72.0, + }, + ); + } + + assert!(!is_table_shape(&content)); + assert!(table_overflow_scale(&content, &rects).is_none()); + assert!(table_columns_exceed_width(&content, &rects).is_none()); +} + +#[test] +fn contiguous_unnamed_rows_with_matching_width_modes_remain_a_table() { + let widths = [json!(180), json!("fill_container"), json!(120), json!(96)]; + let table = json!({ + "type": "frame", "id": "records", "layout": "vertical", "children": [ + row("header", &widths), + row("record-a", &widths), + row("record-b", &widths) + ] + }); + + assert!(is_table_shape(&table)); + assert_eq!(table_rows(&table).len(), 3); +} + #[test] fn padded_row_with_text_fill_column_triggers_on_inner_width() { // test0703.op's exact failure shape: 860px rows padded [12,16] (inner diff --git a/crates/op-orchestrator/src/geometry_validation.rs b/crates/op-orchestrator/src/geometry_validation.rs index ece89fc25..9a1591d97 100644 --- a/crates/op-orchestrator/src/geometry_validation.rs +++ b/crates/op-orchestrator/src/geometry_validation.rs @@ -46,6 +46,9 @@ use text_collision::push_text_collision_diagnostics; #[path = "geometry_bottom_gap.rs"] mod geometry_bottom_gap; use geometry_bottom_gap::push_mobile_bottom_gap_diagnostic; +pub(crate) use geometry_bottom_gap::{ + repair_mobile_bottom_breathing, repair_mobile_bottom_breathing_for_all_roots, +}; #[path = "geometry_interaction_backfill.rs"] mod geometry_interaction_backfill; use geometry_interaction_backfill::push_interaction_backfill_diagnostics; @@ -286,14 +289,8 @@ pub fn fix_rail_width_collapse(sink: &mut dyn DocSink, root_id: &str) -> bool { /// rescale; the design needs fewer columns. Returns `(columns, inner_px)` /// of the worst row for the diagnostic. fn table_columns_exceed_width(v: &Value, rects: &HashMap) -> Option<(usize, i64)> { - if layout_str(v) == Some("horizontal") { - return None; - } - let rows: Vec<&Value> = children(v) - .iter() - .filter(|r| layout_str(r) == Some("horizontal") && children(r).len() >= 3) - .collect(); - if rows.len() < 2 { + let rows = table_rows(v); + if rows.is_empty() { return None; } for row in rows { diff --git a/crates/op-orchestrator/src/geometry_value_readers.rs b/crates/op-orchestrator/src/geometry_value_readers.rs index f7f9c9d1c..bcea39fb8 100644 --- a/crates/op-orchestrator/src/geometry_value_readers.rs +++ b/crates/op-orchestrator/src/geometry_value_readers.rs @@ -86,6 +86,46 @@ pub(super) fn num(v: &Value, key: &str) -> f64 { } } +/// Parse authored numeric padding into CSS-order `[top, right, bottom, left]`. +/// +/// `None` means the padding is expression-backed or malformed. Callers that +/// mutate padding must preserve that distinction instead of replacing intent +/// they cannot evaluate; diagnostic callers may conservatively treat it as no +/// known allowance. +pub(super) fn numeric_padding_sides(v: &Value) -> Option<[f64; 4]> { + match v.get("padding") { + None | Some(Value::Null) => Some([0.0; 4]), + Some(Value::Number(value)) => { + let padding = value.as_f64()?; + Some([padding; 4]) + } + Some(Value::Array(values)) if values.len() == 1 => { + let padding = values[0].as_f64()?; + Some([padding; 4]) + } + Some(Value::Array(values)) if values.len() == 2 => { + let vertical = values[0].as_f64()?; + let horizontal = values[1].as_f64()?; + Some([vertical, horizontal, vertical, horizontal]) + } + Some(Value::Array(values)) if values.len() == 4 => Some([ + values[0].as_f64()?, + values[1].as_f64()?, + values[2].as_f64()?, + values[3].as_f64()?, + ]), + _ => None, + } +} + +/// Known vertical padding that can legitimately participate in post-layout +/// content reconciliation. Negative values are not useful breathing room and +/// therefore cannot widen a spill tolerance. +pub(super) fn numeric_vertical_padding(v: &Value) -> Option { + let [top, _, bottom, _] = numeric_padding_sides(v)?; + Some(top.max(0.0) + bottom.max(0.0)) +} + /// Sum of a frame's LEFT + RIGHT padding — the schema authors `padding` as a /// number (all sides), `[vertical, horizontal]`, or `[top, right, bottom, /// left]`. The overflow math must compare column widths against the row's @@ -93,14 +133,7 @@ pub(super) fn num(v: &Value, key: &str) -> f64 { /// and ignoring that put a real table 2px on the "fits" side of the gate /// while its flex column starved to 6px (measured: test0703.op). pub(super) fn horizontal_padding(v: &Value) -> f64 { - match v.get("padding") { - Some(Value::Number(n)) => n.as_f64().unwrap_or(0.0) * 2.0, - Some(Value::Array(a)) => match a.len() { - 1 => a[0].as_f64().unwrap_or(0.0) * 2.0, - 2 => a[1].as_f64().unwrap_or(0.0) * 2.0, - 4 => a[1].as_f64().unwrap_or(0.0) + a[3].as_f64().unwrap_or(0.0), - _ => 0.0, - }, - _ => 0.0, - } + numeric_padding_sides(v) + .map(|[_, right, _, left]| right + left) + .unwrap_or(0.0) } diff --git a/crates/op-orchestrator/src/loop_finalize.rs b/crates/op-orchestrator/src/loop_finalize.rs index 65e899b44..6c6995725 100644 --- a/crates/op-orchestrator/src/loop_finalize.rs +++ b/crates/op-orchestrator/src/loop_finalize.rs @@ -476,6 +476,7 @@ pub fn apply_loop_finalize_counted(state: &mut EditorState) -> RepairSummary { crate::cleanup::pad_clipping_horizontal_row_for_stroke_for_all_roots(sink); crate::cleanup::equalize_horizontal_card_heights_for_all_roots(sink); crate::cleanup::collapse_fill_container_content_sections_for_all_roots(sink); + crate::geometry_validation::repair_mobile_bottom_breathing_for_all_roots(sink); counter.checkpoint(&mut summary, CheckCategory::Layout); } if state.active_children().is_empty() {