Summary: | SVG paced value animations overwrite user-provided keyTimes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Rogers <pdr> | ||||||||
Component: | SVG | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dino, ews-watchlist, fmalita, gyuyoung.kim, rniwa, sabouhallawa, schenney, sergio, webkit-bug-importer, zimmermann | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 420+ | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=222715 | ||||||||||
Attachments: |
|
Created attachment 425821 [details]
Patch
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. *** Bug 222715 has been marked as a duplicate of this bug. *** Created attachment 425824 [details]
Patch
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. 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]. |
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.