[css-transforms] Bunch of bugs in the interpolation algorithms

[Manishearth]

Found by @canaltinova and me while implementing in Servo

• In 2d recomposition, the "Translate matrix" step should just be matrix[3][0] = translate[0] and matrix[3][1] = translate[0] I think. Otherwise the decomposed+recomposed matrix at t=0 for skew+translate is off. This is what webkit does
• In both interpolation steps, (3D, and 2D) matrix = multiply(matrix, rotationMatrix) should be multiplied the other way around (eg as in Gecko's code). Otherwise rotate+translate is off.
• Imterpolation for 3d uses min and max with the wrong numbers; this will unconditionally set the product to -1 and transform the thing to the point at infinity or some other invisible location.
• The algorithm needs rotate axes to be normalized, but the spec doesn't explicitly say this. We should probably specify that the computed value be normalized or at least mention it with the primitives
• Recomposing a 3d matrix should mention that the returned matrix is initialized to an identity
• Interpolation for 3d uses dot as a variable, probably means product

cc @dirkschulze

[Manishearth]

Not yet sure, but the skew multiplication might be off too. (edit: added "translate matrix" point)

It could be that a different convention is being used, but that should be specified, preferably with an algorithm explicitly listed for multiplication.

[Manishearth]

There's also this bug with the 3d decomposition of 2d matrices being flipped. It occurs in both firefox and chrome, so it might be a spec bug.

Safari gets it right, and the webkit implementation seems to closely follow the spec, so this probably isn't a spec bug.

[Manishearth]

cc @grorg

[cabanier]

There are different ways to decompose a matrix.
The algorithm in the spec should match what Safari has implemented. @dbaron ran a lot of tests and everyone preferred the way WebKit did the decomposition so the spec is reflecting that algorithm.

[Manishearth]

As I mentioned in the bugzilla bug, there may be different ways to decompose, but recompose(decomposed(T)) should be an identity operation (modulo floating point). The first three points are of this form -- they lead to a jump in the animation when it starts or finishes, which is wrong.

[cabanier]

In 2d recomposition, the "Translate matrix" step should just be matrix[3][0] = translate[0] and matrix[3][1] = translate[0] I think. Otherwise the decomposed+recomposed matrix at t=0 for skew+translate is off. This is what webkit does

Are you saying that WebKit does it wrong? Because the spec is reflecting that code...

[Manishearth]

No, I'm saying that the spec makes mistakes in reflecting that code.

It's possible that some of these are only "bugs" because of a differing premultiply/postmultiply/row-major/column-major convention. But I think Servo is following the same convention. So I'm not sure.

[Manishearth]

Oh, you're saying that webkit has the same code here. Perhaps it's a matter of differing convention then?

Hm. In Servo's implementation we had a jump occurring at that stage which could be removed by fixing that multiplication (and at first glance it seemed like the webkit implementation didn't match the spec, but it seems that I'm wrong here)

[cabanier]

Oh, you're saying that webkit has the same code here. Perhaps it's a matter of differing convention then?

Yes, m[0][0] -> m11
Can you find an example with the pseudocode where there's a jump?

[Manishearth]

No, not that convention 😄, I mean the matrix direction convention.

https://github.com/servo/servo/blob/73b296350927bad6d526cce21434ce68a75216fa/components/style/properties/helpers/animated_properties.mako.rs#L985 is the servo implementation. When the "translate matrix" portion matched the spec, certain animations for skew+translate would jump.

[cabanier]

When the "translate matrix" portion matched the spec, certain animations for skew+translate would jump.

Ah.
Well, looking at the servo code https://github.com/servo/servo/blob/73b296350927bad6d526cce21434ce68a75216fa/components/style/properties/helpers/animated_properties.mako.rs#L921 the decompose code is different from the spec so you can't use it to recompose because the ordering is different.

[Manishearth]

The decompose code seems to match the spec exactly? What part of it doesn't?

[cabanier]

You're correct! Sorry, I was looking at different code.

[Manishearth]

AFAICT servo is using the same convention as webkit, column-major.

The spec itself probably should be clear that matrix[i][j] means .m(i+1)(j+1), not .m(j+1)(i+1) -- it explicitly shows the layout of .m12-type indices but not how it is meant to be indexed. But this doesn't seem to be the source of the skew bug. Hm.

[Manishearth]

Interestingly, Safari messes up when the transition is interrupted and restarted. With this testcase, start the animation by moving your mouse into the yellow area, and then move it in and out rapidly. In Firefox the animation reverses smoothly, but in Safari the transformed red box jumps around.

(Servo was having similar issues, so this is probably a Safari/spec issue since Servo tries to implement the spec to the word. But unrelated to the ones listed in the bug above)