Fix hover transfer bugs occurring with the color picker popover (#4146)

* Fix hover transfer bugs occurring with the color picker popover

* Code review fix

Author: Keavon

editor/src/messages/color_picker/color_picker_message_handler.rs

@@ -65,14 +65,6 @@ impl MessageHandler<ColorPickerMessage, ()> for ColorPickerMessageHandler {
 				self.allow_none = allow_none;
 				self.disabled = disabled;
 
-				// Each `<ColorPicker>` Svelte instance maintains its own local layout state, but the Rust `LayoutMessageHandler` keeps a single shared layout per target. When a new picker instance opens after a previous one closed, the new instance's layout starts empty and a diff from the previously-shared state would not apply. Destroying the stored layouts here forces the next `SendLayout` to send the full layout instead of a diff.
-				responses.add(LayoutMessage::DestroyLayout {
-					layout_target: LayoutTarget::ColorPickerPickersAndGradient,
-				});
-				responses.add(LayoutMessage::DestroyLayout {
-					layout_target: LayoutTarget::ColorPickerDetails,
-				});
-
 				match initial_value {
 					FillChoice::None => {
 						self.set_new_hsva(0., 0., 0., 1., true);
@@ -99,10 +91,6 @@ impl MessageHandler<ColorPickerMessage, ()> for ColorPickerMessageHandler {
 				self.send_layouts(responses);
 			}
 			ColorPickerMessage::Close => {
-				self.gradient = None;
-				self.active_marker_index = None;
-				self.active_marker_is_midpoint = false;
-
 				responses.add(DocumentMessage::EndTransaction);
 			}
 			ColorPickerMessage::VisualUpdate { update } => {

frontend/src/components/floating-menus/ColorPicker.svelte

@@ -4,7 +4,7 @@
 	import LayoutCol from "/src/components/layout/LayoutCol.svelte";
 	import LayoutRow from "/src/components/layout/LayoutRow.svelte";
 	import WidgetLayout from "/src/components/widgets/WidgetLayout.svelte";
-	import type { ColorPickerStore } from "/src/stores/color-picker";
+	import type { ColorPickerCallbacks, ColorPickerStore } from "/src/stores/color-picker";
 	import type { EditorWrapper, FillChoice, MenuDirection } from "/wrapper/pkg/graphite_wasm_wrapper";
 
 	const dispatch = createEventDispatcher<{ colorOrGradient: FillChoice; startHistoryTransaction: undefined; commitHistoryTransaction: undefined }>();
@@ -27,23 +27,27 @@
 	// Open/close lifecycle: when `open` flips, register/clear the global callbacks (so events route to *this* instance)
 	// and tell the Rust handler to (re)initialize its state from the current `colorOrGradient`.
 	let lastOpen = false;
+	// Identity used by `clearCallbacks` to skip stale clears when another picker has already taken over the store's callbacks
+	let installedCallbacks: ColorPickerCallbacks | undefined;
 	$: handleOpenChange(open);
 
 	function handleOpenChange(isOpen: boolean) {
 		if (isOpen && !lastOpen) {
-			colorPickerStore.setCallbacks({
+			installedCallbacks = {
 				onColorChanged: (value) => dispatch("colorOrGradient", value),
 				onStartTransaction: () => dispatch("startHistoryTransaction"),
 				onCommitTransaction: () => dispatch("commitHistoryTransaction"),
-			});
+			};
+			colorPickerStore.setCallbacks(installedCallbacks);
 			editor.openColorPicker(colorOrGradient, allowNone, disabled);
 			// Auto-select the hex color code text input. Deferred so the layout has time to render after the picker opens.
 			setTimeout(() => {
 				const hexInput = self?.div()?.querySelector(".text-input input");
 				if (hexInput instanceof HTMLInputElement) hexInput.select();
 			}, 0);
 		} else if (!isOpen && lastOpen) {
-			colorPickerStore.clearCallbacks();
+			if (installedCallbacks) colorPickerStore.clearCallbacks(installedCallbacks);
+			installedCallbacks = undefined;
 			editor.closeColorPicker();
 		}
 		lastOpen = isOpen;
@@ -55,7 +59,8 @@
 
 	onDestroy(() => {
 		if (!lastOpen) return;
-		colorPickerStore.clearCallbacks();
+		if (installedCallbacks) colorPickerStore.clearCallbacks(installedCallbacks);
+		installedCallbacks = undefined;
 		editor.closeColorPicker();
 	});
 </script>

frontend/src/components/layout/FloatingMenu.svelte

@@ -336,7 +336,7 @@
 
 		// HOVER TRANSFER
 		// Transfer from this open floating menu to a sibling floating menu if the pointer hovers to a valid neighboring floating menu spawner
-		hoverTransfer(self, ownSpawner, targetSpawner);
+		if (strayCloses) hoverTransfer(self, ownSpawner, targetSpawner);
 
 		// POINTER STRAY
 		// Close the floating menu if the pointer has strayed far enough from its bounds (and it's not hovering over its own spawner)

frontend/src/components/widgets/inputs/TextInput.svelte

@@ -40,10 +40,16 @@
 		// The `unFocus()` call in `onTextChangeCanceled()` causes itself to be run again, so this if statement skips a second run
 		if (!editing) return;
 
+		// Capture before `onTextChangeCanceled` blurs the input
+		const currentValue = self?.getValue();
+
 		onTextChangeCanceled();
 
-		// TODO: Find a less hacky way to do this
-		if (self) dispatch("commitText", self.getValue());
+		// Only commit on a real edit, so a blur fired when the focused input is removed from the DOM (e.g., from a picker closing
+		// during hover transfer) doesn't round-trip the original value back to the backend and overwrite concurrent state.
+		if (self && currentValue !== undefined && currentValue !== value) {
+			dispatch("commitText", currentValue);
+		}
 
 		// Required if value is not changed by the parent component upon update:value event
 		self?.setInputElementValue(self.getValue());

frontend/src/stores/color-picker.ts

@@ -35,7 +35,8 @@ const { subscribe, update } = store;
 export type ColorPickerStore = {
 	subscribe: typeof subscribe;
 	setCallbacks: (callbacks: ColorPickerCallbacks) => void;
-	clearCallbacks: () => void;
+	// Identity-checked so a hover-transfer race where another picker's `setCallbacks` already replaced these doesn't clobber the new picker's callbacks
+	clearCallbacks: (expected: ColorPickerCallbacks) => void;
 	setDragging: (dragging: boolean) => void;
 };
 
@@ -88,10 +89,12 @@ export function createColorPickerStore(subscriptions: SubscriptionsRouter): Colo
 				return state;
 			});
 		},
-		clearCallbacks: () => {
+		clearCallbacks: (expected: ColorPickerCallbacks) => {
 			update((state) => {
-				state.callbacks = {};
 				state.isDragging = false;
+				if (state.callbacks === expected) {
+					state.callbacks = {};
+				}
 				return state;
 			});
 		},