Changeset 291282 in webkit


Ignore:
Timestamp:
Mar 15, 2022 4:40:00 AM (4 months ago)
Author:
graouts@webkit.org
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.

Location:
trunk
Files:
4 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r291260 r291282  
     12022-03-15  Antoine Quint  <graouts@webkit.org>
     2
     3        Dialog element only animates once
     4        https://bugs.webkit.org/show_bug.cgi?id=236274
     5
     6        Reviewed by Dean Jackson, Tim Nguyen and Antti Koivisto.
     7
     8        Import relevant WPT tests that had already been upstreamed in a previous, reverted
     9        version of this patch.
     10
     11        * web-platform-tests/css/css-animations/dialog-animation-expected.txt: Added.
     12        * web-platform-tests/css/css-animations/dialog-animation.html: Added.
     13        * web-platform-tests/css/css-animations/dialog-backdrop-animation-expected.txt: Added.
     14        * web-platform-tests/css/css-animations/dialog-backdrop-animation.html: Added.
     15        * web-platform-tests/css/css-animations/support/testcommon.js:
     16        (addElement):
     17        (addDiv):
     18
    1192022-03-14  Oriol Brufau  <obrufau@igalia.com>
    220

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

    r289682 r291282  
    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

    r291281 r291282  
     12022-03-15  Antoine Quint  <graouts@webkit.org>
     2
     3        Dialog element only animates once
     4        https://bugs.webkit.org/show_bug.cgi?id=236274
     5
     6        Reviewed by Dean Jackson, Tim Nguyen and Antti Koivisto.
     7
     8        Two issues related to CSS Animation surfaced in this bug which animates both <dialog>
     9        and its ::backdrop as the dialog is open and eventually re-opened.
     10
     11        The first issue was that we didn't clear all CSS Animations state when a <dialog> was
     12        closed and its style was set to `display: none`. We now call setAnimationsCreatedByMarkup
     13        to correctly clear such state both when we identify a Styleable is newly getting
     14        `display: none`. We do the same when cancelDeclarativeAnimations() is called, but also
     15        call setCSSAnimationList() on the associated effect stack since that wasn't done either.
     16        Now both functions do similar cleanup.
     17       
     18       
     19        This allows us to remove removeCSSAnimationCreatedByMarkup() which did a fair bit of work
     20        to clear CSS Animation state per-animation when we only ever used that function for
     21        _all_ animations.
     22
     23        The second issue was that we never called cancelDeclarativeAnimations() for ::backdrop.
     24        We now do that inside of Element::removeFromTopLayer() at a point where the code in
     25        Styleable::fromRenderer() will still work as the element will still be contained in
     26        Document::topLayerElements().
     27
     28        Tests: imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html
     29               imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation.html
     30
     31        * dom/Element.cpp:
     32        (WebCore::Element::removeFromTopLayer):
     33        * style/Styleable.cpp:
     34        (WebCore::Styleable::cancelDeclarativeAnimations const):
     35        (WebCore::Styleable::updateCSSAnimations const):
     36        (WebCore::removeCSSAnimationCreatedByMarkup): Deleted.
     37
    1382022-03-15  Gabriel Nava Marino  <gnavamarino@apple.com>
    239

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

    r290830 r291282  
    34463446    });
    34473447
     3448    // We need to call Styleable::fromRenderer() while this element is still contained in
     3449    // Document::topLayerElements(), since Styleable::fromRenderer() relies on this to
     3450    // find the backdrop's associated element.
     3451    if (auto* renderer = this->renderer()) {
     3452        if (auto backdrop = renderer->backdropRenderer()) {
     3453            if (auto styleable = Styleable::fromRenderer(*backdrop))
     3454                styleable->cancelDeclarativeAnimations();
     3455        }
     3456    }
     3457
    34483458    document().removeTopLayerElement(*this);
    34493459    clearNodeFlag(NodeFlag::IsInTopLayer);

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

    r289682 r291282  
    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{
    … …  
    260233    if (auto* animations = this->animations()) {
    261234        for (auto& animation : *animations) {
    262             if (is<DeclarativeAnimation>(animation)) {
    263                 if (is<CSSAnimation>(animation))
    264                     removeCSSAnimationCreatedByMarkup(*this, downcast<CSSAnimation>(*animation));
     235            if (is<DeclarativeAnimation>(animation))
    265236                downcast<DeclarativeAnimation>(*animation).cancelFromStyle();
    266             }
    267         }
    268     }
     237        }
     238    }
     239
     240    if (auto* effectStack = keyframeEffectStack())
     241        effectStack->setCSSAnimationList(nullptr);
     242
     243    setAnimationsCreatedByMarkup({ });
    269244}
    270245
    … …  
    299274            cssAnimation->cancelFromStyle();
    300275        keyframeEffectStack.setCSSAnimationList(nullptr);
     276        setAnimationsCreatedByMarkup({ });
    301277        return;
    302278    }
Note: See TracChangeset for help on using the changeset viewer.