LibWeb: Cancel animations when element is moved in display none subtree

We already have logic to play or cancel animations in an element's
subtree when the display property changes to or from none. However,
this was not sufficient to cover the case when an element starts/stops
being nested in display none after insertion.

Libraries/LibWeb/CSS/StyleComputer.cpp

@@ -2605,7 +2605,7 @@ GC::Ref<ComputedProperties> StyleComputer::compute_properties(DOM::Element& elem
                 effect->set_target(&element);
                 element.set_cached_animation_name_animation(animation, pseudo_element);
 
-                if (!element.has_display_none_ancestor()) {
+                if (!element.has_inclusive_ancestor_with_display_none()) {
                     HTML::TemporaryExecutionContext context(realm);
                     animation->play().release_value_but_fixme_should_propagate_errors();
                 }

Libraries/LibWeb/DOM/Element.cpp

@@ -534,9 +534,16 @@ CSS::RequiredInvalidationAfterStyleChange Element::recompute_style()
         invalidation = CSS::RequiredInvalidationAfterStyleChange::full();
     }
 
+    auto old_display_is_none = m_computed_properties ? m_computed_properties->display().is_none() : true;
+    auto new_display_is_none = new_computed_properties->display().is_none();
+
     if (!invalidation.is_none())
         set_computed_properties(move(new_computed_properties));
 
+    if (old_display_is_none != new_display_is_none) {
+        play_or_cancel_animations_after_display_property_change();
+    }
+
     // Any document change that can cause this element's style to change, could also affect its pseudo-elements.
     auto recompute_pseudo_element_style = [&](CSS::Selector::PseudoElement::Type pseudo_element) {
         style_computer.push_ancestor(*this);
@@ -2648,69 +2655,8 @@ void Element::set_cascaded_properties(Optional<CSS::Selector::PseudoElement::Typ
     }
 }
 
-bool Element::has_display_none_ancestor()
-{
-    for (auto* ancestor = parent_or_shadow_host(); ancestor; ancestor = ancestor->parent_or_shadow_host()) {
-        if (!ancestor->is_element())
-            continue;
-        auto const& ancestor_element = static_cast<Element&>(*ancestor);
-        if (ancestor_element.computed_properties() && ancestor_element.computed_properties()->display().is_none()) {
-            return true;
-        }
-    }
-    return false;
-}
-
-void Element::play_or_cancel_animations_after_display_property_change(Optional<CSS::Display> old_display, Optional<CSS::Display> new_display)
-{
-    // https://www.w3.org/TR/css-animations-1/#animations
-    // Setting the display property to none will terminate any running animation applied to the element and its descendants.
-    // If an element has a display of none, updating display to a value other than none will start all animations applied to
-    // the element by the animation-name property, as well as all animations applied to descendants with display other than none.
-
-    if (has_display_none_ancestor())
-        return;
-
-    bool previous_display_is_none = old_display.has_value() && old_display->is_none();
-    bool new_display_is_none = new_display.has_value() && new_display->is_none();
-    if (previous_display_is_none == new_display_is_none)
-        return;
-
-    auto play_or_cancel_depending_on_display = [&](Animations::Animation& animation) {
-        if (new_display_is_none) {
-            animation.cancel();
-        } else {
-            HTML::TemporaryExecutionContext context(realm());
-            animation.play().release_value_but_fixme_should_propagate_errors();
-        }
-    };
-
-    for_each_shadow_including_inclusive_descendant([&](auto& node) {
-        if (!node.is_element())
-            return TraversalDecision::Continue;
-
-        auto const& element = static_cast<Element const&>(node);
-        if (auto animation = element.cached_animation_name_animation({}))
-            play_or_cancel_depending_on_display(*animation);
-        for (auto i = 0; i < to_underlying(CSS::Selector::PseudoElement::Type::KnownPseudoElementCount); i++) {
-            auto pseudo_element = static_cast<CSS::Selector::PseudoElement::Type>(i);
-            if (auto animation = element.cached_animation_name_animation(pseudo_element))
-                play_or_cancel_depending_on_display(*animation);
-        }
-        return TraversalDecision::Continue;
-    });
-}
-
 void Element::set_computed_properties(GC::Ptr<CSS::ComputedProperties> style)
 {
-    Optional<CSS::Display> old_display;
-    Optional<CSS::Display> new_display;
-    if (m_computed_properties)
-        old_display = m_computed_properties->display();
-    if (style)
-        new_display = style->display();
-    play_or_cancel_animations_after_display_property_change(old_display, new_display);
-
     m_computed_properties = style;
     computed_properties_changed();
 }

Libraries/LibWeb/DOM/Element.h

@@ -209,8 +209,6 @@ public:
     void set_pseudo_element_computed_properties(CSS::Selector::PseudoElement::Type, GC::Ptr<CSS::ComputedProperties>);
     GC::Ptr<CSS::ComputedProperties> pseudo_element_computed_properties(CSS::Selector::PseudoElement::Type);
 
-    bool has_display_none_ancestor();
-    void play_or_cancel_animations_after_display_property_change(Optional<CSS::Display> old_display, Optional<CSS::Display> new_display);
     void reset_animated_css_properties();
 
     GC::Ptr<CSS::ElementInlineCSSStyleDeclaration> inline_style() { return m_inline_style; }

Libraries/LibWeb/DOM/Node.cpp

@@ -12,8 +12,10 @@
 #include <LibGC/DeferGC.h>
 #include <LibJS/Runtime/FunctionObject.h>
 #include <LibRegex/Regex.h>
+#include <LibWeb/Animations/Animation.h>
 #include <LibWeb/Bindings/MainThreadVM.h>
 #include <LibWeb/Bindings/NodePrototype.h>
+#include <LibWeb/CSS/ComputedProperties.h>
 #include <LibWeb/CSS/StyleComputer.h>
 #include <LibWeb/DOM/Attr.h>
 #include <LibWeb/DOM/CDATASection.h>
@@ -49,6 +51,7 @@
 #include <LibWeb/HTML/Navigable.h>
 #include <LibWeb/HTML/NavigableContainer.h>
 #include <LibWeb/HTML/Parser/HTMLParser.h>
+#include <LibWeb/HTML/Scripting/TemporaryExecutionContext.h>
 #include <LibWeb/Infra/CharacterTypes.h>
 #include <LibWeb/Layout/Node.h>
 #include <LibWeb/Layout/TextNode.h>
@@ -1425,12 +1428,16 @@ void Node::post_connection()
 void Node::inserted()
 {
     set_needs_style_update(true);
+
+    play_or_cancel_animations_after_display_property_change();
 }
 
 void Node::removed_from(Node*, Node&)
 {
     m_layout_node = nullptr;
     m_paintable = nullptr;
+
+    play_or_cancel_animations_after_display_property_change();
 }
 
 ParentNode* Node::parent_or_shadow_host()
@@ -2870,6 +2877,53 @@ void Node::add_registered_observer(RegisteredObserver& registered_observer)
     m_registered_observer_list->append(registered_observer);
 }
 
+bool Node::has_inclusive_ancestor_with_display_none()
+{
+    for (auto* ancestor = this; ancestor; ancestor = ancestor->parent_or_shadow_host()) {
+        if (!ancestor->is_element())
+            continue;
+        auto const& ancestor_element = static_cast<Element&>(*ancestor);
+        if (ancestor_element.computed_properties() && ancestor_element.computed_properties()->display().is_none()) {
+            return true;
+        }
+    }
+    return false;
+}
+
+void Node::play_or_cancel_animations_after_display_property_change()
+{
+    // https://www.w3.org/TR/css-animations-1/#animations
+    // Setting the display property to none will terminate any running animation applied to the element and its descendants.
+    // If an element has a display of none, updating display to a value other than none will start all animations applied to
+    // the element by the animation-name property, as well as all animations applied to descendants with display other than none.
+
+    auto has_display_none_inclusive_ancestor = this->has_inclusive_ancestor_with_display_none();
+
+    auto play_or_cancel_depending_on_display = [&](Animations::Animation& animation) {
+        if (has_display_none_inclusive_ancestor) {
+            animation.cancel();
+        } else {
+            HTML::TemporaryExecutionContext context(realm());
+            animation.play().release_value_but_fixme_should_propagate_errors();
+        }
+    };
+
+    for_each_shadow_including_inclusive_descendant([&](auto& node) {
+        if (!node.is_element())
+            return TraversalDecision::Continue;
+
+        auto const& element = static_cast<Element const&>(node);
+        if (auto animation = element.cached_animation_name_animation({}))
+            play_or_cancel_depending_on_display(*animation);
+        for (auto i = 0; i < to_underlying(CSS::Selector::PseudoElement::Type::KnownPseudoElementCount); i++) {
+            auto pseudo_element = static_cast<CSS::Selector::PseudoElement::Type>(i);
+            if (auto animation = element.cached_animation_name_animation(pseudo_element))
+                play_or_cancel_depending_on_display(*animation);
+        }
+        return TraversalDecision::Continue;
+    });
+}
+
 }
 
 namespace IPC {

Libraries/LibWeb/DOM/Node.h

@@ -502,6 +502,9 @@ public:
 
     bool is_inert() const;
 
+    bool has_inclusive_ancestor_with_display_none();
+    void play_or_cancel_animations_after_display_property_change();
+
 protected:
     Node(JS::Realm&, Document&, NodeType);
     Node(Document&, NodeType);

Tests/LibWeb/Text/expected/WebAnimations/cancel-animation-when-element-moves-in-display-none-subtree.txt

@@ -0,0 +1,2 @@
+Animations count before: 1
+Animations count after: 0

Tests/LibWeb/Text/input/WebAnimations/cancel-animation-when-element-moves-in-display-none-subtree.html

@@ -0,0 +1,59 @@
+<!DOCTYPE html>
+
+<head>
+    <style>
+        .animated-box {
+            width: 50px;
+            height: 50px;
+            background-color: coral;
+            animation: move 2s linear infinite;
+        }
+
+        @keyframes move {
+            0% {
+                transform: translateX(0);
+            }
+
+            50% {
+                transform: translateX(100px);
+            }
+
+            100% {
+                transform: translateX(0);
+            }
+        }
+
+        .hidden-container {
+            display: none;
+        }
+    </style>
+    <script src="../include.js"></script>
+</head>
+
+<body>
+
+    <div id="visible-container">
+        <div class="animated-box" id="animated-box"></div>
+    </div>
+
+    <div id="hidden-container" class="hidden-container">
+    </div>
+
+    <script>
+        asyncTest(done => {
+            requestAnimationFrame(() => {
+                const animatedBox = document.getElementById('animated-box');
+                const hiddenContainer = document.getElementById('hidden-container');
+                const animationsCountBefore = document.getAnimations().length;
+                hiddenContainer.appendChild(animatedBox);
+                const animationsCountAfter = document.getAnimations().length;
+                println('Animations count before: ' + animationsCountBefore);
+                println('Animations count after: ' + animationsCountAfter);
+                done();
+            });
+        });
+    </script>
+
+</body>
+
+</html>