Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The `Idle` state is not always reached at the end of css transitions #20379

Closed
fabricedesre opened this issue Mar 21, 2018 · 9 comments
Closed

The `Idle` state is not always reached at the end of css transitions #20379

fabricedesre opened this issue Mar 21, 2018 · 9 comments

Comments

@fabricedesre
Copy link
Contributor

@fabricedesre fabricedesre commented Mar 21, 2018

When a css transition is ongoing, the embedder is notified by the https://doc.servo.org/compositing/windowing/trait.WindowMethods.html#method.set_animation_state method with state value of Animating, and then back with the Idle value once the animations are done.

I have a couple of cases where we don't go back to the Idle state after a transition, and instead keep hammering the embedder with Animating notifications. This may not be too much noticeable on desktop, but it can be pretty bad on some mobile devices (and it's wrong anyway).

Just filing for now until I manage to build a simple enough test case to reproduce.

@fabricedesre
Copy link
Contributor Author

@fabricedesre fabricedesre commented Mar 22, 2018

Here's a test case:

<!DOCTYPE html>
<html>

<head>
    <style>
        div {
            transition: transform 0.5s;
            position: absolute;
            top: 0;
            left: 0;
            width: 200px;
            height: 200px;
            background-color: teal;
        }

        div.moved {
            transform: translate(150px, 150px);
        }
    </style>
    <script>
        document.addEventListener("DOMContentLoaded", () => {
            window.addEventListener("click", () => {
                document.getElementById("box").classList.toggle("moved");
            });
        });
    </script>
</head>

<body>
    <div id="box"></div>
</body>

</html>

if you apply this patch:

diff --git a/ports/servo/glutin_app/window.rs b/ports/servo/glutin_app/window.rs
index b6ef3e2e3b..0cd6cfcc61 100644
--- a/ports/servo/glutin_app/window.rs
+++ b/ports/servo/glutin_app/window.rs
@@ -905,6 +905,7 @@ impl WindowMethods for Window {
     }
 
     fn set_animation_state(&self, state: AnimationState) {
+        println!("set_animation_state {:?}", state);
         self.animation_state.set(state);
     }

and click a couple of times to get the box back to (0, 0) you will see that set_animation_state Animating is printed constantly.

In components/layout/animation.rs, update_animation_state reinjects a new animation from new_animations_receiver at the end of the expected one, over and over in an endless loop. That stops when we switch back to the .moved state.

@fabricedesre
Copy link
Contributor Author

@fabricedesre fabricedesre commented Mar 22, 2018

Maybe related to issue #20116

@emilio
Copy link
Member

@emilio emilio commented May 5, 2018

#14418 didn't make sense. See the mark_as_expired bit there going away.

@emilio
Copy link
Member

@emilio emilio commented May 5, 2018

Our transitions story in general makes no sense, apparently.

@fabricedesre
Copy link
Contributor Author

@fabricedesre fabricedesre commented May 6, 2018

😢 @emilio do you have time to spend on fixing that?

@emilio
Copy link
Member

@emilio emilio commented May 7, 2018

I fixed animations in #20757, but not really, only my free time...

@fabricedesre fabricedesre mentioned this issue May 19, 2018
1 of 9 tasks complete
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jun 4, 2018

On a transition, are we supposed to wakeup the embedder loop on each frame? It looks like we do, but I thought that the point of setAnimationState(animating) was to avoid having to do so.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Dec 16, 2019

Dupe. STR from #25262

<style>
  div {
    width: 200px;
    height: 200px;
    margin:200px;
    background: red;
    transition: 200ms linear transform;
  }
  div:hover {
    transform:scale(1.2);
  }
</style>
<div></div>
  • mouse hover div
  • animating then not animating (good)
  • move mouse outside of the div
  • animating is sent (good) but NoAnimationsPresent is never sent after the animation is finished
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Dec 16, 2019

After animation is over, it is stopped, and ran again, and stopped, and ran again…

After each animation, the animation sender resend an animation. Basically after the animation is over, this is called again:

/// Inserts transitions into the queue of running animations as applicable for

@jdm jdm mentioned this issue Mar 31, 2020
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Mar 31, 2020
Avoid infinitely looping CSS transitions.

This change addresses the long-standing issue of CSS transitions not ending appropriately. It does not fundamentally change the way we process transitions/animations.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20379
- [x] There are tests for these changes
@jdm jdm self-assigned this Mar 31, 2020
bors-servo added a commit that referenced this issue Apr 1, 2020
Avoid infinitely looping CSS transitions.

This change addresses the long-standing issue of CSS transitions not ending appropriately. It does not fundamentally change the way we process transitions/animations.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20379
- [x] There are tests for these changes
bors-servo added a commit that referenced this issue Apr 1, 2020
Avoid infinitely looping CSS transitions.

This change addresses the long-standing issue of CSS transitions not ending appropriately. It does not fundamentally change the way we process transitions/animations.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20379
- [x] There are tests for these changes
@bors-servo bors-servo closed this in 516279e Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.