Bug 182098 – [Web Animations] Account for provided easings when computing progress and resolving keyframe effect values

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

We've added support for parsing easing values in webkit.org/b/181978, we now need to use those values when computing the progress and resolving keyframe effect values.

[Radar WebKit Bug Importer]

<rdar://problem/36866149>

[Antoine Quint]

Created attachment 332260 [details]
Patch

[Antoine Quint]

Created attachment 332261 [details]
Patch

[EWS Watchlist]

Comment on attachment 332261 [details]
Patch

Attachment 332261 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6208343

New failing tests:
transitions/default-timing-function.html

[EWS Watchlist]

Created attachment 332266 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332261 [details]
Patch

Attachment 332261 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6208379

New failing tests:
transitions/default-timing-function.html

[EWS Watchlist]

Created attachment 332270 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Antoine Quint]

Created attachment 332272 [details]
Patch

[Dean Jackson]

Comment on attachment 332272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332272&action=review

All good, but I think you need to use optionals instead of the -1s everywhere. We've had animation bugs before when using special values, because someone eventually uses them as a boolean.

> Source/WebCore/ChangeLog:16
> +        and identify cubic-bezier(0, 0, 0, 0) and cubic-bezier(1, 1, 1, 1) as linear timing functions, as called out
> +        by the WPT tests.

Hmmm... the spec says this? I wonder what we do for our current animation code.

> Source/WebCore/animation/AnimationEffect.cpp:283
> +            bool goingFordwards = currentDirection() == AnimationEffect::ComputedDirection::Forwards;

Fordwards

> Source/WebCore/animation/KeyframeEffect.cpp:691
> +void KeyframeEffect::setAnimatedPropertiesInStyle(RenderStyle& targetStyle, double iterationProgress)

There is a fair bit of work in this method. I wonder if we could cache some of the computed keyframe lists, rather than computing them every time iterationProgress changes.

> Source/WebCore/animation/KeyframeEffect.cpp:700
> +        // 1. If iteration progress is unresolved abort this procedure.

I assume this is impossible here?

> Source/WebCore/animation/KeyframeEffect.cpp:711
> +        Vector<int> propertySpecificKeyframes;
> +        for (size_t i = 0; i < m_keyframes.size(); ++i) {

Couldn't this be a Vector of keyframes, and use a for : loop?

> Source/WebCore/animation/KeyframeEffect.cpp:720
> +            propertySpecificKeyframes.append(i);

Is there any point appending multiple keyframes with the same offset? e.g. 0 or 1
And if that case is already handled elsewhere, those can be bools rather than unsigned.

> Source/WebCore/animation/KeyframeEffect.cpp:729
> +        //    offset of 0, a property value set to the neutral value for composition, and a composite operation of add, and prepend it to the beginning of
> +        //    property-specific keyframes.

It's a bit confusing that this last bit about setting up the default value happens much later.

> Source/WebCore/animation/KeyframeEffect.cpp:731
> +            propertySpecificKeyframes.insert(0, -1);

I don't like this hack of inserting a -1.

> Source/WebCore/animation/KeyframeEffect.cpp:750
> +            // If iteration progress < 0 and there is more than one keyframe in property-specific keyframes with a computed keyframe offset of 0,
> +            // Add the first keyframe in property-specific keyframes to interval endpoints.
> +            intervalEndpoints.append(propertySpecificKeyframes.first());

So we probably could have handled this above by ignoring the subsequent zero keyframes and keeping a bool.

> Source/WebCore/animation/KeyframeEffect.cpp:761
> +            size_t indexOfLastKeyframeWithZeroOffset;

Initialize this to 0.

> Source/WebCore/animation/KeyframeEffect.cpp:771
> +                double offset;
> +                if (keyframeIndex == -1)
> +                    offset = i ? 1 : 0;
> +                else {
> +                    auto& keyframe = m_keyframes[keyframeIndex];
> +                    offset = keyframe.key();
> +                }

Here's where you use the -1 hack.

I hate to suggest this but I wonder if the propertySpecificKeyframes list could be indexed from 1. Or hold optionals?

Also, there is a nice way to initialize offset immediately:

auto offset = [&] () -> double {
  if (keyframeIndex == -1)
    return i ? 1 : 0;
  return m_keyframes[keyframeIndex].key();
}();

> Source/WebCore/animation/KeyframeEffect.cpp:780
> +            if (indexOfFirstKeyframeToAddToIntervalEndpoints > -1) {

>= 0

Again, I don't like the -1. Can you use an optional<unsigned>?

> Source/WebCore/animation/KeyframeEffect.cpp:785
> +                intervalEndpoints.append(propertySpecificKeyframes[indexOfFirstKeyframeToAddToIntervalEndpoints + 1]);
> +            } else {
> +                intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset]);
> +                intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset + 1]);

Add ASSERT(indexOfLastKeyframeWithZeroOffset < propertySpecificKeyframes.size())

> Source/WebCore/animation/KeyframeEffect.cpp:795
> +            auto keyframeStyle = keyframeIndex == -1 ? &targetStyle : m_keyframes[keyframeIndex].style();

Another -1.

> Source/WebCore/platform/animation/TimingFunction.cpp:91
> +        // decrement current step by one.

Indent.

> Source/WebCore/platform/animation/TimingFunction.h:167
> +    bool isLinear() const
> +    {
> +        return (!m_x1 && !m_y1 && !m_x2 && !m_y2) || (m_x1 == 1.0 && m_y1 == 1.0 && m_x2 == 1.0 && m_y2 == 1.0);
> +    }

It's also linear if 0, 0, 1, 1

[Antoine Quint]

Created attachment 332292 [details]
Patch

[Antoine Quint]

(In reply to Dean Jackson from comment #9)
> Comment on attachment 332272 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332272&action=review
> 
> All good, but I think you need to use optionals instead of the -1s
> everywhere. We've had animation bugs before when using special values,
> because someone eventually uses them as a boolean.

I've changed the two Vector<int> to Vector<std::optional<size_t>>.

> > Source/WebCore/animation/AnimationEffect.cpp:283
> > +            bool goingFordwards = currentDirection() == AnimationEffect::ComputedDirection::Forwards;
> 
> Fordwards

Fixed in new patch.

> > Source/WebCore/animation/KeyframeEffect.cpp:691
> > +void KeyframeEffect::setAnimatedPropertiesInStyle(RenderStyle& targetStyle, double iterationProgress)
> 
> There is a fair bit of work in this method. I wonder if we could cache some
> of the computed keyframe lists, rather than computing them every time
> iterationProgress changes.
> 
> > Source/WebCore/animation/KeyframeEffect.cpp:700
> > +        // 1. If iteration progress is unresolved abort this procedure.
> 
> I assume this is impossible here?

Yes, this would be caught at call sites.

> > Source/WebCore/animation/KeyframeEffect.cpp:711
> > +        Vector<int> propertySpecificKeyframes;
> > +        for (size_t i = 0; i < m_keyframes.size(); ++i) {
> 
> Couldn't this be a Vector of keyframes, and use a for : loop?

This would mean dropping KeyframeList which is a large undertaking that I'm considering but is ought of the scope of this patch.

> > Source/WebCore/animation/KeyframeEffect.cpp:731
> > +            propertySpecificKeyframes.insert(0, -1);
> 
> I don't like this hack of inserting a -1.

Using std::nullopt now.

> > Source/WebCore/animation/KeyframeEffect.cpp:761
> > +            size_t indexOfLastKeyframeWithZeroOffset;
> 
> Initialize this to 0.

Done.

> > Source/WebCore/animation/KeyframeEffect.cpp:771
> > +                double offset;
> > +                if (keyframeIndex == -1)
> > +                    offset = i ? 1 : 0;
> > +                else {
> > +                    auto& keyframe = m_keyframes[keyframeIndex];
> > +                    offset = keyframe.key();
> > +                }
> 
> Here's where you use the -1 hack.
> 
> I hate to suggest this but I wonder if the propertySpecificKeyframes list
> could be indexed from 1. Or hold optionals?
> 
> Also, there is a nice way to initialize offset immediately:
> 
> auto offset = [&] () -> double {
>   if (keyframeIndex == -1)
>     return i ? 1 : 0;
>   return m_keyframes[keyframeIndex].key();
> }();

Done, nice.

> > Source/WebCore/animation/KeyframeEffect.cpp:785
> > +                intervalEndpoints.append(propertySpecificKeyframes[indexOfFirstKeyframeToAddToIntervalEndpoints + 1]);
> > +            } else {
> > +                intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset]);
> > +                intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset + 1]);
> 
> Add ASSERT(indexOfLastKeyframeWithZeroOffset <
> propertySpecificKeyframes.size())

I added ASSERT(indexOfLastKeyframeWithZeroOffset < propertySpecificKeyframes.size() - 1);

> > Source/WebCore/platform/animation/TimingFunction.cpp:91
> > +        // decrement current step by one.
> 
> Indent.

Fixed.

> > Source/WebCore/platform/animation/TimingFunction.h:167
> > +    bool isLinear() const
> > +    {
> > +        return (!m_x1 && !m_y1 && !m_x2 && !m_y2) || (m_x1 == 1.0 && m_y1 == 1.0 && m_x2 == 1.0 && m_y2 == 1.0);
> > +    }
> 
> It's also linear if 0, 0, 1, 1

Added that case too, nice.

[Antoine Quint]

Committed r227622: <https://trac.webkit.org/changeset/227622>