stylo: -moz-transform animations with percentages are broken

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

Trunk
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(3 attachments)

animating between -moz-transform: matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 10%, 10%, 0, 1) and -moz-transform: matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 40%, 40%, 0, 1) doesn't work because we never actually handle MatrixWithPercents in https://github.com/servo/servo/blob/master/components/style/properties/gecko.mako.rs#L3085

(and instead it gets treated as a regular matrix, where we directly read the value as a number, giving us animation between subpixel app unit values)

This will be fixed in my transform rewrite.
I'm assuming this would be too risky for uplift for 57, even if it were ready in time. (correct me if I'm mistaken) --> wontfix for that release.
Attachment #8915373 - Attachment mime type: text/plain → text/html
Attachment #8915373 - Attachment description: testcase.html → testcase 1 (hover & un-hover the yellow div, and watch for animation)
This should be fixed by https://github.com/servo/servo/pull/18750 (not in m-c yet)
https://hg.mozilla.org/mozilla-central/rev/92ff0c88e94d

Should we land the test case as well?
Flags: in-testsuite?
Target Milestone: --- → mozilla58
I went to add the testcase. Got a crash. Thought it was my fault from the refactoring; but nope; firefox has been broken wrt -moz-transform animations forever; it doesn't handle percentages in nsStyleDisplayList's AddTransformFunctions.

I'm going to fix that.
I can add a crashtest for this; but I'm not sure how to add a reftest. I can run an animation from start to finish and expect the finish to be correct, however CSS transitions and animations both reset the value to the specified start value (instead of the result of interpolation) or to the specified finish value (if you use `forwards`). We want to test that the result of the interpolation is correct; not that the parsed start/finish values work correctly; since the whole issue is that the interpolation doesn't match the start/finish values,

Perhaps a web animation is what we should be using. Thoughts on how to write this test?
Flags: needinfo?(hikezoe)
Comment on attachment 8925184 [details]
Bug 1405881 - Fix animation of -moz-transform matrices with percents ;

https://reviewboard.mozilla.org/r/196428/#review201666
Attachment #8925184 - Flags: review?(hikezoe) → review+
(In reply to Manish Goregaokar [:manishearth] from comment #7)
> I can add a crashtest for this; but I'm not sure how to add a reftest. I can
> run an animation from start to finish and expect the finish to be correct,
> however CSS transitions and animations both reset the value to the specified
> start value (instead of the result of interpolation) or to the specified
> finish value (if you use `forwards`). We want to test that the result of the
> interpolation is correct; not that the parsed start/finish values work
> correctly; since the whole issue is that the interpolation doesn't match the
> start/finish values,
> 
> Perhaps a web animation is what we should be using. Thoughts on how to write
> this test?

Yeah, I think your are right.  We can get results interpolation at a given time to set currentTime value [1].  But as for this bug, the problem existed in display list, so we can't test it with web animations.  Perhaps we can test the interpolation value with test refresh mode (layout/style/test/test_animations.html is run with the test mode), but I think it won't work either.  So I think adding the crashtest is fine.

[1] https://w3c.github.io/web-animations/#dom-animation-currenttime
Flags: needinfo?(hikezoe)
Anyway, thanks for fixing this hard to notice bug!
Comment on attachment 8925636 [details]
Bug 1405881 - Add crashtest;

https://reviewboard.mozilla.org/r/196760/#review202006

::: layout/painting/crashtests/1405881-1.html:22
(Diff revision 1)
> +            -moz-transform: matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 40%, 40%, 0, 1);
> +        }
> +    }
> +
> +</style>
> +<div id=container> 

Nit: remove a trailing space.
Attachment #8925636 - Flags: review?(hikezoe) → review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9152f015b963
Fix animation of -moz-transform matrices with percents ; r=hiro
https://hg.mozilla.org/integration/autoland/rev/271acbc4bd78
Add crashtest; r=hiro
https://hg.mozilla.org/mozilla-central/rev/9152f015b963
https://hg.mozilla.org/mozilla-central/rev/271acbc4bd78
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.