Closed Bug 1140134 Opened 5 years ago Closed 5 years ago

property in a CSS animation being overridden by later animation causes later properties in that animation to be skipped

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla39
Tracking Status
firefox36 --- affected
firefox37 + verified
firefox38 + verified
firefox39 + verified

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: regression)

Attachments

(2 files)

This is a regression from bug 1078122 that I found by code inspection while working on bug 1125455.

When a property in a CSS animation is overridden by later animation, it causes later properties in that animation to be skipped, because the code uses return rather than continue.

We just shipped this in Firefox 36.

[Tracking Requested - why for this release]:
I think this is a pretty basic regression.  It's probably not triggered a huge amount, but I'd be surprised if nothing hits it.
I confirmed with Linux nightly builds that it matches the one-day regression window I expected from code inspection:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=33c0181c4a25&tochange=29fbfc1b31aa
Attached file testcase
Both orange boxes should bounce (the first horizontally, the second vertically).  The regression is that the second one doesn't move.
The fix is (I presume, not tested yet):
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/0d82194623ba/dont-skip-in-animation
but I still need to write an automated test.
Both sets of new tests pass with the patch, but without the patch the
"top is animating" test fails.
Attachment #8573563 - Flags: review?(dholbert)
Comment on attachment 8573563 [details] [diff] [review]
Don't skip the rest of the properties in an animation after hitting one that we shouldn't apply

Looks good! r=me
Attachment #8573563 - Flags: review?(dholbert) → review+
Comment on attachment 8573563 [details] [diff] [review]
Don't skip the rest of the properties in an animation after hitting one that we shouldn't apply

Approval Request Comment
[Feature/regressing bug #]: bug 1078122
[User impact if declined]: Parts of CSS animations that should apply don't (because when one property in an animation is overridden by a later one in the animation-name list, we skip the rest of the properties for that animation)
[Describe test coverage new/current, TreeHerder]: Added an automated test that covers it.  (I think we had tests for overriding, but not for the other properties still working when one was overridden.)
[Risks and why]: Very low risk; it fixes a return statement to be a continue statement as it should have been.
[String/UUID change made/needed]: None.
Attachment #8573563 - Flags: approval-mozilla-beta?
Attachment #8573563 - Flags: approval-mozilla-aurora?
Tracking regression. We should be able to get this fix into 37 Beta 4.
https://hg.mozilla.org/mozilla-central/rev/d052486d57c3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8573563 [details] [diff] [review]
Don't skip the rest of the properties in an animation after hitting one that we shouldn't apply

Beta+ Aurora+
Attachment #8573563 - Flags: approval-mozilla-beta?
Attachment #8573563 - Flags: approval-mozilla-beta+
Attachment #8573563 - Flags: approval-mozilla-aurora?
Attachment #8573563 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
This was verified as fixed on Firefox 37 Beta 4 on Windows 8.1 x64, Mac OS X 10.8.5, and Ubuntu 12.04 x64. Additional exploratory testing was also done for animations (https://etherpad.mozilla.org/Fx37b4) and did not reveal any new issues.
I was able to reproduce this issue on Firefox 37 Beta 4 (20150223185154) using Windows 7 64bit.

Verified fixed on Firefox 39.0a1 (2015-03-12), Firefox 38.0a2 (2015-03-12) and Firefox 38.0a2 (2015-03-13) using Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Sorry, I made a mistake: I wanted to say that I reproduced this issue on Firefox 37 Beta 1 (20150223185154).
You need to log in before you can comment on or make changes to this bug.