Bug 142870 – CSS Animations with triggers should map scroll position to duration

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

[Dean Jackson]

Expose a prototype implementation of what will eventually be called animation-timebase, mapping the scroll location to the duration of an animation. This only applies if the animation has a start and end trigger.

[Radar WebKit Bug Importer]

<rdar://problem/20227244>

[Dean Jackson]

Created attachment 249067 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 249067 [details]
Patch

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

> Source/WebCore/css/CSSToStyleMap.cpp:539
> +        bool hasEnd = false;

I don't think you need the hasEnd variable.

> Source/WebCore/page/animation/AnimationBase.cpp:482
> +    // time is determined by the scroll position.

We should stop calling this elapsedTime

> Source/WebCore/page/animation/AnimationBase.cpp:710
> +        if (scrollTrigger.hasEndValue() && m_object) {

Can we make m_object a reference (later)?

> Source/WebCore/page/animation/AnimationBase.cpp:711
> +            float offset = m_object->view().frameView().scrollOffsetForFixedPosition().height().toFloat();

I think we should snapshot the scroll position when we snapshot the animation time. Fetching it all over the place feels error-prone.

> Source/WebCore/page/animation/AnimationBase.cpp:718
> +            return m_animation->duration() * (offset - startValue) / (endValue - startValue);

What if endValue == startValue?

Please add a test for this case.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:871
> +        ScrollAnimationTrigger& scrollTrigger = downcast<ScrollAnimationTrigger>(*anim->trigger().get());
> +        if (scrollTrigger.hasEndValue())

What about startValue?

[Dean Jackson]

Created attachment 249076 [details]
Patch

[Dean Jackson]

Comment on attachment 249067 [details]
Patch

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

>> Source/WebCore/css/CSSToStyleMap.cpp:539
>> +        bool hasEnd = false;
> 
> I don't think you need the hasEnd variable.

I removed it but it means I have two calls to setTrigger, each side of the conditional.

>> Source/WebCore/page/animation/AnimationBase.cpp:482
>> +    // time is determined by the scroll position.
> 
> We should stop calling this elapsedTime

Agreed. I'll do that in a followup patch.

>> Source/WebCore/page/animation/AnimationBase.cpp:710
>> +        if (scrollTrigger.hasEndValue() && m_object) {
> 
> Can we make m_object a reference (later)?

Yes.

>> Source/WebCore/page/animation/AnimationBase.cpp:711
>> +            float offset = m_object->view().frameView().scrollOffsetForFixedPosition().height().toFloat();
> 
> I think we should snapshot the scroll position when we snapshot the animation time. Fetching it all over the place feels error-prone.

Done

>> Source/WebCore/page/animation/AnimationBase.cpp:718
>> +            return m_animation->duration() * (offset - startValue) / (endValue - startValue);
> 
> What if endValue == startValue?
> 
> Please add a test for this case.

OK!

[Dean Jackson]

Checking the patch on EWS before landing.

[WebKit Commit Bot]

Attachment 249076 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1241:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Committed r181778: <http://trac.webkit.org/changeset/181778>

[Alexey Proskuryakov]

(In reply to comment #8)
> Committed r181778: <http://trac.webkit.org/changeset/181778>

This broke animations/trigger-container-scroll-empty.html on Windows and EFL:

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=animations%2Ftrigger-container-scroll-empty.html