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)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: birtles, Assigned: birtles)
Details
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
1.00 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f1d5ad18a14 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca2ad722201f
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49125/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49125/
Attachment #8745858 -
Flags: review?(hiikezoe)
Attachment #8745860 -
Flags: review?(hiikezoe)
Attachment #8745861 -
Flags: review?(hiikezoe)
Attachment #8745862 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49127/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49127/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49129/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49129/
Assignee | ||
Comment 5•8 years ago
|
||
Based on changes to the spec: https://github.com/w3c/web-animations/commit/8bf2b102dedbbc0d96b919d09a8c2d6035a14c98 Review commit: https://reviewboard.mozilla.org/r/49131/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49131/
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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 8•8 years ago
|
||
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 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
(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.
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2f257b08990 https://hg.mozilla.org/integration/mozilla-inbound/rev/a216f6481606 https://hg.mozilla.org/integration/mozilla-inbound/rev/423248a6981d https://hg.mozilla.org/integration/mozilla-inbound/rev/07c56c4b155b
Assignee | ||
Comment 12•8 years ago
|
||
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.
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c2f257b08990 https://hg.mozilla.org/mozilla-central/rev/a216f6481606 https://hg.mozilla.org/mozilla-central/rev/423248a6981d https://hg.mozilla.org/mozilla-central/rev/07c56c4b155b https://hg.mozilla.org/mozilla-central/rev/c70b39895748
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 15•8 years ago
|
||
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).
Assignee | ||
Comment 16•8 years ago
|
||
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?
Updated•8 years ago
|
status-firefox48:
--- → affected
Comment 17•8 years ago
|
||
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+
Comment 18•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/675b0e471e05
You need to log in
before you can comment on or make changes to this bug.
Description
•