From b92a8553c74b5e9907424cd574a3b29c0da8234a Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Tue, 4 Mar 2025 15:32:06 +0100 Subject: [PATCH] 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 | 2 +- Libraries/LibWeb/DOM/Element.cpp | 68 ++----------------- Libraries/LibWeb/DOM/Element.h | 2 - Libraries/LibWeb/DOM/Node.cpp | 54 +++++++++++++++ Libraries/LibWeb/DOM/Node.h | 3 + ...-element-moves-in-display-none-subtree.txt | 2 + ...element-moves-in-display-none-subtree.html | 59 ++++++++++++++++ 7 files changed, 126 insertions(+), 64 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/WebAnimations/cancel-animation-when-element-moves-in-display-none-subtree.txt create mode 100644 Tests/LibWeb/Text/input/WebAnimations/cancel-animation-when-element-moves-in-display-none-subtree.html diff --git a/Libraries/LibWeb/CSS/StyleComputer.cpp b/Libraries/LibWeb/CSS/StyleComputer.cpp index 1fbe13c0126..0c9f1a32cc9 100644 --- a/Libraries/LibWeb/CSS/StyleComputer.cpp +++ b/Libraries/LibWeb/CSS/StyleComputer.cpp @@ -2605,7 +2605,7 @@ GC::Ref 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(); } diff --git a/Libraries/LibWeb/DOM/Element.cpp b/Libraries/LibWeb/DOM/Element.cpp index a3f202c3f2f..6b719a19e3c 100644 --- a/Libraries/LibWeb/DOM/Element.cpp +++ b/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(Optionalparent_or_shadow_host()) { - if (!ancestor->is_element()) - continue; - auto const& ancestor_element = static_cast(*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 old_display, Optional 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(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(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 style) { - Optional old_display; - Optional 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(); } diff --git a/Libraries/LibWeb/DOM/Element.h b/Libraries/LibWeb/DOM/Element.h index d4ebe619909..4ab4f93be51 100644 --- a/Libraries/LibWeb/DOM/Element.h +++ b/Libraries/LibWeb/DOM/Element.h @@ -209,8 +209,6 @@ public: void set_pseudo_element_computed_properties(CSS::Selector::PseudoElement::Type, GC::Ptr); GC::Ptr pseudo_element_computed_properties(CSS::Selector::PseudoElement::Type); - bool has_display_none_ancestor(); - void play_or_cancel_animations_after_display_property_change(Optional old_display, Optional new_display); void reset_animated_css_properties(); GC::Ptr inline_style() { return m_inline_style; } diff --git a/Libraries/LibWeb/DOM/Node.cpp b/Libraries/LibWeb/DOM/Node.cpp index f5d0b1e2124..8f294010206 100644 --- a/Libraries/LibWeb/DOM/Node.cpp +++ b/Libraries/LibWeb/DOM/Node.cpp @@ -12,8 +12,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -49,6 +51,7 @@ #include #include #include +#include #include #include #include @@ -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(*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(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(i); + if (auto animation = element.cached_animation_name_animation(pseudo_element)) + play_or_cancel_depending_on_display(*animation); + } + return TraversalDecision::Continue; + }); +} + } namespace IPC { diff --git a/Libraries/LibWeb/DOM/Node.h b/Libraries/LibWeb/DOM/Node.h index 34d302a469d..e1e7874db70 100644 --- a/Libraries/LibWeb/DOM/Node.h +++ b/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); diff --git a/Tests/LibWeb/Text/expected/WebAnimations/cancel-animation-when-element-moves-in-display-none-subtree.txt b/Tests/LibWeb/Text/expected/WebAnimations/cancel-animation-when-element-moves-in-display-none-subtree.txt new file mode 100644 index 00000000000..44fb9996483 --- /dev/null +++ b/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 diff --git a/Tests/LibWeb/Text/input/WebAnimations/cancel-animation-when-element-moves-in-display-none-subtree.html b/Tests/LibWeb/Text/input/WebAnimations/cancel-animation-when-element-moves-in-display-none-subtree.html new file mode 100644 index 00000000000..b060475f0c1 --- /dev/null +++ b/Tests/LibWeb/Text/input/WebAnimations/cancel-animation-when-element-moves-in-display-none-subtree.html @@ -0,0 +1,59 @@ + + + + + + + + + +
+
+
+ +
+
+ + + + + + \ No newline at end of file