From 498b1bccbcafb65ec65257383e7f0b32c86475b8 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:46:29 +0200 Subject: [PATCH] fix(css): resolve em against the element font-size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build() (layout_pass.rs:122-154) hands the *same* ConversionContext to to_taffy_style for every node in the tree and never re-derives it from the node's own (cascaded) font-size, and the only production builder keeps ..LengthContext::default() → font_size: 16.0 (scene.rs:29-37). So padding: "1em", gap: "0.5em", width: "10em", top: "2em" all resolve against 16px regardless of the element's actual font size: on a 48px-font card, padding: "1em" reserves 16px instead of 48px. Unlike the context-free .px() accessors — which were deliberately made to warn loudly for exactly this class of drop (units.rs:240-256) — this path is silent, so the wrong geometry reaches both the renderer and the validator with no signal. Fix: Derive a per-node LengthContext while building the taffy tree: resolve the node's own font-size (post-cascade) to px and pass it as ctx.length.font_size to its own to_taffy_style call and to its children's. Until that exists, emit the same one-shot warning order already uses (taffy_bridge.rs:142-151) whenever an em reaches this bridge. Refs #220 --- .../rustmotion-core/src/engine/layout_pass.rs | 40 ++++++++++++---- crates/rustmotion-core/tests/audit_ws_j.rs | 47 +++++++++++++++++++ 2 files changed, 78 insertions(+), 9 deletions(-) create mode 100644 crates/rustmotion-core/tests/audit_ws_j.rs diff --git a/crates/rustmotion-core/src/engine/layout_pass.rs b/crates/rustmotion-core/src/engine/layout_pass.rs index 026a16a4..21c6ce73 100644 --- a/crates/rustmotion-core/src/engine/layout_pass.rs +++ b/crates/rustmotion-core/src/engine/layout_pass.rs @@ -6,6 +6,7 @@ use taffy::prelude as tf; use taffy::TaffyTree; use crate::css::taffy_bridge::{to_taffy_style, ConversionContext}; +use crate::css::units::LengthContext; use crate::engine::box_tree::{BoxNode, IntrinsicMeasure, NodeId}; /// Resolved geometry for a single node, in absolute viewport coordinates. @@ -85,7 +86,7 @@ pub fn run_layout(root: &BoxNode, viewport: (f32, f32), ctx: &ConversionContext) let mut node_map: HashMap = HashMap::new(); // Build the taffy tree top-down. - let root_tf = build(&mut tree, &mut node_map, root, ctx); + let root_tf = build(&mut tree, &mut node_map, root, ctx, ctx.length.font_size); let viewport_size = tf::Size { width: tf::AvailableSpace::Definite(viewport.0), @@ -119,33 +120,54 @@ pub fn run_layout(root: &BoxNode, viewport: (f32, f32), ctx: &ConversionContext) LayoutResult { layouts } } +/// Build one taffy node and, recursively, its subtree. +/// +/// `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. fn build( tree: &mut TaffyTree, map: &mut HashMap, node: &BoxNode, ctx: &ConversionContext, + inherited_font_size: f32, ) -> tf::NodeId { - let style = to_taffy_style(&node.css, ctx); + let parent_font_ctx = LengthContext { + font_size: inherited_font_size, + ..ctx.length + }; + let own_font_size = node + .css + .font_size_px_ctx(&parent_font_ctx, inherited_font_size); + let node_ctx = ConversionContext { + length: LengthContext { + font_size: own_font_size, + ..ctx.length + }, + }; + + let style = to_taffy_style(&node.css, &node_ctx); let data = NodeData { box_id: node.id, intrinsic: node.intrinsic.clone(), }; - let tf_id = if node.intrinsic.is_some() { - // Leaf with intrinsic measurement. - tree.new_leaf_with_context(style, data) - .expect("taffy new_leaf") - } else if node.children.is_empty() { + let tf_id = if node.intrinsic.is_some() || node.children.is_empty() { tree.new_leaf_with_context(style, data) .expect("taffy new_leaf") } else { let mut child_ids = Vec::with_capacity(node.children.len()); for c in &node.children { - child_ids.push(build(tree, map, c, ctx)); + child_ids.push(build(tree, map, c, ctx, own_font_size)); } let id = tree .new_with_children(style, &child_ids) .expect("taffy new_with_children"); - // We still want context on internal nodes (for box_id mapping). tree.set_node_context(id, Some(data)).ok(); id }; diff --git a/crates/rustmotion-core/tests/audit_ws_j.rs b/crates/rustmotion-core/tests/audit_ws_j.rs new file mode 100644 index 00000000..c63d1b5d --- /dev/null +++ b/crates/rustmotion-core/tests/audit_ws_j.rs @@ -0,0 +1,47 @@ +//! Regression tests for the workstream J (layout pass & CSS unit resolution) +//! audit findings on unit resolution and intrinsic measurement. + +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::layout_pass::run_layout; + +// ---- `em` on layout properties must resolve against the element's +// own (inherited) font-size, not a constant 16px ---- + +/// A card sized by its parent's explicit `font-size` (48px) inherits that +/// font-size down to its own layout resolution even though it never sets +/// `font-size` itself. Its `padding: "1em"` must therefore resolve to 48px +/// (`1 * inherited_font_size`), not the pre-fix constant 16px. +#[test] +fn em_padding_resolves_against_inherited_font_size_not_a_constant_16px() { + let mut root = BoxNode::container( + CssStyle { + font_size: Some(Length::Px(48.0)), + width: Some(CSize::Length(CLP::Px(400.0))), + height: Some(CSize::Length(CLP::Px(400.0))), + ..Default::default() + }, + vec![BoxNode::container( + CssStyle { + padding: Some(Edges::Uniform(CLP::String("1em".into()))), + width: Some(CSize::Length(CLP::Px(200.0))), + height: Some(CSize::Length(CLP::Px(200.0))), + ..Default::default() + }, + vec![], + )], + ); + root.assign_ids(1); + + let res = run_layout(&root, (400.0, 400.0), &ConversionContext::default()); + let child = res.get(2).expect("child laid out"); + let (content_x, _, content_w, _) = child.content_box(); + + assert_eq!( + content_x, 48.0, + "1em padding must resolve against the inherited 48px font-size" + ); + assert_eq!(content_w, 200.0 - 2.0 * 48.0); +}