1474213 - Clarify document usage in dom/animation

Status: NEW

(Core :: DOM: Animation, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

As far as I can tell PendingAnimationTracker should be used with the rendered document, i.e. the document has the target element, whereas DocumentTimeline should be used with the document for the animation's timeline.  It was confusing me in bug 1472900, bug 1472922 AND bug 1466010!

Comment 1

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e173c78b13b0fdb6d4fae0686e6a492f5b5b6b1

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1474213 - Add assertions checking that PendingAnimationTracker is used for animations whose target element belongs to the same document of the PenginAnimationTracker.

Review commit: https://reviewboard.mozilla.org/r/255686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255686/

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1474213 - Rename KeyframeEffect::GetPresShell to KeyframeEffect::GetRenderedPresShell.

Review commit: https://reviewboard.mozilla.org/r/255688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255688/

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1474213 - Add an assertion in DocumentTimeline::NotifyAnimationUpdated checking the animation's timeline document is the same document of the DocumentTimeline.

Review commit: https://reviewboard.mozilla.org/r/255690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255690/

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1474213 - Add another assertion in DocumentTimeline::NotifyAnimationUpdated checking the animation's timeline is the DocumentTimeline.

Review commit: https://reviewboard.mozilla.org/r/255692/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255692/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1474213 - Add a test case checking animation playback events is fired on the timeline of the animation object.

In this test case, the animation object lives in the parent document and the
animation timeline is the parent document's one.  Whereas the rendered document
for the animation is the subframe one.  On Gecko, the subframe has no refresh
driver since its display style is 'none', which means in this test case if we
were using the rendered document timeline, we don't observe animation playback
events at all.

Review commit: https://reviewboard.mozilla.org/r/255694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255694/

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Hmm, I start thinking that PendingAnimationTracker should use the timeline document too. Brian?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Hmm, I start thinking that PendingAnimationTracker should use the timeline
> document too. Brian?

Hmm, I might be wrong.  PendingAnimationTracker should use the rendered document, i.e. current implementation is correct.  I am not 100% sure though, there may be some annoying edge cases though.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8990622 [details]
Bug 1474213 - Add assertions checking that PendingAnimationTracker is used for animations whose target element belongs to the same document of the PenginAnimationTracker.

https://reviewboard.mozilla.org/r/255686/#review262420

::: dom/animation/Animation.h:393
(Diff revision 1)
> +  /**
> +   * Returns the document associated with the animation target element.
> +   */
> +  nsIDocument* GetRenderedDocument() const;

Returns the composed (flattened) document associated with the target element of the animation's target effect, if any.

::: dom/animation/PendingAnimationTracker.cpp:32
(Diff revision 1)
> +             "Should be used for animations that the target element belongs to "
> +             "the same document");

Pending animations should be animating content in the same document as this tracker is associated with

::: dom/animation/PendingAnimationTracker.cpp:46
(Diff revision 1)
> +  MOZ_ASSERT(aAnimation.GetRenderedDocument() &&
> +               aAnimation.GetRenderedDocument() == mDocument,
> +             "Should be used for animations that the target element belongs to "
> +             "the same document");

Is this right? If we change the target element of an effect whose animation is in the tracker, do we drop it from the tracker? From scanning the source, I don't see that happening.

What I mean is wouldn't this assertion fail if we did:

  const anim = element.animate(...);
  // Now play-pending
  anim.effect.target = null;
  // Animation::GetRenderedDocument() will now return nullptr
  anim.cancel(); // Triggers a call to RemovePending
  // Assertion fails

Is that right?

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8990624 [details]
Bug 1474213 - Add an assertion in DocumentTimeline::NotifyAnimationUpdated checking the animation's timeline document is the same document of the DocumentTimeline.

https://reviewboard.mozilla.org/r/255690/#review262428

::: dom/animation/DocumentTimeline.cpp:136
(Diff revision 1)
> +  MOZ_ASSERT(aAnimation.GetTimelineDocument() &&
> +               aAnimation.GetTimelineDocument() == mDocument,

This seems a bit convoluted. GetTimelineDocument() looks up the document on the timeline.

Wouldn't it be better to assert that aAnimation.GetTimeline() == this

?

That said, AnimationTimeline::NotifyAnimationUpdated already ensures that *will* be the case and in fact it is written under the assumption that it may _not_ initially be the case.

So I think this assertion is not right.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8990625 [details]
Bug 1474213 - Add another assertion in DocumentTimeline::NotifyAnimationUpdated checking the animation's timeline is the DocumentTimeline.

https://reviewboard.mozilla.org/r/255692/#review262430

::: dom/animation/DocumentTimeline.cpp:136
(Diff revision 1)
> +  MOZ_ASSERT(aAnimation.GetTimeline() == this,
> +             "Should be used for animations that the animation's timeline "
> +             "is this DocumentTimeline");

As with the previous patch, I don't think this assertion holds -- AnimationTimeline::NotifyAnimationUpdated specifically handles this case when it doesn't.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8990623 [details]
Bug 1474213 - Rename KeyframeEffect::GetPresShell to KeyframeEffect::GetRenderedPresShell.

https://reviewboard.mozilla.org/r/255688/#review262424

I perhaps find this a little less clear since "PresShell" already suggests rendering to me, but I don't mind either way.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8990626 [details]
Bug 1474213 - Add a test case checking animation playback events is fired on the timeline of the animation object.

https://reviewboard.mozilla.org/r/255694/#review262434

This seems fine but I'm curious if setting display:none actually makes the timeline become null

::: dom/animation/test/mozilla/test_update-and-send-events.html:12
(Diff revision 1)
> +// This is a variant of update-and-send-events.html in web platform test but
> +// for gecko specific.

This is a Gecko-specific variant of update-and-send-events.html from web-platform-tests/web-animations/timing-model/timelines/

::: dom/animation/test/mozilla/test_update-and-send-events.html:20
(Diff revision 1)
> +  // Make the iframe timeline null.
> +  iframe.style.display = 'none';

Does this actually make the timeline null? Or simply stop it from progressing?

::: dom/animation/test/mozilla/test_update-and-send-events.html:26
(Diff revision 1)
> +  const div = iframe.contentDocument.createElement('div');
> +  iframe.contentDocument.body.appendChild(div);

(If we could land this in wpt we could use createDiv and pass in the doc I guess. I think jgraham mentioned a way of putting vendor-specific tests in wpt but I can't seem to find it now.)

::: dom/animation/test/mozilla/test_update-and-send-events.html:40
(Diff revision 1)
> +}, 'Fires animation playback events on the timeline that the animation ' +
> +   'timeline belongs to');

I don't understand this sentence. Is it supposed to be something like:

"Animation playback events are queued on the document associated with the animation's timeline"

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #9)
> Comment on attachment 8990622 [details]
> Bug 1474213 - Add assertions checking that PendingAnimationTracker is used
> for animations whose target element belongs to the same document of the
> PenginAnimationTracker.
> 
> https://reviewboard.mozilla.org/r/255686/#review262420
> 
> ::: dom/animation/Animation.h:393
> (Diff revision 1)
> > +  /**
> > +   * Returns the document associated with the animation target element.
> > +   */
> > +  nsIDocument* GetRenderedDocument() const;
> 
> Returns the composed (flattened) document associated with the target element
> of the animation's target effect, if any.
> 
> ::: dom/animation/PendingAnimationTracker.cpp:32
> (Diff revision 1)
> > +             "Should be used for animations that the target element belongs to "
> > +             "the same document");
> 
> Pending animations should be animating content in the same document as this
> tracker is associated with
> 
> ::: dom/animation/PendingAnimationTracker.cpp:46
> (Diff revision 1)
> > +  MOZ_ASSERT(aAnimation.GetRenderedDocument() &&
> > +               aAnimation.GetRenderedDocument() == mDocument,
> > +             "Should be used for animations that the target element belongs to "
> > +             "the same document");
> 
> Is this right? If we change the target element of an effect whose animation
> is in the tracker, do we drop it from the tracker? From scanning the source,
> I don't see that happening.
> 
> What I mean is wouldn't this assertion fail if we did:
> 
>   const anim = element.animate(...);
>   // Now play-pending
>   anim.effect.target = null;
>   // Animation::GetRenderedDocument() will now return nullptr
>   anim.cancel(); // Triggers a call to RemovePending
>   // Assertion fails
> 
> Is that right?

It seems to me that we don't remove the animation from PendingAnimationTracker.  I will check it with writing a test case.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #10)
> Comment on attachment 8990624 [details]
> Bug 1474213 - Add an assertion in DocumentTimeline::NotifyAnimationUpdated
> checking the animation's timeline document is the same document of the
> DocumentTimeline.
> 
> https://reviewboard.mozilla.org/r/255690/#review262428
> 
> ::: dom/animation/DocumentTimeline.cpp:136
> (Diff revision 1)
> > +  MOZ_ASSERT(aAnimation.GetTimelineDocument() &&
> > +               aAnimation.GetTimelineDocument() == mDocument,
> 
> This seems a bit convoluted. GetTimelineDocument() looks up the document on
> the timeline.
> 
> Wouldn't it be better to assert that aAnimation.GetTimeline() == this
> 
> ?
> 
> That said, AnimationTimeline::NotifyAnimationUpdated already ensures that
> *will* be the case and in fact it is written under the assumption that it
> may _not_ initially be the case.
> 
> So I think this assertion is not right.

I don't quite understand what you mean.  What I am concerned is that, in future, even if  project fission or some refactoring or whatever happens, we should make sure DocumentTimeline is used for the document of the timeline of the animation.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #13)
> Comment on attachment 8990626 [details]
> Bug 1474213 - Add a test case checking animation playback events is fired on
> the timeline of the animation object.
> 
> https://reviewboard.mozilla.org/r/255694/#review262434
> 
> This seems fine but I'm curious if setting display:none actually makes the
> timeline become null

Yeah, that's wrong.  The pres shell is null actually.

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > Comment on attachment 8990624 [details]
> > Bug 1474213 - Add an assertion in DocumentTimeline::NotifyAnimationUpdated
> > checking the animation's timeline document is the same document of the
> > DocumentTimeline.
> > 
> > https://reviewboard.mozilla.org/r/255690/#review262428
> > 
> > ::: dom/animation/DocumentTimeline.cpp:136
> > (Diff revision 1)
> > > +  MOZ_ASSERT(aAnimation.GetTimelineDocument() &&
> > > +               aAnimation.GetTimelineDocument() == mDocument,
> > 
> > This seems a bit convoluted. GetTimelineDocument() looks up the document on
> > the timeline.
> > 
> > Wouldn't it be better to assert that aAnimation.GetTimeline() == this
> > 
> > ?
> > 
> > That said, AnimationTimeline::NotifyAnimationUpdated already ensures that
> > *will* be the case and in fact it is written under the assumption that it
> > may _not_ initially be the case.
> > 
> > So I think this assertion is not right.
> 
> I don't quite understand what you mean.  What I am concerned is that, in
> future, even if  project fission or some refactoring or whatever happens, we
> should make sure DocumentTimeline is used for the document of the timeline
> of the animation.

AnimationTimeline::NotifyAnimationUpdated has the following body:

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

That is, it assumes it is possible that aAnimation.GetTimeline() != this

But in this patch we assert that:

  aAnimation.GetTimelineDocument() && aAnimation.GetTimelineDocument() == mDocument

Given that GetTimelineDocument(), when not null, is equivalent to GetTimeline()->GetDocument() this is equivalent to asserting

  aAnimationTimeline.GetTimeline() && aAnimation.GetTimeline() == this

So either:

(a) This assertion is wrong, or
(b) We don't need the code in AnimationTimeline::NotifyAnimationUpdated, or
(c) The code in AnimationTimeline::NotifyAnimationUpdated is not needed for DocumentTimelines but is needed for other timelines

I'm currently digging through the mercurial history to work out why we added that handling to AnimationTimeline::NotifyAnimationUpdated

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I think the original idea was that animations don't explicitly register with at timeline (they just say, "I need an update next tick") and therefore they shouldn't need to explicitly unregister when they start following a different timeline.

However, I think in bug 1178662 Mantaroh discovered that if he didn't explicitly unregister when changing timelines bad things happen so he made it explicitly drop the connection. So perhaps the code in AnimationTimeline::NotifyAnimationUpdated is not needed anymore.

(In any case I'd rather we just have the assertion comparing GetTimeline() == this and not check the documents.)

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8990622 [details]
Bug 1474213 - Add assertions checking that PendingAnimationTracker is used for animations whose target element belongs to the same document of the PenginAnimationTracker.

https://reviewboard.mozilla.org/r/255686/#review262996

Clearing review for now until we fix the problem with the added assertion being likely to fail.

::: dom/animation/Animation.h:393
(Diff revision 1)
> +  /**
> +   * Returns the document associated with the animation target element.
> +   */
> +  nsIDocument* GetRenderedDocument() const;

Sorry, just to clarify, this comment was about revised wording for the comment text.

::: dom/animation/PendingAnimationTracker.cpp:32
(Diff revision 1)
> +             "Should be used for animations that the target element belongs to "
> +             "the same document");

Likewise here, revised wording for the assertion text.