Changeset 268932 in webkit


Ignore:
Timestamp:
Oct 23, 2020, 11:35:44 AM (5 years ago)
Author:
graouts@webkit.org
Message:

REGRESSION(r268615): certain animations break when moving from one to display to another or resizing the window
​https://bugs.webkit.org/show_bug.cgi?id=218080
<rdar://problem/70547132>

Reviewed by Dean Jackson.

Source/WebCore:

Since transform-related animations include non-interpolating animations meant to insert the static base value for a given
transform-related CSS property, we need to update animations on GraphicsLayerCA whenever one of the transform-related CSS
properties have a new value.

We used to rely on GraphicsLayerCA::setTransform() being called with a different transform than the current one to identify
such cases, but that is suboptimal because that method can be called with a compound interpolated value of transform-related
CSS properties when a rendering update occurs, such as during resizing or moving a window between displays. In those cases,
the static base value of the transform-related CSS properties hasn't actually changed.

Instead, we now provide the non-animated style from the last style change event to the function resolving keyframe effects
so that for a given element we can compare that style with the new, as-yet-non-animated style and see if any of the transform-
related CSS properties have been changed. If that is the case, we inform any KeyframeEffect that has a running accelerated
animation for any of those CSS properties so that the effect may enqueue an accelerated action that will then notify the
GraphicsLayer of such a change, and trigger an animation update.

Since we were changing the applyKeyframeEffects() method signature to add the extra RenderStyle needed to compare the current
and previous non-animated styles, we also moved that method from Element to KeyframeEffectStack since no Element private
API was required.

No new test since this was already tested by webanimations/accelerated-translate-animation-underlying-transform-changed-in-flight.html
and it's not clear how to test the live-resizing or display-change scenario.

  • animation/KeyframeEffect.cpp:

(WebCore::KeyframeEffect::isRunningAcceleratedTransformRelatedAnimation const): New method called from KeyframeEffectStack::applyKeyframeEffects()
to indicate that a keyframe effect has a running accelerated animation targeting a transform-related property.
(WebCore::KeyframeEffect::addPendingAcceleratedAction): Ensure that the new AcceleratedAction::TransformChange accelerated action
recorded in transformRelatedPropertyDidChange() is not ever set as m_lastRecordedAcceleratedAction as we use this member to identify
whether we have a pending running, pause or stop action.
(WebCore::KeyframeEffect::transformRelatedPropertyDidChange): New method meant to be called for an effect that has a running
accelerated animation targeting a transform-related property to notify that one or more of the target element's transform-related
CSS property static values was changed.
(WebCore::KeyframeEffect::applyPendingAcceleratedActions): Call transformRelatedPropertyDidChange() on the composited renderer for
a AcceleratedAction::TransformChange action.

  • animation/KeyframeEffect.h:
  • animation/KeyframeEffectStack.cpp:

(WebCore::KeyframeEffectStack::applyKeyframeEffects): Move the method previously exposed on Element to KeyframeEffectStack. Additionally,
accept an extra RenderStyle parameter to provide the non-animated style from the last style change event so that we can compare that style
with the new, as-yet-non-animated style and see if any of the transform-related CSS properties have been changed and notify the effect
should it run an accelerated animation for one of those properties.

  • animation/KeyframeEffectStack.h:
  • dom/Element.cpp:

(WebCore::Element::applyKeyframeEffects): Deleted. Moved to KeyframeEffectStack.

  • dom/Element.h:
  • platform/graphics/GraphicsLayer.h:

(WebCore::GraphicsLayer::transformRelatedPropertyDidChange):

  • platform/graphics/ca/GraphicsLayerCA.cpp:

(WebCore::GraphicsLayerCA::setTransform): Move the animation-update logic to transformRelatedPropertyDidChange()
(WebCore::GraphicsLayerCA::transformRelatedPropertyDidChange):

  • platform/graphics/ca/GraphicsLayerCA.h:
  • rendering/RenderElement.h:

(WebCore::RenderElement::transformRelatedPropertyDidChange):

  • rendering/RenderLayerBacking.cpp:

(WebCore::RenderLayerBacking::transformRelatedPropertyDidChange):

  • rendering/RenderLayerBacking.h:
  • rendering/RenderLayerModelObject.cpp:

(WebCore::RenderLayerModelObject::transformRelatedPropertyDidChange):

  • rendering/RenderLayerModelObject.h:
  • style/StyleTreeResolver.cpp:

(WebCore::Style::TreeResolver::createAnimatedElementUpdate): Pass the non-animated style from the last style change event to
KeyframeEffectStack::applyKeyframeEffects() to determine whether this style change event includes a change to any of the
transform-related properties.

  • style/Styleable.h:

(WebCore::Styleable::applyKeyframeEffects const):

LayoutTests:

Increase the fidelity of this test where the scale transform would sometimes yield some 0.01% ImageOnlyFailure results.

  • webanimations/accelerated-translate-animation-additional-animation-added-in-flight-expected.html:
  • webanimations/accelerated-translate-animation-additional-animation-added-in-flight.html:
Location:
trunk
Files:
20 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r268927 r268932  
     12020-10-23  Antoine Quint  <graouts@webkit.org>
     2
     3        REGRESSION(r268615): certain animations break when moving from one to display to another or resizing the window
     4        https://bugs.webkit.org/show_bug.cgi?id=218080
     5        <rdar://problem/70547132>
     6
     7        Reviewed by Dean Jackson.
     8
     9        Increase the fidelity of this test where the scale transform would sometimes yield some 0.01% ImageOnlyFailure results.
     10
     11        * webanimations/accelerated-translate-animation-additional-animation-added-in-flight-expected.html:
     12        * webanimations/accelerated-translate-animation-additional-animation-added-in-flight.html:
     13
    1142020-10-23  Hector Lopez  <hector_i_lopez@apple.com>
    215

  • trunk/LayoutTests/webanimations/accelerated-translate-animation-additional-animation-added-in-flight-expected.html

    r268615 r268932  
    88        height: 100px;
    99        background-color: black;
    10         transform: translateX(100px) scale(0.5);
     10        transform: translate3d(50px, 50px, 0);
    1111    }
    1212

  • trunk/LayoutTests/webanimations/accelerated-translate-animation-additional-animation-added-in-flight.html

    r268615 r268932  
    2121    // Start an animation that lasts a day.
    2222    const duration = 24 * 60 * 60 * 1000;
    23     const translateAnimation = document.getElementById("target").animate({ translate: "200px" }, duration);
     23    const translateAnimation = document.getElementById("target").animate({ translate: "100px" }, duration);
    2424    translateAnimation.currentTime = duration / 2;
    2525
    … …  
    2929
    3030    // Add an extra animation.
    31     const scaleAnimation = document.getElementById("target").animate({ scale: "0" }, duration);
    32     scaleAnimation.currentTime = duration / 2;
     31    const transformAnimation = document.getElementById("target").animate({ transform: "translateY(100px)" }, duration);
     32    transformAnimation.currentTime = duration / 2;
    3333
    3434    // Wait until the new animation has been applied.
    35     await scaleAnimation.ready;
     35    await transformAnimation.ready;
    3636    await UIHelper.ensureStablePresentationUpdate();
    3737

  • trunk/Source/WebCore/ChangeLog

    r268926 r268932  
     12020-10-23  Antoine Quint  <graouts@webkit.org>
     2
     3        REGRESSION(r268615): certain animations break when moving from one to display to another or resizing the window
     4        https://bugs.webkit.org/show_bug.cgi?id=218080
     5        <rdar://problem/70547132>
     6
     7        Reviewed by Dean Jackson.
     8
     9        Since transform-related animations include non-interpolating animations meant to insert the static base value for a given
     10        transform-related CSS property, we need to update animations on GraphicsLayerCA whenever one of the transform-related CSS
     11        properties have a new value.
     12
     13        We used to rely on GraphicsLayerCA::setTransform() being called with a different transform than the current one to identify
     14        such cases, but that is suboptimal because that method can be called with a compound interpolated value of transform-related
     15        CSS properties when a rendering update occurs, such as during resizing or moving a window between displays. In those cases,
     16        the static base value of the transform-related CSS properties hasn't actually changed.
     17       
     18        Instead, we now provide the non-animated style from the last style change event to the function resolving keyframe effects
     19        so that for a given element we can compare that style with the new, as-yet-non-animated style and see if any of the transform-
     20        related CSS properties have been changed. If that is the case, we inform any KeyframeEffect that has a running accelerated
     21        animation for any of those CSS properties so that the effect may enqueue an accelerated action that will then notify the
     22        GraphicsLayer of such a change, and trigger an animation update.
     23
     24        Since we were changing the applyKeyframeEffects() method signature to add the extra RenderStyle needed to compare the current
     25        and previous non-animated styles, we also moved that method from Element to KeyframeEffectStack since no Element private
     26        API was required.
     27
     28        No new test since this was already tested by webanimations/accelerated-translate-animation-underlying-transform-changed-in-flight.html
     29        and it's not clear how to test the live-resizing or display-change scenario.
     30
     31        * animation/KeyframeEffect.cpp:
     32        (WebCore::KeyframeEffect::isRunningAcceleratedTransformRelatedAnimation const): New method called from KeyframeEffectStack::applyKeyframeEffects()
     33        to indicate that a keyframe effect has a running accelerated animation targeting a transform-related property.
     34        (WebCore::KeyframeEffect::addPendingAcceleratedAction): Ensure that the new AcceleratedAction::TransformChange accelerated action
     35        recorded in transformRelatedPropertyDidChange() is not ever set as m_lastRecordedAcceleratedAction as we use this member to identify
     36        whether we have a pending running, pause or stop action.
     37        (WebCore::KeyframeEffect::transformRelatedPropertyDidChange): New method meant to be called for an effect that has a running
     38        accelerated animation targeting a transform-related property to notify that one or more of the target element's transform-related
     39        CSS property static values was changed.
     40        (WebCore::KeyframeEffect::applyPendingAcceleratedActions): Call transformRelatedPropertyDidChange() on the composited renderer for
     41        a AcceleratedAction::TransformChange action.
     42        * animation/KeyframeEffect.h:
     43        * animation/KeyframeEffectStack.cpp:
     44        (WebCore::KeyframeEffectStack::applyKeyframeEffects): Move the method previously exposed on Element to KeyframeEffectStack. Additionally,
     45        accept an extra RenderStyle parameter to provide the non-animated style from the last style change event so that we can compare that style
     46        with the new, as-yet-non-animated style and see if any of the transform-related CSS properties have been changed and notify the effect
     47        should it run an accelerated animation for one of those properties.
     48        * animation/KeyframeEffectStack.h:
     49        * dom/Element.cpp:
     50        (WebCore::Element::applyKeyframeEffects): Deleted. Moved to KeyframeEffectStack.
     51        * dom/Element.h:
     52        * platform/graphics/GraphicsLayer.h:
     53        (WebCore::GraphicsLayer::transformRelatedPropertyDidChange):
     54        * platform/graphics/ca/GraphicsLayerCA.cpp:
     55        (WebCore::GraphicsLayerCA::setTransform): Move the animation-update logic to transformRelatedPropertyDidChange()
     56        (WebCore::GraphicsLayerCA::transformRelatedPropertyDidChange):
     57        * platform/graphics/ca/GraphicsLayerCA.h:
     58        * rendering/RenderElement.h:
     59        (WebCore::RenderElement::transformRelatedPropertyDidChange):
     60        * rendering/RenderLayerBacking.cpp:
     61        (WebCore::RenderLayerBacking::transformRelatedPropertyDidChange):
     62        * rendering/RenderLayerBacking.h:
     63        * rendering/RenderLayerModelObject.cpp:
     64        (WebCore::RenderLayerModelObject::transformRelatedPropertyDidChange):
     65        * rendering/RenderLayerModelObject.h:
     66        * style/StyleTreeResolver.cpp:
     67        (WebCore::Style::TreeResolver::createAnimatedElementUpdate): Pass the non-animated style from the last style change event to
     68        KeyframeEffectStack::applyKeyframeEffects() to determine whether this style change event includes a change to any of the
     69        transform-related properties.
     70        * style/Styleable.h:
     71        (WebCore::Styleable::applyKeyframeEffects const):
     72
    1732020-10-23  Chris Dumez  <cdumez@apple.com>
    274

  • trunk/Source/WebCore/animation/KeyframeEffect.cpp

    r268730 r268932  
    12861286}
    12871287
     1288bool KeyframeEffect::isRunningAcceleratedTransformRelatedAnimation() const
     1289{
     1290    if (!isRunningAccelerated())
     1291        return false;
     1292
     1293    return m_blendingKeyframes.properties().contains(CSSPropertyTranslate)
     1294        || m_blendingKeyframes.properties().contains(CSSPropertyScale)
     1295        || m_blendingKeyframes.properties().contains(CSSPropertyRotate)
     1296        || m_blendingKeyframes.properties().contains(CSSPropertyTransform);
     1297}
     1298
    12881299void KeyframeEffect::invalidate()
    12891300{
    … …  
    15931604        m_pendingAcceleratedActions.clear();
    15941605    m_pendingAcceleratedActions.append(action);
    1595     if (action != AcceleratedAction::UpdateTiming)
     1606    if (action != AcceleratedAction::UpdateTiming && action != AcceleratedAction::TransformChange)
    15961607        m_lastRecordedAcceleratedAction = action;
    15971608    animation()->acceleratedStateDidChange();
    … …  
    16181629    else if (canBeAccelerated())
    16191630        m_runningAccelerated = RunningAccelerated::NotStarted;
     1631}
     1632
     1633void KeyframeEffect::transformRelatedPropertyDidChange()
     1634{
     1635    ASSERT(isRunningAcceleratedTransformRelatedAnimation());
     1636    addPendingAcceleratedAction(AcceleratedAction::TransformChange);
    16201637}
    16211638
    … …  
    17091726            m_runningAccelerated = RunningAccelerated::NotStarted;
    17101727            break;
     1728        case AcceleratedAction::TransformChange:
     1729            renderer->transformRelatedPropertyDidChange();
     1730            break;
    17111731        }
    17121732    }

  • trunk/Source/WebCore/animation/KeyframeEffect.h

    r267571 r268932  
    133133    void animationTimelineDidChange(AnimationTimeline*) final;
    134134    void animationTimingDidChange();
     135    void transformRelatedPropertyDidChange();
    135136    void applyPendingAcceleratedActions();
    136137
    … …  
    168169    bool isCurrentlyAffectingProperty(CSSPropertyID, Accelerated = Accelerated::No) const;
    169170    bool isRunningAcceleratedAnimationForProperty(CSSPropertyID) const;
     171    bool isRunningAcceleratedTransformRelatedAnimation() const;
    170172
    171173    bool requiresPseudoElement() const;
    … …  
    174176    KeyframeEffect(Element*, PseudoId);
    175177
    176     enum class AcceleratedAction : uint8_t { Play, Pause, UpdateTiming, Stop };
     178    enum class AcceleratedAction : uint8_t { Play, Pause, UpdateTiming, TransformChange, Stop };
    177179    enum class BlendingKeyframesSource : uint8_t { CSSAnimation, CSSTransition, WebAnimation };
    178180    enum class AcceleratedProperties : uint8_t { None, Some, All };

  • trunk/Source/WebCore/animation/KeyframeEffectStack.cpp

    r267571 r268932  
    112112}
    113113
     114OptionSet<AnimationImpact> KeyframeEffectStack::applyKeyframeEffects(RenderStyle& targetStyle, const RenderStyle& previousLastStyleChangeEventStyle)
     115{
     116    OptionSet<AnimationImpact> impact;
     117
     118    auto transformRelatedPropertyChanged = [&]() -> bool {
     119        return !arePointingToEqualData(targetStyle.translate(), previousLastStyleChangeEventStyle.translate())
     120            || !arePointingToEqualData(targetStyle.scale(), previousLastStyleChangeEventStyle.scale())
     121            || !arePointingToEqualData(targetStyle.rotate(), previousLastStyleChangeEventStyle.rotate())
     122            || targetStyle.transform() != previousLastStyleChangeEventStyle.transform();
     123    }();
     124
     125    for (const auto& effect : sortedEffects()) {
     126        ASSERT(effect->animation());
     127        effect->animation()->resolve(targetStyle);
     128
     129        if (effect->isRunningAccelerated() || effect->isAboutToRunAccelerated())
     130            impact.add(AnimationImpact::RequiresRecomposite);
     131
     132        if (effect->triggersStackingContext())
     133            impact.add(AnimationImpact::ForcesStackingContext);
     134
     135        if (transformRelatedPropertyChanged && effect->isRunningAcceleratedTransformRelatedAnimation())
     136            effect->transformRelatedPropertyDidChange();
     137    }
     138
     139    return impact;
     140}
     141
    114142} // namespace WebCore

  • trunk/Source/WebCore/animation/KeyframeEffectStack.h

    r260139 r268932  
    2828#include "AnimationList.h"
    2929#include "CSSPropertyNames.h"
     30#include "WebAnimationTypes.h"
    3031#include <wtf/Vector.h>
    3132#include <wtf/WeakPtr.h>
    … …  
    4950    bool isCurrentlyAffectingProperty(CSSPropertyID) const;
    5051    bool requiresPseudoElement() const;
     52    OptionSet<AnimationImpact> applyKeyframeEffects(RenderStyle& targetStyle, const RenderStyle& previousLastStyleChangeEventStyle);
    5153
    5254private:

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

    r268808 r268932  
    38233823}
    38243824
    3825 OptionSet<AnimationImpact> Element::applyKeyframeEffects(PseudoId pseudoId, RenderStyle& targetStyle)
    3826 {
    3827     OptionSet<AnimationImpact> impact;
    3828 
    3829     for (const auto& effect : ensureKeyframeEffectStack(pseudoId).sortedEffects()) {
    3830         ASSERT(effect->animation());
    3831         effect->animation()->resolve(targetStyle);
    3832 
    3833         if (effect->isRunningAccelerated() || effect->isAboutToRunAccelerated())
    3834             impact.add(AnimationImpact::RequiresRecomposite);
    3835 
    3836         if (effect->triggersStackingContext())
    3837             impact.add(AnimationImpact::ForcesStackingContext);
    3838     }
    3839 
    3840     return impact;
    3841 }
    3842 
    38433825const AnimationCollection* Element::animations(PseudoId pseudoId) const
    38443826{

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

    r268808 r268932  
    490490    KeyframeEffectStack& ensureKeyframeEffectStack(PseudoId);
    491491    bool hasKeyframeEffects(PseudoId) const;
    492     OptionSet<AnimationImpact> applyKeyframeEffects(PseudoId, RenderStyle&);
    493492
    494493    const AnimationCollection* animations(PseudoId) const;

  • trunk/Source/WebCore/platform/graphics/GraphicsLayer.h

    r268898 r268932  
    486486    virtual void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/) { }
    487487    virtual void removeAnimation(const String& /*animationName*/) { }
    488 
     488    virtual void transformRelatedPropertyDidChange() { }
    489489    WEBCORE_EXPORT virtual void suspendAnimations(MonotonicTime);
    490490    WEBCORE_EXPORT virtual void resumeAnimations();

  • trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp

    r268898 r268932  
    667667    GraphicsLayer::setTransform(t);
    668668    noteLayerPropertyChanged(TransformChanged);
    669 
    670     // If we are currently running a transform-related animation, a change in underlying
    671     // transform value means we must re-evaluate all transform-related animations to ensure
    672     // that the base value transform animations are current.
    673     if (isRunningTransformAnimation())
    674         noteLayerPropertyChanged(AnimationChanged | CoverageRectChanged);
    675669}
    676670
    … …  
    11081102        }
    11091103    }
     1104}
     1105
     1106void GraphicsLayerCA::transformRelatedPropertyDidChange()
     1107{
     1108    // If we are currently running a transform-related animation, a change in underlying
     1109    // transform value means we must re-evaluate all transform-related animations to ensure
     1110    // that the base value transform animations are current.
     1111    if (isRunningTransformAnimation())
     1112        noteLayerPropertyChanged(AnimationChanged | CoverageRectChanged);
    11101113}
    11111114

  • trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h

    r268898 r268932  
    139139    WEBCORE_EXPORT void pauseAnimation(const String& animationName, double timeOffset) override;
    140140    WEBCORE_EXPORT void removeAnimation(const String& animationName) override;
    141 
     141    WEBCORE_EXPORT void transformRelatedPropertyDidChange() override;
    142142    WEBCORE_EXPORT void setContentsToImage(Image*) override;
    143143#if PLATFORM(IOS_FAMILY)

  • trunk/Source/WebCore/rendering/RenderElement.h

    r268021 r268932  
    231231    virtual void animationPaused(double /* timeOffset */, const String& /* name */) { }
    232232    virtual void animationFinished(const String& /* name */) { }
     233    virtual void transformRelatedPropertyDidChange() { }
    233234
    234235    virtual void suspendAnimations(MonotonicTime = MonotonicTime()) { }

  • trunk/Source/WebCore/rendering/RenderLayerBacking.cpp

    r268898 r268932  
    36703670}
    36713671
     3672void RenderLayerBacking::transformRelatedPropertyDidChange()
     3673{
     3674    m_graphicsLayer->transformRelatedPropertyDidChange();
     3675}
     3676
    36723677void RenderLayerBacking::notifyAnimationStarted(const GraphicsLayer*, const String&, MonotonicTime)
    36733678{

  • trunk/Source/WebCore/rendering/RenderLayerBacking.h

    r268615 r268932  
    189189    void animationPaused(double timeOffset, const String& name);
    190190    void animationFinished(const String& name);
    191 
     191    void transformRelatedPropertyDidChange();
    192192    void suspendAnimations(MonotonicTime = MonotonicTime());
    193193    void resumeAnimations();

  • trunk/Source/WebCore/rendering/RenderLayerModelObject.cpp

    r267188 r268932  
    272272}
    273273
     274void RenderLayerModelObject::transformRelatedPropertyDidChange()
     275{
     276    if (!layer() || !layer()->backing())
     277        return;
     278    layer()->backing()->transformRelatedPropertyDidChange();
     279}
     280
    274281void RenderLayerModelObject::suspendAnimations(MonotonicTime time)
    275282{

  • trunk/Source/WebCore/rendering/RenderLayerModelObject.h

    r267188 r268932  
    7373    void animationPaused(double timeOffset, const String& name) override;
    7474    void animationFinished(const String& name) override;
     75    void transformRelatedPropertyDidChange() override;
    7576
    7677    void suspendAnimations(MonotonicTime = MonotonicTime()) override;

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

    r267571 r268932  
    350350    // as animations created via the JS API.
    351351    if (styleable.hasKeyframeEffects()) {
     352        auto previousLastStyleChangeEventStyle = styleable.lastStyleChangeEventStyle() ? RenderStyle::clonePtr(*styleable.lastStyleChangeEventStyle()) : RenderStyle::createPtr();
    352353        // Record the style prior to applying animations for this style change event.
    353354        styleable.setLastStyleChangeEventStyle(RenderStyle::clonePtr(*newStyle));
    354355        // Apply all keyframe effects to the new style.
    355356        auto animatedStyle = RenderStyle::clonePtr(*newStyle);
    356         animationImpact = styleable.applyKeyframeEffects(*animatedStyle);
     357        animationImpact = styleable.applyKeyframeEffects(*animatedStyle, *previousLastStyleChangeEventStyle);
    357358        newStyle = WTFMove(animatedStyle);
    358359    } else

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

    r268808 r268932  
    2727
    2828#include "Element.h"
     29#include "KeyframeEffectStack.h"
    2930#include "PseudoElement.h"
    3031#include "RenderElement.h"
    … …  
    8788    }
    8889
    89     OptionSet<AnimationImpact> applyKeyframeEffects(RenderStyle& style) const
     90    OptionSet<AnimationImpact> applyKeyframeEffects(RenderStyle& targetStyle, const RenderStyle& previousLastStyleChangeEventStyle) const
    9091    {
    91         return element.applyKeyframeEffects(pseudoId, style);
     92        return element.ensureKeyframeEffectStack(pseudoId).applyKeyframeEffects(targetStyle, previousLastStyleChangeEventStyle);
    9293    }
    9394
Note: See TracChangeset for help on using the changeset viewer.