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

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: testcase

Both orange boxes should bounce (the first horizontally, the second vertically).  The regression is that the second one doesn't move.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: Don't skip the rest of the properties in an animation after hitting one that we shouldn't apply

Both sets of new tests pass with the patch, but without the patch the
"top is animating" test fails.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca8e40b6a5a5

Comment 6

[Daniel Holbert [: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

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d052486d57c3

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking regression. We should be able to get this fix into 37 Beta 4.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d052486d57c3

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4b50714e7419

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f2a6cf5957f

Comment 14

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.

Comment 15

[Vasilica Tamas, QA [:vtamas]]

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.

Comment 16

[Vasilica Tamas, QA [:vtamas]]

Sorry, I made a mistake: I wanted to say that I reproduced this issue on Firefox 37 Beta 1 (20150223185154).