Closed Bug 1178662 Opened 9 years ago Closed 8 years ago

Make Animation.timeline writeable

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox42 --- affected
firefox49 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

(
URL
)

Details

Attachments

(7 files, 6 obsolete files)

part 1 - Add tests for setting the timeline of an animation
14.43 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review
MozReview Request: Bug 1178662 part 2 - Rename *NoUpdate function in Animation. r?birtles
58 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1178662 part 3 - Separate SetTimeline function in order to call from style. r?birtles
58 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1178662 part 4 - Implement the setting timeline procedure. r?birtles
58 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec. r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1178662 part 6 - Remove the Animation's w-p-f meta file associated setting the timeline. r?birtles
58 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1178662 part 7 - Remove relationship of timeline and animation when setting new timeline. r?birtles
58 bytes, text/x-review-board-request
birtles
: review+
Details 
      No description provided.
As part of working of document time, I take this bug. I'm going to separate patches as follow:
 * Add the test of null timeline.
 * Modify Animation.timeline for exposing.
 * Modify Animation WebIDL in order to make writable.

As mentioned on bug 1096776, I think we maybe should modify the relationship of Animation and Timeline when we set the new timeline. (See bug 1096776 comment #4, for more detail)

WIP:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efacc9d0385ea65be68229a44457362e78f1fb7d
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> As mentioned on bug 1096776, I think we maybe should modify the relationship
> of Animation and Timeline when we set the new timeline. (See bug 1096776
> comment #4, for more detail)
This problem is occurred when we allowed that setting the timeline except document timeline.
So I'll add these change into the bug 1267510.
Hi Brian.

I have two questions about this bug.
1) The source code comment[1] say that the parameter of 'update an animation's finished state' should be 'DidSeek' in current implementation. But Specification is 'NoSeek'. 
Is this comment correct? If so, I think we must modify the specification.

2) Same question as 1). The comment[2] say that we should call the PostUpdate except that caller is style. Now we call this function from style/nsAnimationManager and style/nsTransitionManager. Does this comment mean that we shouldn't call the PostUpdate function from these?

[1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/dom/animation/Animation.cpp#155
[1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/dom/animation/Animation.cpp#159
Flags: needinfo?(bbirtles)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #3)
> Hi Brian.
> 
> I have two questions about this bug.
> 1) The source code comment[1] say that the parameter of 'update an
> animation's finished state' should be 'DidSeek' in current implementation.
> But Specification is 'NoSeek'. 
> Is this comment correct? If so, I think we must modify the specification.

I spent quite a while looking into this. I think the spec and code are probably right and the comment is wrong.

For my own reference: the reason is that when we set a new timeline, we also preserve the current time when possible by storing it and then doing a seek after updating the timeline. The cases where this flag matters are when we are go from either (a) no active timeline -> active timeline, or (b) the reverse.

For (a) I think it should probably behave as if the animation got clamped at the end of its interval -- as opposed to seeking to the equivalent current time.

For (b) we need to preserve the invariant that we only set *either* the start time or the hold time when we have no timeline. Currently the call to silently set the current time does that for us by clearing the start time. However, I think we probably want to preserve the start time in this case?

  i.e. I think we want
  anim.startTime = 3000;
  anim.timeline = timeline; // Suppose this triggers finishing behavior
                            // where we resolve the hold time
  anim.timeline = null;
  // At this point anim.startTime should still be 3000 as opposed to null

  On the other hand:
  anim.currentTime = 3000;
  anim.timeline = timeline; // Suppose this triggers finishing behavior
  anim.timeline = null;
  // At this point anim.startTime should still be null and anim.currentTime
  // should be 3000

I think we can probably achieve that by simply clearing the hold time if the start time is set (before we call the procedure to silently set the current time) however we might need to check for pending tasks to make sure we handle the pending state correctly. That's probably easier to verify once we have a rough implementation.

> 2) Same question as 1). The comment[2] say that we should call the
> PostUpdate except that caller is style. Now we call this function from
> style/nsAnimationManager and style/nsTransitionManager. Does this comment
> mean that we shouldn't call the PostUpdate function from these?

Yes. We should probably add a separate SetTimelineNoUpdate() method and call that from SetTimeline() and call that directly from style.

(Then we should go back and rename some of the *FromStyle / *FromJS / Do* methods to use the *NoUpdate naming when appropriate.)
Flags: needinfo?(bbirtles)
Attached patch bug1178662.investigation.patch (obsolete) — — Splinter Review 
Thanks Brian,

(In reply to Brian Birtles (:birtles) from comment #6)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #3)
> > Hi Brian.
> I think we can probably achieve that by simply clearing the hold time if the
> start time is set (before we call the procedure to silently set the current
> time) however we might need to check for pending tasks to make sure we
> handle the pending state correctly. That's probably easier to verify once we
> have a rough implementation.
I implemented to SetTimeline function follow the specification.

https://hg.mozilla.org/try/rev/b9b59acc1c0bf39fc7b49edc3e81aeee9cb57d15

The test was failed at following tests.
>  i.e. I think we want
>  anim.startTime = 3000;
>  anim.timeline = timeline; // Suppose this triggers finishing behavior
>                            // where we resolve the hold time
>  anim.timeline = null;
>  // At this point anim.startTime should still be 3000 as opposed to null

Perhaps, Current specification cleared the start time in 'silently set the current time' procedure if new timeline is null[1].
So I think that we need to set hold time to unresolved and set the 'updated hold time flag' to true in step 4 of 'Setting the timeline'[2], and we might need to add the condition 'if updated hold time flag is false' into step 6 of same procedure.

[1]https://w3c.github.io/web-animations/#setting-the-current-time-of-an-animation (at step 3)
[2]https://w3c.github.io/web-animations/#setting-the-timeline

I tried to implement above process, the result was succeed.
https://hg.mozilla.org/try/rev/bd0b4db9e5b5744cb13b7227b12c589c9042b91a

Brian,
I think that we need to add little change into current specification.
How do you feel about this change?
Attachment #8756197 - Flags: feedback?(bbirtles)
Comment on attachment 8756197 [details] [diff] [review]
bug1178662.investigation.patch

I've gone through how this should behave for all states and updated the spec accordingly with what I think we should do:

  https://github.com/w3c/web-animations/commit/a4cba458190a84d267a2b905de25a58c99d5eb26
Attachment #8756197 - Flags: feedback?(bbirtles)
MozReview-Commit-ID: 1xdKJONyFJV
Attachment #8756734 - Flags: review?(mantaroh)
Assignee: mantaroh → bbirtles
Comment on attachment 8756734 [details] [diff] [review]
part 1 - Add tests for setting the timeline of an animation

Review of attachment 8756734 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Brian.
I'll submit a patch of implementation.
Attachment #8756734 - Flags: review?(mantaroh) → review+
Assignee: bbirtles → mantaroh
Keywords: leave-open
Attachment #8756197 - Attachment is obsolete: true
Review commit: https://reviewboard.mozilla.org/r/56162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56162/
Attachment #8757744 - Flags: review?(bbirtles)
Attachment #8757745 - Flags: review?(bbirtles)
Attachment #8757746 - Flags: review?(bbirtles)
Attachment #8757747 - Flags: review?(bbirtles)
Attachment #8757748 - Flags: review?(bbirtles)
Attachment #8757749 - Flags: review?(bbirtles)
If we run following code, content process will crash.
------
anim1.timeline = timeline;
anim2.timeline = document.timeline;
anim2.timeline = timeline;
------

The AnimationTimeline has LinkedList variant. (called mAnimationOrder) And Animation is sub-class of LinkedListElement.
We will create the relationship of AnimationTimeline and Animation when calling the AnimationTimeline::NotifyAnimationUpdated. However we didn't remove these relation ship when setting new timeline.
So we should remove these relationship when setting new timeline object.

Review commit: https://reviewboard.mozilla.org/r/56172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56172/
Attachment #8757195 - Attachment is obsolete: true
Attachment #8757197 - Attachment is obsolete: true
Attachment #8757199 - Attachment is obsolete: true
Attachment #8757200 - Attachment is obsolete: true
Attachment #8757201 - Attachment is obsolete: true
Comment on attachment 8757746 [details]
MozReview Request: Bug 1178662 part 4 - Implement the setting timeline procedure. r?birtles

https://reviewboard.mozilla.org/r/56166/#review52802
Attachment #8757746 - Flags: review?(bbirtles) → review+
Comment on attachment 8757745 [details]
MozReview Request: Bug 1178662 part 3 - Separate SetTimeline function in order to call from style. r?birtles

https://reviewboard.mozilla.org/r/56164/#review52804

::: dom/animation/Animation.h:104
(Diff revision 1)
>    void SetId(const nsAString& aId);
>    KeyframeEffectReadOnly* GetEffect() const { return mEffect; }
>    void SetEffect(KeyframeEffectReadOnly* aEffect);
>    AnimationTimeline* GetTimeline() const { return mTimeline; }
>    void SetTimeline(AnimationTimeline* aTimeline);
> +  void SetTimelineNoUpdate(AnimationTimeline* aTimeline);

This declaration should probably be moved alongside CancelFromStyle.
Attachment #8757745 - Flags: review?(bbirtles) → review+
Comment on attachment 8757747 [details]
MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec.  r?smaug

https://reviewboard.mozilla.org/r/56168/#review52806

This needs review from a DOM peer, not me.
Attachment #8757747 - Flags: review?(bbirtles)
Comment on attachment 8757748 [details]
MozReview Request: Bug 1178662 part 6 - Remove the Animation's w-p-f meta file associated setting the timeline. r?birtles

https://reviewboard.mozilla.org/r/56170/#review52808
Attachment #8757748 - Flags: review?(bbirtles) → review+
Comment on attachment 8757749 [details]
MozReview Request: Bug 1178662 part 7 - Remove relationship of timeline and animation when setting new timeline. r?birtles

https://reviewboard.mozilla.org/r/56172/#review52810

I'd like to know the answer to bug 1096776 comment 11 before reviewing this, i.e. why is AnimationTimeline::NotifyAnimationUpdated not doing this for us? Is it not being called? Or does it not called in time?
Attachment #8757749 - Flags: review?(bbirtles)
Comment on attachment 8757744 [details]
MozReview Request: Bug 1178662 part 2 - Rename *NoUpdate function in Animation. r?birtles

https://reviewboard.mozilla.org/r/56162/#review52816

This isn't really what I had in mind, but I realised that what I had in mind might not work and this is still an improvement.
Attachment #8757744 - Flags: review?(bbirtles) → review+
https://reviewboard.mozilla.org/r/56172/#review52810

Thanks Brian,

We called to AnimationTimeline::NotifyAnimationUpdated in Animation. But I think that the way of calling this function is wrong.
We used following code when calling this function in Animation.[1] 

  mTimeline->NotifyAnimationUpdated(*this) 

If we called AnimationTimeline::NotifyAnimationUpdated function as above code, 'this instance' will be Animation's timeline object and aAnimation of parameter will be Animation in this function.
Then following condition code will ignored.[2] (aAnimtion.GetTimeline() = aAnimation.mTimeline = this)

  if (aAnimation.GetTimeline() && aAnimation.GetTimeline() != this) {
    aAnimation.GetTimeline()->RemoveAnimation(&aAnimation);
  }

So we didn't remove the relationship of Animation and Timeline, even if we called AnimationTimeline::NotifyAnimationUpdated function.

[1] https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/dom/animation/Animation.cpp#1024
[2] https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/dom/animation/AnimationTimeline.cpp#44
Comment on attachment 8757745 [details]
MozReview Request: Bug 1178662 part 3 - Separate SetTimeline function in order to call from style. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56164/diff/1-2/
Attachment #8757747 - Attachment description: MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec. r?birtles → MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec. r?smaug
Attachment #8757747 - Flags: review?(bugs)
Attachment #8757749 - Flags: review?(bbirtles)
Comment on attachment 8757746 [details]
MozReview Request: Bug 1178662 part 4 - Implement the setting timeline procedure. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56166/diff/1-2/
Comment on attachment 8757747 [details]
MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec.  r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56168/diff/1-2/
Comment on attachment 8757748 [details]
MozReview Request: Bug 1178662 part 6 - Remove the Animation's w-p-f meta file associated setting the timeline. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56170/diff/1-2/
Comment on attachment 8757749 [details]
MozReview Request: Bug 1178662 part 7 - Remove relationship of timeline and animation when setting new timeline. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56172/diff/1-2/
(In reply to Brian Birtles (:birtles) from comment #38)
> Comment on attachment 8757744 [details]
> MozReview Request: Bug 1178662 part 2 - Rename *NoUpdate function in
> Animation. r?birtles
> 
> https://reviewboard.mozilla.org/r/56162/#review52816
> 
> This isn't really what I had in mind, but I realised that what I had in mind
> might not work and this is still an improvement.
I filed the bug.(bug 1276581)
Comment on attachment 8757747 [details]
MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec.  r?smaug

https://reviewboard.mozilla.org/r/56168/#review52954

It is somewhat unclear to me why this is a good thing to do in the API, but I guess API designers had good use cases and all.
Attachment #8757747 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #46)
> It is somewhat unclear to me why this is a good thing to do in the API, but
> I guess API designers had good use cases and all.

Any specific concern here? This is part of the groundwork for being able to plug in different types of timelines: https://wiki.mozilla.org/Platform/Layout/Extended_Timelines
Is there a use case for moving an animation from one timeline to another (that is what the API change hints about). Why not just create a new Animation in the other timeline?
I definitely admit that I'm not familiar with all the use cases for which Animation API is designed for.
(In reply to Olli Pettay [:smaug] from comment #48)
> Is there a use case for moving an animation from one timeline to another
> (that is what the API change hints about). Why not just create a new
> Animation in the other timeline?

We haven't really worked out how the interaction will work, but the kind of use case we had in mind was a pull-to-update where the animation initially tracks the scroll/touch position, but after crossing some threshold we substitute in a clock-based timeline. Or at least that's the approach we have in mind to try initially (based on discussion with gfx team last December).
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #39)
> https://reviewboard.mozilla.org/r/56172/#review52810
> We called to AnimationTimeline::NotifyAnimationUpdated in Animation. But I
> think that the way of calling this function is wrong.
> We used following code when calling this function in Animation.[1] 
> 
>   mTimeline->NotifyAnimationUpdated(*this) 
> 
> If we called AnimationTimeline::NotifyAnimationUpdated function as above
> code, 'this instance' will be Animation's timeline object and aAnimation of
> parameter will be Animation in this function.
> Then following condition code will ignored.[2] (aAnimtion.GetTimeline() =
> aAnimation.mTimeline = this)
> 
>   if (aAnimation.GetTimeline() && aAnimation.GetTimeline() != this) {
>     aAnimation.GetTimeline()->RemoveAnimation(&aAnimation);
>   }
> 
> So we didn't remove the relationship of Animation and Timeline, even if we
> called AnimationTimeline::NotifyAnimationUpdated function.

Thanks, I totally forgot what NotifyAnimationUpdated was trying to do but I went through the related changeset[1] and I see that it is just ensuring that when we do add animations to a timeline's linked list, we make sure they're not already in another linked list. However, that assumes that the animation's timeline pointer has not yet been updated. I suspect that code never actually gets called given the way we're updating timelines and it could probably be replaced with an assertion. For now, however, this patch seems fine.

[1] https://hg.mozilla.org/mozilla-central/rev/61455317cc85
Comment on attachment 8757749 [details]
MozReview Request: Bug 1178662 part 7 - Remove relationship of timeline and animation when setting new timeline. r?birtles

https://reviewboard.mozilla.org/r/56172/#review53002
Attachment #8757749 - Flags: review?(bbirtles) → review+
Comment on attachment 8757744 [details]
MozReview Request: Bug 1178662 part 2 - Rename *NoUpdate function in Animation. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56162/diff/1-2/
Comment on attachment 8757745 [details]
MozReview Request: Bug 1178662 part 3 - Separate SetTimeline function in order to call from style. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56164/diff/2-3/
Comment on attachment 8757746 [details]
MozReview Request: Bug 1178662 part 4 - Implement the setting timeline procedure. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56166/diff/2-3/
Comment on attachment 8757747 [details]
MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec.  r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56168/diff/2-3/
Comment on attachment 8757748 [details]
MozReview Request: Bug 1178662 part 6 - Remove the Animation's w-p-f meta file associated setting the timeline. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56170/diff/2-3/
Comment on attachment 8757749 [details]
MozReview Request: Bug 1178662 part 7 - Remove relationship of timeline and animation when setting new timeline. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56172/diff/2-3/
Thanks Brian,

I rebased patches.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1277272
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: