diff --git a/crates/rustmotion-components/src/badge.rs b/crates/rustmotion-components/src/badge.rs index d0f4598..04ea5e2 100644 --- a/crates/rustmotion-components/src/badge.rs +++ b/crates/rustmotion-components/src/badge.rs @@ -2,6 +2,7 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use skia_safe::{Canvas, ColorType, ImageInfo, Paint, PaintStyle, RRect, Rect}; +use rustmotion_core::css::style::AlignSelf; use rustmotion_core::css::CssStyle; use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; @@ -65,7 +66,17 @@ pub struct Badge { pub count: Option, #[serde(flatten)] pub timing: TimingConfig, - #[serde(default)] + /// `align-self` defaults to `flex-start` (not the flex container's + /// `stretch`) — a badge is an atomic icon+label chip that must keep its + /// natural intrinsic width even inside a `flex`/`card` column; without + /// this, the default cross-axis `stretch` wins over `BadgeIntrinsic` + /// and the chip becomes a full-width bar. An author-specified + /// `align-self` in JSON is always respected (this only fills the gap + /// when it's absent). + #[serde( + default = "default_badge_style", + deserialize_with = "deserialize_no_stretch_style" + )] pub style: CssStyle, #[serde(default)] pub timeline: Vec, @@ -73,6 +84,27 @@ pub struct Badge { pub stagger: Option, } +fn default_badge_style() -> CssStyle { + CssStyle { + align_self: Some(AlignSelf::FlexStart), + ..CssStyle::default() + } +} + +/// Deserializes `style` normally, then defaults `align-self` to +/// `flex-start` when the author didn't set it explicitly — see the doc +/// comment on [`Badge::style`]. +fn deserialize_no_stretch_style<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let mut style = CssStyle::deserialize(deserializer)?; + if style.align_self.is_none() { + style.align_self = Some(AlignSelf::FlexStart); + } + Ok(style) +} + rustmotion_core::impl_traits!(Badge { Animatable => animation, Timed => timing, @@ -319,3 +351,39 @@ impl Painter for Badge { self.paint(canvas, layout.width, layout.height, ctx.time); } } + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(json: &str) -> Badge { + serde_json::from_str(json).expect("badge should deserialize") + } + + #[test] + fn style_defaults_to_flex_start_when_absent() { + // #127: `align-items: stretch` (the flex column default) was + // winning over `BadgeIntrinsic`, stretching every badge in a card + // to the container's full width. No `style` key at all in the + // JSON is the common case — this must still default away from + // stretch. + let badge = parse(r#"{"type":"badge","text":"v1"}"#); + assert_eq!(badge.style.align_self, Some(AlignSelf::FlexStart)); + } + + #[test] + fn style_defaults_to_flex_start_with_other_style_keys_present() { + // Same fix, but exercised through the `deserialize_with` path + // (some `style` object present, just not `align-self`) rather + // than the field-level `default` path (`style` entirely absent). + let badge = parse(r##"{"type":"badge","text":"v1","style":{"background":"#f00"}}"##); + assert_eq!(badge.style.align_self, Some(AlignSelf::FlexStart)); + assert_eq!(badge.style.background_color_str(), Some("#f00")); + } + + #[test] + fn explicit_align_self_is_respected() { + let badge = parse(r#"{"type":"badge","text":"v1","style":{"align-self":"center"}}"#); + assert_eq!(badge.style.align_self, Some(AlignSelf::Center)); + } +} diff --git a/crates/rustmotion-components/src/caption.rs b/crates/rustmotion-components/src/caption.rs index f979c49..f4da120 100644 --- a/crates/rustmotion-components/src/caption.rs +++ b/crates/rustmotion-components/src/caption.rs @@ -43,7 +43,7 @@ rustmotion_core::impl_traits!(Caption { }); impl Caption { - fn paint(&self, canvas: &Canvas, layout_width: f32, time: f64) { + fn paint(&self, canvas: &Canvas, layout_width: f32, layout_height: f32, time: f64) { let font_size = self.style.font_size_px_or(48.0); let color = self.style.color_str_or("#FFFFFF"); let font_family = self.style.font_family_or("Inter"); @@ -55,6 +55,37 @@ impl Caption { let font = Font::from_typeface(typeface, font_size); let emoji_font = emoji_typeface().map(|tf| Font::from_typeface(tf, font_size)); + // Every branch below draws text with its *baseline* at local y=0 and + // (for the pill-background presets) a highlight box extending up to + // `font_size + padding/2` above that baseline. Treated as the box's + // own coordinate space (y=0 = box top, as every other painter + // assumes), that put glyphs — and pills further still — above the + // assigned box: `CaptionIntrinsic` sizes the box for one line's + // ascent+descent starting at y=0, not for a baseline sitting at 0 + // with ascenders going negative. Shifting the whole paint down by a + // margin that covers the tallest pill (WordPop's `font_size*0.35` + // padding, ~1.175×font_size) puts the topmost ink at/after y=0 + // without touching any of the per-preset layout math below. + let top_offset = font_size * 1.2; + canvas.save(); + canvas.translate((0.0, top_offset)); + // Safety net: even with the offset above, an unusually large pill + // padding combined with a short assigned box could still spill past + // the bottom edge. Clip vertically only (not horizontally) — a + // caption in `white-space: nowrap` mode is *meant* to bleed past + // its own width when `max_width` doesn't fit the line (see + // `box_builder.rs`'s nowrap comment and the geometry validator's + // `unwrappable_text_overflow`), so clipping width here would hide a + // condition the validator is supposed to catch instead. + if layout_height > 0.0 { + const HALF_PLANE: f32 = 1_000_000.0; + canvas.clip_rect( + Rect::from_xywh(-HALF_PLANE, -top_offset, HALF_PLANE * 2.0, layout_height), + skia_safe::ClipOp::Intersect, + true, + ); + } + match self.mode { CaptionStyle::WordByWord => { for word in &self.words { @@ -249,6 +280,7 @@ impl Caption { } } } + canvas.restore(); } } @@ -269,7 +301,7 @@ impl Painter for Caption { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - self.paint(canvas, layout.width, ctx.time); + self.paint(canvas, layout.width, layout.height, ctx.time); } } @@ -364,6 +396,48 @@ mod tests { (minx <= maxx).then_some((minx, maxx, miny, maxy)) } + #[test] + fn ink_never_starts_above_the_box_top() { + // #127: every branch drew its baseline at local y=0 (the box's own + // top edge) — ascenders, and pill backgrounds further still, + // painted *above* y=0, bleeding out of whatever box the layout + // gave this caption (measured: assigned box top at y=124 in a + // card starting at y=100, but ink started at y≈85 — above the + // card itself, not just the box). + let caption = make_caption("Hello world", None); + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + caption.paint(canvas, W as f32, H as f32, 0.5); + } + let (_minx, _maxx, miny, _maxy) = + ink_bounds(&mut surface, W, H).expect("caption must paint something"); + assert!(miny >= 0, "ink starts above the box top at y={miny}"); + } + + #[test] + fn word_pop_pill_never_starts_above_the_box_top() { + // The pill-background presets pad further above the baseline than + // plain text (up to `font_size * 0.35` extra) — the worst case + // among the four rendering modes. + let mut caption = make_caption("Hello", None); + caption.mode = CaptionStyle::WordPop; + caption.words[0].start = 0.0; + caption.words[0].end = 10.0; + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + caption.paint(canvas, W as f32, H as f32, 0.1); + } + let (_minx, _maxx, miny, _maxy) = + ink_bounds(&mut surface, W, H).expect("word_pop caption must paint something"); + assert!(miny >= 0, "pill starts above the box top at y={miny}"); + } + #[test] fn nowrap_paints_one_wide_line_instead_of_wrapping_at_max_width() { // M1 render-level proof, caption (Highlight mode): `white-space: @@ -379,7 +453,7 @@ mod tests { { let canvas = surface.canvas(); canvas.translate((800.0, 250.0)); - caption.paint(canvas, 80.0, 0.5); + caption.paint(canvas, 80.0, H as f32, 0.5); } let (minx, maxx, miny, maxy) = ink_bounds(&mut surface, W, H).expect("nowrap caption must paint something"); @@ -405,7 +479,7 @@ mod tests { { let canvas = surface.canvas(); canvas.translate((800.0, 250.0)); - caption.paint(canvas, 80.0, 0.5); + caption.paint(canvas, 80.0, H as f32, 0.5); } let (minx, maxx, miny, maxy) = ink_bounds(&mut surface, W, H).expect("wrapped caption must paint something"); diff --git a/crates/rustmotion-components/src/countdown.rs b/crates/rustmotion-components/src/countdown.rs index f7446a8..e3464f7 100644 --- a/crates/rustmotion-components/src/countdown.rs +++ b/crates/rustmotion-components/src/countdown.rs @@ -2,7 +2,8 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use skia_safe::{Canvas, PaintStyle, RRect, Rect}; -use rustmotion_core::css::CssStyle; +use rustmotion_core::css::style::Size as CSize; +use rustmotion_core::css::{CssStyle, LengthPercentage as CLP}; use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::engine::renderer::{ @@ -44,7 +45,18 @@ fn default_border_radius() -> f32 { 12.0 } +/// `#[serde(from = "CountdownRaw")]`: `box_builder.rs`'s width formula for +/// this component (`visible * 2*box_w + (visible-1)*gap`) doesn't include +/// the small inner gap (`gap*0.3`) `paint()` actually inserts *within* +/// each digit pair — only the gap *between* pairs (the separator). That +/// mismatch compounds with every extra group and is exactly what #127 +/// measured (153×67 assigned vs. 187×68 painted). Since `paint()` draws +/// from `self.digit_size`/`self.gap`, not `layout.width` (same pattern as +/// `switch`/`slider`), the fix is to fold the *exact* formula `paint()` +/// uses into `style.width` here, so `box_builder.rs`'s `if +/// css.width.is_none()` never gets a chance to apply its slightly-off one. #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(from = "CountdownRaw")] pub struct Countdown { #[serde(default = "default_seconds")] pub seconds: f64, @@ -66,7 +78,7 @@ pub struct Countdown { pub gap: f32, #[serde(default = "default_border_radius")] pub border_radius: f32, - #[serde(flatten)] + #[serde(default)] pub timing: TimingConfig, #[serde(default)] pub style: CssStyle, @@ -76,6 +88,100 @@ pub struct Countdown { pub stagger: Option, } +#[derive(Debug, Deserialize)] +struct CountdownRaw { + #[serde(default = "default_seconds")] + seconds: f64, + #[serde(default = "default_true")] + show_hours: bool, + #[serde(default = "default_true")] + show_minutes: bool, + #[serde(default = "default_true")] + show_seconds: bool, + #[serde(default = "default_digit_size")] + digit_size: f32, + #[serde(default = "default_digit_color")] + digit_color: String, + #[serde(default = "default_digit_background")] + digit_background: String, + #[serde(default = "default_separator_color")] + separator_color: String, + #[serde(default = "default_gap")] + gap: f32, + #[serde(default = "default_border_radius")] + border_radius: f32, + #[serde(flatten)] + timing: TimingConfig, + #[serde(default)] + style: CssStyle, + #[serde(default)] + timeline: Vec, + #[serde(default)] + stagger: Option, +} + +impl From for Countdown { + fn from(raw: CountdownRaw) -> Self { + let mut style = raw.style; + let box_w = raw.digit_size * 0.75; + let box_h = raw.digit_size * 1.2; + if style.width.is_none() { + let w = countdown_total_width( + box_w, + raw.gap, + raw.show_hours, + raw.show_minutes, + raw.show_seconds, + ); + style.width = Some(CSize::Length(CLP::Px(w))); + } + if style.height.is_none() { + style.height = Some(CSize::Length(CLP::Px(box_h))); + } + Countdown { + seconds: raw.seconds, + show_hours: raw.show_hours, + show_minutes: raw.show_minutes, + show_seconds: raw.show_seconds, + digit_size: raw.digit_size, + digit_color: raw.digit_color, + digit_background: raw.digit_background, + separator_color: raw.separator_color, + gap: raw.gap, + border_radius: raw.border_radius, + timing: raw.timing, + style, + timeline: raw.timeline, + stagger: raw.stagger, + } + } +} + +/// Total ink width `paint()`'s cursor walk produces: each visible group is +/// two digit boxes plus one inner gap (`gap*0.3`), and every group after +/// the first is preceded by a separator that consumes a full `gap`. Shared +/// by `From` (to size the box) and `paint()`'s own `total_w` +/// so the two can never drift apart again. +fn countdown_total_width( + box_w: f32, + gap: f32, + show_hours: bool, + show_minutes: bool, + show_seconds: bool, +) -> f32 { + let visible = [show_hours, show_minutes, show_seconds] + .iter() + .filter(|v| **v) + .count() as f32; + if visible <= 0.0 { + return 0.0; + } + let inner_gap = gap * 0.3; + let digit_pair_w = box_w * 2.0 + inner_gap; + let separators = (visible - 1.0).max(0.0); + digit_pair_w * visible + gap * separators +} + rustmotion_core::impl_traits!(Countdown { Animatable => animation, Timed => timing, @@ -170,7 +276,7 @@ impl Countdown { } impl Countdown { - fn paint(&self, canvas: &Canvas, time: f64) { + fn paint(&self, canvas: &Canvas, layout: &BoxLayout, time: f64) { let remaining = (self.seconds - time).max(0.0); let total_secs = remaining as u64; let hours = total_secs / 3600; @@ -187,28 +293,23 @@ impl Countdown { let (box_w, _box_h) = self.digit_box_size(); - // Calculate total width for centering - let mut num_groups = 0; - if self.show_hours { - num_groups += 1; - } - if self.show_minutes { - num_groups += 1; + // Safety net: `style.width`/`style.height` on this struct are + // already sized (see `From`) to match this exact + // cursor walk, so this shouldn't ever clip in practice — it's a + // backstop against drift if the two formulas are ever edited + // separately again. + canvas.save(); + if layout.width > 0.0 && layout.height > 0.0 { + canvas.clip_rect( + Rect::from_xywh(0.0, 0.0, layout.width, layout.height), + skia_safe::ClipOp::Intersect, + true, + ); } - if self.show_seconds { - num_groups += 1; - } - - let digit_pair_w = box_w * 2.0 + self.gap * 0.3; // two digit boxes + small inner gap - let separators = if num_groups > 1 { num_groups - 1 } else { 0 }; - let total_w = digit_pair_w * num_groups as f32 + self.gap * separators as f32; let mut cursor_x = 0.0_f32; let cursor_y = 0.0_f32; - // Center the whole thing if we have a layout - let _ = total_w; // used for measure - let inner_gap = self.gap * 0.3; let mut need_separator = false; @@ -251,6 +352,8 @@ impl Countdown { cursor_x += box_w + inner_gap; self.draw_digit_box(canvas, cursor_x, cursor_y, chars[1], &font, &emoji_font); } + + canvas.restore(); } } @@ -258,10 +361,67 @@ impl Painter for Countdown { fn paint_content( &self, canvas: &Canvas, - _layout: &BoxLayout, + layout: &BoxLayout, _props: &AnimatedProperties, ctx: &PaintCtx, ) { - self.paint(canvas, ctx.time); + self.paint(canvas, layout, ctx.time); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(json: &str) -> Countdown { + serde_json::from_str(json).expect("countdown should deserialize") + } + + fn px_width(c: &Countdown) -> f32 { + let CSize::Length(CLP::Px(w)) = c.style.width.clone().expect("style.width should be set") + else { + panic!("expected an explicit px width"); + }; + w + } + + #[test] + fn countdown_total_width_matches_paints_own_cursor_walk() { + // #127: box_builder.rs's width formula (`visible*2*box_w + + // (visible-1)*gap`) omits the small inner gap `paint()` inserts + // *within* each digit pair, so the box came out narrower than + // what actually got drawn (153×67 assigned vs. 187×68 painted). + // Recompute by hand here (independent of `countdown_total_width` + // itself) so this test would catch drift in either direction. + let box_w = 42.0_f32; // digit_size 56 * 0.75 + let gap = default_gap(); + let inner_gap = gap * 0.3; + // two visible groups (minutes, seconds): two pairs + one separator + let by_hand = (box_w * 2.0 + inner_gap) * 2.0 + gap; + let got = countdown_total_width(box_w, gap, false, true, true); + assert!( + (got - by_hand).abs() < 0.001, + "formula drift: got {got}, hand-computed {by_hand}" + ); + } + + #[test] + fn style_width_matches_the_shared_formula() { + let c = parse(r#"{"type":"countdown","show_hours":false,"digit_size":56}"#); + let expected = countdown_total_width(56.0 * 0.75, c.gap, false, true, true); + assert!((px_width(&c) - expected).abs() < 0.001); + } + + #[test] + fn explicit_style_width_is_never_overridden() { + let c = parse(r#"{"type":"countdown","style":{"width":900}}"#); + assert_eq!(px_width(&c), 900.0); + } + + #[test] + fn no_visible_groups_is_zero_not_negative() { + // Defensive: `(visible - 1.0)` must not go negative and blow up + // `separators` when nothing is shown. + assert_eq!(countdown_total_width(42.0, 12.0, false, false, false), 0.0); } } diff --git a/crates/rustmotion-components/src/kbd.rs b/crates/rustmotion-components/src/kbd.rs index 0512174..28d0cb7 100644 --- a/crates/rustmotion-components/src/kbd.rs +++ b/crates/rustmotion-components/src/kbd.rs @@ -2,6 +2,7 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use skia_safe::{Canvas, PaintStyle, Rect}; +use rustmotion_core::css::style::AlignSelf; use rustmotion_core::css::CssStyle; use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; @@ -41,7 +42,16 @@ pub struct Kbd { pub text_color: String, #[serde(flatten)] pub timing: TimingConfig, - #[serde(default)] + /// `align-self` defaults to `flex-start` (not the flex container's + /// `stretch`) — a keycap is an atomic chip that must keep its natural + /// intrinsic width even inside a `flex`/`card` column; without this, + /// the default cross-axis `stretch` wins over `KbdIntrinsic` and the + /// key renders as a full-width slab. An author-specified `align-self` + /// in JSON is always respected (this only fills the gap when absent). + #[serde( + default = "default_kbd_style", + deserialize_with = "deserialize_no_stretch_style" + )] pub style: CssStyle, #[serde(default)] pub timeline: Vec, @@ -49,6 +59,27 @@ pub struct Kbd { pub stagger: Option, } +fn default_kbd_style() -> CssStyle { + CssStyle { + align_self: Some(AlignSelf::FlexStart), + ..CssStyle::default() + } +} + +/// Deserializes `style` normally, then defaults `align-self` to +/// `flex-start` when the author didn't set it explicitly — see the doc +/// comment on [`Kbd::style`]. +fn deserialize_no_stretch_style<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let mut style = CssStyle::deserialize(deserializer)?; + if style.align_self.is_none() { + style.align_self = Some(AlignSelf::FlexStart); + } + Ok(style) +} + rustmotion_core::impl_traits!(Kbd { Animatable => animation, Timed => timing, @@ -76,9 +107,15 @@ impl Kbd { .background_color_str() .unwrap_or(&self.background_color); - // Shadow (bottom edge to simulate physical key depth) - let shadow_h = 3.0; - let shadow_rect = Rect::from_xywh(0.0, shadow_h, w, h); + // Shadow (bottom edge to simulate physical key depth), and the key + // face sitting on top of it. Both are inset to `h - shadow_h` tall + // so the "3D lip" the shadow peeks out from under the face stays + // inside the assigned box — the face used to be drawn full-height + // with the shadow offset *below* it, bleeding `shadow_h` px past + // the box bottom every frame. + let shadow_h = 3.0_f32.min(h); + let cap_h = (h - shadow_h).max(0.0); + let shadow_rect = Rect::from_xywh(0.0, shadow_h, w, cap_h); let shadow_rrect = skia_safe::RRect::new_rect_xy(shadow_rect, radius, radius); let mut shadow_paint = paint_from_hex(&self.border_color); shadow_paint.set_style(PaintStyle::Fill); @@ -86,7 +123,7 @@ impl Kbd { canvas.draw_rrect(shadow_rrect, &shadow_paint); // Key face background - let face_rect = Rect::from_xywh(0.0, 0.0, w, h); + let face_rect = Rect::from_xywh(0.0, 0.0, w, cap_h); let face_rrect = skia_safe::RRect::new_rect_xy(face_rect, radius, radius); let mut face_paint = paint_from_hex(bg_color); face_paint.set_style(PaintStyle::Fill); @@ -114,7 +151,7 @@ impl Kbd { let text_w = measure_text_with_fallback(&self.key, &font, &emoji_font, 0.0); let (_, metrics) = font.metrics(); let text_x = (w - text_w) / 2.0; - let text_y = (h + (-metrics.ascent)) / 2.0; + let text_y = (cap_h + (-metrics.ascent)) / 2.0; draw_text_with_fallback( canvas, @@ -140,3 +177,94 @@ impl Painter for Kbd { self.paint(canvas, layout.width, layout.height); } } + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(json: &str) -> Kbd { + serde_json::from_str(json).expect("kbd should deserialize") + } + + #[test] + fn style_defaults_to_flex_start_when_absent() { + // #127: same fix as `badge` — a keycap must keep its intrinsic + // width (`KbdIntrinsic`) instead of stretching to the flex + // container's full cross-axis size. + let kbd = parse(r#"{"type":"kbd","key":"K"}"#); + assert_eq!(kbd.style.align_self, Some(AlignSelf::FlexStart)); + } + + #[test] + fn style_defaults_to_flex_start_with_other_style_keys_present() { + let kbd = parse(r##"{"type":"kbd","key":"K","style":{"color":"#f00"}}"##); + assert_eq!(kbd.style.align_self, Some(AlignSelf::FlexStart)); + assert_eq!(kbd.style.color_str(), Some("#f00")); + } + + #[test] + fn explicit_align_self_is_respected() { + let kbd = parse(r#"{"type":"kbd","key":"K","style":{"align-self":"stretch"}}"#); + assert_eq!(kbd.style.align_self, Some(AlignSelf::Stretch)); + } + + #[test] + fn shadow_never_extends_past_the_assigned_box() { + // The shadow "lip" used to be offset 3px *below* a full-height + // face (`shadow_rect` at y=3 with height=h), so its bottom edge + // sat at `h + 3` — 3px past whatever box the layout gave this + // component. `paint()` now insets the face to `h - shadow_h` and + // draws the shadow directly beneath it, so nothing should paint + // past `h` any more. Render at a tiny height where a 3px miss + // would be highly visible relative to the box. + let kbd = Kbd { + key: "K".to_string(), + font_size: default_font_size(), + background_color: default_bg_color(), + border_color: default_border_color(), + text_color: default_text_color(), + timing: Default::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 100; + const H: i32 = 20; + let box_w = 60.0_f32; + let box_h = 10.0_f32; // shorter than the 3px shadow inset would tolerate if unfixed by a wide margin + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + canvas.translate((20.0, 5.0)); + kbd.paint(canvas, box_w, box_h); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + // Box bottom edge is at absolute y = 5 (translate) + 10 (box_h) = 15. + // Nothing painted should reach row 16 or beyond. + let box_bottom = 15; + for y in (box_bottom + 1)..H { + for x in 0..W { + let a = buf[((y * W + x) * 4 + 3) as usize]; + assert_eq!( + a, 0, + "kbd painted past its assigned box bottom at ({x},{y}), box_bottom={box_bottom}" + ); + } + } + } +} diff --git a/crates/rustmotion-components/src/slider.rs b/crates/rustmotion-components/src/slider.rs index 6af0b79..c24f296 100644 --- a/crates/rustmotion-components/src/slider.rs +++ b/crates/rustmotion-components/src/slider.rs @@ -2,7 +2,8 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use skia_safe::{Canvas, PaintStyle, RRect, Rect}; -use rustmotion_core::css::CssStyle; +use rustmotion_core::css::style::Size as CSize; +use rustmotion_core::css::{CssStyle, LengthPercentage as CLP}; use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::engine::renderer::{ @@ -37,7 +38,20 @@ fn default_thumb_color() -> String { "#FFFFFF".to_string() } +/// `#[serde(from = "SliderRaw")]`: like `switch`/`countdown`, `paint()` +/// draws from this component's own fields, never `layout.width`/ +/// `layout.height` — box_builder.rs's `css.width/height = Px(c.width / +/// c.height)` only budgets for the thin *track*, not the round thumb +/// (`thumb_size`, typically bigger than the track) that can sit centered +/// on either end, nor the `show_value` "N%" label that floats above it +/// (#127 measured 396×7 assigned vs. 417×31 painted). The raw shadow +/// struct computes the true bounding box once — same font metrics +/// `paint()` uses — and folds it into `style.width`/`style.height` before +/// `box_builder.rs` ever runs, without editing that file. `paint()` itself +/// is updated below to offset its drawing into that box instead of +/// treating local (0,0) as the thumb's top-left. #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(from = "SliderRaw")] pub struct Slider { #[serde(default = "default_slider_value")] pub value: f64, @@ -61,7 +75,7 @@ pub struct Slider { pub thumb_color: String, #[serde(default)] pub show_value: bool, - #[serde(flatten)] + #[serde(default)] pub timing: TimingConfig, #[serde(default)] pub style: CssStyle, @@ -71,6 +85,104 @@ pub struct Slider { pub stagger: Option, } +#[derive(Debug, Deserialize)] +struct SliderRaw { + #[serde(default = "default_slider_value")] + value: f64, + #[serde(default)] + animate_to: Option, + #[serde(default)] + animate_at: Option, + #[serde(default = "default_animation_duration")] + animation_duration: f64, + #[serde(default = "default_slider_width")] + width: f32, + #[serde(default = "default_slider_height")] + height: f32, + #[serde(default = "default_track_color")] + track_color: String, + #[serde(default = "default_fill_color")] + fill_color: String, + #[serde(default = "default_thumb_size")] + thumb_size: f32, + #[serde(default = "default_thumb_color")] + thumb_color: String, + #[serde(default)] + show_value: bool, + #[serde(flatten)] + timing: TimingConfig, + #[serde(default)] + style: CssStyle, + #[serde(default)] + timeline: Vec, + #[serde(default)] + stagger: Option, +} + +impl From for Slider { + fn from(raw: SliderRaw) -> Self { + let thumb_r = raw.thumb_size / 2.0; + let mut style = raw.style; + + if style.width.is_none() { + let h_margin = slider_value_label_half_width(raw.thumb_size) + .unwrap_or(0.0) + .max(thumb_r); + style.width = Some(CSize::Length(CLP::Px(raw.width + h_margin * 2.0))); + } + if style.height.is_none() { + let top_margin = if raw.show_value { + slider_value_label_line_height(raw.thumb_size).unwrap_or(0.0) + } else { + 0.0 + }; + style.height = Some(CSize::Length(CLP::Px(top_margin + raw.thumb_size))); + } + + Slider { + value: raw.value, + animate_to: raw.animate_to, + animate_at: raw.animate_at, + animation_duration: raw.animation_duration, + width: raw.width, + height: raw.height, + track_color: raw.track_color, + fill_color: raw.fill_color, + thumb_size: raw.thumb_size, + thumb_color: raw.thumb_color, + show_value: raw.show_value, + timing: raw.timing, + style, + timeline: raw.timeline, + stagger: raw.stagger, + } + } +} + +/// Half-width of the widest value label ("100%") `paint()` can draw, +/// using its exact `font_size = (thumb_size*0.7).max(12.0)` formula — the +/// thumb can sit centered at either end of the track, so this (or the +/// thumb radius, whichever is larger) is how much horizontal margin the +/// box needs past the track on both sides. +fn slider_value_label_half_width(thumb_size: f32) -> Option { + let font_size = (thumb_size * 0.7).max(12.0); + let typeface = typeface_with_fallback("Inter", skia_safe::FontStyle::normal()).ok()?; + let font = skia_safe::Font::from_typeface(typeface, font_size); + let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, font_size)); + let w = measure_text_with_fallback("100%", &font, &emoji_font, 0.0); + Some(w / 2.0) +} + +/// Vertical room `paint()`'s value label needs above the thumb (ascent + +/// descent + the 4px gap it's drawn with), using the same font size. +fn slider_value_label_line_height(thumb_size: f32) -> Option { + let font_size = (thumb_size * 0.7).max(12.0); + let typeface = typeface_with_fallback("Inter", skia_safe::FontStyle::normal()).ok()?; + let font = skia_safe::Font::from_typeface(typeface, font_size); + let (_, metrics) = font.metrics(); + Some(-metrics.ascent + metrics.descent + 4.0) +} + rustmotion_core::impl_traits!(Slider { Animatable => animation, Timed => timing, @@ -96,14 +208,42 @@ impl Slider { } impl Slider { - fn paint(&self, canvas: &Canvas, time: f64) { + fn paint(&self, canvas: &Canvas, layout: &BoxLayout, time: f64) { let w = self.width; let h = self.height; let thumb_r = self.thumb_size / 2.0; - let track_y = thumb_r - h / 2.0; let radius = h / 2.0; let current = self.current_value_at(time).clamp(0.0, 1.0) as f32; + // The track used to be drawn flush with local (0,0) — the thumb + // (radius `thumb_r`, usually bigger than the thin track) and the + // `show_value` label both extend past that origin on every side + // (#127 measured a 396×7 assigned box vs. 417×31 painted). Offset + // everything by the same margins reserved on the `Slider` struct + // (see its `From` impl) so the whole thing — track, + // thumb at either end, and the widest possible "100%" label — sits + // inside `[0, layout.width] x [0, layout.height]`. + let h_margin = slider_value_label_half_width(self.thumb_size) + .unwrap_or(0.0) + .max(thumb_r); + let top_margin = if self.show_value { + slider_value_label_line_height(self.thumb_size).unwrap_or(0.0) + } else { + 0.0 + }; + + canvas.save(); + if layout.width > 0.0 && layout.height > 0.0 { + canvas.clip_rect( + Rect::from_xywh(0.0, 0.0, layout.width, layout.height), + skia_safe::ClipOp::Intersect, + true, + ); + } + canvas.translate((h_margin, top_margin)); + + let track_y = thumb_r - h / 2.0; + let mut track_paint = paint_from_hex(&self.track_color); track_paint.set_style(PaintStyle::Fill); track_paint.set_anti_alias(true); @@ -139,6 +279,7 @@ impl Slider { let font_size = (self.thumb_size * 0.7).max(12.0); let font_style = skia_safe::FontStyle::normal(); let Ok(typeface) = typeface_with_fallback("Inter", font_style) else { + canvas.restore(); return; }; let font = skia_safe::Font::from_typeface(typeface, font_size); @@ -164,6 +305,8 @@ impl Slider { &text_paint, ); } + + canvas.restore(); } } @@ -171,10 +314,76 @@ impl Painter for Slider { fn paint_content( &self, canvas: &Canvas, - _layout: &BoxLayout, + layout: &BoxLayout, _props: &AnimatedProperties, ctx: &PaintCtx, ) { - self.paint(canvas, ctx.time); + self.paint(canvas, layout, ctx.time); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(json: &str) -> Slider { + serde_json::from_str(json).expect("slider should deserialize") + } + + fn px_width(s: &Slider) -> f32 { + let CSize::Length(CLP::Px(w)) = s.style.width.clone().expect("style.width should be set") + else { + panic!("expected an explicit px width"); + }; + w + } + + fn px_height(s: &Slider) -> f32 { + let CSize::Length(CLP::Px(h)) = s.style.height.clone().expect("style.height should be set") + else { + panic!("expected an explicit px height"); + }; + h + } + + #[test] + fn box_reserves_room_for_the_thumb_past_the_track() { + // #127: the thumb (`thumb_size`, usually bigger than the thin + // track) can be centered at either end of the track, so it always + // overhangs a box sized only to the track (396×7 assigned vs. + // 417×31 painted in the audit). + let s = parse(r#"{"type":"slider","width":396,"height":7,"thumb_size":20}"#); + let w = px_width(&s); + assert!( + w >= s.width + s.thumb_size, + "reserved width {w} should cover the track ({}) plus at least one full thumb ({})", + s.width, + s.thumb_size + ); + let h = px_height(&s); + assert!( + h >= s.thumb_size, + "reserved height {h} should cover the thumb ({})", + s.thumb_size + ); + } + + #[test] + fn show_value_reserves_extra_height_above_the_thumb() { + let plain = parse(r#"{"type":"slider","thumb_size":20}"#); + let with_value = parse(r#"{"type":"slider","thumb_size":20,"show_value":true}"#); + assert!( + px_height(&with_value) > px_height(&plain), + "show_value must reserve extra height for the floating label" + ); + } + + #[test] + fn explicit_style_size_is_never_overridden() { + let s = parse( + r#"{"type":"slider","thumb_size":20,"show_value":true,"style":{"width":900,"height":90}}"#, + ); + assert_eq!(px_width(&s), 900.0); + assert_eq!(px_height(&s), 90.0); } } diff --git a/crates/rustmotion-components/src/stat.rs b/crates/rustmotion-components/src/stat.rs index 71e3ea8..e1b3b0d 100644 --- a/crates/rustmotion-components/src/stat.rs +++ b/crates/rustmotion-components/src/stat.rs @@ -86,6 +86,25 @@ impl Stat { fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32) { let w = layout_w; let h = layout_h; + if w <= 0.0 || h <= 0.0 { + return; + } + + // Hard containment: whatever the math below computes, nothing gets + // to paint past the box the layout gave this component (issue + // #127). `stat` used to walk a `y_cursor` from a hardcoded + // `pad = 20.0` — label, then value, then a sparkline with a forced + // `.max(20.0)` minimum height — never once consulting `h`. A short + // box (the audit measured 512×48) still got the same ~120px stack + // every time, overflowing 70+px onto the next flex sibling. The + // clip below is the backstop; the scaling further down is what + // keeps it from ever needing to bite in practice. + canvas.save(); + canvas.clip_rect( + Rect::from_xywh(0.0, 0.0, w, h), + skia_safe::ClipOp::Intersect, + true, + ); // Background if set if let Some(bg) = self.style.background_color_str() { @@ -98,18 +117,43 @@ impl Stat { canvas.draw_rrect(rrect, &bg_paint); } - let pad = 20.0; + // `pad` scales down with the box instead of a fixed 20px, and the + // label/value font sizes are scaled by `content_scale` so the + // label+value stack actually fits in the room `h` gives them. Both + // stay at their authored size (pad=20, scale=1) whenever the box is + // tall enough — which is every existing example — and only shrink + // for a box shorter than the natural stack (like #127's 512×48). + let pad = (h * 0.15).clamp(2.0, 20.0).min(w * 0.15).max(2.0); + let content_h = (h - pad * 2.0).max(0.0); + + let label_natural_h = if self.label.is_some() { + self.label_font_size * 1.5 + } else { + 0.0 + }; + let value_natural_h = self.value_font_size * 1.2; + let text_natural_h = label_natural_h + value_natural_h; + let content_scale = if text_natural_h > 0.0 { + (content_h / text_natural_h).clamp(0.05, 1.0) + } else { + 1.0 + }; + + let eff_label_fs = self.label_font_size * content_scale; + let eff_value_fs = self.value_font_size * content_scale; + let mut y_cursor = pad; // Label (top) if let Some(label) = &self.label { let font_style = skia_safe::FontStyle::normal(); let Ok(typeface) = typeface_with_fallback("Inter", font_style) else { + canvas.restore(); return; }; - let font = skia_safe::Font::from_typeface(typeface, self.label_font_size); + let font = skia_safe::Font::from_typeface(typeface, eff_label_fs); let emoji_font = - emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, self.label_font_size)); + emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, eff_label_fs)); let (_, metrics) = font.metrics(); let mut label_paint = paint_from_hex(&self.label_color); @@ -126,18 +170,19 @@ impl Stat { ly, &label_paint, ); - y_cursor += self.label_font_size * 1.5; + y_cursor += eff_label_fs * 1.5; } // Value (large) { let font_style = skia_safe::FontStyle::bold(); let Ok(typeface) = typeface_with_fallback("Inter", font_style) else { + canvas.restore(); return; }; - let font = skia_safe::Font::from_typeface(typeface, self.value_font_size); + let font = skia_safe::Font::from_typeface(typeface, eff_value_fs); let emoji_font = - emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, self.value_font_size)); + emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, eff_value_fs)); let (_, metrics) = font.metrics(); let mut val_paint = paint_from_hex(&self.value_color); @@ -158,9 +203,10 @@ impl Stat { // Trend inline after value if let Some(trend) = &self.trend { let val_w = measure_text_with_fallback(&self.value, &font, &emoji_font, 0.0); - let trend_fs = self.value_font_size * 0.4; + let trend_fs = eff_value_fs * 0.4; let bold_style = skia_safe::FontStyle::bold(); let Ok(trend_typeface) = typeface_with_fallback("Inter", bold_style) else { + canvas.restore(); return; }; let trend_font = skia_safe::Font::from_typeface(trend_typeface, trend_fs); @@ -178,7 +224,7 @@ impl Stat { let (_, trend_metrics) = trend_font.metrics(); let mut tx = pad + val_w + 12.0; - let ty = vy - self.value_font_size * 0.15 + trend_metrics.ascent * 0.2; + let ty = vy - eff_value_fs * 0.15 + trend_metrics.ascent * 0.2; // Draw trend arrow icon let icon_id = match trend.direction { @@ -258,14 +304,22 @@ impl Stat { ); } - y_cursor += self.value_font_size * 1.2; + y_cursor += eff_value_fs * 1.2; } - // Sparkline (bottom) - if self.sparkline_data.len() >= 2 { - let spark_h = (h - y_cursor - pad).max(20.0); + // Sparkline (bottom) — only drawn if room is actually left. The + // previous `.max(20.0)` forced a sparkline into existence even when + // the label+value stack had already consumed the whole box, which + // is exactly what pushed ink onto the next flex sibling. `spark_y` + // itself eats 4px of gap below the value line, so that has to come + // out of the room budget too — the original formula measured room + // from `y_cursor`, not from `spark_y`, which let the sparkline's + // own bottom edge land 4px past `h - pad`. + let spark_y = y_cursor + 4.0; + let spark_room = (h - pad) - spark_y; + if self.sparkline_data.len() >= 2 && spark_room >= 8.0 { + let spark_h = spark_room; let spark_w = w - pad * 2.0; - let spark_y = y_cursor + 4.0; let max_v = self.sparkline_data.iter().fold(f64::MIN, |a, &b| a.max(b)); let min_v = self.sparkline_data.iter().fold(f64::MAX, |a, &b| a.min(b)); @@ -324,6 +378,8 @@ impl Stat { line_paint.set_stroke_join(skia_safe::paint::Join::Round); canvas.draw_path(&line_path, &line_paint); } + + canvas.restore(); } } @@ -338,3 +394,136 @@ impl Painter for Stat { self.paint(canvas, layout.width, layout.height); } } + +#[cfg(test)] +mod tests { + use super::*; + + fn full_stat() -> Stat { + Stat { + value: "98.2%".to_string(), + label: Some("Tests Passing".to_string()), + trend: Some(StatTrend { + value: "+2.1%".to_string(), + direction: TrendDirection::Up, + color: None, + }), + sparkline_data: vec![80.0, 84.0, 82.0, 88.0, 90.0, 94.0, 98.0], + sparkline_color: None, + value_font_size: default_value_font_size(), + label_font_size: default_label_font_size(), + value_color: default_value_color(), + label_color: default_label_color(), + timing: Default::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + } + } + + /// Bounding box (min_x, max_x, min_y, max_y) of every non-transparent + /// pixel on the surface, or `None` if nothing was painted. Mirrors the + /// helper `caption.rs` already uses for the same kind of proof. + fn ink_bounds( + surface: &mut skia_safe::Surface, + w: i32, + h: i32, + ) -> Option<(i32, i32, i32, i32)> { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (w, h), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (w * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + let (mut minx, mut maxx, mut miny, mut maxy) = (i32::MAX, i32::MIN, i32::MAX, i32::MIN); + for y in 0..h { + for x in 0..w { + if buf[((y * w + x) * 4 + 3) as usize] > 0 { + minx = minx.min(x); + maxx = maxx.max(x); + miny = miny.min(y); + maxy = maxy.max(y); + } + } + } + (minx <= maxx).then_some((minx, maxx, miny, maxy)) + } + + #[test] + fn ink_stays_within_a_tiny_assigned_box() { + // #127's exact repro: a 512×48 box (the measured "assigned box" for + // this component in a 560×300 card) used to take ~120px of ink — + // label + value + a forced-minimum sparkline — 72px onto whatever + // sits below it in the flex column. With a solid background (the + // same way the audit made the assigned box visible), the painted + // background rect *is* the box, so no ink should land outside it. + let stat = full_stat(); + const W: i32 = 512; + const H: i32 = 48; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + stat.paint(canvas, W as f32, H as f32); + } + let (minx, maxx, miny, maxy) = + ink_bounds(&mut surface, W, H).expect("stat must paint something"); + assert!( + minx >= 0 && miny >= 0, + "ink starts outside the box: ({minx},{miny})" + ); + assert!( + maxx < W && maxy < H, + "ink escaped the {W}x{H} box: max=({maxx},{maxy})" + ); + } + + #[test] + fn ink_stays_within_box_even_without_a_background() { + // Same box, no `style.background` — the clip still has to hold + // even when there's no filled rect to visually anchor it to. + let mut stat = full_stat(); + stat.style = CssStyle::default(); + const W: i32 = 512; + const H: i32 = 48; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + stat.paint(canvas, W as f32, H as f32); + } + if let Some((minx, maxx, miny, maxy)) = ink_bounds(&mut surface, W, H) { + assert!(minx >= 0 && miny >= 0 && maxx < W && maxy < H); + } + } + + #[test] + fn generous_box_keeps_the_original_full_size_layout() { + // A box as tall as the existing examples use (e.g. mega-showcase's + // 380×220 stat cards) must not be affected by the new scaling — + // `content_scale` should resolve to 1.0 and the value should still + // render at its authored `value_font_size`. + let stat = full_stat(); + const W: i32 = 380; + const H: i32 = 220; + let pad = (H as f32 * 0.15) + .clamp(2.0, 20.0) + .min(W as f32 * 0.15) + .max(2.0); + let content_h = (H as f32 - pad * 2.0).max(0.0); + let text_natural_h = stat.label_font_size * 1.5 + stat.value_font_size * 1.2; + let scale = (content_h / text_natural_h).clamp(0.05, 1.0); + assert_eq!( + scale, 1.0, + "a generously sized box should never shrink the text" + ); + } +} diff --git a/crates/rustmotion-components/src/switch.rs b/crates/rustmotion-components/src/switch.rs index c1769c4..0d6f2a9 100644 --- a/crates/rustmotion-components/src/switch.rs +++ b/crates/rustmotion-components/src/switch.rs @@ -2,11 +2,13 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use skia_safe::{Canvas, PaintStyle, RRect, Rect}; -use rustmotion_core::css::CssStyle; +use rustmotion_core::css::style::Size as CSize; +use rustmotion_core::css::{CssStyle, LengthPercentage as CLP}; use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::engine::renderer::{ - draw_text_with_fallback, emoji_typeface, paint_from_hex, typeface_with_fallback, + draw_text_with_fallback, emoji_typeface, measure_text_with_fallback, paint_from_hex, + typeface_with_fallback, }; use rustmotion_core::schema::TimelineStep; use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig}; @@ -30,7 +32,18 @@ fn default_transition_duration() -> f64 { 0.3 } +/// `#[serde(from = "SwitchRaw")]`: `paint()` draws the label at +/// `self.width + 8`, entirely outside the track — and, like `slider` and +/// `countdown`, this painter draws from its own fields (`self.width`/ +/// `self.height`), never `layout.width`/`layout.height`, so the box +/// `box_builder.rs` assigns (`css.width = Px(c.width)`, no label +/// awareness — see #127) needs to be widened to fit the label, not the +/// paint code changed to fit a box it doesn't consult. Going through a raw +/// shadow struct lets that extra width be computed once (with the same +/// font metrics `paint()` uses) and folded into `style.width` before +/// `box_builder.rs` ever sees this component, without touching that file. #[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[serde(from = "SwitchRaw")] pub struct Switch { #[serde(default)] pub value: bool, @@ -50,7 +63,7 @@ pub struct Switch { pub thumb_color: String, #[serde(default = "default_transition_duration")] pub transition_duration: f64, - #[serde(flatten)] + #[serde(default)] pub timing: TimingConfig, #[serde(default)] pub style: CssStyle, @@ -60,6 +73,81 @@ pub struct Switch { pub stagger: Option, } +#[derive(Debug, Deserialize)] +struct SwitchRaw { + #[serde(default)] + value: bool, + #[serde(default)] + toggle_at: Option, + #[serde(default)] + label: Option, + #[serde(default = "default_switch_width")] + width: f32, + #[serde(default = "default_switch_height")] + height: f32, + #[serde(default = "default_track_color_on")] + track_color_on: String, + #[serde(default = "default_track_color_off")] + track_color_off: String, + #[serde(default = "default_thumb_color")] + thumb_color: String, + #[serde(default = "default_transition_duration")] + transition_duration: f64, + #[serde(flatten)] + timing: TimingConfig, + #[serde(default)] + style: CssStyle, + #[serde(default)] + timeline: Vec, + #[serde(default)] + stagger: Option, +} + +impl From for Switch { + fn from(raw: SwitchRaw) -> Self { + let mut style = raw.style; + if style.width.is_none() { + if let Some(extra) = raw + .label + .as_deref() + .and_then(|label| switch_label_extra_width(label, raw.height)) + { + style.width = Some(CSize::Length(CLP::Px(raw.width + extra))); + } + } + Switch { + value: raw.value, + toggle_at: raw.toggle_at, + label: raw.label, + width: raw.width, + height: raw.height, + track_color_on: raw.track_color_on, + track_color_off: raw.track_color_off, + thumb_color: raw.thumb_color, + transition_duration: raw.transition_duration, + timing: raw.timing, + style, + timeline: raw.timeline, + stagger: raw.stagger, + } + } +} + +/// Extra width (gap + label text) `paint()` needs past the track — mirrors +/// its `font_size = (h*0.5).max(12.0)` / `text_x = w + 8.0` exactly, so the +/// box reserved here always matches what gets drawn. `None` on font-load +/// failure (e.g. a headless test env with no fonts) — `style.width` is then +/// left unset and `box_builder.rs`'s plain `c.width` fallback applies, same +/// as before this fix. +fn switch_label_extra_width(label: &str, height: f32) -> Option { + let font_size = (height * 0.5).max(12.0); + let typeface = typeface_with_fallback("Inter", skia_safe::FontStyle::normal()).ok()?; + let font = skia_safe::Font::from_typeface(typeface, font_size); + let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, font_size)); + let label_w = measure_text_with_fallback(label, &font, &emoji_font, 0.0); + Some(8.0 + label_w) +} + rustmotion_core::impl_traits!(Switch { Animatable => animation, Timed => timing, @@ -170,3 +258,53 @@ impl Painter for Switch { self.paint(canvas, ctx.time); } } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::style::Size as SzCheck; + + fn parse(json: &str) -> Switch { + serde_json::from_str(json).expect("switch should deserialize") + } + + #[test] + fn no_label_leaves_style_width_unset() { + // No extra room needed — box_builder.rs's plain `c.width` fallback + // should still apply, exactly as before this fix. + let s = parse(r#"{"type":"switch"}"#); + assert!(s.style.width.is_none()); + } + + #[test] + fn label_widens_style_width_past_the_track() { + // #127: `paint()` draws the label at `self.width + 8`, entirely + // outside a box sized only for the track (64×34 assigned vs. + // 155×34 painted in the audit). `style.width` must now reserve at + // least `width` (the track) plus *some* label room. + let s = parse(r#"{"type":"switch","label":"Dark mode","width":64,"height":34}"#); + let SzCheck::Length(rustmotion_core::css::LengthPercentage::Px(w)) = + s.style.width.expect("label should reserve style.width") + else { + panic!("expected an explicit px width"); + }; + assert!( + w > s.width, + "reserved width {w} should exceed the bare track width {}", + s.width + ); + } + + #[test] + fn explicit_style_width_is_never_overridden() { + let s = parse( + r#"{"type":"switch","label":"Dark mode","width":64,"height":34,"style":{"width":500}}"#, + ); + let SzCheck::Length(rustmotion_core::css::LengthPercentage::Px(w)) = + s.style.width.expect("width should still be set") + else { + panic!("expected an explicit px width"); + }; + assert_eq!(w, 500.0, "author's explicit style.width must win"); + } +} diff --git a/crates/rustmotion-components/src/timeline.rs b/crates/rustmotion-components/src/timeline.rs index fed10ff..d51a066 100644 --- a/crates/rustmotion-components/src/timeline.rs +++ b/crates/rustmotion-components/src/timeline.rs @@ -187,7 +187,27 @@ impl Timeline { ) { let total_w = self.width; let bar_y = r; // Center of nodes - let spacing = if n > 1 { total_w / (n - 1) as f32 } else { 0.0 }; + + // Node 0 used to sit at local x=0 and node n-1 at x=total_w, so + // their circles (radius `r`) bled `r` px past both edges of the + // assigned box — and further still wherever a step's label was + // wider than the node itself (issue #127 measured ink starting + // 33px left of the box, 9px outside the card). Insetting every + // node by `max(r, widest_label / 2)` keeps both the circles and + // their centered labels inside `[0, total_w]` regardless of which + // step has the longest label. + let max_label_half_w = self + .steps + .iter() + .map(|s| measure_text_with_fallback(&s.label, font, &None, 0.0) * 0.5) + .fold(0.0f32, f32::max); + let inset = r.max(max_label_half_w).min(total_w / 2.0); + let usable_w = (total_w - inset * 2.0).max(0.0); + let spacing = if n > 1 { + usable_w / (n - 1) as f32 + } else { + 0.0 + }; // Draw background bar let bar_rect = @@ -226,7 +246,7 @@ impl Timeline { // Draw nodes and labels for (i, step) in self.steps.iter().enumerate() { let cx = if n > 1 { - i as f32 * spacing + inset + i as f32 * spacing } else { total_w / 2.0 }; @@ -317,10 +337,21 @@ impl Timeline { let spacing = 80.0; let bar_x = r; let total_h = if n > 1 { (n - 1) as f32 * spacing } else { 0.0 }; + // Node 0 used to sit at local y=0, so its circle (radius `r`) bled + // `r` px above the assigned box's top edge — same class of bug as + // the horizontal direction's left/right overflow. Insetting every + // node's `cy` by `r` keeps the whole column inside `[0, box height]` + // (box_builder.rs already reserves `r*2 + 64` px per step, well + // past what this needs). + let top_inset = r; // Background bar - let bar_rect = - Rect::from_xywh(bar_x - self.bar_height / 2.0, 0.0, self.bar_height, total_h); + let bar_rect = Rect::from_xywh( + bar_x - self.bar_height / 2.0, + top_inset, + self.bar_height, + total_h, + ); let mut bar_paint = paint_from_hex(&self.bar_color); bar_paint.set_style(PaintStyle::Fill); canvas.draw_round_rect( @@ -333,8 +364,12 @@ impl Timeline { // Filled bar if fill_progress > 0.001 { let fill_h = total_h * fill_progress.clamp(0.0, 1.0); - let fill_rect = - Rect::from_xywh(bar_x - self.bar_height / 2.0, 0.0, self.bar_height, fill_h); + let fill_rect = Rect::from_xywh( + bar_x - self.bar_height / 2.0, + top_inset, + self.bar_height, + fill_h, + ); let mut fill_paint = paint_from_hex(&self.bar_fill_color); fill_paint.set_style(PaintStyle::Fill); canvas.draw_round_rect( @@ -347,7 +382,7 @@ impl Timeline { for (i, step) in self.steps.iter().enumerate() { let cx = bar_x; - let cy = if n > 1 { i as f32 * spacing } else { 0.0 }; + let cy = top_inset + if n > 1 { i as f32 * spacing } else { 0.0 }; let step_progress = if n > 1 { i as f32 / (n - 1) as f32 @@ -409,3 +444,130 @@ impl Painter for Timeline { self.paint(canvas, props); } } + +#[cfg(test)] +mod tests { + use super::*; + + fn step(label: &str) -> TimelineStep { + TimelineStep { + label: label.to_string(), + sublabel: None, + color: default_node_color(), + icon: None, + } + } + + fn ink_bounds( + surface: &mut skia_safe::Surface, + w: i32, + h: i32, + ) -> Option<(i32, i32, i32, i32)> { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (w, h), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (w * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + let (mut minx, mut maxx, mut miny, mut maxy) = (i32::MAX, i32::MIN, i32::MAX, i32::MIN); + for y in 0..h { + for x in 0..w { + if buf[((y * w + x) * 4 + 3) as usize] > 0 { + minx = minx.min(x); + maxx = maxx.max(x); + miny = miny.min(y); + maxy = maxy.max(y); + } + } + } + (minx <= maxx).then_some((minx, maxx, miny, maxy)) + } + + #[test] + fn horizontal_nodes_and_labels_stay_within_the_declared_width() { + // #127: node 0's circle used to be centered at local x=0 (and node + // n-1 at x=width), so the circle — and a wide first/last label — + // bled `node_radius` (or more) past both edges of the box the + // layout gave this component. A long first label and a long last + // label both push on the insets from either side. + let tl = Timeline { + steps: vec![ + step("Design phase kickoff"), + step("Build"), + step("Test"), + step("Ship it now"), + ], + width: 512.0, + direction: TimelineDirection::Horizontal, + node_radius: default_node_radius(), + bar_color: default_bar_color(), + bar_fill_color: default_bar_fill_color(), + bar_height: default_bar_height(), + fill_progress: default_fill_progress(), + font_size: default_label_font_size(), + label_color: default_label_color(), + sublabel_color: default_sublabel_color(), + timing: Default::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 512; + const H: i32 = 100; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + let props = AnimatedProperties::default(); + tl.paint(canvas, &props); + } + let (minx, maxx, _miny, _maxy) = + ink_bounds(&mut surface, W, H).expect("timeline must paint something"); + assert!(minx >= 0, "ink starts left of the box at x={minx}"); + assert!( + maxx < W, + "ink escapes the box on the right at x={maxx} (width={W})" + ); + } + + #[test] + fn single_step_stays_centered_within_the_box() { + let tl = Timeline { + steps: vec![step("Only step")], + width: 300.0, + direction: TimelineDirection::Horizontal, + node_radius: default_node_radius(), + bar_color: default_bar_color(), + bar_fill_color: default_bar_fill_color(), + bar_height: default_bar_height(), + fill_progress: default_fill_progress(), + font_size: default_label_font_size(), + label_color: default_label_color(), + sublabel_color: default_sublabel_color(), + timing: Default::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 300; + const H: i32 = 100; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + let props = AnimatedProperties::default(); + tl.paint(canvas, &props); + } + let (minx, maxx, _miny, _maxy) = + ink_bounds(&mut surface, W, H).expect("timeline must paint something"); + assert!(minx >= 0 && maxx < W); + } +}