[webkit-changes] [289498] trunk

Wed, 09 Feb 2022 12:48:48 -0800

Title: [289498] trunk
Revision
289498
Author
grao...@webkit.org
Date
2022-02-09 12:48:11 -0800 (Wed, 09 Feb 2022)

Log Message

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.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/imported/w3c/ChangeLog (289497 => 289498)


--- trunk/LayoutTests/imported/w3c/ChangeLog	2022-02-09 20:36:06 UTC (rev 289497)
+++ trunk/LayoutTests/imported/w3c/ChangeLog	2022-02-09 20:48:11 UTC (rev 289498)
@@ -1,3 +1,22 @@
+2022-02-09  Antoine Quint  <grao...@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  <achristen...@webkit.org>
 
         Add TAO check to PerformanceResourceTiming::fetchStart

Added: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation-expected.txt (0 => 289498)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation-expected.txt	2022-02-09 20:48:11 UTC (rev 289498)
@@ -0,0 +1,3 @@
+
+PASS CSS Animations tied to <dialog open> are canceled and restarted as the dialog is hidden and shown
+

Added: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html (0 => 289498)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html	                        (rev 0)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html	2022-02-09 20:48:11 UTC (rev 289498)
@@ -0,0 +1,44 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>CSS Animations on a &lt;dialog></title>
+<link rel="help" href=""
+<script src=""
+<script src=""
+<script src=""
+<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>

Added: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation-expected.txt (0 => 289498)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation-expected.txt	2022-02-09 20:48:11 UTC (rev 289498)
@@ -0,0 +1,3 @@
+
+PASS CSS Animations on a <dialog> ::backdrop are canceled and restarted as the dialog is hidden and shown
+

Added: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation.html (0 => 289498)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation.html	                        (rev 0)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation.html	2022-02-09 20:48:11 UTC (rev 289498)
@@ -0,0 +1,44 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>CSS Animations on a &lt;dialog> ::backdrop</title>
+<link rel="help" href=""
+<script src=""
+<script src=""
+<script src=""
+<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>

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/support/testcommon.js (289497 => 289498)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/support/testcommon.js	2022-02-09 20:36:06 UTC (rev 289497)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/support/testcommon.js	2022-02-09 20:48:11 UTC (rev 289498)
@@ -87,34 +87,46 @@
 }
 
 /**
- * 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);
+}
+
+/**
  * Appends a style div to the document head.
  *
  * @param t  The testharness.js Test object. If provided, this will be used

Modified: trunk/Source/WebCore/ChangeLog (289497 => 289498)


--- trunk/Source/WebCore/ChangeLog	2022-02-09 20:36:06 UTC (rev 289497)
+++ trunk/Source/WebCore/ChangeLog	2022-02-09 20:48:11 UTC (rev 289498)
@@ -1,3 +1,38 @@
+2022-02-09  Antoine Quint  <grao...@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.fu...@sony.com>
 
         compositing/masks/compositing-clip-path-mask-change.html is failing for ports using TextureMapper

Modified: trunk/Source/WebCore/dom/Element.cpp (289497 => 289498)


--- trunk/Source/WebCore/dom/Element.cpp	2022-02-09 20:36:06 UTC (rev 289497)
+++ trunk/Source/WebCore/dom/Element.cpp	2022-02-09 20:48:11 UTC (rev 289498)
@@ -3442,6 +3442,15 @@
         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);

Modified: trunk/Source/WebCore/style/Styleable.cpp (289497 => 289498)


--- trunk/Source/WebCore/style/Styleable.cpp	2022-02-09 20:36:06 UTC (rev 289497)
+++ trunk/Source/WebCore/style/Styleable.cpp	2022-02-09 20:48:11 UTC (rev 289498)
@@ -215,33 +215,6 @@
         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 @@
     }
 }
 
+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 @@
 
     // 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;
     }
 

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to