Skip to content

Commit 95ffad7

Browse files
committed
fix(css): resolve em against the element font-size
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
1 parent 3a156c2 commit 95ffad7

2 files changed

Lines changed: 78 additions & 9 deletions

File tree

‎crates/rustmotion-core/src/engine/layout_pass.rs‎

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ use taffy::prelude as tf;
66
use taffy::TaffyTree;
77

88
use crate::css::taffy_bridge::{to_taffy_style, ConversionContext};
9+
use crate::css::units::LengthContext;
910
use crate::engine::box_tree::{BoxNode, IntrinsicMeasure, NodeId};
1011

1112
/// 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)
8586
let mut node_map: HashMap<NodeId, tf::NodeId> = HashMap::new();
8687

8788
// Build the taffy tree top-down.
88-
let root_tf = build(&mut tree, &mut node_map, root, ctx);
89+
let root_tf = build(&mut tree, &mut node_map, root, ctx, ctx.length.font_size);
8990

9091
let viewport_size = tf::Size {
9192
width: tf::AvailableSpace::Definite(viewport.0),
@@ -119,33 +120,54 @@ pub fn run_layout(root: &BoxNode, viewport: (f32, f32), ctx: &ConversionContext)
119120
LayoutResult { layouts }
120121
}
121122

123+
/// Build one taffy node and, recursively, its subtree.
124+
///
125+
/// `inherited_font_size` is the already-resolved (px) font-size of `node`'s
126+
/// parent: CSS resolves `em` on every layout property against the
127+
/// element's *own* computed font-size, and font-size itself inherits down
128+
/// the tree unless overridden. `to_taffy_style` only ever sees the single
129+
/// `ConversionContext` handed to it, so its `em` resolution is only as
130+
/// correct as the per-node context built here — a call site building one
131+
/// shared `ConversionContext` for the whole tree (as every production
132+
/// caller of `to_taffy_style` still does directly) resolves every node's
133+
/// `em` against that one context's `font_size` instead.
122134
fn build(
123135
tree: &mut TaffyTree<NodeData>,
124136
map: &mut HashMap<NodeId, tf::NodeId>,
125137
node: &BoxNode,
126138
ctx: &ConversionContext,
139+
inherited_font_size: f32,
127140
) -> tf::NodeId {
128-
let style = to_taffy_style(&node.css, ctx);
141+
let parent_font_ctx = LengthContext {
142+
font_size: inherited_font_size,
143+
..ctx.length
144+
};
145+
let own_font_size = node
146+
.css
147+
.font_size_px_ctx(&parent_font_ctx, inherited_font_size);
148+
let node_ctx = ConversionContext {
149+
length: LengthContext {
150+
font_size: own_font_size,
151+
..ctx.length
152+
},
153+
};
154+
155+
let style = to_taffy_style(&node.css, &node_ctx);
129156
let data = NodeData {
130157
box_id: node.id,
131158
intrinsic: node.intrinsic.clone(),
132159
};
133-
let tf_id = if node.intrinsic.is_some() {
134-
// Leaf with intrinsic measurement.
135-
tree.new_leaf_with_context(style, data)
136-
.expect("taffy new_leaf")
137-
} else if node.children.is_empty() {
160+
let tf_id = if node.intrinsic.is_some() || node.children.is_empty() {
138161
tree.new_leaf_with_context(style, data)
139162
.expect("taffy new_leaf")
140163
} else {
141164
let mut child_ids = Vec::with_capacity(node.children.len());
142165
for c in &node.children {
143-
child_ids.push(build(tree, map, c, ctx));
166+
child_ids.push(build(tree, map, c, ctx, own_font_size));
144167
}
145168
let id = tree
146169
.new_with_children(style, &child_ids)
147170
.expect("taffy new_with_children");
148-
// We still want context on internal nodes (for box_id mapping).
149171
tree.set_node_context(id, Some(data)).ok();
150172
id
151173
};
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
//! Regression tests for the workstream J (layout pass & CSS unit resolution)
2+
//! audit findings on unit resolution and intrinsic measurement.
3+
4+
use rustmotion_core::css::style::{CssStyle, Edges, Size as CSize};
5+
use rustmotion_core::css::taffy_bridge::ConversionContext;
6+
use rustmotion_core::css::units::{Length, LengthPercentage as CLP};
7+
use rustmotion_core::engine::box_tree::BoxNode;
8+
use rustmotion_core::engine::layout_pass::run_layout;
9+
10+
// ---- `em` on layout properties must resolve against the element's
11+
// own (inherited) font-size, not a constant 16px ----
12+
13+
/// A card sized by its parent's explicit `font-size` (48px) inherits that
14+
/// font-size down to its own layout resolution even though it never sets
15+
/// `font-size` itself. Its `padding: "1em"` must therefore resolve to 48px
16+
/// (`1 * inherited_font_size`), not the pre-fix constant 16px.
17+
#[test]
18+
fn em_padding_resolves_against_inherited_font_size_not_a_constant_16px() {
19+
let mut root = BoxNode::container(
20+
CssStyle {
21+
font_size: Some(Length::Px(48.0)),
22+
width: Some(CSize::Length(CLP::Px(400.0))),
23+
height: Some(CSize::Length(CLP::Px(400.0))),
24+
..Default::default()
25+
},
26+
vec![BoxNode::container(
27+
CssStyle {
28+
padding: Some(Edges::Uniform(CLP::String("1em".into()))),
29+
width: Some(CSize::Length(CLP::Px(200.0))),
30+
height: Some(CSize::Length(CLP::Px(200.0))),
31+
..Default::default()
32+
},
33+
vec![],
34+
)],
35+
);
36+
root.assign_ids(1);
37+
38+
let res = run_layout(&root, (400.0, 400.0), &ConversionContext::default());
39+
let child = res.get(2).expect("child laid out");
40+
let (content_x, _, content_w, _) = child.content_box();
41+
42+
assert_eq!(
43+
content_x, 48.0,
44+
"1em padding must resolve against the inherited 48px font-size"
45+
);
46+
assert_eq!(content_w, 200.0 - 2.0 * 48.0);
47+
}

0 commit comments

Comments
 (0)