Dialog element only animates once

https://bugs.webkit.org/show_bug.cgi?id=236274 rdar://88635487 Reviewed by Tim Nguyen and Dean Jackson. LayoutTests/imported/w3c: Add two new tests that check that we correctly start, stop and resume animations on <dialog> and ::backdrop as a <dialog> element is open, closed and open again. * web-platform-tests/css/css-animations/dialog-animation-expected.txt: Added. * web-platform-tests/css/css-animations/dialog-animation.html: Added. * web-platform-tests/css/css-animations/dialog-backdrop-animation-expected.txt: Added. * web-platform-tests/css/css-animations/dialog-backdrop-animation.html: Added. * web-platform-tests/css/css-animations/support/testcommon.js: (addElement): (addDiv): Source/WebCore: Two issues related to CSS Animation surfaced in this bug which animates both <dialog> and its ::backdrop as the dialog is open and eventually re-opened. The first issue was that we didn't clear all CSS Animations state when a <dialog> was closed and its style was set to `display: none`. We now use the clearCSSAnimationsForStyleable() function (static so that it's not exposed on the Styleable struct) to correctly clear such state both when we identify a Styleable is newly getting `display: none` and when cancelDeclarativeAnimations() was called. This allows us to remove removeCSSAnimationCreatedByMarkup() a fair bit of work to clear CSS Animation state per-animation when we only ever used that function for _all_ animations. The second issue was that we never called cancelDeclarativeAnimations() for ::backdrop. We now do that inside of Element::removeFromTopLayer() at a point where the code in Styleable::fromRenderer() will still work as the element will still be contained in Document::topLayerElements(). Tests: imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation.html * dom/Element.cpp: (WebCore::Element::removeFromTopLayer): * style/Styleable.cpp: (WebCore::clearCSSAnimationsForStyleable): (WebCore::Styleable::cancelDeclarativeAnimations const): (WebCore::Styleable::updateCSSAnimations const): (WebCore::removeCSSAnimationCreatedByMarkup): Deleted. Canonical link: https://commits.webkit.org/247036@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@289498 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Feb 9, 2022 · 2c54c794ba7d185af4564614996805db9a2a0020 · 2c54c79
1 parent 4af5702
commit 2c54c794ba7d185af4564614996805db9a2a0020
Show file tree

Hide file tree

Showing 9 changed files with 193 additions and 45 deletions.
diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
@@ -1,3 +1,22 @@
+2022-02-09  Antoine Quint  <graouts@webkit.org>
+
+        Dialog element only animates once
+        https://bugs.webkit.org/show_bug.cgi?id=236274
+        rdar://88635487
+
+        Reviewed by Tim Nguyen and Dean Jackson.
+
+        Add two new tests that check that we correctly start, stop and resume animations on <dialog>
+        and ::backdrop as a <dialog> element is open, closed and open again.
+
+        * web-platform-tests/css/css-animations/dialog-animation-expected.txt: Added.
+        * web-platform-tests/css/css-animations/dialog-animation.html: Added.
+        * web-platform-tests/css/css-animations/dialog-backdrop-animation-expected.txt: Added.
+        * web-platform-tests/css/css-animations/dialog-backdrop-animation.html: Added.
+        * web-platform-tests/css/css-animations/support/testcommon.js:
+        (addElement):
+        (addDiv):
+
 2022-02-09  Alex Christensen  <achristensen@webkit.org>
 
         Add TAO check to PerformanceResourceTiming::fetchStart

diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation-expected.txt
@@ -0,0 +1,3 @@
+
+PASS CSS Animations tied to <dialog open> are canceled and restarted as the dialog is hidden and shown
+
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html
@@ -0,0 +1,44 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>CSS Animations on a &lt;dialog></title>
+<link rel="help" href="https://drafts.csswg.org/css-animations-1/">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="support/testcommon.js"></script>
+<style>
+
+dialog[open] {
+    animation: dialog-open-animation 1ms;
+}
+
+@keyframes dialog-open-animation {
+    from { opacity: 0 }
+}
+
+</style>
+<div id="log"></div>
+<script>
+
+"use strict";
+
+promise_test(async t => {
+  const dialog = addElement(t, "dialog");
+
+  // Open the dialog a first time, this should trigger a CSS Animation.
+  dialog.open = true;
+  const animations = dialog.getAnimations();
+  assert_equals(animations.length, 1, "As the <dialog> is shown intially an animation is started");
+
+  await animations[0].finished;
+
+  await waitForNextFrame();
+
+  dialog.open = false;
+  assert_equals(dialog.getAnimations().length, 0, "As the <dialog> is closed the animation is removed");
+
+  await waitForNextFrame();
+
+  dialog.open = true;
+  assert_equals(dialog.getAnimations().length, 1, "As the <dialog> is shown again an animation is started again");
+}, "CSS Animations tied to <dialog open> are canceled and restarted as the dialog is hidden and shown");
+</script>
diff --git a/...imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation-expected.txt b/...imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation-expected.txt
@@ -0,0 +1,3 @@
+
+PASS CSS Animations on a <dialog> ::backdrop are canceled and restarted as the dialog is hidden and shown
+
diff --git a/...utTests/imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation.html b/...utTests/imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation.html
@@ -0,0 +1,44 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>CSS Animations on a &lt;dialog> ::backdrop</title>
+<link rel="help" href="https://drafts.csswg.org/css-animations-1/">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="support/testcommon.js"></script>
+<style>
+
+dialog[open]::backdrop {
+    animation: dialog-backdrop-animation 1ms;
+}
+
+@keyframes dialog-backdrop-animation {
+    from { opacity: 0 }
+}
+
+</style>
+<div id="log"></div>
+<script>
+
+"use strict";
+
+promise_test(async t => {
+  const dialog = addElement(t, "dialog");
+
+  // Open the dialog a first time, this should trigger a CSS Animation.
+  dialog.showModal();
+  const animations = dialog.getAnimations({ subtree: true });
+  assert_equals(animations.length, 1, "As the <dialog> is shown intially an animation is started on its ::backdrop");
+
+  await animations[0].finished;
+
+  await waitForNextFrame();
+
+  dialog.close();
+  assert_equals(dialog.getAnimations({ subtree: true }).length, 0, "As the <dialog> is closed the animation is removed from its ::backdrop");
+
+  await waitForNextFrame();
+
+  dialog.showModal();
+  assert_equals(dialog.getAnimations({ subtree: true }).length, 1, "As the <dialog> is shown again an animation is started again on its ::backdrop");
+}, "CSS Animations on a <dialog> ::backdrop are canceled and restarted as the dialog is hidden and shown");
+</script>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/support/testcommon.js b/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/support/testcommon.js
@@ -87,31 +87,43 @@ function assert_frame_lists_equal(actual, expected) {
 }
 
 /**
- * Appends a div to the document body.
+ * Appends an element to the document body.
  *
  * @param t  The testharness.js Test object. If provided, this will be used
  *           to register a cleanup callback to remove the div when the test
  *           finishes.
  *
+ * @param name  A string specifying the element name.
+ *
  * @param attrs  A dictionary object with attribute names and values to set on
  *               the div.
  */
-function addDiv(t, attrs) {
-  var div = document.createElement('div');
+function addElement(t, name, attrs) {
+  var element = document.createElement(name);
   if (attrs) {
     for (var attrName in attrs) {
-      div.setAttribute(attrName, attrs[attrName]);
+      element.setAttribute(attrName, attrs[attrName]);
     }
   }
-  document.body.appendChild(div);
+  document.body.appendChild(element);
   if (t && typeof t.add_cleanup === 'function') {
-    t.add_cleanup(function() {
-      if (div.parentNode) {
-        div.remove();
-      }
-    });
+      t.add_cleanup(() => element.remove());
   }
-  return div;
+  return element;
+}
+
+/**
+ * Appends a div to the document body.
+ *
+ * @param t  The testharness.js Test object. If provided, this will be used
+ *           to register a cleanup callback to remove the div when the test
+ *           finishes.
+ *
+ * @param attrs  A dictionary object with attribute names and values to set on
+ *               the div.
+ */
+function addDiv(t, attrs) {
+  return addElement(t, "div", attrs);
 }
 
 /**

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,38 @@
+2022-02-09  Antoine Quint  <graouts@webkit.org>
+
+        Dialog element only animates once
+        https://bugs.webkit.org/show_bug.cgi?id=236274
+        rdar://88635487
+
+        Reviewed by Tim Nguyen and Dean Jackson.
+
+        Two issues related to CSS Animation surfaced in this bug which animates both <dialog>
+        and its ::backdrop as the dialog is open and eventually re-opened.
+
+        The first issue was that we didn't clear all CSS Animations state when a <dialog> was
+        closed and its style was set to `display: none`. We now use the clearCSSAnimationsForStyleable()
+        function (static so that it's not exposed on the Styleable struct) to correctly clear
+        such state both when we identify a Styleable is newly getting `display: none` and when
+        cancelDeclarativeAnimations() was called. This allows us to remove removeCSSAnimationCreatedByMarkup()
+        a fair bit of work to clear CSS Animation state per-animation when we only ever used that
+        function for _all_ animations.
+
+        The second issue was that we never called cancelDeclarativeAnimations() for ::backdrop.
+        We now do that inside of Element::removeFromTopLayer() at a point where the code in
+        Styleable::fromRenderer() will still work as the element will still be contained in
+        Document::topLayerElements().
+
+        Tests: imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html
+               imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::removeFromTopLayer):
+        * style/Styleable.cpp:
+        (WebCore::clearCSSAnimationsForStyleable):
+        (WebCore::Styleable::cancelDeclarativeAnimations const):
+        (WebCore::Styleable::updateCSSAnimations const):
+        (WebCore::removeCSSAnimationCreatedByMarkup): Deleted.
+
 2022-02-09  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         compositing/masks/compositing-clip-path-mask-change.html is failing for ports using TextureMapper

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
@@ -3442,6 +3442,15 @@ void Element::removeFromTopLayer()
         layer.establishesTopLayerWillChange();
     });
 
+    // We need to call Styleable::fromRenderer() while this document is still contained
+    // in Document::topLayerElements().
+    if (auto* renderer = this->renderer()) {
+        if (auto backdrop = renderer->backdropRenderer()) {
+            if (auto styleable = Styleable::fromRenderer(*backdrop))
+                styleable->cancelDeclarativeAnimations();
+        }
+    }
+
     document().removeTopLayerElement(*this);
     clearNodeFlag(NodeFlag::IsInTopLayer);
 

diff --git a/Source/WebCore/style/Styleable.cpp b/Source/WebCore/style/Styleable.cpp
@@ -215,33 +215,6 @@ void Styleable::animationWasRemoved(WebAnimation& animation) const
         removeDeclarativeAnimationFromListsForOwningElement(animation);
 }
 
-static void removeCSSAnimationCreatedByMarkup(const Styleable& styleable, CSSAnimation& cssAnimation)
-{
-    styleable.animationsCreatedByMarkup().remove(&cssAnimation);
-
-    if (!styleable.hasKeyframeEffects())
-        return;
-
-    auto& keyframeEffectStack = styleable.ensureKeyframeEffectStack();
-    auto* cssAnimationList = keyframeEffectStack.cssAnimationList();
-    if (!cssAnimationList || cssAnimationList->isEmpty())
-        return;
-
-    auto& backingAnimation = cssAnimation.backingAnimation();
-    for (size_t i = 0; i < cssAnimationList->size(); ++i) {
-        if (cssAnimationList->animation(i) == backingAnimation) {
-            // It is important we do not make a clone of the Animation references contained
-            // within cssAnimationList since sorting animations in compareCSSAnimations()
-            // makes pointer comparisons to distinguish between backing animations of various
-            // CSSAnimation objects.
-            auto newAnimationList = cssAnimationList->shallowCopy();
-            newAnimationList->remove(i);
-            keyframeEffectStack.setCSSAnimationList(WTFMove(newAnimationList));
-            return;
-        }
-    }
-}
-
 void Styleable::elementWasRemoved() const
 {
     cancelDeclarativeAnimations();
@@ -255,17 +228,25 @@ void Styleable::willChangeRenderer() const
     }
 }
 
+static void clearCSSAnimationsForStyleable(const Styleable& styleable)
+{
+    for (auto& cssAnimation : styleable.animationsCreatedByMarkup())
+        cssAnimation->cancelFromStyle();
+    if (auto* effectStack = styleable.keyframeEffectStack())
+        effectStack->setCSSAnimationList(nullptr);
+    styleable.setAnimationsCreatedByMarkup({ });
+}
+
 void Styleable::cancelDeclarativeAnimations() const
 {
     if (auto* animations = this->animations()) {
         for (auto& animation : *animations) {
-            if (is<DeclarativeAnimation>(animation)) {
-                if (is<CSSAnimation>(animation))
-                    removeCSSAnimationCreatedByMarkup(*this, downcast<CSSAnimation>(*animation));
+            if (is<DeclarativeAnimation>(animation))
                 downcast<DeclarativeAnimation>(*animation).cancelFromStyle();
-            }
         }
     }
+
+    clearCSSAnimationsForStyleable(*this);
 }
 
 static bool keyframesRuleExistsForAnimation(Element& element, const Animation& animation, const String& animationName)
@@ -295,9 +276,7 @@ void Styleable::updateCSSAnimations(const RenderStyle* currentStyle, const Rende
 
     // In case this element is newly getting a "display: none" we need to cancel all of its animations and disregard new ones.
     if (currentStyle && currentStyle->display() != DisplayType::None && newStyle.display() == DisplayType::None) {
-        for (auto& cssAnimation : animationsCreatedByMarkup())
-            cssAnimation->cancelFromStyle();
-        keyframeEffectStack.setCSSAnimationList(nullptr);
+        clearCSSAnimationsForStyleable(*this);
         return;
     }