LibWeb: Don't allow "display: none" start CSS animations

This is both a correctness fix and a performance optimization.

Libraries/LibWeb/CSS/StyleComputer.cpp

@@ -2502,8 +2502,10 @@ GC::Ref<ComputedProperties> StyleComputer::compute_properties(DOM::Element& elem
                 effect->set_target(&element);
                 element.set_cached_animation_name_animation(animation, pseudo_element);
 
-                HTML::TemporaryExecutionContext context(realm);
-                animation->play().release_value_but_fixme_should_propagate_errors();
+                if (!element.has_display_none_ancestor()) {
+                    HTML::TemporaryExecutionContext context(realm);
+                    animation->play().release_value_but_fixme_should_propagate_errors();
+                }
             } else {
                 // The animation hasn't changed, but some properties of the animation may have
                 if (auto animation = element.cached_animation_name_animation(pseudo_element); animation)

Libraries/LibWeb/DOM/Element.cpp

@@ -10,6 +10,7 @@
 #include <AK/StringBuilder.h>
 #include <LibUnicode/CharacterTypes.h>
 #include <LibUnicode/Locale.h>
+#include <LibWeb/Animations/Animation.h>
 #include <LibWeb/Bindings/ElementPrototype.h>
 #include <LibWeb/Bindings/ExceptionOrUtils.h>
 #include <LibWeb/Bindings/MainThreadVM.h>
@@ -20,6 +21,7 @@
 #include <LibWeb/CSS/SelectorEngine.h>
 #include <LibWeb/CSS/StyleComputer.h>
 #include <LibWeb/CSS/StyleValues/CSSKeywordValue.h>
+#include <LibWeb/CSS/StyleValues/DisplayStyleValue.h>
 #include <LibWeb/CSS/StyleValues/LengthStyleValue.h>
 #include <LibWeb/CSS/StyleValues/NumberStyleValue.h>
 #include <LibWeb/DOM/Attr.h>
@@ -56,6 +58,7 @@
 #include <LibWeb/HTML/Numbers.h>
 #include <LibWeb/HTML/Parser/HTMLParser.h>
 #include <LibWeb/HTML/Scripting/Agent.h>
+#include <LibWeb/HTML/Scripting/TemporaryExecutionContext.h>
 #include <LibWeb/HTML/TraversableNavigable.h>
 #include <LibWeb/HTML/Window.h>
 #include <LibWeb/Infra/CharacterTypes.h>
@@ -2459,8 +2462,69 @@ 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,6 +209,8 @@ 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; }

Tests/LibWeb/Text/expected/wpt-import/css/css-animations/display-none-prevents-starting-in-subtree.txt

@@ -0,0 +1,6 @@
+Harness status: OK
+
+Found 1 tests
+
+1 Pass
+Pass	Elements in a "display: none" tree cannot start CSS Animations.

Tests/LibWeb/Text/expected/wpt-import/css/css-display/animations/display-interpolation.txt

@@ -2,8 +2,8 @@ Harness status: OK
 
 Found 42 tests
 
-28 Pass
-14 Fail
+31 Pass
+11 Fail
 Pass	CSS Transitions with transition-behavior:allow-discrete: property <display> from [none] to [flex] at (-0.3) should be [flex]
 Pass	CSS Transitions with transition-behavior:allow-discrete: property <display> from [none] to [flex] at (0) should be [flex]
 Pass	CSS Transitions with transition-behavior:allow-discrete: property <display> from [none] to [flex] at (0.3) should be [flex]
@@ -32,9 +32,9 @@ Pass	CSS Transitions with transition: all: property <display> from [none] to [fl
 Pass	CSS Transitions with transition: all: property <display> from [none] to [flex] at (0.6) should be [flex]
 Pass	CSS Transitions with transition: all: property <display> from [none] to [flex] at (1) should be [flex]
 Pass	CSS Transitions with transition: all: property <display> from [none] to [flex] at (1.5) should be [flex]
-Fail	CSS Animations: property <display> from [none] to [flex] at (-0.3) should be [block]
-Fail	CSS Animations: property <display> from [none] to [flex] at (0) should be [block]
-Fail	CSS Animations: property <display> from [none] to [flex] at (0.3) should be [block]
+Pass	CSS Animations: property <display> from [none] to [flex] at (-0.3) should be [block]
+Pass	CSS Animations: property <display> from [none] to [flex] at (0) should be [block]
+Pass	CSS Animations: property <display> from [none] to [flex] at (0.3) should be [block]
 Fail	CSS Animations: property <display> from [none] to [flex] at (0.5) should be [block]
 Fail	CSS Animations: property <display> from [none] to [flex] at (0.6) should be [block]
 Fail	CSS Animations: property <display> from [none] to [flex] at (1) should be [block]

Tests/LibWeb/Text/input/wpt-import/css/css-animations/display-none-prevents-starting-in-subtree.html

@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<link rel=author href="mailto:graouts@webkit.org">
+<link rel=help href="https://drafts.csswg.org/css-animations/#animations">
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script src="../../css/css-animations/support/testcommon.js"></script>
+<style>
+@keyframes margin {
+    100% { margin-left: 200px }
+}
+
+#child {
+  animation: margin 1s forwards;
+}
+</style>
+<div id="container">
+    <div>
+        <div id="child"></div>
+    </div>
+</div>
+<script>
+test(() => {
+  const container = document.getElementById("container");
+  const animationCount = () => container.getAnimations({ subtree: true }).length;
+
+  assert_equals(animationCount(), 1, `An animation is running on the child initially with "display: block" on the container.`);
+
+  container.style.display = "none";
+  assert_equals(animationCount(), 0, `Setting "display: none" on the container canceled the animation.`);
+
+  container.style.marginLeft = "50px";
+  container.firstElementChild.style.marginLeft = "100px";
+  assert_equals(animationCount(), 0, `Manipulating styles on the container and a child element does not restart the animation.`);
+}, 'Elements in a "display: none" tree cannot start CSS Animations.');
+</script>