Rework `ElementAnimationSet::start_transition_if_applicable` to be more like the specification #145

yezhizhen · 2025-03-14T07:47:14Z

This improves the behavior of transition toggling, cancellation, and
delay. In particular, there are many improvements with how transitions
are started when replacing an existing one.

Servo PR: servo/servo#35978

Co-authored-by: Martin Robinson [email protected]
Signed-off-by: Euclid Ye [email protected]
Signed-off-by: Martin Robinson [email protected]

nicoburns

This needs to be rebased on top of the latest main

style/animation.rs

yezhizhen · 2025-03-14T16:44:13Z

Test re-running
https://github.com/yezhizhen/servo/actions/runs/13869145195/job/38814205426

PR has now been rebased

style/animation.rs

yezhizhen · 2025-03-17T10:14:29Z

What should I rebase on now? I got many conflicts when rebasing on main.

Loirooriol · 2025-03-17T10:22:05Z

I force-pushed the main branch to sync changes from upstream, which is not ideal for PRs.

With your branch at 08279e1, you should be able to do

git rebase --onto main 7ee9643e7ff1633c709f4ffd683050eb3c726ec9 fix-transition-order

Where 7ee9643 is the nearest common ancestor of your branch and https://github.com/servo/stylo/commits/2025-03-11/

yezhizhen · 2025-03-17T10:49:52Z

Somehow I cannot compile anymore.. I just fetched the latest Servo main branch.

I am on the latest Servo main branch.
Latest Stylo branch, with my five commits.
But I get a lot of

error[E0053]: method `report_error` has an incompatible type for trait
   --> components\script\dom\fontface.rs:180:20
    |
180 |         _location: cssparser::SourceLocation,
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `style::values::SourceLocation`, found `cssparser::SourceLocation`

nicoburns · 2025-03-17T11:00:27Z

Somehow I cannot compile anymore.. I just fetched the latest Servo main branch.
I am on the latest Servo main branch. Latest Stylo branch, with my five commits.

@yezhizhen You upgraded to the latest Stylo branch a little too quickly! CI is still running on the corresponding Servo PR (servo/servo#35990). Your code should compile again once that merges. Until then you can run against the 2025-03-12 Stylo branch.

yezhizhen · 2025-03-17T11:01:39Z

Haha thanks Nico! I will get back tomorrow then :)

style/animation.rs

yezhizhen · 2025-03-17T15:41:23Z

I still cannot compile locally, with latest pulled repo
I think there is something wrong with current Cargo.toml in Servo for using local stylo.

I made some adjustments to Cargo.toml in Servo. It starts to compile, but has following errors now:

error[E0277]: the trait bound `ElementState: MallocSizeOf` is not satisfied
   --> D:\Rust\Cargo\git\checkouts\stylo-482338307e42a9ea\6aa5733\style\servo\selector_parser.rs:778:26
    |
778 | #[derive(Debug, Default, MallocSizeOf)]
    |                          ^^^^^^^^^^^^ the trait `MallocSizeOf` is not implemented for `ElementState`

EDIT: Seems to work now after servo/servo#36011

yezhizhen · 2025-03-27T05:58:20Z

it will be easier to see what you have fixed. Thank you!

current.servo+stylo.main.branch.mp4

after.apply.fix.mp4

mrobinson · 2025-03-27T12:41:02Z

@yezhizhen Thanks! I'll take a look at this today.

mrobinson · 2025-03-28T12:09:44Z

Okay. I've spent some time looking more closely at this change and I think it's a good start! Apologies for the current mess, but I think this is a good opportunity to actually implement the specification in the order it is written.

Add a start_new_transition method that can be used for early returns.
Restructure start_transition_if_applicable to match the specification.
- First check all the conditions of step 1, caching any values that are used twice, such as:
```
    let combined_duration_is_greater_than_zero = ...
```
- If all the conditions for 1 are met, you can do an early return:
```
    if ... &&
       combined_duration &&
       existing_transition.is_none()
  {
     self.start_new_transition(...);
     return;
  }
```
- After step 1 you can do an early return if there is no pre-existing transition as all the subsequent steps rely on there being one:
```
     if let Some(existing_transition) = existing_transition else {
         return;
     }
```
The final part of start_transition_if_applicable would be checking the various specification conditions related to existing_transition in the order specified, finally canceling existing_transition and starting the new transition in step 4 if necessary.

yezhizhen · 2025-03-29T09:15:23Z

Okay. I've spent some time looking more closely at this change and I think it's a good start! Apologies for the current mess, but I think this is a good opportunity to actually implement the specification in the order it is written.

Thanks for the suggestions! When I opened this PR I try to work around original code without altering it too much. It is nice we can do this now :)

yezhizhen · 2025-03-29T15:51:18Z

@mrobinson I just restructured the code completely to follow exact same order as spec is written in. And my newly added wpt-tests and other manual tests are still passing.

EDIT: Latest test result: https://github.com/yezhizhen/servo/actions/runs/14153448812

mrobinson · 2025-04-02T09:19:21Z

@yezhizhen I've pushed a couple commits onto your branch with some minor changes. Please take a look. Do these seeem okay to you?

yezhizhen

@mrobinson LGTM mostly, but Condition 3 of step 1 is not checked in your latest commit.
I also have some other questions below :)

style/animation.rs

yezhizhen · 2025-04-02T09:52:53Z

style/animation.rs

+            match ignore_transitions {
+                IgnoreTransitions::Canceled => {


I feel a bit clumsy, as this check is done in each iteration despite the static execution path. And we need to introduce a extra enum.

Does compiler optimize it away? Would it still be better, if we just create two functions?

I think that two functions would lead to a good deal of code repetition, so a single function makes more sense to me. This kind of check should be very cheap. Another option here could be using bitflags.

yezhizhen · 2025-04-02T09:55:00Z

style/matching.rs

        &self,
        context: &mut StyleContext<Self>,
-        old_styles: &ElementStyles,
+        old_styles: &mut ElementStyles,


It is not mutated tho. Would it be better to just remove mut?

Yep, that's correct, but these changes are unrelated so should be posted in another PR.

style/matching.rs

yezhizhen · 2025-04-02T10:08:17Z

style/animation.rs

-        if !has_running_transition && (transitionable && from != to) && (duration + delay > 0.0) {
-            // > 3. the element does not have a completed transition for the property or the end value 
-            // >    of the completed transition is different from the after-change style for the property
-            let mut start_new_transition = !has_completed_transition;
-            if has_completed_transition {
-                let completed_transition = old_transition.as_ref().unwrap();
-                start_new_transition = completed_transition.property_animation.to != to;


This condition 3 also needs to be checked in step 1.
Right now it is removed in your latest commit.

Nice catch. I've added this back.

yezhizhen · 2025-04-02T12:31:54Z

https://github.com/yezhizhen/servo/actions/runs/14217536133

@mrobinson The latest try run in Servo PR for this Stylo PR is successful.
Also, what do you think about above unresolved conversation?

style/matching.rs

mrobinson · 2025-04-02T10:03:47Z

style/matching.rs

        &self,
        context: &mut StyleContext<Self>,
-        old_styles: &ElementStyles,
+        old_styles: &mut ElementStyles,


Yep, that's correct, but these changes are unrelated so should be posted in another PR.

mrobinson · 2025-04-02T10:11:35Z

style/animation.rs

+            match ignore_transitions {
+                IgnoreTransitions::Canceled => {


I think that two functions would lead to a good deal of code repetition, so a single function makes more sense to me. This kind of check should be very cheap. Another option here could be using bitflags.

mrobinson · 2025-04-02T10:11:55Z

style/animation.rs

-        if !has_running_transition && (transitionable && from != to) && (duration + delay > 0.0) {
-            // > 3. the element does not have a completed transition for the property or the end value 
-            // >    of the completed transition is different from the after-change style for the property
-            let mut start_new_transition = !has_completed_transition;
-            if has_completed_transition {
-                let completed_transition = old_transition.as_ref().unwrap();
-                start_new_transition = completed_transition.property_animation.to != to;


Nice catch. I've added this back.

mrobinson · 2025-04-02T14:09:50Z

@yezhizhen Sorry, for some reason my comment on the unresolved conversation was left in a draft state. You can find it above now.

mrobinson · 2025-04-02T14:10:16Z

If this looks okay to you, I will squash the changes and land the PR.

yezhizhen · 2025-04-02T14:55:42Z

@mrobinson LGTM. Please land :)

For the Servo PR, I believe that I need to rebase and revert Cargo.toml and Cargo.lock?

…re like the specification This improves the behavior of transition toggling, cancellation, and delay. In particular, there are many improvements with how transitions are started when replacing an existing one. Co-authored-by: Martin Robinson <[email protected]> Signed-off-by: Euclid Ye <[email protected]> Signed-off-by: Martin Robinson <[email protected]>

mrobinson

Apologies for the long process to review this!

mrobinson · 2025-04-04T07:50:06Z

I'm going to land this one event though the Servo PR isn't approved yet as it shouldn't cause any issues in Servo except another test will start passing.

yezhizhen · 2025-04-04T08:34:52Z

I'm going to land this one event though the Servo PR isn't approved yet as it shouldn't cause any issues in Servo except another test will start passing.

Should be fine as Cargo.lock in Servo main branch hasn't updated yet.

@Asun0204

More details in Stylo PR: servo/stylo#145 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes - fixes #35833 - fixes #35982  - [x] There are new passing test: `css/css-logical/animation-004.html: Transitions from physical to logical update when the direction is changed` Created new test files as well: 1. `css-transitions/transition-remove-and-change-immediate.html` 2. `css-transitions/transition-zero-duration-with-delay.html` 3. `css-transitions/transitioncancel-003.html`   cc @Asun0204 @xiaochengh @stevennovaryo Signed-off-by: Euclid Ye <[email protected]>

@Asun0204

More details in Stylo PR: servo/stylo#145 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes - fixes servo#35833 - fixes servo#35982  - [x] There are new passing test: `css/css-logical/animation-004.html: Transitions from physical to logical update when the direction is changed` Created new test files as well: 1. `css-transitions/transition-remove-and-change-immediate.html` 2. `css-transitions/transition-zero-duration-with-delay.html` 3. `css-transitions/transitioncancel-003.html`   cc @Asun0204 @xiaochengh @stevennovaryo Signed-off-by: Euclid Ye <[email protected]>

nicoburns previously requested changes Mar 14, 2025

View reviewed changes

yezhizhen force-pushed the fix-transition-order branch from 230370b to ed063e7 Compare March 14, 2025 07:55

yezhizhen changed the title ~~Fix transition order~~ Fix broken transition toggle Mar 14, 2025

yezhizhen marked this pull request as ready for review March 14, 2025 11:11

yezhizhen requested a review from nicoburns March 14, 2025 11:11

yezhizhen changed the title ~~Fix broken transition toggle~~ Fix broken transition toggle and cancelling Mar 14, 2025

Loirooriol reviewed Mar 14, 2025

View reviewed changes

style/animation.rs Outdated Show resolved Hide resolved

style/animation.rs Outdated Show resolved Hide resolved

style/animation.rs Outdated Show resolved Hide resolved

yezhizhen force-pushed the fix-transition-order branch 2 times, most recently from 7f43b7b to 090b1fe Compare March 14, 2025 16:37

yezhizhen mentioned this pull request Mar 14, 2025

Fix transition toggle & cancellation & delay servo/servo#35978

Merged

4 tasks

yezhizhen commented Mar 15, 2025

View reviewed changes

style/animation.rs Outdated Show resolved Hide resolved

yezhizhen changed the title ~~Fix broken transition toggle and cancelling~~ Fix broken transition toggle, cancellation, and delay Mar 15, 2025

yezhizhen force-pushed the fix-transition-order branch 2 times, most recently from 80fa179 to 560638c Compare March 16, 2025 07:28

yezhizhen commented Mar 17, 2025

View reviewed changes

style/animation.rs Outdated Show resolved Hide resolved

Loirooriol force-pushed the main branch from ae81d0a to 6aa5733 Compare March 17, 2025 10:06

yezhizhen force-pushed the fix-transition-order branch 2 times, most recently from 172dac0 to 28fc84e Compare March 17, 2025 10:35

Loirooriol reviewed Mar 17, 2025

View reviewed changes

style/animation.rs Outdated Show resolved Hide resolved

style/animation.rs Outdated Show resolved Hide resolved

yezhizhen force-pushed the fix-transition-order branch 2 times, most recently from 4a2dd8c to 4ef8b50 Compare March 17, 2025 14:55

yezhizhen requested a review from Loirooriol March 18, 2025 02:57

yezhizhen force-pushed the fix-transition-order branch 2 times, most recently from ecdab05 to e46917b Compare March 30, 2025 06:26

yezhizhen commented Apr 2, 2025

View reviewed changes

mrobinson reviewed Apr 2, 2025

View reviewed changes

mrobinson force-pushed the fix-transition-order branch from 33247f9 to f51e372 Compare April 2, 2025 17:41

mrobinson force-pushed the fix-transition-order branch from f51e372 to ea495b1 Compare April 2, 2025 17:43

mrobinson changed the title ~~Fix broken transition toggle, cancellation, and delay~~ Rework ElementAnimationSet::start_transition_if_applicable to be more like the specification Apr 2, 2025

mrobinson approved these changes Apr 2, 2025

View reviewed changes

mrobinson approved these changes Apr 4, 2025

View reviewed changes

mrobinson added this pull request to the merge queue Apr 4, 2025

Merged via the queue into servo:main with commit 127952e Apr 4, 2025
3 checks passed

yezhizhen deleted the fix-transition-order branch April 4, 2025 08:15

yezhizhen mentioned this pull request May 30, 2025

matching.rs: remove unnecessary mut specifier #193

Merged

Rework ElementAnimationSet::start_transition_if_applicable to be more like the specification #145

Rework ElementAnimationSet::start_transition_if_applicable to be more like the specification #145

Uh oh!

Conversation

yezhizhen commented Mar 14, 2025 • edited by mrobinson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoburns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yezhizhen commented Mar 17, 2025

Uh oh!

Loirooriol commented Mar 17, 2025

Uh oh!

yezhizhen commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoburns commented Mar 17, 2025

Uh oh!

yezhizhen commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

yezhizhen commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yezhizhen commented Mar 27, 2025

Uh oh!

mrobinson commented Mar 27, 2025

Uh oh!

mrobinson commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yezhizhen commented Mar 29, 2025

Uh oh!

yezhizhen commented Mar 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrobinson commented Apr 2, 2025

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!

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 Apr 2, 2025 • 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 commented Apr 2, 2025

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!

mrobinson commented Apr 2, 2025

Uh oh!

Rework `ElementAnimationSet::start_transition_if_applicable` to be more like the specification #145

Rework `ElementAnimationSet::start_transition_if_applicable` to be more like the specification #145

yezhizhen commented Mar 14, 2025 •

edited by mrobinson

Loading

yezhizhen commented Mar 14, 2025 •

edited

Loading

yezhizhen commented Mar 17, 2025 •

edited

Loading

yezhizhen commented Mar 17, 2025 •

edited

Loading

mrobinson commented Mar 28, 2025 •

edited

Loading

yezhizhen commented Mar 29, 2025 •

edited

Loading

yezhizhen left a comment •

edited

Loading

yezhizhen Apr 2, 2025 •

edited

Loading

yezhizhen commented Apr 2, 2025 •

edited

Loading