Correctly compute keyframe at timestamp 0; Early return when possible by yezhizhen · Pull Request #354 · servo/stylo

yezhizhen · 2026-04-20T04:41:45Z

The keyframe selection used <= for normal direction, causing the first keyframe to be selected at progress 0; changed to < for normal direction (kept <= for reverse to avoid Fix CSS step timing functions jump-both, jump-start, start at timestamp 0; Early return when possible #342 (comment)) and added direction‑aware step‑function handling.
Guard against division‑by‑zero for zero‑length keyframe intervals.

Make early return happen earlier:

total_progress == 1.0 and total_progress < 0 now handled immediately. Also make it direction aware.
Zero-interval: we exit early to avoid division‑by‑zero in percentage_between_keyframes

yezhizhen · 2026-04-24T15:26:34Z

@Loirooriol Just friendly reminder.

Loirooriol · 2026-04-30T12:12:36Z


+        let prev_keyframe = &self.computed_steps[prev_keyframe_index];
        let Some(next_keyframe_index) = next_keyframe_index else {
+            debug_assert!(false, "next_keyframe_index should always be Some");


Using debug_unreachable may be clearer.
Also the message isn't providing much, it should rather explain why.

Done in 2701332

The explanation is a bit lengthy tho. We are relying on

stylo/style/servo/animation.rs

Lines 175 to 176 in 40aa59d

debug_assert!(intermediate_steps.first().unwrap().start_percentage == 0.);

debug_assert!(intermediate_steps.last().unwrap().start_percentage == 1.);

Loirooriol · 2026-04-30T12:17:11Z

+        // Progress clamped to the current iteration (0.0 to 1.0).
+        let total_progress = progress.min(self.current_iteration_end_progress()).max(0.0);
+
+        if total_progress >= 1.0 {


Why can this handle the equality, but above you check progress < 0.0?

In 2701332 I updated to ==1.0 as it is always in [0,1].

but above you check progress < 0.0?

At progress == 0.0, we don't want to early return because we need to reach the step‑function handling block at L878 we added.

But don't we have the same jumping problem at the end?

servo/servo#43086

End has always been fine. We only have problem at the beginning.

yezhizhen · 2026-05-07T09:48:44Z

@Loirooriol Do you still have other concerns for this?

xiaochengh

My previous comments at #342 seem unaddressed:

yezhizhen · 2026-05-14T07:10:19Z

I have removed special casing for step functions in 4c6b772. It is no longer imported.

This should be much more straightforward to review now. We do miss one early return, but the readability is worth it.

Now the PR is mostly renaming, reordering, adding comments, and computing index at boundary correctly.

yezhizhen · 2026-05-14T07:16:31Z

@Loirooriol Xiaocheng would probably be off this week. Do you mind checking this again?

…rst keyframe to be selected at progress 0; changed to < for normal direction (kept <= for reverse to avoid #342 (comment)) and added direction‑aware step‑function handling. - Guard against division‑by‑zero for zero‑length keyframe intervals. 1. total_progress ≥ 1.0 and total_progress < 0 now handled immediately. Also make it direction aware. 2. At timestamp 0, for normal direction, non‑jump step functions (jump‑end, jump‑none, end) now return the start‑keyframe value early, instead of proceeding to interpolation. 3. Zero-interval: we exit early to avoid division‑by‑zero in `percentage_between_keyframes` Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

xiaochengh

The implementation change looks very reasonable to me.

yezhizhen · 2026-05-18T03:21:51Z

@Loirooriol @mrobinson I don't mean to nag. This isn't rocket science, but has been pended for two months. I also asked @Loirooriol two weeks ago if you still have concerns but just getting ignored.

xiaochengh · 2026-05-18T05:32:44Z

Nit: maybe we need to modify the PR title, since the special handling for step functions have been removed?

mrobinson

Looks good with the nits below:

mrobinson · 2026-05-18T08:58:06Z

+            let keyframe = match self.current_direction {
+                AnimationDirection::Normal => self.computed_steps.first().unwrap(),
+                AnimationDirection::Reverse => self.computed_steps.last().unwrap(),
+                _ => unreachable!(),
+            };
+            add_declarations_to_map(keyframe);


Let's avoid unwrap() here and also below:

Suggested change

let keyframe = match self.current_direction {

AnimationDirection::Normal => self.computed_steps.first().unwrap(),

AnimationDirection::Reverse => self.computed_steps.last().unwrap(),

_ => unreachable!(),

};

add_declarations_to_map(keyframe);

if let Some(keyframe) = match self.current_direction {

AnimationDirection::Normal => self.computed_steps.first(),

AnimationDirection::Reverse => self.computed_steps.last(),

_ => unreachable!(),

} {

add_declarations_to_map(keyframe);

}

stylo/style/servo/animation.rs

Lines 173 to 176 in a683655

// We should always have a first and a last step, even if these are just

// generated by KeyframesStepValue::ComputedValues.

debug_assert!(intermediate_steps.first().unwrap().start_percentage == 0.);

debug_assert!(intermediate_steps.last().unwrap().start_percentage == 1.);

But the animation is guaranteed to have them. We also unwrapped earlier in previous code.

Still we should avoid unwrap when possible (servo/servo#40744). If the code above changes, the person changing it might not get to this code, which can cause a panic later on.

Good point! I guess there's nothing we can do with unwrap in this debug_assert.
Done in 01ef6d6

yezhizhen · 2026-05-18T09:10:15Z

@mrobinson Thanks for review! Addressing this soon.

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

yezhizhen

I have replaced the unreachable string as "Current animation direction can only be normal or reverse" instead, to be consistent with

stylo/style/servo/animation.rs

Lines 557 to 558 in a683655

    
               /// The current animation direction. This can only be `normal` or `reverse`. 
        
               pub current_direction: AnimationDirection,

. Also added it to fn iterate.

yezhizhen · 2026-05-18T09:53:52Z

+            let keyframe = match self.current_direction {
+                AnimationDirection::Normal => self.computed_steps.first().unwrap(),
+                AnimationDirection::Reverse => self.computed_steps.last().unwrap(),
+                _ => unreachable!(),
+            };
+            add_declarations_to_map(keyframe);


stylo/style/servo/animation.rs

Lines 173 to 176 in a683655

// We should always have a first and a last step, even if these are just

// generated by KeyframesStepValue::ComputedValues.

debug_assert!(intermediate_steps.first().unwrap().start_percentage == 0.);

debug_assert!(intermediate_steps.last().unwrap().start_percentage == 1.);

But the animation is guaranteed to have them. We also unwrapped earlier in previous code.

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

…ame correctly; Eliminate Stylo static preference hashing (servo#44365) Stylo PR: servo/stylo#354 This also includes servo/stylo#367 which eliminates static pref hashing Testing: New passes in existing tests: revert-rule, css-easing, css-logical. [revert-rule](https://drafts.csswg.org/css-cascade-5/#revert-rule-keyword) seems to be one of latest CSS keywords. I cannot even find it in MDN. Fixes: servo#43086 I'm unable to reopen the previous PR servo#43594. --------- Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

yezhizhen requested a review from xiaochengh April 20, 2026 04:41

yezhizhen marked this pull request as ready for review April 20, 2026 05:21

yezhizhen requested a review from Loirooriol April 20, 2026 09:59

Loirooriol reviewed Apr 30, 2026

View reviewed changes

yezhizhen marked this pull request as draft May 1, 2026 06:01

yezhizhen force-pushed the fix-quantization branch from 398c241 to 1d1f9ce Compare May 1, 2026 06:32

yezhizhen marked this pull request as ready for review May 1, 2026 06:55

yezhizhen force-pushed the fix-quantization branch from dbc9ddd to 0cf455c Compare May 1, 2026 08:36

yezhizhen requested a review from Loirooriol May 1, 2026 08:45

Loirooriol force-pushed the main branch from 40aa59d to 572ecba Compare May 4, 2026 11:17

yezhizhen force-pushed the fix-quantization branch from 0cf455c to ff53dff Compare May 6, 2026 08:57

xiaochengh reviewed May 11, 2026

View reviewed changes

yezhizhen marked this pull request as draft May 14, 2026 06:55

yezhizhen marked this pull request as ready for review May 14, 2026 07:07

yezhizhen added 4 commits May 14, 2026 15:59

Be more accurate

a3ed071

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

Fix warning from unreachable_debug

a30b8df

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

no special casing for step functions

fdd0908

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

yezhizhen force-pushed the fix-quantization branch from 4c6b772 to fdd0908 Compare May 14, 2026 07:59

xiaochengh approved these changes May 15, 2026

View reviewed changes

Comment thread style/servo/animation.rs

yezhizhen changed the title ~~Fix CSS step timing functions jump-both, jump-start, start at timestamp 0; Early return when possible~~ Correctly compute first keyframe at timestamp 0; Early return when possible May 18, 2026

yezhizhen changed the title ~~Correctly compute first keyframe at timestamp 0; Early return when possible~~ Correctly compute keyframe at timestamp 0; Early return when possible May 18, 2026

mrobinson approved these changes May 18, 2026

View reviewed changes

review

4132d64

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

yezhizhen commented May 18, 2026

View reviewed changes

yezhizhen requested a review from mrobinson May 18, 2026 09:59

don't unwrap

01ef6d6

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

mrobinson added this pull request to the merge queue May 18, 2026

Merged via the queue into main with commit f611007 May 18, 2026
5 checks passed

mrobinson deleted the fix-quantization branch May 18, 2026 13:39

	debug_assert!(intermediate_steps.first().unwrap().start_percentage == 0.);
	debug_assert!(intermediate_steps.last().unwrap().start_percentage == 1.);

	// We should always have a first and a last step, even if these are just
	// generated by KeyframesStepValue::ComputedValues.
	debug_assert!(intermediate_steps.first().unwrap().start_percentage == 0.);
	debug_assert!(intermediate_steps.last().unwrap().start_percentage == 1.);

	/// The current animation direction. This can only be `normal` or `reverse`.
	pub current_direction: AnimationDirection,

Conversation

yezhizhen commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Make early return happen earlier:

Uh oh!

yezhizhen commented Apr 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yezhizhen May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yezhizhen May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yezhizhen commented May 7, 2026

Uh oh!

xiaochengh left a comment

Choose a reason for hiding this comment

Uh oh!

yezhizhen commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yezhizhen commented May 14, 2026

Uh oh!

xiaochengh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yezhizhen commented May 18, 2026

Uh oh!

xiaochengh commented May 18, 2026

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yezhizhen commented May 18, 2026

Uh oh!

yezhizhen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yezhizhen commented Apr 20, 2026 •

edited

Loading

yezhizhen May 1, 2026 •

edited

Loading

yezhizhen May 1, 2026 •

edited

Loading

yezhizhen commented May 14, 2026 •

edited

Loading

yezhizhen left a comment •

edited

Loading