Closed Bug 1134163 Opened 10 years ago Closed 9 years ago

Delay the dispatch of the animationstart event until the refresh driver tick after the animation starts

Categories

(Core :: DOM: Core & HTML, defect)

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

Tracking

()

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

People

(Reporter: jwatt, Assigned: mantaroh)

References

(Depends on 1 open bug)

Details

(Whiteboard: [tw-dom])

Attachments

(2 files, 4 obsolete files)

MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles
58 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1134163 - Part4. Add tests of getComputedStyle with seeking. r?birtles
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 927349 changed a bunch of things. When a refresh driver tick takes us into an animation, we delay the animation by shifting the animation to start at the moment painting for that tick finishes. Only then do we know the start time of the animation player, so we have to wait until the next tick (the next available time when we can run script) to resolve the AnimationPlayer's ready Promise. Bug 927349 did not change handling of the animationstart event though. We still dispatch the animationstart event during handling of the first tick (when style resolution realizes an animation should start). This means that we dispatch the animationstart event during the tick before the tick when we resolve the ready Promise. That seems bad in itself, but it also means that when the animationstart event is dispatched the AnimationPlayer's startTime is unresolved (null). It seems to me that the animationstart event should dispatch (immediately) after the ready Promise is resolved. The one undesirable thing about dispatching notifications on the "tick after" is that animation may visually start before script is notified of it. OMTA makes it impossible for script to exactly synchronize with animations anyway though.
I looked into this a little bit in bug 1180125 comment 8.
Assignee: nobody → mantaroh
The previous patch isn't following CSS Animation specification[1]. > "If there is an ‘animation-delay’ then this event will fire once the delay period has expired." So I tried to change the event timing of animation start. [1] https://www.w3.org/TR/css3-animations/#AnimationEvent-types Current implement seems queue the animationstart event when building the animation, and this event fired when next tick. =========================================== (Frame) : Behavior 1 : Build the animation[2] 2 : Tick Dispatch Events[3] =========================================== [2] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsAnimationManager.cpp#665 [3] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp#1709 So I think that we had better consider the condition of pending state when firing startanimation event like Animation.ready promise[4]. [4] https://dxr.mozilla.org/mozilla-central/source/dom/animation/Animation.cpp#495
Attachment #8738061 - Attachment is obsolete: true
Comment on attachment 8738426 [details] [diff] [review] [Investigation] Change event timing of animationstart. Hi brian. I changed the event timing of animationstart in order to queue this event after the animation is actually ready to run. Do you know the better way to detect that the animation is ready to run? Thanks.
Attachment #8738426 - Flags: feedback?(bbirtles)
Attachment #8738426 - Flags: feedback?(bbirtles)
We should consider to following points. 1. Several test supposed that 'animationstart' event fired immediately. (see layout/style/test/test_animations.html) 2. If animation have a following situation, 'animationstart' and 'animationiteration' event will happen at same tick. - animation have a iteration and too short duration. - animation have a negative delay nearly iteration end position, (e.g. animation have a 'anim 0.001s 3' or 'anim 1s -0.999s'.) At above situation, we will fire the 'startanimation' in current implementation. So we should modify the above tests and modify animation event queue mechanism in order to queueable multi event on same tick.
In reply to Mantaroh Yoshinaga[:mantaroh] from comment #7) > 2. If animation have a following situation, 'animationstart' and > 'animationiteration' event will happen at same tick. > - animation have a iteration and too short duration. > - animation have a negative delay nearly iteration end position, > (e.g. animation have a 'anim 0.001s 3' or 'anim 1s -0.999s'.) I think that this 'iteration' event will not fire in above situation. If the animation have a thin duration and multiple iterations, the 'iteration' event will not fire when handling to multiple iteration at same tick. Following diagram is show the above situation. (e.g 'anim 0.01s 3') -------------------------------------------------------- <- iteration 0 -><- iteration 1 -><- iteration 2 -> <---- (1)tick ----><---- (2)tick ----><---- (3)tick ----><---- (4)tick ----> (1) Any events won't fire. (2) The animationstart event will fire. (3) The animationiteration event will fire. (4) The animationedn event will fire. -------------------------------------------------------- Example page is as follow. If tick process is faster than animation duration, the number of 'animationiteration' event will be twice. http://codepen.io/mantaroh/pen/RaJddz If we modified delaying the 'animationstart' event, this 'iteration' event won't fire when same situation more than ever before. Because 'animationstart' event will wait for pending tasks. https://treeherder.mozilla.org/#/jobs?repo=try&revision=51a91535a0182aa42f7089b92888b4e30f0ca081 In CSS-Animation specification[1], 'animationiteration' event defined as follow. >'The animationiteration event occurs at the end of each iteration of an animation, >except when an animationend event would fire at the same time. ' [1] https://drafts.csswg.org/css-animations-1/#event-animationevent
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #8) > In CSS-Animation specification[1], 'animationiteration' event defined as > follow. > >'The animationiteration event occurs at the end of each iteration of an animation, > >except when an animationend event would fire at the same time. ' > > [1] https://drafts.csswg.org/css-animations-1/#event-animationevent Hi Brian, Some 'iteration' events are ignored in special case.(see comment #8) However CSS Animation event does not define those behavior. Is this all right? Other browser's behavior is as follow. (1) Chrome(52) 'iteration' will not fire with thin duration value. (2) Internet Explorer(11) 'iteration' will fire any duration value. (Even if duration is 1ms.) (3) Edge 'iteration' will fire any duration value. (Even if duration is 1ms.) (4) Safari(9.1) 'iteration' will not fire with thin duration value. Thanks.
Flags: needinfo?(bbirtles)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #9) > (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #8) > > In CSS-Animation specification[1], 'animationiteration' event defined as > > follow. > > >'The animationiteration event occurs at the end of each iteration of an animation, > > >except when an animationend event would fire at the same time. ' > > > > [1] https://drafts.csswg.org/css-animations-1/#event-animationevent > Hi Brian, > Some 'iteration' events are ignored in special case.(see comment #8) > However CSS Animation event does not define those behavior. Is this all > right? Yes, I've tried to get that clarified before[1] but it didn't get anywhere. Now that I'm the editor of that spec I guess I can make it happen. I filed [2] for this. [1] https://lists.w3.org/Archives/Public/www-style/2014Sep/0055.html [2] https://github.com/w3c/csswg-drafts/issues/106 > Other browser's behavior is as follow. > (1) Chrome(52) > 'iteration' will not fire with thin duration value. > (2) Internet Explorer(11) > 'iteration' will fire any duration value. (Even if duration is 1ms.) > (3) Edge > 'iteration' will fire any duration value. (Even if duration is 1ms.) > (4) Safari(9.1) > 'iteration' will not fire with thin duration value. Yes, and it's also possibly to hang Edge by simply creating an animation with duration 0.000000001s and infinite duration. So I don't think we want to spec the Edge behavior.
Flags: needinfo?(bbirtles)
Note: This patch contain the bug fix of mis-commit.(bug 1260034) In bug 1260034, I injected the investigation code about this bug incorrectly. https://hg.mozilla.org/mozilla-central/rev/1b9787fb3e8b So I removed this code and modified the test code. Review commit: https://reviewboard.mozilla.org/r/48349/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48349/
Thanks Brian. (In reply to Brian Birtles (:birtles) from comment #10) > Yes, and it's also possibly to hang Edge by simply creating an animation > with duration 0.000000001s and infinite duration. So I don't think we want > to spec the Edge behavior. I agree with you. Those event should not not have to depend on internal state of user agent. I think that the event should have to depend on something which rendered actually. I wrote the patch which modify the 'animationstart' event timing. This modification is using the 'Animation::mPendingState' when queueing the event. Could you please confirm this patch?
Attachment #8738426 - Attachment is obsolete: true
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles https://reviewboard.mozilla.org/r/48347/#review45111 Looks good but I'd like to see if we can: * drop making QueueEvents virtual * make the logic in CSSAnimation::QueueEvents slightly easier to follow ::: dom/animation/Animation.h:432 (Diff revision 1) > // yet to be created. This is not set when mFinished is rejected since > // in that case mFinished is immediately reset to represent a new current > // finished promise. > bool mFinishedIsResolved; > > + virtual void QueueEvents(); See below, I'm not sure we need this. ::: dom/animation/Animation.cpp:508 (Diff revision 1) > mPendingReadyTime.SetValue(std::min(mTimeline->GetCurrentTime().Value(), > mPendingReadyTime.Value())); > FinishPendingAt(mPendingReadyTime.Value()); > mPendingReadyTime.SetNull(); > + > + QueueEvents(); Why do we need this? This is in Animation::Tick, which, for a CSS animation will be called from CSSAnimation::Tick() which calls CSSAnimation::QueueEvents, right? ::: layout/style/nsAnimationManager.cpp:196 (Diff revision 1) > bool wasActive = mPreviousPhaseOrIteration != PREVIOUS_PHASE_BEFORE && > - mPreviousPhaseOrIteration != PREVIOUS_PHASE_AFTER; > + mPreviousPhaseOrIteration != PREVIOUS_PHASE_AFTER && > + mPendingState == PendingState::NotPending; Strictly speaking, I think we shouldn't check mPendingState here. 'wasActive' is purely about the *past* state, not the current state. So we should set it accurately even if mPendingState is 'PlayPending' or something like that. Instead, we should add the checks for pending state later, I think. It's not a big deal, but I think we could make this slightly easier to read if we drop the check for mPendingState here. ::: layout/style/nsAnimationManager.cpp:213 (Diff revision 1) > if (computedTiming.mPhase == ComputedTiming::AnimationPhase::Before) { > mPreviousPhaseOrIteration = PREVIOUS_PHASE_BEFORE; > - } else if (computedTiming.mPhase == ComputedTiming::AnimationPhase::Active) { > + } else if (computedTiming.mPhase == ComputedTiming::AnimationPhase::Active && > + mPendingState == PendingState::NotPending) { > mPreviousPhaseOrIteration = computedTiming.mCurrentIteration; > } else if (computedTiming.mPhase == ComputedTiming::AnimationPhase::After) { > mPreviousPhaseOrIteration = PREVIOUS_PHASE_AFTER; > } What if we wrapped this whole block in a check? // If we are pending don't update the previous state if (mPendingState == PendingState::NotPending) { ... }
Attachment #8744161 - Flags: review?(bbirtles)
Comment on attachment 8744162 [details] MozReview Request: Bug 1134163 - Part2.Modify animation tests which rely on animationstart timing. r?birtles https://reviewboard.mozilla.org/r/48349/#review45119 I'll have another look at this once we separate out the fallout from bug 1260084. ::: dom/events/test/test_legacy_event.html:35 (Diff revision 1) > + legacy_name: "webkitTransitionEnd", > + modern_name: "transitionend", > + trigger_event: triggerShortTransition, > + }, > + { > + legacy_name: "webkitAnimationStart", > + modern_name: "animationstart", > + trigger_event: triggerShortAnimation, > + }, > + { > + legacy_name: "webkitAnimationEnd", > + modern_name: "animationend", > + trigger_event: triggerShortAnimation, > + }, > + { As discussed, we should fix the mistaken commit from bug 1260084 in a separate bug. ::: dom/events/test/test_legacy_event.html:91 (Diff revision 1) > - dump("************ Add Animation! **********"); > + node.style.animation = "anim1 0.1s linear 2"; > - node.style.animation = "anim1 1s -0.999s linear 2"; This doesn't match the comment. ::: layout/style/test/test_animations.html:1194 (Diff revision 1) > +advance_clock(0); > +advance_clock(0); // For animationstart delay. Are two ticks needed? If so, we should document why. e.g. animationstart events are not dispatched until the animation has actually begun moving. Typically the sequence is: Initially: a) Animation is created b) Paint c) Pending start time of animation is established Next refresh driver tick: d) Animation start time is set e) Events are queued Next refresh driver tick: f) Queued events are run But we need to check if that is actually what happens. I really hope (e) and (f) happen in the same frame. ::: layout/style/test/test_animations_event_order.html:221 (Diff revision 1) > advance_clock(0); > +advance_clock(0); // For animationstart delay. > advance_clock(5000); > +advance_clock(0); // For animationstart delay. Is this last line actually needed? I thought we would have already resolved the start time at the beginning of the delay?
Attachment #8744162 - Flags: review?(bbirtles)
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/1-2/
Attachment #8744161 - Flags: review?(bbirtles)
Attachment #8744162 - Flags: review?(bbirtles)
Comment on attachment 8744162 [details] MozReview Request: Bug 1134163 - Part2.Modify animation tests which rely on animationstart timing. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48349/diff/1-2/
https://reviewboard.mozilla.org/r/48349/#review45119 > As discussed, we should fix the mistaken commit from bug 1260084 in a separate bug. Thanks Brian. I fixed in bug 1266650. > Are two ticks needed? If so, we should document why. > > e.g. > > animationstart events are not dispatched until the animation has actually begun moving. Typically the sequence is: > > Initially: > a) Animation is created > b) Paint > c) Pending start time of animation is established > > Next refresh driver tick: > d) Animation start time is set > e) Events are queued > > Next refresh driver tick: > f) Queued events are run > > But we need to check if that is actually what happens. I really hope (e) and (f) happen in the same frame. We don't need to two tick always. However when calling AdvanceTimeAndRefresh function, animation style may not apply, So some tests will fail. AdvanceTimeAndRefresh function is calling Tick, but Animation::Tick won't call when we didn't flushing animation style. In previous implementation, animation events aren't relate with ticking.(firing the animation event when calling Animation::Build.) e.g. Previous test sample.(Console log won't display) div.style.animation = "anim 1s -1s 2"; div.addEventListener("animationstart", ()=>{ console.log("animationstart"); } SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh(0); So I modified some tests using to getComputedStyle. e.g. Modified above sample. div.style.animation = "anim 1s -1s 2"; getComputedStyle(div).animationName; // force flush animation div.addEventListener("animationstart", ()=>{ console.log("animationstart"); } SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh(0);
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #18) > https://reviewboard.mozilla.org/r/48349/#review45119 > > > As discussed, we should fix the mistaken commit from bug 1260084 in a separate bug. > > Thanks Brian. > I fixed in bug 1266650. > > > Are two ticks needed? If so, we should document why. > > > > e.g. > > > > animationstart events are not dispatched until the animation has actually begun moving. Typically the sequence is: > > > > Initially: > > a) Animation is created > > b) Paint > > c) Pending start time of animation is established > > > > Next refresh driver tick: > > d) Animation start time is set > > e) Events are queued > > > > Next refresh driver tick: > > f) Queued events are run > > > > But we need to check if that is actually what happens. I really hope (e) and (f) happen in the same frame. > > We don't need to two tick always. However when calling AdvanceTimeAndRefresh > function, animation style may not apply, So some tests will fail. > AdvanceTimeAndRefresh function is calling Tick, but Animation::Tick won't > call when we didn't flushing animation style. In previous implementation, > animation events aren't relate with ticking.(firing the animation event when > calling Animation::Build.) > > e.g. Previous test sample.(Console log won't display) > div.style.animation = "anim 1s -1s 2"; > div.addEventListener("animationstart", ()=>{ console.log("animationstart"); } > SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh(0); > > So I modified some tests using to getComputedStyle. > > e.g. Modified above sample. > div.style.animation = "anim 1s -1s 2"; > getComputedStyle(div).animationName; // force flush animation > div.addEventListener("animationstart", ()=>{ console.log("animationstart"); } > SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh(0); Great! Thanks for that analysis! So, for my own summary, as I understand it, the particular issue is that in the first case, we would build the animation as part of flushing style during nsRefreshDriver::Tick and *also* dispatch the event then. Now, however, we would still build the animation as part of flushing style during nsRefreshDriver::Tick but we wouldn't queue the event then (the animation would still be pending at that point). However, if we forcefully flush style we cause the animations to be built within the current (first) frame. They'll still be pending however. But on the next call to nsDOMWindowUtils::AdvanceTimeAndRefresh we will trigger those pending animations, then, as part of the tick that takes place then we'll queue and dispatch their animationstart events.
Attachment #8744161 - Flags: review?(bbirtles) → review+
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles https://reviewboard.mozilla.org/r/48347/#review45695 r=me with comments addressed but we also need to change CSSAnimation::ElapsedTimeToTimeStamp either in this patch or a separate patch. At very least we need to update the comment there since if refers to this bug. Also, if we are confident (after looking at all the code paths leading to that function) that mStartTime will now longer be null, we should drop the check for mStartTime.IsNull() and add an assertion about that. ::: dom/interfaces/base/nsIDOMWindowUtils.idl:1416 (Diff revision 2) > - * Note that this affects other connected docshells of the same type > - * in the same docshell tree, such as parent frames. > + * Note: > + * - This affects other connected docshells of the same type in the > + * same docshell tree, such as parent frames. > + * - If you will determine animationstart event using this function, > + * you should apply animation style. > + * e.g. using getComputedStyle. I think we probably don't need this change. The fact that you need to call getComputedStyle to get animationstart events to occur in a fewer number of frames is not really specific to this method but just a fact of how animations work now. ::: layout/style/nsAnimationManager.cpp:212 (Diff revision 2) > computedTiming.mPhase == ComputedTiming::AnimationPhase::Before); > > MOZ_ASSERT(!skippedActivePhase || (!isActive && !wasActive), > "skippedActivePhase only makes sense if we were & are inactive"); > > + if (mPendingState == PendingState::NotPending) { Let's add a comment here: // If the animation is pending, keep the same "previous phase or iteration"
Comment on attachment 8744162 [details] MozReview Request: Bug 1134163 - Part2.Modify animation tests which rely on animationstart timing. r?birtles https://reviewboard.mozilla.org/r/48349/#review45709 We need to go back and fix part 1 in order to get these tests to pass (we really should actually combine parts 1 and 2 or switch their order and mark the failing tests as todo). Also, perhaps in another patch we need to address this comments: https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/dom/animation/test/css-animations/file_animation-currenttime.html#28 https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/dom/animation/test/css-animations/file_animation-starttime.html#28 https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/dom/animation/test/css-transitions/file_animation-currenttime.html#22 https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/dom/animation/test/css-transitions/file_animation-starttime.html#22 ::: dom/events/test/test_legacy_event.html:76 (Diff revision 2) > -// This function triggers a long animation with two iterations, which is > -// *nearly* at the end of its first iteration. It will hit the end of that > -// iteration (firing an event) almost immediately, 1ms in the future. > +// This function trigger a short animation with two iterations, which is > +// *nearly* at the end of its first iteration. It will hit the end of that > +// iteration (firing an event) almost immediately, 100ms in the futre. > // > -// NOTE: It's important that this animation have a *long* duration. If it were > -// short (e.g. 1ms duration), then we might jump past all its iterations in > -// a single refresh-driver tick. And if that were to happens, we'd *never* fire > -// any animationiteration events -- the CSS Animations spec says this event > -// must not be fired "...when an animationend event would fire at the same time" > -// (which would be the case in this example with a 1ms duration). So, to make > +// NOTE: It's imporant that this animation does not start *very nearly* at the > +// end of its first iteration (e.g. -0.999s delay). If it were very near at the > +// end of its first iteration, then we might skip first iteration event due to > +// delay the animation start event. (For detail, see bug 1134163.) > +unction triggerAnimationIteration(node) { > + node.style.animation = "anim1 1s linear 2"; I'm pretty sure this isn't right. Firstly, I don't think 'unction' is right. Secondly, I don't think we want to use such a short duration due to the reason in the original commit message. Instead, we should probably modify part 1 so that if mPreviousPhaseOrIteration == PREVIOUS_PHASE_BEFORE and computedTiming.mCurrentIteration > 0 we dispatch both animationstart and animationiteration. ::: layout/style/test/test_animations.html:1192 (Diff revision 2) > +cs.animationName; // force flush > +advance_clock(0); // For delay start of animation. How about: // flush styles so animation is created // complete pending animation start ::: layout/style/test/test_animations.html:1573 (Diff revision 2) > +cs.animationName; // force flush > advance_clock(0); // build animation // finish pending ::: layout/style/test/test_animations.html:1739 (Diff revision 2) > +cs.animationName; // force flush > advance_clock(0); // build animation // finish pending ::: layout/style/test/test_animations_event_order.html:95 (Diff revision 2) > document.createElement('div') ]; > divs.forEach((div, i) => { > gDisplay.appendChild(div); > div.setAttribute('style', 'animation: anim 10s'); > div.setAttribute('id', 'div' + i); > + getComputedStyle(div).animationName; // force flush. // trigger building of animation ::: layout/style/test/test_animations_event_order.html:128 (Diff revision 2) > divs[0].appendChild(divs[2]); > > divs.forEach((div, i) => { > div.setAttribute('style', 'animation: anim 10s'); > div.setAttribute('id', 'div' + i); > + getComputedStyle(div).animationName; // force flush // trigger building of animation ::: layout/style/test/test_animations_event_order.html:137 (Diff revision 2) > checkEventOrder([ divs[0], 'animationstart' ], > [ divs[2], 'animationstart' ], > [ divs[1], 'animationstart' ], > 'Simultaneous start on children'); > > -divs.forEach(div => div.remove()); > +divs.forEach((div)=>{ Nit: div => { i.e. No need for () around 'div'. Need space either side of '=>' ::: layout/style/test/test_animations_event_order.html:139 (Diff revision 2) > [ divs[1], 'animationstart' ], > 'Simultaneous start on children'); > > -divs.forEach(div => div.remove()); > +divs.forEach((div)=>{ > + div.remove(); > + getComputedStyle(div).animationName; // force flush Is this needed? ::: layout/style/test/test_animations_event_order.html:171 (Diff revision 2) > +getComputedStyle(divs[0]).animationName; // force flush > +getComputedStyle(divs[1]).animationName; // force flush // build animation // build animation ::: layout/style/test/test_animations_event_order.html:181 (Diff revision 2) > [ divs[0], '::before', 'animationstart' ], > [ divs[0], '::after', 'animationstart' ], > [ divs[1], 'animationstart' ], > 'Simultaneous start on pseudo-elements'); > > -divs.forEach(div => div.remove()); > +divs.forEach((div)=>{ Nit: likewise here, div => { ::: layout/style/test/test_animations_event_order.html:183 (Diff revision 2) > [ divs[1], 'animationstart' ], > 'Simultaneous start on pseudo-elements'); > > -divs.forEach(div => div.remove()); > +divs.forEach((div)=>{ > + div.remove(); > + getComputedStyle(div).animationName; // force flush Is this needed? The div is removed, it's animations will have been canceled. ::: layout/style/test/test_animations_event_order.html:225 (Diff revision 2) > gDisplay.appendChild(div); > div.style.animation = 'anim 4s 2, ' + // Repeat at t=4s > 'anim 10s 5s, ' + // Start at t=5s > 'anim 3s'; // End at t=3s > div.setAttribute('id', 'div'); > +getComputedStyle(div).animationName; // force flush // build animation ::: layout/style/test/test_animations_event_order.html:275 (Diff revision 2) > > div = document.createElement('div'); > gDisplay.appendChild(div); > div.style.animation = 'animA 10s, animB 5s, animC 5s 2'; > div.setAttribute('id', 'div'); > +getComputedStyle(div).animationName; // force flush // build animation
Attachment #8744162 - Flags: review?(bbirtles)
https://reviewboard.mozilla.org/r/48349/#review45709 As discussed, I'll modify those test as other patches. > I'm pretty sure this isn't right. Firstly, I don't think 'unction' is right. Secondly, I don't think we want to use such a short duration due to the reason in the original commit message. > > Instead, we should probably modify part 1 so that if mPreviousPhaseOrIteration == PREVIOUS_PHASE_BEFORE and computedTiming.mCurrentIteration > 0 we dispatch both animationstart and animationiteration. I modified QueueEvent function in order to fire 'animationstart' and 'animationiteration' events in same tick.
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/2-3/
Attachment #8744162 - Flags: review?(bbirtles)
Comment on attachment 8744162 [details] MozReview Request: Bug 1134163 - Part2.Modify animation tests which rely on animationstart timing. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48349/diff/2-3/
Attachment #8744161 - Flags: review+
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles https://reviewboard.mozilla.org/r/48347/#review45981 I want to have a quick look after factoring out the common code in QueueEvents. ::: layout/style/nsAnimationManager.cpp:155 (Diff revision 3) > + // If animation have a pending task, we ignore animation event until > + // complete those pending task. Nit: // If the animation is pending, we ignore animation events until we finish // pending. ::: layout/style/nsAnimationManager.cpp:213 (Diff revision 3) > bool skippedActivePhase = > (mPreviousPhaseOrIteration == PREVIOUS_PHASE_BEFORE && > computedTiming.mPhase == ComputedTiming::AnimationPhase::After) || > (mPreviousPhaseOrIteration == PREVIOUS_PHASE_AFTER && > computedTiming.mPhase == ComputedTiming::AnimationPhase::Before); > + bool skippedBeforePhase = Maybe we should call this skippedFirstIteration ::: layout/style/nsAnimationManager.cpp:215 (Diff revision 3) > + (mPreviousPhaseOrIteration == PREVIOUS_PHASE_BEFORE) && > + (computedTiming.mCurrentIteration > 0); Nit: I don't think we need the () around each of the sub-conditions here. ::: layout/style/nsAnimationManager.cpp:221 (Diff revision 3) > + (computedTiming.mCurrentIteration > 0); > > MOZ_ASSERT(!skippedActivePhase || (!isActive && !wasActive), > "skippedActivePhase only makes sense if we were & are inactive"); > > + // If the animation is pending, keep the same "previous phase or iteration", This comment is no longer necessary. ::: layout/style/nsAnimationManager.cpp:234 (Diff revision 3) > + StickyTimeDuration elapsedTime = > + std::min(StickyTimeDuration(InitialAdvance()), > + computedTiming.mActiveDuration); The indentation here seems to be wrong. ::: layout/style/nsAnimationManager.cpp:234 (Diff revision 3) > + StickyTimeDuration elapsedTime = > + std::min(StickyTimeDuration(InitialAdvance()), > + computedTiming.mActiveDuration); > + manager->QueueEvent(AnimationEventInfo(owningElement, owningPseudoType, > + eAnimationStart, mAnimationName, > + elapsedTime, > + ElapsedTimeToTimeStamp(elapsedTime), > + this)); This duplicates code from the skippedActivePhase branch. We should factor out the common code. (I imagine we could have an AutoTArray<Pair<EventMessage, StickyTimeDuration>, 2> that records the event message and elapsed time. Then we could just iterate over it and queue the needed events. We'd still have a bit of duplication for some of the elapsedTime calculations I suspect but we can fix that later if needed.)
Comment on attachment 8744162 [details] MozReview Request: Bug 1134163 - Part2.Modify animation tests which rely on animationstart timing. r?birtles https://reviewboard.mozilla.org/r/48349/#review45977 The title of this patch needs s/Mofify/Modify/ (I really wish MozReview made it easier to comment on the patch title / description!) ::: layout/style/test/test_animations.html:1193 (Diff revision 3) > // test large negative delay that causes the animation to start > // in the fourth iteration > new_div("animation: anim2 1s -3.6s ease-in 5 alternate forwards"); > listen(); // rely on no flush having happened yet > +cs.animationName; // flush styles so animation is created > +advance_clock(0); // compolete pending animation start Nit: complete
Attachment #8744162 - Flags: review?(bbirtles) → review+
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/3-4/
Attachment #8744162 - Attachment description: MozReview Request: Bug 1134163 - Part2.Mofify animation tests which rely on animationstart timing. r?birtles → MozReview Request: Bug 1134163 - Part2.Modify animation tests which rely on animationstart timing. r?birtles
Attachment #8744161 - Flags: review?(bbirtles)
Comment on attachment 8744162 [details] MozReview Request: Bug 1134163 - Part2.Modify animation tests which rely on animationstart timing. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48349/diff/3-4/
https://reviewboard.mozilla.org/r/48347/#review45981 Many thanks brian. I modified the QueueEvent in order to prevent duplation code.
Attachment #8744161 - Flags: review?(bbirtles)
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles https://reviewboard.mozilla.org/r/48347/#review46195 Nearly there, but I want to check those changes to the elapsedTime calculations. ::: layout/style/nsAnimationManager.h:266 (Diff revision 4) > + > + typedef Pair<EventMessage, StickyTimeDuration> EventPair; Could we move this typedef to an anonymous namespace block in nsAnimationManager.cpp? I don't think this needs to be visible to the outside world. We should also add a comment: // Pair of an event message and elapsed time used when determining the set of // events to queue. ::: layout/style/nsAnimationManager.cpp:230 (Diff revision 4) > } else if (computedTiming.mPhase == ComputedTiming::AnimationPhase::After) { > mPreviousPhaseOrIteration = PREVIOUS_PHASE_AFTER; > } > > - EventMessage message; > + AutoTArray<EventPair, 2> events; > + StickyTimeDuration initialAdvanceTime = StickyTimeDuration(InitialAdvance()); Nit: s/initialAdvanceTime/initialAdvance/ ? ::: layout/style/nsAnimationManager.cpp:237 (Diff revision 4) > + std::min(initialAdvanceTime, > + computedTiming.mActiveDuration))); This should be just initialAdvance I think. We only need this std::min(initialAdvance, activeDuration) behavior when we might have a situation where the end is before the start -- but that's clearly not the case here (otherwise we'd be in the skippedActivePhase branch). ::: layout/style/nsAnimationManager.cpp:244 (Diff revision 4) > + > + events.AppendElement(EventPair(eAnimationIteration, > + std::max(iterationStart, initialAdvanceTime))); > + } else if (!wasActive && isActive) { > + events.AppendElement(EventPair(eAnimationStart, > + std::max(iterationStart, initialAdvanceTime))); This should be just initialAdvanceTime (i.e. *not* std::max(iterationStart, initialAdvanceTime) iterationStart will be zero here and initialAdvanceTime will never be < 0 (see the definition in CSSAnimation::InitialAdvance). ::: layout/style/nsAnimationManager.cpp:246 (Diff revision 4) > + std::max(iterationStart, initialAdvanceTime))); > + } else if (!wasActive && isActive) { > + events.AppendElement(EventPair(eAnimationStart, > + std::max(iterationStart, initialAdvanceTime))); > } else if (wasActive && !isActive) { > - message = eAnimationEnd; > + events.AppendElement(EventPair(eAnimationEnd, computedTiming.mActiveDuration)); Some of these lines are > 80 chars. Please fix your editor settings. ::: layout/style/nsAnimationManager.cpp:251 (Diff revision 4) > // First notifying for start of 0th iteration by appending an > // 'animationstart': We don't really need this comment anymore I think. ::: layout/style/nsAnimationManager.cpp:257 (Diff revision 4) > - manager->QueueEvent(AnimationEventInfo(owningElement, owningPseudoType, > + > - eAnimationStart, mAnimationName, > - elapsedTime, > - ElapsedTimeToTimeStamp(elapsedTime), > - this)); > // Then have the shared code below append an 'animationend': This comment is no longer correct. We should just remove it. ::: layout/style/nsAnimationManager.cpp:264 (Diff revision 4) > + computedTiming.mActiveDuration)); > } else { > return; // No events need to be sent > } > > - StickyTimeDuration elapsedTime; > + for (const EventPair pair : events){ Can we use a const ref here instead?
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/4-5/
Attachment #8744161 - Flags: review?(bbirtles)
Comment on attachment 8744162 [details] MozReview Request: Bug 1134163 - Part2.Modify animation tests which rely on animationstart timing. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48349/diff/4-5/
Attachment #8744161 - Flags: review?(bbirtles)
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles https://reviewboard.mozilla.org/r/48347/#review46221 r=me with comments addressed ::: layout/style/nsAnimationManager.cpp:38 (Diff revision 5) > using mozilla::dom::KeyframeEffectReadOnly; > using mozilla::dom::CSSAnimation; > > +// Pair of an event message and elapsed time used when determining the set of > +// events to queue. > +typedef Pair<EventMessage, StickyTimeDuration> EventPair; Did putting it in an anonymous namespace block work? Nit: Space after this line. (Also, you could put it right before the method where we use it?) ::: layout/style/nsAnimationManager.cpp:236 (Diff revision 5) > > - EventMessage message; > + AutoTArray<EventPair, 2> events; > + StickyTimeDuration initialAdvance = StickyTimeDuration(InitialAdvance()); > + StickyTimeDuration iterationStart = computedTiming.mDuration * > + computedTiming.mCurrentIteration; > + StickyTimeDuration activeDuration = computedTiming.mActiveDuration; If you want to alias a variable to make line-wrapping easier, use a reference (probably a const-ref in this case). ::: layout/style/nsAnimationManager.cpp:241 (Diff revision 5) > + StickyTimeDuration activeDuration = computedTiming.mActiveDuration; > > - if (!wasActive && isActive) { > - message = eAnimationStart; > + if (skippedFirstIteration) { > + // Notify animationstart and animationiteration in same tick. > + events.AppendElement(EventPair(eAnimationStart, initialAdvance)); > + Nit: I think we can drop this line. ::: layout/style/nsAnimationManager.cpp:253 (Diff revision 5) > } else if (wasActive && isActive && !isSameIteration) { > - message = eAnimationIteration; > + events.AppendElement(EventPair(eAnimationIteration, iterationStart)); > } else if (skippedActivePhase) { > - // First notifying for start of 0th iteration by appending an > - // 'animationstart': > - StickyTimeDuration elapsedTime = > + events.AppendElement(EventPair(eAnimationStart, > + std::min(initialAdvance, activeDuration))); > + Nit: I think we can drop this line--it contains trailing whitespace anyway.
Attachment #8744161 - Flags: review+
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/5-6/
Comment on attachment 8744162 [details] MozReview Request: Bug 1134163 - Part2.Modify animation tests which rely on animationstart timing. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48349/diff/5-6/
https://reviewboard.mozilla.org/r/48347/#review46235 ::: layout/style/nsAnimationManager.cpp:241 (Diff revisions 5 - 6) > > AutoTArray<EventPair, 2> events; > StickyTimeDuration initialAdvance = StickyTimeDuration(InitialAdvance()); > StickyTimeDuration iterationStart = computedTiming.mDuration * > computedTiming.mCurrentIteration; > - StickyTimeDuration activeDuration = computedTiming.mActiveDuration; > + const StickyTimeDuration &activeDuration = computedTiming.mActiveDuration; & and * go with the type in Gecko code.[1] [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/6-7/
Comment on attachment 8744162 [details] MozReview Request: Bug 1134163 - Part2.Modify animation tests which rely on animationstart timing. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48349/diff/6-7/
https://reviewboard.mozilla.org/r/48347/#review46235 > & and * go with the type in Gecko code.[1] > > [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations Thanks Brian. I modified this declaration.
Keywords: leave-open
Keywords: checkin-needed
Hi Brian. (In reply to Brian Birtles (:birtles, away 3-5 May) from comment #21) > Comment on attachment 8744162 [details] > Also, perhaps in another patch we need to address this comments: > https://dxr.mozilla.org/mozilla-central/rev/ > fc15477ce628599519cb0055f52cc195d640dc94/dom/animation/test/css-animations/ > file_animation-currenttime.html#28 > https://dxr.mozilla.org/mozilla-central/rev/ > fc15477ce628599519cb0055f52cc195d640dc94/dom/animation/test/css-animations/ > file_animation-starttime.html#28 > https://dxr.mozilla.org/mozilla-central/rev/ > fc15477ce628599519cb0055f52cc195d640dc94/dom/animation/test/css-transitions/ > file_animation-currenttime.html#22 > https://dxr.mozilla.org/mozilla-central/rev/ > fc15477ce628599519cb0055f52cc195d640dc94/dom/animation/test/css-transitions/ > file_animation-starttime.html#22 I modified the file_animation-currenttime.html. I think that this tests contained several unrelated tests code. (e.g. playState.) Basically, I think that this tests should verify the Animation.currentTime and those behavior. So I think that we can remove those unrelated test code. However we can't remove the test code of animationPlayState. If we've removed the animationPlayState test code, those test code is not exist in gecko. What do you think of this modification?
Attachment #8747594 - Flags: feedback?(bbirtles)
Comment on attachment 8747594 [details] [diff] [review] Part3. Modify the currentTime test code. This is definitely an improvement. However, I think it still tests a lot of things that aren't really related to the currentTime like the computedStyle. If you have time I think it would be better to: * Write tests for getting/setting the currentTime by referring to: https://w3c.github.io/web-animations/#current-time https://w3c.github.io/web-animations/#set-the-current-time Put them in testing/web-platform/tests/web-animations/timing-model/animations/current-time.html and set-the-current-time.html * Simplify these tests to focus on CSS animation events and other CSS-specific features (e.g. checking that the currentTime of a new CSS animation is zero etc.) I did something similar in bug 1267893 for startTime but without simplifying the existing tests (see testing/web-platform/tests/web-animations/timing-model/animations/set-the-animation-start-time.html). Otherwise, we can do that in a separate bug and we just do the simplification here (which is basically your patch but without some of the extra tests for computedStyle etc. -- they could be moved to dom/animation/test/style even?).
Attachment #8747594 - Flags: feedback?(bbirtles)
Thanks Brian. (In reply to Brian Birtles (:birtles) from comment #43) > Otherwise, we can do that in a separate bug and we just do the > simplification here (which is basically your patch but without some of the > extra tests for computedStyle etc. -- they could be moved to > dom/animation/test/style even?). As your comment, We can remove unnecessary test code on css-animation/current-time.html and start-time.html. I'll start to separate this test(currenttime.html/starttime.html). https://treeherder.mozilla.org/#/jobs?repo=try&revision=905b47b4d2d4ace28751df857525b25ad251f05e
I separate the bug which adding testing/web-platform/tests/web-animation/timing-model/animations/current-time.html and set-the-current-time.html. And we can separate 'playState' tests, So I'll separate the those tests. In this bug, I focused on minimum modification. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f65babdfa477
Status: NEW → ASSIGNED
Depends on: 1271901
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/7-8/
Attachment #8744161 - Attachment description: MozReview Request: Bug 1134163 - Part1.Modify animationstart event timing in order to fire event after end of pending task. r?birtles → MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles
Attachment #8744162 - Attachment is obsolete: true
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Oh sorry. I created new review request as new queue patch. Then review-board obsoleted previous reviews. So I added review request flag manually.
Attachment #8744161 - Flags: review+ → review?(bbirtles)
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles https://reviewboard.mozilla.org/r/48347/#review49003 Thanks for doing this. This is pretty hard to review because there are so many simultaneous changes. Could you please drop the checks for playState at least -- that would make it a little easier to read. Also, I think splitting the new tests into a separate file would help. ::: dom/animation/test/css-animations/file_animation-currenttime.html:45 (Diff revision 8) > -function currentTimeForEndOfActiveInterval(timeline) { > - return ANIM_DELAY_MS + ANIM_DUR_MS; > -} > - > > -// Expected computed 'margin-left' values at points during the active interval: > +promise_test(function(t) See my comments below, but I think we can keep this as a regular test( ::: dom/animation/test/css-animations/file_animation-currenttime.html:48 (Diff revision 8) > -} > - > -test(function(t) > { > var div = addDiv(t, {'class': 'animated-div'}); > - > + div.style.animation = "anim 100s 100s"; What do we need the delay for? Would "anim 100s" be enough? ::: dom/animation/test/css-animations/file_animation-currenttime.html:55 (Diff revision 8) > > - assert_equals(animation.playState, "pending", > - 'Animation.playState should be "pending" when an animation ' + > - 'is initially created'); > - > assert_equals(animation.effect.target.style.animationPlayState, 'running', > 'Animation.effect.target.style.animationPlayState should be ' + > '"running" when an animation is initially created'); I think we can probably drop checking the play state. ::: dom/animation/test/css-animations/file_animation-currenttime.html:60 (Diff revision 8) > - > assert_equals(animation.effect.target.style.animationPlayState, 'running', > 'Animation.effect.target.style.animationPlayState should be ' + > '"running" when an animation is initially created'); > > - // XXX Ideally we would have a test to check the ready Promise is initially > + return animation.ready.then(function() { Perhaps add a comment: // Make sure the animation is running before we set the current time. ::: dom/animation/test/css-animations/file_animation-currenttime.html:61 (Diff revision 8) > - // unresolved, but currently there is no Web API to do that. Waiting for the > - // ready Promise with a timeout doesn't work because the resolved callback > - // will be called (async) regardless of whether the Promise was resolved in > + assert_equals(animation.effect.target.style.animationPlayState, 'running', > + 'Animation.effect.target.style.animationPlayState should be ' + > + '"running" at the start of the start delay'); It would probably be more useful to check here that animation.currentTime != 0. ::: dom/animation/test/css-animations/file_animation-currenttime.html:68 (Diff revision 8) > > - checkStateOnSettingCurrentTimeToZero(animation); > + assert_equals(animation.effect.target.style.animationPlayState, 'running', > + 'Animation.effect.target.style.animationPlayState should be ' + > + '"running" at the start of the start delay'); I think we can probably drop this check. ::: dom/animation/test/css-animations/file_animation-currenttime.html:83 (Diff revision 8) > - checkStateOnReadyPromiseResolved(animation); > + // the 0.0001 here is for rounding error > + assert_less_than_equal(animation.currentTime, > + animation.timeline.currentTime - animation.startTime + 0.0001, > + 'Animation.currentTime should be less than the local time ' + > + 'equivalent of the timeline\'s currentTime on the first paint tick ' + > + 'after animation creation'); > > - animation.currentTime = > - currentTimeForStartOfActiveInterval(animation.timeline); > + assert_equals(animation.effect.target.style.animationPlayState, 'running', > + 'Animation.effect.target.style.animationPlayState should be ' + > + '"running" on the first paint tick after animation creation'); > + I don't think we need these lines. ::: dom/animation/test/css-animations/file_animation-currenttime.html:97 (Diff revision 8) > - checkStateAtActiveIntervalStartTime(animation); > - > - animation.currentTime = > + assert_equals(animation.effect.target.style.animationPlayState, 'running', > + 'Animation.effect.target.style.animationPlayState should be ' + > + '"running" at the start of the active interval'); > - currentTimeForFiftyPercentThroughActiveInterval(animation.timeline); > - checkStateAtFiftyPctOfActiveInterval(animation); > I don't think we need these either. ::: dom/animation/test/css-animations/file_animation-currenttime.html:103 (Diff revision 8) > }).then(function() { > - checkStateAtActiveIntervalEndTime(animation); > + assert_equals(animation.effect.target.style.animationPlayState, "running", > + 'Animation.effect.target.style.animationPlayState should be ' + > + '"finished" at the end of the active interval'); I don't think we need this -- besides, the test and comment don't match. I suspect this test is wrong. ::: dom/animation/test/css-animations/file_animation-currenttime.html:121 (Diff revision 8) > - // testing going backwards since EventWatcher will fail the test if it gets > - // an event that we haven't told it about. > var retPromise = eventWatcher.wait_for(['animationstart', > 'animationend']).then(function() { > assert_true(document.timeline.currentTime - previousTimelineTime < > - ANIM_DUR_MS, > + 100 * MS_PER_SEC, Indentation is off here. ::: dom/animation/test/css-transitions/file_animation-currenttime.html (Diff revision 8) > -// TODO: add equivalent tests without an animation-delay, but first we need to > -// change the timing of animationstart dispatch. (Right now the animationstart > -// event will fire before the ready Promise is resolved if there is no > -// animation-delay.) > -// See https://bugzilla.mozilla.org/show_bug.cgi?id=1134163 > - We haven't added these tests to this file? Why is it ok to remove this comment? ::: dom/animation/test/css-transitions/file_animation-starttime.html (Diff revision 8) > -// TODO: add equivalent tests without an animation-delay, but first we need to > -// change the timing of animationstart dispatch. (Right now the animationstart > -// event will fire before the ready Promise is resolved if there is no > -// animation-delay.) > -// See https://bugzilla.mozilla.org/show_bug.cgi?id=1134163 > - Here too ::: dom/animation/test/style/file_animation-seeking.html:26 (Diff revision 8) > +'use strict'; > + > +const CSS_ANIM_EVENTS = > + ['animationstart', 'animationiteration', 'animationend']; > + > +// Sekking test using the animation current time. Nit: Seeking ::: dom/animation/test/style/file_animation-seeking.html:112 (Diff revision 8) > + 'animation-fill-mode is none'); > + > + return retPromise; > +}, 'Seeking backwards through animation using current time'); > + > +// Sekking test using the animation start time. Seeking
Attachment #8744161 - Flags: review?(bbirtles)
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/8-9/
Attachment #8744161 - Flags: review?(bbirtles)
https://reviewboard.mozilla.org/r/48347/#review49003 Thanks Brian! > We haven't added these tests to this file? Why is it ok to remove this comment? The css-transition test doesn't use an animation events, So I think that we can drop this comment. I think that we can remove the 'delay' code furthermore. The test of transition's startTime and currenTime didn't use the 'delay' code completely. (I'd like to fix those modification on bug 1271901)
Attachment #8744161 - Flags: review?(bbirtles) → review+
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles https://reviewboard.mozilla.org/r/48347/#review49275 r=me but only for the existing files. Please separate the new files into a separate patch and request review for that. These tests look a *lot* better, thank you. But, in general, whenever possible please try to make smaller changes using more patches. Doing that leads to faster reviews, more effective reviews, a much easier to understand revision history, and a much easier to bisect revision history. ::: dom/animation/test/css-animations/file_animation-currenttime.html:59 (Diff revision 9) > - assert_approx_equals(animation.currentTime, 0, 0.0001, // rounding error > - 'Check setting of currentTime actually works'); > + assert_not_equals(animation.currentTime, 0, > + 'Animation.current should be non-zero after playing an animation'); I don't think this is right. I think we can just drop this line. ::: dom/animation/test/css-animations/file_animation-currenttime.html:63 (Diff revision 9) > + // rounding error > + assert_approx_equals(animation.currentTime, 50 * MS_PER_SEC, 0.0001, > + 'Check setting of currentTime actually works'); > + }); I think we should just add assert_times_equal to ../testcommon.js like we have here: https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/testing/web-platform/tests/web-animations/testcommon.js#20 (But without the check for window.assert_times_equal first. That's only there to try to help other implementations integrate their own precision requirements.) The we can make this a little more simple, I think. ::: dom/animation/test/css-animations/file_animation-currenttime.html:113 (Diff revision 9) > }).then(function() { > - checkStateOnReadyPromiseResolved(animation); > + assert_less_than_equal(animation.currentTime, > + animation.timeline.currentTime - animation.startTime + 0.0001, > + 'Animation.currentTime should be less than the local time ' + > + 'equivalent of the timeline\'s currentTime on the first paint tick ' + > + 'after animation creation'); I don't think we need this part (also the comment is not right since it refers to "animation creation") ::: dom/animation/test/css-animations/file_animation-currenttime.html:161 (Diff revision 9) > var retPromise = eventWatcher.wait_for('animationstart').then(function() { > - animation.currentTime = currentTimeForBeforePhase(animation.timeline); > - animation.currentTime = currentTimeForActivePhase(animation.timeline); > + animation.currentTime = 50 * MS_PER_SEC; > + animation.currentTime = 150 * MS_PER_SEC; > > return waitForAnimationFrames(2); > }); > // get us into the initial state: > - animation.currentTime = currentTimeForActivePhase(animation.timeline); > + animation.currentTime = 150 * MS_PER_SEC; > > return retPromise; (It seems like these tests could be written in a much easier to understand way, but that's not your fault! Just something I noticed!) ::: dom/animation/test/css-animations/file_animation-starttime.html:163 (Diff revision 9) > + assert_approx_equals(animation.startTime, currentTime, 0.0001, > 'Check setting of startTime actually works'); I think we could use assert_times_equal here (and probably elsewhere in this file) ::: dom/animation/test/css-animations/file_animation-starttime.html (Diff revision 9) > - // Despite going backwards from after the end of the animation (to being > - // in the active interval), we now expect an 'animationstart' event > - // because the animation should go from being inactive to active. I think we should keep this first sentence of the comment. ::: dom/animation/test/css-animations/file_animation-starttime.html (Diff revision 9) > - // Despite going backwards from just after the active interval starts to > - // the animation start time, we now expect an animationend event > - // because we went from inside to outside the active interval. I think we should keep this comment. ::: dom/animation/test/css-animations/file_animation-starttime.html:212 (Diff revision 9) > - // This must come after we've set up the Promise chain, since requesting > + }); > - // computed style will force events to be dispatched. > - checkStateAtActiveIntervalEndTime(animation); Excellent! This fixes an existing bug as shown here: https://treeherder.mozilla.org/logviewer.html#?job_id=20734898&repo=try#L2632 ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:5 (Diff revision 9) > +<!doctype html> > +<html> > + <head> > + <meta charset=utf-8> > + <title>Tests for the behavior of seeking an Animation.currentTime</title> Nit: Tests for seeking using Animation.currentTime ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:26 (Diff revision 9) > +// Seeking test using the animation current time. > + We don't need this comment any more. ::: dom/animation/test/style/file_animation-seeking-with-start-time.html:5 (Diff revision 9) > +<!doctype html> > +<html> > + <head> > + <meta charset=utf-8> > + <title>Tests for the behavior of seeking an Animation.startTime</title> Nit: Tests for seeking using Animation.startTime ::: dom/animation/test/style/file_animation-seeking-with-start-time.html:26 (Diff revision 9) > +// Seeking test using the animation start time. > + We don't need this comment anymore. ::: dom/animation/test/style/file_animation-seeking-with-start-time.html:28 (Diff revision 9) > +promise_test(function(t) { > + var div = addDiv(t, {'class': 'animated-div'}); > + var eventWatcher = new EventWatcher(t, div, CSS_ANIM_EVENTS); > + div.style.animation = "anim 100s 100s"; > + var animation = div.getAnimations()[0]; > + > + return animation.ready.then(function() { > + var marginLeft = parseFloat(getComputedStyle(div).marginLeft); > + assert_equals(marginLeft, 10, > + 'the computed value of margin-left should be unaffected ' + > + 'by an animation with a delay on ready Promise resolve'); > + > + animation.startTime = animation.timeline.currentTime - 100 * MS_PER_SEC; > + return eventWatcher.wait_for('animationstart'); > + }).then(function() { > + var marginLeft = parseFloat(getComputedStyle(div).marginLeft); > + assert_between_inclusive(marginLeft, 100, 110, > + 'the computed value of margin-left should be close to the value at the ' + > + 'beginning of the animation'); > + > + animation.startTime = animation.timeline.currentTime - 150 * MS_PER_SEC; > + marginLeft = parseFloat(getComputedStyle(div).marginLeft); > + assert_equals(marginLeft, 150, > + 'the computed value of margin-left should be half way through the ' + > + 'animation at the midpoint of the active interval'); > + > + animation.startTime = animation.timeline.currentTime - 200 * MS_PER_SEC; > + return eventWatcher.wait_for('animationend'); > + }).then(function() { > + var marginLeft = parseFloat(getComputedStyle(div).marginLeft); > + assert_equals(marginLeft, 10, > + 'the computed value of margin-left should be unaffected ' + > + 'by the animation at the end of the active duration when the ' + > + 'animation-fill-mode is none'); > + }); > +}, 'Seeking forward through animation using start time'); Please move these new test files to a separate patch. I think these tests could be a *lot* simpler. We need to decide, "What needs to be tested?" I think the answer is probably something like: * Seeking within the active interval updates style. * Probably need one test going forwards and one going backwards * Seeking from "in effect" -> not "in effect" clears the animation result from the computed style * Probably should cover active -> after and active -> before * The reverse (seeking from not "in effect" -> "in effect") * Again, before -> active and after -> active That might be it? Any other cases that need testing? Then I think we don't need the event watching (we've already tested that elsewhere) or any of the promise waiting.
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/9-10/
Comment on attachment 8752083 [details] MozReview Request: Bug 1134163 - Part4. Add tests of getComputedStyle with seeking. r?birtles https://reviewboard.mozilla.org/r/52413/#review49341 This looks really good. However, please let me take one more brief look with the changes made. ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:8 (Diff revision 1) > + margin-left: 10px; > + animation-timing-function: linear ! important; > +} > + > +@keyframes anim { > + from { margin-left: 100px; } > + to { margin-left: 200px; } Nit: I think this test would be easier to follow if we make the initial value for margin-left something like -100px and then make the animation from 0px to 100px. Then, for example, it's obvious that seeking to 90% of the active interval should give you a margin-left of 90px. ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:39 (Diff revision 1) > + var animation = div.getAnimations()[0]; > + > + return animation.ready.then(function() { > + animation.currentTime = 90 * MS_PER_SEC; > + assert_marginLeft_equals(div, 190, > + 'the computed value of margin-left should be 190.'); Nit: I think a message such as, 'Computed style is updated when seeking forwards in active interval', would be more useful. If assert_marginLeft_equals fails, it will automatically generate an error message with the expected value so we don't need to repeat it here. Here we should describe more generally what we're testing. ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:43 (Diff revision 1) > + assert_marginLeft_equals(div, 190, > + 'the computed value of margin-left should be 190.'); > + > + animation.currentTime = 10 * MS_PER_SEC; > + assert_marginLeft_equals(div, 110, > + 'the computed value of margin-left should be 110.'); 'Computed style is updated when seeking backwards in active interval' ? ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:53 (Diff revision 1) > + assert_marginLeft_equals(div, 10, > + 'the computed value of margin-left should be uneffected.'); 'Computed style is unaffected in before phase with no backwards fill' ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:58 (Diff revision 1) > + assert_marginLeft_equals(div, 100, > + 'the computed value of margin-left should be 100'); 'Computed style is updated when seeking forwards from not \'in effect\' to \'in effect\' state' ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:71 (Diff revision 1) > + assert_marginLeft_equals(div, 10, > + 'the computed value of margin-left should be uneffected ' + > + 'after around setting the current time to end.'); 'Computed style is unaffected in after phase with no forwards fill' ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:77 (Diff revision 1) > + assert_marginLeft_equals(div, 150, > + 'the computed value of margin-left should be 150'); 'Computed style is updated when seeking backwards from not \'in effect\' to \'in effect\' state' ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:90 (Diff revision 1) > + assert_marginLeft_equals(div, 150, > + 'the computed value of margin-left should be 150'); 'Computed value is set during active phase' ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:95 (Diff revision 1) > + assert_marginLeft_equals(div, 10, > + 'the computed value of margin-left should be unaffected'); 'Computed value is not affected after seeking backwards from \'in effect\' to not \'in effect\' state' ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:108 (Diff revision 1) > + assert_marginLeft_equals(div, 150, > + 'the computed value of margin-left should be 150'); 'Computed value is set during active phase' ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:113 (Diff revision 1) > + assert_marginLeft_equals(div, 10, > + 'the computed value of margin-left should be unaffected'); 'Computed value is not affected after seeking forwards from \'in effect\' to not \'in effect\' state' ::: dom/animation/test/style/file_animation-seeking-with-start-time.html:7 (Diff revision 1) > +.animated-div { > + margin-left: 10px; > + animation-timing-function: linear ! important; > +} > + > +@keyframes anim { > + from { margin-left: 100px; } > + to { margin-left: 200px; } > +} My feedback for this test is basically the same as the last time. I think it would be more clear if the animation when from 0px to 100px. ::: dom/animation/test/style/file_animation-seeking-with-start-time.html:23 (Diff revision 1) > +const CSS_ANIM_EVENTS = > + ['animationstart', 'animationiteration', 'animationend']; I don't think this is needed? ::: dom/animation/test/style/file_animation-seeking-with-start-time.html:38 (Diff revision 1) > + assert_marginLeft_equals(div, 190, > + 'the computed value of margin-left should be 190.'); Please update all these messages in the same way suggested for the currentTime tests.
Attachment #8752083 - Flags: review?(bbirtles)
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/10-11/
Attachment #8752083 - Flags: review?(bbirtles)
Comment on attachment 8752083 [details] MozReview Request: Bug 1134163 - Part4. Add tests of getComputedStyle with seeking. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52413/diff/1-2/
https://reviewboard.mozilla.org/r/52413/#review49341 Thanks Brian. I updated the patch, could you please confirm this patch? > Nit: I think this test would be easier to follow if we make the initial value for margin-left something like -100px and then make the animation from 0px to 100px. > > Then, for example, it's obvious that seeking to 90% of the active interval should give you a margin-left of 90px. I modified animated range and unaffected position. (I would like to clarify the affected position and unaffected position.) > I don't think this is needed? Yes, this definition is unnecessary. This is my mistake.
Comment on attachment 8752083 [details] MozReview Request: Bug 1134163 - Part4. Add tests of getComputedStyle with seeking. r?birtles https://reviewboard.mozilla.org/r/52413/#review49630 ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:23 (Diff revision 2) > +const CSS_ANIM_EVENTS = > + ['animationstart', 'animationiteration', 'animationend']; > + I'm pretty sure we don't need this. ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:62 (Diff revision 2) > + animation.currentTime = 100 * MS_PER_SEC; > + assert_marginLeft_equals(div, 0, > + 'Computed style is updated when seeking forwards from ' + > + 'not \'in effect\' to \'in effect\' state'); > + }); > +}, 'Seeking to non-\'in effect\' from \'in effect\'(before -> active)'); Nit: missing space before ( ::: dom/animation/test/style/file_animation-seeking-with-current-time.html:81 (Diff revision 2) > + animation.currentTime = 150 * MS_PER_SEC; > + assert_marginLeft_equals(div, 50, > + 'Computed style is updated when seeking backwards from ' + > + 'not \'in effect\' to \'in effect\' state'); > + }); > +}, 'Seeking to non-\'in effect\' from \'in effect\'(after -> active)'); Nit: Missing space here and elsewhere in this file.
Attachment #8752083 - Flags: review?(bbirtles) → review+
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/11-12/
Comment on attachment 8752083 [details] MozReview Request: Bug 1134163 - Part4. Add tests of getComputedStyle with seeking. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52413/diff/2-3/
Attachment #8747594 - Attachment is obsolete: true
Keywords: leave-open → checkin-needed
Comment on attachment 8744161 [details] MozReview Request: Bug 1134163 - Part3. Modify the currentTime and startTime tests. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48347/diff/12-13/
Comment on attachment 8752083 [details] MozReview Request: Bug 1134163 - Part4. Add tests of getComputedStyle with seeking. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52413/diff/3-4/
Comment on attachment 8752083 [details] MozReview Request: Bug 1134163 - Part4. Add tests of getComputedStyle with seeking. r?birtles This failure is occurred when setting the start time to boundary of animation effect range. So I modified start time value. https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0ac8722e275da93fbb8537b29e3a4ed56b8fc75f Hi Brian, Could you confirm this change?
Looks fine. Let's see how this goes https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4a419595a54acde76bfe8c649482ac8bacefdfc (Which I guess is what you meant to quote above)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/7a4e2dfbccf5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: