Changeset 289498 in webkit


Ignore:
Timestamp:
Feb 9, 2022 12:48:11 PM (5 months ago)
Author:
graouts@webkit.org
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.

Location:
trunk
Files:
4 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r289494 r289498  
     12022-02-09  Antoine Quint  <graouts@webkit.org>
     2
     3        Dialog element only animates once
     4        https://bugs.webkit.org/show_bug.cgi?id=236274
     5        rdar://88635487
     6
     7        Reviewed by Tim Nguyen and Dean Jackson.
     8
     9        Add two new tests that check that we correctly start, stop and resume animations on <dialog>
     10        and ::backdrop as a <dialog> element is open, closed and open again.
     11
     12        * web-platform-tests/css/css-animations/dialog-animation-expected.txt: Added.
     13        * web-platform-tests/css/css-animations/dialog-animation.html: Added.
     14        * web-platform-tests/css/css-animations/dialog-backdrop-animation-expected.txt: Added.
     15        * web-platform-tests/css/css-animations/dialog-backdrop-animation.html: Added.
     16        * web-platform-tests/css/css-animations/support/testcommon.js:
     17        (addElement):
     18        (addDiv):
     19
    1202022-02-09  Alex Christensen  <achristensen@webkit.org>
    221

  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/support/testcommon.js

    r277053 r289498  
    8888
    8989/**
    90  * Appends a div to the document body.
     90 * Appends an element to the document body.
    9191 *
    9292 * @param t  The testharness.js Test object. If provided, this will be used
    … …  
    9494 *           finishes.
    9595 *
     96 * @param name  A string specifying the element name.
     97 *
    9698 * @param attrs  A dictionary object with attribute names and values to set on
    9799 *               the div.
    98100 */
    99 function addDiv(t, attrs) {
    100   var div = document.createElement('div');
     101function addElement(t, name, attrs) {
     102  var element = document.createElement(name);
    101103  if (attrs) {
    102104    for (var attrName in attrs) {
    103       div.setAttribute(attrName, attrs[attrName]);
    104     }
    105   }
    106   document.body.appendChild(div);
     105      element.setAttribute(attrName, attrs[attrName]);
     106    }
     107  }
     108  document.body.appendChild(element);
    107109  if (t && typeof t.add_cleanup === 'function') {
    108     t.add_cleanup(function() {
    109       if (div.parentNode) {
    110         div.remove();
    111       }
    112     });
    113   }
    114   return div;
     110      t.add_cleanup(() => element.remove());
     111  }
     112  return element;
     113}
     114
     115/**
     116 * Appends a div to the document body.
     117 *
     118 * @param t  The testharness.js Test object. If provided, this will be used
     119 *           to register a cleanup callback to remove the div when the test
     120 *           finishes.
     121 *
     122 * @param attrs  A dictionary object with attribute names and values to set on
     123 *               the div.
     124 */
     125function addDiv(t, attrs) {
     126  return addElement(t, "div", attrs);
    115127}
    116128

  • trunk/Source/WebCore/ChangeLog

    r289497 r289498  
     12022-02-09  Antoine Quint  <graouts@webkit.org>
     2
     3        Dialog element only animates once
     4        https://bugs.webkit.org/show_bug.cgi?id=236274
     5        rdar://88635487
     6
     7        Reviewed by Tim Nguyen and Dean Jackson.
     8
     9        Two issues related to CSS Animation surfaced in this bug which animates both <dialog>
     10        and its ::backdrop as the dialog is open and eventually re-opened.
     11
     12        The first issue was that we didn't clear all CSS Animations state when a <dialog> was
     13        closed and its style was set to `display: none`. We now use the clearCSSAnimationsForStyleable()
     14        function (static so that it's not exposed on the Styleable struct) to correctly clear
     15        such state both when we identify a Styleable is newly getting `display: none` and when
     16        cancelDeclarativeAnimations() was called. This allows us to remove removeCSSAnimationCreatedByMarkup()
     17        a fair bit of work to clear CSS Animation state per-animation when we only ever used that
     18        function for _all_ animations.
     19
     20        The second issue was that we never called cancelDeclarativeAnimations() for ::backdrop.
     21        We now do that inside of Element::removeFromTopLayer() at a point where the code in
     22        Styleable::fromRenderer() will still work as the element will still be contained in
     23        Document::topLayerElements().
     24
     25        Tests: imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html
     26               imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation.html
     27
     28        * dom/Element.cpp:
     29        (WebCore::Element::removeFromTopLayer):
     30        * style/Styleable.cpp:
     31        (WebCore::clearCSSAnimationsForStyleable):
     32        (WebCore::Styleable::cancelDeclarativeAnimations const):
     33        (WebCore::Styleable::updateCSSAnimations const):
     34        (WebCore::removeCSSAnimationCreatedByMarkup): Deleted.
     35
    1362022-02-09  Fujii Hironori  <Hironori.Fujii@sony.com>
    237

  • trunk/Source/WebCore/dom/Element.cpp

    r289457 r289498  
    34433443    });
    34443444
     3445    // We need to call Styleable::fromRenderer() while this document is still contained
     3446    // in Document::topLayerElements().
     3447    if (auto* renderer = this->renderer()) {
     3448        if (auto backdrop = renderer->backdropRenderer()) {
     3449            if (auto styleable = Styleable::fromRenderer(*backdrop))
     3450                styleable->cancelDeclarativeAnimations();
     3451        }
     3452    }
     3453
    34453454    document().removeTopLayerElement(*this);
    34463455    clearNodeFlag(NodeFlag::IsInTopLayer);

  • trunk/Source/WebCore/style/Styleable.cpp

    r289455 r289498  
    216216}
    217217
    218 static void removeCSSAnimationCreatedByMarkup(const Styleable& styleable, CSSAnimation& cssAnimation)
    219 {
    220     styleable.animationsCreatedByMarkup().remove(&cssAnimation);
    221 
    222     if (!styleable.hasKeyframeEffects())
    223         return;
    224 
    225     auto& keyframeEffectStack = styleable.ensureKeyframeEffectStack();
    226     auto* cssAnimationList = keyframeEffectStack.cssAnimationList();
    227     if (!cssAnimationList || cssAnimationList->isEmpty())
    228         return;
    229 
    230     auto& backingAnimation = cssAnimation.backingAnimation();
    231     for (size_t i = 0; i < cssAnimationList->size(); ++i) {
    232         if (cssAnimationList->animation(i) == backingAnimation) {
    233             // It is important we do not make a clone of the Animation references contained
    234             // within cssAnimationList since sorting animations in compareCSSAnimations()
    235             // makes pointer comparisons to distinguish between backing animations of various
    236             // CSSAnimation objects.
    237             auto newAnimationList = cssAnimationList->shallowCopy();
    238             newAnimationList->remove(i);
    239             keyframeEffectStack.setCSSAnimationList(WTFMove(newAnimationList));
    240             return;
    241         }
    242     }
    243 }
    244 
    245218void Styleable::elementWasRemoved() const
    246219{
    … …  
    256229}
    257230
     231static void clearCSSAnimationsForStyleable(const Styleable& styleable)
     232{
     233    for (auto& cssAnimation : styleable.animationsCreatedByMarkup())
     234        cssAnimation->cancelFromStyle();
     235    if (auto* effectStack = styleable.keyframeEffectStack())
     236        effectStack->setCSSAnimationList(nullptr);
     237    styleable.setAnimationsCreatedByMarkup({ });
     238}
     239
    258240void Styleable::cancelDeclarativeAnimations() const
    259241{
    260242    if (auto* animations = this->animations()) {
    261243        for (auto& animation : *animations) {
    262             if (is<DeclarativeAnimation>(animation)) {
    263                 if (is<CSSAnimation>(animation))
    264                     removeCSSAnimationCreatedByMarkup(*this, downcast<CSSAnimation>(*animation));
     244            if (is<DeclarativeAnimation>(animation))
    265245                downcast<DeclarativeAnimation>(*animation).cancelFromStyle();
    266             }
    267         }
    268     }
     246        }
     247    }
     248
     249    clearCSSAnimationsForStyleable(*this);
    269250}
    270251
    … …  
    296277    // In case this element is newly getting a "display: none" we need to cancel all of its animations and disregard new ones.
    297278    if (currentStyle && currentStyle->display() != DisplayType::None && newStyle.display() == DisplayType::None) {
    298         for (auto& cssAnimation : animationsCreatedByMarkup())
    299             cssAnimation->cancelFromStyle();
    300         keyframeEffectStack.setCSSAnimationList(nullptr);
     279        clearCSSAnimationsForStyleable(*this);
    301280        return;
    302281    }
Note: See TracChangeset for help on using the changeset viewer.