- Revision
- 291282
- Author
- [email protected]
- Date
- 2022-03-15 04:40:00 -0700 (Tue, 15 Mar 2022)
Log Message
Dialog element only animates once
https://bugs.webkit.org/show_bug.cgi?id=236274
Reviewed by Dean Jackson, Tim Nguyen and Antti Koivisto.
LayoutTests/imported/w3c:
Import relevant WPT tests that had already been upstreamed in a previous, reverted
version of this patch.
* 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 call setAnimationsCreatedByMarkup
to correctly clear such state both when we identify a Styleable is newly getting
`display: none`. We do the same when cancelDeclarativeAnimations() is called, but also
call setCSSAnimationList() on the associated effect stack since that wasn't done either.
Now both functions do similar cleanup.
This allows us to remove removeCSSAnimationCreatedByMarkup() which did 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::Styleable::cancelDeclarativeAnimations const):
(WebCore::Styleable::updateCSSAnimations const):
(WebCore::removeCSSAnimationCreatedByMarkup): Deleted.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (291281 => 291282)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2022-03-15 11:01:41 UTC (rev 291281)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2022-03-15 11:40:00 UTC (rev 291282)
@@ -1,3 +1,21 @@
+2022-03-15 Antoine Quint <[email protected]>
+
+ Dialog element only animates once
+ https://bugs.webkit.org/show_bug.cgi?id=236274
+
+ Reviewed by Dean Jackson, Tim Nguyen and Antti Koivisto.
+
+ Import relevant WPT tests that had already been upstreamed in a previous, reverted
+ version of this patch.
+
+ * 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-03-14 Oriol Brufau <[email protected]>
[css-cascade] Fix 'revert' on low-priority properties
Added: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation-expected.txt (0 => 291282)
--- 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-03-15 11:40:00 UTC (rev 291282)
@@ -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 => 291282)
--- 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-03-15 11:40:00 UTC (rev 291282)
@@ -0,0 +1,44 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>CSS Animations on a <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 => 291282)
--- 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-03-15 11:40:00 UTC (rev 291282)
@@ -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 => 291282)
--- 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-03-15 11:40:00 UTC (rev 291282)
@@ -0,0 +1,44 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>CSS Animations on a <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 (291281 => 291282)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/support/testcommon.js 2022-03-15 11:01:41 UTC (rev 291281)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/support/testcommon.js 2022-03-15 11:40:00 UTC (rev 291282)
@@ -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 (291281 => 291282)
--- trunk/Source/WebCore/ChangeLog 2022-03-15 11:01:41 UTC (rev 291281)
+++ trunk/Source/WebCore/ChangeLog 2022-03-15 11:40:00 UTC (rev 291282)
@@ -1,3 +1,40 @@
+2022-03-15 Antoine Quint <[email protected]>
+
+ Dialog element only animates once
+ https://bugs.webkit.org/show_bug.cgi?id=236274
+
+ Reviewed by Dean Jackson, Tim Nguyen and Antti Koivisto.
+
+ 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 call setAnimationsCreatedByMarkup
+ to correctly clear such state both when we identify a Styleable is newly getting
+ `display: none`. We do the same when cancelDeclarativeAnimations() is called, but also
+ call setCSSAnimationList() on the associated effect stack since that wasn't done either.
+ Now both functions do similar cleanup.
+
+
+ This allows us to remove removeCSSAnimationCreatedByMarkup() which did 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::Styleable::cancelDeclarativeAnimations const):
+ (WebCore::Styleable::updateCSSAnimations const):
+ (WebCore::removeCSSAnimationCreatedByMarkup): Deleted.
+
2022-03-15 Gabriel Nava Marino <[email protected]>
Crash in KeyframeList.cpp:183 in WebCore::KeyframeList::fillImplicitKeyframes
Modified: trunk/Source/WebCore/dom/Element.cpp (291281 => 291282)
--- trunk/Source/WebCore/dom/Element.cpp 2022-03-15 11:01:41 UTC (rev 291281)
+++ trunk/Source/WebCore/dom/Element.cpp 2022-03-15 11:40:00 UTC (rev 291282)
@@ -3445,6 +3445,16 @@
layer.establishesTopLayerWillChange();
});
+ // We need to call Styleable::fromRenderer() while this element is still contained in
+ // Document::topLayerElements(), since Styleable::fromRenderer() relies on this to
+ // find the backdrop's associated element.
+ 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 (291281 => 291282)
--- trunk/Source/WebCore/style/Styleable.cpp 2022-03-15 11:01:41 UTC (rev 291281)
+++ trunk/Source/WebCore/style/Styleable.cpp 2022-03-15 11:40:00 UTC (rev 291282)
@@ -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();
@@ -259,13 +232,15 @@
{
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();
- }
}
}
+
+ if (auto* effectStack = keyframeEffectStack())
+ effectStack->setCSSAnimationList(nullptr);
+
+ setAnimationsCreatedByMarkup({ });
}
static bool keyframesRuleExistsForAnimation(Element& element, const Animation& animation, const String& animationName)
@@ -298,6 +273,7 @@
for (auto& cssAnimation : animationsCreatedByMarkup())
cssAnimation->cancelFromStyle();
keyframeEffectStack.setCSSAnimationList(nullptr);
+ setAnimationsCreatedByMarkup({ });
return;
}
