Closed Bug 1267893 Opened 8 years ago Closed 8 years ago

Make setting the start time act like a seek

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: birtles, Assigned: birtles)

Details

Attachments

(5 files)

The spec was recently updated to make setting the start time set the "did seek" flag to true:

  https://github.com/w3c/web-animations/commit/8bf2b102dedbbc0d96b919d09a8c2d6035a14c98
Comment on attachment 8745858 [details]
MozReview Request: Bug 1267893 part 1 - Add assert_times_equal for comparing times while allowing for a degree of imprecision

https://reviewboard.mozilla.org/r/49125/#review45955
Attachment #8745858 - Flags: review?(hiikezoe) → review+
Comment on attachment 8745860 [details]
MozReview Request: Bug 1267893 part 2 - Move existing timing model tests into animation-effects subfolder

https://reviewboard.mozilla.org/r/49127/#review45957
Attachment #8745860 - Flags: review?(hiikezoe) → review+
Comment on attachment 8745861 [details]
MozReview Request: Bug 1267893 part 3 - Add tests for start time

https://reviewboard.mozilla.org/r/49129/#review45949

I think we are using style that each brace is positioned at the end of line, isn't it?

::: testing/web-platform/tests/web-animations/animation/startTime.html:28
(Diff revision 1)
> +{
> +  var animation = new Animation(new KeyframeEffect(createDiv(t), null),
> +                                document.timeline);
> +  animation.play();
> +  assert_equals(animation.startTime, null, 'startTime is unresolved');
> +}, 'startTime of a pause-pending animation is unresolved');

Nit:  play-pending

::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-animation-start-time.html:94
(Diff revision 1)
> +  assert_equals(animation.playState, 'running',
> +                'Animation reports it is running after setting a resolved'
> +                + ' start time');
> +}, 'Setting the start time clears the hold time');
> +
> +promise_test(function(t)

test(function(t) ?

::: testing/web-platform/tests/web-animations/timing-model/animations/set-the-animation-start-time.html:187
(Diff revision 1)
> +  animation.startTime = document.timeline.currentTime
> +                        - 110 * MS_PER_SEC;
> +  assert_equals(animation.playState, 'finished',
> +                'Seeked to finished state using the startTime');
> +
> +  // If the 'did seek' flag is true, the current should be greater than

Nit: the current time.
Attachment #8745861 - Flags: review?(hiikezoe) → review+
Comment on attachment 8745862 [details]
MozReview Request: Bug 1267893 part 4 - Make setting the start time set 'did seek' to true

https://reviewboard.mozilla.org/r/49131/#review45961
Attachment #8745862 - Flags: review?(hiikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> Comment on attachment 8745861 [details]
> MozReview Request: Bug 1267893 part 3 - Add tests for start time
> 
> https://reviewboard.mozilla.org/r/49129/#review45949
> 
> I think we are using style that each brace is positioned at the end of line,
> isn't it?

You mean instead of:

  test(function(t)
  {

we should have:

  test(function(t) {

?

Yes, I think you're right. I copied the original style from one of the other tests but it was probably wrong.
I made a mistake in the precision value used in part 1 here since I forgot that we are comparing *milliseconds* not *seconds*. This caused intermittent failures such as:

  https://treeherder.mozilla.org/logviewer.html#?job_id=26711451&repo=mozilla-inbound

Pushing a follow-up to correct this.
This patch just contains the one-line code change without all the test
refactoring since that could get messy to merge into Aurora (since it touches
the web-platform-tests manifest files which and they're yet to be merged to
the central test repository).
Comment on attachment 8747567 [details] [diff] [review]
Simplified patch for landing on Aurora

Approval Request Comment
[Feature/regressing bug #]: No regressing bug. We plan to ship Element.animate in Firefox 48 (bug 1245000) and there has since been a minor tweak to the spec that we have landed on m-c (Firefox 49). This just updates Aurora so that when we ship we comply with the spec.
[User impact if declined]: In some circumstances, setting the startTime of an animation may produce different behavior to the specified behavior and the behavior in Firefox 49.
[Describe test coverage new/current, TreeHerder]: Has baked on m-c since the end of last week. Has tests on m-c but I don't plan to uplift them (as per comment 15) just the one-line code change.
[Risks and why]: No known risks.
[String/UUID change made/needed]: None.
Attachment #8747567 - Flags: approval-mozilla-aurora?
Comment on attachment 8747567 [details] [diff] [review]
Simplified patch for landing on Aurora

Please uplift to aurora so we comply with a new spec.
Attachment #8747567 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.