Fix Gradient tool inserting new nodes in the wrong position when updating a Fill node's gradient chain (#4203)

* Fix Gradient tool inserting new nodes in the wrong position when updating a Fill node's gradient chain

* Improve robustness

Author: Keavon

editor/src/messages/portfolio/document/graph_operation/utility_types.rs

@@ -362,14 +362,11 @@ impl<'a> ModifyInputsContext<'a> {
 			return None;
 		}
 
-		// Splice new node between target_input and its current upstream
+		// Splice a new node onto the wire feeding `target_input`, positioning it sensibly within the chain.
 		let node_definition = resolve_proto_node_type(reference)?;
-		let current_input = self.network_interface.input_from_connector(target_input, &[])?.clone();
-
 		let node_id = NodeId::new();
 		self.network_interface.insert_node(node_id, node_definition.default_node_template(), &[]);
-		self.network_interface.set_input(&InputConnector::node(node_id, 0), current_input, &[]);
-		self.network_interface.set_input(target_input, NodeInput::node(node_id, 0), &[]);
+		self.network_interface.insert_node_before_input(&node_id, target_input, &[]);
 
 		Some(node_id)
 	}

editor/src/messages/portfolio/document/utility_types/network_interface.rs

@@ -5911,6 +5911,46 @@ impl NodeNetworkInterface {
 		self.create_wire(&upstream_output, &InputConnector::node(*node_id, insert_node_input_index), network_path);
 	}
 
+	/// Inserts the freshly-created `node_id` onto the wire feeding `input_connector`: the previous upstream becomes the
+	/// new node's primary (index 0) input, and the new node feeds `input_connector`.
+	///
+	/// When the wire is part of a layer's encapsulated primary chain, `set_input` chain-positions the new node
+	/// automatically. On an unencapsulated secondary-input branch (e.g. a 'Fill' node's fill input) chain positioning
+	/// doesn't apply, so the node would otherwise land at the graph origin; instead it's placed on the displaced
+	/// upstream node's spot and that whole branch is shifted left (in absolute graph space) to make room.
+	pub fn insert_node_before_input(&mut self, node_id: &NodeId, input_connector: &InputConnector, network_path: &[NodeId]) {
+		let feeder = self.upstream_output_connector(input_connector, network_path).and_then(|output| output.node_id());
+
+		let Some(current_input) = self.input_from_connector(input_connector, network_path).cloned() else {
+			log::error!("Could not get input in insert_node_before_input");
+			return;
+		};
+
+		if self.input_from_connector(&InputConnector::node(*node_id, 0), network_path).is_none() {
+			return;
+		}
+
+		self.set_input(&InputConnector::node(*node_id, 0), current_input, network_path);
+		self.set_input(input_connector, NodeInput::node(*node_id, 0), network_path);
+
+		// If `set_input` chain-positioned the node (it joined a layer chain), there's nothing more to do.
+		if !self.is_absolute(node_id, network_path) {
+			return;
+		}
+
+		// Otherwise place the node where the displaced feeder was, then shift the feeder's branch left to make room.
+		let Some(feeder) = feeder else { return };
+		let Some(node_position) = self.position(node_id, network_path) else { return };
+		let Some(feeder_position) = self.position(&feeder, network_path) else { return };
+
+		self.shift_node(node_id, feeder_position - node_position, network_path);
+		// Deduplicate, since `UpstreamFlow` can yield a shared node more than once and we must shift each node only once.
+		let upstream_nodes: HashSet<NodeId> = self.upstream_flow_back_from_nodes(vec![feeder], network_path, FlowType::UpstreamFlow).collect();
+		for upstream_node in &upstream_nodes {
+			self.shift_node(upstream_node, IVec2::new(-NODE_CHAIN_WIDTH, 0), network_path);
+		}
+	}
+
 	/// Moves a node to the start of a layer chain (feeding into the secondary input of the layer).
 	/// When `import` is true, uses lightweight wiring that skips `is_acyclic` checks and per-node cache invalidation.
 	pub fn move_node_to_chain_start(&mut self, node_id: &NodeId, parent: LayerNodeIdentifier, network_path: &[NodeId], import: bool) {

editor/src/messages/tool/tool_messages/gradient_tool.rs

@@ -2502,4 +2502,98 @@ mod test_gradient {
 		assert_eq!(SRGBA8::from(updated.stops.color[1]), SRGBA8::from(Color::GREEN), "Middle stop color should be preserved");
 		assert_eq!(SRGBA8::from(updated.stops.color[2]), SRGBA8::from(Color::BLUE), "Last stop color should be preserved");
 	}
+
+	// When the gradient chain feeds a 'Fill' node's secondary input it's an unencapsulated side-branch (no layer
+	// background to lay it out), so a node inserted there must be placed onto the displaced feeder's spot in absolute
+	// graph space rather than stranded at the origin.
+	#[tokio::test]
+	async fn gradient_chain_node_on_fill_secondary_input_takes_feeder_slot() {
+		use graphene_std::vector::style::GradientSpreadMethod;
+
+		let mut editor = EditorTestUtils::create();
+		editor.new_document().await;
+		editor.drag_tool(ToolType::Ellipse, 0., 0., 100., 100., ModifierKeys::empty()).await;
+		let layer = editor.active_document().metadata().all_layers().next().unwrap();
+
+		// Find the 'Fill' node in the layer's primary chain.
+		let fill_reference = DefinitionIdentifier::ProtoNode(graphene_std::vector::fill::IDENTIFIER);
+		let fill_node_id = {
+			let network_interface = &editor.active_document().network_interface;
+			network_interface
+				.document_network()
+				.nodes
+				.keys()
+				.copied()
+				.find(|node_id| network_interface.reference(node_id, &[]).as_ref() == Some(&fill_reference))
+				.expect("Fill node should exist")
+		};
+
+		// Feed a 'Gradient Value' node into the Fill node's secondary (fill) input.
+		let gradient_value_id = editor.create_node_by_name(DefinitionIdentifier::ProtoNode(graphene_std::math_nodes::gradient_value::IDENTIFIER)).await;
+		editor
+			.handle_message(NodeGraphMessage::CreateWire {
+				output_connector: OutputConnector::node(gradient_value_id, 0),
+				input_connector: InputConnector::node(fill_node_id, 1),
+			})
+			.await;
+		editor
+			.handle_message(NodeGraphMessage::SetInputValue {
+				node_id: gradient_value_id,
+				input_index: 1,
+				value: TaggedValue::Gradient(GradientStops::new([
+					GradientStop {
+						position: 0.,
+						midpoint: 0.5,
+						color: Color::RED,
+					},
+					GradientStop {
+						position: 1.,
+						midpoint: 0.5,
+						color: Color::BLUE,
+					},
+				])),
+			})
+			.await;
+
+		// Move the feeder off the origin so its slot is unambiguous, then record where it sits.
+		editor
+			.handle_message(NodeGraphMessage::ShiftNodePosition {
+				node_id: gradient_value_id,
+				x: 4,
+				y: 6,
+			})
+			.await;
+		let feeder_position = editor.active_document_mut().network_interface.position(&gradient_value_id, &[]).expect("Gradient Value position");
+
+		// Set the spread method through the tool, which splices a 'Spread Method' node onto the Fill's fill input wire.
+		editor.handle_message(NodeGraphMessage::SelectedNodesSet { nodes: vec![layer.to_node()] }).await;
+		editor.select_tool(ToolType::Gradient).await;
+		editor
+			.handle_message(GradientToolMessage::UpdateOptions {
+				options: GradientOptionsUpdate::SetSpreadMethod(GradientSpreadMethod::Reflect),
+			})
+			.await;
+
+		let spread_reference = DefinitionIdentifier::ProtoNode(graphene_std::math_nodes::spread_method::IDENTIFIER);
+		let spread_node_id = {
+			let network_interface = &editor.active_document().network_interface;
+			network_interface
+				.document_network()
+				.nodes
+				.keys()
+				.copied()
+				.find(|node_id| network_interface.reference(node_id, &[]).as_ref() == Some(&spread_reference))
+				.expect("Spread Method node should have been inserted")
+		};
+
+		let spread_position = editor.active_document_mut().network_interface.position(&spread_node_id, &[]).expect("Spread Method position");
+		let feeder_position_after = editor.active_document_mut().network_interface.position(&gradient_value_id, &[]).expect("Gradient Value position after");
+
+		assert_eq!(spread_position, feeder_position, "the inserted node should occupy the feeder's former slot, not the graph origin");
+		assert_eq!(
+			feeder_position_after,
+			feeder_position - glam::IVec2::new(crate::consts::NODE_CHAIN_WIDTH, 0),
+			"the feeder's branch should shift one chain-width left to make room"
+		);
+	}
 }