diff --git a/crates/op-design-lint/src/plan.rs b/crates/op-design-lint/src/plan.rs index 5bd35756b..762c16f63 100644 --- a/crates/op-design-lint/src/plan.rs +++ b/crates/op-design-lint/src/plan.rs @@ -338,6 +338,63 @@ mod tests { ); } + /// Load the `invisible-container-with-var` fixture (doc declares + /// `color-border` variable) and assert equivalence — exercises the + /// `$color-border` design-token reference path. + /// + /// Regression guard: caught by stop-time review — `LintPreValidator` + /// previously dropped `$color-border` refs while reporting success + /// because `cmd_set_node_stroke_hex` strict-parsed the hex. The + /// op-design-lint side (this test) ensures `detect_and_plan + apply` + /// produces the same `$color-border`-stamped doc as `detect_and_fix`; + /// the host parity test confirms the same through `EditorCommand`. + #[test] + fn equivalence_invisible_container_with_var() { + let raw = include_str!("../tests/fixtures/docs/invisible-container-with-var.json"); + let doc_a: PenDocument = serde_json::from_str(raw).expect("parse"); + let doc_b: PenDocument = serde_json::from_str(raw).expect("parse"); + + // Clone A: detect_and_fix (the reference path). + let mut clone_a = doc_a; + let report = detect_and_fix(&mut clone_a); + + // Clone B: detect_and_plan + apply each fix via node_mut primitives. + let mut clone_b = doc_b; + let plan = detect_and_plan(&clone_b); + let applied: usize = plan + .iter() + .filter(|fix| apply_planned_fix_to_doc(&mut clone_b, fix)) + .count(); + + assert_eq!(report.total as usize, applied); + assert_eq!(plan.len(), applied); + assert_eq!( + clone_a, clone_b, + "var-ref stroke must round-trip identically" + ); + + // Verify the plan carries the $color-border ref (not a resolved hex). + let stroke_plan = plan + .iter() + .find(|f| f.node_id == "light-on-light") + .expect("light-on-light should be in the plan"); + let value = match &stroke_plan.action { + PlannedAction::SetStroke(v) => v, + other => panic!("expected SetStroke, got {other:?}"), + }; + let color = value + .get("fill") + .and_then(|f| f.as_array()) + .and_then(|arr| arr.first()) + .and_then(|entry| entry.get("color")) + .and_then(|c| c.as_str()) + .expect("color field"); + assert_eq!( + color, "$color-border", + "plan must preserve design-token ref, not resolve to hex" + ); + } + /// Load the `text-explicit-height` fixture and assert equivalence. /// /// This fixture exercises `FixProperty::Height` with `"fit_content"`. diff --git a/crates/op-design-lint/tests/fixtures/docs/invisible-container-with-var.json b/crates/op-design-lint/tests/fixtures/docs/invisible-container-with-var.json new file mode 100644 index 000000000..225be4500 --- /dev/null +++ b/crates/op-design-lint/tests/fixtures/docs/invisible-container-with-var.json @@ -0,0 +1,23 @@ +{ + "version": "1.0", + "variables": { + "color-border": { "type": "color", "value": "#CBD5E1" } + }, + "children": [ + { + "type": "frame", + "id": "root", + "layout": "vertical", + "fill": [{ "type": "solid", "color": "#FFFFFF" }], + "children": [ + { + "type": "frame", + "id": "light-on-light", + "layout": "vertical", + "fill": [{ "type": "solid", "color": "#FFFFFF" }], + "children": [{ "type": "text", "id": "lol-label", "content": "Inside light wrapper" }] + } + ] + } + ] +} diff --git a/crates/op-design-lint/tests/fixtures/golden/invisible-container-with-var.json b/crates/op-design-lint/tests/fixtures/golden/invisible-container-with-var.json new file mode 100644 index 000000000..7154a13bc --- /dev/null +++ b/crates/op-design-lint/tests/fixtures/golden/invisible-container-with-var.json @@ -0,0 +1,19 @@ +[ + { + "nodeId": "light-on-light", + "category": "invisible-container", + "severity": "warning", + "property": "stroke", + "currentValue": null, + "suggestedValue": { + "thickness": 1, + "fill": [ + { + "type": "solid", + "color": "$color-border" + } + ] + }, + "reason": "same fill as parent (#FFFFFF ≈ #FFFFFF, contrast=1.00)" + } +] diff --git a/crates/op-design-lint/tests/parity.rs b/crates/op-design-lint/tests/parity.rs index a1ebed85b..284d32e15 100644 --- a/crates/op-design-lint/tests/parity.rs +++ b/crates/op-design-lint/tests/parity.rs @@ -178,6 +178,10 @@ parity_case!(parity_edge_section_padding, "edge-section-padding"); parity_case!(parity_empty_path, "empty-path"); parity_case!(parity_excessive_frame_effects, "excessive-frame-effects"); parity_case!(parity_invisible_container, "invisible-container"); +parity_case!( + parity_invisible_container_with_var, + "invisible-container-with-var" +); parity_case!(parity_mixed_sibling, "mixed-sibling"); parity_case!(parity_sibling_inconsistency, "sibling-inconsistency"); parity_case!( diff --git a/crates/op-editor-core/src/command_node_attrs.rs b/crates/op-editor-core/src/command_node_attrs.rs index 02234628a..6d07cd6be 100644 --- a/crates/op-editor-core/src/command_node_attrs.rs +++ b/crates/op-editor-core/src/command_node_attrs.rs @@ -171,8 +171,19 @@ impl EditorState { /// `SetNodeStrokeHex` — set the stroke color on a node. A node with /// no stroke gets a fresh 1-px stroke so the color always lands. + /// + /// Accepts either a literal `#RGB`/`#RRGGBB` hex OR a `$variable-name` + /// design-token reference. The underlying `PenStroke.fill[*].color` + /// field is a free `String`; `op-design-lint` detectors emit + /// `$color-border` refs when the doc declares that variable, and a + /// strict hex parse here would silently drop them (regression caught + /// by a stop-time review of `LintPreValidator`). pub(crate) fn cmd_set_node_stroke_hex(&mut self, node_id: &NodeId, hex: &str) -> bool { - if !node_id.is_real() || crate::color_picker::parse_hex_rgb(hex).is_none() { + if !node_id.is_real() { + return false; + } + let is_var_ref = hex.starts_with('$'); + if !is_var_ref && crate::color_picker::parse_hex_rgb(hex).is_none() { return false; } let Some(node) = find_node_mut(self.active_children_mut(), node_id) else { @@ -216,8 +227,16 @@ impl EditorState { } /// `SetNodeFillHex` — set the fill color on a node by id. + /// + /// Accepts either a literal `#RGB`/`#RRGGBB` hex OR a `$variable-name` + /// design-token reference (same rationale as `SetNodeStrokeHex` — + /// `PenFill::Solid.color` is a free `String`). pub(crate) fn cmd_set_node_fill_hex(&mut self, node_id: &NodeId, hex: &str) -> bool { - if !node_id.is_real() || crate::color_picker::parse_hex_rgb(hex).is_none() { + if !node_id.is_real() { + return false; + } + let is_var_ref = hex.starts_with('$'); + if !is_var_ref && crate::color_picker::parse_hex_rgb(hex).is_none() { return false; } let Some(node) = find_node_mut(self.active_children_mut(), node_id) else { diff --git a/crates/op-host-desktop/src/pre_validator.rs b/crates/op-host-desktop/src/pre_validator.rs index 9aa755ca3..ea1015bc1 100644 --- a/crates/op-host-desktop/src/pre_validator.rs +++ b/crates/op-host-desktop/src/pre_validator.rs @@ -399,4 +399,22 @@ mod tests { "empty-path", ); } + + /// invisible-container-with-var: exercises `$color-border` design-token + /// reference path through `SetNodeStrokeHex`. Regression guard caught + /// by stop-time review — `cmd_set_node_stroke_hex` previously rejected + /// `$`-prefixed strings via `parse_hex_rgb`, dropping the color silently + /// while `SetNodeStrokeWidth` still applied (so `any_applied=true` and + /// `applied_count` overreported). The op-editor-core fix relaxed both + /// `cmd_set_node_stroke_hex` + `cmd_set_node_fill_hex` to passthrough + /// `$ref` strings (the underlying `color: String` field accepts them). + #[test] + fn parity_invisible_container_with_var() { + assert_parity( + include_str!( + "../../op-design-lint/tests/fixtures/docs/invisible-container-with-var.json" + ), + "invisible-container-with-var", + ); + } }