LibWeb: Invalidate viewport-relative lengths nested in math functions - #10869
Conversation
Style invalidation on viewport resize only touches elements whose computed values recorded a viewport metric dependency. Lengths resolved by the Rust calc simplification had no way to report that dependency back, so `calc(50vh - 10px)`, `min(50vh, ...)`, `clamp()` and anything reaching them through var() kept their stale pixel values after a resize until something else dirtied the element's style. On sites like YouTube that made a window resize appear to do nothing until a later unrelated invalidation moved everything at once. Carry the resolution context's tracking flag across the FFI boundary so the Rust length absolutization records viewport (and viewport-dependent font metric) resolutions the same way the C++ path does. Adds a test resizing an iframe and checking each construct picks up the new viewport height.
📝 WalkthroughWalkthroughViewport-relative length resolution now propagates a dependency flag through the C++/Rust context, including font-relative and viewport-relative cases. A new iframe resize test verifies recalculation for direct lengths and lengths nested in CSS math functions. ChangesViewport dependency tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Libraries/LibWeb/CSS/Rust/src/style_compute.rs (1)
56-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the new write-through path in a Rust unit test.
test_context()always suppliesnull_mut(), so the current tests do not executerecord_viewport_relative_length_resolution()’s unsafe pointer write. Add a test with a local boolean pointer and verify both a viewport unit and a viewport-dependent font unit set it.Also applies to: 3860-3860
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/CSS/Rust/src/style_compute.rs` around lines 56 - 68, Add a Rust unit test alongside the existing tests that creates a local boolean, assigns its pointer to an FfiLengthResolutionContext derived from test_context(), and verifies viewport-unit resolution sets the flag. Reset the flag and resolve a viewport-dependent font unit, asserting that record_viewport_relative_length_resolution() sets it there as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Libraries/LibWeb/CSS/Rust/src/style_compute.rs`:
- Around line 56-68: Add a Rust unit test alongside the existing tests that
creates a local boolean, assigns its pointer to an FfiLengthResolutionContext
derived from test_context(), and verifies viewport-unit resolution sets the
flag. Reset the flag and resolve a viewport-dependent font unit, asserting that
record_viewport_relative_length_resolution() sets it there as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a76f2efb-17af-4326-b953-a8be53ee5c17
📒 Files selected for processing (6)
Libraries/LibWeb/CSS/Length.hLibraries/LibWeb/CSS/Rust/src/animation.rsLibraries/LibWeb/CSS/Rust/src/style_compute.rsLibraries/LibWeb/CSS/StyleComputeFFI.hTests/LibWeb/Text/expected/css/viewport-relative-length-in-math-function-invalidation.txtTests/LibWeb/Text/input/css/viewport-relative-length-in-math-function-invalidation.html
Style invalidation on viewport resize only touches elements whose computed values recorded a viewport metric dependency. Lengths resolved by the Rust calc simplification had no way to report that dependency back, so
calc(50vh - 10px),min(50vh, ...),clamp()and anything reaching them through var() kept their stale pixel values after a resize until something else dirtied the element's style. On sites like YouTube that made a window resize appear to do nothing until a later unrelated invalidation moved everything at once.Carry the resolution context's tracking flag across the FFI boundary so the Rust length absolutization records viewport (and viewport-dependent font metric) resolutions the same way the C++ path does. Adds a test resizing an iframe and checking each construct picks up the new viewport height.