fix(agent): rerun gutter dedupe after content rails install their padding
strip_wrapper_double_inset has always known how to strip a transparent wrapper's duplicate side padding, but it runs before the mobile rail pass installs the rail gutter it deduplicates against; the later rerun used the single-child collapse whose contract cannot see a wrapper with siblings. A padded sheet wrapper inside a freshly railed section thus kept a second 24px inset and its card measured 279 against 327 rail siblings. The dedupe now reruns after rail installation, and gutter ownership is inherited along transparent chains — resetting at any painting surface — so nested wrappers converge on one gutter owner instead of reproducing the bug one level deeper. The rail skill's wrapper rule is sharpened to name the shape models actually emit. Claude-Session: https://claude.ai/code/session_01FqKQqNj8exYwopGDpYUU7x
This commit is contained in:
parent
9e417c238a
commit
093d7ed094
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
197
crates/op-orchestrator/src/cleanup_rail_wrapper_gutter_tests.rs
Normal file
197
crates/op-orchestrator/src/cleanup_rail_wrapper_gutter_tests.rs
Normal file
|
|
@ -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<f64> {
|
||||
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"
|
||||
);
|
||||
}
|
||||
|
|
@ -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::<PenNode>(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
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue