diff --git a/crates/op-ai-skills/skills/phases/generation/mobile-ui.md b/crates/op-ai-skills/skills/phases/generation/mobile-ui.md index be45f0cde..71f4087e3 100644 --- a/crates/op-ai-skills/skills/phases/generation/mobile-ui.md +++ b/crates/op-ai-skills/skills/phases/generation/mobile-ui.md @@ -17,7 +17,7 @@ NO PHONE MOCKUP WRAPPER: The whole design IS a mobile screen. Do NOT wrap your s MOBILE WIDTH SAFETY: Every visible child must stay inside the 390px screen width. Do not create horizontal rows, chips, cards, or buttons that overflow outside the root; wrap, shrink, or clip horizontal lists instead. -MOBILE SINGLE CONTENT RAIL: The root page may keep 0 horizontal padding so the pre-inserted status bar, integrated bottom navigation, and intentional full-bleed media remain full width. Every ordinary transparent root-direct content section must own the same 24px left/right rail exactly once (`padding: [0,24]`) with width="fill_container" and height="fit_content". Do not repeat that inset on an inner wrapper, and do not create full-width colored wrapper surfaces just to hold content. +MOBILE SINGLE CONTENT RAIL: The root page may keep 0 horizontal padding so the pre-inserted status bar, integrated bottom navigation, and intentional full-bleed media remain full width. Every ordinary transparent root-direct content section must own the same 24px left/right rail exactly once (`padding: [0,24]`) with width="fill_container" and height="fit_content". Do not repeat that inset on an inner wrapper: a purely structural container (the "… Sheet Container" / "… Section Wrapper" shape) inherits the rail's gutter and must carry `padding: [N,0,N,0]` at most — vertical rhythm only, never left/right. Do not create full-width colored wrapper surfaces just to hold content. MOBILE SCROLLER RAIL: A clipped horizontal scroller is the exception to section padding. Keep the scroller section full width, inset its header 24px on both sides, and give the clipped viewport a 24px leading inset with a flush 0px trailing edge so its first item aligns to the content rail while the next item can clip. diff --git a/crates/op-orchestrator/src/cleanup.rs b/crates/op-orchestrator/src/cleanup.rs index 5867a6d22..517d4c5ac 100644 --- a/crates/op-orchestrator/src/cleanup.rs +++ b/crates/op-orchestrator/src/cleanup.rs @@ -495,6 +495,18 @@ pub fn run_cleanup_passes_with_summary( // existing ownership collapse after rail repair so only one layer owns // the gutter. collapse_nested_horizontal_padding(sink, rid); + // …and re-run the double-inset stripper for the same reason. Its first + // run above sits before the mobile chrome / content-rail passes, so a + // section that only BECOMES a padded rail there was still unpadded when + // it was checked, and any transparent wrapper under it kept its own + // gutter. `collapse_nested_horizontal_padding` above cannot cover the + // gap: it only fires on a rail whose wrapper is its ONLY child, so a + // section holding the wrapper plus any sibling (a tab-bar spacer, a + // second module) stayed double-inset — measured on `0727-1-gm`, where + // the wrapped card came out 279px wide against 327px siblings. + let rid_owned = + apply_root_transform(sink, rid, crate::spacing_repair::strip_wrapper_double_inset); + let rid = rid_owned.as_str(); crate::mobile_reflow::repair_mobile_trailing_nav_reflow_for_root_in_sink(sink, rid); cleanup_mobile_dense::repair_dense_mobile_rows(sink, rid); cleanup_desktop_dashboard::repair_sparse_desktop_dashboard_rows(sink, plan, rid); @@ -642,6 +654,10 @@ mod tests_bottom_nav; #[path = "cleanup_nested_horizontal_padding_tests.rs"] mod tests_nested_horizontal_padding; +#[cfg(test)] +#[path = "cleanup_rail_wrapper_gutter_tests.rs"] +mod tests_rail_wrapper_gutter; + #[cfg(test)] #[path = "cleanup_absolute_container_tests.rs"] mod tests_absolute_container; diff --git a/crates/op-orchestrator/src/cleanup_rail_wrapper_gutter_tests.rs b/crates/op-orchestrator/src/cleanup_rail_wrapper_gutter_tests.rs new file mode 100644 index 000000000..66f3f57d8 --- /dev/null +++ b/crates/op-orchestrator/src/cleanup_rail_wrapper_gutter_tests.rs @@ -0,0 +1,197 @@ +//! Regression: a transparent wrapper must not re-add the rail's gutter. +//! +//! `strip_wrapper_double_inset` owns this contract, but it runs BEFORE the +//! mobile chrome / content-rail passes that establish the rail's own gutter. +//! A section that only becomes a padded rail during those later passes used +//! to keep an inner transparent wrapper's own horizontal padding, so the card +//! inside it rendered narrower than every sibling section on the same rail. +//! Measured on `0727-1-gm`: a Quick Add card at 279px against 327px siblings. + +use super::*; +use crate::test_support::VecDocSink; +use serde_json::{json, Value}; + +fn plan() -> crate::plan::OrchestratorPlan { + crate::plan::OrchestratorPlan { + root_frame: crate::plan::RootFrameSpec { + id: "root".into(), + name: "Habit Tracker".into(), + width: 375.0, + height: 812.0, + layout: None, + gap: None, + padding: None, + fill: None, + }, + subtasks: vec![], + style_guide_name: None, + } +} + +fn insert(sink: &mut VecDocSink, tree: Value) { + let node: PenNode = serde_json::from_value(tree).expect("fixture json"); + sink.state.apply(EditorCommand::InsertAuthoredSubtree { + nodes: vec![node], + parent_id: NodeId::NONE, + page_id: None, + }); + sink.applied.clear(); +} + +fn node_by_name<'a>(nodes: &'a [PenNode], name: &str) -> Option<&'a PenNode> { + for n in nodes { + if n.base().name.as_deref() == Some(name) { + return Some(n); + } + if let Some(hit) = n.children().and_then(|c| node_by_name(c, name)) { + return Some(hit); + } + } + None +} + +fn horizontal_padding(sink: &VecDocSink, name: &str) -> (f64, f64) { + let node = node_by_name(sink.state.active_children(), name).expect("node survives cleanup"); + let props = match node { + PenNode::Frame(n) => &n.container, + _ => panic!("{name} is a frame"), + }; + let sides = props + .padding + .as_ref() + .map(padding_sides) + .unwrap_or_default(); + (sides[1], sides[3]) +} + +/// A card surface — opaque, so it owns its inner padding legitimately. +fn card(id: &str) -> Value { + json!({ + "type": "frame", "id": id, "name": id, + "width": "fill_container", "height": "fit_content", + "layout": "vertical", "padding": 20, "gap": 16, "cornerRadius": 20, + "fill": [{"type": "solid", "color": "#FFFFFF"}], + "children": [ + {"type": "text", "id": format!("{id}-t"), "content": "Quick Add", + "width": "fill_container", "height": 24} + ] + }) +} + +/// A rail-width section that already carries the screen gutter, so the +/// resulting sibling widths are directly comparable. +fn padded_section(id: &str, name: &str) -> Value { + json!({ + "type": "frame", "id": id, "name": name, + "width": "fill_container", "height": "fit_content", + "layout": "vertical", "padding": [0, 24], "gap": 16, + "children": [card(&format!("{id}-card"))] + }) +} + +fn mobile_screen_with_wrapped_sheet() -> Value { + json!({ + "type": "frame", "id": "root", "name": "Habit Tracker", + "width": 375, "height": 812, "layout": "vertical", + "children": [ + {"type": "frame", "id": "status", "name": "Status Bar", + "width": "fill_container", "height": 62, "layout": "none", + "children": [ + {"type": "text", "id": "clock", "content": "9:41", + "width": "fit_content", "height": 20} + ]}, + padded_section("header", "Header & Daily Progress Summary"), + padded_section("habits", "Today's Habits & Rituals"), + // The defect: a transparent section with NO gutter of its own yet + // (the rail pass gives it one later), holding a transparent + // wrapper that already re-adds the same 24px gutter. + {"type": "frame", "id": "content", "name": "App Content", + "width": "fill_container", "height": "fit_content", + "layout": "vertical", + "children": [ + {"type": "frame", "id": "sheet", "name": "Quick Add Sheet Container", + "width": "fill_container", "height": "fit_content", + "layout": "vertical", "padding": [16, 24, 0, 24], + "children": [card("quick-add")]}, + // A second child: the sheet is no longer the section's only + // child, so the single-child gutter collapse cannot see it. + {"type": "frame", "id": "spacer", "name": "Tab Bar Spacer", + "width": "fill_container", "height": 72, "layout": "none", + "children": []} + ]} + ] + }) +} + +#[test] +fn wrapper_inside_a_late_railed_section_loses_its_duplicate_gutter() { + let mut sink = VecDocSink::new(); + insert(&mut sink, mobile_screen_with_wrapped_sheet()); + + crate::cleanup::run_cleanup_passes(&mut sink, &plan(), &["root"]); + + let (section_r, section_l) = horizontal_padding(&sink, "App Content"); + assert!( + section_r > 0.0 && section_l > 0.0, + "the section owns the rail gutter after cleanup, got ({section_r}, {section_l})" + ); + assert_eq!( + horizontal_padding(&sink, "Quick Add Sheet Container"), + (0.0, 0.0), + "the transparent wrapper must not re-add the gutter its section already owns" + ); +} + +/// Resolved width of the named node, via the same jian flex pass +/// `snapshot_layout` uses — the fact the defect is actually judged on. Looked +/// up by name because the cleanup root transforms re-key the whole subtree. +fn resolved_width(sink: &VecDocSink, name: &str) -> f64 { + fn walk(nodes: &[jian_scene::layout_scene::SceneNode], id: &str) -> Option { + for n in nodes { + if n.id == id { + return Some(f64::from(n.aggregate_bounds().size.x)); + } + if let Some(hit) = walk(&n.children, id) { + return Some(hit); + } + } + None + } + let id = node_by_name(sink.state.active_children(), name) + .expect("node survives cleanup") + .id_str() + .to_string(); + let scene = op_pen_loader::editor_state_to_active_page_layout_scene(&sink.state); + let page = scene.active_page().expect("active page"); + walk(&page.children, &id).expect("node has a resolved rect") +} + +#[test] +fn wrapped_card_resolves_to_the_same_width_as_its_rail_siblings() { + let mut sink = VecDocSink::new(); + insert(&mut sink, mobile_screen_with_wrapped_sheet()); + + crate::cleanup::run_cleanup_passes(&mut sink, &plan(), &["root"]); + + let sibling = resolved_width(&sink, "header-card"); + let wrapped = resolved_width(&sink, "quick-add"); + assert_eq!( + wrapped, sibling, + "the wrapped card must sit on the same rail as its siblings \ + (was 279 against 327 before the post-rail re-run)" + ); +} + +#[test] +fn opaque_card_keeps_its_own_inner_padding() { + let mut sink = VecDocSink::new(); + insert(&mut sink, mobile_screen_with_wrapped_sheet()); + + crate::cleanup::run_cleanup_passes(&mut sink, &plan(), &["root"]); + + assert_eq!( + horizontal_padding(&sink, "quick-add"), + (20.0, 20.0), + "a filled, rounded card's padding is its own inset, never a rail gutter" + ); +} diff --git a/crates/op-orchestrator/src/spacing_repair.rs b/crates/op-orchestrator/src/spacing_repair.rs index b84208aee..d6f2b76d7 100644 --- a/crates/op-orchestrator/src/spacing_repair.rs +++ b/crates/op-orchestrator/src/spacing_repair.rs @@ -28,7 +28,7 @@ pub(crate) fn strip_wrapper_double_inset(root: &mut PenNode) -> bool { let Ok(mut v) = serde_json::to_value(&*root) else { return false; }; - if !strip_in_value(&mut v) { + if !strip_in_value(&mut v, false) { return false; } match serde_json::from_value::(v) { @@ -40,11 +40,20 @@ pub(crate) fn strip_wrapper_double_inset(root: &mut PenNode) -> bool { } } -fn strip_in_value(v: &mut Value) -> bool { +/// `gutter_above` — some ancestor reached through an unbroken chain of +/// NON-PAINTING frames already owns a horizontal gutter, so this level's +/// children may not re-add one either. Without it a chain of transparent +/// wrappers only lost its outermost duplicate: stripping the first layer left +/// the second one's parent unpadded, which read as "nobody owns the gutter +/// here" and let the third layer keep an inset it had no claim to — the same +/// misalignment this pass exists to remove, one level deeper. It does NOT +/// cross a painting surface: a card's own padding is its inner inset, not a +/// rail gutter, so `is_painting_surface` resets the chain. +fn strip_in_value(v: &mut Value, gutter_above: bool) -> bool { let mut changed = false; let is_column = v.get("layout").and_then(Value::as_str) == Some("vertical"); let (pt, pr, pb, pl) = padding_sides(v); - let parent_pads_h = pr >= 16.0 && pl >= 16.0; + let parent_pads_h = (pr >= 16.0 && pl >= 16.0) || gutter_above; let parent_gaps = num(v, "gap") >= 12.0; let _ = (pt, pb); if is_column && (parent_pads_h || parent_gaps) { @@ -82,12 +91,33 @@ fn strip_in_value(v: &mut Value) -> bool { } if let Some(kids) = v.get_mut("children").and_then(Value::as_array_mut) { for c in kids.iter_mut() { - changed |= strip_in_value(c); + let inherits = parent_pads_h && !is_painting_surface(c); + changed |= strip_in_value(c, inherits); } } changed } +/// Does this node paint a surface of its own — a fill, a clip, or any stroke +/// beyond a top/bottom rule? Such a node re-establishes the inset frame for +/// everything below it, so an ancestor's gutter claim stops here. +fn is_painting_surface(v: &Value) -> bool { + let paints_fill = v + .get("fill") + .map(|f| match f { + Value::Array(a) => !a.is_empty(), + Value::Null => false, + _ => true, + }) + .unwrap_or(false); + let clips = v.get("clipContent").and_then(Value::as_bool) == Some(true); + let strokes = match v.get("stroke") { + None | Some(Value::Null) => false, + Some(stroke) => !stroke_is_horizontal_rule_only(stroke), + }; + paints_fill || clips || strokes +} + /// Per-axis padding transparency of a padded layout wrapper — /// `(horizontal, vertical)`. A wrapper that paints NOTHING is transparent on /// both axes. A wrapper whose only paint is a TOP/BOTTOM hairline is a diff --git a/crates/op-orchestrator/src/spacing_repair_tests.rs b/crates/op-orchestrator/src/spacing_repair_tests.rs index d5d69e751..aba045b3a 100644 --- a/crates/op-orchestrator/src/spacing_repair_tests.rs +++ b/crates/op-orchestrator/src/spacing_repair_tests.rs @@ -122,3 +122,107 @@ fn side_stroked_panel_keeps_all_padding() { })); assert!(!strip_wrapper_double_inset(&mut root)); } + +#[test] +fn nested_transparent_chain_converges_on_one_gutter_owner() { + // rail(24) > sheet(24) > inner(24): every structural layer re-adding the + // rail's gutter. Only the rail may keep it — otherwise the card lands + // 48px inside its siblings instead of 24. + let mut root = node(json!({ + "type":"frame","id":"rail","layout":"vertical","padding":[0,24], + "children":[ + {"type":"frame","id":"sheet","layout":"vertical","padding":[16,24], + "children":[ + {"type":"frame","id":"inner","layout":"vertical","padding":[8,24], + "children":[card("c")]} + ]} + ] + })); + assert!(strip_wrapper_double_inset(&mut root)); + let v = serde_json::to_value(&root).unwrap(); + let sheet = &v["children"][0]; + let inner = &sheet["children"][0]; + assert_eq!(sheet["padding"], json!([16.0, 0.0, 16.0, 0.0])); + assert_eq!(inner["padding"], json!([8.0, 0.0, 8.0, 0.0])); + assert_eq!( + inner["children"][0]["padding"], + json!(24.0), + "the opaque card's own inset survives the whole chain" + ); +} + +#[test] +fn nested_chain_strip_is_a_fixed_point() { + let mut root = node(json!({ + "type":"frame","id":"rail","layout":"vertical","padding":[0,24], + "children":[ + {"type":"frame","id":"sheet","layout":"vertical","padding":[16,24], + "children":[ + {"type":"frame","id":"inner","layout":"vertical","padding":[8,24], + "children":[card("c")]} + ]} + ] + })); + assert!(strip_wrapper_double_inset(&mut root)); + let once = serde_json::to_value(&root).unwrap(); + assert!( + !strip_wrapper_double_inset(&mut root), + "a second run must find nothing left to strip" + ); + assert_eq!(once, serde_json::to_value(&root).unwrap()); +} + +#[test] +fn gutter_claim_does_not_cross_a_card_surface() { + // rail(24) > card(fill, padding 8) > lane(padding 12): the lane sits + // inside a real surface whose own inset is too small to claim a gutter, + // so nothing authorizes stripping it. Were the rail's claim to leak + // through the card, the lane would lose its 12px. + let mut root = node(json!({ + "type":"frame","id":"rail","layout":"vertical","padding":[0,24], + "children":[ + {"type":"frame","id":"surface","layout":"vertical","padding":8, + "fill":[{"type":"solid","color":"#141414"}], + "children":[ + {"type":"frame","id":"lane","layout":"vertical","padding":[8,12], + "children":[card("c")]} + ]} + ] + })); + let v = serde_json::to_value(&root).unwrap(); + let before = v["children"][0].clone(); + strip_wrapper_double_inset(&mut root); + assert_eq!( + serde_json::to_value(&root).unwrap()["children"][0], + before, + "nothing under a painted card is a rail-gutter duplicate" + ); +} + +#[test] +fn uniformly_wrapped_siblings_stay_equal_width() { + // All three sections inset through an identical transparent wrapper. The + // gutter moves to the single owner (the rail), so the sections stay equal + // to each other — the pass never introduces the asymmetry it removes. + let wrapper = |id: &str| { + json!({"type":"frame","id":id,"layout":"vertical","padding":[16,24], + "children":[card(&format!("{id}-c"))]}) + }; + let mut root = node(json!({ + "type":"frame","id":"rail","layout":"vertical","padding":[0,24],"gap":20, + "children":[wrapper("a"), wrapper("b"), wrapper("c")] + })); + assert!(strip_wrapper_double_inset(&mut root)); + let v = serde_json::to_value(&root).unwrap(); + let pads: Vec<_> = v["children"] + .as_array() + .unwrap() + .iter() + .map(|c| c["padding"].clone()) + .collect(); + assert_eq!( + pads, + vec![Value::Null; 3], + "every sibling loses the same padding, so their widths stay identical" + ); +}