Open
Bug 1474213
Opened 6 years ago
Updated 2 years ago
Clarify document usage in dom/animation
Categories
(Core :: DOM: Animation, enhancement, P3)
Core
DOM: Animation
Tracking
()
NEW
People
(Reporter: hiro, Unassigned)
Details
Attachments
(5 files)
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!
Reporter | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e173c78b13b0fdb6d4fae0686e6a492f5b5b6b1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•6 years ago
|
||
Hmm, I start thinking that PendingAnimationTracker should use the timeline document too. Brian?
Reporter | ||
Comment 8•6 years ago
|
||
(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•6 years ago
|
||
mozreview-review |
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•6 years ago
|
||
mozreview-review |
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.
Attachment #8990624 -
Flags: review?(bbirtles)
Comment 11•6 years ago
|
||
mozreview-review |
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.
Attachment #8990625 -
Flags: review?(bbirtles)
Comment 12•6 years ago
|
||
mozreview-review |
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.
Attachment #8990623 -
Flags: review?(bbirtles) → review+
Comment 13•6 years ago
|
||
mozreview-review |
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"
Attachment #8990626 -
Flags: review?(bbirtles)
Reporter | ||
Comment 14•6 years ago
|
||
(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.
Reporter | ||
Comment 15•6 years ago
|
||
(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.
Reporter | ||
Comment 16•6 years ago
|
||
(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•6 years ago
|
||
(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•6 years ago
|
||
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•6 years ago
|
||
mozreview-review |
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.
Attachment #8990622 -
Flags: review?(bbirtles)
Updated•6 years ago
|
Priority: -- → P3
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•