Repository navigation
fix(animation): make every animated property reach layout and the frame - #443
Merged
Merged
Conversation
Two issues, one architecture. #426 reported that an animation touching only border_radius, clip_path_progress or font_size never applied; #430 reported letter_spacing having no effect on rich_text and gradient_text, animated tracking overflowing because layout used the static value, and a static opacity: 0 cancelling a fade-in. Three separate defects sat behind them. props_has_paint_overrides gated whether apply_animated_props ran at all. It listed translate, scale, rotation, opacity, blur, glow, perspective, width and height, and had gone stale against every property added to AnimatedProperties since. The gate is deleted rather than completed: apply_animated_props already guards each branch against the resting value, so the list bought nothing and left the same trap for the next property anyone adds. with_cascaded_style folded the box's own resolved style onto a clone of the same component through inherit_from, which only fills fields that are None. A component declaring its own font-size or letter-spacing therefore never saw the animated value. inherit_from is right for what it is named after -- parent to child -- so the re-cascade gets its own function, overlay_resolved_typography, which overwrites. Conflating the two was the bug. rich_text ignored style.letter-spacing outright, static or animated: compute_layout discarded the resolved value into a binding named _letter_spacing_unused and resolve_span_fonts defaulted every span to 0.0. Neither fix above would have reached it. An animation on opacity now replaces the declared static value instead of multiplying with it, which is how every other property in apply_animated_props already treats its declaration. This changes the render of a scenario that declares an opacity and animates it: a node at opacity 0.6 with a pulse used to paint 0.6 * pulse and now paints the pulse. Effects still compose with each other -- that happens upstream in AnimatedProperties::merge and is untouched. Closes #426 Closes #430
with_cascaded_style had two meanings depending on who called it. The render path passes the node's own already-resolved style, where overwriting is right because that style has already applied the component's own declaration over its parent's. These two tests passed it a bare parent style instead, where filling only the gaps was right. Making the function overwrite fixed the first caller and broke the second, which is what turned table_own_color_wins_over_cascaded_card_color red on CI while every local scoped run stayed green. The invariant both tests assert is unchanged and still worth asserting. They now build the resolved style the box tree builds -- the component's own, with inherit_from filling what it left unset -- and pass that, which is the shape the render path actually uses.
12 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #426. Closes #430. Part of #438.
Two issues, one architecture. #426 reported that an animation touching only
border_radius,clip_path_progressorfont_sizenever applied; #430 reportedletter_spacinghaving no effect onrich_textandgradient_text, animatedtracking overflowing because layout used the static value, and a static
opacity: 0cancelling a fade-in. Three separate defects sat behind them, andonly one was the shared root cause the decomposition predicted.
The gate.
props_has_paint_overridesdecided whetherapply_animated_propsran at all. It listed translate, scale, rotation, opacity, blur, glow,
perspective, width and height, and had gone stale against every property added to
AnimatedPropertiessince. It is deleted rather than completed:apply_animated_propsalready guards each branch against the resting value, sothe list bought nothing and left the same trap for the next property anyone adds.
Reverting it turns seven tests red.
The cascade.
with_cascaded_stylefolded the box's own resolved style onto aclone of the same component through
inherit_from, which only fills fields thatare
None. A component declaring its ownfont-sizeorletter-spacingtherefore never saw the animated value — at measurement time, which is why the
painted run could be right while the layout was wrong.
inherit_fromis correctfor what it is named after, parent to child, so the re-cascade gets its own
function:
overlay_resolved_typography, which overwrites. Conflating the twowas the bug, and a boolean flag on
inherit_fromwould have preserved it.rich_text. It ignoredstyle.letter-spacingoutright, static or animated:compute_layoutdiscarded the resolved value into a binding literally named_letter_spacing_unused, andresolve_span_fontsdefaulted every span to0.0.Neither fix above would have reached it.
gradient_textneeded no change — italready reads its own cascaded style correctly, and was broken only upstream.
An animation on
opacitynow replaces the declared static value instead ofmultiplying with it, which is how every other property in
apply_animated_propsalready treats its declaration. A node at
opacity: 0.6carrying apulseusedto paint
0.6 × pulseand now paints the pulse.This does not contradict
rules/animations-compose.md. That rule is about severaleffects on one property composing with each other, which happens upstream in
AnimatedProperties::mergeand is untouched — by the timeapply_animated_propssees
props.opacity, the effects are already multiplied together. The axis hereis the static declaration against the merged result, which the rule does not
cover.
Tests
Eight full-pipeline render tests in
box_builder.rs— scenario, layout, paint,read pixels — plus unit tests on
apply_animated_props,overlay_resolved_typographyand
with_cascaded_style. Every fix was reverted individually and watched go red:cargo test -p rustmotion-core --lib css::114 passed. Clippy and the full suiterun at integration; a sibling workstream is still mid-flight in this checkout.