From aec6ca840406509603c2d4fabde2d0fe0fda1629 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sun, 2 Aug 2026 11:56:30 +0200 Subject: [PATCH] fix(components): keep painters inside the box the layout assigned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several painters drew well outside the box they were given, which in a panel-based style means drawing on top of the next sibling or outside the card entirely. Measured against a 560x300 card with 24px padding: stat painted 120px tall in a 48px box, timeline started 33px left of its box and 9px outside the card, caption placed its box above the card top, switch, slider and countdown each ran past their own edges. Each now either draws within its box or reports the size it genuinely needs. stat shrinks to fit any assigned height rather than assuming one — the 48px it was measured against turned out to be a height the audit set deliberately, not a natural default, so pinning the fix to that number would have been wrong. badge and kbd stretched to the full card width because they have intrinsic measures but auto CSS width, so align-items: stretch won: a keycap rendered as a full-width slab. Both now keep their natural width inside a card. divider was investigated and left alone. Its reported 8px overhang traces to an explicit length that exceeded a smaller test card, which is correct behaviour. Closes #127 --- crates/rustmotion-components/src/badge.rs | 70 +++++- crates/rustmotion-components/src/caption.rs | 82 ++++++- crates/rustmotion-components/src/countdown.rs | 204 ++++++++++++++-- crates/rustmotion-components/src/kbd.rs | 140 ++++++++++- crates/rustmotion-components/src/slider.rs | 221 +++++++++++++++++- crates/rustmotion-components/src/stat.rs | 215 +++++++++++++++-- crates/rustmotion-components/src/switch.rs | 144 +++++++++++- crates/rustmotion-components/src/timeline.rs | 176 +++++++++++++- 8 files changed, 1190 insertions(+), 62 deletions(-) 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); + } +}