-
-
Notifications
You must be signed in to change notification settings - Fork 223
Description
Summary
- Context:
get_parent_font_sizeis a helper function in the CSS property getter module that retrieves the parent element's computed font-size, which is used for resolving relative CSS units (em, %, etc.) in child elements. - Bug: The function passes the child's
node_stateparameter toget_element_font_sizewhen querying the parent's font-size, instead of retrieving and using the parent'snode_state. - Actual vs. expected: When a parent element has pseudo-class-specific font-size rules (e.g.,
:hover,:active), the function returns the wrong font-size based on the child's pseudo-class state instead of the parent's actual state. - Impact: CSS properties with relative units (em, %, rem) in child elements will be computed incorrectly when parent elements have pseudo-class-specific font-size rules, causing visual inconsistencies and broken layouts during user interactions.
Code with bug
pub fn get_parent_font_size(
styled_dom: &StyledDom,
dom_id: NodeId,
node_state: &StyledNodeState, // <-- This is the CHILD's node_state
) -> f32 {
styled_dom
.node_hierarchy
.as_container()
.get(dom_id)
.and_then(|node| node.parent_id())
.map(|parent_id| get_element_font_size(styled_dom, parent_id, node_state))
// ^^^^^^^^^^ BUG 🔴: Passes child's node_state instead of parent's
.unwrap_or(azul_css::props::basic::pixel::DEFAULT_FONT_SIZE)
}From layout/src/solver3/getters.rs:115-127
Evidence
Failing test
Test script
/// Test for bug in get_parent_font_size using wrong node_state
///
/// Bug: get_parent_font_size passes child's node_state to get_element_font_size
/// when querying the parent's font-size, which means parent's pseudo-class
/// states are ignored.
///
/// Expected behavior: Parent's font-size should be computed using parent's
/// node_state (with parent's :hover, :active, :focus states), not child's.
use azul_core::{
dom::Dom,
styled_dom::{StyledDom, StyledNodeState},
};
use azul_layout::solver3::getters::get_parent_font_size;
#[test]
fn test_parent_font_size_uses_parent_node_state() {
// Create a DOM: <div><span>text</span></div>
let mut dom = Dom::create_div()
.with_child(Dom::create_text("child text"));
// Add CSS:
// - div { font-size: 16px; }
// - div:hover { font-size: 32px; } <-- parent has hover style
let css_str = "
div { font-size: 16px; }
div:hover { font-size: 32px; }
";
let (css, _errors) = azul_css::parser2::new_from_str(css_str);
// Create styled DOM
let styled_dom = StyledDom::create(&mut dom, css);
// Node IDs:
// 0 = div (parent)
// 1 = text node (child)
let child_id = azul_core::id::NodeId::new(1);
// Test 1: Parent NOT hovered, child NOT hovered
// Expected: parent font-size = 16px
let child_state_normal = StyledNodeState {
hover: false,
active: false,
focused: false,
disabled: false,
checked: false,
focus_within: false,
visited: false,
};
let parent_font_size_normal = get_parent_font_size(&styled_dom, child_id, &child_state_normal);
assert_eq!(
parent_font_size_normal, 16.0,
"Parent font-size should be 16px when not hovered"
);
// Test 2: Parent IS hovered, child NOT hovered
// Expected: parent font-size = 32px (because parent is hovered)
// BUG: Currently returns 16px because get_parent_font_size passes child's
// node_state (not hovered) instead of parent's node_state (hovered)
let child_state_not_hovered = StyledNodeState {
hover: false, // Child is NOT hovered
active: false,
focused: false,
disabled: false,
checked: false,
focus_within: false,
visited: false,
};
// This should use parent's node_state (hovered), but bug causes it to use child's node_state (not hovered)
let parent_font_size_hovered = get_parent_font_size(&styled_dom, child_id, &child_state_not_hovered);
// This assertion will FAIL due to the bug
assert_eq!(
parent_font_size_hovered, 32.0,
"BUG: Parent font-size should be 32px when parent is hovered, \
but get_parent_font_size incorrectly uses child's node_state \
instead of parent's node_state"
);
}
#[test]
fn test_parent_font_size_active_state() {
// Create a DOM: <body><span>text</span></body>
let mut dom = Dom::create_body()
.with_child(Dom::create_text("child text"));
// Add CSS:
// - body { font-size: 14px; }
// - body:active { font-size: 28px; } <-- parent has active style
let css_str = "
body { font-size: 14px; }
body:active { font-size: 28px; }
";
let (css, _errors) = azul_css::parser2::new_from_str(css_str);
// Create styled DOM
let styled_dom = StyledDom::create(&mut dom, css);
let child_id = azul_core::id::NodeId::new(1);
// Parent IS active, child NOT active
let child_state_normal = StyledNodeState {
hover: false,
active: false, // Child is NOT active
focused: false,
disabled: false,
checked: false,
focus_within: false,
visited: false,
};
// Bug: This will use child_state_normal instead of parent's active state
let parent_font_size = get_parent_font_size(&styled_dom, child_id, &child_state_normal);
// This assertion will FAIL due to the bug
assert_eq!(
parent_font_size, 28.0,
"BUG: Parent font-size should be 28px when parent is :active, \
but get_parent_font_size incorrectly uses child's node_state"
);
}Test output
running 2 tests
test test_parent_font_size_active_state ... FAILED
test test_parent_font_size_uses_parent_node_state ... FAILED
failures:
---- test_parent_font_size_active_state stdout ----
thread 'test_parent_font_size_active_state' (5979) panicked at layout/tests/test_parent_font_size_node_state_bug.rs:122:5:
assertion `left == right` failed: BUG: Parent font-size should be 28px when parent is :active, but get_parent_font_size incorrectly uses child's node_state
left: 14.0
right: 28.0
---- test_parent_font_size_uses_parent_node_state stdout ----
thread 'test_parent_font_size_uses_parent_node_state' (5980) panicked at layout/tests/test_parent_font_size_node_state_bug.rs:77:5:
assertion `left == right` failed: BUG: Parent font-size should be 32px when parent is hovered, but get_parent_font_size incorrectly uses child's node_state instead of parent's node_state
left: 16.0
right: 32.0
failures:
test_parent_font_size_active_state
test_parent_font_size_uses_parent_node_state
test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Example
Consider this CSS and DOM structure:
div { font-size: 16px; }
div:hover { font-size: 32px; }
span { padding: 1em; } /* 1em should = parent's font-size */<div> <!-- parent with :hover state -->
<span>text</span> <!-- child without :hover state -->
</div>When the user hovers over the <div> (parent):
- Parent's pseudo-class state:
hover = true - Child's pseudo-class state:
hover = false(child not directly hovered)
Expected behavior:
- Parent font-size = 32px (from
div:hover) - Child's padding = 1em = 32px (because 1em refers to parent font-size)
Actual behavior (due to bug):
- Parent font-size = 32px ✓ (correct)
- When computing child's padding,
get_parent_font_sizeis called with child'snode_state(hover=false) - This causes parent font-size to be computed as 16px (from
div, notdiv:hover) - Child's padding = 1em = 16px ✗ (wrong - should be 32px)
Inconsistency within the codebase
Reference code
layout/src/solver3/getters.rs:1402-1423 - The get_style_properties function correctly retrieves parent's node_state:
let parent_font_size = styled_dom
.node_hierarchy
.as_container()
.get(dom_id)
.and_then(|node| {
let parent_id = CoreNodeId::from_usize(node.parent)?;
// Recursively get parent's font-size
cache
.get_font_size(
&styled_dom.node_data.as_container()[parent_id],
&parent_id,
&styled_dom.styled_nodes.as_container()[parent_id].styled_node_state, // <-- Correct: uses parent's node_state
)
.and_then(|v| v.get_property().cloned())
.map(|v| {
use azul_css::props::basic::pixel::DEFAULT_FONT_SIZE;
v.inner.to_pixels_internal(0.0, DEFAULT_FONT_SIZE)
})
})
.unwrap_or(azul_css::props::basic::pixel::DEFAULT_FONT_SIZE);Current code
layout/src/solver3/getters.rs:115-127:
pub fn get_parent_font_size(
styled_dom: &StyledDom,
dom_id: NodeId,
node_state: &StyledNodeState, // This is the CHILD's node_state
) -> f32 {
styled_dom
.node_hierarchy
.as_container()
.get(dom_id)
.and_then(|node| node.parent_id())
.map(|parent_id| get_element_font_size(styled_dom, parent_id, node_state))
// ^^^^^^^^^^ Wrong: uses child's node_state
.unwrap_or(azul_css::props::basic::pixel::DEFAULT_FONT_SIZE)
}Contradiction
In get_style_properties, the code correctly retrieves the parent's node_state from styled_dom.styled_nodes.as_container()[parent_id].styled_node_state before querying the parent's CSS properties. However, get_parent_font_size incorrectly uses the child's node_state parameter instead of retrieving the parent's actual node_state, violating the principle that CSS properties should be queried with the element's own pseudo-class state.
Full context
StyledNodeState Definition
StyledNodeState (defined in core/src/styled_dom.rs:110-120) contains pseudo-class flags that are node-specific:
pub struct StyledNodeState {
pub hover: bool, // Element is being hovered (:hover)
pub active: bool, // Element is active/being clicked (:active)
pub focused: bool, // Element has focus (:focus)
pub disabled: bool, // Element is disabled (:disabled)
pub checked: bool, // Element is checked/selected (:checked)
pub focus_within: bool,
pub visited: bool,
}These states are used by the CSS property cache to determine which CSS rules apply to an element based on pseudo-class selectors.
Usage in Layout System
get_parent_font_size is a widely-used utility function that is called when creating ResolutionContext objects for resolving CSS length units. It appears in:
- Inline text layout (
fc.rs:2462, 2494) - Used to resolvetext-indentandcolumn-gapwith em units - Border calculations (
fc.rs:2885) - Used for em/rem resolution in border widths - Table layout (
fc.rs:3368, 4410) - Used for resolving table cell spacing with relative units - Absolute positioning (
positioning.rs:93) - Used for resolving positioned element properties - General layout tree creation (
layout_tree.rs:1182) - Used in resolution context creation
CSS Property Resolution
The CSS property cache (CssPropertyCache) methods take three parameters:
node_data: The element's node datanode_id: The element's IDnode_state: The element's pseudo-class state
The node_state parameter is critical because it determines which CSS rules match. For example, with these rules:
div { font-size: 16px; }
div:hover { font-size: 32px; }The cache will return different values depending on node_state.hover:
- If
node_state.hover == false→ returns 16px - If
node_state.hover == true→ returns 32px
Why the Bug Exists
When getting a parent's font-size for use in the child's property resolution, the function needs to query the parent's CSS properties using the parent's pseudo-class state. However, the current implementation receives the child's node_state as a parameter and incorrectly forwards it when querying the parent's properties.
This means that if a parent has :hover-specific font-size but the child is not hovered, the function will query the parent's properties as if the parent is also not hovered, even when it actually is.
Why has this bug gone undetected?
This bug has likely gone undetected for several reasons:
-
Uncommon CSS pattern: It's relatively rare to have pseudo-class-specific font-sizes on parent elements combined with em-unit properties on child elements. Most developers set static font-sizes on parents or use em units at the same element level.
-
Pseudo-class propagation behavior: In typical hover interactions, when you hover a parent, you're often also hovering the child (since the child is inside the parent's bounding box). In these cases, both parent and child have
hover: true, so the bug doesn't manifest. The bug only appears in edge cases where:- Parent is hovered but child is not (e.g., parent has padding and mouse is over the padding area)
- Parent has one pseudo-class active (e.g.,
:active) but child has different states - Programmatic state changes where parent and child states diverge
-
Visual subtlety: When the bug occurs, it causes incorrect sizing/spacing but may not be immediately obvious. For example, if padding is 31px instead of 32px due to incorrect em calculation, most users wouldn't notice the 1px difference.
-
Limited test coverage: There were no existing tests that specifically checked parent font-size resolution with different parent/child pseudo-class states. Tests typically use uniform states across the DOM tree.
-
Recent introduction: The font-size resolution code was refactored in commit
5e460beb("Fix font-size resolution bugs"), and this bug was introduced or preserved during that refactoring. The main functionality worked correctly for the common cases, so the edge case with divergent pseudo-class states wasn't caught. -
Caching masks the issue: The CSS property cache includes a dependency chain mechanism that caches resolved values. In many cases, cached values are used instead of re-resolving, which can hide the incorrect resolution path.
Recommended fix
The function should retrieve the parent element's node_state from styled_dom.styled_nodes before calling get_element_font_size:
pub fn get_parent_font_size(
styled_dom: &StyledDom,
dom_id: NodeId,
node_state: &StyledNodeState, // Child's node_state (not used after fix)
) -> f32 {
styled_dom
.node_hierarchy
.as_container()
.get(dom_id)
.and_then(|node| node.parent_id())
.map(|parent_id| {
// FIX 🟢: Get the parent's node_state before querying parent's font-size
let parent_node_state = &styled_dom.styled_nodes.as_container()[parent_id].styled_node_state;
get_element_font_size(styled_dom, parent_id, parent_node_state) // FIX 🟢
})
.unwrap_or(azul_css::props::basic::pixel::DEFAULT_FONT_SIZE)
}Note: After this fix, the node_state parameter becomes unused. The function could either:
- Remove the parameter entirely (breaking API change, requires updating all call sites)
- Keep the parameter for backward compatibility but add a comment explaining it's not used
- Add a debug assertion to verify the passed
node_statematches the child's actual state (as documentation)
The same fix should be applied to get_root_font_size at line 130-133, which has the same bug where it passes the child's node_state when querying the root element's font-size.