LibWeb: Interpolate axis-keyword and calculated rotate values - #11087
LibWeb: Interpolate axis-keyword and calculated rotate values#11087tcl3 wants to merge 1 commit into
rotate values#11087Conversation
Interpolation of the `rotate` property only handled the bare angle and numeric axis forms. The `x`, `y`, and `z` axis keyword forms fell back to discrete interpolation. Arguments containing math functions did the same. Normalize the axis keyword forms to their rotate3d axis vectors. Resolve calculated angle and axis arguments before interpolating. The same resolution now applies to rotate3d pairs in `transform` lists.
📝 WalkthroughWalkthroughRotation interpolation now accepts an optional animation context. The implementation recognizes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Libraries/LibWeb/Rust/src/css/animation.rs (1)
5138-5144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the transform-list context path.
The new WPT math fixture covers only the individual
rotateproperty. The local test at Line 7085 callsinterpolate_rotate_3dwithout a context and with literal arguments. Add a local Rust test fortransform: rotate3d(...)with context-dependent calculated axis and angle values. Also coverrotateZ()normalization. This will detect a lost context argument or an incorrect axis conversion.🤖 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/Rust/src/css/animation.rs` around lines 5138 - 5144, Add a local Rust regression test near the existing interpolate_rotate_3d test that exercises the transform-list context path with context-dependent calculated axis and angle values, passing the context through the transform interpolation flow. Also add coverage verifying rotateZ() normalization, including the expected converted axis and interpolation result.
🤖 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/Rust/src/css/animation.rs`:
- Around line 5138-5144: Add a local Rust regression test near the existing
interpolate_rotate_3d test that exercises the transform-list context path with
context-dependent calculated axis and angle values, passing the context through
the transform interpolation flow. Also add coverage verifying rotateZ()
normalization, including the expected converted axis and interpolation result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bab1a315-6d2b-4d94-9156-0b1a265cc37e
📒 Files selected for processing (5)
Libraries/LibWeb/Rust/src/css/animation.rsTests/LibWeb/Text/expected/wpt-import/css/css-transforms/animation/rotate-composition.txtTests/LibWeb/Text/expected/wpt-import/css/css-transforms/animation/rotate-interpolation-math-functions-tentative.txtTests/LibWeb/Text/input/wpt-import/css/css-transforms/animation/rotate-composition.htmlTests/LibWeb/Text/input/wpt-import/css/css-transforms/animation/rotate-interpolation-math-functions-tentative.html
1 participant
Interpolation of the
rotateproperty only handled the bare angle and numeric axis forms. Thex,y, andzaxis keyword forms fell back to discrete interpolation. Arguments containing math functions did the same. Normalize the axis keyword forms to their rotate3d axis vectors. Resolve calculated angle and axis arguments before interpolating. The same resolution now applies to rotate3d pairs intransformlists.This makes the rotate buttons on https://benjaminaster.com/css-minecraft/ work (there's no smooth animation when pressing the buttons but I believe that's a separate issue).