From 0c542354d200175a71fb79212a31698e4272cb17 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:57:38 +0200 Subject: [PATCH] fix(layout): hand IntrinsicMeasure both sizes in the content box MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit taffy 0.10.1 (`compute/leaf.rs`) subtracts `content_box_inset` (padding+border) from `available_space` before calling the measure fn, but passes `known_dimensions` — the outer border-box size — untouched. The two arguments are therefore in different coordinate spaces, and neither `IntrinsicMeasure`'s trait doc (box_tree.rs:114-122) nor this closure says so. `TextIntrinsic::measure` (rustmotion-components/src/intrinsic.rs:224) prefers `known.0` as its wrap width, while `Text::paint` wraps at `layout.content_box()` width (legacy_dispatch.rs:145-155). A `text` with horizontal padding is therefore measured at a wider line width than it is painted at: fewer wrapped lines, a reserved box one line too short, and the text spills out of its own box. The geometry validator re-measures through the same intrinsic, so it signs the overflow off. intrinsic.rs:258-265 explicitly asserts the opposite ("taffy hands a leaf its own known/available height already padding/border-subtracted") — true for `available`, false for `known`. Refs #220 --- .../rustmotion-core/src/css/taffy_bridge.rs | 61 ++++++++++++++ .../rustmotion-core/src/engine/layout_pass.rs | 34 ++++++-- crates/rustmotion-core/tests/audit_ws_j.rs | 81 ++++++++++++++++++- 3 files changed, 167 insertions(+), 9 deletions(-) diff --git a/crates/rustmotion-core/src/css/taffy_bridge.rs b/crates/rustmotion-core/src/css/taffy_bridge.rs index 380cb86d..e30401ce 100644 --- a/crates/rustmotion-core/src/css/taffy_bridge.rs +++ b/crates/rustmotion-core/src/css/taffy_bridge.rs @@ -238,6 +238,67 @@ pub fn to_taffy_style(css: &CssStyle, ctx: &ConversionContext) -> tf::Style { style } +/// Resolve `padding` + `border` width into a single content-box inset, in +/// px, per axis: `(horizontal, vertical)` i.e. `(left + right, top + +/// bottom)`. +/// +/// Mirrors what taffy 0.10.1's own `compute_leaf_layout` (`content_box_inset +/// = padding + border`) subtracts from a leaf's `available_space` before +/// handing it to the measure function — see [`crate::engine::layout_pass`], +/// which uses this to bring `known_dimensions` (still border-box) into that +/// same content-box space. Percentage padding/border resolves +/// against `ctx.length.parent_size` like every other percentage in this +/// module; a leaf's own known/available width is not threaded here, so a +/// percentage inset on a leaf is only as accurate as that shared context. +pub(crate) fn content_box_inset(css: &CssStyle, ctx: &ConversionContext) -> (f32, f32) { + let (pt, pr, pb, pl) = css.padding.as_ref().map(Edges::resolve).unwrap_or_default(); + let padding = ( + pt.resolve(&ctx.length), + pr.resolve(&ctx.length), + pb.resolve(&ctx.length), + pl.resolve(&ctx.length), + ); + let border = resolve_border_widths_px(css.border.as_ref(), ctx); + ( + padding.1 + padding.3 + border.1 + border.3, + padding.0 + padding.2 + border.0 + border.2, + ) +} + +fn resolve_border_widths_px( + b: Option<&super::style::BorderEdges>, + ctx: &ConversionContext, +) -> (f32, f32, f32, f32) { + let Some(b) = b else { + return (0.0, 0.0, 0.0, 0.0); + }; + let uniform = b.width.as_ref().map(Edges::resolve); + let pick_side = |side: Option<&super::style::BorderSide>, idx: usize| -> f32 { + if let Some(side) = side { + if let Some(w) = side.width.as_ref() { + return w.resolve(&ctx.length); + } + } + if let Some((t, r, btm, l)) = uniform.as_ref() { + let pick = match idx { + 0 => t, + 1 => r, + 2 => btm, + 3 => l, + _ => t, + }; + return pick.resolve(&ctx.length); + } + 0.0 + }; + ( + pick_side(b.top.as_ref(), 0), + pick_side(b.right.as_ref(), 1), + pick_side(b.bottom.as_ref(), 2), + pick_side(b.left.as_ref(), 3), + ) +} + fn align_items_to_taffy(a: AlignItems) -> tf::AlignItems { match a { AlignItems::Stretch => tf::AlignItems::Stretch, diff --git a/crates/rustmotion-core/src/engine/layout_pass.rs b/crates/rustmotion-core/src/engine/layout_pass.rs index 21c6ce73..386c201d 100644 --- a/crates/rustmotion-core/src/engine/layout_pass.rs +++ b/crates/rustmotion-core/src/engine/layout_pass.rs @@ -5,7 +5,7 @@ use std::collections::HashMap; use taffy::prelude as tf; use taffy::TaffyTree; -use crate::css::taffy_bridge::{to_taffy_style, ConversionContext}; +use crate::css::taffy_bridge::{content_box_inset, to_taffy_style, ConversionContext}; use crate::css::units::LengthContext; use crate::engine::box_tree::{BoxNode, IntrinsicMeasure, NodeId}; @@ -70,10 +70,20 @@ impl LayoutResult { /// Per-node user data stored in the taffy tree to keep the link between /// taffy nodes and our `BoxNode` ids + intrinsic measurers. +/// +/// `inset_width`/`inset_height` are this node's own resolved padding+border +///: taffy's `compute_leaf_layout` already subtracts them from +/// `available_space` before calling the measure function, but forwards +/// `known_dimensions` — the outer border-box size — untouched, handing an +/// `IntrinsicMeasure` implementor two arguments in different coordinate +/// spaces. The measure closure below subtracts the same inset from `known` +/// so both arguments describe the content box. struct NodeData { #[allow(dead_code)] box_id: NodeId, intrinsic: Option>, + inset_width: f32, + inset_height: f32, } /// Run taffy on a [`BoxNode`] tree and return the resolved layouts. @@ -102,8 +112,12 @@ pub fn run_layout(root: &BoxNode, viewport: (f32, f32), ctx: &ConversionContext) let Some(intr) = ctx.intrinsic.as_ref() else { return tf::Size::ZERO; }; + let content_known = ( + known.width.map(|w| (w - ctx.inset_width).max(0.0)), + known.height.map(|h| (h - ctx.inset_height).max(0.0)), + ); let (w, h) = intr.measure( - (known.width, known.height), + content_known, (available.width.into(), available.height.into()), ); tf::Size { @@ -125,12 +139,13 @@ pub fn run_layout(root: &BoxNode, viewport: (f32, f32), ctx: &ConversionContext) /// `inherited_font_size` is the already-resolved (px) font-size of `node`'s /// parent: CSS resolves `em` on every layout property against the /// element's *own* computed font-size, and font-size itself inherits down -/// the tree unless overridden. `to_taffy_style` only ever sees the single -/// `ConversionContext` handed to it, so its `em` resolution is only as -/// correct as the per-node context built here — a call site building one -/// shared `ConversionContext` for the whole tree (as every production -/// caller of `to_taffy_style` still does directly) resolves every node's -/// `em` against that one context's `font_size` instead. +/// the tree unless overridden. `to_taffy_style` and [`content_box_inset`] +/// only ever see the single `ConversionContext` handed to them, so their +/// `em` resolution is only as correct as the per-node context built here — +/// a call site building one shared `ConversionContext` for the whole tree +/// (as every production caller of `to_taffy_style` still does directly) +/// resolves every node's `em` against that one context's `font_size` +/// instead. fn build( tree: &mut TaffyTree, map: &mut HashMap, @@ -153,9 +168,12 @@ fn build( }; let style = to_taffy_style(&node.css, &node_ctx); + let (inset_width, inset_height) = content_box_inset(&node.css, &node_ctx); let data = NodeData { box_id: node.id, intrinsic: node.intrinsic.clone(), + inset_width, + inset_height, }; let tf_id = if node.intrinsic.is_some() || node.children.is_empty() { tree.new_leaf_with_context(style, data) diff --git a/crates/rustmotion-core/tests/audit_ws_j.rs b/crates/rustmotion-core/tests/audit_ws_j.rs index c63d1b5d..6b3501b9 100644 --- a/crates/rustmotion-core/tests/audit_ws_j.rs +++ b/crates/rustmotion-core/tests/audit_ws_j.rs @@ -1,10 +1,12 @@ //! Regression tests for the workstream J (layout pass & CSS unit resolution) //! audit findings on unit resolution and intrinsic measurement. +use std::sync::{Arc, Mutex}; + use rustmotion_core::css::style::{CssStyle, Edges, Size as CSize}; use rustmotion_core::css::taffy_bridge::ConversionContext; use rustmotion_core::css::units::{Length, LengthPercentage as CLP}; -use rustmotion_core::engine::box_tree::BoxNode; +use rustmotion_core::engine::box_tree::{AvailableSpace, BoxNode, IntrinsicMeasure}; use rustmotion_core::engine::layout_pass::run_layout; // ---- `em` on layout properties must resolve against the element's @@ -45,3 +47,80 @@ fn em_padding_resolves_against_inherited_font_size_not_a_constant_16px() { ); assert_eq!(content_w, 200.0 - 2.0 * 48.0); } + +// ---- a leaf's `IntrinsicMeasure::measure` must receive `known` and +// `available` in the same (content-box) coordinate space ---- + +type RecordedCall = ((Option, Option), (AvailableSpace, AvailableSpace)); + +#[derive(Default)] +struct RecordingIntrinsic { + calls: Mutex>, +} + +impl IntrinsicMeasure for RecordingIntrinsic { + fn measure( + &self, + known: (Option, Option), + available: (AvailableSpace, AvailableSpace), + ) -> (f32, f32) { + self.calls.lock().unwrap().push((known, available)); + (50.0, 30.0) + } +} + +/// A leaf with 20px uniform padding, stretched to its column-flex parent's +/// full 300px content width but auto-height (so taffy must measure its +/// intrinsic height with the width already resolved). Any call where +/// `known.0` is definite must agree with `available.0` when that is also +/// definite: both describe the same box, and per taffy 0.10.1's own +/// `compute_leaf_layout`, `available_space` has already had padding+border +/// subtracted before reaching the measure function. +#[test] +fn measure_fn_known_and_available_agree_on_content_box_width() { + use rustmotion_core::css::style::{AlignItems, Display, FlexDirection}; + + let recorder = Arc::new(RecordingIntrinsic::default()); + let leaf = BoxNode::leaf( + CssStyle { + padding: Some(Edges::Uniform(CLP::Px(20.0))), + ..Default::default() + }, + recorder.clone(), + ); + let mut root = BoxNode::container( + CssStyle { + display: Some(Display::Flex), + flex_direction: Some(FlexDirection::Column), + align_items: Some(AlignItems::Stretch), + width: Some(CSize::Length(CLP::Px(300.0))), + height: Some(CSize::Length(CLP::Px(300.0))), + ..Default::default() + }, + vec![leaf], + ); + root.assign_ids(1); + + run_layout(&root, (300.0, 300.0), &ConversionContext::default()); + + let calls = recorder.calls.lock().unwrap(); + let definite_known_calls: Vec<_> = calls + .iter() + .filter(|(known, _)| known.0.is_some()) + .collect(); + assert!( + !definite_known_calls.is_empty(), + "expected at least one measure call with a definite known width, got {calls:?}" + ); + + for (known, available) in definite_known_calls { + if let AvailableSpace::Definite(available_w) = available.0 { + assert_eq!( + known.0.unwrap(), + available_w, + "known.width and available.width must describe the same \ + (content-box) box; known={known:?} available={available:?}" + ); + } + } +}