Closed Bug 1472900 Opened 2 years ago Closed Last year

Use the timestamp associated with the timeline for cancel events

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(3 files)

See bug 1354501 comment 60.

Just using the timestamp breaks the order of cancel events and CSS cancel events.  We need to use the timestamp for CSS cancel events too.

This should be fixed in the same release that bug 1354501 landed.
Comment on attachment 8989872 [details]
Bug 1472900 - Use timestamp associated with the timeline for animation cancel events.

https://reviewboard.mozilla.org/r/254868/#review261704

::: testing/web-platform/tests/web-animations/timing-model/timelines/update-and-send-events.html:201
(Diff revision 1)
> +    [ 'transitionstart',
> +      'finish',
> +      'cancel',
> +      'transitioncancel' ],

I am wondering, in this test case, is it OK that 'finish' event is dispatched eariler than 'cancel' event?  Normally, (in the case where finish() is not called), target effect end is a time later when cancel() is called, but if finish() is called, the start time of the animation is modified as if the target effect end is the time just when finish() is called.  As a result, in this case, 'finish' event is dispatched before 'cancel' event (since bothe of the scheduled event time are the same and both of are play back events and the sort is stable).
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8989870 [details]
Bug 1472900 - Rename mTimeStamp in AnimationEventInfo to mScheduledEventTimeStamp.

https://reviewboard.mozilla.org/r/254864/#review261718
Attachment #8989870 - Flags: review?(bbirtles) → review+
Comment on attachment 8989871 [details]
Bug 1472900 - Make some closures for arrow function a single line.

https://reviewboard.mozilla.org/r/254866/#review261720
Attachment #8989871 - Flags: review?(bbirtles) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Comment on attachment 8989872 [details]
> Bug 1472900 - Use timestamp associated with the timeline for animation
> cancel events.
> 
> https://reviewboard.mozilla.org/r/254868/#review261704
> 
> :::
> testing/web-platform/tests/web-animations/timing-model/timelines/update-and-
> send-events.html:201
> (Diff revision 1)
> > +    [ 'transitionstart',
> > +      'finish',
> > +      'cancel',
> > +      'transitioncancel' ],
> 
> I am wondering, in this test case, is it OK that 'finish' event is
> dispatched eariler than 'cancel' event?  Normally, (in the case where
> finish() is not called), target effect end is a time later when cancel() is
> called, but if finish() is called, the start time of the animation is
> modified as if the target effect end is the time just when finish() is
> called.  As a result, in this case, 'finish' event is dispatched before
> 'cancel' event (since bothe of the scheduled event time are the same and
> both of are play back events and the sort is stable).

Brian, what do you think about this?
Flags: needinfo?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > testing/web-platform/tests/web-animations/timing-model/timelines/update-and-
> > send-events.html:201
> > (Diff revision 1)
> > > +    [ 'transitionstart',
> > > +      'finish',
> > > +      'cancel',
> > > +      'transitioncancel' ],
> > 
> > I am wondering, in this test case, is it OK that 'finish' event is
> > dispatched eariler than 'cancel' event?  Normally, (in the case where
> > finish() is not called), target effect end is a time later when cancel() is
> > called, but if finish() is called, the start time of the animation is
> > modified as if the target effect end is the time just when finish() is
> > called.  As a result, in this case, 'finish' event is dispatched before
> > 'cancel' event (since bothe of the scheduled event time are the same and
> > both of are play back events and the sort is stable).
> 
> Brian, what do you think about this?

Yes, I think that's the correct behvaior. finish() synchronously does the finish actions and we called finish() then cancel() so the finish event should come first.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #9)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > > testing/web-platform/tests/web-animations/timing-model/timelines/update-and-
> > > send-events.html:201
> > > (Diff revision 1)
> > > > +    [ 'transitionstart',
> > > > +      'finish',
> > > > +      'cancel',
> > > > +      'transitioncancel' ],
> > > 
> > > I am wondering, in this test case, is it OK that 'finish' event is
> > > dispatched eariler than 'cancel' event?  Normally, (in the case where
> > > finish() is not called), target effect end is a time later when cancel() is
> > > called, but if finish() is called, the start time of the animation is
> > > modified as if the target effect end is the time just when finish() is
> > > called.  As a result, in this case, 'finish' event is dispatched before
> > > 'cancel' event (since bothe of the scheduled event time are the same and
> > > both of are play back events and the sort is stable).
> > 
> > Brian, what do you think about this?
> 
> Yes, I think that's the correct behvaior. finish() synchronously does the
> finish actions and we called finish() then cancel() so the finish event
> should come first.

Thanks Brian.  I am going to set a review request for this patch as it is.
Attachment #8989872 - Flags: review?(bbirtles)
Comment on attachment 8989872 [details]
Bug 1472900 - Use timestamp associated with the timeline for animation cancel events.

https://reviewboard.mozilla.org/r/254868/#review263026

::: dom/animation/Animation.h:532
(Diff revision 1)
> +    Nullable<TimeDuration> timelineTime = mTimeline->GetCurrentTime();
> +    if (!timelineTime.IsNull()) {
> +      result = mTimeline->ToTimeStamp(timelineTime.Value());
> +    }

Would it be better to put this on AnimationTimeline?

We already have GetCurrenttimeAsDouble:

  Nullable<double> GetCurrentTimeAsDouble() const {
    return AnimationUtils::TimeDurationToDouble(GetCurrentTime());
  }

So we could just add:

  TimeStamp GetCurrentTimeAsTimestamp() const {
    Nullable<TimeDuration> currentTime = GetCurrentTime();
    return !currentTime.IsNull() ? ToTimestamp(GetCurrentTime()) : TimeStamp();
  }

?

Then I think this would become:

  result = mTimeline->GetCurrentTimeAsTimestamp();

?
Attachment #8989872 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #11)
> Comment on attachment 8989872 [details]
> Bug 1472900 - Use timestamp associated with the timeline for animation
> cancel events.
> 
> https://reviewboard.mozilla.org/r/254868/#review263026
> 
> ::: dom/animation/Animation.h:532
> (Diff revision 1)
> > +    Nullable<TimeDuration> timelineTime = mTimeline->GetCurrentTime();
> > +    if (!timelineTime.IsNull()) {
> > +      result = mTimeline->ToTimeStamp(timelineTime.Value());
> > +    }
> 
> Would it be better to put this on AnimationTimeline?
> 
> We already have GetCurrenttimeAsDouble:
> 
>   Nullable<double> GetCurrentTimeAsDouble() const {
>     return AnimationUtils::TimeDurationToDouble(GetCurrentTime());
>   }
> 
> So we could just add:
> 
>   TimeStamp GetCurrentTimeAsTimestamp() const {
>     Nullable<TimeDuration> currentTime = GetCurrentTime();
>     return !currentTime.IsNull() ? ToTimestamp(GetCurrentTime()) :
> TimeStamp();
>   }
> 
> ?
> 
> Then I think this would become:
> 
>   result = mTimeline->GetCurrentTimeAsTimestamp();

Thanks! Sounds nice.  Will do.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f173d7778612
Rename mTimeStamp in AnimationEventInfo to mScheduledEventTimeStamp. r=birtles
https://hg.mozilla.org/integration/autoland/rev/3dfd3db23cf1
Make some closures for arrow function a single line. r=birtles
https://hg.mozilla.org/integration/autoland/rev/1a2b5f7c797e
Use timestamp associated with the timeline for animation cancel events. r=birtles
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11919 for changes under testing/web-platform/tests
Depends on: 1474811
It looks like the upstream stability check is failing here. I guess that's expected since it's also intermittent locally, so I'll merge to avoid conflicts, but it would be nice if we had a real fix :)
Thanks James. Yes, re-enabling this is P1 (bug 1475127) -- it should be done next week.
You need to log in before you can comment on or make changes to this bug.