Bug 109010 – SVG paced value animations overwrite user-provided keyTimes

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Philip Rogers]

Created attachment 186766 [details]
Repro

Paced animations can synthesize keytimes in SVGAnimationElement::calculateKeyTimesForCalcModePaced() but this overwrites user-provided values. If an animation's calcMode is later changed to be something other than 'paced', m_keyTimes will contain synthesized values instead of the user-provided values.

[Said Abou-Hallawa]

Created attachment 425821 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 425821 [details]
Patch

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

> Source/WebCore/svg/SVGAnimationElement.cpp:385
> -        if (m_keyTimes[index] > percent)
> +        if (keyTimes()[index] > percent)

Can we store a reference instead of keep calling this function?

> Source/WebCore/svg/SVGAnimationElement.cpp:414
> +    float fromPercent = keyTimes()[index];
> +    float toPercent = keyTimes()[index + 1];

Ditto.

> Source/WebCore/svg/SVGAnimationElement.cpp:432
> +    if (calcMode() == CalcMode::Discrete && keyTimes().size() == 2)

Ditto.

> Source/WebCore/svg/SVGAnimationElement.cpp:489
> +        fromPercent = keyTimes()[index];
> +        toPercent = keyTimes()[index + 1];

Ditto.

> Source/WebCore/svg/SVGAnimationElement.cpp:537
> +        && (hasAttributeWithoutSynchronization(SVGNames::keyPointsAttr) && hasAttributeWithoutSynchronization(SVGNames::keyTimesAttr) && (keyTimes().size() < 2 || keyTimes().size() != m_keyPoints.size())))

Ditto.

> Source/WebCore/svg/SVGAnimationElement.cpp:552
> +            && (calcMode == CalcMode::Discrete || !keyTimes().size() || keyTimes().last() == 1)

Ditto.

> Source/WebCore/svg/SVGAnimationElement.cpp:554
> +            && (!hasAttributeWithoutSynchronization(SVGNames::keyPointsAttr) || (keyTimes().size() > 1 && keyTimes().size() == m_keyPoints.size()));

Ditto.

> Source/WebCore/svg/SVGAnimationElement.cpp:560
> +        m_animationValid = calcMode == CalcMode::Paced || !hasAttributeWithoutSynchronization(SVGNames::keyPointsAttr) || (keyTimes().size() > 1 && keyTimes().size() == m_keyPoints.size());

Ditto.

[Ryosuke Niwa]

*** Bug 222715 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

Created attachment 425824 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 425821 [details]
Patch

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

> Source/WebCore/svg/SVGAnimationElement.cpp:-352
> -    Vector<float> keyTimesForPaced;

This is the reason of the assertions in some svg layout tests. Appending times directly into m_keyTimesForPaced allowed partial calculation. If one of the distances is zero this function will return with incomplete m_keyTimesForPaced. To fix these assertions we need to either set m_keyTimesForPaced to a complete list or an empty list.

> LayoutTests/svg/animations/animate-calcMode-paced-overwrite-key-times.html:10
> +      setTimeout(function() {

This test was failing on the EWS because of this setTimeout(). We need to add waitUntilDone()/notifyDone() to properly synchronize the test.

[EWS]

Committed r275868 (236433@main): <https://commits.webkit.org/236433@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425824 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76580027>