LibWeb: Stop spamming animation events on the wrong event target

This patch fixes two issues: - Animation events that should go to the target element now do (some were previously being dispatched on the animation itself.) - We update the "previous phase" and "previous iteration" fields of animation effects, so that we can actually detect phase changes. This means we stop thinking animations always just started, something that caused each animation to send 60 animationstart events every second (to the wrong target!)
awesomekling · May 23, 2024 · c3c3c7a · c3c3c7a
1 parent f29ac85
commit c3c3c7a
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 11 deletions.
diff --git a/Tests/LibWeb/Text/expected/WebAnimations/misc/animation-events-basic.txt b/Tests/LibWeb/Text/expected/WebAnimations/misc/animation-events-basic.txt
@@ -0,0 +1,2 @@
+    animationstart
+animationend
diff --git a/Tests/LibWeb/Text/input/WebAnimations/misc/animation-events-basic.html b/Tests/LibWeb/Text/input/WebAnimations/misc/animation-events-basic.html
@@ -0,0 +1,46 @@
+<!doctype html><style>
+        body {
+            margin: 0;
+            display: flex;
+            justify-content: center;
+            align-items: center;
+            height: 100vh;
+            background-color: #f0f0f0;
+        }
+
+        div {
+            width: 100px;
+            height: 100px;
+            background-color: #3498db;
+            position: relative;
+            animation: moveRight 0.1s linear;
+            animation-play-state: paused;
+        }
+
+        @keyframes moveRight {
+            0% {
+                left: 0;
+            }
+            100% {
+                left: 100px;
+            }
+        }
+</style>
+<body>
+<script src="../../include.js"></script>
+<div id="d"></div>
+<script>
+    asyncTest(done => {
+        let div = document.getElementById("d");
+        div.addEventListener("animationstart", () => {
+            println("animationstart");
+        });
+        div.addEventListener("animationend", () => {
+            println("animationend");
+            div.style.animationPlayState = "paused";
+            div.style.display = "none";
+            done();
+        });
+    });
+</script>
+</body>
diff --git a/Userland/Libraries/LibWeb/Animations/Animation.cpp b/Userland/Libraries/LibWeb/Animations/Animation.cpp
@@ -456,7 +456,7 @@ void Animation::cancel(ShouldInvalidate should_invalidate)
             Optional<double> scheduled_event_time;
             if (m_timeline && !m_timeline->is_inactive() && m_timeline->can_convert_a_timeline_time_to_an_origin_relative_time())
                 scheduled_event_time = m_timeline->convert_a_timeline_time_to_an_origin_relative_time(m_timeline->current_time());
-            document->append_pending_animation_event({ cancel_event, *this, scheduled_event_time });
+            document->append_pending_animation_event({ cancel_event, *this, *this, scheduled_event_time });
         } else {
             HTML::queue_global_task(HTML::Task::Source::DOMManipulation, realm.global_object(), JS::create_heap_function(heap(), [this, cancel_event]() {
                 dispatch_event(cancel_event);
@@ -1127,9 +1127,12 @@ void Animation::update_finished_state(DidSeek did_seek, SynchronouslyNotify sync
             //    animation event queue along with its target, animation. For the scheduled event time, use the result
             //    of converting animation’s associated effect end to an origin-relative time.
             if (auto document_for_timing = this->document_for_timing()) {
-                document_for_timing->append_pending_animation_event({ .event = finish_event,
+                document_for_timing->append_pending_animation_event({
+                    .event = finish_event,
+                    .animation = *this,
                     .target = *this,
-                    .scheduled_event_time = convert_a_timeline_time_to_an_origin_relative_time(associated_effect_end()) });
+                    .scheduled_event_time = convert_a_timeline_time_to_an_origin_relative_time(associated_effect_end()),
+                });
             }
             //    Otherwise, queue a task to dispatch finishEvent at animation. The task source for this task is the DOM
             //    manipulation task source.

diff --git a/Userland/Libraries/LibWeb/DOM/Document.cpp b/Userland/Libraries/LibWeb/DOM/Document.cpp
@@ -2017,7 +2017,12 @@ void Document::dispatch_events_for_animation_if_necessary(JS::NonnullGCPtr<Anima
         return;
 
     auto& css_animation = verify_cast<CSS::CSSAnimation>(*animation);
-    if (auto target = effect->target(); target && target->paintable())
+
+    JS::GCPtr<Element> target = effect->target();
+    if (!target)
+        return;
+
+    if (target->paintable())
         target->paintable()->set_needs_display();
 
     auto previous_phase = effect->previous_phase();
@@ -2037,7 +2042,8 @@ void Document::dispatch_events_for_animation_if_necessary(JS::NonnullGCPtr<Anima
                         css_animation.id(),
                         elapsed_time,
                     }),
-                .target = animation,
+                .animation = css_animation,
+                .target = *target,
                 .scheduled_event_time = HighResolutionTime::unsafe_shared_current_time(),
             });
         };
@@ -2101,6 +2107,8 @@ void Document::dispatch_events_for_animation_if_necessary(JS::NonnullGCPtr<Anima
             dispatch_event(HTML::EventNames::animationcancel, effect->active_time_using_fill(Bindings::FillMode::Both).value());
         }
     }
+    effect->set_previous_phase(current_phase);
+    effect->set_previous_current_iteration(current_iteration);
 }
 
 // https://html.spec.whatwg.org/multipage/browsing-the-web.html#scroll-to-the-fragment-identifier
@@ -4238,8 +4246,8 @@ void Document::update_animations_and_send_events(Optional<double> const& timesta
 
     // 6. Perform a stable sort of the animation events in events to dispatch as follows:
     auto sort_events_by_composite_order = [](auto const& a, auto const& b) {
-        auto& a_effect = verify_cast<Animations::KeyframeEffect>(*a.target->effect());
-        auto& b_effect = verify_cast<Animations::KeyframeEffect>(*b.target->effect());
+        auto& a_effect = verify_cast<Animations::KeyframeEffect>(*a.animation->effect());
+        auto& b_effect = verify_cast<Animations::KeyframeEffect>(*b.animation->effect());
         return Animations::KeyframeEffect::composite_order(a_effect, b_effect) < 0;
     };
 
@@ -4352,9 +4360,10 @@ void Document::remove_replaced_animations()
             //   timeline with which animation is associated.
             if (auto document = animation->document_for_timing()) {
                 PendingAnimationEvent pending_animation_event {
-                    remove_event,
-                    animation,
-                    animation->timeline()->convert_a_timeline_time_to_an_origin_relative_time(init.timeline_time),
+                    .event = remove_event,
+                    .animation = animation,
+                    .target = animation,
+                    .scheduled_event_time = animation->timeline()->convert_a_timeline_time_to_an_origin_relative_time(init.timeline_time),
                 };
                 document->append_pending_animation_event(pending_animation_event);
             }

diff --git a/Userland/Libraries/LibWeb/DOM/Document.h b/Userland/Libraries/LibWeb/DOM/Document.h
@@ -599,7 +599,8 @@ class Document
 
     struct PendingAnimationEvent {
         JS::NonnullGCPtr<DOM::Event> event;
-        JS::NonnullGCPtr<Animations::Animation> target;
+        JS::NonnullGCPtr<Animations::Animation> animation;
+        JS::NonnullGCPtr<DOM::EventTarget> target;
         Optional<double> scheduled_event_time;
     };
     void append_pending_animation_event(PendingAnimationEvent const&);