Closed Bug 1424798 Opened 2 years ago Closed 2 years ago

stylo: Cannot animation from translate(0%) to translate(0%, 100px) in Firefox 58 onwards

Categories

(Core :: CSS Parsing and Computation, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 + verified
firefox59 --- verified

People

(Reporter: birtles, Assigned: manishearth)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached file Test case
See the attached test case which attempts to transition the transform property from 'translate(0%)' to 'translate(0% 100px)'.

In Firefox 58 onwards I see no transition unless I turn off stylo. In Firefox 57.0.2, Edge, and Chrome there is a transition.

[Tracking Requested - why for this release]: Web facing regression causing some animations not to run.
Yeah, I'm pretty sure that's the regressor.
Regression window:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6bc39f03ea571fb109ae603b0c8f69778ec14384&tochange=92ff0c88e94d176ff9615abe61322413b7f38e55

Regressed by: 92ff0c88e94d	Manish Goregaokar — servo: Merge #18750 - Make transforms generic (from Manishearth:transform-generic); r=emilio,xidorn


Ah shit, No BMO bug number :( unable to block this....
(In reply to Alice0775 White from comment #3)
> Ah ****, No BMO bug number :( unable to block this....

Alice, we'll take that as Servo upstream fix. That's why no bug attached to it. Thanks for the help. :)
Yeah, but mostly the change is for bug 1391145.
Blocks: 1391145
I'll push a commit with tests later.

I feel like I originally had code that did this but had to remove it for *some* reason; but now I don't recall what. Let's see what try comes up with.
Flags: needinfo?(manishearth)
Assignee: nobody → manishearth
Comment on attachment 8936367 [details]
Bug 1424798 - stylo: Correctly handle interpolation where optional second argument for translate(), skew(), scale() exists in one but not the other;

https://reviewboard.mozilla.org/r/207086/#review212976

r=me, but needs tests.
Attachment #8936367 - Flags: review?(emilio) → review+
tracking as web-facing regression in 58.
P1 because this is a web-facing regression in 58.
Priority: -- → P1
Comment on attachment 8937621 [details]
Bug 1424798 - stylo: Add reftest;

https://reviewboard.mozilla.org/r/208296/#review214026

Overall looks good to me.  Two things I am concerned.

1) This test case uses a Web Animation API, pause(), we need to set pref, dom.animations-api.core.enabled, explicitly.
2) Though I am not 100% sure, but we should use reftest-wait here.

Or, you can make the animation CSS animation and use 'paused' state and negative delay. layout/reftests/css-animations/content-on-pseudo-element-at-half.html would be a reference.

::: layout/reftests/bugs/1424798-1.html:4
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<title>Animation between 1-argument and 2-argument transforms should work</title>
> +<style>
> +

Nit: drop a blank line.

::: layout/reftests/bugs/1424798-1.html:11
(Diff revision 1)
> +
> +

And here.

::: layout/reftests/bugs/1424798-1.html:22
(Diff revision 1)
> +
> +<script>
> +var anim = document.getElementById('box').animate(
> +  [
> +    { transform: 'translate(0%)' },
> +    { transform: 'translate(0%, 100px)' }   

Nit: trailing spaces here.

::: layout/reftests/bugs/1424798-1.html:25
(Diff revision 1)
> +  [
> +    { transform: 'translate(0%)' },
> +    { transform: 'translate(0%, 100px)' }   
> +  ], {
> +    duration: 8000,
> +    fill: 'both'

Nit: I don't think fill is necessary for this test case.
Attachment #8937621 - Flags: review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Comment on attachment 8937621 [details]
> Bug 1424798 - stylo: Add reftest;
> 
> https://reviewboard.mozilla.org/r/208296/#review214026
> 
> Overall looks good to me.  Two things I am concerned.
> 
> 1) This test case uses a Web Animation API, pause(), we need to set pref,
> dom.animations-api.core.enabled, explicitly.

You don't need to set that pref for pause(). It's part of the subset that ships by default with Element.animate().
(In reply to Brian Birtles (:birtles) from comment #16)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> > Comment on attachment 8937621 [details]
> > Bug 1424798 - stylo: Add reftest;
> > 
> > https://reviewboard.mozilla.org/r/208296/#review214026
> > 
> > Overall looks good to me.  Two things I am concerned.
> > 
> > 1) This test case uses a Web Animation API, pause(), we need to set pref,
> > dom.animations-api.core.enabled, explicitly.
> 
> You don't need to set that pref for pause(). It's part of the subset that
> ships by default with Element.animate().

Oh, I had been misunderstanding the pref. I thought it's just for Element.animate()!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> Oh, I had been misunderstanding the pref. I thought it's just for
> Element.animate()!

No, it includes most of the Animation interface. Only the members guarded by nsDocument::IsWebAnimationsEnabled in Animation.webidl are not shipped.
Should this reftest have been landed to wpt instead?
https://hg.mozilla.org/mozilla-central/rev/2607c50a6985
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(In reply to Emilio Cobos Álvarez [:emilio] from comment #21)
> Should this reftest have been landed to wpt instead?

You could easily add a test to the set of transform-list tests here:

https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js#804

For CSS animations there is also:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-transforms/animation/translate-interpolation.html
Please request uplift to beta when you get a chance.
Flags: needinfo?(manishearth)
Comment on attachment 8936367 [details]
Bug 1424798 - stylo: Correctly handle interpolation where optional second argument for translate(), skew(), scale() exists in one but not the other;

Approval Request Comment
[Feature/Bug causing the regression]: https://github.com/servo/servo/pull/18750
[User impact if declined]: Some CSS animations will be broken
[Is this code covered by automated tests?]: Yes, see next patch
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: https://bug1424798.bmoattachments.org/attachment.cgi?id=8936314 should animate
[List of other uplifts needed for the feature/fix]: The other patch on this bug (automated test)
[Is the change risky?]: No
[Why is the change risky/not risky?]: Minor code fix
[String changes made/needed]: N/A
Flags: needinfo?(manishearth)
Attachment #8936367 - Flags: approval-mozilla-beta?
Comment on attachment 8936367 [details]
Bug 1424798 - stylo: Correctly handle interpolation where optional second argument for translate(), skew(), scale() exists in one but not the other;

Fix a stylo regression. Beta58+.
Attachment #8936367 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have reproduced this issue using Firefox  59.0a1 (2017.12.11) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 58.0b14 and 59.0a1 (2018.01.05) on Win 8.1 x64,  Ubuntu 14.04 x64 and Mac OS X 10.13.2
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.