From d40dc8eaf24a64e021475a04f43060ba4b3e198d Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sun, 19 Jul 2026 14:40:36 +0200 Subject: [PATCH] =?UTF-8?q?fix(studio):=20color=20picker=20expands=20inlin?= =?UTF-8?q?e=20=E2=80=94=20floating=20positioning=20removed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third real-world failure of computed floating placement (right-edge clip, top-edge clip, then detached at app center — webview coordinate spaces vs Retina logical/physical proved structurally fragile). The architecture changes instead of the math: the open picker renders display:contents with a flex-basis:100% editor that wraps below its own row in the panel flow — full control-column width, pushes content down, scrolls with the panel. Clipping and detachment are impossible by construction. Exclusivity via a panel-context signal (one picker open; re-click closes, another swatch switches), Escape handled locally inside the editor subtree; the round-4 live-drag HSV mirror is untouched. All floating code deleted: popover_position AND its 4 geometry tests (dead tested code is still dead code), trigger/popover primitives, measurement plumbing, popover CSS. Host rows gain flex-wrap only. --- .../src/components/color_picker/component.rs | 400 ++++++------------ .../src/components/color_picker/style.css | 57 +-- .../rustmotion-studio/src/editor/inspector.rs | 13 +- 3 files changed, 142 insertions(+), 328 deletions(-) diff --git a/crates/rustmotion-studio/src/components/color_picker/component.rs b/crates/rustmotion-studio/src/components/color_picker/component.rs index 2596346..4e593bc 100644 --- a/crates/rustmotion-studio/src/components/color_picker/component.rs +++ b/crates/rustmotion-studio/src/components/color_picker/component.rs @@ -1,9 +1,9 @@ +use std::sync::atomic::{AtomicU64, Ordering}; + use dioxus::prelude::*; use dioxus_primitives::color_picker::{self, Color, ColorAreaProps, ColorPickerContext}; use dioxus_primitives::label::Label; -use dioxus_primitives::popover; use dioxus_primitives::slider::*; -use dioxus_primitives::use_controlled; use palette::{encoding, FromColor, Hsv, IntoColor, RgbHue, Srgb}; use crate::components::input::Input; @@ -12,49 +12,32 @@ fn format_color_hex(color: Color) -> String { format!("#{color:X}") } -/// Compute the popover's fixed position from the anchor (trigger swatch) -/// rect, the popover size and the window viewport, all in logical pixels. -/// Preference: below the anchor, right edges aligned (opens leftward — the -/// inspector hugs the right window edge); flips above when the bottom would -/// clip; then a HARD clamp of both axes into `[margin, viewport - size - -/// margin]` — the clamp always wins, even after the flip. -pub fn popover_position( - anchor: (f64, f64, f64, f64), // x, y, w, h - popover: (f64, f64), // w, h - viewport: (f64, f64), // w, h - margin: f64, -) -> (f64, f64) { - const GAP: f64 = 4.0; - let (ax, ay, aw, ah) = anchor; - let (pw, ph) = popover; - let (vw, vh) = viewport; - - // Right-aligned under the anchor. - let x = ax + aw - pw; - let mut y = ay + ah + GAP; - // Flip above when the bottom clips. - if y + ph > vh - margin { - y = ay - ph - GAP; +/// Which color picker is currently expanded, shared across the inspector +/// panel so only ONE picker is open at a time: opening a swatch closes any +/// other. Provided by the panel; pickers fall back to a local open state when +/// the context is absent. +#[derive(Clone, Copy)] +pub struct OpenPicker(pub Signal>); + +/// Toggle decision for the exclusive open-picker state (pure): clicking the +/// open picker closes it, clicking any other opens that one. +pub fn toggle_picker(current: Option, id: u64) -> Option { + if current == Some(id) { + None + } else { + Some(id) } - // Hard clamp (min first so the margin wins on tiny viewports). - let cx = x.min(vw - pw - margin).max(margin); - let cy = y.min(vh - ph - margin).max(margin); - (cx, cy) } -#[derive(Clone, Copy)] -struct ColorPickerRootContext { - open: Memo, - disabled: ReadSignal, - color: ReadSignal>, - /// The trigger's mounted node — the popover anchors its fixed position on - /// this rect at open time. - trigger: Signal>>, +/// Stable per-instance picker id. +fn next_picker_id() -> u64 { + static NEXT: AtomicU64 = AtomicU64::new(1); + NEXT.fetch_add(1, Ordering::Relaxed) } -/// The props for the [`ColorPickerRoot`] component. +/// The props for the [`ColorPicker`] component. #[derive(Props, Clone, PartialEq)] -pub struct ColorPickerRootProps { +pub struct ColorPickerProps { /// The selected color #[props(default)] pub color: ReadSignal>, @@ -67,7 +50,12 @@ pub struct ColorPickerRootProps { #[props(default)] pub disabled: ReadSignal, - /// The controlled open state of the popover. + /// Optional label on the trigger button + #[props(default)] + pub label: Option, + + /// Unused (kept for call-site compatibility): the picker expands inline + /// and manages its own exclusive open state. pub open: ReadSignal>, /// The default open state when uncontrolled. @@ -82,17 +70,28 @@ pub struct ColorPickerRootProps { #[props(extends = GlobalAttributes)] pub attributes: Vec, - /// The children of the color picker element + /// Additional content to append to the expanded editor pub children: Element, } +/// # ColorPicker — inline expansion +/// +/// The open picker renders NO floating popover: it expands IN FLOW directly +/// below its row (`flex-basis:100%` wraps it to the next line of the +/// `flex-wrap` row), pushing the content below. No fixed/absolute +/// positioning, no measurement, no clamping — clipping is impossible by +/// construction and it scrolls naturally with the panel. +/// +/// Close: re-click on the swatch, Escape inside the picker subtree, or +/// opening another swatch (exclusive [`OpenPicker`] state). +/// +/// The local HSV mirror (live drag preview + guaranteed propagation) is kept +/// unchanged from the optimistic-edit round. #[component] -pub fn ColorPickerRoot(props: ColorPickerRootProps) -> Element { - let (open, set_open) = use_controlled(props.open, props.default_open, props.on_open_change); - +pub fn ColorPicker(props: ColorPickerProps) -> Element { // Local mirror of the color: the picker internals (area drag, hue slider, // hex field) read/write THIS state, so every selection — including - // mid-drag moves — previews live in the popover AND propagates through + // mid-drag moves — previews live AND propagates through // `on_color_change` immediately, independent of whether the parent echoes // the new color back through the `color` prop. let mut local = use_signal(|| (props.color)()); @@ -108,198 +107,90 @@ pub fn ColorPickerRoot(props: ColorPickerRootProps) -> Element { forward.call(c); }; - let trigger = use_signal(|| None); - use_context_provider(|| ColorPickerRootContext { - open, - disabled: props.disabled, - color: local.into(), - trigger, - }); + // Exclusive open state (panel-wide when the context is provided). + let id = use_hook(next_picker_id); + let shared_open = try_consume_context::(); + let local_open = use_signal(|| false); + let is_open = match shared_open { + Some(s) => (s.0)() == Some(id), + None => local_open(), + }; - rsx! { - color_picker::ColorPicker { - class: "dx-color-picker", - color: local(), - on_color_change: on_change, - disabled: props.disabled, - attributes: props.attributes, - popover::PopoverRoot { - is_modal: false, - open: Some(open()), - on_open_change: move |v| set_open.call(v), - {props.children} + let on_open_change = props.on_open_change; + let toggle = move |_| { + let now_open = match shared_open { + Some(s) => { + let mut sig = s.0; + let next = toggle_picker(sig(), id); + sig.set(next); + next == Some(id) } - } - } -} - -/// The props for the [`ColorPicker`] component. -#[derive(Props, Clone, PartialEq)] -pub struct ColorPickerProps { - /// The selected color - #[props(default)] - pub color: ReadSignal>, - - /// Callback when color changes - #[props(default)] - pub on_color_change: Callback>, - - /// Whether the color picker is disabled - #[props(default)] - pub disabled: ReadSignal, - - /// Optional label on the trigger button - #[props(default)] - pub label: Option, - - /// The controlled open state of the popover. - pub open: ReadSignal>, - - /// The default open state when uncontrolled. - #[props(default)] - pub default_open: bool, - - /// Callback fired when the open state changes. - #[props(default)] - pub on_open_change: Callback, - - /// Additional attributes to extend the color picker element - #[props(extends = GlobalAttributes)] - pub attributes: Vec, - - /// Additional content to append to the default color picker popover - pub children: Element, -} - -#[component] -pub fn ColorPicker(props: ColorPickerProps) -> Element { - rsx! { - ColorPickerRoot { - color: props.color, - on_color_change: props.on_color_change, - disabled: props.disabled, - open: props.open, - default_open: props.default_open, - on_open_change: props.on_open_change, - attributes: props.attributes, - ColorPickerTrigger { - label: props.label, + None => { + let mut l = local_open; + let v = !l(); + l.set(v); + v + } + }; + on_open_change.call(now_open); + }; + let close = move || { + match shared_open { + Some(s) => { + let mut sig = s.0; + if sig() == Some(id) { + sig.set(None); + } } - ColorPickerPopover { - ColorPickerSelect {} - {props.children} + None => { + let mut l = local_open; + l.set(false); } } - } -} - -/// The props for the [`ColorPickerTrigger`] component. -#[derive(Props, Clone, PartialEq)] -pub struct ColorPickerTriggerProps { - /// Optional label on the trigger button - #[props(default)] - pub label: Option, - - /// Additional attributes to extend the trigger button - #[props(extends = GlobalAttributes)] - pub attributes: Vec, - - /// Additional content to render inside the trigger button - pub children: Element, -} + on_open_change.call(false); + }; -#[component] -pub fn ColorPickerTrigger(props: ColorPickerTriggerProps) -> Element { - let ctx = use_context::(); let aria_hex = use_memo(move || { - let rgb: Color = Srgb::::from_color((ctx.color)()).into_format(); + let rgb: Color = Srgb::::from_color(local()).into_format(); format_color_hex(rgb) }); - let mut trigger = ctx.trigger; rsx! { - div { - style: "display:inline-flex;", - onmounted: move |e| trigger.set(Some(e.data())), - popover::PopoverTrigger { + color_picker::ColorPicker { + class: "dx-color-picker", + // `display:contents`: the trigger button and the inline editor + // become direct items of the surrounding `flex-wrap` row, so the + // editor wraps below the row at full row width. + style: "display:contents;", + color: local(), + on_color_change: on_change, + disabled: props.disabled, + attributes: props.attributes, + button { class: "dx-color-picker-button", - disabled: if (ctx.disabled)() { true }, - aria_label: format!("Color picker {aria_hex}"), - aria_expanded: (ctx.open)(), - attributes: props.attributes, - ColorSwatch { color: ctx.color } - if let Some(label) = props.label { span { {label} } } - {props.children} - } - } - } -} - -/// The props for the [`ColorPickerPopover`] component. -#[derive(Props, Clone, PartialEq)] -pub struct ColorPickerPopoverProps { - /// Additional attributes to extend the popover content - #[props(extends = GlobalAttributes)] - pub attributes: Vec, - - /// The children of the color picker popover - pub children: Element, -} - -#[component] -pub fn ColorPickerPopover(props: ColorPickerPopoverProps) -> Element { - let ctx = use_context::(); - // Fixed positioning computed at open time by [`popover_position`]: below - // the swatch opening leftward, flipped above near the window bottom, and - // hard-clamped into the viewport on both axes. `position:fixed` escapes - // every scrollable/clipping container of the panel. - let mut node = use_signal(|| None::>); - let mut pos = use_signal(|| None::<(f64, f64)>); - use_effect(move || { - if (ctx.open)() { - if let (Some(t), Some(n)) = ((ctx.trigger)(), node()) { - spawn(async move { - let (Ok(anchor), Ok(content)) = - (t.get_client_rect().await, n.get_client_rect().await) - else { - return; - }; - let win = dioxus::desktop::window(); - let vs = win.inner_size().to_logical::(win.scale_factor()); - // The measured node is the inner content: compensate for - // the popover's own padding (12px each side). - const PAD: f64 = 24.0; - pos.set(Some(popover_position( - ( - anchor.origin.x, - anchor.origin.y, - anchor.size.width, - anchor.size.height, - ), - (content.size.width + PAD, content.size.height + PAD), - (vs.width, vs.height), - 8.0, - ))); - }); + disabled: (props.disabled)(), + aria_label: "Color picker {aria_hex}", + aria_expanded: is_open, + onclick: toggle, + ColorSwatch { color: local } + if let Some(label) = props.label { + span { {label} } + } } - } - }); - let style = match pos() { - Some((x, y)) => { - format!("position:fixed; left:{x}px; top:{y}px; right:auto; bottom:auto; margin:0;") - } - // Not measured yet (first open): keep it invisible for one frame. - None => "visibility:hidden;".to_string(), - }; - - rsx! { - popover::PopoverContent { - class: "dx-color-picker-popover".to_string(), - style: "{style}", - attributes: props.attributes, - div { - onmounted: move |e| node.set(Some(e.data())), - {props.children} + if is_open { + div { + class: "dx-color-picker-inline", + // Escape closes THIS picker; handled locally because the + // panel wrapper stops keydown propagation to the app root. + onkeydown: move |evt: KeyboardEvent| { + if evt.key() == Key::Escape { + evt.stop_propagation(); + close(); + } + }, + ColorPickerSelect {} + {props.children} + } } } } @@ -580,60 +471,15 @@ pub fn ColorPickerSelect(props: ColorPickerSelectProps) -> Element { #[cfg(test)] mod tests { - use super::popover_position; - - #[test] - fn anchor_top_right_opens_below_clamped_into_viewport() { - // Swatch near the top-right corner of a 1200x800 window. - let (x, y) = popover_position( - (1150.0, 60.0, 24.0, 24.0), - (260.0, 320.0), - (1200.0, 800.0), - 8.0, - ); - // Below the anchor… - assert_eq!(y, 60.0 + 24.0 + 4.0); - // …right-aligned would be 1174-260=914, fits; but never past the right margin. - assert_eq!(x, 914.0); - assert!(x + 260.0 <= 1200.0 - 8.0); - } - - #[test] - fn anchor_near_bottom_flips_above() { - let (x, y) = popover_position( - (900.0, 700.0, 24.0, 24.0), - (260.0, 320.0), - (1200.0, 800.0), - 8.0, - ); - // 700+24+4+320 > 792 → flip above: 700-320-4 = 376. - assert_eq!(y, 376.0); - assert_eq!(x, 900.0 + 24.0 - 260.0); - } - - #[test] - fn anchor_top_left_never_clips_left_or_top() { - // Right-aligned x would be negative → clamped to the margin. - let (x, y) = popover_position( - (10.0, 10.0, 24.0, 24.0), - (260.0, 320.0), - (1200.0, 800.0), - 8.0, - ); - assert_eq!(x, 8.0); - assert_eq!(y, 10.0 + 24.0 + 4.0); - } + use super::toggle_picker; #[test] - fn tiny_viewport_clamps_to_margins() { - // Popover larger than the window: both axes pinned at the margin - // (the clamp always wins, even after the flip). - let (x, y) = popover_position( - (50.0, 90.0, 24.0, 24.0), - (260.0, 320.0), - (200.0, 150.0), - 8.0, - ); - assert_eq!((x, y), (8.0, 8.0)); + fn only_one_picker_open_at_a_time() { + // Opening from nothing. + assert_eq!(toggle_picker(None, 7), Some(7)); + // Re-click on the open picker closes it. + assert_eq!(toggle_picker(Some(7), 7), None); + // Clicking ANOTHER swatch switches to it (previous one closes). + assert_eq!(toggle_picker(Some(7), 9), Some(9)); } } diff --git a/crates/rustmotion-studio/src/components/color_picker/style.css b/crates/rustmotion-studio/src/components/color_picker/style.css index d7a4f07..16a7cf1 100644 --- a/crates/rustmotion-studio/src/components/color_picker/style.css +++ b/crates/rustmotion-studio/src/components/color_picker/style.css @@ -172,55 +172,20 @@ color-mix(in oklab, var(--focused-border-color) 50%, transparent); } -.dx-color-picker-popover { - position: absolute; - z-index: 1000; - top: 100%; - /* The inspector docks against the right window edge: anchor the popover's - right edge to the swatch so it opens LEFTWARD instead of clipping. */ - right: 0; - left: auto; - display: block; - border-radius: 0.5rem; +/* Inline expansion: the open editor lives IN FLOW below its row + (flex-basis:100% wraps it in the flex-wrap row) — no positioning, no + clipping, scrolls with the panel. */ +.dx-color-picker-inline { + flex-basis: 100%; + width: 100%; + box-sizing: border-box; margin-top: 4px; - padding: 12px; - background: var(--rm-surface); - box-shadow: - inset 0 0 0 1px var(--rm-border), - 0 8px 24px rgba(0, 0, 0, 0.35); -} - -.dx-color-picker-popover[data-state="open"] { - animation: dx-color-picker-popover-fade-in .15s ease-out; -} - -.dx-color-picker-popover[data-state="closed"] { - animation: dx-color-picker-popover-fade-out .15s ease-out forwards; - pointer-events: none; -} - -@keyframes dx-color-picker-popover-fade-in { - from { - opacity: 0; - transform: translateY(-4px); - } - - to { - opacity: 1; - transform: translateY(0); - } + padding: 10px; + border-radius: 0.5rem; + background: var(--rm-surface-2); + box-shadow: inset 0 0 0 1px var(--rm-border); } -@keyframes dx-color-picker-popover-fade-out { - from { - opacity: 1; - transform: translateY(0); - } - - to { - opacity: 0; - transform: translateY(-4px); - } } .dx-color-picker-dialog { diff --git a/crates/rustmotion-studio/src/editor/inspector.rs b/crates/rustmotion-studio/src/editor/inspector.rs index aaee6be..a6a1e1c 100644 --- a/crates/rustmotion-studio/src/editor/inspector.rs +++ b/crates/rustmotion-studio/src/editor/inspector.rs @@ -610,6 +610,9 @@ pub fn InspectorPanel( ) -> Element { // Provide the debounce handle so all child write helpers share one slot. use_context_provider(|| WriteDebounce(Rc::new(RefCell::new(None)))); + // One expanded color picker at a time across the whole panel. + let open_picker = use_signal(|| None::); + use_context_provider(|| crate::components::color_picker::OpenPicker(open_picker)); let fam = family(&kind); rsx! { div { style: "width:300px; flex:none; min-height:0; background:var(--rm-surface); border-left:1px solid var(--rm-border); box-sizing:border-box; display:flex; flex-direction:column; overflow:auto;", @@ -969,7 +972,7 @@ fn GenericRow( let pick_commit = commit.clone(); let hex_commit = commit.clone(); rsx! { - div { style: "display:flex; align-items:center; gap:6px; width:100%;", + div { style: "display:flex; flex-wrap:wrap; align-items:center; gap:6px; width:100%;", ColorPicker { color: color(), on_color_change: move |c: Hsv| { @@ -1422,7 +1425,7 @@ fn FieldRow(field: Field, pointer: String, style: serde_json::Value) -> Element Ctrl::Color { clearable } => { let mut color = use_signal(|| parse_hsv(&value)); rsx! { - div { style: "display:flex; align-items:center; gap:6px; width:100%;", + div { style: "display:flex; flex-wrap:wrap; align-items:center; gap:6px; width:100%;", ColorPicker { color: color(), on_color_change: { @@ -1602,7 +1605,7 @@ fn NumberListControl(pointer: String, name: String, value: String) -> Element { rsx! { div { style: "display:flex; flex-direction:column; gap:6px; width:100%;", for (i, e) in entries().iter().cloned().enumerate() { - div { key: "{i}", style: "display:flex; align-items:center; gap:6px;", + div { key: "{i}", style: "display:flex; flex-wrap:wrap; align-items:center; gap:6px;", input { r#type: "number", step: "0.1", @@ -1664,7 +1667,7 @@ fn ColorRows(colors: Signal>, on_change: EventHandler<()>) -> Elemen rsx! { div { style: "display:flex; flex-direction:column; gap:6px; width:100%;", for (i, c) in colors().iter().cloned().enumerate() { - div { key: "{i}", style: "display:flex; align-items:center; gap:6px;", + div { key: "{i}", style: "display:flex; flex-wrap:wrap; align-items:center; gap:6px;", ColorPicker { color: parse_hsv(&c), on_color_change: { @@ -1807,7 +1810,7 @@ fn FillControl(pointer: String, name: String, value: String) -> Element { {seg(FillMode::Radial, "Radial")} } if mode() == FillMode::Single { - div { style: "display:flex; align-items:center; gap:6px;", + div { style: "display:flex; flex-wrap:wrap; align-items:center; gap:6px;", ColorPicker { color: parse_hsv(colors.read().first().map(String::as_str).unwrap_or("#ffffff")), on_color_change: {