Closed
Bug 1467344
Opened 6 years ago
Closed 6 years ago
Upstream tests for Web animations <-> CSS transitions integration to web-platform-tests
Categories
(Core :: DOM: Animation, enhancement, P3)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla63
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(21 files)
No description provided.
Comment 1•6 years ago
|
||
What's the difference with bug 1466031? Is it just some follow-up bug to upstream more tests?
Assignee | ||
Comment 2•6 years ago
|
||
Yeah, this is for transitions.
Assignee | ||
Comment 3•6 years ago
|
||
I'm about half way through doing this.
Assignee | ||
Comment 4•6 years ago
|
||
I've updated all the tests in css-transitions but have yet to move them into wpt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea82f3c6d02d76fc5823d2e0d3784407f965239b
Assignee | ||
Comment 5•6 years ago
|
||
Assignee | ||
Comment 6•6 years ago
|
||
As part of this change, the "Restart transition after cancelling transition immediately" test in test_event-dispatch.html needed significant changes. The reason is that this test was calling: watcher.wait_for([ 'transitioncancel', 'transitionrun', 'transitionstart' ]); However, it was not waiting for the result of that call and hence was not actually checking if the events were being dispatched. And, they are not. There are two problems. Firstly, this test cancels the transition by setting display:none. However, transitions don't run on display:none elements so attempting to restart the transition will not generate transition events. This might be a bug, but it is not a recent regression in any case (I tested back to Firefox 54). However, this test does not require using display:none to cancel. There are _many_ tests that check that display:none generates a transitioncancel event. This test only needs to make the transition idle. As a result, this patch makes that test call transition.cancel() instead. However, even with that change this test will not pass because it sets a transition-delay of 100s (presumably so that it does not need to check for a transitionstart event). As a result this test should not wait on _both_ transitionrun and transitionstart but just transitionrun.
Assignee | ||
Comment 7•6 years ago
|
||
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
Assignee | ||
Comment 10•6 years ago
|
||
We're a bit inconsistent about this, but generally we try to keep this files as minimal as possible. There is still a lot of other cruft in the starttime and currenttime test files but we will remove that when we tidy up those files in later patches in this series.
Assignee | ||
Comment 11•6 years ago
|
||
Assignee | ||
Comment 12•6 years ago
|
||
This patch basically completely rewrites this file to make it simpler and remove unnecessary tests. In particular: * It sets the duration and delay to 100s to match what we use in most tests * All the helper methods at the start are inlined into the relevant tests because doing so makes the tests much easier to read and debug. * The first two tests are combined and extended - Testing the initial current time is zero is not generally useful but it is when we're testing setting it - What the second test really wants to test is that the style updates (i.e. you really can seek a transition, not just that the currentTime value updates) - The trick where we set the start time just to get out of the pending state seems unnecessary, we may as well just wait on ready. In fact, we don't _need_ to do that, but this test is about checking you can seek a transition in motion. * In the fourth test, 'Skipping backwards through transition', we don't check the event is dispatched since that is coverted in test_event-dispatch.html in the 'Active -> Before' test. * The final test, 'Animation.currentTime after pausing', is covered by web-animations/timing-model/animations/pausing-an-animation.html
Assignee | ||
Comment 13•6 years ago
|
||
For the first test, we really want to check that it is possible to restart transitions (since they are otherwise disassociated once they finish) hence this patch updates the test to the actual output in style rather than timing. The second test is covered in web-animations/timing-model/animations/reversing-an-animation.html by the 'Playing a finished and reversed animation seeks to end' test so this patch removes it from this file.
Assignee | ||
Comment 14•6 years ago
|
||
This seems to be adequately covered by: web-animations/timing-model/animations/pausing-an-animation.html (specifically the last test: 'The animation's current time remains fixed after pausing') web-animations/timing-model/animation-effects/simple-iteration-progress.html (for the specific checks of the iteration progress)
Assignee | ||
Comment 15•6 years ago
|
||
The test: 'A new ready promise is created each time play() is called the animation property', is covered in: web-animations/interfaces/Animation/ready.html by the 'A new ready promise is created when play()/pause() is called' test. As a result, this patch removes that test.
Assignee | ||
Comment 16•6 years ago
|
||
Apart from obvious tidy-ups the substantive changes to tests in this file are as follows: * Skipping forward through animation -> This is really testing two things: (a) That you can seek a transition using the start time. (b) That seeking a transition using the start time triggers dispatching events. This patch splits the above into two separate tests. * Skipping backwards through animation, -> All these tests are really just exercising event dispatch which is already covered by test_event-dispatch.html. As a result this patch drops these tests. * Setting startTime to null -> Covered by 'Setting an unresolved start time sets the hold time' in wpt/web-animations/timing-model/animations/setting-the-start-time-of-an-animation.html * Animation.startTime after pausing -> Covered by 'Pausing clears the start time' in wpt/web-animations/timing-model/animations/pausing-an-animation.html
Assignee | ||
Comment 17•6 years ago
|
||
This is mostly whitespace tidy-ups. The only substantive change is the fix to the test description which previously seemed to be missing some words.
Assignee | ||
Comment 18•6 years ago
|
||
This mostly just tidies up these tests to give them sensible titles. The only test with major substantive changes is the first test. This test tests three things: 1) That getAnimations() returns one transition per transitioning property 2) That getAnimations() returns transitions in the order they were generated 3) That CSS transitions have their start time set based on when they were generated. (2) is covered later in this file by the test: 'getAnimations sorts transitions by when they were generated' (3) is really a test for startTime, if anything, not getAnimations(). I'm not sure how necessary it is, but I've added it to test_animation-starttime.html for now. As a result, this patch updates this first test to only cover (1).
Assignee | ||
Comment 19•6 years ago
|
||
This patch merely tidies up some whitespace and uses of quotes / template strings. There are no substantive changes.
Assignee | ||
Comment 20•6 years ago
|
||
This patch splits the first test into four separate tests since it really seems to be testing four different things. Likewise, the later patch for replacing the effect is split into two parts: - One to test the playState behavior - One to test the value reported by transitionProperty
Assignee | ||
Comment 21•6 years ago
|
||
flushComputedStyle is as follows: var cs = getComputedStyle(elem); cs.marginLeft; That _probably_ flushes style in most engines, but it's possible some engine in the future will optimize things in a way that it can flush marginLeft without flushing, say, backgroundColor. Before moving these tests to wpt, it would be better to explicitly flush the property (or at least one of the properties) we are transitioning. This patch also updates the "Setting zero combined duration" test from test_animation-cancel.html since, in making this change, I realized the test was wrong. Specifically, it was testing that when we set a combined duration of zero that a transition is canceled. However, nothing in the spec requires this. The spec only requires a non-zero combined duration to _start_ a transition and only cancels an existing transition if the combined duration becomes _zero_ if the "the end value of the running transition is not equal to the value of the property in the after-change style". This test passed, however, because it changed the transition property to margin-top, hence it was actually testing the same condition as the previous test.
Assignee | ||
Comment 22•6 years ago
|
||
Assignee | ||
Comment 23•6 years ago
|
||
Assignee | ||
Comment 24•6 years ago
|
||
Assignee | ||
Comment 25•6 years ago
|
||
This is a collection of fix-ups to the corresponding tests for CSS animations to apply the same sort of naming etc. that we have now applied to CSS transitions tests.
Assignee | ||
Updated•6 years ago
|
Attachment #9001486 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001487 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001488 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001489 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001490 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001491 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001492 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001493 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001495 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001496 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001497 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001498 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001499 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001500 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001501 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001502 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001503 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001504 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001505 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001506 -
Flags: review?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Attachment #9001507 -
Flags: review?(hikezoe)
Assignee | ||
Comment 26•6 years ago
|
||
Sorry for the review bomb. I ended up giving up on Phabricator because it can't handle patches for MANIFEST.json without specifying extra options (and, not being able to use mq with moz-phab, the extra hassle of dealing with merge conflicts to MANIFEST.json would make this quite difficult to update).
Comment 27•6 years ago
|
||
Comment on attachment 9001486 [details] [diff] [review] Use arrow functions in dom/animation/test/css-transitions/ Review of attachment 9001486 [details] [diff] [review]: ----------------------------------------------------------------- r=me with a commit message for dropping functions. ::: dom/animation/test/css-transitions/test_animation-currenttime.html @@ -74,5 @@ > // > // http://drafts.csswg.org/web-animations/#animation-effect-phases-and-states > > -// Called when currentTime is set to zero (the beginning of the start delay). > -function checkStateOnSettingCurrentTimeToZero(animation) Where were this function moved? Oh, ok, this function is no longer used. Can you please add a commit message for this removal?
Attachment #9001486 -
Flags: review?(hikezoe) → review+
Comment 28•6 years ago
|
||
Comment on attachment 9001487 [details] [diff] [review] Use async/await in dom/animation/test/css-transitions/ Review of attachment 9001487 [details] [diff] [review]: ----------------------------------------------------------------- Would you mind filing a bug for the display:none case and another bug for the check there is no cancel event after a redundant cancel call? The former might not be a bug though, I don't want to lose just in case it's a bug. ::: dom/animation/test/css-transitions/test_event-dispatch.html @@ +349,5 @@ > + await watcher.wait_for('transitioncancel'); > + > + transition.cancel(); > + // Then wait a couple of frames and check that no event was dispatched > + await waitForAnimationFrames(2); Though it's not relevant with this change, I don' think this test checks what it supposed originally. I mean, we should check there is no further transitioncancel event after here? @@ +379,5 @@ > + transition.cancel(); > + await watcher.wait_for('transitioncancel'); > + > + // Then wait a couple of frames and check that no event was dispatched > + await waitForAnimationFrames(2); Same here. We should check no transitioncancel event happens after this in another bug.
Attachment #9001487 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001488 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001489 -
Flags: review?(hikezoe) → review+
Comment 29•6 years ago
|
||
Comment on attachment 9001490 [details] [diff] [review] Update the titles of each test file Review of attachment 9001490 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/test/css-transitions/test_animation-computed-timing.html @@ +1,3 @@ > <!doctype html> > <meta charset=utf-8> > +<title>AnimationEffect.getComputedTiming() for CSS transitions</title> Nit: CSSTransition.getComputedTiming() since we use 'CSSAnimation.getComputedTiming' for tests for CSS Animations. Or do we want to revise the CSS animation's one?
Attachment #9001490 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001491 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001492 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001495 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001497 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001499 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001501 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001506 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001507 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001505 -
Flags: review?(hikezoe) → review+
Comment 30•6 years ago
|
||
Comment on attachment 9001504 [details] [diff] [review] Move tests from dom/animations/tests/css-transitions to WPT Review of attachment 9001504 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/css/css-transitions/support/helper.js @@ +226,5 @@ > + } > + } > + > + if (t && typeof t.add_cleanup === 'function') { > + t.add_cleanup(function() { Nit: it would be nice to use an arrow function here as well. @@ +236,5 @@ > +/** > + * Promise wrapper for requestAnimationFrame. > + */ > +root.waitForFrame = () => { > + return new Promise(function(resolve, reject) { Same here. @@ +250,5 @@ > + * @param onFrame An optional function to be processed in each animation frame. > + */ > +root.waitForAnimationFrames = (frameCount, onFrame) => { > + const timeAtStart = document.timeline.currentTime; > + return new Promise(function(resolve, reject) { Same here.
Attachment #9001504 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001503 -
Flags: review?(hikezoe) → review+
Comment 31•6 years ago
|
||
Comment on attachment 9001493 [details] [diff] [review] Update tests in test_animation-currenttime.html Review of attachment 9001493 [details] [diff] [review]: ----------------------------------------------------------------- Nice! I was about to leave comments about these checkXX() method in the first patch. Also it would be nice to set transition longhand property in each test respectively, since during I was reviewing this patch, I was getting into a trouble that I misunderstand that the transition in question doesn't have positive delay. E.g. when I saw 'animation.currentTime = 150 * MS_PER_SEC', I thought the transition finishes. As for the fourth test, though I wouldn't too much worry, it's slightly different from what 'Active -> Before' test in test_event-dispatch.html. The test in this file sets the currentTime back to the end of delay. So it might be an edge case.
Attachment #9001493 -
Flags: review?(hikezoe) → review+
Comment 32•6 years ago
|
||
Comment on attachment 9001496 [details] [diff] [review] Drop test_animation-pausing.html Review of attachment 9001496 [details] [diff] [review]: ----------------------------------------------------------------- I agree 'The animation's current time remains fixed after pausing' covers the first half of this test, but it seems to me that what simple-iteration-progress.html does looks different. It doesn't involve any pending state changes, right?
Comment 33•6 years ago
|
||
Comment on attachment 9001502 [details] [diff] [review] Update tests in test_setting-effect.html Review of attachment 9001502 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/test/css-transitions/test_setting-effect.html @@ +76,4 @@ > assert_equals(getComputedStyle(div).left, '100px'); > > await watcher.wait_for('transitionend'); > +}, 'After setting a transition\'s effect to null, transitionend is still' It seems to me that this event test case is covered by the last test in test_event-displath.html.
Attachment #9001502 -
Flags: review?(hikezoe) → review+
Updated•6 years ago
|
Attachment #9001500 -
Flags: review?(hikezoe) → review+
Comment 34•6 years ago
|
||
Comment on attachment 9001498 [details] [diff] [review] Update tests in test_animation-starttime.html Review of attachment 9001498 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/test/css-transitions/test_animation-starttime.html @@ +8,4 @@ > > .animated-div { > margin-left: 100px; > + transition: margin-left 100s linear 100s; It would be nice to set transition property in each test, especially in the case where transition has positive delay like this case. @@ +79,3 @@ > > + animation.startTime = timelineTime - 200 * MS_PER_SEC; > + await eventWatcher.wait_for('transitionend'); It seems to me that this test hasn't originally work as expected, since both events will be fired sooner or later. We should check each event happens within the next reqeustAnimationCallbacl? Would you mind filing a bug for this?
Attachment #9001498 -
Flags: review?(hikezoe) → review+
Assignee | ||
Comment 35•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #28) > ::: dom/animation/test/css-transitions/test_event-dispatch.html > @@ +349,5 @@ > > + await watcher.wait_for('transitioncancel'); > > + > > + transition.cancel(); > > + // Then wait a couple of frames and check that no event was dispatched > > + await waitForAnimationFrames(2); > > Though it's not relevant with this change, I don' think this test checks > what it supposed originally. I mean, we should check there is no further > transitioncancel event after here? EventWatcher checks this. It catches the original 'transitioncancel' event in the wait_for call. If a subsequent 'transitioncancel' event is dispatched then EventWatcher's event handler 'transitioncancel' will assert because |waitingFor| is empty.[1] [1] https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/testing/web-platform/tests/resources/testharness.js#631-635 > @@ +379,5 @@ > > + transition.cancel(); > > + await watcher.wait_for('transitioncancel'); > > + > > + // Then wait a couple of frames and check that no event was dispatched > > + await waitForAnimationFrames(2); > > Same here. We should check no transitioncancel event happens after this in > another bug. Likewise here.
Assignee | ||
Comment 36•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #28) > Would you mind filing a bug for the display:none case and another bug for > the check there is no cancel event after a redundant cancel call? The > former might not be a bug though, I don't want to lose just in case it's a > bug. Filed bug 1484145 for the display:none case. For the cancel call, I believe the test is working as expected.
Assignee | ||
Comment 37•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #29) > Comment on attachment 9001490 [details] [diff] [review] ... > Nit: CSSTransition.getComputedTiming() since we use > 'CSSAnimation.getComputedTiming' for tests for CSS Animations. Or do we want > to revise the CSS animation's one? Right, that's fixed in the last patch in this series (to make the CSS animation one consistent with this one).
Comment 38•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #36) > (In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #28) > > Would you mind filing a bug for the display:none case and another bug for > > the check there is no cancel event after a redundant cancel call? The > > former might not be a bug though, I don't want to lose just in case it's a > > bug. > > Filed bug 1484145 for the display:none case. For the cancel call, I believe > the test is working as expected. Thanks!
Assignee | ||
Comment 39•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #31) > Also it would be nice to set transition longhand property in each test > respectively, since during I was reviewing this patch, I was getting into a > trouble that I misunderstand that the transition in question doesn't have > positive delay. E.g. when I saw 'animation.currentTime = 150 * MS_PER_SEC', > I thought the transition finishes. Fixed. > As for the fourth test, though I wouldn't too much worry, it's slightly > different from what 'Active -> Before' test in test_event-dispatch.html. > The test in this file sets the currentTime back to the end of delay. So it > might be an edge case. Yeah, I think it's similar enough for now. We can add more tests here in future if we think there's complexity that needs testing.
Assignee | ||
Comment 40•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #32) > Comment on attachment 9001496 [details] [diff] [review] > Drop test_animation-pausing.html > > Review of attachment 9001496 [details] [diff] [review]: > ----------------------------------------------------------------- > > I agree 'The animation's current time remains fixed after pausing' covers > the first half of this test, but it seems to me that what > simple-iteration-progress.html does looks different. It doesn't involve any > pending state changes, right? Fair enough. Of the progress checks in that file we have: > assert_equals(previousProgress, 0, 'Initial value of progress is zero'); Which is covered by web-animations/timing-model/animation-effects/simple-iteration-progress.html since when it calls assert_computed_timing_for_each_phase it checks the progress before seeking. > assert_greater_than(animation.effect.getComputedTiming().progress, > previousProgress, > 'Iteration progress is initially increasing'); This is covered by the combination of web-platform/tests/web-animations/interfaces/Animation/pause.html ('pause() a running animation') which tests that the currentTime increases [although it really should be in the-current-time-of-an-animation.html] and simple-iteration-progress.html which tests the calculation of the progress from the currentTime. > previousProgress = animation.effect.getComputedTiming().progress; This is covered by pausing-an-animation.html while simple-iteration-progress.html tests the calculation of the progress from the currentTime. > assert_greater_than(animation.effect.getComputedTiming().progress, > previousProgress, > 'Iteration progress increases after calling play()'); Resuming an animation is not really covered anywhere as far as I can tell. Maybe I can get to adding such a test next week.
Comment 41•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #40) > > assert_greater_than(animation.effect.getComputedTiming().progress, > > previousProgress, > > 'Iteration progress increases after calling play()'); > > Resuming an animation is not really covered anywhere as far as I can tell. > Maybe I can get to adding such a test next week. I am OK with adding the test in a later bug or in a subsequent path.
Assignee | ||
Comment 42•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #34) > Comment on attachment 9001498 [details] [diff] [review] > Update tests in test_animation-starttime.html > > Review of attachment 9001498 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/animation/test/css-transitions/test_animation-starttime.html > @@ +8,4 @@ > > > > .animated-div { > > margin-left: 100px; > > + transition: margin-left 100s linear 100s; > > It would be nice to set transition property in each test, especially in the > case where transition has positive delay like this case. Fixed. > @@ +79,3 @@ > > > > + animation.startTime = timelineTime - 200 * MS_PER_SEC; > > + await eventWatcher.wait_for('transitionend'); > > It seems to me that this test hasn't originally work as expected, since both > events will be fired sooner or later. We should check each event happens > within the next reqeustAnimationCallbacl? Would you mind filing a bug for > this? Yeah, I think this test was relying on 100s being shorter than the timeout for tests. That might be enough but it's probably not good to rely on that. Filed bug 1484148 for fixing this.
Assignee | ||
Comment 43•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #41) > (In reply to Brian Birtles (:birtles) from comment #40) > > > assert_greater_than(animation.effect.getComputedTiming().progress, > > > previousProgress, > > > 'Iteration progress increases after calling play()'); > > > > Resuming an animation is not really covered anywhere as far as I can tell. > > Maybe I can get to adding such a test next week. > > I am OK with adding the test in a later bug or in a subsequent path. Thanks, I added that to bug 1484148. Can I take that as r+ on this patch for now?
Assignee | ||
Comment 44•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #33) > Comment on attachment 9001502 [details] [diff] [review] > Update tests in test_setting-effect.html > > Review of attachment 9001502 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/animation/test/css-transitions/test_setting-effect.html > @@ +76,4 @@ > > assert_equals(getComputedStyle(div).left, '100px'); > > > > await watcher.wait_for('transitionend'); > > +}, 'After setting a transition\'s effect to null, transitionend is still' > > It seems to me that this event test case is covered by the last test in > test_event-displath.html. Indeed. Dropped!
Assignee | ||
Comment 45•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6438608acbbc778c15520335b4e3eefe71bbb691 This should be ready to land once I get the final r+. I'm not going to re-upload the whole series up to Bugzilla though.
Comment 46•6 years ago
|
||
Comment on attachment 9001496 [details] [diff] [review] Drop test_animation-pausing.html Review of attachment 9001496 [details] [diff] [review]: ----------------------------------------------------------------- Ship it!
Attachment #9001496 -
Flags: review?(hikezoe) → review+
Comment 47•6 years ago
|
||
Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ae57ad9f15 Use arrow functions in dom/animation/test/css-transitions/; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/8d053037352a Use async/await in dom/animation/test/css-transitions/; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/5097e03c335b Replace var with const/let in dom/animation/test/css-transitions/; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/9427f6cff88f Fix the spelling of canceled/canceling in dom/animation/test/css-transitions/; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/fbdd599db449 Update the titles of each test file; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/14200486590f Drop some unnecessary <body> elements from test files; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/242d77984071 Fix some minor formatting in test_animation-computed-timing.html; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/c5722de174bc Update tests in test_animation-currenttime.html; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/b023fa315b32 Update tests in test_animation-finished.html; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/a611b59253c5 Drop test_animation-pausing.html; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a1c2383121 Update tests in test_animation-ready.html; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/2fcae0203857 Update tests in test_animation-starttime.html; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/ffce5e4f4d45 Tidy up test in test_csstransition-transitionproperty.html; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/45a715e4cf0d Update tests in test_animation-element-get-animations.html; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/0160e2c379e3 Update tests in test_keyframeeffect-getkeyframes.html; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/64abf6978f00 Update tests in test_setting-effect.html; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8c402641ac Drop use of flushComputedStyle in css-transitions tests; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c3ab5c291b Move tests from dom/animations/tests/css-transitions to WPT; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/37038c5ebcb4 Add spec links to test files; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/0394a44b3955 Update event-dispatch test now that we can use the updated EventWatcher; r=hiro https://hg.mozilla.org/integration/mozilla-inbound/rev/877cdf25b483 Update corresponding tests for CSS animations to make them consistent; r=hiro
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12558 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Comment 50•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a8ae57ad9f15 https://hg.mozilla.org/mozilla-central/rev/8d053037352a https://hg.mozilla.org/mozilla-central/rev/5097e03c335b https://hg.mozilla.org/mozilla-central/rev/9427f6cff88f https://hg.mozilla.org/mozilla-central/rev/fbdd599db449 https://hg.mozilla.org/mozilla-central/rev/14200486590f https://hg.mozilla.org/mozilla-central/rev/242d77984071 https://hg.mozilla.org/mozilla-central/rev/c5722de174bc https://hg.mozilla.org/mozilla-central/rev/b023fa315b32 https://hg.mozilla.org/mozilla-central/rev/a611b59253c5 https://hg.mozilla.org/mozilla-central/rev/a4a1c2383121 https://hg.mozilla.org/mozilla-central/rev/2fcae0203857 https://hg.mozilla.org/mozilla-central/rev/ffce5e4f4d45 https://hg.mozilla.org/mozilla-central/rev/45a715e4cf0d https://hg.mozilla.org/mozilla-central/rev/0160e2c379e3 https://hg.mozilla.org/mozilla-central/rev/64abf6978f00 https://hg.mozilla.org/mozilla-central/rev/5b8c402641ac https://hg.mozilla.org/mozilla-central/rev/d0c3ab5c291b https://hg.mozilla.org/mozilla-central/rev/37038c5ebcb4 https://hg.mozilla.org/mozilla-central/rev/0394a44b3955 https://hg.mozilla.org/mozilla-central/rev/877cdf25b483
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Upstream PR merged
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•