From c1ab6ca6b4be87cd6c9e36e54beebefcc0bd229b Mon Sep 17 00:00:00 2001 From: Matthew Olsson Date: Sat, 2 Mar 2024 08:54:37 -0700 Subject: [PATCH] LibWeb: Do not invalidate elements with animations in the CSS cascade See the note added to Animation::cancel for more info --- Userland/Libraries/LibWeb/Animations/Animation.cpp | 11 +++++++++-- Userland/Libraries/LibWeb/Animations/Animation.h | 6 +++++- Userland/Libraries/LibWeb/CSS/StyleComputer.cpp | 4 ++-- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/Userland/Libraries/LibWeb/Animations/Animation.cpp b/Userland/Libraries/LibWeb/Animations/Animation.cpp index d91e7002ec..3457ee9407 100644 --- a/Userland/Libraries/LibWeb/Animations/Animation.cpp +++ b/Userland/Libraries/LibWeb/Animations/Animation.cpp @@ -396,8 +396,14 @@ void Animation::set_onremove(JS::GCPtr event_handler) } // https://www.w3.org/TR/web-animations-1/#dom-animation-cancel -void Animation::cancel() +void Animation::cancel(ShouldInvalidate should_invalidate) { + // Note: When called from JS, we always want to invalidate the animation target's style. However, this method is + // also called from the StyleComputer when the animation-name CSS property changes. That happens in the + // middle of a cascade, and importantly, _before_ computing the animation effect stack, so there is no + // need for another invalidation. And in fact, if we did invalidate, it would lead to a crash, as the element + // would not have it's "m_needs_style_update" flag cleared. + auto& realm = this->realm(); // 1. If animation’s play state is not idle, perform the following steps: @@ -452,7 +458,8 @@ void Animation::cancel() // 3. Make animation’s start time unresolved. m_start_time = {}; - invalidate_effect(); + if (should_invalidate == ShouldInvalidate::Yes) + invalidate_effect(); } // https://www.w3.org/TR/web-animations-1/#dom-animation-finish diff --git a/Userland/Libraries/LibWeb/Animations/Animation.h b/Userland/Libraries/LibWeb/Animations/Animation.h index 9f47512de6..0449f282d2 100644 --- a/Userland/Libraries/LibWeb/Animations/Animation.h +++ b/Userland/Libraries/LibWeb/Animations/Animation.h @@ -77,7 +77,11 @@ public: Yes, No, }; - void cancel(); + enum class ShouldInvalidate { + Yes, + No, + }; + void cancel(ShouldInvalidate = ShouldInvalidate::Yes); WebIDL::ExceptionOr finish(); WebIDL::ExceptionOr play(); WebIDL::ExceptionOr play_an_animation(AutoRewind); diff --git a/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp b/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp index f4cb456001..44442142cc 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp +++ b/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp @@ -1395,7 +1395,7 @@ ErrorOr StyleComputer::compute_cascaded_values(StyleProperties& style, DOM if (source_declaration != element.cached_animation_name_source()) { // This animation name is new, so we need to create a new animation for it. if (auto existing_animation = element.cached_animation_name_animation()) - existing_animation->cancel(); + existing_animation->cancel(Animations::Animation::ShouldInvalidate::No); element.set_cached_animation_name_source(source_declaration); auto effect = Animations::KeyframeEffect::create(realm); @@ -1422,7 +1422,7 @@ ErrorOr StyleComputer::compute_cascaded_values(StyleProperties& style, DOM } else { // If the element had an existing animation, cancel it if (auto existing_animation = element.cached_animation_name_animation()) { - existing_animation->cancel(); + existing_animation->cancel(Animations::Animation::ShouldInvalidate::No); element.set_cached_animation_name_animation({}); element.set_cached_animation_name_source({}); }