1405881 - stylo: -moz-transform animations with percentages are broken

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Manish Goregaokar [:manishearth]]

Attachment: testcase 1 (hover & un-hover the yellow div, and watch for animation)

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.

Comment 1

[Manish Goregaokar [:manishearth]]

This will be fixed in https://github.com/servo/servo/pull/18750

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Manish Goregaokar [:manishearth]]

This should be fixed by https://github.com/servo/servo/pull/18750 (not in m-c yet)

Comment 4

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/mozilla-central/rev/92ff0c88e94d

Should we land the test case as well?

Comment 5

[Manish Goregaokar [:manishearth]]

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.

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1405881 - Fix animation of -moz-transform matrices with percents ;

Review commit: https://reviewboard.mozilla.org/r/196428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196428/

Comment 7

[Manish Goregaokar [:manishearth]]

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?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8925184 [details]
Bug 1405881 - Fix animation of -moz-transform matrices with percents ;

https://reviewboard.mozilla.org/r/196428/#review201666

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Anyway, thanks for fixing this hard to notice bug!

Comment 11

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1405881 - Add crashtest;

Review commit: https://reviewboard.mozilla.org/r/196760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196760/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 13

[Manish Goregaokar [:manishearth]]

Comment on attachment 8925636 [details]
Bug 1405881 - Add crashtest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196760/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9152f015b963
https://hg.mozilla.org/mozilla-central/rev/271acbc4bd78